Validate df.break() is inside a loop at graph construction time#59
Open
pinodeca wants to merge 1 commit into
Open
Validate df.break() is inside a loop at graph construction time#59pinodeca wants to merge 1 commit into
pinodeca wants to merge 1 commit into
Conversation
pinodeca
force-pushed
the
pinodeca/break-loop
branch
2 times, most recently
from
68010ae to
a613bc2
Compare
df.break() used outside a df.loop() body previously silently produced a corrupted result (the raw __break__ sentinel) and marked the instance as completed. Now df.start() rejects such graphs immediately with a clear error. Changes: - src/types.rs: add Durofut::validate_structural(in_loop) which walks the graph tracking loop context and returns an error if a BREAK node is encountered outside a LOOP body - src/dsl.rs: call validate_structural(false) in df.start() after the existing validate_recursive() check, before any nodes are inserted - tests/e2e/sql/39_break_outside_loop.sql: E2E test covering bare break, break in sequence, break in conditional (all must error), and valid break inside loop (must complete)
pinodeca
force-pushed
the
pinodeca/break-loop
branch
from
a613bc2 to
89bfe38
Compare
There was a problem hiding this comment.
Pull request overview
Adds graph-construction-time validation to prevent df.break() from being used outside a df.loop() body, turning a previously silent “completed with sentinel output” failure mode into an immediate df.start() error.
Changes:
- Add
Durofut::validate_structural(in_loop: bool)to enforce loop-context structural rules. - Invoke structural validation in
df.start()before inserting any nodes. - Add an E2E test covering invalid
df.break()placements and a valid in-loop usage.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/types.rs |
Adds structural graph validation that tracks loop context and rejects invalid BREAK placement. |
src/dsl.rs |
Calls the new structural validator from df.start() before DB writes. |
tests/e2e/sql/39_break_outside_loop.sql |
Adds E2E coverage for df.break() outside vs. inside df.loop(). |
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+566
to
+574
| // For all other nodes propagate the current loop context. | ||
| if let Some(ref left) = self.left_node { | ||
| left.validate_structural(in_loop)?; | ||
| } | ||
| if let Some(ref right) = self.right_node { | ||
| right.validate_structural(in_loop)?; | ||
| } | ||
| self.for_each_config_child(|child| child.validate_structural(in_loop))?; | ||
| Ok(()) |
Comment on lines
+7
to
+12
| BEGIN | ||
| PERFORM df.start(df.break()); | ||
| RAISE EXCEPTION 'TEST FAILED: bare df.break() at top level should have been rejected'; | ||
| EXCEPTION WHEN OTHERS THEN | ||
| RAISE NOTICE 'Case 1 OK - caught expected error: %', SQLERRM; | ||
| END; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
df.break()used outside adf.loop()body silently produced a corrupted result — the raw internal{"__break__": true, "value": ...}sentinel was written as the instance output, and the instance was markedcompleted. No error was raised.Fix
Reject such graphs immediately at
df.start()time, before any nodes are inserted into the database.Changes
src/types.rs: AddDurofut::validate_structural(in_loop: bool)which walks the graph, tracking whether execution is inside a loop body. ABREAKnode encountered within_loop = falsereturns a clear error.LOOPnodes recurse into their body within_loop = trueand into their while-condition within_loop = false.src/dsl.rs: Callvalidate_structural(false)indf.start()after the existingvalidate_recursive()check, before any nodes are inserted.tests/e2e/sql/39_break_outside_loop.sql: E2E test covering four cases:df.break()at top level → must errordf.break()inside~>sequence without loop → must errordf.break()in conditional branch outside loop → must errordf.break()insidedf.loop()body → must complete successfullyBehavior table
df.start(df.break())df.start()df.start('step1' ~> df.break())df.start()df.start(df.loop('step' ~> df.break()))