From d3f18a511a0e1784dcc681b9faa0a2bcf4bd8824 Mon Sep 17 00:00:00 2001 From: Brit Myers Date: Tue, 16 Jul 2024 01:08:41 -0400 Subject: [PATCH] fix(dal): 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 | 25 ++- lib/dal/src/pkg/export.rs | 1 + lib/dal/src/schema/variant.rs | 2 +- lib/dal/src/schema/variant/authoring.rs | 15 +- lib/dal/src/schema/variant/json.rs | 4 +- .../schema/variant/authoring/clone_variant.rs | 2 +- .../schema/variant/authoring/save_variant.rs | 192 +++++++++++++++++- lib/si-pkg/src/node/schema_variant.rs | 10 +- lib/si-pkg/src/pkg/variant.rs | 7 + lib/si-pkg/src/spec/variant.rs | 2 + 10 files changed, 250 insertions(+), 10 deletions(-) diff --git a/lib/dal-test/src/helpers/change_set.rs b/lib/dal-test/src/helpers/change_set.rs index 7706f100d2..c8249d57d9 100644 --- a/lib/dal-test/src/helpers/change_set.rs +++ b/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?; diff --git a/lib/dal/src/pkg/export.rs b/lib/dal/src/pkg/export.rs index 403f8a62fd..79e5425509 100644 --- a/lib/dal/src/pkg/export.rs +++ b/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()) diff --git a/lib/dal/src/schema/variant.rs b/lib/dal/src/schema/variant.rs index ced8b06669..bb1e583f0e 100644 --- a/lib/dal/src/schema/variant.rs +++ b/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. diff --git a/lib/dal/src/schema/variant/authoring.rs b/lib/dal/src/schema/variant/authoring.rs index 9e2dd4d4cf..96b5ff9c4e 100644 --- a/lib/dal/src/schema/variant/authoring.rs +++ b/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)] diff --git a/lib/dal/src/schema/variant/json.rs b/lib/dal/src/schema/variant/json.rs index fcb448ad5c..560d6596f5 100644 --- a/lib/dal/src/schema/variant/json.rs +++ b/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) diff --git a/lib/dal/tests/integration_test/schema/variant/authoring/clone_variant.rs b/lib/dal/tests/integration_test/schema/variant/authoring/clone_variant.rs index d091a7fd47..9b0c2f4c81 100644 --- a/lib/dal/tests/integration_test/schema/variant/authoring/clone_variant.rs +++ b/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 diff --git a/lib/dal/tests/integration_test/schema/variant/authoring/save_variant.rs b/lib/dal/tests/integration_test/schema/variant/authoring/save_variant.rs index a4b0c0d1ca..80e06b01c2 100644 --- a/lib/dal/tests/integration_test/schema/variant/authoring/save_variant.rs +++ b/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()); +} diff --git a/lib/si-pkg/src/node/schema_variant.rs b/lib/si-pkg/src/node/schema_variant.rs index 5156785835..b8bd4bc830 100644 --- a/lib/si-pkg/src/node/schema_variant.rs +++ b/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, pub component_type: SchemaVariantSpecComponentType, pub func_unique_id: String, + pub description: Option, } #[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,6 +97,7 @@ 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(), @@ -101,6 +105,7 @@ impl ReadBytes for SchemaVariantNode { 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, diff --git a/lib/si-pkg/src/pkg/variant.rs b/lib/si-pkg/src/pkg/variant.rs index bdaca298ea..9988703d49 100644 --- a/lib/si-pkg/src/pkg/variant.rs +++ b/lib/si-pkg/src/pkg/variant.rs @@ -25,6 +25,7 @@ pub struct SiPkgSchemaVariantData { color: Option, component_type: SchemaVariantSpecComponentType, func_unique_id: String, + description: Option, } impl SiPkgSchemaVariantData { @@ -47,6 +48,9 @@ impl SiPkgSchemaVariantData { pub fn func_unique_id(&self) -> &str { self.func_unique_id.as_str() } + pub fn description(&self) -> Option<&str> { + self.description.as_deref() + } } #[derive(Clone, Debug)] @@ -117,6 +121,7 @@ impl<'a> SiPkgSchemaVariant<'a> { color: data.color, component_type: data.component_type, func_unique_id: data.func_unique_id, + description: data.description, }), unique_id: schema_variant_node.unique_id, deleted: schema_variant_node.deleted, @@ -418,6 +423,8 @@ impl<'a> SiPkgSchemaVariant<'a> { if let Some(color) = data.color() { data_builder.color(color); } + + data_builder.description(data.description().map(ToOwned::to_owned)); data_builder.func_unique_id(data.func_unique_id()); builder.data(data_builder.build()?); } diff --git a/lib/si-pkg/src/spec/variant.rs b/lib/si-pkg/src/spec/variant.rs index 6d8f578fc8..5952176bd9 100644 --- a/lib/si-pkg/src/spec/variant.rs +++ b/lib/si-pkg/src/spec/variant.rs @@ -118,6 +118,8 @@ pub struct SchemaVariantSpecData { pub component_type: SchemaVariantSpecComponentType, #[builder(setter(into))] pub func_unique_id: String, + #[builder(setter(into), default)] + pub description: Option, } impl SchemaVariantSpecData {