Fail Neo4jOperator argument mistakes at Dag parse time - #70508
Draft
ColtenOuO wants to merge 1 commit into
Draft
Conversation
Whether `cypher` or `sql` was passed is a property of how the Dag was written, not of the rendered value, and the constructor is the only place that can read it: under render_template_as_native_obj a supplied field can render to None, so the same check in execute reports an argument the author did provide as missing. Checking at construction also surfaces the mistake as a Dag import error instead of once per task instance. The execute guard stays: a passed field can still render to None and the hook needs a query string. related: apache#70503
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.
Note
Blocked on #70505. The
validate-operators-inithook onmainstill flagsis Noneprovision checks in__init__, so CI will be red until that merges.Kept as a draft until then — verified clean against the checker from #70505
(exit 0, no exemption entry needed).
Moves the two argument-provision checks of
Neo4jOperatorback to__init__, one of the reverts tracked in #70503.Whether
cypherorsqlwas passed is a property of how the Dag was written, not of the rendered value.execute()cannot answer it: withrender_template_as_native_obj=Truea supplied field can render toNone, so the check there reports an argument the author did provide as missing. Checking at construction also turns a static authoring mistake into a Dag import error rather than a failure on every task instance.Both provision checks move, not only
cypher is None. After #70373 the operator keepssqlandcypheras separate fields, soNeo4jOperator(sql=...)is valid withcypherunset — the constructor check has to becypher is None and sql is None. Once it is written that way, leaving the mutual-exclusivity check inexecute()would split two checks that ask the same question across two phases.The
execute()guard is kept deliberately:Neo4jHook.runtakes astr, and the constructor check does not narrowself.cyphertostr(it is legitimatelyNonewhensqlwas used). A passed field can also render toNone, which is exactly the case the guard covers.castwould hide that andassertis not allowed in production code.Testing done
providers/neo4j/tests/unit/neo4j/operators/test_neo4j.py: 6 passed. The tworeverted tests assert construction-time failure again; a new test covers the
execute()guard by rendering a supplied field toNone.main: exit 1, as expected before Allow argument-provision checks of templated fields in operator __init__ #70505 merges.breeze run mypyon the operator: clean.related: #70503, #70505
Drafted-by: Claude Code (Opus 5); reviewed by @ColtenOuO before posting