Skip to content

fix: Disallow subscribing to values of the wrong kind #6934

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 15, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions app/docs/src/reference/public-api.md
Original file line number Diff line number Diff line change
Expand Up @@ -4984,7 +4984,6 @@ xor
{
"component": "ComponentName",
"function": "string",
"keepExistingSubscriptions": true,
"propPath": "string"
}

Expand All @@ -5004,7 +5003,6 @@ and
|---|---|---|---|---|
|*anonymous*|object|false|none|none|
|» function|string|false|none|none|
|» keepExistingSubscriptions|boolean,null|false|none|none|
|» propPath|string|true|none|none|

## [SystemStatusResponse](#tocS_SystemStatusResponse)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -942,7 +942,6 @@ const processHighlighted = () => {
$source: {
component: from.componentId,
path: from.attributePath,
keepExistingSubscriptions: appendConnection.value,
},
},
});
Expand All @@ -953,16 +952,13 @@ const processHighlighted = () => {
focusOnInput();
};

const appendConnection = ref(false);

// Modal Mgmt
function open(initialState: ConnectionMenuData) {
highlightedIndex.value = 0;
doneOpening.value = false;
activeSide.value = "a";
connectionData.A = {};
connectionData.B = {};
appendConnection.value = initialState.appendConnection ?? false;

if (inputBRef.value) {
inputBRef.value.searchString = "";
Expand Down
31 changes: 8 additions & 23 deletions app/web/src/store/components.store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -341,19 +341,6 @@ type EventBusEvents = {
// }
// }
//
//
// SOON TO BE DEPRECATED: You may also APPEND a subscription by adding `keepExistingSubscriptions: true` to the
// subscription:
//
// {
// "/domain/SubnetId": {
// "$source": { "component": "ComponentNameOrId", "path": "/resource/SubnetId", keepExistingSubscriptions: true }
// }
// }
//
// If you do this, the subscription will be added to the list if it's not already there, and
// any other subscriptions will also be kept.
//
// - ESCAPE HATCH for setting a value: setting an attribute to `{ "$source": { "value": <value> } }`
// has the same behavior as all the above cases. The reason this exists is, if you happen to
// have an object with a "$source" key, the existing interface would treat that as an error.
Expand All @@ -380,7 +367,6 @@ type AttributeSourceSetSubscription = {
$source: {
component: ComponentId | ComponentName;
path: AttributePath;
keepExistingSubscriptions?: boolean;
func?: string;
};
};
Expand Down Expand Up @@ -1409,15 +1395,14 @@ export const useComponentsStore = (forceChangeSetId?: ChangeSetId) => {
this.rawEdgesById[newEdge.id] = newEdge;
this.processRawEdge(newEdge.id);

const edgesBeingReplaced = update.$source
.keepExistingSubscriptions
? []
: Object.values(this.rawEdgesById).filter(
(e) =>
isSubscriptionEdge(e) &&
e.toAttributePath === toPath &&
e.toComponentId === componentId,
);
const edgesBeingReplaced = Object.values(
this.rawEdgesById,
).filter(
(e) =>
isSubscriptionEdge(e) &&
e.toAttributePath === toPath &&
e.toComponentId === componentId,
);

// TODO Bring back the edges that were deleted by the optimistic call on the callback
// Or don't, this won't live much anyway
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import re # noqa: F401
import json

from pydantic import BaseModel, ConfigDict, Field, StrictBool, StrictStr
from pydantic import BaseModel, ConfigDict, Field, StrictStr
from typing import Any, ClassVar, Dict, List, Optional
from typing import Optional, Set
from typing_extensions import Self
Expand All @@ -29,9 +29,8 @@ class Subscription(BaseModel):
component: StrictStr
component_id: StrictStr = Field(alias="componentId")
function: Optional[StrictStr] = None
keep_existing_subscriptions: Optional[StrictBool] = Field(default=None, alias="keepExistingSubscriptions")
prop_path: StrictStr = Field(alias="propPath")
__properties: ClassVar[List[str]] = ["component", "componentId", "function", "keepExistingSubscriptions", "propPath"]
__properties: ClassVar[List[str]] = ["component", "componentId", "function", "propPath"]

model_config = ConfigDict(
populate_by_name=True,
Expand Down Expand Up @@ -72,11 +71,6 @@ def to_dict(self) -> Dict[str, Any]:
exclude=excluded_fields,
exclude_none=True,
)
# set to None if keep_existing_subscriptions (nullable) is None
# and model_fields_set contains the field
if self.keep_existing_subscriptions is None and "keep_existing_subscriptions" in self.model_fields_set:
_dict['keepExistingSubscriptions'] = None

return _dict

@classmethod
Expand All @@ -92,7 +86,6 @@ def from_dict(cls, obj: Optional[Dict[str, Any]]) -> Optional[Self]:
"component": obj.get("component"),
"componentId": obj.get("componentId"),
"function": obj.get("function"),
"keepExistingSubscriptions": obj.get("keepExistingSubscriptions"),
"propPath": obj.get("propPath")
})
return _obj
Expand Down
1 change: 0 additions & 1 deletion generated-sdks/python/test/test_subscription.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ def make_instance(self, include_optional) -> Subscription:
component = '',
component_id = '01H9ZQD35JPMBGHH69BT0Q79VY',
function = '',
keep_existing_subscriptions = True,
prop_path = ''
)
else:
Expand Down
6 changes: 0 additions & 6 deletions generated-sdks/typescript/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2221,12 +2221,6 @@ export interface Subscription {
* @memberof Subscription
*/
'function'?: string;
/**
*
* @type {boolean}
* @memberof Subscription
*/
'keepExistingSubscriptions'?: boolean | null;
/**
*
* @type {string}
Expand Down
18 changes: 7 additions & 11 deletions lib/dal-test/src/helpers/attribute/value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,30 +33,26 @@ pub async fn vivify(ctx: &DalContext, key: impl AttributeValueKey) -> Result<Att
}

/// Set the subscriptions on a value
pub async fn subscribe<S: AttributeValueKey>(
pub async fn subscribe(
ctx: &DalContext,
subscriber: impl AttributeValueKey,
subscriptions: impl IntoIterator<Item = S>,
subscription: impl AttributeValueKey,
) -> Result<()> {
subscribe_with_custom_function(ctx, subscriber, subscriptions, None).await
subscribe_with_custom_function(ctx, subscriber, subscription, None).await
}

/// Set the subscriptions on a value
pub async fn subscribe_with_custom_function<S: AttributeValueKey>(
pub async fn subscribe_with_custom_function(
ctx: &DalContext,
subscriber: impl AttributeValueKey,
subscriptions: impl IntoIterator<Item = S>,
subscription: impl AttributeValueKey,
func_id: Option<FuncId>,
) -> Result<()> {
let subscriber = vivify(ctx, subscriber).await?;
let mut converted_subscriptions = vec![];
for subscription in subscriptions {
converted_subscriptions.push(AttributeValueKey::to_subscription(ctx, subscription).await?);
}
AttributeValue::set_to_subscriptions(
AttributeValue::set_to_subscription(
ctx,
subscriber,
converted_subscriptions,
AttributeValueKey::to_subscription(ctx, subscription).await?,
func_id,
Reason::new_user_added(ctx),
)
Expand Down
42 changes: 9 additions & 33 deletions lib/dal/src/attribute/attributes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,18 +159,6 @@ pub struct AttributeUpdateCounts {
/// }
/// }
///
/// SOON TO BE DEPRECATED: You may also APPEND a subscription by adding `keepExistingSubscriptions: true` to the
/// subscription:
///
/// {
/// "/domain/SubnetId": {
/// "$source": { "component": "ComponentNameOrId", "path": "/resource/SubnetId", keepExistingSubscriptions: true }
/// }
/// }
///
/// If you do this, the subscription will be added to the list if it's not already there, and
/// any other subscriptions will also be kept.
///
/// - ESCAPE HATCH for setting a value: setting an attribute to `{ "$source": { "value": <value> } }`
/// has the same behavior as all the above cases. The reason this exists is, if you happen to
/// have an object with a "$source" key, the existing interface would treat that as an error.
Expand Down Expand Up @@ -223,8 +211,8 @@ pub async fn update_attributes(
Source::Subscription {
component: source_component,
path: source_path,
keep_existing_subscriptions,
func: func_ident,
_keep_existing_subscriptions,
} => {
counts.subscription_count += 1;

Expand All @@ -243,33 +231,17 @@ pub async fn update_attributes(
path: AttributePath::from_json_pointer(source_path),
};

// Make sure the subscribed-to path is valid (i.e. it doesn't have to resolve
// to a value *right now*, but it must be a valid path to the schema as it
// exists--correct prop names, numeric indices for arrays, etc.)
subscription.validate(ctx).await?;

// TODO remove keep_existing_subscriptions so we can only have an av subscribe to single source
// Add our subscription unless the subscription is already there
let existing_subscriptions = match keep_existing_subscriptions {
Some(true) => AttributeValue::subscriptions(ctx, target_av_id).await?,
Some(false) | None => None,
};
let mut subscriptions = existing_subscriptions.unwrap_or(vec![]);
if !subscriptions.contains(&subscription) {
subscriptions.push(subscription);
}

let maybe_func_id = if let Some(func) = func_ident {
func.resolve(ctx).await?
} else {
None
};

// Subscribe!
AttributeValue::set_to_subscriptions(
AttributeValue::set_to_subscription(
ctx,
target_av_id,
subscriptions,
subscription,
maybe_func_id,
Reason::new_user_added(ctx),
)
Expand Down Expand Up @@ -363,9 +335,13 @@ pub enum Source {
component: ComponentIdent,
path: String,
#[serde(skip_serializing_if = "Option::is_none")]
keep_existing_subscriptions: Option<bool>,
#[serde(skip_serializing_if = "Option::is_none")]
func: Option<FuncIdent>,
// DEPRECATED and ignored. But kept here until we're sure callers are not using it
#[serde(
rename = "keep_existing_subscriptions",
skip_serializing_if = "Option::is_none"
)]
_keep_existing_subscriptions: Option<bool>,
},
}

Expand Down
12 changes: 9 additions & 3 deletions lib/dal/src/attribute/path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,14 @@ impl AttributePath {

/// Validate that the JSON pointer can refer to something real under the given prop.
///
/// Returns the PropId of the referenced path.
///
/// Errors if the JSON pointer refers to a missing field under an object.
pub async fn validate(&self, ctx: &DalContext, prop_id: PropId) -> AttributeValueResult<()> {
pub async fn validate(
&self,
ctx: &DalContext,
prop_id: PropId,
) -> AttributeValueResult<PropId> {
match self {
AttributePath::JsonPointer(pointer) => {
let pointer = jsonptr::Pointer::parse(pointer)
Expand Down Expand Up @@ -270,7 +276,7 @@ async fn validate_json_pointer(
ctx: &DalContext,
mut parent_id: PropId,
pointer: &jsonptr::Pointer,
) -> AttributeValueResult<()> {
) -> AttributeValueResult<PropId> {
// Go through each segment of the JSON pointer (e.g. /foo/bar/0 = foo, bar, 0)
// and validate that the child prop exists or that it is a valid index and not -
for token in pointer {
Expand Down Expand Up @@ -311,5 +317,5 @@ async fn validate_json_pointer(
}
}
}
Ok(())
Ok(parent_id)
}
Loading