-
Notifications
You must be signed in to change notification settings - Fork 67
fix: resolve issue where some destinations will fail due to null/missing stream namespace
keys in catalog
#748
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 16 commits
4e24489
054093a
0c2f196
5c05e1d
761554e
3a2cb45
d098050
201187a
808ec52
9aad1fb
85a3b4e
4c5a6d2
50b07ad
89ff68a
9c4f594
82e7730
405b947
35d9d60
d0a6f24
a8205c4
c8d1d41
c3dce14
88756af
57c9863
dbb96d0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -265,7 +265,7 @@ def _write_airbyte_message_stream( | |||||||||||||||||||||||||||
with as_temp_files( | ||||||||||||||||||||||||||||
files_contents=[ | ||||||||||||||||||||||||||||
self._hydrated_config, | ||||||||||||||||||||||||||||
catalog_provider.configured_catalog.model_dump_json(), | ||||||||||||||||||||||||||||
catalog_provider.configured_catalog.model_dump_json(exclude_none=True), | ||||||||||||||||||||||||||||
] | ||||||||||||||||||||||||||||
Comment on lines
265
to
269
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainSwitching to exclude_none=True for catalog JSON avoids strict validator null errors — please confirm downstream tolerance and add a rationale comment. This aligns with the PR’s intent to stop surfacing explicit nulls in ConfiguredAirbyteStream fields (e.g., namespace, source_defined_cursor, is_resumable). The change reduces spurious OpenAPI4J validation failures. The one behavioral change to watch is that optional fields previously present with null will now be omitted entirely; if any destination implementations (custom or legacy) were relying on the presence of keys even when null, they might need an adjustment. Would you mind confirming there are no destinations that require those keys to be present when the values are None, and adding a brief comment here to document the rationale, wdyt? Suggested inline comment for future readers: with as_temp_files(
files_contents=[
self._hydrated_config,
- catalog_provider.configured_catalog.model_dump_json(exclude_none=True),
+ # Omit None-valued optional fields (e.g., namespace, source_defined_cursor, is_resumable)
+ # to avoid strict validators rejecting explicit nulls in some CDKs.
+ catalog_provider.configured_catalog.model_dump_json(exclude_none=True),
]
) To help double-check consistency across the repo, you could run this quick search to ensure all configured catalog dumps now use exclude_none=True (and spot any stragglers): 🏁 Script executed: #!/bin/bash
# Find all places where configured_catalog JSON is dumped
rg -n -A2 -B2 'configured_catalog\.model_dump_json\('
# Inspect all model_dump_json calls and see which ones use exclude_none=True
rg -n -A1 -B1 'model_dump_json\('
# Optional: scan for hardcoded "namespace": null in code/tests
rg -n -A2 -B2 '"namespace"\s*:\s*null' Length of output: 2474 Add a short rationale comment here and confirm downstream tolerance for omitted None fields — wdyt? Quick check: rg shows this repo already uses model_dump_json(exclude_none=True) in multiple places and I found no occurrences of a hardcoded Files to update / reference:
Suggested inline diff to add at the destinations file: with as_temp_files(
files_contents=[
self._hydrated_config,
+ # Omit None-valued optional fields (e.g., namespace, source_defined_cursor, is_resumable)
+ # to avoid strict validators rejecting explicit nulls in some CDKs.
catalog_provider.configured_catalog.model_dump_json(exclude_none=True),
]
) Would you mind adding that comment and confirming there are no destination implementations that expect those keys to exist with null values? wdyt? 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
|
||||||||||||||||||||||||||||
) as [ | ||||||||||||||||||||||||||||
config_file, | ||||||||||||||||||||||||||||
|
Uh oh!
There was an error while loading. Please reload this page.