Skip to content

Commit ebfbb90

Browse files
committed
Code review cleanup
1 parent 9b01dd2 commit ebfbb90

8 files changed

Lines changed: 117 additions & 23 deletions

File tree

document/graph-storage/src/delta.rs

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,9 @@ pub fn compute_deltas(from: &Registry, to: &Registry) -> Vec<RegistryDelta> {
4141
let from_node = &from.node_instances[&node_id];
4242
let to_node = &to.node_instances[&node_id];
4343

44-
// No `ChangeImplementation` op; the only path is remove + re-add. Same for input-count changes.
45-
let structural_change = !nodes_have_same_implementation(from_node, to_node) || from_node.inputs.len() != to_node.inputs.len();
44+
// No `ChangeImplementation` op; the only path is remove + re-add. Same for input-count and
45+
// containing-network changes (a moved node has no in-place op either).
46+
let structural_change = !nodes_have_same_implementation(from_node, to_node) || from_node.inputs.len() != to_node.inputs.len() || from_node.network != to_node.network;
4647
if structural_change {
4748
deltas.push(RegistryDelta::RemoveNode { node_id, snapshot: from_node.clone() });
4849
deltas.push(RegistryDelta::AddNode { node_id, node: to_node.clone() });
@@ -111,6 +112,11 @@ pub fn compute_deltas(from: &Registry, to: &Registry) -> Vec<RegistryDelta> {
111112
deltas.push(RegistryDelta::ChangeDocumentAttribute { delta });
112113
}
113114

115+
// Public library export list (whole-list LWW).
116+
if from.exported_nodes != to.exported_nodes {
117+
deltas.push(RegistryDelta::SetExportedNodes { nodes: to.exported_nodes.clone() });
118+
}
119+
114120
compute_resource_deltas(from, to, &mut deltas);
115121

116122
deltas

document/graph-storage/src/from_runtime.rs

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -219,20 +219,28 @@ fn collect_referenced_resources(network: &NodeNetwork) -> std::collections::Hash
219219
}
220220

221221
fn collect_referenced_resources_inner(network: &NodeNetwork, referenced: &mut std::collections::HashSet<ResourceId>) {
222+
for export in &network.exports {
223+
collect_input_resource(export, referenced);
224+
}
225+
222226
for node in network.nodes.values() {
223227
for input in &node.inputs {
224-
if let GraphCraftNodeInput::Value { tagged_value, .. } = input
225-
&& let TaggedValue::Resource(id) = &**tagged_value
226-
{
227-
referenced.insert(*id);
228-
}
228+
collect_input_resource(input, referenced);
229229
}
230230
if let DocumentNodeImplementation::Network(nested) = &node.implementation {
231231
collect_referenced_resources_inner(nested, referenced);
232232
}
233233
}
234234
}
235235

236+
fn collect_input_resource(input: &GraphCraftNodeInput, referenced: &mut std::collections::HashSet<ResourceId>) {
237+
if let GraphCraftNodeInput::Value { tagged_value, .. } = input
238+
&& let TaggedValue::Resource(id) = &**tagged_value
239+
{
240+
referenced.insert(*id);
241+
}
242+
}
243+
236244
/// Register a proto-node declaration as a content-addressed resource: a single `DataSource::Embedded`
237245
/// source resolved to `hash`. The bytes themselves are persisted by the caller's byte store.
238246
fn register_declaration_resource(registry: &mut Registry, id: ResourceId, hash: ResourceHash, peer: PeerId) {
@@ -443,9 +451,7 @@ fn write_ui_input_attributes<M: NodeMetadataSource + ?Sized>(
443451

444452
non_empty_string(UI_INPUT_NAME, metadata.input_name(metadata_path, runtime_node_id, input_index), attributes);
445453
non_empty_string(UI_INPUT_DESCRIPTION, metadata.input_description(metadata_path, runtime_node_id, input_index), attributes);
446-
if let Some(widget) = metadata.widget_override(metadata_path, runtime_node_id, input_index) {
447-
attributes.set(UI_WIDGET_OVERRIDE, serde_json::Value::String(widget.to_string()), timestamp);
448-
}
454+
non_empty_string(UI_WIDGET_OVERRIDE, metadata.widget_override(metadata_path, runtime_node_id, input_index), attributes);
449455

450456
for (sub_key, value) in metadata.input_data(metadata_path, runtime_node_id, input_index) {
451457
attributes.set(&format!("{UI_INPUT_DATA_PREFIX}{sub_key}"), value, timestamp);

document/graph-storage/src/lib.rs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,10 @@ pub enum Position {
8484
/// Root network ID. The renderable graph lives in `networks[&ROOT_NETWORK]`.
8585
pub const ROOT_NETWORK: NetworkId = 0;
8686

87+
/// Upper bound on a network's export slot count, guarding `SetExport` against a malicious or corrupted
88+
/// slot index forcing an unbounded `exports` allocation.
89+
const MAX_EXPORT_SLOTS: usize = 1 << 16;
90+
8791
#[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize)]
8892
pub struct Registry {
8993
pub node_instances: HashMap<NodeId, Node>,
@@ -1480,6 +1484,9 @@ impl Document {
14801484
let slot_idx = slot as usize;
14811485

14821486
if slot_idx >= net.exports.len() {
1487+
if slot_idx >= MAX_EXPORT_SLOTS {
1488+
return Err(CrdtError::ExportSlotOutOfBounds);
1489+
}
14831490
net.exports.resize(
14841491
slot_idx + 1,
14851492
ExportSlot {
@@ -1795,6 +1802,8 @@ pub enum CrdtError {
17951802
NetworkDoesNotExist,
17961803
#[error("Input index out of bounds")]
17971804
InputIndexOutOfBounds,
1805+
#[error("Export slot index out of bounds")]
1806+
ExportSlotOutOfBounds,
17981807
#[error("Delta not found in history")]
17991808
NotFoundInHistory,
18001809
#[error("Nothing to undo")]

document/graph-storage/src/to_runtime.rs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ pub enum ConversionError {
2222
DeclarationNotFound(ResourceId),
2323
#[error("Deserialization error: {0}")]
2424
DeserializationError(String),
25+
#[error("Node {node:?} has {inputs} inputs but {attributes} input-attribute entries")]
26+
InputAttributeCountMismatch { node: NodeId, inputs: usize, attributes: usize },
2527
}
2628

2729
/// Resolved proto-node declarations, keyed by the `ResourceId` that `Implementation::ProtoNode`
@@ -99,6 +101,14 @@ fn convert_network(
99101
let local_id = node.attributes.get(ORIGINAL_NODE_ID).and_then(|v| v.value.as_u64()).unwrap_or(global_id);
100102
let runtime_id = RuntimeNodeId(local_id);
101103

104+
if node.inputs.len() != node.inputs_attributes.len() {
105+
return Err(ConversionError::InputAttributeCountMismatch {
106+
node: global_id,
107+
inputs: node.inputs.len(),
108+
attributes: node.inputs_attributes.len(),
109+
});
110+
}
111+
102112
if let Some(collector) = node_collector.as_mut()
103113
&& let Some(entry) = extract_ui_metadata(node, metadata_path, runtime_id)
104114
{

node-graph/graph-craft/src/document.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,7 @@ impl DocumentNode {
172172
}
173173

174174
/// Represents the possible inputs to a node.
175-
#[derive(Debug, Clone, PartialEq, Hash, core_types::CacheHash, DynAny, serde::Serialize, serde::Deserialize)]
175+
#[derive(Debug, Clone, PartialEq, core_types::CacheHash, DynAny, serde::Serialize, serde::Deserialize)]
176176
pub enum NodeInput {
177177
/// A reference to another node in the same network from which this node can receive its input.
178178
Node { node_id: NodeId, output_index: usize },

node-graph/graph-craft/src/document/value.rs

Lines changed: 72 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -679,12 +679,19 @@ fn unwrap_legacy_table_payloads(mut value: serde_json::Value) -> serde_json::Val
679679
*payload = elements.into_iter().next().unwrap_or(serde_json::Value::Null);
680680
}
681681
}
682-
"Gradient" | "GradientTable" | "GradientPositions" => {
683-
if let Some(mut elements) = legacy_elements(payload)
684-
&& let Some(first) = elements.drain(..).next()
682+
"Gradient" | "GradientTable" | "GradientPositions" | "GradientStops" => {
683+
if let Some(elements) = legacy_elements(payload)
684+
&& let Some(first) = elements.into_iter().next()
685685
{
686686
*payload = first;
687687
}
688+
689+
// Pre-midpoint documents stored stops as a `[(position, color), ...]` array; convert to the struct form.
690+
if let Some(stops) = payload.as_array()
691+
&& let Some(converted) = tuple_stops_to_struct(stops)
692+
{
693+
*payload = converted;
694+
}
688695
}
689696
"BrushStrokes" | "BrushStrokeTable" => {
690697
if let Some(elements) = legacy_elements(payload) {
@@ -696,6 +703,31 @@ fn unwrap_legacy_table_payloads(mut value: serde_json::Value) -> serde_json::Val
696703
value
697704
}
698705

706+
/// Converts the pre-midpoint `[(position, color), ...]` gradient-stops encoding into the modern
707+
/// `{ position, midpoint, color }` struct form. Returns `None` if the array isn't in that tuple
708+
/// shape (so an already-migrated struct payload, which deserializes as an object, is left alone).
709+
/// Midpoints default to `0.5`, matching the old hand-written `GradientStops` deserializer.
710+
#[cfg(feature = "loading")]
711+
fn tuple_stops_to_struct(stops: &[serde_json::Value]) -> Option<serde_json::Value> {
712+
let mut positions = Vec::with_capacity(stops.len());
713+
let mut colors = Vec::with_capacity(stops.len());
714+
715+
for stop in stops {
716+
let pair = stop.as_array()?;
717+
let [position, color] = pair.as_slice() else { return None };
718+
positions.push(position.clone());
719+
colors.push(color.clone());
720+
}
721+
722+
let midpoints = vec![serde_json::Value::from(0.5); stops.len()];
723+
724+
Some(serde_json::json!({
725+
"position": positions,
726+
"midpoint": midpoints,
727+
"color": colors,
728+
}))
729+
}
730+
699731
impl Display for TaggedValue {
700732
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
701733
match self {
@@ -835,3 +867,40 @@ mod typedefault_dispatch {
835867
for_each_type_default!(check);
836868
}
837869
}
870+
871+
#[cfg(all(test, feature = "loading"))]
872+
mod legacy_gradient_migration {
873+
use super::unwrap_legacy_table_payloads;
874+
use serde_json::json;
875+
876+
/// Pre-midpoint documents stored gradient stops as `[(position, color), ...]`. After dropping the
877+
/// hand-written `GradientStops` deserializer, the migration layer must convert that tuple form into
878+
/// the `{ position, midpoint, color }` struct shape so old documents still load.
879+
#[test]
880+
fn bare_tuple_stops_become_struct() {
881+
let value = json!({ "Gradient": [[0.0, "red"], [1.0, "blue"]] });
882+
883+
let migrated = unwrap_legacy_table_payloads(value);
884+
885+
assert_eq!(migrated, json!({ "Gradient": { "position": [0.0, 1.0], "midpoint": [0.5, 0.5], "color": ["red", "blue"] } }));
886+
}
887+
888+
/// The `LegacyTable`-wrapped tuple form (a one-row table holding the tuple-stops value) must unwrap
889+
/// the table first, then migrate the inner tuple array.
890+
#[test]
891+
fn table_wrapped_tuple_stops_become_struct() {
892+
let value = json!({ "GradientTable": { "element": [[[0.0, "red"], [1.0, "blue"]]] } });
893+
894+
let migrated = unwrap_legacy_table_payloads(value);
895+
896+
assert_eq!(migrated, json!({ "GradientTable": { "position": [0.0, 1.0], "midpoint": [0.5, 0.5], "color": ["red", "blue"] } }));
897+
}
898+
899+
/// An already-migrated struct payload must pass through untouched.
900+
#[test]
901+
fn struct_stops_pass_through() {
902+
let value = json!({ "Gradient": { "position": [0.0], "midpoint": [0.5], "color": ["red"] } });
903+
904+
assert_eq!(unwrap_legacy_table_payloads(value.clone()), value);
905+
}
906+
}

node-graph/graph-craft/src/proto.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ impl Hash for ConstructionArgs {
108108
node.hash(state);
109109
}
110110
}
111-
Self::Value(value) => value.hash(state),
111+
Self::Value(value) => value.cache_hash(state),
112112
Self::Inline(inline) => inline.hash(state),
113113
}
114114
}

node-graph/libraries/core-types/src/memo.rs

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,8 @@ pub struct MemoHash<T: CacheHash> {
2020
// Equality and ordering compare the *value*, never the cache `hash`. For most types the hash is a
2121
// pure function of the value (so it's redundant in comparisons), but some types hash a fresh UUID to
2222
// force cache invalidation (e.g. `VectorModification`); including their hash would make structurally
23-
// identical values compare unequal. Cache invalidation rides the `Hash`/`CacheHash` impls (which do
24-
// read `self.hash`), so excluding it here doesn't affect dedup behavior.
23+
// identical values compare unequal. There is deliberately no `std::hash::Hash` impl, since reading
24+
// `self.hash` would contradict this value-only `eq`; cache-identity hashing goes through `CacheHash`.
2525
impl<T: CacheHash + PartialEq> PartialEq for MemoHash<T> {
2626
fn eq(&self, other: &Self) -> bool {
2727
self.value == other.value
@@ -92,12 +92,6 @@ impl<T: CacheHash> From<T> for MemoHash<T> {
9292
}
9393
}
9494

95-
impl<T: CacheHash> Hash for MemoHash<T> {
96-
fn hash<H: Hasher>(&self, state: &mut H) {
97-
self.hash.hash(state)
98-
}
99-
}
100-
10195
impl<T: CacheHash> CacheHash for MemoHash<T> {
10296
fn cache_hash<H: Hasher>(&self, state: &mut H) {
10397
self.hash.hash(state);

0 commit comments

Comments
 (0)