merge: #3429 #3440

3429: feat(dal,sdf) delete and create array/map items from prop editor r=vbustamante a=vbustamante

<img src="https://media2.giphy.com/media/wQq7MBdNdxU2MjeBNI/giphy.gif"/>

CLOSES ENG-2393
CLOSES ENG-2394
CLOSES ENG-2395
CLOSES ENG-2396


3440: feat(dal, sdf-server): Restore Abandon ChangeSet Route r=stack72 a=stack72



Co-authored-by: Victor Bustamante <[email protected]>
Co-authored-by: wendybujalski <[email protected]>
Co-authored-by: stack72 <[email protected]>

lib/dal-test/src/lib.rs

@@ -814,6 +814,7 @@ async fn migrate_local_builtins(
     schemas::migrate_test_exclusive_schema_bethesda_secret(&ctx).await?;
     schemas::migrate_test_exclusive_schema_swifty(&ctx).await?;
     schemas::migrate_test_exclusive_schema_katy_perry(&ctx).await?;
+    schemas::migrate_test_exclusive_schema_pirate(&ctx).await?;
 
     ctx.blocking_commit().await?;
 

lib/dal-test/src/schemas/mod.rs

@@ -1,12 +1,14 @@
+pub use test_exclusive_schema_bethesda_secret::migrate_test_exclusive_schema_bethesda_secret;
+pub use test_exclusive_schema_fallout::migrate_test_exclusive_schema_fallout;
+pub use test_exclusive_schema_katy_perry::migrate_test_exclusive_schema_katy_perry;
+pub use test_exclusive_schema_pirate::migrate_test_exclusive_schema_pirate;
+pub use test_exclusive_schema_starfield::migrate_test_exclusive_schema_starfield;
+pub use test_exclusive_schema_swifty::migrate_test_exclusive_schema_swifty;
+
 mod schema_helpers;
 mod test_exclusive_schema_bethesda_secret;
 mod test_exclusive_schema_fallout;
 mod test_exclusive_schema_katy_perry;
+mod test_exclusive_schema_pirate;
 mod test_exclusive_schema_starfield;
 mod test_exclusive_schema_swifty;
-
-pub use test_exclusive_schema_bethesda_secret::migrate_test_exclusive_schema_bethesda_secret;
-pub use test_exclusive_schema_fallout::migrate_test_exclusive_schema_fallout;
-pub use test_exclusive_schema_katy_perry::migrate_test_exclusive_schema_katy_perry;
-pub use test_exclusive_schema_starfield::migrate_test_exclusive_schema_starfield;
-pub use test_exclusive_schema_swifty::migrate_test_exclusive_schema_swifty;

lib/dal-test/src/schemas/test_exclusive_schema_pirate.rs

@@ -0,0 +1,108 @@
+use crate::schemas::schema_helpers::{build_asset_func, create_identity_func};
+use dal::pkg::import_pkg_from_pkg;
+use dal::{pkg, prop::PropPath, ComponentType};
+use dal::{BuiltinsResult, DalContext, PropKind};
+use si_pkg::SchemaSpecData;
+use si_pkg::{
+    AttrFuncInputSpec, AttrFuncInputSpecKind, PkgSpec, PropSpec, SchemaSpec, SchemaVariantSpec,
+    SchemaVariantSpecData, SiPkg,
+};
+
+pub async fn migrate_test_exclusive_schema_pirate(ctx: &DalContext) -> BuiltinsResult<()> {
+    let mut builder = PkgSpec::builder();
+
+    builder
+        .name("pirate")
+        .version("2024-03-12")
+        .created_by("System Initiative");
+
+    let identity_func_spec = create_identity_func();
+
+    // Create Scaffold Func
+    let fn_name = "test:scaffoldPirateAsset";
+    let authoring_schema_func = build_asset_func(fn_name).await?;
+
+    let schema = SchemaSpec::builder()
+        .name("pirate")
+        .data(
+            SchemaSpecData::builder()
+                .name("pirate")
+                .category("test exclusive")
+                .category_name("pirate")
+                .build()
+                .expect("schema spec data build"),
+        )
+        .variant(
+            SchemaVariantSpec::builder()
+                .name("v0")
+                .unique_id("pirate_sv")
+                .data(
+                    SchemaVariantSpecData::builder()
+                        .name("v0")
+                        .color("#ff00ff")
+                        .func_unique_id(&authoring_schema_func.unique_id)
+                        .component_type(ComponentType::Component)
+                        .build()
+                        .expect("build variant spec data"),
+                )
+                .domain_prop(
+                    PropSpec::builder()
+                        .name("name")
+                        .kind(PropKind::String)
+                        .func_unique_id(&identity_func_spec.unique_id)
+                        .input(
+                            AttrFuncInputSpec::builder()
+                                .kind(AttrFuncInputSpecKind::Prop)
+                                .name("identity")
+                                .prop_path(PropPath::new(["root", "si", "name"]))
+                                .build()?,
+                        )
+                        .build()?,
+                )
+                .domain_prop(
+                    PropSpec::builder()
+                        .name("parrot_names")
+                        .kind(PropKind::Array)
+                        .type_prop(
+                            PropSpec::builder()
+                                .kind(PropKind::String)
+                                .name("parrot_name")
+                                .build()?,
+                        )
+                        .build()?,
+                )
+                .domain_prop(
+                    PropSpec::builder()
+                        .name("treasure")
+                        .kind(PropKind::Map)
+                        .type_prop(
+                            PropSpec::builder()
+                                .kind(PropKind::String)
+                                .name("location")
+                                .build()?,
+                        )
+                        .build()?,
+                )
+                .build()?,
+        )
+        .build()?;
+
+    let spec = builder
+        .func(identity_func_spec)
+        .func(authoring_schema_func)
+        .schema(schema)
+        .build()?;
+
+    let pkg = SiPkg::load_from_spec(spec)?;
+    import_pkg_from_pkg(
+        ctx,
+        &pkg,
+        Some(pkg::ImportOptions {
+            schemas: Some(vec!["pirate".into()]),
+            ..Default::default()
+        }),
+    )
+    .await?;
+
+    Ok(())
+}

lib/dal/src/attribute/value.rs

@@ -147,6 +147,8 @@ pub enum AttributeValueError {
     NodeWeight(#[from] NodeWeightError),
     #[error("node weight mismatch, expected {0:?} to be {1:?}")]
     NodeWeightMismatch(NodeIndex, NodeWeightDiscriminants),
+    #[error("attribute value does not have ordering node as expected: {0}")]
+    NoOrderingNodeForAttributeValue(AttributeValueId),
     #[error("attribute value not found for component ({0}) and input socket ({1})")]
     NotFoundForComponentAndInputSocket(ComponentId, InputSocketId),
     #[error("attribute value {0} has no outgoing edge to a prop or socket")]
@@ -159,6 +161,8 @@ pub enum AttributeValueError {
     PropMoreThanOneChild(PropId),
     #[error("prop not found for attribute value: {0}")]
     PropNotFound(AttributeValueId),
+    #[error("trying to delete av that's not related to child of map or array: {0}")]
+    RemovingWhenNotChildOrMapOrArray(AttributeValueId),
     #[error("serde_json: {0}")]
     SerdeJson(#[from] serde_json::Error),
     #[error("store error: {0}")]
@@ -1941,4 +1945,102 @@ impl AttributeValue {
             None => None,
         })
     }
+
+    pub async fn get_parent_av_id_for_ordered_child(
+        ctx: &DalContext,
+        id: AttributeValueId,
+    ) -> AttributeValueResult<Option<AttributeValueId>> {
+        let workspace_snapshot = ctx.workspace_snapshot()?;
+
+        let ordering_node_id = match workspace_snapshot
+            .incoming_sources_for_edge_weight_kind(id, EdgeWeightKindDiscriminants::Ordinal)
+            .await?
+            .first()
+            .copied()
+        {
+            Some(ordering_idx) => workspace_snapshot.get_node_weight(ordering_idx).await?.id(),
+            None => return Ok(None),
+        };
+
+        let parent_av_id = if let Some(parent_av_idx) = workspace_snapshot
+            .incoming_sources_for_edge_weight_kind(
+                ordering_node_id,
+                EdgeWeightKindDiscriminants::Ordering,
+            )
+            .await?
+            .first()
+            .copied()
+        {
+            let parent_av_id: AttributeValueId = workspace_snapshot
+                .get_node_weight(parent_av_idx)
+                .await?
+                .id()
+                .into();
+
+            let prop_id = AttributeValue::prop(ctx, parent_av_id).await?;
+
+            let parent_prop = Prop::get_by_id(ctx, prop_id).await?;
+
+            if ![PropKind::Map, PropKind::Array].contains(&parent_prop.kind) {
+                return Ok(None);
+            }
+
+            parent_av_id
+        } else {
+            return Ok(None);
+        };
+
+        Ok(Some(parent_av_id))
+    }
+
+    pub async fn get_child_av_ids_for_ordered_parent(
+        ctx: &DalContext,
+        id: AttributeValueId,
+    ) -> AttributeValueResult<Vec<AttributeValueId>> {
+        let workspace_snapshot = ctx.workspace_snapshot()?;
+
+        if let Some(ordering) = workspace_snapshot
+            .outgoing_targets_for_edge_weight_kind(id, EdgeWeightKindDiscriminants::Ordering)
+            .await?
+            .pop()
+        {
+            let node_weight = workspace_snapshot.get_node_weight(ordering).await?;
+            if let NodeWeight::Ordering(ordering_weight) = node_weight {
+                Ok(ordering_weight
+                    .order()
+                    .clone()
+                    .into_iter()
+                    .map(|ulid| ulid.into())
+                    .collect())
+            } else {
+                Err(AttributeValueError::NodeWeightMismatch(
+                    ordering,
+                    NodeWeightDiscriminants::Ordering,
+                ))
+            }
+        } else {
+            Err(AttributeValueError::NoOrderingNodeForAttributeValue(id))
+        }
+    }
+
+    pub async fn remove_by_id(ctx: &DalContext, id: AttributeValueId) -> AttributeValueResult<()> {
+        let parent_av_id = Self::get_parent_av_id_for_ordered_child(ctx, id)
+            .await?
+            .ok_or(AttributeValueError::RemovingWhenNotChildOrMapOrArray(id))?;
+
+        let av = Self::get_by_id(ctx, id).await?;
+
+        ctx.workspace_snapshot()?
+            .remove_node_by_id(ctx.change_set_pointer()?, av.id)
+            .await?;
+
+        ctx.enqueue_job(DependentValuesUpdate::new(
+            ctx.access_builder(),
+            *ctx.visibility(),
+            vec![parent_av_id],
+        ))
+        .await?;
+
+        Ok(())
+    }
 }

lib/dal/src/property_editor/values.rs

@@ -49,7 +49,6 @@ impl PropertyEditorValues {
                     .value(ctx)
                     .await?
                     .unwrap_or(Value::Null),
-                // TODO(nick): restore all these fields below.
                 is_from_external_source: false,
                 can_be_set_by_socket: false,
                 is_controlled_by_intrinsic_func: true,
@@ -66,28 +65,49 @@ impl PropertyEditorValues {
             // Collect all child attribute values.
             let mut cache: Vec<(AttributeValueId, Option<String>)> = Vec::new();
             {
-                let child_attribute_values_with_keys: Vec<(NodeIndex, Option<String>)> =
-                    workspace_snapshot
-                        .edges_directed(attribute_value_id, Direction::Outgoing)
-                        .await?
-                        .into_iter()
-                        .filter_map(|(edge_weight, _, target_idx)| {
-                            if let EdgeWeightKind::Contain(key) = edge_weight.kind() {
-                                Some((target_idx, key.to_owned()))
-                            } else {
-                                None
-                            }
-                        })
-                        .collect();
+                let mut child_attribute_values_with_keys_by_id: HashMap<
+                    AttributeValueId,
+                    (NodeIndex, Option<String>),
+                > = HashMap::new();
+
+                for (edge_weight, _, target_idx) in workspace_snapshot
+                    .edges_directed(attribute_value_id, Direction::Outgoing)
+                    .await?
+                {
+                    if let EdgeWeightKind::Contain(key) = edge_weight.kind() {
+                        let child_id = workspace_snapshot
+                            .get_node_weight(target_idx)
+                            .await?
+                            .id()
+                            .into();
+
+                        child_attribute_values_with_keys_by_id
+                            .insert(child_id, (target_idx, key.to_owned()));
+                    }
+                }
+
+                let maybe_ordering =
+                    AttributeValue::get_child_av_ids_for_ordered_parent(ctx, attribute_value_id)
+                        .await
+                        .ok();
 
-                // NOTE(nick): this entire function is likely wasteful. Zack and Jacob, have mercy on me.
-                for (child_attribute_value_node_index, key) in child_attribute_values_with_keys {
+                // Ideally every attribute value with children is connected via an ordering node
+                // We don't error out on ordering not existing here because we don't have that
+                // guarantee. If that becomes a certainty we should fail on maybe_ordering==None.
+                for av_id in maybe_ordering.unwrap_or_else(|| {
+                    child_attribute_values_with_keys_by_id
+                        .keys()
+                        .cloned()
+                        .collect()
+                }) {
+                    let (child_attribute_value_node_index, key) =
+                        &child_attribute_values_with_keys_by_id[&av_id];
                     let child_attribute_value_node_weight = workspace_snapshot
-                        .get_node_weight(child_attribute_value_node_index)
+                        .get_node_weight(*child_attribute_value_node_index)
                         .await?;
                     let content =
                         child_attribute_value_node_weight.get_attribute_value_node_weight()?;
-                    cache.push((content.id().into(), key));
+                    cache.push((content.id().into(), key.clone()));
                 }
             }
 

lib/dal/tests/integration_test/change_set.rs

@@ -1,6 +1,6 @@
 use dal::change_set_pointer::view::OpenChangeSetsView;
 use dal::change_set_pointer::ChangeSetPointer;
-use dal::DalContext;
+use dal::{ChangeSetStatus, DalContext};
 use dal_test::test;
 use pretty_assertions_sorted::assert_eq;
 use std::collections::HashSet;
@@ -118,3 +118,50 @@ async fn open_change_sets(ctx: &mut DalContext) {
         change_set_ids_again,                                                   // actual
     );
 }
+
+#[test]
+async fn abandon_change_set(ctx: &mut DalContext) {
+    let change_set_name = "for abandonment".to_string();
+    let mut abandonment_change_set = ChangeSetPointer::fork_head(ctx, change_set_name.clone())
+        .await
+        .expect("could not create new change set");
+    ctx.update_visibility_and_snapshot_to_visibility(abandonment_change_set.id)
+        .await
+        .expect("could not update visibility");
+    ctx.commit_no_rebase()
+        .await
+        .expect("could not perform commit");
+
+    // List open changesets.
+    let view = OpenChangeSetsView::assemble(ctx)
+        .await
+        .expect("could not assemble view");
+
+    // Check that the expected number of change sets exist....
+    assert_eq!(
+        3,                      // expected
+        view.change_sets.len()  // actual
+    );
+
+    ctx.update_visibility_and_snapshot_to_visibility_no_editing_change_set(&abandonment_change_set)
+        .await
+        .expect("could not update visibility");
+    abandonment_change_set
+        .update_status(ctx, ChangeSetStatus::Abandoned)
+        .await
+        .expect("Unable to abandon changeset");
+
+    // relist the open changesets.
+    let view = OpenChangeSetsView::assemble(ctx)
+        .await
+        .expect("could not assemble view");
+
+    // Check that we no longer have the abandoned changeset
+    assert_eq!(
+        2,                      // expected
+        view.change_sets.len()  // actual
+    );
+
+    let change_set_names = Vec::from_iter(view.change_sets.iter().map(|c| c.name.clone()));
+    assert!(!change_set_names.contains(&change_set_name))
+}