Merge pull request #4160 from systeminit/brit/bug-459-ensure-descript…

…ion-is-not-lost-on-unlock

Add description to Schema Variant pkg import/export to ensure it persists across lock/unlock and import/export variant

lib/dal-test/src/helpers/change_set.rs

@@ -6,7 +6,7 @@ use color_eyre::eyre::eyre;
 use color_eyre::Result;
 use dal::action::dependency_graph::ActionDependencyGraph;
 use dal::action::{Action, ActionState};
-use dal::{ChangeSet, DalContext};
+use dal::{ChangeSet, DalContext, Func, Schema, SchemaVariant};
 
 use crate::helpers::generate_fake_name;
 
@@ -59,7 +59,30 @@ impl ChangeSetTestHelpers {
     /// Applies the current [`ChangeSet`] to its base [`ChangeSet`]. Then, it updates the snapshot
     /// to the visibility without using an editing [`ChangeSet`]. In other words, the resulting,
     /// snapshot is "HEAD" without an editing [`ChangeSet`].
+    /// Also locks existing editing funcs and schema variants to mimic SDF
     pub async fn apply_change_set_to_base(ctx: &mut DalContext) -> Result<()> {
+        // Lock all unlocked variants
+        for schema_id in Schema::list_ids(ctx).await? {
+            let schema = Schema::get_by_id(ctx, schema_id).await?;
+            let Some(variant) = SchemaVariant::get_unlocked_for_schema(ctx, schema_id).await?
+            else {
+                continue;
+            };
+
+            let variant_id = variant.id();
+
+            variant.lock(ctx).await?;
+            schema.set_default_schema_variant(ctx, variant_id).await?;
+        }
+        // Lock all unlocked functions too
+        for func in Func::list_for_default_and_editing(ctx).await? {
+            if !func.is_locked {
+                func.lock(ctx).await?;
+            }
+        }
+
+        Self::commit_and_update_snapshot_to_visibility(ctx).await?;
+
         let applied_change_set = ChangeSet::apply_to_base_change_set(ctx).await?;
 
         Self::blocking_commit(ctx).await?;

lib/dal/src/pkg/export.rs

@@ -185,6 +185,7 @@ impl PkgExporter {
         data_builder.display_name(variant.display_name());
 
         data_builder.component_type(get_component_type(ctx, variant).await?);
+        data_builder.description(variant.description());
 
         if let Some(authoring_func_id) =
             overridden_asset_func_id.or_else(|| variant.asset_func_id())

lib/dal/src/schema/variant.rs

@@ -1976,7 +1976,7 @@ impl SchemaVariant {
     }
 
     pub fn generate_version_string() -> String {
-        format!("{}", Utc::now().format("%Y%m%d%H%M%S"))
+        format!("{}", Utc::now().format("%Y%m%d%H%M%S%f"))
     }
 
     /// Lists all default [`SchemaVariantIds`](SchemaVariant) that have a secret definition.

lib/dal/src/schema/variant/authoring.rs

@@ -641,8 +641,7 @@ impl VariantAuthoringClient {
             .ok_or(VariantAuthoringError::PkgMissingSchemaVariant)?;
 
         let mut thing_map = import_only_new_funcs(ctx, pkg.funcs()?).await?;
-
-        Ok(import_schema_variant(
+        let new_schema_variant = import_schema_variant(
             ctx,
             &schema,
             schema_spec.clone(),
@@ -651,7 +650,17 @@ impl VariantAuthoringClient {
             &mut thing_map,
             None,
         )
-        .await?)
+        .await?;
+
+        // need to manually modify the variant to get the new version
+        // as the pkg spec used to generate this new version only has the
+        // old one.
+        Ok(new_schema_variant
+            .modify(ctx, |sv| {
+                sv.version = metadata.version;
+                Ok(())
+            })
+            .await?)
     }
 
     #[allow(clippy::too_many_arguments)]

lib/dal/src/schema/variant/json.rs

@@ -98,6 +98,7 @@ impl SchemaVariantJson {
         if let Some(link) = metadata.link {
             data_builder.try_link(link.as_str())?;
         }
+        data_builder.description(metadata.description);
 
         data_builder.func_unique_id(asset_func_spec_unique_id);
         builder.data(data_builder.build()?);
@@ -165,6 +166,7 @@ impl SchemaVariantJson {
                     link: None,
                     component_type: si_pkg::SchemaVariantSpecComponentType::Component,
                     func_unique_id: "0".into(),
+                    description: None,
                 });
 
         let metadata = SchemaVariantMetadataJson {
@@ -178,7 +180,7 @@ impl SchemaVariantJson {
                 .unwrap_or(DEFAULT_SCHEMA_VARIANT_COLOR.into()),
             component_type: variant_spec_data.component_type.into(),
             link: variant_spec_data.link.as_ref().map(|l| l.to_string()),
-            description: None, // XXX - does this exist?
+            description: variant_spec_data.description.to_owned(),
         };
 
         Ok(metadata)

lib/dal/tests/integration_test/schema/variant/authoring/clone_variant.rs

@@ -40,7 +40,7 @@ async fn clone_variant(ctx: &mut DalContext) {
     assert_eq!(new_schema_variant.category(), existing_variant.category());
     assert_eq!(
         new_schema_variant.display_name(),
-        "dummy-secret".to_string()
+        existing_variant.display_name()
     );
     assert_eq!(
         new_schema_variant

lib/dal/tests/integration_test/schema/variant/authoring/save_variant.rs

@@ -1,6 +1,9 @@
 use dal::func::FuncKind;
 use dal::schema::variant::authoring::VariantAuthoringClient;
-use dal::{ChangeSet, DalContext, Func, FuncBackendResponseType};
+use dal::{
+    ChangeSet, ComponentType, DalContext, Func, FuncBackendResponseType, Schema, SchemaVariant,
+};
+use dal_test::helpers::ChangeSetTestHelpers;
 use dal_test::test;
 
 #[test]
@@ -113,3 +116,190 @@ async fn save_variant(ctx: &mut DalContext) {
         func.code_plaintext().expect("Unable to get code plaintext")
     );
 }
+
+#[test]
+async fn unlock_and_save_variant(ctx: &mut DalContext) {
+    let new_change_set = ChangeSet::fork_head(ctx, "new change set")
+        .await
+        .expect("could not create new change set");
+    ctx.update_visibility_and_snapshot_to_visibility(new_change_set.id)
+        .await
+        .expect("could not update visibility");
+
+    let schema = Schema::find_by_name(ctx, "dummy-secret")
+        .await
+        .expect("unable to get schema")
+        .expect("schema not found");
+    let default_schema_variant = schema
+        .get_default_schema_variant_id(ctx)
+        .await
+        .expect("Unable to find the default schema variant id");
+    let existing_variant = SchemaVariant::get_by_id_or_error(
+        ctx,
+        default_schema_variant.expect("unable to unwrap schema variant id"),
+    )
+    .await
+    .expect("unable to lookup the default schema variant");
+
+    assert!(default_schema_variant.is_some());
+
+    let unlocked_schema_variant =
+        VariantAuthoringClient::create_unlocked_variant_copy(ctx, existing_variant.id())
+            .await
+            .expect("could not create unlocked copy");
+    // new variant is unlocked
+    assert!(!unlocked_schema_variant.is_locked());
+    // data matches
+    assert_eq!(
+        unlocked_schema_variant.category(),
+        existing_variant.category()
+    );
+    assert_eq!(
+        unlocked_schema_variant.display_name(),
+        existing_variant.display_name()
+    );
+    assert_eq!(
+        unlocked_schema_variant.description(),
+        existing_variant.description()
+    );
+    assert_eq!(unlocked_schema_variant.color(), existing_variant.color());
+    assert_eq!(
+        unlocked_schema_variant.component_type(),
+        existing_variant.component_type()
+    );
+
+    assert_eq!(
+        unlocked_schema_variant
+            .get_asset_func(ctx)
+            .await
+            .expect("could not get asset func")
+            .code_base64,
+        existing_variant
+            .get_asset_func(ctx)
+            .await
+            .expect("could not get asset func")
+            .code_base64
+    );
+
+    // unlocked variant has a newer version
+    assert!(
+        *unlocked_schema_variant.version().to_string() > *existing_variant.version().to_string()
+    );
+
+    // now let's change some stuff
+
+    let new_description = Some("fancy new description".to_string());
+    let new_display_name = "fancy display name too".to_string();
+    let new_category = "Fancy";
+    let new_color = "#191919";
+    let new_link = Some("https://fancy.ai".to_string());
+
+    VariantAuthoringClient::save_variant_content(
+        ctx,
+        unlocked_schema_variant.id,
+        unlocked_schema_variant
+            .schema(ctx)
+            .await
+            .expect("could not get schema")
+            .name,
+        new_display_name.clone(),
+        new_category,
+        new_description.clone(),
+        new_link.clone(),
+        new_color,
+        ComponentType::ConfigurationFrameDown,
+        unlocked_schema_variant
+            .get_asset_func(ctx)
+            .await
+            .expect("could not get asset func")
+            .code_plaintext()
+            .expect("got the code"),
+    )
+    .await
+    .expect("could not save variant content");
+
+    // commit changes
+    ChangeSetTestHelpers::commit_and_update_snapshot_to_visibility(ctx)
+        .await
+        .expect("could not commit");
+    // apply change set and check head
+    ChangeSetTestHelpers::apply_change_set_to_base(ctx)
+        .await
+        .expect("could not apply to head");
+
+    // check head and make sure everything looks good
+    let schema = Schema::find_by_name(ctx, "dummy-secret")
+        .await
+        .expect("unable to get schema")
+        .expect("schema not found");
+    let default_schema_variant = schema
+        .get_default_schema_variant_id(ctx)
+        .await
+        .expect("Unable to find the default schema variant id");
+    let new_merged_variant = SchemaVariant::get_by_id_or_error(
+        ctx,
+        default_schema_variant.expect("unable to unwrap schema variant id"),
+    )
+    .await
+    .expect("unable to lookup the default schema variant");
+
+    assert!(default_schema_variant.is_some());
+    // schema variant ids should match
+    assert_eq!(new_merged_variant.id(), unlocked_schema_variant.id());
+    // new variant is now locked
+    assert!(new_merged_variant.is_locked());
+
+    // data matches
+
+    assert_eq!(new_merged_variant.category(), new_category);
+    assert_eq!(new_merged_variant.display_name(), new_display_name);
+    assert_eq!(new_merged_variant.description(), new_description);
+    assert_eq!(new_merged_variant.color(), new_color);
+    assert_eq!(
+        unlocked_schema_variant
+            .get_asset_func(ctx)
+            .await
+            .expect("could not get asset func")
+            .code_base64,
+        new_merged_variant
+            .get_asset_func(ctx)
+            .await
+            .expect("could not get asset func")
+            .code_base64
+    );
+
+    assert_eq!(
+        new_merged_variant.component_type(),
+        ComponentType::ConfigurationFrameDown
+    );
+
+    // merged variant has a newer version
+    assert!(new_merged_variant.version() > unlocked_schema_variant.version());
+
+    // create a new changeset, and unlock another copy
+
+    ChangeSetTestHelpers::fork_from_head_change_set_with_name(ctx, "change set 2")
+        .await
+        .expect("could not fork head");
+
+    let second_editing_variant =
+        VariantAuthoringClient::create_unlocked_variant_copy(ctx, new_merged_variant.id())
+            .await
+            .expect("could not unlock schema variant");
+
+    // unlocked variant is unlocked
+    assert!(!second_editing_variant.is_locked());
+
+    // data matches
+    assert_eq!(
+        second_editing_variant.component_type(),
+        ComponentType::ConfigurationFrameDown
+    );
+    assert_eq!(second_editing_variant.description(), new_description);
+    assert_eq!(second_editing_variant.category(), new_category);
+    assert_eq!(second_editing_variant.display_name(), new_display_name);
+    assert_eq!(second_editing_variant.color(), new_color);
+
+    // version of newly unlocked variant is newer
+    assert!(second_editing_variant.version() > new_merged_variant.version());
+}

lib/si-pkg/src/node/schema_variant.rs

@@ -4,8 +4,8 @@ use std::{
 };
 
 use object_tree::{
-    read_key_value_line, read_key_value_line_opt, write_key_value_line, GraphError, NameStr,
-    NodeChild, NodeKind, NodeWithChildren, ReadBytes, WriteBytes,
+    read_key_value_line, read_key_value_line_opt, write_key_value_line, write_key_value_line_opt,
+    GraphError, NameStr, NodeChild, NodeKind, NodeWithChildren, ReadBytes, WriteBytes,
 };
 use url::Url;
 
@@ -19,6 +19,7 @@ const KEY_VERSION_STR: &str = "name"; // This got renamed to Version, but we kep
 const KEY_COMPONENT_TYPE_STR: &str = "component_type";
 const KEY_FUNC_UNIQUE_ID_STR: &str = "func_unique_id";
 const KEY_IS_BUILTIN_STR: &str = "is_builtin";
+const KEY_DESCRIPTION_STR: &str = "description";
 
 #[derive(Clone, Debug)]
 pub struct SchemaVariantData {
@@ -27,6 +28,7 @@ pub struct SchemaVariantData {
     pub color: Option<String>,
     pub component_type: SchemaVariantSpecComponentType,
     pub func_unique_id: String,
+    pub description: Option<String>,
 }
 
 #[derive(Clone, Debug)]
@@ -60,6 +62,7 @@ impl WriteBytes for SchemaVariantNode {
                 KEY_FUNC_UNIQUE_ID_STR,
                 data.func_unique_id.to_string(),
             )?;
+            write_key_value_line_opt(writer, KEY_DESCRIPTION_STR, data.description.as_deref())?;
         }
 
         write_common_fields(writer, self.unique_id.as_deref(), self.deleted)?;
@@ -94,13 +97,15 @@ impl ReadBytes for SchemaVariantNode {
                     .map_err(GraphError::parse)?;
 
                 let func_unique_id = read_key_value_line(reader, KEY_FUNC_UNIQUE_ID_STR)?;
+                let description = read_key_value_line_opt(reader, KEY_DESCRIPTION_STR)?;
 
                 Some(SchemaVariantData {
                     version: version.to_owned(),
                     link,
                     color,
                     component_type,
                     func_unique_id,
+                    description,
                 })
             }
             None => None,
@@ -168,6 +173,7 @@ impl NodeChild for SchemaVariantSpec {
                     color: data.color.as_ref().cloned(),
                     component_type: data.component_type,
                     func_unique_id: data.func_unique_id.to_owned(),
+                    description: data.description.to_owned(),
                 }),
                 unique_id: self.unique_id.to_owned(),
                 deleted: self.deleted,