fix(source-google-analytics-data-api): scope stream naming mapping to top-level name - #83343
fix(source-google-analytics-data-api): scope stream naming mapping to top-level name#83343devin-ai-integration[bot] wants to merge 3 commits into
Conversation
…pping to top-level name Co-Authored-By: bot_apk <apk@cognition.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksPR Slash CommandsAirbyte Maintainers (that's you!) can execute the following slash commands on your PR:
📚 Show Repo GuidanceHelpful Resources
|
Co-Authored-By: bot_apk <apk@cognition.ai>
|
Note Autopilot progressive rollouts are not enabled for the following modified connector(s):
This is a courtesy heads-up only — it does not block merge or fail any check. |
|
Deploy preview for airbyte-docs ready!
Deployed with vercel-action |
|
Co-Authored-By: bot_apk <apk@cognition.ai>
|
↪️ Triggering Reason: Draft |
|
✅ Fix Proven —
|
| Check | Result |
|---|---|
| Regression test (comparison mode, all streams) | ✅ SPEC · ✅ CHECK · ✅ DISCOVER · ✅ READ |
SPEC / DISCOVER output, target vs control |
byte-identical (59 streams, identical name set) |
READ records, target vs control |
byte-identical — all 1799 records, identical per-stream counts |
| New unit test pins the fix | ✅ fails on the old field_path, passes on the new one |
| Resolved-config equivalence, 3 configs | ✅ identical, including the 171-stream / 114-suffixed-name multi-property case |
🔬 Evidence
1. Live regression test — workflow run. Target = connector built from PR head c170a91 (2.9.46, SDM base 7.23.8); control = published 2.9.45. All four commands passed, no both_failed, no infrastructure annotation. Going beyond the summary table into the artifacts: spec.jsonl and catalog.jsonl are byte-identical after canonical sort, and every one of the 1799 READ records matches between versions, with identical RECORD/STATE/LOG/TRACE counts and identical per-stream record counts across all 53 streams that returned data.
2. The added unit test actually pins the fix (mutation test). Locally, the connector unit suite is 16 passed on the PR head. Reverting only the manifest line back to ["**", "name"] turns that into 1 failed, 15 passed, with test_dynamic_stream_naming_preserves_nested_name failing on its nested-name assertion. So the test fails on the old behavior and passes on the new — it is not a tautological test. It also asserts the first two positional names, so it covers the multi-property branch.
3. Independent equivalence check. Resolving the real manifest.yaml under both field paths and diffing the entire resolved dynamic-stream config list (not just names) reproduces the PR's claim:
| Config | Streams | Suffixed names | Resolved configs identical? |
|---|---|---|---|
| 1 property | 57 | 0 | ✅ |
| 3 properties | 171 | 114 | ✅ |
| 1 property + custom report | 58 | 0 | ✅ |
Positional naming (daily_active_users, daily_active_usersProperty222, …) is preserved exactly.
4. Was the change exercised? Yes. Every stream's name is written through this mapping, so all 59 discovered streams in the regression run went through the corrected path — and the differing behavior itself is covered by (2). The differing behavior is not reachable from any customer config: it requires a second, nested name key in the stream template (or a reordered components_mapping), which is a property of the manifest, not of user config. That is why this is validated by regression equivalence plus the unit test rather than by pinning a live connection — a pinned sync would reproduce the byte-identical result already shown, with customer risk and no extra signal. Testing detail is recorded in the internal oncall issue.
✅ Pre-flight checks
- Viability — one-line declarative correction plus a regression test, patch bump
2.9.45 → 2.9.46, changelog row. It addresses the reported defect directly. - Safety — manifest + unit test + metadata + docs only. No new network calls, no credential handling, no new dependencies, no obfuscation.
- Breaking change: none. No spec, schema, primary-key, cursor, or state-format change; no stream added, removed, or renamed. The interpolated value producing the
Property<id>suffix is untouched. - Reversibility — trivially revertible; downgrading to
2.9.45resolves identical stream configs, with no config or state migration.
Notes
- The positional
Property<id>naming this mapping produces stays unchanged by design — this PR removes an order-dependent over-write, it does not change naming semantics. The ID-stable renaming discussed on the linked issue remains a separate, breaking change. - Pre-release
2.9.46-preview.c170a91is published for downstream use. The first publish attempt failed on a transient GitHub429 Too Many Requestsand was re-triggered; no evidence above depends on that image, since the regression test builds the connector from source. - Recommended next step:
/ai-canary-prereleasebefore promoting to a normal release.
|
|
|
↪️ Triggering Reason: |
Reviewing PR for connector safety and quality.
|
🛡️ AI PR Review Report🟢 Review Action: APPROVEDAll 12 gates passed. 🔶 Risk Level: 2/5Single-connector declarative correction with regression-proven identical resolved stream configs; no existing behavior changed. 📋 PR DetailsConnector(s): 🔍 Gate Evaluation DetailsGate-by-Gate Analysis
Change surface: 📚 Evidence ConsultedEvidence
|
|
↪️ Triggering Reason: |
Auto-merge evaluation: FAILPreconditions (all must pass)
Change scope (at least one must pass)❌ No matching change scope detected. |
|
🙋 Escalated to #human-in-the-loop per Hands-Free AI Triage Project triage next step. Reason: |
What
Related to https://github.com/airbytehq/oncall/issues/13231:
The dynamic-stream naming
ComponentMappingDefinitioninmanifest.yamlusedfield_path: ["**", "name"]. The CDK passesfield_pathsegments straight todpath.set(airbyte_cdk/sources/declarative/resolvers/config_components_resolver.py), anddpathtreats**as a recursive glob — so this mapping sets everynamekey at any depth in the resolved stream config, not just the stream name.Today that happens to be harmless: at the point this mapping runs, the only
namekey in the template is the top-level stream name (request_body_json.metrics/dimensionsare still empty lists and are populated by later mappings). So the current correctness depends on mapping order and on the template containing no othernamekey. Adding any nestedname:to the stream template, or reorderingcomponents_mapping, would silently corrupt request bodies. Injecting a single nestednameinto the template reproduces the clobber:retriever.requester.request_parameters.namecomes back asdaily_active_usersProperty222instead of the injected value.This PR scopes the mapping to the top-level stream name. Naming semantics are unchanged — the positional
Property<id>suffixing behavior is preserved exactly.How
One-line manifest change:
The interpolated
valueis untouched.Equivalence proof
The resolved
dynamic_streamsoutput was generated with both the old and newfield_pathand diffed as canonical JSON — full resolved stream configs, not just names (includingretriever.requester.request_body_jsonmetrics/dimensions,schema_loaderpaths, transformations, paginator, and incremental sync). Three configs were compared:property_ids: ["111"],property_ids: ["111","222","333"], and a config with acustom_reports_array(cohort report) entry.And the behavior that actually differs — with a nested
namepresent in the template:Declarative-First Evaluation
This is a declarative-only fix: a single
field_pathcorrection on an existingComponentMappingDefinition. No custom Python component was added or modified.Review guide
airbyte-integrations/connectors/source-google-analytics-data-api/manifest.yaml— the one-linefield_pathchange.airbyte-integrations/connectors/source-google-analytics-data-api/unit_tests/test_components.py—test_dynamic_stream_naming_preserves_nested_nameresolves the real manifest with an injected nestednameand asserts it survives while stream names stay positional. Verified this test fails on the old["**", "name"]path and passes with the fix. Full suite:16 passed.metadata.yaml+docs/integrations/sources/google-analytics-data-api.md— patch bump to 2.9.46 and changelog row.Serhii Lazebnyi (@lazebnyi) — you introduced this mapping in #61550; flagging you for the naming semantics, which this PR deliberately leaves unchanged.
User Impact
None. The resolved stream configs are byte-identical for all tested configurations, so no stream is renamed and no request body changes. This removes an order-dependent footgun so future edits to the stream template can safely introduce nested
namekeys.Out of scope (tracked on the oncall issue)
Property<id>suffixing to ID-stable suffixing — that renames existing streams and needs the breaking-change process plus a product decision.property_ids: ["111"]plus a custom report namedpagesyields two streams namedpages). Real defect, but the remedy is config validation, which is not cleanly expressible in the declarative resolver.Can this PR be safely reverted and rolled back?
Requested via the
/ai-fixworkflow on https://github.com/airbytehq/oncall/issues/13231.Devin session