Skip to content

Refactor DSL to use nested JSON graph construction#5

Merged
pinodeca merged 1 commit into
mainfrom
pinodeca/nested-graph
Mar 5, 2026
Merged

Refactor DSL to use nested JSON graph construction#5
pinodeca merged 1 commit into
mainfrom
pinodeca/nested-graph

Conversation

@pinodeca
Copy link
Copy Markdown
Contributor

Summary

Refactors the DSL graph construction to use nested JSON instead of inserting nodes into df.nodes during construction. DSL functions now return self-contained JSON that embeds the complete subtree. Node IDs are only generated when df.start() is called, which is the single point where database writes occur.

Motivation

The previous implementation inserted nodes into df.nodes during graph construction (inside df.sql(), df.join(), etc.), causing several problems:

  1. Premature database writes — Graph construction performed I/O before df.start() was called
  2. Orphaned nodes — Errors during graph construction left partial state in the database with no instance_id
  3. No transaction management — Nodes inserted before df.start() did not participate in transaction rollback
  4. Complex explain mode — Required temporary tables and session variables to avoid polluting the database

Design

DSL functions now return self-contained JSON with embedded children instead of database-stored node ID references:

-- df.sql() creates: {"node_type":"SQL","query":"SELECT 1",...}
-- Chaining embeds children directly (no node IDs yet):
SELECT df.sql('SELECT 1') ~> df.sql('SELECT 2');
-- Returns:
-- {
--   "node_type":"THEN",
--   "left_node": {"node_type":"SQL","query":"SELECT 1",...},
--   "right_node": {"node_type":"SQL","query":"SELECT 2",...}
-- }

-- Node IDs are generated only when df.start() inserts into df.nodes
SELECT df.start(df.sql('SELECT 1') ~> df.sql('SELECT 2'));

Key Changes

src/types.rs

  • Durofut struct now uses Option<Box<Durofut>> for left_node and right_node (embedded children instead of ID references)
  • Removed node_id field — IDs are generated during df.start()
  • Removed insert_node() method — no database writes during construction
  • Removed is_explain_mode() — no longer needed
  • ensure() simplified — no node insertion

src/dsl.rs

  • All DSL functions (sql, seq, join, race, if_fn, loop_fn, sleep, http, etc.) updated to embed children directly
  • Removed all insert_node() calls from DSL functions
  • df.start() now contains insert_nodes() which recursively traverses the nested graph, generates node IDs, and inserts all nodes in a single pass
  • df.as_named() no longer writes to database

src/explain.rs

  • Drastically simplified — parses nested JSON directly instead of using temp tables
  • Removed _explain_mode session variable and temp table logic
  • explain_expression() generates temporary IDs (N1, N2, ...) for visualization
  • explain_instance() unchanged (reads from df.nodes for running instances)
  • Now supports explaining plain SQL strings (auto-wrapped with df.sql())

src/types.rs — Strict validation

  • node_type validation added with strict allowlist
  • Config-embedded nodes (IF condition, LOOP condition, JOIN3 extras) now validated during insert_nodes()

Benefits

  • No database writes during construction — Pure string/JSON manipulation
  • Transaction-safe — Only df.start() writes to the database
  • No orphaned nodes — Failed DSL calls leave no state
  • Simple explain mode — Just parse JSON and visualize, no temp tables
  • Stateless — No TLS, no registry, no cleanup required

Testing

  • All existing E2E tests pass without modification
  • Unit tests updated to verify JSON structure
  • cargo clippy and cargo fmt clean

See docs/nested-graph-design.md for full design document including discarded alternatives.

@pinodeca pinodeca force-pushed the pinodeca/nested-graph branch from 084e01d to 224c913 Compare February 17, 2026 23:30
@pinodeca
Copy link
Copy Markdown
Contributor Author

pinodeca commented Feb 17, 2026

Thorough review complete — I found two issues worth addressing before merge.

1) High severity: malformed nested condition_node can bypass validation and persist invalid graph state

Why this matters

For IF/LOOP, malformed condition_node payloads are not consistently rejected. They can be inserted in a shape the runtime does not handle predictably, causing either silent behavior changes (e.g., loop condition not evaluated) or runtime failures.

