fix: do not advertise incremental sync mode for streams without cursor field#876
Conversation
…r field Co-Authored-By: AJ Steers <aj@airbyte.io>
Original prompt from AJ Steers |
🤖 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:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where declarative streams without incremental sync configuration were incorrectly advertising support for incremental sync mode. The root cause was that streams without incremental_sync configured were receiving an empty CursorField object instead of None, causing them to incorrectly report incremental sync capability.
Key changes:
- Changed the fallback value for
cursor_fieldfromCursorField(cursor_field_key="")toNonewhen a cursor doesn't have acursor_fieldattribute - Removed a FIXME comment acknowledging this issue
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. Testing 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@devin/1767906101-fix-incremental-sync-mode-for-streams-without-cursor#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 devin/1767906101-fix-incremental-sync-mode-for-streams-without-cursorHelpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
Co-Authored-By: AJ Steers <aj@airbyte.io>
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (1)
2113-2117: Nice fix! This correctly prevents advertising incremental sync for streams without a cursor.The change properly handles the case where
FinalStateCursor(returned when there's noincremental_syncconfiguration) doesn't have acursor_fieldattribute. By returningNoneinstead of an emptyCursorField, theDefaultStream.as_airbyte_stream()method'sif self._cursor_field:check ensures streams without incremental sync only advertisefull_refreshmode and skip the incremental sync mode addition.Would it be worth adding a test to verify both scenarios (streams with and without incremental_sync advertising correctly)? This could help prevent regressions if the cursor field handling logic changes in the future, wdyt?
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ChristoGrab
Repo: airbytehq/airbyte-python-cdk PR: 58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the `YamlDeclarativeSource` class in `airbyte_cdk/sources/declarative/yaml_declarative_source.py`, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.
🧬 Code graph analysis (1)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (5)
airbyte_cdk/sources/file_based/stream/concurrent/adapters.py (1)
cursor_field(133-137)airbyte_cdk/sources/streams/concurrent/adapters.py (1)
cursor_field(189-193)airbyte_cdk/sources/streams/concurrent/default_stream.py (1)
cursor_field(52-53)airbyte_cdk/sources/streams/concurrent/cursor.py (1)
cursor_field(213-214)airbyte_cdk/sources/streams/core.py (1)
cursor_field(363-368)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Check: destination-motherduck
- GitHub Check: Check: source-shopify
- GitHub Check: Check: source-hardcoded-records
- GitHub Check: Check: source-intercom
- GitHub Check: Pytest (Fast)
- GitHub Check: Manifest Server Docker Image Build
- GitHub Check: SDM Docker Image Build
- GitHub Check: Pytest (All, Python 3.12, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.13, Ubuntu)
80bcfc5
into
main
Summary
Fixes a bug where declarative streams without
incremental_syncconfiguration were incorrectly advertising support for incremental sync mode with an empty cursor field (defaultCursorField: [""]).Root cause: When a stream doesn't have
incremental_syncconfigured,_build_concurrent_cursor()returns aFinalStateCursorwhich lacks acursor_fieldattribute. The code was falling back toCursorField(cursor_field_key="")instead ofNone. SinceDefaultStream.as_airbyte_stream()checksif self._cursor_field:, the CursorField object (despite having an empty key) caused incremental sync to be advertised.The fix: Change the fallback from
CursorField(cursor_field_key="")toNone, so streams without incremental sync configuration correctly advertise onlyfull_refreshmode.Updates since last revision
Review & Testing Checklist for Human
incremental_syncstill work correctly - Test a connector with incremental sync configured and confirm it still advertises incremental mode with the correct cursor fieldincremental_syncnow only showfull_refresh- Test source-pokeapi or similar connector and confirm the catalog no longer shows incremental as a supported sync modeRecommended test plan: Run
discoveron source-pokeapi and verify thepokemonstream only showssupported_sync_modes: ["full_refresh"](not["full_refresh", "incremental"]).Notes
FIXMEcomment that acknowledged the issueLink to Devin run: https://app.devin.ai/sessions/81b4056e835d448fa41bf4ae376f8b7f
Requested by: Aaron ("AJ") Steers (@aaronsteers)
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.