Skip to content

feat: add ForEach transformation for transforming nested collections - #1163

Draft
Anatolii Yatsuk (tolik0) wants to merge 4 commits into
mainfrom
tolik0/cdk/foreach-transformation
Draft

Anatolii Yatsuk (tolik0) wants to merge 4 commits into
mainfrom
tolik0/cdk/foreach-transformation

Conversation

@tolik0

@tolik0 Anatolii Yatsuk (tolik0) commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Status: parked. One connector adopts this exactly (source-monday, with the value_type: string caveat below), no connector loses its components.py, and the source-github case it was written for is not covered. It stays open until a second adopter turns up; the design is finished and the tests pass.

What

Adds a ForEach declarative transformation: a RecordTransformation that applies a list of nested transformations to every element of a collection inside the record, instead of to the record as a whole.

transformations:
  - type: ForEach
    field_path: ["column_values"]
    transformations:
      - type: AddFields
        condition: "{{ record.get('display_value') and not record.get('text') }}"
        fields:
          - path: ["text"]
            value: "{{ record['display_value'] }}"
            value_type: string

Why

Today a connector that needs to apply the same fix-up to every element of a nested list has no declarative way to write a loop, so it drops to a CustomTransformation. A single Python class forfeits the Connector Builder for the entire connector — and it does so for logic that is otherwise a plain AddFields.

RecordTransformation.transform() returns None and mutates the record in place (airbyte_cdk/sources/declarative/transformations/transformation.py:19-34; see add_fields.py:128-146, which does kwargs = {"record": record, ...} then dpath.new(record, ...)). That is what makes ForEach cheap: rebinding the nested call's record argument to a collection element mutates that element inside the parent record. No copy, no copy-back.

Verified connector precedents

Each was checked by running the replacement against the same records as the custom class, not by comparing shape.

  1. source-mondayMondayTransformation, airbyte-integrations/connectors/source-monday/components.py:462-472. Loops record["column_values"] and copies display_value into text when text is falsy. The class collapses into the YAML above only with value_type: string on the inner AddFields. monday copies display_value verbatim, always a string; AddFields without value_type runs the rendered value through JinjaInterpolation._literal_eval, so a display_value of "42" lands as the integer 42, "True" as a boolean and "1000.0" as a float. Measured on six values: [(42, int), (True, bool), ('2024-01-01', str), ([1, 2], list), ('hello', str), (1000.0, float)] without the flag, identical to monday with it. One difference remains and is an improvement: a null column_values is a no-op here where monday raises. monday keeps six other custom classes, so it does not become manifest-only.
  2. source-githubReleasesRecordTransformation._assets on the unmerged tolik0/source-github/graphql-streams branch. Not an adopter. The per-asset loop pops uploader into uploader_id, which ForEach with AddFields and RemoveFields expresses, and then recovers the asset's database id by base64-decoding a node ID and unpacking its last four bytes, which no Jinja filter does. Because the loop has to stay in Python for the second half, ForEach saves that connector nothing. An earlier revision listed it as a precedent.
  3. source-bing-adscomponents.py:88, a per-Settings[] fix-up that wraps Details, casts *Id strings to integers under dynamic key names and rewrites PageFeedIds. Partially expressible; the dynamic-key casts keep it custom.

No other RecordTransformation in the 74 components.py files of the monorepo loops over a list-valued field.

Before / after — MondayTransformation

Before (Python, forfeits the Builder for the whole connector):

@dataclass
class MondayTransformation(RecordTransformation):
    def transform(self, record, config=None, **kwargs):
        # Oncall issue: https://github.com/airbytehq/oncall/issues/4337
        column_values = record.get("column_values", [])
        for values in column_values:
            display_value, text = values.get("display_value"), values.get("text")
            if display_value and not text:
                values["text"] = display_value
        return record

After (manifest only; value_type: string is what keeps "42" a string):

transformations:
  - type: ForEach
    field_path: ["column_values"]
    transformations:
      - type: AddFields
        condition: "{{ record.get('display_value') and not record.get('text') }}"
        fields:
          - path: ["text"]
            value: "{{ record['display_value'] }}"
            value_type: string

Design decisions

