From 483090260dd2346ebb0590add0643c1d49a47579 Mon Sep 17 00:00:00 2001 From: Jacob Helwig Date: Wed, 5 Jun 2024 11:55:25 -0700 Subject: [PATCH 1/8] Include Components only in the base change set that would be actively removed by a change set when building the summary diagram We now look at the series of updates that would be applied if we were to merge a change set into its base, and include "virtual" summary diagram component entries for any Components that would be removed and that only exist in the base change set. These virtual entries will have `from_base_change_set` set to true to signal that they don't actually exist in the snapshot for the change set. --- lib/dal/src/diagram.rs | 64 +++++++++++++++- lib/dal/src/workspace_snapshot.rs | 90 +++++++++++++++++++++++ lib/dal/tests/integration_test/diagram.rs | 53 +++++++++++++ lib/dal/tests/integration_test/mod.rs | 1 + 4 files changed, 204 insertions(+), 4 deletions(-) create mode 100644 lib/dal/tests/integration_test/diagram.rs diff --git a/lib/dal/src/diagram.rs b/lib/dal/src/diagram.rs index 7bd8b46f17..196f0c73af 100644 --- a/lib/dal/src/diagram.rs +++ b/lib/dal/src/diagram.rs @@ -20,8 +20,9 @@ use crate::socket::input::InputSocketError; use crate::socket::output::OutputSocketError; use crate::workspace_snapshot::WorkspaceSnapshotError; use crate::{ - AttributePrototypeId, Component, ComponentId, DalContext, HistoryEventError, InputSocketId, - OutputSocketId, SchemaId, SchemaVariant, SchemaVariantId, SocketArity, StandardModelError, + AttributePrototypeId, ChangeSetError, Component, ComponentId, DalContext, HistoryEventError, + InputSocketId, OutputSocketId, SchemaId, SchemaVariant, SchemaVariantId, SocketArity, + StandardModelError, TransactionsError, Workspace, WorkspaceError, }; #[remain::sorted] @@ -35,6 +36,8 @@ pub enum DiagramError { AttributeValue(#[from] AttributeValueError), #[error("attribute value not found")] AttributeValueNotFound, + #[error("Change Set error: {0}")] + ChangeSet(#[from] ChangeSetError), #[error("component error: {0}")] Component(#[from] ComponentError), #[error("component not found")] @@ -77,8 +80,12 @@ pub enum DiagramError { SocketNotFound, #[error("standard model error: {0}")] StandardModel(#[from] StandardModelError), + #[error("Transactions error: {0}")] + Transactions(#[from] TransactionsError), #[error("could not acquire lock: {0}")] TryLock(#[from] tokio::sync::TryLockError), + #[error("Workspace error: {0}")] + Workspace(#[from] WorkspaceError), #[error("workspace snapshot error: {0}")] WorkspaceSnapshot(#[from] WorkspaceSnapshotError), } @@ -437,8 +444,57 @@ impl Diagram { ); } - // We also want to display the edges & components that would be actively removed when we - // merge this change set into its base change set. + // Even though the default change set for a workspace can have a base change set, we don't + // want to consider anything as new/modified/removed when looking at the default change + // set. + let workspace = Workspace::get_by_pk_or_error( + ctx, + &ctx.tenancy() + .workspace_pk() + .ok_or(WorkspaceSnapshotError::WorkspaceMissing)?, + ) + .await?; + if workspace.default_change_set_id() != ctx.change_set_id() + && ctx.change_set()?.base_change_set_id.is_some() + { + // We also want to display the edges & components that would be actively removed when we + // merge this change set into its base change set. + let removed_component_ids = ctx + .workspace_snapshot()? + .components_removed_relative_to_base(ctx) + .await?; + + // We now need to retrieve these Components from the base change set, and build + // SummaryDiagramComponents for them with from_base_change_set true, and change_status + // ChangeStatus::Removed + let base_snapshot_ctx = ctx.clone_with_base().await?; + let base_snapshot_ctx = &base_snapshot_ctx; + + for removed_component_id in removed_component_ids { + // We don't need to worry about these duplicating SummaryDiagramComponents that + // have already been generated as they're generated from the list of Components + // still in the change set's snapshot, while the list of Component IDs we're + // working on here are explicitly ones that do not exist in that snapshot anymore. + let base_change_set_component = + Component::get_by_id(base_snapshot_ctx, removed_component_id).await?; + let mut summary_diagram_component = SummaryDiagramComponent::assemble( + base_snapshot_ctx, + &base_change_set_component, + ChangeStatus::Deleted, + ) + .await?; + summary_diagram_component.from_base_change_set = true; + + component_views.push(summary_diagram_component); + } + + // We need to bring in any AttributePrototypeArguments for incoming & outgoing connections. + + // let removed_attribute_prototype_aguments = ctx + // .workspace_snapshot()? + // .socket_edges_removed_relative_to_base(ctx) + // .await?; + } Ok(Self { edges: diagram_edges, diff --git a/lib/dal/src/workspace_snapshot.rs b/lib/dal/src/workspace_snapshot.rs index 6bdd270168..74c4f4fb1d 100644 --- a/lib/dal/src/workspace_snapshot.rs +++ b/lib/dal/src/workspace_snapshot.rs @@ -1438,6 +1438,96 @@ impl WorkspaceSnapshot { Ok(new_component_ids) } + #[instrument( + name = "workspace_snapshot.components_removed_relative_to_base", + level = "debug", + skip_all + )] + pub async fn components_removed_relative_to_base( + &self, + ctx: &DalContext, + ) -> WorkspaceSnapshotResult> { + let mut removed_component_ids = Vec::new(); + let base_change_set_id = if let Some(change_set_id) = ctx.change_set()?.base_change_set_id { + change_set_id + } else { + return Ok(removed_component_ids); + }; + + // Even though the default change set for a workspace can have a base change set, we don't + // want to consider anything as new/modified/removed when looking at the default change + // set. + let workspace = Workspace::get_by_pk_or_error( + ctx, + &ctx.tenancy() + .workspace_pk() + .ok_or(WorkspaceSnapshotError::WorkspaceMissing)?, + ) + .await + .map_err(Box::new)?; + if workspace.default_change_set_id() == ctx.change_set_id() { + return Ok(removed_component_ids); + } + + let base_snapshot = WorkspaceSnapshot::find_for_change_set(ctx, base_change_set_id).await?; + // We need to use the index of the Category node in the base snapshot, as that's the + // `to_rebase` when detecting conflicts & updates later, and the source node index in the + // Update is from the `to_rebase` graph. + let component_category_idx = if let Some((_category_id, category_idx)) = base_snapshot + .read_only_graph + .get_category_node(None, CategoryNodeKind::Component)? + { + category_idx + } else { + // Can't have any new Components if there's no Component category node to put them + // under. + return Ok(removed_component_ids); + }; + let conflicts_and_updates = base_snapshot.read_only_graph.detect_conflicts_and_updates( + VectorClockId::from(Ulid::from(base_change_set_id)), + &self.read_only_graph, + VectorClockId::from(Ulid::from(ctx.change_set_id())), + )?; + + for update in &conflicts_and_updates.updates { + match update { + Update::ReplaceSubgraph { + onto: _, + to_rebase: _, + } + | Update::MergeCategoryNodes { + to_rebase_category_id: _, + onto_category_id: _, + } + | Update::NewEdge { + source: _, + destination: _, + edge_weight: _, + } => { + /* Updates unused for determining if a Component is removed with regards to the updates */ + } + Update::RemoveEdge { + source, + destination, + edge_kind: _, + } => { + if !(source.index == component_category_idx + && destination.node_weight_kind == NodeWeightDiscriminants::Component) + { + // We are only interested in updates that remove the edge from the + // Components category to the Component itself, as this means we would + // be deleting that Component. + continue; + } + + removed_component_ids.push(ComponentId::from(Ulid::from(destination.id))); + } + } + } + + Ok(removed_component_ids) + } + #[instrument( name = "workspace_snapshot.socket_edges_added_relative_to_base", level = "debug", diff --git a/lib/dal/tests/integration_test/diagram.rs b/lib/dal/tests/integration_test/diagram.rs new file mode 100644 index 0000000000..70f094ad46 --- /dev/null +++ b/lib/dal/tests/integration_test/diagram.rs @@ -0,0 +1,53 @@ +use dal::{change_status::ChangeStatus, diagram::Diagram, Component, DalContext}; +use dal_test::{ + helpers::{create_component_for_schema_name, ChangeSetTestHelpers}, + test, +}; + +#[test] +async fn components_removed_from_snapshot_have_virtual_diagram_entries(ctx: &mut DalContext) { + let component_to_remove = + create_component_for_schema_name(ctx, "Docker Image", "Removed in sub-change set").await; + let _component_still_in_change_set = + create_component_for_schema_name(ctx, "Docker Image", "Still here").await; + ChangeSetTestHelpers::commit_and_update_snapshot_to_visibility(ctx) + .await + .expect("Unable to commit"); + ChangeSetTestHelpers::apply_change_set_to_base(ctx) + .await + .expect("Unable to merge setup to HEAD"); + + ChangeSetTestHelpers::fork_from_head_change_set(ctx) + .await + .expect("Unable to create forked change set"); + + let removed_component = Component::get_by_id(ctx, component_to_remove.id()) + .await + .expect("Unable to get component to remove"); + removed_component + .delete(ctx) + .await + .expect("Unable to remove component"); + ChangeSetTestHelpers::commit_and_update_snapshot_to_visibility(ctx) + .await + .expect("Unable to commit"); + + assert!(Component::get_by_id(ctx, component_to_remove.id()) + .await + .is_err()); + + let summary_diagram = Diagram::assemble(ctx) + .await + .expect("Unable to assemble summary diagram"); + + let removed_component_summary = summary_diagram + .components + .iter() + .find(|comp| comp.id == component_to_remove.id()) + .expect("Removed Component not found in summary diagram"); + assert!(removed_component_summary.from_base_change_set); + assert_eq!( + ChangeStatus::Deleted, + removed_component_summary.change_status + ); +} diff --git a/lib/dal/tests/integration_test/mod.rs b/lib/dal/tests/integration_test/mod.rs index ad38407d62..4f330bcb56 100644 --- a/lib/dal/tests/integration_test/mod.rs +++ b/lib/dal/tests/integration_test/mod.rs @@ -6,6 +6,7 @@ mod component; mod connection; mod cycle_check_guard; mod dependent_values_update; +mod diagram; mod frame; mod func; mod input_sources; From ee6ede908c61b528b94015b2f1cbae05bf3fbdc9 Mon Sep 17 00:00:00 2001 From: John Obelenus Date: Thu, 6 Jun 2024 10:59:01 -0400 Subject: [PATCH 2/8] Adding support for fromBaseChangeSet to the front end --- app/web/src/api/sdf/dal/component.ts | 1 + app/web/src/components/ComponentCard.vue | 1 + app/web/src/components/ModelingDiagram/DiagramGroup.vue | 5 ++++- app/web/src/components/ModelingDiagram/DiagramNode.vue | 5 ++++- app/web/src/components/ModelingDiagram/diagram_types.ts | 2 ++ 5 files changed, 12 insertions(+), 2 deletions(-) diff --git a/app/web/src/api/sdf/dal/component.ts b/app/web/src/api/sdf/dal/component.ts index 49f058ee99..689548ed38 100644 --- a/app/web/src/api/sdf/dal/component.ts +++ b/app/web/src/api/sdf/dal/component.ts @@ -52,6 +52,7 @@ export interface RawComponent { updatedInfo: ActorAndTimestamp; toDelete: boolean; canBeUpgraded: boolean; + fromBaseChangeSet: boolean; } export type EdgeId = string; diff --git a/app/web/src/components/ComponentCard.vue b/app/web/src/components/ComponentCard.vue index 9dce0fdf53..b50d10b8d7 100644 --- a/app/web/src/components/ComponentCard.vue +++ b/app/web/src/components/ComponentCard.vue @@ -6,6 +6,7 @@ 'p-xs border-l-4 border relative', titleCard ? 'mb-xs' : 'rounded-md', component.toDelete && 'opacity-70', + component.fromBaseChangeSet && 'opacity-70', ) " :style="{ diff --git a/app/web/src/components/ModelingDiagram/DiagramGroup.vue b/app/web/src/components/ModelingDiagram/DiagramGroup.vue index e74caecb0a..a262d55355 100644 --- a/app/web/src/components/ModelingDiagram/DiagramGroup.vue +++ b/app/web/src/components/ModelingDiagram/DiagramGroup.vue @@ -437,7 +437,10 @@ const size = computed( ); const isDeleted = computed( - () => props.group.def.changeStatus === "deleted" || props.group.def.toDelete, + () => + props.group.def.changeStatus === "deleted" || + props.group.def.toDelete || + props.group.def.fromBaseChangeSet, ); const isModified = computed(() => props.group.def.changeStatus === "modified"); const isAdded = computed(() => props.group.def.changeStatus === "added"); diff --git a/app/web/src/components/ModelingDiagram/DiagramNode.vue b/app/web/src/components/ModelingDiagram/DiagramNode.vue index 7ad137331e..714b87cf93 100644 --- a/app/web/src/components/ModelingDiagram/DiagramNode.vue +++ b/app/web/src/components/ModelingDiagram/DiagramNode.vue @@ -320,7 +320,10 @@ const diagramContext = useDiagramContext(); const { edgeDisplayMode } = diagramContext; const isDeleted = computed( - () => props.node.def.changeStatus === "deleted" || props.node.def.toDelete, + () => + props.node.def.changeStatus === "deleted" || + props.node.def.toDelete || + props.node.def.fromBaseChangeSet, ); const isModified = computed(() => props.node.def.changeStatus === "modified"); const isAdded = computed(() => props.node.def.changeStatus === "added"); diff --git a/app/web/src/components/ModelingDiagram/diagram_types.ts b/app/web/src/components/ModelingDiagram/diagram_types.ts index 66e8ac51cc..d836bb3282 100644 --- a/app/web/src/components/ModelingDiagram/diagram_types.ts +++ b/app/web/src/components/ModelingDiagram/diagram_types.ts @@ -213,6 +213,8 @@ export type DiagramNodeDef = { changeStatus?: ChangeStatus; /** component will be deleted after next action run */ toDelete: boolean; + /** component is deleted in this changeset, but exists in base change set */ + fromBaseChangeSet: boolean; /** can the component be upgraded */ canBeUpgraded: boolean; }; From 85b48a813cdbeae48a42b9e43ed6317f8b38eb03 Mon Sep 17 00:00:00 2001 From: Jacob Helwig Date: Tue, 11 Jun 2024 14:46:11 -0700 Subject: [PATCH 3/8] List edges from base change set that would be actively removed on merge --- lib/dal/src/diagram.rs | 87 +++++++++++++++-- lib/dal/src/workspace_snapshot.rs | 151 +++++++++++++++++++++++++++++- 2 files changed, 226 insertions(+), 12 deletions(-) diff --git a/lib/dal/src/diagram.rs b/lib/dal/src/diagram.rs index 196f0c73af..1336ebdde4 100644 --- a/lib/dal/src/diagram.rs +++ b/lib/dal/src/diagram.rs @@ -8,7 +8,7 @@ use thiserror::Error; use crate::actor_view::ActorView; use crate::attribute::prototype::argument::{ - AttributePrototypeArgumentError, AttributePrototypeArgumentId, + AttributePrototypeArgument, AttributePrototypeArgumentError, AttributePrototypeArgumentId, }; use crate::attribute::value::AttributeValueError; use crate::change_status::ChangeStatus; @@ -385,6 +385,8 @@ impl Diagram { let mut diagram_edges: Vec = vec![]; let mut diagram_inferred_edges: Vec = vec![]; let components = Component::list(ctx).await?; + let mut virtual_and_real_components_by_id: HashMap = + components.iter().cloned().map(|c| (c.id(), c)).collect(); let mut component_views = Vec::with_capacity(components.len()); let new_component_ids: HashSet = ctx .workspace_snapshot()? @@ -467,8 +469,8 @@ impl Diagram { // We now need to retrieve these Components from the base change set, and build // SummaryDiagramComponents for them with from_base_change_set true, and change_status // ChangeStatus::Removed - let base_snapshot_ctx = ctx.clone_with_base().await?; - let base_snapshot_ctx = &base_snapshot_ctx; + let base_change_set_ctx = ctx.clone_with_base().await?; + let base_change_set_ctx = &base_change_set_ctx; for removed_component_id in removed_component_ids { // We don't need to worry about these duplicating SummaryDiagramComponents that @@ -476,24 +478,89 @@ impl Diagram { // still in the change set's snapshot, while the list of Component IDs we're // working on here are explicitly ones that do not exist in that snapshot anymore. let base_change_set_component = - Component::get_by_id(base_snapshot_ctx, removed_component_id).await?; + Component::get_by_id(base_change_set_ctx, removed_component_id).await?; let mut summary_diagram_component = SummaryDiagramComponent::assemble( - base_snapshot_ctx, + base_change_set_ctx, &base_change_set_component, ChangeStatus::Deleted, ) .await?; summary_diagram_component.from_base_change_set = true; + virtual_and_real_components_by_id + .insert(removed_component_id, base_change_set_component); component_views.push(summary_diagram_component); } - // We need to bring in any AttributePrototypeArguments for incoming & outgoing connections. + // We need to bring in any AttributePrototypeArguments for incoming & outgoing + // connections that have been removed. + let removed_attribute_prototype_argument_ids = ctx + .workspace_snapshot()? + .socket_edges_removed_relative_to_base(ctx) + .await?; + let mut incoming_connections_by_component_id: HashMap< + ComponentId, + Vec, + > = HashMap::new(); + + for removed_attribute_prototype_argument_id in &removed_attribute_prototype_argument_ids + { + let attribute_prototype_argument = AttributePrototypeArgument::get_by_id( + base_change_set_ctx, + *removed_attribute_prototype_argument_id, + ) + .await?; - // let removed_attribute_prototype_aguments = ctx - // .workspace_snapshot()? - // .socket_edges_removed_relative_to_base(ctx) - // .await?; + // This should always be Some as + // `WorkspaceSnapshot::socket_edges_removed_relative_to_base` only returns the + // IDs of arguments that have targets. + if let Some(targets) = attribute_prototype_argument.targets() { + if let hash_map::Entry::Vacant(vacant_entry) = + incoming_connections_by_component_id.entry(targets.destination_component_id) + { + let removed_component = Component::get_by_id( + base_change_set_ctx, + targets.destination_component_id, + ) + .await?; + let mut incoming_connections = removed_component + .incoming_connections(base_change_set_ctx) + .await?; + // We only care about connections going to the Component in the base that + // are *ALSO* ones that we are considered to have removed. + incoming_connections.retain(|connection| { + removed_attribute_prototype_argument_ids + .contains(&connection.attribute_prototype_argument_id) + }); + vacant_entry.insert(incoming_connections); + } + } + } + + for incoming_connections in incoming_connections_by_component_id.values() { + for incoming_connection in incoming_connections { + let from_component = virtual_and_real_components_by_id + .get(&incoming_connection.from_component_id) + .cloned() + .ok_or(ComponentError::NotFound( + incoming_connection.from_component_id, + ))?; + let to_component = virtual_and_real_components_by_id + .get(&incoming_connection.to_component_id) + .ok_or(ComponentError::NotFound( + incoming_connection.to_component_id, + ))?; + let mut summary_diagram_edge = SummaryDiagramEdge::assemble( + incoming_connection.clone(), + from_component, + to_component, + ChangeStatus::Deleted, + )?; + summary_diagram_edge.from_base_change_set = true; + + diagram_edges.push(summary_diagram_edge); + } + } } Ok(Self { diff --git a/lib/dal/src/workspace_snapshot.rs b/lib/dal/src/workspace_snapshot.rs index 74c4f4fb1d..2287f5924f 100644 --- a/lib/dal/src/workspace_snapshot.rs +++ b/lib/dal/src/workspace_snapshot.rs @@ -61,7 +61,7 @@ use crate::workspace_snapshot::node_weight::category_node_weight::CategoryNodeKi use crate::workspace_snapshot::node_weight::NodeWeight; use crate::workspace_snapshot::update::Update; use crate::workspace_snapshot::vector_clock::VectorClockId; -use crate::{pk, ComponentId, Workspace, WorkspaceError}; +use crate::{pk, Component, ComponentError, ComponentId, Workspace, WorkspaceError}; use crate::{ workspace_snapshot::{graph::WorkspaceSnapshotGraphError, node_weight::NodeWeightError}, DalContext, TransactionsError, WorkspaceSnapshotGraph, @@ -91,6 +91,8 @@ pub enum WorkspaceSnapshotError { ChangeSet(#[from] ChangeSetError), #[error("change set {0} has no workspace snapshot address")] ChangeSetMissingWorkspaceSnapshotAddress(ChangeSetId), + #[error("Component error: {0}")] + Component(#[from] Box), #[error("edge weight error: {0}")] EdgeWeight(#[from] EdgeWeightError), #[error("join error: {0}")] @@ -1400,7 +1402,7 @@ impl WorkspaceSnapshot { VectorClockId::from(Ulid::from(ctx.change_set_id())), )?; - for update in &conflicts_and_updates.updates { + for update in conflicts_and_updates.updates { match update { Update::RemoveEdge { source: _, @@ -1618,6 +1620,151 @@ impl WorkspaceSnapshot { Ok(new_attribute_prototype_argument_ids) } + #[instrument( + name = "workspace_snapshot.socket_edges_removed_relative_to_base", + level = "debug", + skip_all + )] + pub async fn socket_edges_removed_relative_to_base( + &self, + ctx: &DalContext, + ) -> WorkspaceSnapshotResult> { + let mut removed_attribute_prototype_argument_ids = Vec::new(); + let base_change_set_id = if let Some(change_set_id) = ctx.change_set()?.base_change_set_id { + change_set_id + } else { + return Ok(removed_attribute_prototype_argument_ids); + }; + + // Even though the default change set for a workspace can have a base change set, we don't + // want to consider anything as new/modified/removed when looking at the default change + // set. + let workspace = Workspace::get_by_pk_or_error( + ctx, + &ctx.tenancy() + .workspace_pk() + .ok_or(WorkspaceSnapshotError::WorkspaceMissing)?, + ) + .await + .map_err(Box::new)?; + if workspace.default_change_set_id() == ctx.change_set_id() { + return Ok(removed_attribute_prototype_argument_ids); + } + + let base_change_set_ctx = ctx.clone_with_base().await?; + let base_change_set_ctx = &base_change_set_ctx; + + // * For each Component being removed (all edges to/from removed components should also + // show as removed): + let removed_component_ids: HashSet = self + .components_removed_relative_to_base(ctx) + .await? + .iter() + .copied() + .collect(); + let remaining_component_ids: HashSet = Component::list(ctx) + .await + .map_err(Box::new)? + .iter() + .map(Component::id) + .collect(); + for removed_component_id in &removed_component_ids { + let base_change_set_component = + Component::get_by_id(base_change_set_ctx, *removed_component_id) + .await + .map_err(Box::new)?; + + // * Get incoming edges + for incoming_connection in base_change_set_component + .incoming_connections(base_change_set_ctx) + .await + .map_err(Box::new)? + { + //* Interested in: + // * Edge is coming from a Component being removed + // * Edge is coming from a Component that exists in current change set + if removed_component_ids.contains(&incoming_connection.from_component_id) + || remaining_component_ids.contains(&incoming_connection.from_component_id) + { + removed_attribute_prototype_argument_ids + .push(incoming_connection.attribute_prototype_argument_id); + } + } + + //* Get outgoing edges + for outgoing_connection in base_change_set_component + .outgoing_connections(base_change_set_ctx) + .await + .map_err(Box::new)? + { + // * Interested in: + // * Edge is going to a Component being removed + // * Edge is going to a Component that exists in current change set + if removed_component_ids.contains(&outgoing_connection.to_component_id) + || remaining_component_ids.contains(&outgoing_connection.to_component_id) + { + removed_attribute_prototype_argument_ids + .push(outgoing_connection.attribute_prototype_argument_id); + } + } + } + + // * For each removed AttributePrototypeArgument (removed edge connects two Components + // that have not been removed): + let conflicts_and_updates = base_change_set_ctx + .workspace_snapshot()? + .read_only_graph + .detect_conflicts_and_updates( + VectorClockId::from(Ulid::from(base_change_set_id)), + &self.read_only_graph, + VectorClockId::from(Ulid::from(ctx.change_set_id())), + )?; + + for update in conflicts_and_updates.updates { + match update { + Update::ReplaceSubgraph { + onto: _, + to_rebase: _, + } + | Update::NewEdge { + source: _, + destination: _, + edge_weight: _, + } + | Update::MergeCategoryNodes { + to_rebase_category_id: _, + onto_category_id: _, + } => { + /* Updates unused for determining if a connection between sockets has been removed */ + } + Update::RemoveEdge { + source: _, + destination, + edge_kind, + } => { + if edge_kind != EdgeWeightKindDiscriminants::PrototypeArgument { + continue; + } + let attribute_prototype_argument = AttributePrototypeArgument::get_by_id( + base_change_set_ctx, + AttributePrototypeArgumentId::from(Ulid::from(destination.id)), + ) + .await + .map_err(Box::new)?; + + // * Interested in all of them that have targets (connecting two Components + // via sockets). + if attribute_prototype_argument.targets().is_some() { + removed_attribute_prototype_argument_ids + .push(attribute_prototype_argument.id()); + } + } + } + } + + Ok(removed_attribute_prototype_argument_ids) + } + /// Returns whether or not any Actions were dispatched. pub async fn dispatch_actions(ctx: &DalContext) -> WorkspaceSnapshotResult { let mut did_dispatch = false; From 76f737a761a6b2dd41c91534e11806e039e91e7e Mon Sep 17 00:00:00 2001 From: John Obelenus Date: Tue, 11 Jun 2024 17:58:05 -0400 Subject: [PATCH 4/8] FE changes to re-use the remove_delete_intent endpoint to support fromBaseChangeSet flags --- .../ModelingView/RestoreSelectionModal.vue | 9 ++++++--- app/web/src/store/components.store.ts | 13 +++++++++---- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/app/web/src/components/ModelingView/RestoreSelectionModal.vue b/app/web/src/components/ModelingView/RestoreSelectionModal.vue index 0b214a7ec4..d81c0e4081 100644 --- a/app/web/src/components/ModelingView/RestoreSelectionModal.vue +++ b/app/web/src/components/ModelingView/RestoreSelectionModal.vue @@ -32,6 +32,7 @@ import { import { computed, onBeforeUnmount, onMounted, ref } from "vue"; import { useComponentsStore } from "@/store/components.store"; +import { ComponentId } from "@/api/sdf/dal/component"; const componentsStore = useComponentsStore(); const selectedEdge = computed(() => componentsStore.selectedEdge); @@ -78,9 +79,11 @@ function open() { async function onConfirmRestore() { if (componentsStore.selectedComponentIds) { - await componentsStore.RESTORE_COMPONENTS( - componentsStore.selectedComponentIds, - ); + const data = {} as Record; + componentsStore.selectedComponentIds.forEach((cId) => { + data[cId] = !!componentsStore.componentsById[cId]?.fromBaseChangeSet; + }); + await componentsStore.RESTORE_COMPONENTS(data); } componentsStore.setSelectedComponentId(null); close(); diff --git a/app/web/src/store/components.store.ts b/app/web/src/store/components.store.ts index ffc70313af..6a675234ef 100644 --- a/app/web/src/store/components.store.ts +++ b/app/web/src/store/components.store.ts @@ -1499,28 +1499,33 @@ export const useComponentsStore = (forceChangeSetId?: ChangeSetId) => { }); }, - async RESTORE_COMPONENTS(componentIds: ComponentId[]) { + async RESTORE_COMPONENTS(components: Record) { if (changeSetsStore.creatingChangeSet) throw new Error("race, wait until the change set is created"); if (changeSetId === changeSetsStore.headChangeSetId) changeSetsStore.creatingChangeSet = true; + const payload = []; + for (const [key, value] of Object.entries(components)) { + payload.push({ componentId: key, fromBaseChangeSet: value }); + } return new ApiRequest({ method: "post", url: "diagram/remove_delete_intent", - keyRequestStatusBy: componentIds, + keyRequestStatusBy: Object.keys(components), params: { - componentIds, + components: payload, ...visibilityParams, }, onSuccess: () => { - for (const componentId of componentIds) { + for (const componentId of Object.keys(components)) { const component = this.rawComponentsById[componentId]; if (component) { this.rawComponentsById[componentId] = { ...component, changeStatus: "unmodified", toDelete: false, + fromBaseChangeSet: false, deletedInfo: undefined, }; } From b47e4697d7c2da8aa6a5677de16ed6370b98a541 Mon Sep 17 00:00:00 2001 From: Jacob Helwig Date: Wed, 12 Jun 2024 13:36:19 -0700 Subject: [PATCH 5/8] Allow importing a Component from another snapshot graph This allows us to "undelete"/"restore" a Component from the base change set if we've removed it from the current change set, and change our mind. --- lib/dal/src/component.rs | 30 ++++ lib/dal/src/workspace_snapshot.rs | 22 +-- lib/dal/src/workspace_snapshot/graph.rs | 167 +++++++++++++++++- .../service/diagram/remove_delete_intent.rs | 58 +++++- 4 files changed, 263 insertions(+), 14 deletions(-) diff --git a/lib/dal/src/component.rs b/lib/dal/src/component.rs index 1d57b4f0e7..43fa0f9af3 100644 --- a/lib/dal/src/component.rs +++ b/lib/dal/src/component.rs @@ -1237,6 +1237,7 @@ impl Component { Ok(incoming_edges) } + pub async fn get_children_for_id( ctx: &DalContext, component_id: ComponentId, @@ -1261,6 +1262,7 @@ impl Component { Ok(children) } + #[instrument(level = "debug" skip(ctx))] pub async fn get_parent_by_id( ctx: &DalContext, @@ -1290,6 +1292,7 @@ impl Component { }; Ok(maybe_parent) } + pub async fn parent(&self, ctx: &DalContext) -> ComponentResult> { Self::get_parent_by_id(ctx, self.id).await } @@ -1587,6 +1590,7 @@ impl Component { None => None, }) } + #[instrument(level="debug" skip(ctx))] pub async fn get_type_by_id( ctx: &DalContext, @@ -1773,6 +1777,7 @@ impl Component { Ok(socket_values) } + #[instrument(level="debug" skip_all)] pub async fn input_socket_match( ctx: &DalContext, @@ -1783,6 +1788,7 @@ impl Component { Self::input_socket_attribute_values_for_component_id(ctx, component_id).await?; Ok(all_input_sockets.get(&input_socket_id).cloned()) } + pub async fn output_socket_match( ctx: &DalContext, component_id: ComponentId, @@ -1832,6 +1838,7 @@ impl Component { ) -> ComponentResult> { Self::input_socket_attribute_values_for_component_id(ctx, self.id()).await } + #[instrument(level = "info", skip(ctx))] async fn connect_inner( ctx: &DalContext, @@ -1931,6 +1938,7 @@ impl Component { attribute_prototype_argument.id(), ))) } + pub async fn remove_edge_from_frame( ctx: &DalContext, parent_id: ComponentId, @@ -2101,6 +2109,7 @@ impl Component { Ok(result) } + /// Checks the destination and source component to determine if data flow between them /// Both "deleted" and not deleted Components can feed data into /// "deleted" Components. **ONLY** not deleted Components can feed @@ -2122,6 +2131,7 @@ impl Component { }, ) } + /// Simply gets the to_delete status for a component via the Node Weight async fn is_set_to_delete( ctx: &DalContext, @@ -2143,6 +2153,7 @@ impl Component { None => Ok(None), } } + async fn modify(self, ctx: &DalContext, lambda: L) -> ComponentResult where L: FnOnce(&mut Self) -> ComponentResult<()>, @@ -2575,6 +2586,7 @@ impl Component { pasted_comp.clone_attributes_from(ctx, self.id()).await?; Ok(pasted_comp) } + /// For a given [`ComponentId`], map each input socket to the inferred output sockets /// it is connected to. Inferred socket connections are determined by following /// the ancestry line of FrameContains edges and matching the relevant input to output sockets. @@ -3558,6 +3570,7 @@ impl Component { format!("{} - Copy", name) } } + /// This method finds the [`AttributeValueId`](crate::AttributeValue) corresponding to either "/root/code" or /// "/root/qualification" for the given [`ComponentId`](Component) and ['LeafKind'](LeafKind). pub async fn find_map_attribute_value_for_leaf_kind( @@ -3575,6 +3588,23 @@ impl Component { }; Ok(attribute_value_id) } + + pub async fn restore_from_base_change_set( + ctx: &DalContext, + component_id: ComponentId, + ) -> ComponentResult<()> { + let base_change_set_ctx = ctx.clone_with_base().await?; + + ctx.workspace_snapshot()? + .import_component_subgraph( + ctx.change_set()?.vector_clock_id(), + &*base_change_set_ctx.workspace_snapshot()?, + component_id, + ) + .await?; + + Ok(()) + } } #[derive(Clone, Deserialize, Serialize, Debug, PartialEq, Eq)] diff --git a/lib/dal/src/workspace_snapshot.rs b/lib/dal/src/workspace_snapshot.rs index 2287f5924f..c933ea7846 100644 --- a/lib/dal/src/workspace_snapshot.rs +++ b/lib/dal/src/workspace_snapshot.rs @@ -606,20 +606,22 @@ impl WorkspaceSnapshot { } #[instrument( - name = "workspace_snapshot.import_subgraph", + name = "workspace_snapshot.import_component_subgraph", level = "debug", - skip_all, - fields() + skip_all )] - pub async fn import_subgraph( + pub async fn import_component_subgraph( &self, - other: &mut Self, - root_index: NodeIndex, + vector_clock_id: VectorClockId, + other: &Self, + component_id: ComponentId, ) -> WorkspaceSnapshotResult<()> { - Ok(self - .working_copy_mut() - .await - .import_subgraph(&*other.working_copy().await, root_index)?) + let component_node_index = other.read_only_graph.get_node_index_by_id(component_id)?; + Ok(self.working_copy_mut().await.import_component_subgraph( + vector_clock_id, + &*other.read_only_graph, + component_node_index, + )?) } /// Calls [`WorkspaceSnapshotGraph::replace_references()`] diff --git a/lib/dal/src/workspace_snapshot/graph.rs b/lib/dal/src/workspace_snapshot/graph.rs index c772f723b5..998c21e474 100644 --- a/lib/dal/src/workspace_snapshot/graph.rs +++ b/lib/dal/src/workspace_snapshot/graph.rs @@ -1,4 +1,4 @@ -use std::collections::{HashMap, HashSet, VecDeque}; +use std::collections::{hash_map::Entry, HashMap, HashSet, VecDeque}; use std::fs::File; use std::io::Write; @@ -41,6 +41,8 @@ pub type LineageId = Ulid; pub enum WorkspaceSnapshotGraphError { #[error("Cannot compare ordering of container elements between ordered, and un-ordered container: {0:?}, {1:?}")] CannotCompareOrderedAndUnorderedContainers(NodeIndex, NodeIndex), + #[error("could not find category node of kind: {0:?}")] + CategoryNodeNotFound(CategoryNodeKind), #[error("ChangeSet error: {0}")] ChangeSet(#[from] ChangeSetError), #[error("Unable to retrieve content for ContentHash")] @@ -1686,6 +1688,169 @@ impl WorkspaceSnapshotGraph { Ok(()) } + pub fn import_component_subgraph( + &mut self, + vector_clock_id: VectorClockId, + other: &WorkspaceSnapshotGraph, + component_node_index: NodeIndex, + ) -> WorkspaceSnapshotGraphResult<()> { + // * DFS event-based traversal. + // * DfsEvent::Discover(attribute_prototype_argument_node_index, _): + // If APA has targets, skip & return Control::Prune, since we don't want to bring in + // Components other than the one specified. Only arguments linking Inout & Output + // Sockets will have targets (the source & destination ComponentIDs). + // * DfsEvent::Discover(func_node_index, _): + // Add edge from Funcs Category node to imported Func node. + let mut edges_by_tail = HashMap::new(); + petgraph::visit::depth_first_search(&other.graph, Some(component_node_index), |event| { + self.import_component_subgraph_process_dfs_event( + other, + &mut edges_by_tail, + vector_clock_id, + event, + ) + })?; + + Ok(()) + } + + /// This assumes that the SchemaVariant for the Component is already present in [`self`][Self]. + fn import_component_subgraph_process_dfs_event( + &mut self, + other: &WorkspaceSnapshotGraph, + edges_by_tail: &mut HashMap>, + vector_clock_id: VectorClockId, + event: DfsEvent, + ) -> WorkspaceSnapshotGraphResult> { + match event { + // We only check to see if we can prune graph traversal in the node discovery event. + // The "real" work is done in the node finished event. + DfsEvent::Discover(other_node_index, _) => { + let other_node_weight = other.get_node_weight(other_node_index)?; + + // AttributePrototypeArguments with targets connect Input & Output Sockets, and we + // don't want to import either the Component on the other end of the connection, or + // the connection itself. Unfortunately, we can't prune when looking at the + // relevant edge, as that would prune _all remaining edges_ outgoing from the + // AttributePrototype. + if NodeWeightDiscriminants::AttributePrototypeArgument == other_node_weight.into() { + let apa_node_weight = + other_node_weight.get_attribute_prototype_argument_node_weight()?; + if apa_node_weight.targets().is_some() { + return Ok(petgraph::visit::Control::Prune); + } + } + + // When we hit something that already exists, we're pretty much guaranteed to have + // left "the component" and have gone into already-existing Funcs or the Schema + // Variant. + if self + .find_equivalent_node(other_node_weight.id(), other_node_weight.lineage_id())? + .is_some() + { + return Ok(petgraph::visit::Control::Prune); + } + + Ok(petgraph::visit::Control::Continue) + } + // We wait to do the "real" import work in the Finish event as these happen in + // post-order, so we are guaranteed that all of the targets of the outgoing edges + // have been imported. + DfsEvent::Finish(other_node_index, _) => { + // See if we already have the node from other in self. + let other_node_weight = other.get_node_weight(other_node_index)?; + // Even though we prune when the equivalent node is_some() in the Discover event, + // we will still get a Finish event for the node that returned Control::Prune in + // its Discover event. + if self + .find_equivalent_node(other_node_weight.id(), other_node_weight.lineage_id())? + .is_none() + { + // AttributePrototypeArguments for cross-component connections will still have + // their DfsEvent::Finish fire, and won't already exist in self, but we do not + // want to import them. + if let NodeWeight::AttributePrototypeArgument( + attribute_prototype_argument_node_weight, + ) = other_node_weight + { + if attribute_prototype_argument_node_weight.targets().is_some() { + return Ok(petgraph::visit::Control::Prune); + } + } + + // Import the node. + self.add_node(other_node_weight.clone())?; + + // Create all edges with this node as the tail. + if let Entry::Occupied(edges) = edges_by_tail.entry(other_node_index) { + for (other_head_node_index, edge_weight) in edges.get() { + // Need to get this on every iteration as the node index changes as we + // add edges. + let self_node_index = + self.get_node_index_by_id(other_node_weight.id())?; + let other_head_node_weight = + other.get_node_weight(*other_head_node_index)?; + let self_head_node_index = + self.get_node_index_by_id(other_head_node_weight.id())?; + self.add_edge( + self_node_index, + edge_weight.clone(), + self_head_node_index, + )?; + } + } + + // Funcs and Components have incoming edges from their Category nodes that we + // want to exist, but won't be discovered by the graph traversal on its own. + let category_kind = if NodeWeightDiscriminants::Func == other_node_weight.into() + { + Some(CategoryNodeKind::Func) + } else if NodeWeightDiscriminants::Component == other_node_weight.into() { + Some(CategoryNodeKind::Component) + } else { + None + }; + + if let Some(category_node_kind) = category_kind { + let self_node_index = self.get_node_index_by_id(other_node_weight.id())?; + let (_, category_node_idx) = self + .get_category_node(None, category_node_kind)? + .ok_or(WorkspaceSnapshotGraphError::CategoryNodeNotFound( + CategoryNodeKind::Func, + ))?; + self.add_edge( + category_node_idx, + EdgeWeight::new(vector_clock_id, EdgeWeightKind::new_use())?, + self_node_index, + )?; + } + } + + Ok(petgraph::visit::Control::Continue) + } + DfsEvent::BackEdge(tail_node_index, head_node_index) + | DfsEvent::CrossForwardEdge(tail_node_index, head_node_index) + | DfsEvent::TreeEdge(tail_node_index, head_node_index) => { + // We'll keep track of the edges we encounter, instead of doing something with them + // right away as the common case (TreeEdge) is that we'll encounter the edge before + // we encounter the node for the head (target) of the edge. We can't actually do + // anything about importing the edge until we've also imported the head node. + for edgeref in other + .graph + .edges_connecting(tail_node_index, head_node_index) + { + edges_by_tail + .entry(tail_node_index) + .and_modify(|entry| { + entry.push((head_node_index, edgeref.weight().clone())); + }) + .or_insert_with(|| vec![(head_node_index, edgeref.weight().clone())]); + } + Ok(petgraph::visit::Control::Continue) + } + } + } + pub fn is_acyclic_directed(&self) -> bool { // Using this because "is_cyclic_directed" is recursive. algo::toposort(&self.graph, None).is_ok() diff --git a/lib/sdf-server/src/server/service/diagram/remove_delete_intent.rs b/lib/sdf-server/src/server/service/diagram/remove_delete_intent.rs index 126d23d47c..d964edbb5a 100644 --- a/lib/sdf-server/src/server/service/diagram/remove_delete_intent.rs +++ b/lib/sdf-server/src/server/service/diagram/remove_delete_intent.rs @@ -42,10 +42,43 @@ async fn remove_single_delete_intent( Ok(()) } +async fn restore_component_from_base_change_set( + ctx: &DalContext, + component_id: ComponentId, + original_uri: &Uri, + PosthogClient(posthog_client): &PosthogClient, +) -> DiagramResult<()> { + Component::restore_from_base_change_set(ctx, component_id).await?; + let comp = Component::get_by_id(ctx, component_id).await?; + let comp_schema = comp.schema(ctx).await?; + + track( + posthog_client, + ctx, + original_uri, + "restore_from_base_change_set", + serde_json::json!({ + "how": "/diagram/remove_delete_intent", + "component_id": component_id, + "component_schema_name": comp_schema.name(), + "change_set_id": ctx.change_set_id(), + }), + ); + + Ok(()) +} + +#[derive(Deserialize, Serialize, Debug, Copy, Clone)] +#[serde(rename_all = "camelCase")] +pub struct RemoveDeleteIntentComponentInformation { + pub component_id: ComponentId, + pub from_base_change_set: bool, +} + #[derive(Deserialize, Serialize, Debug)] #[serde(rename_all = "camelCase")] pub struct RemoveDeleteIntentRequest { - pub component_ids: Vec, + pub components: Vec, #[serde(flatten)] pub visibility: Visibility, } @@ -62,8 +95,27 @@ pub async fn remove_delete_intent( let force_change_set_id = ChangeSet::force_new(&mut ctx).await?; - for component_id in request.component_ids { - remove_single_delete_intent(&ctx, component_id, &original_uri, &posthog_client).await?; + for component_info in request.components { + match component_info.from_base_change_set { + true => { + restore_component_from_base_change_set( + &ctx, + component_info.component_id, + &original_uri, + &posthog_client, + ) + .await? + } + false => { + remove_single_delete_intent( + &ctx, + component_info.component_id, + &original_uri, + &posthog_client, + ) + .await? + } + } } ctx.commit().await?; From 2ce31f97bd2a8f5082ad975545e28a24e9ce1ebf Mon Sep 17 00:00:00 2001 From: Jacob Helwig Date: Wed, 26 Jun 2024 10:19:19 -0700 Subject: [PATCH 6/8] Restore Component->Component connections from base changeset whenever possible MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rather than creating a new API endpoint & method for attempting to restore a `Component -> Component` (`Output Socket -> Input Socket`) connection, `Component::connect` (and by extension `diagram/create_connection`) will now check the base change set to see if an equivalent connection exists there. If it does, then we import that pre-existing connection into the current change set. If it does not, then we'll create the connection as a brand new one. This means that if you remove a connection, decide you didn’t actually want to do that, and re-draw it instead of clicking the “restore connection” button, we’ll do the right thing and restore it, so you don’t end up seeing a remove+add of the “same” connection, and should generally help reduce the potential churn in looking at diffs. --- lib/dal/src/component.rs | 405 ++++++++++++++++++---- lib/dal/src/workspace_snapshot.rs | 2 +- lib/dal/tests/integration_test/diagram.rs | 8 +- 3 files changed, 343 insertions(+), 72 deletions(-) diff --git a/lib/dal/src/component.rs b/lib/dal/src/component.rs index 43fa0f9af3..971b058f40 100644 --- a/lib/dal/src/component.rs +++ b/lib/dal/src/component.rs @@ -185,6 +185,8 @@ pub enum ComponentError { WorkspaceSnapshot(#[from] WorkspaceSnapshotError), #[error("attribute value {0} has wrong type for operation: {0}")] WrongAttributeValueType(AttributeValueId, ValueIsFor), + #[error("Attribute Prototype Argument used by too many Attribute Prototypes: {0}")] + WrongNumberOfPrototypesForAttributePrototypeArgument(AttributePrototypeArgumentId), #[error("WsEvent error: {0}")] WsEvent(#[from] WsEventError), } @@ -1297,34 +1299,60 @@ impl Component { Self::get_parent_by_id(ctx, self.id).await } - async fn get_node_weight_and_content( + async fn try_get_node_weight_and_content( ctx: &DalContext, component_id: ComponentId, - ) -> ComponentResult<(ComponentNodeWeight, ComponentContentV1)> { - let (component_node_weight, hash) = - Self::get_node_weight_and_content_hash(ctx, component_id).await?; + ) -> ComponentResult> { + if let Some((component_node_weight, content_hash)) = + Self::try_get_node_weight_and_content_hash(ctx, component_id).await? + { + let content: ComponentContent = ctx + .layer_db() + .cas() + .try_read_as(&content_hash) + .await? + .ok_or(WorkspaceSnapshotError::MissingContentFromStore( + component_id.into(), + ))?; - let content: ComponentContent = ctx.layer_db().cas().try_read_as(&hash).await?.ok_or( - WorkspaceSnapshotError::MissingContentFromStore(component_id.into()), - )?; + let ComponentContent::V1(inner) = content; - let ComponentContent::V1(inner) = content; + return Ok(Some((component_node_weight, inner))); + } - Ok((component_node_weight, inner)) + Ok(None) } - async fn get_node_weight_and_content_hash( + async fn get_node_weight_and_content( ctx: &DalContext, component_id: ComponentId, - ) -> ComponentResult<(ComponentNodeWeight, ContentHash)> { - let workspace_snapshot = ctx.workspace_snapshot()?; + ) -> ComponentResult<(ComponentNodeWeight, ComponentContentV1)> { + Self::try_get_node_weight_and_content(ctx, component_id) + .await? + .ok_or(ComponentError::NotFound(component_id)) + } + + async fn try_get_node_weight_and_content_hash( + ctx: &DalContext, + component_id: ComponentId, + ) -> ComponentResult> { let id: Ulid = component_id.into(); - let node_index = workspace_snapshot.get_node_index_by_id(id).await?; - let node_weight = workspace_snapshot.get_node_weight(node_index).await?; + if let Some(node_index) = ctx + .workspace_snapshot()? + .try_get_node_index_by_id(id) + .await? + { + let node_weight = ctx + .workspace_snapshot()? + .get_node_weight(node_index) + .await?; + + let hash = node_weight.content_hash(); + let component_node_weight = node_weight.get_component_node_weight()?; + return Ok(Some((component_node_weight, hash))); + } - let hash = node_weight.content_hash(); - let component_node_weight = node_weight.get_component_node_weight()?; - Ok((component_node_weight, hash)) + Ok(None) } pub async fn list(ctx: &DalContext) -> ComponentResult> { @@ -1441,6 +1469,19 @@ impl Component { Ok(Self::assemble(&node_weight, content)) } + pub async fn try_get_by_id( + ctx: &DalContext, + component_id: ComponentId, + ) -> ComponentResult> { + if let Some((node_weight, content)) = + Self::try_get_node_weight_and_content(ctx, component_id).await? + { + return Ok(Some(Self::assemble(&node_weight, content))); + } + + Ok(None) + } + pub async fn set_geometry( &mut self, ctx: &DalContext, @@ -1839,18 +1880,66 @@ impl Component { Self::input_socket_attribute_values_for_component_id(ctx, self.id()).await } - #[instrument(level = "info", skip(ctx))] - async fn connect_inner( + #[instrument(level = "debug", skip(ctx))] + async fn create_new_connection( ctx: &DalContext, source_component_id: ComponentId, source_output_socket_id: OutputSocketId, destination_component_id: ComponentId, destination_input_socket_id: InputSocketId, - ) -> ComponentResult> { - let total_start = std::time::Instant::now(); - + destination_prototype_id: AttributePrototypeId, + ) -> ComponentResult { + debug!( + "Creating new Component connection: {:?}, {:?}, {:?}, {:?}", + source_component_id, + source_output_socket_id, + destination_component_id, + destination_input_socket_id + ); let cycle_check_guard = ctx.workspace_snapshot()?.enable_cycle_check().await; + let attribute_prototype_argument = AttributePrototypeArgument::new_inter_component( + ctx, + source_component_id, + source_output_socket_id, + destination_component_id, + destination_prototype_id, + ) + .await?; + + drop(cycle_check_guard); + debug!("Cycle Check Guard dropped"); + + Ok(attribute_prototype_argument.id()) + } + + pub async fn remove_edge_from_frame( + ctx: &DalContext, + parent_id: ComponentId, + child_id: ComponentId, + ) -> ComponentResult<()> { + ctx.workspace_snapshot()? + .remove_edge_for_ulids( + ctx.change_set()?, + parent_id, + child_id, + EdgeWeightKindDiscriminants::FrameContains, + ) + .await?; + + Ok(()) + } + + #[instrument(level = "info", skip(ctx))] + pub async fn connect( + ctx: &DalContext, + source_component_id: ComponentId, + source_output_socket_id: OutputSocketId, + destination_component_id: ComponentId, + destination_input_socket_id: InputSocketId, + ) -> ComponentResult> { + let total_start = std::time::Instant::now(); + // Already have this connection? Nothing to do. let destination_component = Component::get_by_id(ctx, destination_component_id).await?; for connection in destination_component.incoming_connections(ctx).await? { if connection.from_component_id == source_component_id @@ -1886,8 +1975,54 @@ impl Component { let destination_prototype_id = AttributeValue::prototype_id(ctx, destination_attribute_value_id).await?; - // check for socket arity on the input socket - // if the input socket has arity of one, and there's an existing edge, need to remove it before adding the new one + Self::connect_arity_cleanup( + ctx, + destination_component_id, + destination_input_socket_id, + destination_prototype_id, + ) + .await?; + + let attribute_prototype_argument_id = match Self::restore_connection_from_base_change_set( + ctx, + source_component_id, + source_output_socket_id, + destination_component_id, + destination_input_socket_id, + ) + .await? + { + Some(apa_id) => apa_id, + None => { + Self::create_new_connection( + ctx, + source_component_id, + source_output_socket_id, + destination_component_id, + destination_input_socket_id, + destination_prototype_id, + ) + .await? + } + }; + + ctx.add_dependent_values_and_enqueue(vec![destination_attribute_value_id]) + .await?; + + debug!("Component::connect took {:?}", total_start.elapsed()); + + Ok(Some(attribute_prototype_argument_id)) + } + + /// Check for socket arity on the input socket; if the input socket has arity of + /// one, and there's an existing edge, need to remove it before we can add a new one. + #[instrument(level = "debug", skip(ctx))] + async fn connect_arity_cleanup( + ctx: &DalContext, + destination_component_id: ComponentId, + destination_input_socket_id: InputSocketId, + destination_prototype_id: AttributePrototypeId, + ) -> ComponentResult<()> { let input_socket = InputSocket::get_by_id(ctx, destination_input_socket_id).await?; if input_socket.arity() == SocketArity::One { let existing_attribute_prototype_args = @@ -1918,68 +2053,188 @@ impl Component { } } - let attribute_prototype_argument = AttributePrototypeArgument::new_inter_component( - ctx, - source_component_id, - source_output_socket_id, - destination_component_id, - destination_prototype_id, - ) - .await?; - - drop(cycle_check_guard); - info!( - "Cycle Check Guard dropped, add edge took {:?}", - total_start.elapsed() - ); - - Ok(Some(( - destination_attribute_value_id, - attribute_prototype_argument.id(), - ))) - } - - pub async fn remove_edge_from_frame( - ctx: &DalContext, - parent_id: ComponentId, - child_id: ComponentId, - ) -> ComponentResult<()> { - ctx.workspace_snapshot()? - .remove_edge_for_ulids( - ctx.change_set()?, - parent_id, - child_id, - EdgeWeightKindDiscriminants::FrameContains, - ) - .await?; - Ok(()) } - pub async fn connect( + #[instrument(level = "debug", skip(ctx))] + async fn restore_connection_from_base_change_set( ctx: &DalContext, source_component_id: ComponentId, source_output_socket_id: OutputSocketId, destination_component_id: ComponentId, destination_input_socket_id: InputSocketId, ) -> ComponentResult> { - let maybe = Self::connect_inner( - ctx, + debug!( + "Restoring connection from base change set: {:?}, {:?}, {:?}, {:?}", + source_component_id, + source_output_socket_id, + destination_component_id, + destination_input_socket_id + ); + let base_change_set_ctx = ctx.clone_with_base().await?; + let base_change_set_ctx = &base_change_set_ctx; + let base_change_set_component = if let Some(component) = + Component::try_get_by_id(base_change_set_ctx, destination_component_id).await? + { + component + } else { + return Ok(None); + }; + let base_change_set_connection = match base_change_set_component + .incoming_connections(base_change_set_ctx) + .await? + .iter() + .find(|ic| { + ic.from_component_id == source_component_id + && ic.from_output_socket_id == source_output_socket_id + && ic.to_component_id == destination_component_id + && ic.to_input_socket_id == destination_input_socket_id + }) + .cloned() + { + Some(connection) => connection, + None => return Ok(None), + }; + debug!( + "Restoring connection from base change set: {:?}, {:?}, {:?}, {:?}, {:?}", source_component_id, source_output_socket_id, destination_component_id, destination_input_socket_id, + base_change_set_connection, + ); + + let base_attribute_prototype_argument_node_index = base_change_set_ctx + .workspace_snapshot()? + .get_node_index_by_id(base_change_set_connection.attribute_prototype_argument_id) + .await?; + let base_attribute_prototype_argument_node_weight = base_change_set_ctx + .workspace_snapshot()? + .get_node_weight(base_attribute_prototype_argument_node_index) + .await?; + let base_func_arg_id = AttributePrototypeArgument::func_argument_id_by_id( + base_change_set_ctx, + base_change_set_connection.attribute_prototype_argument_id, ) .await?; - if let Some((destination_attribute_value_id, attribute_prototype_argument_id)) = maybe { - ctx.add_dependent_values_and_enqueue(vec![destination_attribute_value_id]) - .await?; + // We want to recreate the `AttributePrototype -> AttributePrototypeArgument` edge as it + // exists in the base change set. + let mut base_prototype_argument_incoming_edges = base_change_set_ctx + .workspace_snapshot()? + .edges_directed( + base_change_set_connection.attribute_prototype_argument_id, + petgraph::Direction::Incoming, + ) + .await?; + base_prototype_argument_incoming_edges.retain( + |(edge_weight, _source_index, _destination_index)| { + EdgeWeightKindDiscriminants::PrototypeArgument == edge_weight.kind().into() + }, + ); + let (base_prototype_to_argument_edge_weight, prototype_id) = + if base_prototype_argument_incoming_edges.len() == 1 { + match base_prototype_argument_incoming_edges.first() { + Some((edge_weight, source_node_index, _destination_node_index)) => { + let prototype_weight = base_change_set_ctx + .workspace_snapshot()? + .get_node_weight(*source_node_index) + .await?; + (edge_weight, prototype_weight.id()) + } + None => { + // We just made sure that there was exactly one element in the Vec. + unreachable!("Unable to get first element of a one element Vec"); + } + } + } else { + return Err( + ComponentError::WrongNumberOfPrototypesForAttributePrototypeArgument( + base_change_set_connection.attribute_prototype_argument_id, + ), + ); + }; - Ok(Some(attribute_prototype_argument_id)) - } else { - Ok(None) - } + // We want to recreate the `AttributePrototypeArgument -> OutputSocket` edge + // (EdgeWeightKind::PrototypeArgumentValue). + let base_prototype_arg_to_output_socket_edges = base_change_set_ctx + .workspace_snapshot()? + .get_edges_between_nodes( + base_change_set_connection + .attribute_prototype_argument_id + .into(), + source_output_socket_id.into(), + ) + .await?; + let base_prototype_arg_to_output_socket_edge_weight = + match base_prototype_arg_to_output_socket_edges.first() { + Some(edge_weight) => edge_weight, + None => { + return Err(AttributePrototypeArgumentError::MissingSource( + base_change_set_connection.attribute_prototype_argument_id, + ) + .into()); + } + }; + + // We want to recreate the `AttributePrototypeArgument -> FuncArg` edge + // (EdgeWeightKind::Use). + let base_prototype_arg_to_func_arg_edges = base_change_set_ctx + .workspace_snapshot()? + .get_edges_between_nodes( + base_change_set_connection + .attribute_prototype_argument_id + .into(), + base_func_arg_id.into(), + ) + .await?; + let base_prototype_arg_to_func_arg_edge_weight = + match base_prototype_arg_to_func_arg_edges.first() { + Some(edge_weight) => edge_weight, + None => { + return Err(AttributePrototypeArgumentError::MissingFuncArgument( + base_change_set_connection.attribute_prototype_argument_id, + ) + .into()) + } + }; + + let cycle_check_guard = ctx.workspace_snapshot()?.enable_cycle_check().await; + + // Recreate the AttributePrototypeArgument & associated edges. + // We only need to import the AttributePrototypeArgument node, as all of the other relevant + // nodes should already exist. + ctx.workspace_snapshot()? + .add_node(base_attribute_prototype_argument_node_weight.clone()) + .await?; + ctx.workspace_snapshot()? + .add_edge( + prototype_id, + base_prototype_to_argument_edge_weight.clone(), + base_change_set_connection.attribute_prototype_argument_id, + ) + .await?; + ctx.workspace_snapshot()? + .add_edge( + base_change_set_connection.attribute_prototype_argument_id, + base_prototype_arg_to_func_arg_edge_weight.clone(), + base_func_arg_id, + ) + .await?; + ctx.workspace_snapshot()? + .add_edge( + base_change_set_connection.attribute_prototype_argument_id, + base_prototype_arg_to_output_socket_edge_weight.clone(), + source_output_socket_id, + ) + .await?; + + drop(cycle_check_guard); + debug!("Cycle Check Guard dropped"); + + Ok(Some( + base_attribute_prototype_argument_node_weight.id().into(), + )) } // Returns map of node id -> parent node ids @@ -3603,6 +3858,18 @@ impl Component { ) .await?; + let component = Component::get_by_id(ctx, component_id).await?; + + ctx.add_dependent_values_and_enqueue( + component + .input_socket_attribute_values(ctx) + .await? + .values() + .map(|ism| ism.attribute_value_id) + .collect(), + ) + .await?; + Ok(()) } } diff --git a/lib/dal/src/workspace_snapshot.rs b/lib/dal/src/workspace_snapshot.rs index c933ea7846..023030514c 100644 --- a/lib/dal/src/workspace_snapshot.rs +++ b/lib/dal/src/workspace_snapshot.rs @@ -619,7 +619,7 @@ impl WorkspaceSnapshot { let component_node_index = other.read_only_graph.get_node_index_by_id(component_id)?; Ok(self.working_copy_mut().await.import_component_subgraph( vector_clock_id, - &*other.read_only_graph, + &other.read_only_graph, component_node_index, )?) } diff --git a/lib/dal/tests/integration_test/diagram.rs b/lib/dal/tests/integration_test/diagram.rs index 70f094ad46..a92fc50d17 100644 --- a/lib/dal/tests/integration_test/diagram.rs +++ b/lib/dal/tests/integration_test/diagram.rs @@ -7,9 +7,13 @@ use dal_test::{ #[test] async fn components_removed_from_snapshot_have_virtual_diagram_entries(ctx: &mut DalContext) { let component_to_remove = - create_component_for_schema_name(ctx, "Docker Image", "Removed in sub-change set").await; + create_component_for_schema_name(ctx, "Docker Image", "Removed in sub-change set") + .await + .expect("Unable to create component."); let _component_still_in_change_set = - create_component_for_schema_name(ctx, "Docker Image", "Still here").await; + create_component_for_schema_name(ctx, "Docker Image", "Still here") + .await + .expect("Unable to create component."); ChangeSetTestHelpers::commit_and_update_snapshot_to_visibility(ctx) .await .expect("Unable to commit"); From 5044d9b651bd9a8c918c8edb0676f30c6fffae49 Mon Sep 17 00:00:00 2001 From: Jacob Helwig Date: Thu, 27 Jun 2024 08:54:06 -0700 Subject: [PATCH 7/8] Only try to create Component -> Component connections when both are in the current change set Since we display "virtual" Components of some things that aren't actually in the current change set, we need to make sure that we don't try to create a connection to them as that would effectively be a dangling pointer. --- lib/dal/src/component.rs | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/lib/dal/src/component.rs b/lib/dal/src/component.rs index 971b058f40..525eb19b73 100644 --- a/lib/dal/src/component.rs +++ b/lib/dal/src/component.rs @@ -1939,14 +1939,34 @@ impl Component { destination_input_socket_id: InputSocketId, ) -> ComponentResult> { let total_start = std::time::Instant::now(); + // Make sure both source & destination Components exist in the "current" change set. + // Eventually, this should probably be reported as an error actionable by the frontend, but + // for now, this is a no-op so we don't end up creating a broken graph. + let (_source_component, destination_component) = match ( + Component::try_get_by_id(ctx, source_component_id).await?, + Component::try_get_by_id(ctx, destination_component_id).await?, + ) { + (Some(source), Some(destination)) => (source, destination), + (source, destination) => { + warn!( + "Not creating edge; either source or destination component does not exist in current change set: {:?}, {:?}", + source, + destination, + ); + return Ok(None); + } + }; // Already have this connection? Nothing to do. - let destination_component = Component::get_by_id(ctx, destination_component_id).await?; for connection in destination_component.incoming_connections(ctx).await? { if connection.from_component_id == source_component_id && connection.from_output_socket_id == source_output_socket_id && connection.to_component_id == destination_component_id && connection.to_input_socket_id == destination_input_socket_id { + warn!( + "Not creating edge; already have this connection in change set: {:?}", + connection + ); return Ok(None); } } From 70b72cb3b632e9a708abbeecd20f008bf70165b9 Mon Sep 17 00:00:00 2001 From: wendybujalski Date: Fri, 28 Jun 2024 17:09:40 -0400 Subject: [PATCH 8/8] feat(web) - edges can be restored again! --- .../src/components/DetailsPanelTimestamps.vue | 6 +++--- .../ModelingView/RestoreSelectionModal.vue | 16 +++++++++++++++- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/app/web/src/components/DetailsPanelTimestamps.vue b/app/web/src/components/DetailsPanelTimestamps.vue index 89533959ab..36f7a36067 100644 --- a/app/web/src/components/DetailsPanelTimestamps.vue +++ b/app/web/src/components/DetailsPanelTimestamps.vue @@ -17,7 +17,7 @@ class="shrink-0" tone="inherit" /> -
+
{{ formatters.timeAgo(created.timestamp) }} by {{ created.actor.label }}
@@ -42,7 +42,7 @@ class="shrink-0" tone="inherit" /> -
+
{{ formatters.timeAgo(modified?.timestamp) }} by {{ modified?.actor.label }}
@@ -58,7 +58,7 @@ class="shrink-0" tone="inherit" /> -
+
{{ formatters.timeAgo(deleted?.timestamp) }} by {{ deleted?.actor.label }}
diff --git a/app/web/src/components/ModelingView/RestoreSelectionModal.vue b/app/web/src/components/ModelingView/RestoreSelectionModal.vue index d81c0e4081..ec2206d693 100644 --- a/app/web/src/components/ModelingView/RestoreSelectionModal.vue +++ b/app/web/src/components/ModelingView/RestoreSelectionModal.vue @@ -78,12 +78,26 @@ function open() { } async function onConfirmRestore() { - if (componentsStore.selectedComponentIds) { + if ( + componentsStore.selectedComponentIds && + componentsStore.selectedComponentIds.length > 0 + ) { const data = {} as Record; componentsStore.selectedComponentIds.forEach((cId) => { data[cId] = !!componentsStore.componentsById[cId]?.fromBaseChangeSet; }); await componentsStore.RESTORE_COMPONENTS(data); + } else if (componentsStore.selectedEdge) { + await componentsStore.CREATE_COMPONENT_CONNECTION( + { + componentId: componentsStore.selectedEdge.fromComponentId, + socketId: componentsStore.selectedEdge.fromSocketId, + }, + { + componentId: componentsStore.selectedEdge.toComponentId, + socketId: componentsStore.selectedEdge.toSocketId, + }, + ); } componentsStore.setSelectedComponentId(null); close();