Merge pull request #4156 from systeminit/jhelwig/bug-460-orphan-child…

…-should-detach-components-from-all-frames

Allow Frame::orphan_child to remove all incoming FrameContains edges

lib/dal/src/component/frame.rs

@@ -7,7 +7,9 @@ use thiserror::Error;
 use crate::attribute::value::AttributeValueError;
 use crate::diagram::SummaryDiagramInferredEdge;
 use crate::socket::input::InputSocketError;
-use crate::workspace_snapshot::edge_weight::{EdgeWeightError, EdgeWeightKind};
+use crate::workspace_snapshot::edge_weight::{
+    EdgeWeightError, EdgeWeightKind, EdgeWeightKindDiscriminants,
+};
 use crate::workspace_snapshot::WorkspaceSnapshotError;
 use crate::{
     Component, ComponentError, ComponentId, ComponentType, DalContext, TransactionsError, WsEvent,
@@ -54,9 +56,48 @@ impl Frame {
     /// Provides an ability to remove the existing ['Component']'s parent``
     #[instrument(level = "info", skip_all)]
     pub async fn orphan_child(ctx: &DalContext, child_id: ComponentId) -> FrameResult<()> {
-        if let Some(parent_id) = Component::get_parent_by_id(ctx, child_id).await? {
+        // Normally, we'd call `Component::get_parent_by_id` to get the parent's ID, but that
+        // returns a hard error if there are multiple parents. Since we want to be able to use this
+        // as an escape hatch for if we get into the situation of a Component having multiple
+        // parents, we can't use that, and have to do the same thing it would have done to get
+        // _all_ of the parents to truly orphan the child.
+        let parent_idxs = ctx
+            .workspace_snapshot()?
+            .incoming_sources_for_edge_weight_kind(
+                child_id,
+                EdgeWeightKindDiscriminants::FrameContains,
+            )
+            .await?;
+        if parent_idxs.len() == 1 {
+            // We just determined that there is exactly one element in the vec, so if Vec::first()
+            // returns anything other than `Some` we can't trust anything.
+            let parent_idx = parent_idxs
+                .first()
+                .expect("Unable to get the first element of a Vec of len 1");
+            let parent_id = ctx
+                .workspace_snapshot()?
+                .get_node_weight(*parent_idx)
+                .await?
+                .id()
+                .into();
+
             Self::detach_child_from_parent_inner(ctx, parent_id, child_id).await?;
+        } else {
+            // When there are multiple parents, we're trying to recover from a broken state, and we
+            // can't reliably detect everything necessary to do a DependentValuesUpdate, or most of
+            // the other things we'd normally do. This means there won't be any WsEvents for
+            // removal of the inferred edges.
+            for parent_idx in parent_idxs {
+                let parent_id = ctx
+                    .workspace_snapshot()?
+                    .get_node_weight(parent_idx)
+                    .await?
+                    .id()
+                    .into();
+                Component::remove_edge_from_frame(ctx, parent_id, child_id).await?;
+            }
         }
+
         Ok(())
     }
 

lib/dal/tests/integration_test/frame.rs

@@ -1,7 +1,9 @@
 use dal::component::frame::{Frame, FrameError};
 use dal::diagram::SummaryDiagramInferredEdge;
 use dal::diagram::{Diagram, DiagramResult, SummaryDiagramComponent, SummaryDiagramEdge};
-use dal::{AttributeValue, Component, DalContext, Schema, SchemaVariant};
+use dal::{
+    AttributeValue, Component, ComponentError, DalContext, EdgeWeightKind, Schema, SchemaVariant,
+};
 use dal::{ComponentType, InputSocket, OutputSocket};
 use dal_test::helpers::{
     connect_components_with_socket_names, create_component_for_default_schema_name,
@@ -2239,6 +2241,78 @@ async fn multiple_frames_with_complex_connections_no_nesting(ctx: &mut DalContex
     }
 }
 
+/// A Component/Frame is not _supposed_ to have multiple parents, but if we somehow end up with
+/// multiple, we want to be able to remove them all to correct the situation.
+#[test]
+async fn orphan_frames_multiple_parents(ctx: &mut DalContext) {
+    // create a large up frame
+    let parent_a = create_component_for_schema_name_with_type(
+        ctx,
+        "large even lego",
+        "parent A",
+        ComponentType::ConfigurationFrameDown,
+    )
+    .await
+    .expect("created frame");
+    // put another medium frame inside
+    let child = create_component_for_schema_name_with_type(
+        ctx,
+        "medium even lego",
+        "child",
+        ComponentType::ConfigurationFrameDown,
+    )
+    .await
+    .expect("could not create component");
+    // Create another "parent" frame
+    let parent_b = create_component_for_schema_name_with_type(
+        ctx,
+        "large even lego",
+        "parent B",
+        ComponentType::ConfigurationFrameDown,
+    )
+    .await
+    .expect("created frame");
+
+    // Insert the child into "parent A"
+    Frame::upsert_parent(ctx, child.id(), parent_a.id())
+        .await
+        .expect("could not upsert parent");
+    // We have to manually add this connection from "parent B" to "child", as our "normal"
+    // interface for putting "child" inside of "parent B" would (correctly) remove the association
+    // between "parent A" and "child".
+    Component::add_edge_to_frame(
+        ctx,
+        parent_b.id(),
+        child.id(),
+        EdgeWeightKind::FrameContains,
+    )
+    .await
+    .expect("could not add second parent");
+
+    // Normally we'd commit here & run Dependent Values Update, but DVU will always blow up as
+    // we've created a Component with multiple parents.
+
+    assert!(matches!(
+        Component::get_parent_by_id(ctx, child.id()).await,
+        Err(ComponentError::MultipleParentsForComponent(x)) if x == child.id()
+    ));
+
+    Frame::orphan_child(ctx, child.id())
+        .await
+        .expect("could not orphan component");
+
+    ChangeSetTestHelpers::commit_and_update_snapshot_to_visibility(ctx)
+        .await
+        .expect("could not commit and update snapshot to visibility");
+
+    assert_eq!(
+        None,
+        Component::get_parent_by_id(ctx, child.id())
+            .await
+            .expect("Unable to get component's parent"),
+    );
+}
+
 struct DiagramByKey {
     pub components: HashMap<String, (SummaryDiagramComponent, Vec<SummaryDiagramInferredEdge>)>,
     pub edges: HashMap<String, SummaryDiagramEdge>,