Question Decision Why
field_path resolution Entries are InterpolatedString, resolved per record, as in the other dpath components. From there the paths diverge: a path without a glob is resolved by a plain dict/list walk that deliberately bypasses dpath, and only a glob path calls into dpath.values. dpath resolves by folding over the whole object graph, so it costs O(total nodes in the record) per record — 18 ms on a record carrying a 10 k-element unrelated sibling, against 0.35 µs for the walk. The walk is written to match dpath's semantics exactly rather than to approximate them: negative list indices count from the end (["a", "-1"] is the last element), an index outside the list matches nothing, and whether a segment is an index at all is decided by int() — the predicate dpath itself uses (dpath.segments.match, dpath.types.ListIndex.__eq__), not isdigit(), which is True for superscripts int() rejects. A parity test asserts the walk agrees with dpath.values segment for segment.
field_path segment that cannot be used as a path Raises AirbyteTracedException. A segment that interpolates to something other than a non-empty string or an integer — empty string, float, boolean, null, list — is a config_error. A segment written as an empty string literally in the manifest is a system_error, raised once at construction instead of per record. Without the check the component is a silent no-op for the entire sync, which is the hardest failure mode to diagnose. The split follows the same rule as the non-object-element row below: a bad config value is the end user's fault, a bad manifest is the connector developer's. An empty string cannot be the render of a template, so it can only have come from the manifest. Integers are let through because Jinja renders a numeric segment as an int and dpath accepts an integer list index — rejecting them would make list indexing unreachable.
Glob segments Supported. A segment containing *, ? or [ — so *, **, item*, ?, [ab] — routes the whole path through dpath.values, and every matching value is iterated as its own collection. Matches are then de-duplicated by identity, because two glob patterns in one path (and ** on its own) can match the same object twice. DpathExtractor and DpathFlattenFields both accept globs; dpath.values returns references, not copies, so in-place mutation still holds. dpath.values rather than dpath.get because get raises a bare ValueError as soon as a glob matches more than one leaf. De-duplication because applying a nested transformation twice to one object is never what the manifest asked for, and JSON parsing never yields two references to one object, so identity cannot collapse two distinct elements.
A glob match that is not a collection Skipped, never an exception — unlike the same value reached by a plain path, which raises (see the non-object-element row). A glob selects rather than addresses, so it routinely lands on scalars unrelated to the intended collection: ["*", "items"] on a record with a top-level "name": "abc" would otherwise fail the sync on every record. A plain path addresses the list itself, so a scalar element there is unambiguously a mis-pointed field_path.
A key that literally contains *, ? or [ Cannot be addressed. The segment is glob-matched, so a[0] matches the key a0, not a[0]. Inherited from dpath, which matches through fnmatch and offers no escape syntax — dpath.values({"a[0]": …}, ["a[0]"]) is empty on its own. DpathExtractor and DpathFlattenFields behave identically, so an escape hatch here would make ForEach the only dpath-based component with one.
Path does not resolve Silent no-op, never an exception. A collection absent from some records is the normal case; raising would fail the sync.
Resolved value is an object (dict) Treated as a collection of one and transformed. Matches DpathExtractor, which wraps an object as a single record.
Resolved value is a scalar / null / empty list No-op. There is nothing to iterate.
Element of the list is null Skipped. A JSON null inside an array is ordinary API payload rather than a manifest mistake, so it must not fail the sync. (Not for parity with MondayTransformation — that one calls values.get(...) and raises AttributeError on a None element. An earlier revision of this description claimed otherwise; the claim was wrong.)
Element of the list is a non-object other than null Raises AirbyteTracedException with FailureType.system_error, naming the field_path. Applies to a list addressed without a glob; a glob match is lenient (row above). A list of strings cannot be mutated in place. Skipping silently would hide a mis-pointed field_path behind missing data. system_error rather than config_error because a mis-pointed field_path is a connector-developer fault the end user cannot fix. The check runs over every element, across all wildcard expansions, before any element is transformed, so a non-object element leaves the record untouched. This covers element shape only: nothing in the transformation pipeline is atomic, so a nested transformation that raises part-way through leaves the elements already transformed mutated.
Empty field_path Resolves to the record itself. Same as DpathExtractor with field_path: [].
Context for nested transformations Nested transformations get the element as record, but the real config, stream_state and stream_slice. {{ config[...] }} and {{ stream_slice[...] }} must keep working inside the loop. Tested.
Non-glob field_path parity with dpath The plain walk matches an integer dict key from a string segment, as dpath does (it glob-matches against str(key)). Otherwise adding a glob elsewhere in the path changes which values the same field_path resolves. Pinned by a parity assertion against dpath.values.
Copying No deep copy anywhere. The collection and its elements are mutated by reference. A copy that is not written back is a silent data-loss bug. Pinned by an identity assertion (record["items"][0] is element).
Nesting ForEach in ForEach Falls out for free; the schema anyOf is self-referential. Pinned by tests at both the component and factory level so a refactor cannot break it.

Backward compatibility: additive only

Nothing existing changes behaviour. The PR adds one new component class, one new schema definition, one new entry in PYDANTIC_MODEL_TO_CONSTRUCTOR, and one new member (ForEach) in each of the three existing transformation anyOf lists (DeclarativeStream.transformations, DynamicSchemaLoader.schema_transformations, JsonSchemaPropertySelector.transformations). No existing definition, field, default, or code path is modified or removed, so every manifest that validates today still validates and still builds the same components. A manifest that does not use ForEach produces a byte-identical component tree.

Changes

  • airbyte_cdk/sources/declarative/transformations/for_each.py — the new component.
  • airbyte_cdk/sources/declarative/transformations/__init__.py — export ForEach.
  • airbyte_cdk/sources/declarative/declarative_component_schema.yamlForEach definition, plus ForEach added to all three transformation anyOf sites (found via grep "#/definitions/RemoveFields").
  • airbyte_cdk/sources/declarative/parsers/model_to_component_factory.pyForEachModel: self.create_for_each plus create_for_each, which builds the nested transformations through self._create_component_from_model, the same call path the stream-level transformations list uses.
  • airbyte_cdk/sources/declarative/models/declarative_component_schema.py — generated model (see note below).

Generated models — hand-applied, and please do NOT run /poe build here

The generated declarative_component_schema.py in this PR was updated by hand to match the schema, rather than by committing the local poe assemble output — and unlike the first revision of this description, that is not because of a machine-specific quirk.

main's committed generated file is itself divergent from the codegen pipeline checked into this repo. Regenerating from an unmodified origin/main YAML produces ~240 lines of drift, including an OAuthScopeScope/OptionalScope public rename. So running /poe build on this branch would import that unrelated drift into this PR, and would make it conflict with #1162. The drift is real and worth fixing — in a separate re-sync PR against main, not here.

What was hand-applied is exactly what codegen produces for this schema change, and nothing else. It was reproduced against pinned codegen (datamodel_code_generator==0.26.3 in python:3.10, the repo's exact flags and post-processing) and the added lines are byte-identical to it (the file is not, because codegen also relocates JsonSchemaPropertySelector further down — see the next paragraph):

  • the ForEach model class;
  • ForEach added to the three transformation Unions;
  • ForEach.update_forward_refs(), in codegen's position.

Correction to the first revision of this description: it claimed the class had to be moved because placing ForEach at codegen's position would leave JsonSchemaPropertySelector.transformations an unresolved ForwardRef. That rationale was wrong. Codegen also emits ForEach before JsonSchemaPropertySelector — it places ForEach where that class was and relocates it further down the file. There is no ForwardRef hazard, and CI regeneration would not reintroduce the failure. There are zero unresolved forward refs at head on ForEach, JsonSchemaPropertySelector, DeclarativeStream and DynamicSchemaLoader.

The self-referential $ref cycle (ForEach inside the same anyOf it contains) is handled by datamodel-codegen itself: the file has from __future__ import annotations and codegen emits ForEach.update_forward_refs(). No hand-invented workaround.

Tests

unit_tests/sources/declarative/transformations/test_for_each.py (79 tests):

  • the monday case end to end — column_values with a nested AddFields and a condition (the test does not set value_type; the coercion caveat above is about the connector manifest, not the component);
  • in-place mutation is visible on the parent record, and the collection and element objects are the same objects (is), so an accidental copy fails the test;
  • transform() returns None, matching the ABC;
  • multi-segment field_path;
  • * wildcard iterating several matching collections and leaving non-matching siblings alone;
  • no-op cases, parametrised: path absent, null, string, number, empty list, sibling-only, and a missing nested path;
  • object target treated as a collection of one;
  • null elements are skipped; only-nulls is a no-op; a string / number / list element raises system_error naming the path and leaves the record untouched;
  • nested transformations read {{ config[...] }} and {{ stream_slice[...] }};
  • field_path itself is interpolated from config;
  • RemoveFields inside the loop (different mutation path — dpath.delete, not dpath.new);
  • a transformation that ignores config (KeysToLower) inside the loop;
  • ForEach nested inside ForEach;
  • nested transformations apply in declared order;
  • field_path list indices: -1, -2, -3 and -0 counted from the end, 0 and 2 from the start, each selecting exactly one element;
  • segments that are not an in-range index are a no-op, not a crash: -4, 3, abc, and the numeric-but-not-decimal ² and ("²".isdigit() is True but int("²") raises, so isdigit() would have produced an uncaught ValueError here);
  • a parity test asserting the plain walk resolves a list segment exactly like dpath.values, over fourteen segments including +1, " 1" and "1_0";
  • a glob match that is not a collection is skipped while the same value reached by a plain path raises, asserted in one test so the asymmetry is deliberate rather than accidental;
  • ** over nested collections transforms each object exactly once (identity de-duplication);
  • the plain walk matches an integer dict key exactly like dpath.values;
  • a key literally containing [ is a no-op, with the dpath half of the claim asserted alongside it;
  • a nested transformation that raises on the second element leaves the first mutated — the documented, non-atomic behaviour;
  • a literal empty field_path segment raises system_error at construction; a segment interpolating to an empty string, float, boolean, null or list raises config_error; a numeric config value is a valid list index rather than an error;
  • the non-glob path never reaches dpath (asserted by patching the module), and a large unrelated sibling does not slow the lookup down.

unit_tests/sources/declarative/parsers/test_model_to_component_factory.py (2 tests in TestCreateTransformations):

  • test_for_each — full manifest round trip: schema validation, model registration, create_for_each, the nested AddFields + RemoveFields built recursively through the factory, and the resulting component actually transforming a record;
  • test_for_each_nested_in_for_each — the recursive schema round trip.

No new test is marked slow, flaky, super_slow or linting.

Verification run locally

  • poetry run pytest unit_tests/sources/declarative/transformations -q — 174 passed
  • poetry run pytest unit_tests/sources/declarative/parsers -q — 201 passed
  • poetry run pytest unit_tests/sources/declarative -q -k "not memory_usage" — 1877 passed, 2 deselected (the two *_memory_usage tests crash this arm64 machine with SIGILL on main as well; unrelated)
  • poe type-check — Success, no issues in 459 source files
  • poe lint-fix / poetry run ruff format . — clean

🤖 Generated with Claude Code

Connectors that need to apply the same fix-up to every element of a nested
collection have no declarative way to write a loop, so they drop to a
CustomTransformation. A single Python class forfeits the Connector Builder for
the entire connector, and it does so for logic that is otherwise a plain
AddFields.

ForEach resolves a dpath field_path to a collection inside the record and runs
its nested transformations against each element. RecordTransformation.transform
mutates in place, so rebinding the nested call's `record` argument to the
element mutates that element inside the parent record - no copy and no
copy-back, which is what keeps this cheap and safe.

source-monday's MondayTransformation (components.py:463-472) collapses entirely
into YAML, and the release-asset loop in source-github's
ReleasesRecordTransformation (components.py:148-151) becomes an AddFields plus a
RemoveFields inside a ForEach.

A missing or non-collection field_path is a silent no-op, because a collection
that is absent from some records is normal and raising would fail the sync. A
non-object element raises a config-flavoured AirbyteTracedException naming the
stream and the path, because silently skipping it would hide a manifest mistake
behind missing data.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

👋 Greetings, Airbyte Team Member!

Here are some helpful tips and reminders for your convenience.

💡 Show Tips and Tricks

Testing This CDK Version

You can test this version of the CDK using the following:

# Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/airbyte-python-cdk.git@tolik0/cdk/foreach-transformation#egg=airbyte-python-cdk[dev]' --help

# Update a connector to use the CDK from this branch ref:
cd airbyte-integrations/connectors/source-example
poe use-cdk-branch tolik0/cdk/foreach-transformation

PR Slash Commands

Airbyte Maintainers can execute the following slash commands on your PR:

  • /autofix - Fixes most formatting and linting issues
  • /poetry-lock - Updates poetry.lock file
  • /test - Runs connector tests with the updated CDK
  • /prerelease - Triggers a prerelease publish with default arguments
  • /poe build - Regenerate git-committed build artifacts, such as the pydantic models which are generated from the manifest JSON schema in YAML.
  • /poe <command> - Runs any poe command in the CDK environment
📚 Show Repo Guidance

Helpful Resources

📝 Edit this welcome message.

@github-actions

github-actions Bot commented Sep 15, 2026

Copy link
Copy Markdown

PyTest Results (Fast)

4 470 tests  +82   4 458 ✅ +82   9m 37s ⏱️ -1s
    1 suites ± 0      12 💤 ± 0 
    1 files   ± 0       0 ❌ ± 0 

Results for commit 282023f. ± Comparison against base commit 96a7c0a.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 15, 2026

Copy link
Copy Markdown

PyTest Results (Full)

4 473 tests  +82   4 461 ✅ +82   14m 23s ⏱️ +22s
    1 suites ± 0      12 💤 ± 0 
    1 files   ± 0       0 ❌ ± 0 

Results for commit 282023f. ± Comparison against base commit 96a7c0a.

♻️ This comment has been updated with latest results.

Behaviour
- A `null` inside the collection is now skipped instead of aborting the sync.
  A null in an array is ordinary API payload, and `MondayTransformation` — the
  Python code this component replaces — skips it, so raising was a regression.
  Any other non-object element still raises, but as `system_error` rather than
  `config_error`: a mis-pointed `field_path` is a connector-development fault the
  end user cannot fix from their connection configuration, and sibling decoders
  already classify read-time payload-shape faults that way.
- Every collection a glob expands to is resolved and validated before any element
  is transformed, so the documented "a bad element leaves the record untouched"
  contract now holds per record, not just per collection.
- A `field_path` segment that interpolates to something other than a non-empty
  string is now a `config_error` instead of silently turning the whole
  transformation into a no-op for the entire sync.
- The error message no longer promises a stream name. `name` is a
  `DeclarativeStream` field, not a `$parameter`, so `self._parameters.get("name")`
  was always empty for a modern manifest. Threading the real name through
  `_create_component_from_model` was rejected: the kwarg would reach
  `create_custom_component`, which merges unknown kwargs into the custom class's
  own fields and would override a manifest-declared `name` on any existing custom
  transformation. The message still names the `field_path` and the element type.
- Messages shortened and de-duplicated per the error-message guidelines.

Performance
- `dpath.get` resolves by folding over the entire object graph, so it costs
  O(total nodes in the record) per record: 17.7 ms measured on a record with a
  10,000-element *unrelated* sibling. Paths without a glob segment now use a plain
  walk — 0.4 us on the same record, ~45,000x faster. `dpath.values` is used for
  every glob form, which also removes the bare `ValueError: globs must match only
  one leaf` that `**`, `?` and `a[ab]` used to raise.

Manifest contract
- `"ForEach.transformations": "CustomTransformation"` added to
  `CUSTOM_COMPONENTS_MAPPING`. Without it a `class_name`-only custom
  transformation — the style used in six production manifests — gets no `type`
  injected when nested in a `ForEach` and fails schema validation.
- `field_path` declares `interpolation_context: [config, parameters]`, matching
  the examples the block advertises. The generated model mirrors the reworded
  description exactly; `interpolation_context` is not emitted by codegen.
- The redundant `__eq__` override is gone. The base class body is byte-identical,
  and the dataclass-generated one correctly returns False for foreign types
  instead of raising `AttributeError`.

Tests
- Null elements skipped; non-object elements raise `system_error`; no stream name
  in the message; nothing mutated when a later collection under a glob is invalid
  (the previous assertion was vacuous — every element in its fixture was a
  non-dict, so nothing could have mutated). Both new guards were confirmed to fail
  against the pre-fix implementation.
- `field_path: []`, `$parameters` interpolation of `field_path`, `stream_state`
  forwarding, non-`*` globs, equality, and a deep copy in the no-op test.
- `dpath` is not called for a path without a glob, plus a wall-clock guard that a
  large unrelated sibling does not dominate the lookup.
- A `ForEach` manifest fragment is validated against the JSON schema, including
  all four `$ref` sites — `DeclarativeStream.transformations`,
  `DynamicSchemaLoader.schema_transformations`,
  `JsonSchemaPropertySelector.transformations` and `ForEach.transformations`.
  Nothing validated against the schema before, so dropping `ForEach` from one of
  those `anyOf` lists would only have surfaced in a real connector.
- A factory-built nested `class_name`-only `CustomTransformation`.

Notes for the PR body
- Do not run `/poe build` on this branch. The hand-applied model delta was
  reproduced against pinned codegen and is byte-identical to it; `main`'s
  generated file is itself ~240 lines divergent from codegen (including an
  `OAuthScope` -> `Scope`/`OptionalScope` public rename), so regenerating here
  would import that drift and would make #1162 conflict. The drift belongs in a
  separate re-sync PR against `main`.
- The earlier `ForwardRef` rationale for the class placement was wrong. Codegen
  also emits `ForEach` before `JsonSchemaPropertySelector`; there is no hazard and
  CI regeneration would not reintroduce the failure.
- The `source-github` `ReleasesRecordTransformation` citation is from the unmerged
  `tolik0/source-github/graphql-streams` branch, not monorepo `main`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The round-2 performance fix replaced `dpath.get` with a hand-rolled walk
for non-glob paths. The walk is worth keeping -- it takes path resolution
from 18 ms to 0.35 us per record -- but it diverged from `dpath` in two
ways, and a third divergence was introduced alongside it.

* Negative list indices were dropped. `"-1".isdigit()` is `False`, so the
  walk fell through to `return []` where `dpath` returns the last element.

* `isdigit()` is the wrong predicate: it is `True` for superscripts such
  as `"²"` that `int()` rejects, so such a segment raised an uncaught
  `ValueError` against a list node, where the contract is a no-op.
  `dpath` decides with `int()` itself (`dpath.segments.match`, through
  `ListIndex.__eq__`), so the walk now does too -- which also covers the
  `+1`, `" 1"` and `"1_0"` forms that `isdecimal()` would have rejected.

* Neither was reachable in the first place. `_evaluate_path`, added in the
  same round, rejects any segment that is not a string, and Jinja renders
  `"-1"` as an `int` -- so every list index, `"0"` as much as `"-1"`, was
  a `config_error` before the walk ever ran. Integer segments are now
  accepted and stringified; `dpath` takes an integer list index too.

Also: a literal empty `field_path` segment was classified `config_error`,
but an empty string cannot be the render of a template, so it is a
manifest fault. It is now a `system_error` raised once at construction,
consistent with how `_elements_of` classifies a mis-pointed `field_path`.
Drops the now-dead `self._parameters`, matching `AddFields`/`RemoveFields`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@devin-ai-integration devin-ai-integration Bot left a comment

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.

Review of 7c4a4d49b8ca931f16c68a5d235a65b69fcc6298

Checked out the branch and executed everything below; every finding has a repro.

Breaking-change call: NON_BREAKING

Search in airbytehq/airbyte (airbyte-integrations/connectors, *.yaml|*.py|*.json):

rg -l "ForEach" airbyte-integrations/connectors --glob '*.yaml' --glob '*.py' --glob '*.json' | wc -l   # 0
rg -l "^\s*transformations:" airbyte-integrations/connectors --glob manifest.yaml | wc -l               # 174

0 of 174 manifests using transformations reference ForEach, and no components.py defines a class by that name, so the new type: ForEach discriminator cannot collide with an existing custom component. Schema/model changes are purely additive (new definition + appended anyOf/Union members); no existing definition changed.

What I ran

  • pytest unit_tests/sources/declarative/transformations/test_for_each.py unit_tests/sources/declarative/parsers/test_model_to_component_factory.py → 234 passed
  • pytest unit_tests/ -x → 2729 passed, then stopped on file_based/test_file_based_stream_reader.py::test_parse_start_date. That test also fails on clean origin/main (2 params fail there) → pre-existing/local-env, not this PR. CI at this SHA reports the full suite green.
  • ruff check / ruff format --check → clean. mypy --config-file mypy.ini airbyte_cdk → 459 files, no issues.
  • test_a_large_unrelated_sibling_does_not_slow_down_the_lookup ×5 → 0.01s call time each run.
  • Codegen: regenerated the model with the pinned datamodel-code-generator==0.26.3 in python:3.10 against both origin/main and this branch's schema. The only PR-attributable delta is the ForEach class, its Union entries and update_forward_refs(); the remaining diff (e.g. JsonSchemaPropertySelector placement) is the pre-existing drift on main. The hand-applied model matches what codegen would emit for the new definition.
  • Repro'd MondayTransformation on a null element: values.get(...) on NoneAttributeError. It does not skip nulls (see inline P3 on the docstring).

Findings

No P0/P1. Inline:

  • P2 glob path over a scalar list silently no-ops while the plain path raises (_elements_of).
  • P2 ** glob visits the same nested object twice (_resolve_collections).
  • P3 literal keys containing *, ?, [ cannot be addressed; int dict keys differ from dpath (_GLOB_CHARACTERS / plain walk).
  • P3 "leaves the record untouched" docstring only holds for validation, not for a nested transformation raising mid-loop.
  • P3 docstring claim that the replaced Python transformations skip null is false for MondayTransformation.

Also: no connector in the CI matrix exercises this component, so the green CI here is only evidence for the unit tests, not for end-to-end behaviour — worth pairing with the source-monday migration PR before merging.

def _elements_of(collection: Any, path: List[str]) -> List[Dict[str, Any]]:
if isinstance(collection, dict):
return [collection]
if not isinstance(collection, list):

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.

P2 — Plain and glob paths disagree on non-object elements.

The docstring (line 65) says "Any other non-object element (a string, a number) raises". That holds for a plain path, but with a glob dpath.values yields the scalars themselves and this branch swallows them:

r = {"items": ["a", "b"]}
fe(["items"]).transform(copy.deepcopy(r))       # raises AirbyteTracedException
fe(["items", "*"]).transform(copy.deepcopy(r))  # no exception, record unchanged

So the "mis-pointed field_path hidden behind missing data" case the error is meant to catch is exactly what happens once someone adds a *. Suggest either raising here too when the collection came from a glob over a list, or documenting that globs are lenient.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Confirmed, and taken the second option: globs are lenient, now documented.

Raising here would make * unusable. A glob selects rather than addresses, so it routinely lands on scalars that have nothing to do with the intended collection — field_path: ["*", "items"] on a record with a top-level "name": "abc" would fail the sync on every record. The asymmetry is the point rather than an oversight: ["items"] addresses the list itself, so a scalar element there is unambiguously a mis-pointed field_path; ["items", "*"] asks for whatever matches, so a non-match is a non-match.

Docstring bullet and the field_path schema description now say so explicitly, and test_a_glob_match_that_is_not_an_object_is_skipped_instead_of_raising pins both halves of the asymmetry in one test.

Fixed in 282023f.

if not path:
return [record]
if any(character in segment for segment in path for character in _GLOB_CHARACTERS):
return list(dpath.values(record, path))

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.

P2** transforms the same object more than once.

dpath.values(record, ["**"]) yields every container at every depth, so a nested object is returned both as a member of its parent list and as its own match:

r = {"a": [{"id": 1, "b": [{"id": 2}]}]}
ForEach(config={}, field_path=["**"], transformations=[Counter()], parameters={}).transform(r)
# {'a': [{'id': 1, 'b': [{'id': 2, 'n': 2}], 'n': 2}]}   <- each object hit twice

For idempotent nested transformations this is just wasted work; for AddFields with a non-idempotent expression or a RemoveFields inside a ForEach it changes results. Either dedupe by id() before applying, or reject/document **.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Real bug, fixed. Reproduced your snippet exactly — {"a": [{"id": 1, "b": [{"id": 2, "n": 2}], "n": 2}]}.

Went with de-duplication by id() rather than rejecting **, because the overlap is not specific to **: any two glob patterns in one path can match the same object. Applying a transformation twice to one object is never what the manifest asked for, and JSON parsing never produces two references to one object, so identity de-duplication cannot collapse two legitimately distinct elements.

Order is preserved, and the de-duplication sits after collection resolution and element validation, so the untouched-record contract is unaffected. test_overlapping_glob_matches_transform_each_object_exactly_once asserts applied == 1 on both objects of your record.

Fixed in 282023f.

# Segments containing any of these need `dpath` glob matching. Everything else is resolved by a
# plain walk, because `dpath` traverses the whole record to glob-match and that cost is paid per
# record.
_GLOB_CHARACTERS = ("*", "?", "[")

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.

P3 — Any segment containing *, ? or [ is routed to dpath, so a literal key with one of those characters can't be addressed at all:

r = {"a[0]": [{"id": 1}]}
fe(["a[0]"]).transform(r)   # no-op: treated as glob `a[0]`, never matches the literal key

Same class of parity gap on the plain walk (line 180): dpath matches string segment "1" against int dict key 1, the plain walk doesn't (dpath.values(r, ["a","1"]) finds {"a": {1: [...]}}, fe(["a","1"]) is a no-op). Both are edge cases, but the docstring sells the plain walk as "the way dpath resolves it" — worth a sentence listing the known divergences, or an escape hatch.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Split these two, they have different answers.

The integer dict key is a real parity gap — fixed. The plain walk now falls back to an int() interpretation of the segment when the string key is absent, so ["a", "1"] reaches {"a": {1: [...]}} as dpath does. This mattered more than it looks: without it, adding a glob anywhere else in the path changed which values the same field_path resolved. test_the_plain_walk_matches_an_integer_dict_key_like_dpath asserts _resolve_collections equals dpath.values on that record.

The literal a[0] key is not a divergence — dpath cannot address it either. It glob-matches through fnmatch, so the pattern a[0] matches the key a0, not a[0]:

>>> list(dpath.values({"a[0]": [{"id": 1}]}, ["a[0]"]))
[]
>>> list(dpath.values({"a0": [{"id": 1}]}, ["a[0]"]))
[[{"id": 1}]]

So ForEach behaves exactly like DpathExtractor and DpathFlattenFields here, and an escape hatch would make it the only dpath-based component with one. Documented as an inherited limitation instead, with test_a_key_that_literally_contains_a_glob_character_cannot_be_addressed asserting the dpath half alongside the ForEach half so the claim cannot rot.

Fixed in 282023f.

and silently skipping it would hide a mis-pointed `field_path` behind missing data.

Every collection the path matches is resolved and validated before any element is transformed, so a bad
element anywhere under a glob leaves the whole record untouched.

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.

P3 — This guarantee covers element validation only. If a nested transformation raises on the second element, the first stays mutated:

r = {"items": [{}, {}]}
ForEach(config={}, field_path=["items"], transformations=[Boom()], parameters={}).transform(r)  # raises on 2nd
# r == {'items': [{'x': 1}, {}]}

That's consistent with how other transformations behave (no atomicity anywhere in the pipeline), so I'd just narrow the wording to "a non-object element … leaves the record untouched".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed, the wording overclaimed. Narrowed to element validation, and added the reason so nobody re-widens it: nothing in the transformation pipeline is atomic, and ForEach should not be the one component that pretends otherwise.

Also added test_a_nested_transformation_that_raises_leaves_the_earlier_elements_mutated, which pins your exact scenario — {"items": [{"id": 1, "added": "value"}, {"id": 2}]} after the raise on the second element.

Fixed in 282023f.

* An empty `field_path` resolves to the record itself, the same way `DpathExtractor` treats an empty
`field_path`.
* A `null` element inside the collection is skipped. A `null` in an array is ordinary API payload, not a
manifest mistake, and the Python transformations this component replaces skip it too.

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.

P3 — "the Python transformations this component replaces skip it too" is not true for the cited precedent. MondayTransformation does values.get("display_value") on each element; on a None element that is:

AttributeError: 'NoneType' object has no attribute 'get'

Skipping null is still the right choice here, just drop the parity claim (the PR description makes the same claim).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Correct, the parity claim was wrong — MondayTransformation calls values.get("display_value") and raises AttributeError on a None element. Claim dropped from the docstring and from the PR description. Skipping stays, on the merits: a null in an array is ordinary API payload rather than a manifest mistake, so it must not fail the sync.

Fixed in 282023f.

…claims

Addresses the review findings on the ForEach transformation:

- Overlapping glob matches no longer transform the same object twice.
  `dpath.values(record, ["**"])` yields every container at every depth, so a
  nested object came back both on its own and as a member of its parent list,
  and a non-idempotent nested transformation ran on it twice. Matches are now
  de-duplicated by identity.
- The plain walk now matches an integer dict key from a string segment, the way
  `dpath` does (it glob-matches against `str(key)`). Without this, adding a glob
  elsewhere in the path changed which values the same `field_path` resolved.
- Documented that globs are lenient: a glob selects rather than addresses, so a
  match that is neither an object nor a list of objects is skipped instead of
  raising. Raising there would make `*` unusable, since it routinely lands on
  unrelated scalars.
- Documented that a key literally containing `*`, `?` or `[` cannot be
  addressed. This is inherited from `dpath`/`fnmatch`, which offer no escape
  syntax, and is identical to `DpathExtractor`.
- Narrowed the "leaves the whole record untouched" guarantee to element
  validation. Nothing in the transformation pipeline is atomic, and a nested
  transformation that raises part-way through leaves earlier elements mutated.
- Dropped the claim that the Python transformations this component replaces skip
  `null` elements. `MondayTransformation` raises `AttributeError` on one.
  Skipping is still the right behavior, just not for that reason.

Tests cover each of the above, including a parity assertion against
`dpath.values` for the integer dict key.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@devin-ai-integration devin-ai-integration Bot left a comment

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.

Re-review of 282023fd2154c350aaa1f312ccae6122db23a7d9

Checked out 282023fd and re-ran everything against it. All five findings from the previous review are resolved; no new P0–P2.

Verification of the fixes (same repros as before, now on 282023fd)

Finding Before Now
P2 glob over scalar list silent no-op, undocumented still no-op — now documented in docstring + schema description as intentional leniency; plain path still raises
P2 ** double-transform n: 2 {'a': [{'id': 1, 'b': [{'id': 2, 'n': 1}], 'n': 1}]} — each object once
P3 int dict key plain walk no-op {'a': {1: [{'id': 1, 'n': 1}]}} — matches dpath; negative int key (-1) also works
P3 glob-char literal keys unaddressable still unaddressable, now documented as inherited dpath behaviour + test
P3 atomicity wording overclaimed narrowed to element validation, non-atomic nested raise now stated + tested
P3 Monday null claim false removed from docstring; PR description updated

What I ran

  • pytest unit_tests/sources/declarative/transformations/test_for_each.py unit_tests/sources/declarative/parsers/test_model_to_component_factory.py → 239 passed (5 new tests, one per finding)
  • ruff check / ruff format --check on the changed files → clean; mypy --config-file mypy.ini airbyte_cdk/sources/declarative/transformations/for_each.py → no issues
  • Schema ↔ hand-applied model: field_path.description in declarative_component_schema.yaml and in the generated ForEach model are byte-identical (diffed programmatically). Nothing else changed in the model.
  • Extra probes: a single dict object referenced from two collections under ["*"] is transformed once (identity dedupe works as described); a nested list inside a glob-matched list is transformed leniently, the same nested list under a plain path raises — consistent with the documented plain/glob split.

One residual P3 (informational, no change requested)

With {"a": {1: {...}, "1": {...}}} and path ["a", "1"], dpath.values yields both objects while the plain walk takes only the "1" key (string key wins, int fallback is skipped). Mixed int/str keys colliding on the same digit string is not a realistic API payload, so I'd leave it; noting it only so the "parity with dpath" docstring is read as best-effort.

Breaking-change call: unchanged, NON_BREAKING

The delta since 7c4a4d49 touches only for_each.py, its tests, and the ForEach.field_path description string in schema/model. The airbytehq/airbyte search from the previous review (0 of 174 transformations manifests reference ForEach, no components.py defines the name) still applies.

CI at this SHA: Pytest (All, 3.10–3.13), Pytest (Fast) and source-shopify still running at the time of writing; destination-motherduck red (known baseline).


Devin session

# `dpath` glob-matches a segment against `str(key)`, so a string segment
# addresses an integer dict key too. Keep the walk in parity with it.
index = _as_list_index_or_none(segment)
if index is None or index not in node:

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.

P3 (informational) — Verified this now matches dpath for {1: ...} and {-1: ...} keys. One remaining divergence: when a dict has both 1 and "1", dpath yields both values, this walk only the string key. Not worth code; just noting the docstring's "parity" is best-effort.


Devin session

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant