feat(RecordExpander): add parent_fields to copy named parent values onto expanded items - #1165
Anatolii Yatsuk (tolik0) wants to merge 3 commits into
Conversation
… onto expanded items
`RecordExpander` can already carry parent context onto each expanded item, but only
by way of `remain_original_record: true`, which deep-copies the entire parent record
once per item. When the parent is large and the nested list is long - a GitHub pull
request node with ten reviews, say - that copies the whole pull request ten times to
carry one URL.
`parent_fields` names the values to copy instead:
```yaml
record_expander:
type: RecordExpander
expand_records_from_field: ["reviews", "nodes"]
parent_fields:
- type: ParentFieldPath
parent_path: ["url"]
record_path: ["pull_request_url"]
```
Both paths are lists, so a value nested in the parent can be written to a nested
position on the item; intermediate objects are created as needed. An existing value
at `record_path` is overwritten, and a `parent_path` the parent does not have copies
`None` - which is what `parent.get(field)` does in the connector classes this
replaces. Glob metacharacters are rejected in both paths, matching the existing rule
on `truncation_indicator_path`.
The copy happens in `_apply_parent_context`, the single point both the embedded-items
path and the `truncated_list_retriever` path go through, so fetched items are stamped
the same way as embedded ones.
`parent_fields` and `remain_original_record` are independent and may both be set.
Making one suppress the other would be surprising; the reason to prefer
`parent_fields` is that it does not need the deep copy, which the docstring and the
schema description both say.
Scalar list items previously fell through unwrapped unless `remain_original_record`
was set, in which case they were wrapped as `{"value": ..., "original_record": ...}`.
They are now wrapped as `{"value": ...}` plus the copied fields whenever either option
asks for parent context, and still yielded bare when neither does.
The generated model was hand-applied and verified against the pinned generator
(`datamodel_code_generator==0.26.3` with the flag set from
`bin/generate_component_manifest_files.py`): the `ParentFieldPath` class and the
`parent_fields` field are byte-identical to its output after `ruff format`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksTesting This CDK VersionYou 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/record-expander-parent-fields#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/record-expander-parent-fieldsPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
…onto each expanded item
With `merge_parent: true` each expanded item becomes the parent record shallow-merged
with the item, the item's own keys winning on collision, and the value at
`expand_records_from_field` removed from the parent's copy. Only that value is removed:
on a multi-segment path the top-level key stays with its other fields, and the parent
record itself is never mutated. This is the `{**record, **item}` flatten that
source-mailchimp (`components.py:16`) and source-orb (`components.py:16-45`) implement in
custom Python today.
The order inside `_apply_parent_context` is fixed: the merge first, then `parent_fields`
copies, then `original_record` when `remain_original_record` is set, so a named copy can
overwrite a merged value. The three options are independent and may be combined. Scalar
list items are wrapped as `{"value": item}` before the merge, and items fetched through
`truncated_list_retriever` are merged the same way as embedded ones. The stripped parent
is built once per parent record rather than once per item.
The generated model change was hand-applied in the same style as `parent_fields`.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review of 120c1efd (tolik0/cdk/record-expander-parent-fields)
Checked out the branch and ran everything below; no finding here is by inspection alone.
Breaking-change call: NON_BREAKING
Evidence from airbytehq/airbyte master (checkout dated 2026-09-16):
rg -l "record_expander:" airbyte-integrations/connectors --glob '*.yaml'→ 1 of 511 manifests:source-stripe/manifest.yaml, 2 occurrences, bothexpand_records_from_field: [data, object, {lines|items}, data]withremain_original_record: true.rg -l "RecordExpander|record_expander" --glob '*.py'→ 1 file,source-stripe/unit_tests/integration/test_original_record_guard.py(tests only, no runtime use).rg -l "merge_parent|parent_fields|ParentFieldPath"→ 0 connectors.- Ran
RecordExpanderon a stripe-shaped record (dict items, a string item and an int item underdata.object.lines.data) withremain_original_record=Trueand with no options, onmainand on120c1efd:diff→ byte-identical (744 bytes each). So the scalar-wrapping refactor does not change the one production consumer, and the generated model'sConfig.extra = Extra.allowmeans older CDKs ignore the new keys as the schema description claims.
Findings
| Sev | Where | What |
|---|---|---|
| P1 | record_expander.py:439 |
merge_parent aliases every nested container of the parent into every sibling item. A downstream nested AddFields overwrote the value on all previously yielded records and on the parent (repro through RecordSelector.select_records inline). Contradicts the "parent is never mutated" docstring. |
| P1 | record_expander.py:101 |
Same aliasing for parent_fields when parent_path points at a dict/list; copy.deepcopy(value) fixes it at cost proportional to the named value. |
| P2 | record_expander.py:142 |
Docstring example omits type: ParentFieldPath and fails validation, because RecordExpander.parent_fields is not in manifest_component_transformer.DEFAULT_MODEL_TYPES. |
| P3 | record_expander.py:79 |
Glob rejection error text says "when truncation handling is configured" for parent_fields paths. |
| P3 | record_expander.py:101 |
A record_path whose intermediate already holds a scalar on the item (child={"parent": 3}, record_path=["parent","id"]) surfaces as dpath.exceptions.PathNotFound: Path: ['parent', 'id'][0], not a CDK error. A numeric-string segment (["items","0"]) creates a list ({"items": [5]}). Both are dpath.new behaviour and match AddFields; informational, but worth a sentence in the schema description. |
Verified as claimed (no action): on_no_records: emit_parent yields the parent untouched with merge_parent; glob and ** expand paths strip every matched list from the merge base and leave the parent intact (sections as a dict and as a list); a parent_path through a scalar ({"a": 5}, ["a","b"]) copies None rather than raising; scalar items are bare with no options and {"value": ..} + context otherwise.
What I ran
poetry run pytest unit_tests/sources/declarative/expanders unit_tests/sources/declarative/parsers/test_model_to_component_factory.py→ 248 passed.poetry run ruff check ./ruff format --check .→ clean.mypy --config-file mypy.inionrecord_expander.pyandmodel_to_component_factory.py→ clean.- Codegen drift: per the note that the ~350-line
poe assembledelta is pre-existing onmain, not re-reported here; the hand-appliedParentFieldPath/parent_fields/merge_parentmodel entries load and validate (factory tests above). - CI at review time: 21 passed, 1 failed (
destination-motherduck, stated baseline), 3 pending. No connector in the matrix uses the new options, so green CI does not exercise them.
Verdict: request changes for the two P1s. The design is sound and the one production consumer is provably unaffected, but with merge_parent or a container-valued parent_fields the first nested AddFields a connector adds will silently emit wrong data.
| overwrite a merged value. | ||
| """ | ||
| if merge_base is not None: | ||
| child_record = {**merge_base, **child_record} |
There was a problem hiding this comment.
P1 — merge_parent shares every nested container of the parent between all sibling items (and with the parent). A downstream transformation that writes into one of them corrupts records that were already yielded.
{**merge_base, **child_record} is a top-level shallow merge, so item[k] is parent[k] for every non-expanded key whose value is a dict/list. Reproduced end to end through RecordSelector.select_records on 120c1efd with a manifest expand_records_from_field: ["activity"], merge_parent: true and a single AddFields writing path: ["meta", "item_id"], value: "{{ record['id'] }}":
# response body
[{"email_id": "e1", "meta": {"list": "L"}, "activity": [{"id": 1}, {"id": 2}, {"id": 3}]}]
# records returned by select_records
[{"email_id": "e1", "meta": {"list": "L", "item_id": 3}, "id": 1},
{"email_id": "e1", "meta": {"list": "L", "item_id": 3}, "id": 2},
{"email_id": "e1", "meta": {"list": "L", "item_id": 3}, "id": 3}]All three records carry item_id: 3, and parent["meta"] is now {"list": "L", "item_id": 3} too. remain_original_record does not have this problem because it deep-copies (checked: out[0]["original_record"]["meta"] is out[1]["original_record"]["meta"] → False on this branch).
The PR description lists "untouched nested values shared rather than deep-copied" as a tested feature, so this is a deliberate choice, but the docstring on _without_expanded_list says the parent "is never mutated", which the repro disproves as soon as any AddFields/RemoveFields with a multi-segment path runs after the expander. RecordTransformation.transform mutates in place by contract, so this is the normal path, not an edge case.
Suggested fix: copy.deepcopy(merge_base) per item inside _apply_parent_context (or build merge_base per item). The expensive part of the parent — the expanded list — is already stripped from merge_base, so the efficiency argument against remain_original_record still holds. If you'd rather keep sharing, the docstring, the schema description and the merge_parent docs need to say explicitly that nested values are shared with the parent and with sibling items and must not be mutated downstream — and the "never mutated" sentence has to go.
There was a problem hiding this comment.
Fixed in 606c32f — took the deep-copy option rather than documenting the sharing, since the hazard is on the normal path as you say.
merge_base is now deep-copied per item inside _apply_parent_context:
if merge_base is not None:
child_record = {**copy.deepcopy(merge_base), **child_record}Your repro through RecordSelector.select_records now returns:
[{"email_id": "e1", "meta": {"list": "L", "item_id": 1}, "id": 1},
{"email_id": "e1", "meta": {"list": "L", "item_id": 2}, "id": 2},
{"email_id": "e1", "meta": {"list": "L", "item_id": 3}, "id": 3}]
# parent["meta"] -> {"list": "L"}merge_base is still stripped of the expanded list and still built once per parent record, so the copy is proportional to the parent minus the list and the efficiency argument against remain_original_record holds. The "never mutated" sentence on _without_expanded_list is rewritten to say the base is not mutated there and that the per-item copy in _apply_parent_context is what isolates the parent and the siblings; the schema description and the PR description say the same. test_merge_parent_shares_untouched_nested_values_rather_than_deep_copying_them is replaced by test_merge_parent_gives_each_item_its_own_copy_of_nested_values, which writes into item 0 and asserts item 1 and the parent are untouched.
| value = dpath.get(dict(parent_record), self.evaluated_parent_path()) | ||
| except (KeyError, ValueError): | ||
| value = None | ||
| dpath.new(child_record, self.evaluated_record_path(), value) |
There was a problem hiding this comment.
P1 — same aliasing for parent_fields when the copied value is a container.
dpath.get returns the parent's object by reference and dpath.new stores that reference on the child, so every sibling holds the same dict. Repro on 120c1efd:
exp = RecordExpander(expand_records_from_field=["reviews", "nodes"], config={}, parameters={},
parent_fields=[ParentFieldPath(parent_path=["repository"], record_path=["repository"], config={}, parameters={})])
parent = {"repository": {"name": "r"}, "reviews": {"nodes": [{"id": 1}, {"id": 2}]}}
add = AddFields(fields=[AddedFieldDefinition(path=["repository", "review_id"], value="{{ record['id'] }}", value_type=None, parameters={})], parameters={})
out = []
for rec in exp.expand_record(parent):
add.transform(rec, config={}); out.append(rec)
# out -> [{"id": 1, "repository": {"name": "r", "review_id": 2}}, {"id": 2, "repository": {"name": "r", "review_id": 2}}]
# parent["repository"] -> {"name": "r", "review_id": 2}parent_fields is sold as the cheap alternative to remain_original_record because it copies named values, so copy.deepcopy(value) here costs proportionally to the named value, not the parent, and removes the hazard entirely. Recommend doing that.
There was a problem hiding this comment.
Fixed in 606c32f, as recommended. ParentFieldPath.copy_onto now detaches a container value before writing it:
def _detached(value: Any) -> Any:
"""Copy of `value` sharing nothing with the record it came from; scalars are returned as-is."""
if isinstance(value, (Mapping, list, set, tuple)):
return copy.deepcopy(value)
return valueScalars — the common case, and the one the source-github adopter uses — pass through untouched, so the copy costs proportionally to the named value. Your repro now gives:
[{"id": 1, "repository": {"name": "r", "review_id": 1}},
{"id": 2, "repository": {"name": "r", "review_id": 2}}]
# parent["repository"] -> {"name": "r"}Pinned by test_parent_fields_gives_each_item_its_own_copy_of_a_copied_container.
| self._record_path: list[InterpolatedString] = [ | ||
| InterpolatedString.create(path, parameters=parameters) for path in self.record_path | ||
| ] | ||
| RecordExpander._reject_globs(self.evaluated_parent_path(), "parent_path") |
There was a problem hiding this comment.
P3 — misleading error text. _reject_globs hard-codes "when truncation handling is configured", so an invalid parent_fields path reports (verified):
ValueError: Glob characters ('*', '?', '[') are not supported in `parent_path` when truncation handling is configured: the path must identify a single field.
No truncation handling is involved. Drop that clause from the shared message or pass a reason string.
There was a problem hiding this comment.
Fixed in 606c32f — dropped the clause instead of threading a reason through, since "the path must identify a single field" is the whole rule in every caller:
ValueError: Glob characters ('*', '?', '[') are not supported in `parent_path`: the path must identify a single field.
test_glob_rejection_message_does_not_mention_truncation_handling asserts "truncation" not in str(error.value).
| - "reviews" | ||
| - "nodes" | ||
| parent_fields: | ||
| - parent_path: ["url"] |
There was a problem hiding this comment.
P2 — this example does not build. ParentFieldPath requires type and RecordExpander.parent_fields is not in DEFAULT_MODEL_TYPES in manifest_component_transformer.py (unlike AddFields.fields → AddedFieldDefinition), so running exactly this YAML through propagate_types_and_parameters + the factory on 120c1efd fails:
ValidationError: extractor -> record_expander -> parent_fields -> 0 -> type: field required
The factory test only passes because it spells out - type: ParentFieldPath. Either add "RecordExpander.parent_fields": "ParentFieldPath" to DEFAULT_MODEL_TYPES (and a factory test without type, which is what Builder users will write), or add type: ParentFieldPath to this example and the PR description.
There was a problem hiding this comment.
Fixed in 606c32f by taking the first option — "RecordExpander.parent_fields": "ParentFieldPath" is now in manifest_component_transformer.DEFAULT_MODEL_TYPES, next to AddFields.fields, so the untyped form a Builder user writes resolves and the docstring example builds as written.
test_create_record_expander_with_parent_fields_without_explicit_type runs a manifest with no type key under parent_fields through propagate_types_and_parameters + the factory, asserts the transformer filled in type: ParentFieldPath, and then asserts the extracted records.
…d sibling items `merge_parent` shallow-merged the parent, so every nested container of the parent was aliased into every expanded item and into the parent itself. The same held for a `parent_fields` entry whose `parent_path` pointed at a dict or a list, because `dpath.get` returns the parent's object by reference. A downstream `AddFields`/`RemoveFields` with a multi-segment path then wrote into records already yielded, and into the record being expanded. `merge_base` is now deep-copied per item, and a container copied by `ParentFieldPath.copy_onto` is deep-copied as well; scalars pass through untouched. `merge_base` still excludes the expanded list and is still built once per parent record, so the copy stays cheaper than `remain_original_record`. Also: - register `RecordExpander.parent_fields` in `DEFAULT_MODEL_TYPES`, so a manifest may omit `type: ParentFieldPath` as it may for `AddFields.fields` - drop the "when truncation handling is configured" clause from the shared glob-rejection message, which `parent_fields` paths also reach - document `dpath.new` behaviour on `record_path` and the per-item copying in the schema descriptions and docstrings Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Re-review of 606c32f3 — all four findings from the 120c1efd review are resolved
Everything below was executed on a checkout of 606c32f3f8aa0b80f30d82cea8017b8151219e95 (the only new commit since the previous review).
Findings status
| Prev | Finding | Status | Evidence |
|---|---|---|---|
| P1 | merge_parent aliased nested containers across items and the parent |
Fixed | {**copy.deepcopy(merge_base), **child_record}. End-to-end repro (selector + nested AddFields on meta.item_id) now yields item_ids: [1, 2, 3] (was [3, 3, 3]); out[0]['meta'] is out[1]['meta'] -> False; parent record unchanged after mutating an item. |
| P1 | parent_fields aliased container-valued parent fields |
Fixed | _detached() deep-copies Mapping/list/set/tuple values before dpath.new. Same repro on repository.review_id yields [1, 2] per item; parent and siblings untouched. Nested lists inside a copied list are also detached. |
| P2 | Docstring example without type: ParentFieldPath failed validation |
Fixed | "RecordExpander.parent_fields": "ParentFieldPath" added to DEFAULT_MODEL_TYPES. The previously failing YAML (no type) now builds and extracts records through the factory; covered by test_create_record_expander_with_parent_fields_without_explicit_type. |
| P3 | Glob error text mentioned truncation handling | Fixed | Message is now: Glob characters ('*', '?', '[') are not supported in parent_path: the path must identify a single field. |
| P3 (info) | dpath.new scalar-intermediate / numeric-segment behavior |
Documented | record_path description now states intermediate objects are created, scalar pass-through fails, numeric segment creates an array — matches AddFields. |
Checks run on 606c32f3
poetry run pytest unit_tests/sources/declarative/expanders unit_tests/sources/declarative/parsers/test_model_to_component_factory.py unit_tests/sources/declarative/parsers/test_manifest_component_transformer.py -q
-> 269 passed
poetry run ruff check . -> clean (PR files)
poetry run ruff format --check . -> 792 files already formatted
poetry run mypy record_expander.py model_to_component_factory.py manifest_component_transformer.py
-> Success: no issues found in 3 source files
New tests test_merge_parent_gives_each_item_its_own_copy_of_nested_values and test_parent_fields_gives_each_item_its_own_copy_of_a_copied_container assert identity (is not) and post-mutation isolation, so a regression to shallow sharing would fail.
Breaking-change call: NON_BREAKING (unchanged)
The fix commit only touches the new merge_parent / parent_fields code paths, the transformer default type for RecordExpander.parent_fields, and descriptions. The remain_original_record path and existing RecordExpander behavior are untouched; the airbytehq/airbyte count from the previous review still holds (1/511 manifests use record_expander, source-stripe, remain_original_record only; 0 use the new options).
No new findings. Approving.
1 participant
What
Adds
parent_fieldsto the existingRecordExpander: a list of named values to copy from the record being expanded onto each expanded item.Also adds
merge_parent, a boolean that shallow-merges the parent record, minus the expanded list, underneath each expanded item, the item's own keys winning on collision.Why
RecordExpandercan already carry parent context onto each expanded item, but only throughremain_original_record: true, which deep-copies the whole parent once per item (record_expander.py,_apply_parent_context). For a GitHub pull request node with ten reviews that copies the entire pull request ten times in order to carry one URL.parent_fieldscopies only what is named.This replaces #1164 (closed), which proposed a new
NestedRecordExtractorcomponent for the same job. An architecture review of #1149/#1162/#1163/#1164 found thatDpathExtractor.record_expander— which has been onmainsince before #1164 was written, and which neither #1164 nor the source-github PR that motivated it mentioned — already reproduces both of source-github's custom extractors record-for-record across seven response shapes, including the four-levelpull_request_comment_reactionstraversal, when combined withAddFieldsandRemoveFields. What #1164 genuinely added overRecordExpanderwas the named copy. So the gap is real; a parallel component to close it is not.Shipping #1164 would have left
mainwith two ways to write "explode this list and stamp the parent onto each child", differing in which field carries the parent, whether the copy is whole or named, and whether truncation is handled — with no way for a Connector Builder user to choose between them, and every future feature needing to be added twice.Adopters
tolik0/source-github/graphql-streams):NestedGraphQLRecordExtractorandDeepNestedGraphQLRecordExtractorare replaced byDpathExtractorwith aRecordExpanderusingparent_fields, combined with theunionmode of feat: add CombinedExtractor for combining multiple record extractors #1162 to cover the listing and drill-down documents, plusAddFieldsforrepositoryanduser.type. Verified by executing both on this branch against the connector's seven fixture shapes (reviews listing, drill-down, null repository; reactions from the repository root and from PullRequest, Review and Commentnoderoots): every shape matches record-for-record, and noRemoveFieldsstep is needed becauseoriginal_recordis never embedded. About 120 lines of the connector'scomponents.py; the two pagination strategies andReleasesRecordTransformationstay.components.py:11-31): single-level flatten ofticket_events[].child_events[]copying three parent fields, with aRecordFilteronevent_type == "Comment"after expansion.email_activity(components.py:16):{**record, **activity_item}for each item ofrecord.pop("activity"). Replaced byexpand_records_from_field: ["activity"]withmerge_parent: true; nothing else is needed, since the child fields win and theactivitykey is removed exactly as the connector does. The factory test reproduces this shape end to end.components.py:16-45): for eachusage[]item,del record["usage"]thenrecord.update(subrecord), plus a quantity filter and abillable_metricunnest. Replaced byexpand_records_from_field: ["usage"]withmerge_parent: true, aRecordFilterwithcondition: "{{ record['quantity'] > 0 }}",AddFieldsforbillable_metric_name({{ record['billable_metric']['name'] }}),billable_metric_id({{ record['billable_metric']['id'] }}) andsubscription_id({{ stream_slice['subscription_id'] }}), andRemoveFieldson["billable_metric"].Checked and not covered by this PR alone, with the change that would cover them:
components.py:27and source-hubspot associationscomponents.py:894(results[].to[].associationTypes[]) expand twice and stamp fields from both ancestors; that is chained expanders, see below.components.py:144expands the values of a dict and stamps the key onto each child; one adopter, and it interacts withtruncated_list_retriever, which assumes a list.Design decisions
parent_fieldsandremain_original_recordare independent and may both be set. The review recommended thatparent_fieldsreplace the deep copy when present; I implemented them as orthogonal instead, because silently disabling a set option is surprising. The efficiency win comes from usingparent_fieldsinstead ofremain_original_record, which the docstring and the schema description both state.parent_paththe parent does not have copiesNone, rather than skipping the field. This matchesparent.get(field)in the connector classes this replaces, and it means a present-but-null parent value and a missing one behave the same. Tests pin both.record_pathis overwritten. Also matches the connector classes.RecordExpander._reject_globsand the rule already applied totruncation_indicator_path: each path must identify a single field. The shared message no longer claims that truncation handling is involved, sinceparent_fieldsreaches it too.type: ParentFieldPathis optional in a manifest.RecordExpander.parent_fieldsis registered inmanifest_component_transformer.DEFAULT_MODEL_TYPES, the same wayAddFields.fieldsmaps toAddedFieldDefinition, so a Builder user can write the entries without atypekey. A factory test covers the untyped form end to end.dpath.getreturns the parent's object by reference and a shallow merge aliases every nested container, so without this a downstreamAddFieldswith a multi-segment path would write into the parent record and into every item already yielded. Forparent_fieldsthe copy costs proportionally to the named value; formerge_parentit costs the parent minus the expanded list, which is still cheaper thanremain_original_record. Scalars are passed through untouched._apply_parent_context, the one point both the embedded-items path and thetruncated_list_retrieverpath pass through, so fetched items are stamped identically to embedded ones. Covered by a test.remain_original_recordwas set, in which case it was wrapped as{"value": ..., "original_record": ...}. It is now wrapped as{"value": ...}plus the copied fields whenever any of the three parent-context options is set, and still yielded bare when none is. Both cases are tested, formerge_parenttoo.merge_parent, thenparent_fields, thenremain_original_record. The merge lays the parent underneath the item first, the named copies are written on top of that (so aparent_fieldsentry can overwrite a merged value, which is tested), andoriginal_recordis embedded last. All three options are independent and may be combined.merge_parentremoves only the expanded list from the parent's copy. For a single-segment path that is the top-level key; for a multi-segment path such as["reviews", "nodes"]thereviewskey stays with its other fields (totalCount,has_more) and onlynodesis dropped from a copy of it. Every match of a glob path is removed. The stripped parent is built once per parent record, not once per item, and then deep-copied per item so that the parent and the sibling items stay isolated.Breaking change: NON_BREAKING
parent_fieldsis a new optional field defaulting toNone, andmerge_parenta new boolean defaulting tofalse. The only behavioural change to existing code paths is the scalar-item wrapping described above, and it is unreachable without setting one of the parent-context options —_carries_parent_context()returnsFalseotherwise, which is exactly the pre-existing condition. A manifest that sets neitherparent_fieldsnormerge_parentproduces byte-identical output.Generated model
Hand-applied, then verified. Docker was unavailable, so rather than skip the check I ran the pinned generator directly —
datamodel_code_generator==0.26.3with the flag set frombin/generate_component_manifest_files.py— and confirmed theParentFieldPathclass and theparent_fieldsfield are byte-identical to its output afterruff format. Themerge_parentfield was hand-applied the same way, following theremain_original_recordpattern on the same class (Optional[bool] = Field(False, ...)with the description copied from the YAML). The rest of the generated file was discarded: the pinned generator emitsconint(...)/confloat(...)annotations that mypy rejects, because the invocation does not pass--field-constraints.Testing
unit_tests/sources/declarative/expanders/test_record_expander.py— copy onto every item, nooriginal_recordembedded, independence fromremain_original_record, nested read and nested write, overwrite, missing parent field, present-null parent field, several entries applied in order, parent not mutated, scalar wrapping with and without parent context, items from the truncated-list retriever, glob rejection in each path, empty-path rejection, config interpolation of both paths.reviewslisting shape end to end throughRecordSelectorand asserting the three expected records.merge_parentin the same file — the source-mailchimp shape end to end asserted on the full list, item wins on collision, multi-segment path keeps siblings and removes only the list, glob path removes every matched list (sections as a dict and as a list),parent_fieldsapplied after the merge overwrites a merged value, combined withremain_original_record, scalar items wrapped as{"value": ...}and merged, parent not mutated, each item getting its own copy of a nested value (writing into one reaches neither the parent nor its siblings), items from the truncated-list retriever merged (both a mapping and a scalar), and unsetmerge_parentleaves items unchanged.merge_parent: truebuilds the expander with the flag set and reproduces the mailchimp shape throughRecordSelector; one without it defaults toFalse.parent_fields, the same for a value merged bymerge_parent(both assert that writing into item 0 leaves item 1 and the parent untouched), and the glob-rejection message no longer mentions truncation handling. Plus a factory test buildingparent_fieldsfrom a manifest that omitstype: ParentFieldPath. The test asserting that untouched nested values were shared is replaced by its opposite.120c1efd, run per directory with the worktree on the import path:expanders77 passed,parsers212 passed,retrievers52 passed,extractors82 passed withtest_response_to_file_extractor_memory_usagedeselected (it dies with SIGILL on this machine onmainas well, before any of this code runs).mypyclean onrecord_expander.pyandmodel_to_component_factory.py;ruff formatandruff checkclean on every touched file.Not in scope
Chained expanders, so a grandchild can be stamped from both parent and grandparent: two fleet instances (source-zendesk-talk
components.py:27, source-hubspotcomponents.py:894), own PR when one of them migrates.merge_parentlanded in this PR (second commit).Review round 2
Addresses the review of
120c1efd(third commit):merge_parentaliased every nested container of the parent into every sibling item; a nestedAddFieldsafter the expander overwrote already-yielded records and the parentmerge_baseis deep-copied per item in_apply_parent_context. Reviewer's repro now yieldsitem_id1, 2, 3 and leavesparent["meta"]at{"list": "L"}parent_fieldscopyParentFieldPath.copy_ontodeep-copies a container value; scalars pass through. Reviewer's repro now yieldsreview_id1 and 2 withparent["repository"]unchangedtype: ParentFieldPathand did not validate, sinceRecordExpander.parent_fieldswas missing fromDEFAULT_MODEL_TYPESparent_fieldspathdpath.newbehaviour onrecord_path(scalar intermediate raises, numeric segment creates a list) was undocumentedrecord_pathschema description, noting it matchesAddFieldsThe two P1 fixes make the "no deep copy" claim in the previous description wrong, and the design-decision bullets and docstrings are updated accordingly: the efficiency argument for
parent_fieldsoverremain_original_recordis that it copies the named value rather than the whole parent, not that it copies nothing.CI at this head
606c32f3(review round 2): CI pending. At120c1efd, Pytest (Fast) and Pytest (All) on Python 3.10–3.13, MyPy, Ruff Lint and Ruff Format all passed;Check: destination-motherduckfailed and is unrelated, it is red on #1149 and #1162 at the same time. Locally on the new head:unit_tests/sources/declarativefull suite passed,mypyclean onrecord_expander.pyandmanifest_component_transformer.py,ruff checkandruff formatclean.🤖 Generated with Claude Code