Skip to content

Revert Dataform workflow invocation dict normalization (#69161) - #70395

Merged
potiuk merged 1 commit into
apache:mainfrom
shahar1:revert-dataform-noop-69161
Aug 1, 2026
Merged

Revert Dataform workflow invocation dict normalization (#69161)#70395
potiuk merged 1 commit into
apache:mainfrom
shahar1:revert-dataform-noop-69161

Conversation

@shahar1

@shahar1 shahar1 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Reverts #69161. The change is a provably wire-identical no-op: proto-plus already converts nested dicts recursively, so the pre- and post-change CreateWorkflowInvocationRequest serialize byte-identically and the dict path never dropped invocation_config. Unknown fields raise ValueError in proto-plus, so a silent drop at this layer was never possible.

The symptom reported in #53843 was caused outside Airflow: dataform-core 3.0.10–3.0.16 defaulted incremental tables to protected: true (dataform-co/dataform#1914), which silently disables full refresh server-side; fixed upstream in core 3.0.17 (dataform-co/dataform#1942). Repos pin dataformCoreVersion, which is why the symptom persisted after the upstream fix. #53843 has been closed as not planned accordingly.

The accompanying test only pinned third-party proto-plus conversion behavior, which the project's testing standards exclude.

related: #53843


Was generative AI tooling used to co-author this PR?
  • Yes — Claude Code (Fable 5)

Generated-by: Claude Code (Fable 5) following the guidelines

The reverted change was meant to fix apache#53843 (invocation_config believed
dropped when workflow_invocation is passed as a dict), but it is a
wire-identical no-op: proto-plus already converts nested dicts
recursively, and the pre- and post-change requests serialize
byte-identically, so the dict path never dropped the config. The
reported symptom came from dataform-core 3.0.10-3.0.16 defaulting
incremental tables to protected=true (silently disabling full refresh
server-side; fixed upstream in core 3.0.17 via
dataform-co/dataform#1942). apache#53843 is closed as not planned. The
accompanying test only pinned third-party proto-plus conversion
behavior, which our testing standards exclude.

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified the load-bearing claim rather than taking it on trust — building the request both ways and comparing:

wire-identical:                             True
invocation_config preserved via dict path:  True
unknown field raises ValueError:            Unknown field for WorkflowInvocation: bogus_field

So the removed normalization was a genuine no-op: proto-plus already converts nested dicts recursively when the client builds CreateWorkflowInvocationRequest, invocation_config was never dropped on the dict path, and a silent drop was impossible since unknown fields raise.

The test removal is the more valuable half. Asserting that proto-plus converts a nested dict is pinning third-party behaviour, which our testing standards exclude, and it gave false confidence that we were fixing something we weren't.

Clean revert too — WorkflowInvocation is still imported and used for the State enum in both files, so nothing dangles.

I didn't independently check the upstream dataform-core attribution, but the revert doesn't rest on it.


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

@potiuk
potiuk merged commit 7333cad into apache:main Aug 1, 2026
86 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers provider:google Google (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants