From 9b7c560ad517011ced8b454aae3f25a55f326e10 Mon Sep 17 00:00:00 2001 From: John Keiser Date: Wed, 28 Aug 2024 14:51:45 -0700 Subject: [PATCH] Copy computed value cache when pasting a node. When we pasted a node, constant values (things the user edited) were being copied, but the JSON value cache was not carried over. Since constant values do not get touched in dependent value updates, they never got set. Now we clone the value cache when pasting a node. --- lib/dal-test/src/expected.rs | 29 ++++ lib/dal/src/attribute/value.rs | 112 ++++++++++++++- lib/dal/src/component.rs | 117 ++++------------ lib/dal/tests/integration_test/component.rs | 142 ++++++++++++-------- 4 files changed, 252 insertions(+), 148 deletions(-) diff --git a/lib/dal-test/src/expected.rs b/lib/dal-test/src/expected.rs index f00fc73673..dc31610aa0 100644 --- a/lib/dal-test/src/expected.rs +++ b/lib/dal-test/src/expected.rs @@ -5,6 +5,7 @@ use crate::helpers::ChangeSetTestHelpers; use dal::{ self, + component::ComponentGeometry, prop::{Prop, PropPath}, property_editor::values::PropertyEditorValues, schema::variant::authoring::VariantAuthoringClient, @@ -368,6 +369,18 @@ impl ExpectComponent { .expect("get component by id") } + pub async fn geometry(self, ctx: &DalContext) -> ComponentGeometry { + self.component(ctx).await.geometry() + } + + pub async fn view(self, ctx: &DalContext) -> Option { + self.component(ctx) + .await + .view(ctx) + .await + .expect("get component value") + } + pub async fn get_type(self, ctx: &DalContext) -> ComponentType { dal::Component::get_type_by_id(ctx, self.0) .await @@ -480,10 +493,16 @@ impl ExpectComponentProp { self.attribute_value(ctx).await.children(ctx).await } + // The value of the prop, or its default value. pub async fn view(self, ctx: &DalContext) -> Option { self.attribute_value(ctx).await.view(ctx).await } + // Whether this attribute has a value explicitly set + pub async fn has_value(self, ctx: &DalContext) -> bool { + self.attribute_value(ctx).await.has_value(ctx).await + } + pub async fn update(self, ctx: &DalContext, value: Option) { self.attribute_value(ctx).await.update(ctx, value).await } @@ -657,6 +676,16 @@ impl ExpectAttributeValue { .expect("attribute value view") } + // Whether this attribute has a value explicitly set + pub async fn has_value(self, ctx: &DalContext) -> bool { + self.attribute_value(ctx) + .await + .value(ctx) + .await + .expect("get attribute value") + .is_some() + } + pub async fn get(self, ctx: &DalContext) -> Value { self.view(ctx).await.expect("attribute must have value") } diff --git a/lib/dal/src/attribute/value.rs b/lib/dal/src/attribute/value.rs index 9594270839..4ac9c670a6 100644 --- a/lib/dal/src/attribute/value.rs +++ b/lib/dal/src/attribute/value.rs @@ -56,7 +56,7 @@ use tokio::sync::{RwLock, TryLockError}; pub use dependent_value_graph::DependentValueGraph; -use crate::attribute::prototype::AttributePrototypeError; +use crate::attribute::prototype::{AttributePrototypeError, AttributePrototypeSource}; use crate::change_set::ChangeSetError; use crate::component::socket::ComponentInputSocket; use crate::func::argument::{FuncArgument, FuncArgumentError}; @@ -157,6 +157,8 @@ pub enum AttributeValueError { InsertionForInvalidPropKind(PropKind), #[error("layer db error: {0}")] LayerDb(#[from] si_layer_cache::LayerDbError), + #[error("missing attribute prototype argument source: {0}")] + MissingAttributePrototypeArgumentSource(AttributePrototypeArgumentId), #[error("missing attribute value with id: {0}")] MissingForId(AttributeValueId), #[error("attribute value {0} missing prop edge when one was expected")] @@ -2048,6 +2050,114 @@ impl AttributeValue { Ok(()) } + async fn clone_node_weight_values_from( + ctx: &DalContext, + dest_av_id: AttributeValueId, + from_av_id: AttributeValueId, + ) -> AttributeValueResult<()> { + let workspace_snapshot = ctx.workspace_snapshot()?; + + let mut dest_node_weight = workspace_snapshot + .get_node_weight_by_id(dest_av_id) + .await? + .get_attribute_value_node_weight()?; + let from_node_weight = workspace_snapshot + .get_node_weight_by_id(from_av_id) + .await? + .get_attribute_value_node_weight()?; + dest_node_weight.set_unprocessed_value(from_node_weight.unprocessed_value()); + dest_node_weight.set_value(from_node_weight.value()); + workspace_snapshot + .add_or_replace_node(NodeWeight::AttributeValue(dest_node_weight)) + .await?; + + Ok(()) + } + + pub async fn clone_value_from( + ctx: &DalContext, + dest_av_id: AttributeValueId, + from_av_id: AttributeValueId, + ) -> AttributeValueResult<()> { + // If the old component has a non-link value (prototype), copy it over + if let Some(from_prototype_id) = + AttributeValue::component_prototype_id(ctx, from_av_id).await? + { + let from_func_id = AttributePrototype::func_id(ctx, from_prototype_id).await?; + let dest_prototype = AttributePrototype::new(ctx, from_func_id).await?; + + for from_apa_id in + AttributePrototypeArgument::list_ids_for_prototype(ctx, from_prototype_id).await? + { + let from_func_arg_id = + AttributePrototypeArgument::func_argument_id_by_id(ctx, from_apa_id).await?; + let from_value_source = + AttributePrototypeArgument::value_source_by_id(ctx, from_apa_id) + .await? + .ok_or( + AttributeValueError::MissingAttributePrototypeArgumentSource( + from_apa_id, + ), + )?; + + let dest_apa = + AttributePrototypeArgument::new(ctx, dest_prototype.id(), from_func_arg_id) + .await?; + AttributePrototypeArgument::set_value_source(ctx, dest_apa.id(), from_value_source) + .await?; + } + + AttributeValue::set_component_prototype_id(ctx, dest_av_id, dest_prototype.id, None) + .await?; + + let sources = AttributePrototype::input_sources(ctx, dest_prototype.id).await?; + for source in sources { + match source { + AttributePrototypeSource::AttributeValue(_, _) => { + continue; + } + AttributePrototypeSource::Prop(prop_id, key) => { + Prop::add_edge_to_attribute_prototype( + ctx, + prop_id, + dest_prototype.id, + EdgeWeightKind::Prototype(key), + ) + .await?; + } + AttributePrototypeSource::InputSocket(socket_id, key) => { + InputSocket::add_edge_to_attribute_prototype( + ctx, + socket_id, + dest_prototype.id, + EdgeWeightKind::Prototype(key), + ) + .await?; + } + AttributePrototypeSource::OutputSocket(socket_id, key) => { + OutputSocket::add_edge_to_attribute_prototype( + ctx, + socket_id, + dest_prototype.id, + EdgeWeightKind::Prototype(key), + ) + .await?; + } + } + } + } else if let Some(existing_prototype_id) = + AttributeValue::component_prototype_id(ctx, dest_av_id).await? + { + AttributePrototype::remove(ctx, existing_prototype_id).await?; + } + + if !Self::is_set_by_dependent_function(ctx, dest_av_id).await? { + Self::clone_node_weight_values_from(ctx, dest_av_id, from_av_id).await?; + } + + Ok(()) + } + pub async fn get_by_id_or_error( ctx: &DalContext, attribute_value_id: AttributeValueId, diff --git a/lib/dal/src/component.rs b/lib/dal/src/component.rs index c5b8f861a7..fbf300f216 100644 --- a/lib/dal/src/component.rs +++ b/lib/dal/src/component.rs @@ -32,6 +32,7 @@ use crate::change_set::ChangeSetError; use crate::change_status::ChangeStatus; use crate::code_view::CodeViewError; use crate::diagram::{SummaryDiagramEdge, SummaryDiagramInferredEdge}; +use crate::func::argument::FuncArgumentError; use crate::history_event::HistoryEventMetadata; use crate::layer_db_types::{ComponentContent, ComponentContentV1}; use crate::prop::{PropError, PropPath}; @@ -117,6 +118,8 @@ pub enum ComponentError { Frame(#[from] Box), #[error("func error: {0}")] Func(#[from] FuncError), + #[error("func argument error: {0}")] + FuncArgumentError(#[from] FuncArgumentError), #[error("helper error: {0}")] Helper(#[from] HelperError), #[error("input socket error: {0}")] @@ -127,8 +130,6 @@ pub enum ComponentError { InputSocketTooManyAttributeValues(InputSocketId), #[error("layer db error: {0}")] LayerDb(#[from] si_layer_cache::LayerDbError), - #[error("missing attribute prototype argument source: {0}")] - MissingAttributePrototypeArgumentSource(AttributePrototypeArgumentId), #[error("component {0} missing attribute value for code")] MissingCodeValue(ComponentId), #[error("missing controlling func data for parent attribute value id: {0}")] @@ -332,6 +333,15 @@ impl Component { self.height.as_deref() } + pub fn geometry(&self) -> ComponentGeometry { + ComponentGeometry { + x: self.x.to_owned(), + y: self.y.to_owned(), + width: self.width.to_owned(), + height: self.height.to_owned(), + } + } + pub fn timestamp(&self) -> &Timestamp { &self.timestamp } @@ -938,117 +948,48 @@ impl Component { pub async fn clone_attributes_from( &self, ctx: &DalContext, - old_component_id: ComponentId, + from_component_id: ComponentId, ) -> ComponentResult<()> { - let old_sv_id = Component::schema_variant_id(ctx, old_component_id).await?; - let new_sv_id = Component::schema_variant_id(ctx, self.id).await?; + let from_sv_id = Component::schema_variant_id(ctx, from_component_id).await?; + let dest_sv_id = Component::schema_variant_id(ctx, self.id).await?; - if old_sv_id != new_sv_id { + if from_sv_id != dest_sv_id { return Err(ComponentError::CannotCloneFromDifferentVariants); } // Paste attribute value "values" from original component (or create them for maps/arrays) // // We could make this more efficient by skipping everything set by non builtins (si:setString, si:setObject, etc), since everything that is propagated will be re-propagated - let old_root_id = Component::root_attribute_value_id(ctx, old_component_id).await?; - let new_root_id = Component::root_attribute_value_id(ctx, self.id).await?; - let mut work_queue = VecDeque::from([(old_root_id, new_root_id)]); + let from_root_id = Component::root_attribute_value_id(ctx, from_component_id).await?; + let dest_root_id = Component::root_attribute_value_id(ctx, self.id).await?; + let mut work_queue = VecDeque::from([(from_root_id, dest_root_id)]); // Paste attribute prototypes // - either updates component prototype to a copy of the original component // - or removes component prototype, restoring the schema one (needed because of manual update from the block above) while - while let Some((old_av_id, new_av_id)) = work_queue.pop_front() { - // If the old component has a value (prototype), copy it over - if let Some(old_prototype_id) = - AttributeValue::component_prototype_id(ctx, old_av_id).await? - { - let old_func_id = AttributePrototype::func_id(ctx, old_prototype_id).await?; - let new_prototype = AttributePrototype::new(ctx, old_func_id).await?; - - for old_apa_id in - AttributePrototypeArgument::list_ids_for_prototype(ctx, old_prototype_id) - .await? - { - let old_func_arg_id = - AttributePrototypeArgument::func_argument_id_by_id(ctx, old_apa_id).await?; - let old_value_source = - AttributePrototypeArgument::value_source_by_id(ctx, old_apa_id) - .await? - .ok_or(ComponentError::MissingAttributePrototypeArgumentSource( - old_apa_id, - ))?; - - let apa = - AttributePrototypeArgument::new(ctx, new_prototype.id(), old_func_arg_id) - .await?; - AttributePrototypeArgument::set_value_source(ctx, apa.id(), old_value_source) - .await?; - } - - AttributeValue::set_component_prototype_id(ctx, new_av_id, new_prototype.id, None) - .await?; - - let sources = AttributePrototype::input_sources(ctx, new_prototype.id).await?; - for source in sources { - match source { - AttributePrototypeSource::AttributeValue(_, _) => { - continue; - } - AttributePrototypeSource::Prop(prop_id, key) => { - Prop::add_edge_to_attribute_prototype( - ctx, - prop_id, - new_prototype.id, - EdgeWeightKind::Prototype(key), - ) - .await?; - } - AttributePrototypeSource::InputSocket(socket_id, key) => { - InputSocket::add_edge_to_attribute_prototype( - ctx, - socket_id, - new_prototype.id, - EdgeWeightKind::Prototype(key), - ) - .await?; - } - AttributePrototypeSource::OutputSocket(socket_id, key) => { - OutputSocket::add_edge_to_attribute_prototype( - ctx, - socket_id, - new_prototype.id, - EdgeWeightKind::Prototype(key), - ) - .await?; - } - } - } - } else if let Some(existing_prototype_id) = - AttributeValue::component_prototype_id(ctx, new_av_id).await? - { - AttributePrototype::remove(ctx, existing_prototype_id).await?; - } + while let Some((from_av_id, dest_av_id)) = work_queue.pop_front() { + AttributeValue::clone_value_from(ctx, dest_av_id, from_av_id).await?; // Get children, possibly creating new ones if we don't have them yet for child_pair in - AttributeValue::get_child_av_id_pairs_in_order(ctx, old_av_id, new_av_id).await? + AttributeValue::get_child_av_id_pairs_in_order(ctx, from_av_id, dest_av_id).await? { match child_pair { - ChildAttributeValuePair::Both(_, old_child_av_id, new_child_av_id) => { - work_queue.push_back((old_child_av_id, new_child_av_id)); + ChildAttributeValuePair::Both(_, from_child_av_id, dest_child_av_id) => { + work_queue.push_back((from_child_av_id, dest_child_av_id)); } // If the child is only in the copied component, we create a new one for // ourselves - ChildAttributeValuePair::FirstOnly(key, old_child_av_id) => { - let new_child_av_id = AttributeValue::new( + ChildAttributeValuePair::FirstOnly(key, from_child_av_id) => { + let dest_child_av_id = AttributeValue::new( ctx, - AttributeValue::is_for(ctx, old_child_av_id).await?, + AttributeValue::is_for(ctx, from_child_av_id).await?, Some(self.id), - Some(new_av_id), + Some(dest_av_id), key, ) .await? .id; - work_queue.push_back((old_child_av_id, new_child_av_id)); + work_queue.push_back((from_child_av_id, dest_child_av_id)); } // TODO this case wasn't handled before, and shouldn't really be possible ... ChildAttributeValuePair::SecondOnly(..) => { diff --git a/lib/dal/tests/integration_test/component.rs b/lib/dal/tests/integration_test/component.rs index 42d8c28b54..58b2fa1dc3 100644 --- a/lib/dal/tests/integration_test/component.rs +++ b/lib/dal/tests/integration_test/component.rs @@ -1,15 +1,13 @@ use dal::attribute::value::DependentValueGraph; -use dal::component::{ComponentGeometry, DEFAULT_COMPONENT_HEIGHT, DEFAULT_COMPONENT_WIDTH}; +use dal::component::{DEFAULT_COMPONENT_HEIGHT, DEFAULT_COMPONENT_WIDTH}; use dal::diagram::Diagram; use dal::prop::{Prop, PropPath}; use dal::property_editor::values::PropertyEditorValues; use dal::{AttributeValue, AttributeValueId}; use dal::{Component, DalContext, Schema, SchemaVariant}; use dal_test::expected::{self, ExpectComponent}; +use dal_test::helpers::create_component_for_default_schema_name; use dal_test::helpers::ChangeSetTestHelpers; -use dal_test::helpers::{ - connect_components_with_socket_names, create_component_for_default_schema_name, -}; use dal_test::test; use pretty_assertions_sorted::assert_eq; use serde_json::json; @@ -701,73 +699,98 @@ async fn set_the_universe(ctx: &mut DalContext) { } #[test] -async fn paste_component(ctx: &mut DalContext) { - let pirate_name = "Long John Silver"; - let parrot_name = "Captain Flint"; - let pirate_component = create_component_for_default_schema_name(ctx, "pirate", pirate_name) - .await - .expect("could not create component"); +async fn paste_component_with_value(ctx: &mut DalContext) { + let component = ExpectComponent::create_named(ctx, "pirate", "Long John Silver").await; + let parrots = component + .prop(ctx, ["root", "domain", "parrot_names"]) + .await; - let parrots_path = &["root", "domain", "parrot_names"]; + // set value on pet shop component + parrots.push(ctx, "Captain Flint").await; + expected::commit_and_update_snapshot_to_visibility(ctx).await; - let pet_shop_component = create_component_for_default_schema_name(ctx, "pet_shop", "Petopia") - .await - .expect("could not create component"); + assert!(parrots.has_value(ctx).await); - // set value on source component - { - let pet_shop_parrot_av_id = pet_shop_component - .attribute_values_for_prop(ctx, parrots_path) + // Copy/paste the pirate component + let component_copy = ExpectComponent( + component + .component(ctx) + .await + .copy_paste(ctx, component.geometry(ctx).await) .await - .expect("find value ids for prop parrot_names") - .pop() - .expect("there should only be one value id"); + .expect("unable to paste component") + .id(), + ); + let parrots_copy = component_copy.prop(ctx, parrots).await; + + assert_ne!(component.id(), component_copy.id()); + + expected::commit_and_update_snapshot_to_visibility(ctx).await; + + // Validate that component_copy has the new value + assert!(parrots_copy.has_value(ctx).await); + assert_eq!(json!(["Captain Flint"]), parrots_copy.get(ctx).await); + + assert!(parrots.has_value(ctx).await); +} + +#[test] +async fn paste_component_with_dependent_value(ctx: &mut DalContext) { + let source = ExpectComponent::create_named(ctx, "pet_shop", "Petopia").await; + let downstream = ExpectComponent::create_named(ctx, "pirate", "Long John Silver").await; + let source_parrots = source.prop(ctx, ["root", "domain", "parrot_names"]).await; + let downstream_parrots = downstream + .prop(ctx, ["root", "domain", "parrot_names"]) + .await; + + // set value on source component + source_parrots.push(ctx, "Captain Flint").await; + source + .connect(ctx, "parrot_names", downstream, "parrot_names") + .await; + expected::commit_and_update_snapshot_to_visibility(ctx).await; + + // Check that downstream has the parrots value, and that it is not explicitly set + assert!(downstream_parrots.has_value(ctx).await); + assert_eq!( + Some(json!(["Captain Flint"])), + downstream_parrots.view(ctx).await + ); - AttributeValue::insert(ctx, pet_shop_parrot_av_id, Some(parrot_name.into()), None) + // Copy/paste the downstream component + let downstream_copy = ExpectComponent( + downstream + .component(ctx) + .await + .copy_paste(ctx, downstream.geometry(ctx).await) .await - .expect("insert value in pet_shop parrot_names array"); - } + .expect("unable to paste component") + .id(), + ); + let downstream_copy_parrots = downstream_copy.prop(ctx, downstream_parrots).await; - connect_components_with_socket_names( - ctx, - pet_shop_component.id(), - "parrot_names", - pirate_component.id(), - "parrot_names", - ) - .await - .expect("could not connect components with socket names"); + assert_ne!(downstream.id(), downstream_copy.id()); - ChangeSetTestHelpers::commit_and_update_snapshot_to_visibility(ctx) - .await - .expect("could not commit and update snapshot to visibility"); + // Check that the copy does *not* have the parrots value, because it is not explicitly set + // (because it has no link) + assert!(!downstream_copy_parrots.has_value(ctx).await); + assert_eq!(None, downstream_copy_parrots.view(ctx).await); - let pasted_pirate_component = pirate_component - .copy_paste( - ctx, - ComponentGeometry { - x: pirate_component.x().to_string(), - y: pirate_component.y().to_string(), - width: None, - height: None, - }, - ) - .await - .expect("unable to paste component"); + expected::commit_and_update_snapshot_to_visibility(ctx).await; - ChangeSetTestHelpers::commit_and_update_snapshot_to_visibility(ctx) - .await - .expect("could not commit and update snapshot to visibility"); + // Check that the copy does *not* have the parrots value, because it is not explicitly set + // (because it has no link) + assert!(!downstream_copy_parrots.has_value(ctx).await); + assert_eq!(None, downstream_copy_parrots.view(ctx).await); - let view = pasted_pirate_component - .view(ctx) - .await - .expect("unable to get materialized view of component") - .expect("no view found"); + assert!(downstream_parrots.has_value(ctx).await); + assert_eq!( + Some(json!(["Captain Flint"])), + downstream_parrots.view(ctx).await + ); assert_eq!( - view, - serde_json::json!({ + Some(json!({ "domain": { // Propagated from /si/name, which means the attribute prototype has been copied // from the copied component (since we manually set all values, which removes the @@ -786,6 +809,7 @@ async fn paste_component(ctx: &mut DalContext) { "name": "Long John Silver - Copy", "type": "component", }, - }) + })), + downstream_copy.view(ctx).await, ); }