henry3260 opened a new issue, #76902: URL: https://github.com/apache/airflow/issues/76902
Follow-up from a review comment on #75602, where `DagRun.UnmarshalConf` landed ([comment](https://github.com/apache/airflow/pull/75602#discussion_r4434941788)): > Follow-up: unify with binding.go `foldArgName`/`collectStructFields`; they disagree on shadowing. The Go SDK now folds field names in two places, with two copies of the helpers and two sets of rules. ## Duplicated helpers | `go-sdk/sdk/conf.go` | `go-sdk/pkg/binding/binding.go` | |---|---| | `foldConfName` (L347) | `foldArgName` (L703) — identical bodies | | `implementsConfUnmarshaler` (L358) | `implementsUnmarshaler` (L888) | | `jsonUnmarshalerType`, `textUnmarshalerType` | the same two vars | `pkg/binding` imports `go-sdk/sdk` (binding.go L45), so the two cannot simply share a definition. A shared home needs a third package, for example under `go-sdk/internal/`. ## Where the rules disagree | | `binding` (TaskFlow arguments) | `UnmarshalConf` (Dag run conf) | |---|---|---| | Tagged fields | folds untagged fields only (`!sf.tagged`, L280) | folds `json:` tag names too | | Two names fold to one field | leaves the field at its zero value and logs a warning (L287, L310) | returns an error | | Depth | top-level fields only | every depth, including objects in slices and map values | | Two embedded structs at the same depth | n/a | the first one met wins — see below | That last row is a real divergence from `encoding/json`. For ```go type embUntagged struct{ Source table } type embTagged struct { Src bucket `json:"Source"` } type etlConf struct { embUntagged embTagged } ``` `encoding/json` prefers the tagged field at equal depth and fills `Src`. `UnmarshalConf` steers the nested keys by `embUntagged.Source`'s type instead, and fills neither field: ```text conf {"Source": {"bucket_name": "raw", "table_name": "sales"}} UnmarshalConf : Source.TableName="" Src.BucketName="" no error encoding/json : Source.TableName="" Src.BucketName="raw" no error ``` An *exact* duplicate at the same depth, two embedded structs both declaring `Env`, is **not** a divergence: `encoding/json` cancels those out too, and `UnmarshalConf` already matches it. ## Work - [ ] Move the shared helpers somewhere both `sdk` and `pkg/binding` can import. - [ ] Decide whether the two rule sets should converge or stay deliberately different. They serve different inputs — a Dag author's conf versus a stub's argument list — so "share the helpers, document the rules" may be a better answer than one shared matcher. - [ ] Settle the same-depth embedded-struct rule: either follow `encoding/json` and let the tagged field win, or reject the struct with the existing fold-clash error, rather than silently picking the first. - [ ] Add tests for the `conf.go` branches that currently have none: the `json.Unmarshaler` short-circuit, `json:"-,"`, `json:",omitempty"`, the self-embedding guard, the unexported-field skip, and a tagged embedded struct. Each can be broken today without any test failing. ## Acceptance One definition of the folding helper; a test that fails if the two rule sets drift apart, or a doc note saying they are deliberately different; and a decided, tested rule for same-depth embedded structs. --- Drafted-by: Claude Code (Opus 5) (no human review before posting) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
