merge: #3479

3479: fix(dal): make sure you can manually override maps and arrays r=vbustamante a=vbustamante

Previously, the rebaser was skipping any unordered children of ordered containers.

Since an attribute value for a map has an ordering node connection, when computing its children, the rebaser was only considering those edges to those nodes as updates. Since when we do an override we’re binding an attributePrototype directly to the attribute value, without the ordering connection, the rebaser was skipping it.

But this was working fine for string attribute values, for instance, since they are considered unordered containers (they’re not containers at all, but since they don’t have the ordering node that’s how we treat them).
 
This PR also fixes an issue in which adding an ordered node to a fresh graph fails, which was forcing unit tests to be more complex than they needed to be

Co-authored-by: Victor Bustamante <victor@systeminit.com>

Author: si-bors-ng[bot]

app/web/src/components/AttributesPanel/AttributesPanelItem.vue

@@ -308,6 +308,7 @@
         />
         <Icon
           v-if="
+            sourceOverridden &&
             currentValue !== null &&
             !propPopulatedBySocket &&
             !propControlledByParent
@@ -445,7 +446,7 @@
           v-if="
             propControlledByParent ||
             (featureFlagsStore.INDICATORS_MANUAL_FUNCTION_SOCKET &&
-              propPopulatedBySocket &&
+              propSetByDynamicFunc &&
               !editOverride)
           "
           v-tooltip="
@@ -556,8 +557,8 @@
           function from an ancestor prop.
         </template>
         <template v-else>
-          Editing the prop "{{ propName }}" directly will override the socket
-          which is currently setting its value.
+          Editing the prop "{{ propName }}" directly will override the value
+          that is set by a dynamic function.
         </template>
       </div>
       <div class="flex gap-sm">
@@ -751,7 +752,7 @@ const propPopulatedBySocket = computed(
 const propHasSocket = computed(
   () => props.attributeDef.value?.canBeSetBySocket,
 );
-const propSetByFunc = computed(
+const propSetByDynamicFunc = computed(
   () =>
     props.attributeDef.value?.isControlledByDynamicFunc &&
     !propHasSocket.value &&
@@ -762,18 +763,18 @@ const propManual = computed(
     !(
       propPopulatedBySocket.value ||
       propHasSocket.value ||
-      propSetByFunc.value
+      propSetByDynamicFunc.value
     ),
 );
 const propSource = computed<SourceKind>(() => {
   if (propHasSocket.value || propPopulatedBySocket.value) return "via socket";
-  else if (propSetByFunc.value) return "via attribute func";
+  else if (propSetByDynamicFunc.value) return "via attribute func";
   else return "manually";
 });
 
 const sourceIcon = computed(() => {
   if (propPopulatedBySocket.value) return "circle-full";
-  else if (propSetByFunc.value) return "func";
+  else if (propSetByDynamicFunc.value) return "func";
   else if (propHasSocket.value) return "circle-empty";
   else return "cursor";
 });
@@ -784,7 +785,7 @@ const sourceTooltip = computed(() => {
   if (sourceOverridden.value) {
     if (propPopulatedBySocket.value) {
       return `${propName.value} has been overriden to be set via a populated socket`;
-    } else if (propSetByFunc.value) {
+    } else if (propSetByDynamicFunc.value) {
       return `${propName.value} has been overriden to be set by a dynamic function`;
     } else if (propHasSocket.value) {
       return `${propName.value} has been overriden to be set via an empty socket`;
@@ -793,7 +794,7 @@ const sourceTooltip = computed(() => {
   } else {
     if (propPopulatedBySocket.value) {
       return `${propName.value} is set via a populated socket`;
-    } else if (propSetByFunc.value) {
+    } else if (propSetByDynamicFunc.value) {
       return `${propName.value} is set by a dynamic function`;
     } else if (propHasSocket.value) {
       return `${propName.value} is set via an empty socket`;
@@ -876,17 +877,6 @@ function unsetHandler() {
   newValueBoolean.value = false;
   newValueString.value = "";
 
-  // TODO(Wendy) - OLD CODE, REMOVE ONCE RESET_PROPERTY_VALUE IS WIRED UP TO THE BACKEND
-  // attributesStore.UPDATE_PROPERTY_VALUE({
-  //   update: {
-  //     attributeValueId: props.attributeDef.valueId,
-  //     parentAttributeValueId: props.attributeDef.parentValueId,
-  //     propId: props.attributeDef.propId,
-  //     componentId,
-  //     value: null,
-  //   },
-  // });
-
   attributesStore.RESET_PROPERTY_VALUE({
     attributeValueId: props.attributeDef.valueId,
   });
@@ -911,7 +901,7 @@ function updateValue() {
     if (newVal === "" && !currentValue.value) skipUpdate = true;
   }
 
-  // dont trigger an update if the value has not changed
+  // don't trigger an update if the value has not changed
   // (and some special cases handled for specific types)
   if (skipUpdate || newVal === currentValue.value) {
     return;
@@ -970,7 +960,7 @@ const secret = computed(
 const secretDefinitionId = computed(() => {
   if (props.attributeDef.propDef.widgetKind.kind !== "secret") return;
   const widgetOptions = props.attributeDef.propDef.widgetKind.options;
-  // WHAT? this setup doesn't really make sense...
+  // A widget of kind=secret has a single option that points to its secret definition
   const secretKind = _.find(
     widgetOptions,
     (o) => o.label === "secretKind",

lib/dal/src/attribute/prototype.rs

@@ -18,6 +18,8 @@ use si_layer_cache::LayerDbError;
 use telemetry::prelude::*;
 use thiserror::Error;
 
+use crate::attribute::prototype::argument::value_source::ValueSource;
+use crate::attribute::prototype::argument::AttributePrototypeArgument;
 use crate::change_set_pointer::ChangeSetPointerError;
 use crate::layer_db_types::{AttributePrototypeContent, AttributePrototypeContentV1};
 use crate::workspace_snapshot::content_address::{ContentAddress, ContentAddressDiscriminants};
@@ -29,14 +31,17 @@ use crate::workspace_snapshot::node_weight::{
 };
 use crate::workspace_snapshot::WorkspaceSnapshotError;
 use crate::{
-    pk, AttributeValueId, DalContext, FuncId, OutputSocketId, PropId, Timestamp, TransactionsError,
+    pk, AttributeValueId, DalContext, FuncId, InputSocketId, OutputSocketId, PropId, Timestamp,
+    TransactionsError,
 };
 
 pub mod argument;
 
 #[remain::sorted]
 #[derive(Error, Debug)]
 pub enum AttributePrototypeError {
+    #[error("attribute prototype argument error: {0}")]
+    AttributePrototypeArgument(String),
     #[error("change set error: {0}")]
     ChangeSet(#[from] ChangeSetPointerError),
     #[error("edge weight error: {0}")]
@@ -378,4 +383,25 @@ impl AttributePrototype {
 
         Ok(())
     }
+
+    pub async fn list_input_socket_sources_for_id(
+        ctx: &DalContext,
+        ap_id: AttributePrototypeId,
+    ) -> AttributePrototypeResult<Vec<InputSocketId>> {
+        let apa_ids = AttributePrototypeArgument::list_ids_for_prototype(ctx, ap_id)
+            .await
+            .map_err(|e| AttributePrototypeError::AttributePrototypeArgument(e.to_string()))?;
+
+        let mut input_socket_ids = Vec::<InputSocketId>::new();
+        for apa_id in apa_ids {
+            let maybe_value_source = AttributePrototypeArgument::value_source_by_id(ctx, apa_id)
+                .await
+                .map_err(|e| AttributePrototypeError::AttributePrototypeArgument(e.to_string()))?;
+            if let Some(ValueSource::InputSocket(socket_id)) = maybe_value_source {
+                input_socket_ids.push(socket_id);
+            }
+        }
+
+        Ok(input_socket_ids)
+    }
 }

lib/dal/src/attribute/value.rs

@@ -2106,23 +2106,11 @@ impl AttributeValue {
         Ok(())
     }
 
-    pub async fn list_input_sockets_sources_for_id(
+    pub async fn list_input_socket_sources_for_id(
         ctx: &DalContext,
         av_id: AttributeValueId,
     ) -> AttributeValueResult<Vec<InputSocketId>> {
         let prototype_id = Self::prototype_id(ctx, av_id).await?;
-
-        let apa_ids = AttributePrototypeArgument::list_ids_for_prototype(ctx, prototype_id).await?;
-
-        let mut input_socket_ids = Vec::<InputSocketId>::new();
-        for apa_id in apa_ids {
-            let maybe_value_source =
-                AttributePrototypeArgument::value_source_by_id(ctx, apa_id).await?;
-            if let Some(ValueSource::InputSocket(socket_id)) = maybe_value_source {
-                input_socket_ids.push(socket_id);
-            }
-        }
-
-        Ok(input_socket_ids)
+        Ok(AttributePrototype::list_input_socket_sources_for_id(ctx, prototype_id).await?)
     }
 }

lib/dal/src/prop.rs

@@ -24,12 +24,12 @@ use crate::workspace_snapshot::edge_weight::{EdgeWeight, EdgeWeightKind};
 use crate::workspace_snapshot::edge_weight::{EdgeWeightError, EdgeWeightKindDiscriminants};
 use crate::workspace_snapshot::node_weight::{NodeWeight, NodeWeightError};
 use crate::workspace_snapshot::WorkspaceSnapshotError;
-use crate::AttributeValueId;
 use crate::{
     label_list::ToLabelList, pk, property_editor::schema::WidgetKind, AttributePrototype,
     AttributePrototypeId, DalContext, Func, FuncBackendResponseType, FuncId, SchemaVariantId,
     Timestamp, TransactionsError,
 };
+use crate::{AttributeValueId, InputSocketId};
 
 pub const PROP_VERSION: PropContentDiscriminants = PropContentDiscriminants::V1;
 
@@ -762,6 +762,11 @@ impl Prop {
             .into())
     }
 
+    pub async fn input_socket_sources(&self, ctx: &DalContext) -> PropResult<Vec<InputSocketId>> {
+        let prototype_id = Self::prototype_id(ctx, self.id).await?;
+        Ok(AttributePrototype::list_input_socket_sources_for_id(ctx, prototype_id).await?)
+    }
+
     pub async fn set_default_value<T: Serialize>(
         ctx: &DalContext,
         prop_id: PropId,

lib/dal/src/property_editor/schema.rs

@@ -32,7 +32,7 @@ impl PropertyEditorSchema {
         let root_prop_id =
             Prop::find_prop_id_by_path(ctx, schema_variant_id, &PropPath::new(["root"])).await?;
         let root_prop = Prop::get_by_id(ctx, root_prop_id).await?;
-        let root_property_editor_prop = PropertyEditorProp::new(root_prop);
+        let root_property_editor_prop = PropertyEditorProp::new(ctx, root_prop).await?;
         let root_property_editor_prop_id = root_property_editor_prop.id;
         props.insert(root_property_editor_prop_id, root_property_editor_prop);
 
@@ -72,7 +72,7 @@ impl PropertyEditorSchema {
                 // NOTE(nick): we already have the node weight, but I believe we still want to use "get_by_id" to
                 // get the content from the store. Perhaps, there's a more efficient way that we can do this.
                 let child_prop = Prop::get_by_id(ctx, child_prop_id).await?;
-                let child_property_editor_prop = PropertyEditorProp::new(child_prop);
+                let child_property_editor_prop = PropertyEditorProp::new(ctx, child_prop).await?;
 
                 // Load the work queue with the child prop.
                 work_queue.push_back((child_prop_id, child_property_editor_prop.id));
@@ -104,11 +104,14 @@ pub struct PropertyEditorProp {
     pub doc_link: Option<String>,
     pub documentation: Option<String>,
     pub validation_format: Option<String>,
+    pub default_can_be_set_by_socket: bool,
 }
 
 impl PropertyEditorProp {
-    pub fn new(prop: Prop) -> PropertyEditorProp {
-        PropertyEditorProp {
+    pub async fn new(ctx: &DalContext, prop: Prop) -> PropertyEditorResult<PropertyEditorProp> {
+        let default_can_be_set_by_socket = !prop.input_socket_sources(ctx).await?.is_empty();
+
+        Ok(PropertyEditorProp {
             id: prop.id.into(),
             name: prop.name,
             kind: prop.kind.into(),
@@ -119,7 +122,8 @@ impl PropertyEditorProp {
             doc_link: prop.doc_link.map(Into::into),
             documentation: prop.documentation.map(Into::into),
             validation_format: prop.validation_format.map(Into::into),
-        }
+            default_can_be_set_by_socket,
+        })
     }
 }
 

lib/dal/src/property_editor/values.rs

@@ -135,7 +135,7 @@ impl PropertyEditorValues {
                 let child_property_editor_value_id = PropertyEditorValueId::from(child_av_id);
 
                 let sockets_for_av =
-                    AttributeValue::list_input_sockets_sources_for_id(ctx, child_av_id).await?;
+                    AttributeValue::list_input_socket_sources_for_id(ctx, child_av_id).await?;
 
                 let is_from_external_source = sockets_for_av
                     .iter()

lib/dal/src/schema/variant.rs

@@ -621,7 +621,7 @@ impl SchemaVariant {
     /// [`AttributePrototypeArgument`](crate::attribute::prototype::argument::AttributePrototypeArgument)
     /// for the /root/si/color [`Prop`](crate::Prop). If a prototype already
     /// exists pointing to a function other than
-    /// [`IntrinsicFunc::SetString`](`crate::func::intrinsics::IntrinsicFunc::SetString`)
+    /// [`IntrinsicFunc::SetString`](crate::func::intrinsics::IntrinsicFunc::SetString)
     /// we will remove that edge and replace it with one pointing to
     /// `SetString`.
     pub async fn set_color(
@@ -642,7 +642,7 @@ impl SchemaVariant {
     /// [`AttributePrototypeArgument`](crate::attribute::prototype::argument::AttributePrototypeArgument)
     /// for the /root/si/type [`Prop`](crate::Prop). If a prototype already
     /// exists pointing to a function other than
-    /// [`IntrinsicFunc::SetString`](`crate::func::intrinsics::IntrinsicFunc::SetString`)
+    /// [`IntrinsicFunc::SetString`](crate::func::intrinsics::IntrinsicFunc::SetString)
     /// we will remove that edge and replace it with one pointing to
     /// `SetString`.
     pub async fn set_type(
@@ -663,7 +663,7 @@ impl SchemaVariant {
     /// [`AttributePrototypeArgument`](crate::attribute::prototype::argument::AttributePrototypeArgument)
     /// for the /root/si/type [`Prop`](crate::Prop). If a prototype already
     /// exists pointing to a function other than
-    /// [`IntrinsicFunc::SetString`](`crate::func::intrinsics::IntrinsicFunc::SetString`)
+    /// [`IntrinsicFunc::SetString`](crate::func::intrinsics::IntrinsicFunc::SetString)
     /// we will remove that edge and replace it with one pointing to
     /// `SetString`.
     pub async fn get_type(&self, ctx: &DalContext) -> SchemaVariantResult<Option<ComponentType>> {

lib/dal/src/workspace_snapshot/edge_weight.rs

@@ -42,7 +42,7 @@ pub enum EdgeWeightKind {
     Ordinal,
     /// Used to link an attribute value to the prop that it is for.
     Prop,
-    /// An edge from a [`socket`](crate::socket) or an [`AttributeValue`](`crate::AttributeValue`)
+    /// An edge from a [`socket`](crate::socket), [`AttributeValue`](crate::AttributeValue) or [`Prop`](crate::Prop)
     /// to an [`AttributePrototype`](crate::AttributePrototype). The optional [`String`] is used for
     /// maps, arrays and relevant container types to indicate which element the prototype is for.
     Prototype(Option<String>),