Evidence

  • Validation currently only errors on string IDs for condition nodes, but not on malformed objects:
    • src/types.rs (validate_recursive) around condition_node handling.
  • Insertion path has the same gap and may silently skip conversion for malformed condition data:
    • src/dsl.rs (start -> insert_nodes) IF/LOOP condition_node conversion logic.
  • Runtime expects condition_node to be a string node id:
    • src/orchestrations/execute_function_graph.rs (execute_if_node / execute_loop_node) using config["condition_node"].as_str().

Suggested fix

In both validation and insertion paths, reject any condition_node value for IF/LOOP that is not a valid Durofut object (or, after conversion, a string node id). Don't silently continue on parse failures.

✅ Resolution

Fixed. Both validate_recursive and insert_nodes now use for_each_config_child() / transform_config_children() — unified visitor methods on Durofut in src/types.rs that reject any condition_node (or extra_nodes entry) that cannot be deserialized as a valid Durofut, returning an error with a descriptive message. Added unit tests (test_validate_rejects_malformed_condition_node_object, test_validate_rejects_condition_node_string_id, test_validate_rejects_malformed_extra_nodes) and E2E test 33_malformed_condition_node.sql.


2) Medium severity: misleading error message in strict parsing path

Why this matters

ensure_strict can report Unknown node_type even when the node type is valid but the JSON structure is invalid. This makes debugging and migration harder.

Evidence

  • src/types.rs ensure_strict: on deserialization failure, any JSON containing node_type is reported as unknown node type, regardless of whether the value is actually valid.

Suggested fix

Differentiate:

  • unknown node type,
  • malformed Durofut JSON (shape/type mismatch),
  • non-JSON SQL text.

✅ Resolution

Fixed. ensure_strict now checks VALID_NODE_TYPES on the fallback path: if node_type is valid but the structure is malformed, it reports "Malformed Durofut JSON with node_type 'X': <serde error>" instead of "Unknown node_type". Added unit test test_ensure_strict_malformed_structure_valid_type.


Test coverage gap

Current negative-path E2E (tests/e2e/sql/28_invalid_node_type.sql) covers unknown top-level node type, but not malformed nested condition_node cases. I recommend adding at least one E2E test for malformed nested condition configuration.

✅ Resolution

Added E2E test 33_malformed_condition_node.sql covering malformed condition_node in both IF and LOOP nodes, plus unit tests for deeply nested invalid node types (test_validate_rejects_deeply_nested_invalid_node_type), IF condition config structure (test_if_condition_embedded_in_config), and JOIN3 extra_nodes structure (test_join3_extra_nodes_embedded_in_config). See docs/nested-graph-review.md for the full coverage analysis.

@pinodeca pinodeca force-pushed the pinodeca/nested-graph branch from 224c913 to 3679fd5 Compare February 18, 2026 16:16
@pinodeca pinodeca force-pushed the pinodeca/nested-graph branch 3 times, most recently from fdfbd44 to 9b3100e Compare March 5, 2026 18:21
DSL functions now build stateless nested JSON structures instead of
writing to df.nodes during construction. This eliminates orphaned nodes,
enables transaction-safe graph building, and simplifies df.explain().

Key changes:
- Durofut embeds children as Box<Durofut> instead of ID references
- node_id removed from Durofut; IDs generated during df.start()
- df.start() validates the graph (validate_recursive, ensure_strict)
  then recursively inserts all nodes in a single transaction
- df.explain() parses nested JSON directly without temp tables
- Config-embedded nodes (condition_node, extra_nodes) handled via
  for_each_config_child() and transform_config_children() visitors

Tests:
- Add E2E tests 30-33 (graph reuse, explain plain SQL, invalid node
  type, malformed condition node)
- Add unit tests for config structure, validation, and error messages

Docs: update ARCHITECTURE.md, USER_GUIDE.md; add nested-graph-design.md
@pinodeca pinodeca force-pushed the pinodeca/nested-graph branch from 9b3100e to 2d892ac Compare March 5, 2026 18:31
@pinodeca pinodeca added the test-pg18 Run CI tests against PostgreSQL 18 label Mar 5, 2026
@pinodeca pinodeca merged commit 8c435e6 into main Mar 5, 2026
9 checks passed
@pinodeca pinodeca deleted the pinodeca/nested-graph branch March 5, 2026 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test-pg18 Run CI tests against PostgreSQL 18

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant