Fix PsrpOperator constructor validation to use provision checks inste… by bujjibabukatta · Pull Request #70656 · apache/airflow

bujjibabukatta · 2026-07-29T02:22:36Z

Summary

Part of the burn-down tracked in #70296.

PsrpOperator.__init__ validated command/powershell/cmdlet/arguments/parameters
using truthiness checks instead of is not None checks. Per the guidance in #70296,
"was this provided" checks are allowed to stay in __init__, but must use is not None
polarity rather than truthiness — so this is a fix in place, not a move to execute().

What was wrong

command/powershell/cmdlet were collected into a set before being passed to
exactly_one(). If two of them happened to render to the same string, the set would
collapse them and validation would incorrectly pass.
if arguments and ..., if parameters and ..., if cmdlet: treated an explicitly
provided empty list/dict/string as "not provided", silently skipping validation.

Fix

Replaced truthiness checks with is not None checks, and pass fields to exactly_one()
directly instead of via a set. __init__ is now limited to this provision check plus
plain self.field = field assignments.

Testing

Added three regression tests in test_psrp.py covering: two duplicate field values,
an empty arguments list, and an empty parameters dict — all now correctly raise
ValueError.

Related: #70296

Was generative AI tooling used to co-author this PR?

Yes - Claude

Generated-by: Claude following the guidelines

…ad of truthiness

shahar1 · 2026-07-29T06:25:23Z

Please note that it's a competitor PR of #70347 (you may keep this open, but I might end up merging the other - depending which provides the best match first)

eladkal · 2026-07-29T06:25:53Z

Why was this file modified?

potiuk · 2026-07-30T11:31:54Z

Thanks — your diagnosis is right, and the fix is correct: args = {command, powershell, cmdlet} built a set (collapsing equal values) and exactly_one counted truthiness rather than presence, so the is not None rewrite is the proper correction.

It duplicates #70347 though, which makes the identical change to the same two files and was opened on 23 July, six days before this one. That PR also carries the fix through execute() and removes the operator from validate_operators_init_exemptions.txt, so it's the more complete version — I'm closing this in its favour and have asked there for the one extra behavioural change it contains to be split out or documented.

One unrelated observation while I'm here: this PR also touches scripts/in_container/run_generate_constraints.py, which doesn't appear connected to the PsrpOperator fix. If that was an accidental inclusion it's worth checking your branch; if it's a real fix it deserves its own PR where it'll actually get reviewed rather than riding along unnoticed.

Worth a quick gh pr list --search "<operator>" before starting on one of these — several of the validate_operators_init_exemptions.txt entries have had two people working on them independently this week.

Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting

Dropping it changes task identity for Dags that rely on it, so history, logs, XComs and UI links all move. That is a breaking change and deserves its own review rather than riding along with a validation fix. Splitting it out lets the option checks land on their own and unblocks the burn-down in apache#70296, since apache#70656 was closed in favour of this PR. The removal follows as a separate change with a provider changelog note. Signed-off-by: 1fanwang <1fannnw@gmail.com>

* Validate PsrpOperator parameters after rendering command, powershell, cmdlet, arguments and parameters are template fields, rendered after __init__ runs. The constructor validated their combination and derived task_id from cmdlet, all reading the un-rendered Jinja expressions. Move the validation into execute(), which runs after rendering. This drops the cmdlet-derived task_id default (a construction-time read of a template field that cannot move): task_id must now be passed explicitly when using cmdlet. related: #70296 Signed-off-by: 1fanwang <1fannnw@gmail.com> * Tighten PsrpOperator validation comment Signed-off-by: 1fanwang <1fannnw@gmail.com> * Drop narrating comment from PsrpOperator Signed-off-by: 1fanwang <1fannnw@gmail.com> * Cover PsrpOperator arguments/parameters validation at execute Signed-off-by: 1fanwang <1fannnw@gmail.com> * Add render-then-execute regression test for PsrpOperator validation command is a template field, so validating it in __init__ checked the raw Jinja expression, not the rendered value. Add a test that renders a command resolving to an empty string, then executes: it fails on the pre-fix source (the empty command slips past __init__ and reaches execution) and passes with validation in execute(). Signed-off-by: 1fanwang <1fannnw@gmail.com> * Fix PsrpOperator accepting duplicate command options exactly_one deduped equal values before counting them, so passing the same value to two of command/powershell/cmdlet passed validation. Pass the options positionally so each is counted. Drop the manual changelog note; the provider release manager regenerates the changelog from git log. Signed-off-by: 1fanwang <1fannnw@gmail.com> * Validate PsrpOperator options by provision, not rendered value The mutual-exclusivity and arguments/parameters checks are about which options the Dag author provided, not what the templates render to. Keying them off rendered truthiness dropped a provided option that rendered to a falsy value, and let a second option slip through when the first rendered empty. Check is-not-None (usage) instead, and dispatch the chosen option consistently. Signed-off-by: 1fanwang <1fannnw@gmail.com> * Keep PsrpOperator option checks in the constructor Only the constructor can tell whether an option was passed: with render_template_as_native_obj a provided field can render to None, so the same check in execute() reports a supplied argument as missing. Compare against None rather than truthiness, because a provided option can itself be empty. related: #70505 Signed-off-by: 1fanwang <1fannnw@gmail.com> * Drop narrating comment from PsrpOperator constructor Signed-off-by: 1fanwang <1fannnw@gmail.com> * Keep the cmdlet-derived task_id default Dropping it changes task identity for Dags that rely on it, so history, logs, XComs and UI links all move. That is a breaking change and deserves its own review rather than riding along with a validation fix. Splitting it out lets the option checks land on their own and unblocks the burn-down in #70296, since #70656 was closed in favour of this PR. The removal follows as a separate change with a provider changelog note. Signed-off-by: 1fanwang <1fannnw@gmail.com> --------- Signed-off-by: 1fanwang <1fannnw@gmail.com> Co-authored-by: Shahar Epstein <60007259+shahar1@users.noreply.github.com>

* Validate PsrpOperator parameters after rendering command, powershell, cmdlet, arguments and parameters are template fields, rendered after __init__ runs. The constructor validated their combination and derived task_id from cmdlet, all reading the un-rendered Jinja expressions. Move the validation into execute(), which runs after rendering. This drops the cmdlet-derived task_id default (a construction-time read of a template field that cannot move): task_id must now be passed explicitly when using cmdlet. related: apache#70296 Signed-off-by: 1fanwang <1fannnw@gmail.com> * Tighten PsrpOperator validation comment Signed-off-by: 1fanwang <1fannnw@gmail.com> * Drop narrating comment from PsrpOperator Signed-off-by: 1fanwang <1fannnw@gmail.com> * Cover PsrpOperator arguments/parameters validation at execute Signed-off-by: 1fanwang <1fannnw@gmail.com> * Add render-then-execute regression test for PsrpOperator validation command is a template field, so validating it in __init__ checked the raw Jinja expression, not the rendered value. Add a test that renders a command resolving to an empty string, then executes: it fails on the pre-fix source (the empty command slips past __init__ and reaches execution) and passes with validation in execute(). Signed-off-by: 1fanwang <1fannnw@gmail.com> * Fix PsrpOperator accepting duplicate command options exactly_one deduped equal values before counting them, so passing the same value to two of command/powershell/cmdlet passed validation. Pass the options positionally so each is counted. Drop the manual changelog note; the provider release manager regenerates the changelog from git log. Signed-off-by: 1fanwang <1fannnw@gmail.com> * Validate PsrpOperator options by provision, not rendered value The mutual-exclusivity and arguments/parameters checks are about which options the Dag author provided, not what the templates render to. Keying them off rendered truthiness dropped a provided option that rendered to a falsy value, and let a second option slip through when the first rendered empty. Check is-not-None (usage) instead, and dispatch the chosen option consistently. Signed-off-by: 1fanwang <1fannnw@gmail.com> * Keep PsrpOperator option checks in the constructor Only the constructor can tell whether an option was passed: with render_template_as_native_obj a provided field can render to None, so the same check in execute() reports a supplied argument as missing. Compare against None rather than truthiness, because a provided option can itself be empty. related: apache#70505 Signed-off-by: 1fanwang <1fannnw@gmail.com> * Drop narrating comment from PsrpOperator constructor Signed-off-by: 1fanwang <1fannnw@gmail.com> * Keep the cmdlet-derived task_id default Dropping it changes task identity for Dags that rely on it, so history, logs, XComs and UI links all move. That is a breaking change and deserves its own review rather than riding along with a validation fix. Splitting it out lets the option checks land on their own and unblocks the burn-down in apache#70296, since apache#70656 was closed in favour of this PR. The removal follows as a separate change with a provider changelog note. Signed-off-by: 1fanwang <1fannnw@gmail.com> --------- Signed-off-by: 1fanwang <1fannnw@gmail.com> Co-authored-by: Shahar Epstein <60007259+shahar1@users.noreply.github.com>

Fix PsrpOperator constructor validation to use provision checks inste…
7eb4f2a
…ad of truthiness

bujjibabukatta requested review from amoghrajesh, ashb, bugraoz93, gopidesupavan, jason810496, jscheffl and potiuk as code owners July 29, 2026 02:22

boring-cyborgBot added area:dev-tools area:providers backport-to-v3-3-test Backport to v3-3-test provider:microsoft-psrp labels Jul 29, 2026

Restore PsrpOperator exemption and update gremlinpython
9e429c9

bujjibabukatta force-pushed the fix/#70296 branch from 801c96f to 9e429c9Compare July 29, 2026 05:43

Merge branch 'main' into fix/apache#70296
0cb0c56

eladkal reviewed Jul 29, 2026
View reviewed changes

Comment threadscripts/in_container/run_generate_constraints.py
eladkalJul 29, 2026
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this file modified?

potiuk mentioned this pull request Jul 30, 2026
Fix PsrpOperator option checks #70347
Merged

potiuk closed this Jul 30, 2026

potiuk mentioned this pull request Jul 30, 2026
Move template-field validation/transformation out of operator __init__ (exemption-list burn-down) #70296
Open
78 tasks

Navigation Menu

Uh oh!

Fix PsrpOperator constructor validation to use provision checks inste… - #70656

Fix PsrpOperator constructor validation to use provision checks inste…#70656
bujjibabukatta wants to merge 3 commits into
apache:mainfrom
bujjibabukatta:fix/#70296

bujjibabukatta commented Jul 29, 2026

Uh oh!

shahar1 commented Jul 29, 2026

Uh oh!

eladkalJul 29, 2026

Uh oh!

potiuk commented Jul 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

bujjibabukatta commented Jul 29, 2026

Summary

What was wrong

Fix

Testing

Uh oh!

shahar1 commented Jul 29, 2026

Uh oh!

eladkalJul 29, 2026

Choose a reason for hiding this comment

Uh oh!

potiuk commented Jul 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants