fix(low-code CDK): reference to the DeclarativeStreamModel which is the actual type passed in #722
Conversation
👋 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@brian/fix_state_migration_to_use_declarative_stream_model#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 brian/fix_state_migration_to_use_declarative_stream_modelHelpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
📝 WalkthroughWalkthroughReplaced DeclarativeStream with DeclarativeStreamModel in unit_tests/sources/declarative/custom_state_migration.py, updating the import path and type annotations for the declarative_stream attribute and init parameter. No other logic changes are indicated. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
unit_tests/sources/declarative/custom_state_migration.py (1)
24-26: Bug: evaluated cursor field is computed but not used in migrate(); use the resolved value, wdyt?You compute
self._cursor_fieldviaInterpolatedString.create(...).eval(self._config)but still referenceself._cursor.cursor_field(possibly a template) when reading and writing state. This can cause KeyError or incorrect keys if templating is involved.- updated_at = stream_state[self._cursor.cursor_field] + updated_at = stream_state[self._cursor_field] migrated_stream_state = { "states": [ { "partition": {"type": "type_1"}, - "cursor": {self._cursor.cursor_field: updated_at}, + "cursor": {self._cursor_field: updated_at}, }, { "partition": {"type": "type_2"}, - "cursor": {self._cursor.cursor_field: updated_at}, + "cursor": {self._cursor_field: updated_at}, }, ] }Also applies to: 31-45
🧹 Nitpick comments (2)
unit_tests/sources/declarative/custom_state_migration.py (2)
16-17: Remove stray class-levelconfigannotation (or initialize an instance attribute), wdyt?There’s a class-level
config: Configannotation but onlyself._configis set in__init__. This can confuse readers and type checkers.Apply this minimal cleanup:
class CustomStateMigration(StateMigration): declarative_stream: DeclarativeStreamModel - config: Config
19-23: Minor robustness in init: type the assignment and default parameters to dict, wdyt?Typing the instance attribute and guarding against
Noneparameters avoids edge-case Nones from models and keeps type-checkers happy.- def __init__(self, declarative_stream: DeclarativeStreamModel, config: Config): - self._config = config - self.declarative_stream = declarative_stream - self._cursor = declarative_stream.incremental_sync - self._parameters = declarative_stream.parameters + def __init__(self, declarative_stream: DeclarativeStreamModel, config: Config): + self._config: Config = config + self.declarative_stream = declarative_stream + self._cursor = declarative_stream.incremental_sync + self._parameters = (declarative_stream.parameters or {})Optional: consider asserting
self._cursoris present for clearer failures when a non-incremental stream is passed (can be added later if needed).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
unit_tests/sources/declarative/custom_state_migration.py(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ChristoGrab
PR: airbytehq/airbyte-python-cdk#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.
📚 Learning: 2024-11-18T23:40:06.391Z
Learnt from: ChristoGrab
PR: airbytehq/airbyte-python-cdk#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.
Applied to files:
unit_tests/sources/declarative/custom_state_migration.py
📚 Learning: 2024-11-15T01:04:21.272Z
Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/cli/source_declarative_manifest/_run.py:62-65
Timestamp: 2024-11-15T01:04:21.272Z
Learning: The files in `airbyte_cdk/cli/source_declarative_manifest/`, including `_run.py`, are imported from another repository, and changes to these files should be minimized or avoided when possible to maintain consistency.
Applied to files:
unit_tests/sources/declarative/custom_state_migration.py
🧬 Code graph analysis (1)
unit_tests/sources/declarative/custom_state_migration.py (1)
airbyte_cdk/sources/declarative/migrations/state_migration.py (1)
StateMigration(7-24)
⏰ 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). (4)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
🔇 Additional comments (1)
unit_tests/sources/declarative/custom_state_migration.py (1)
9-11: Verified: custom_state_migration.py has no runtimeDeclarativeStreamimports — I ran the sweep and confirmed that this file now only uses the Pydantic model alias. All remainingDeclarativeStreamimports live in other tests and source files by design. Does that match your expectation? wdyt?
brianjlai
deleted the
brian/fix_state_migration_to_use_declarative_stream_model
branch
We were accidentally referencing the runtime
DeclarativeStreamcomponent as a parameter to theCustomStateMigration. This should have actually instead been the Pydantic model of the component as shown by the fields we're referencing in the__init__(). This is needed for when we deprecate the legacyDeclarativeStreamcomponent.This also had the unfortunate effect of making a few of our actual connectors import the wrong component which prevents us from moving
DeclarativeStreamto a legacy folder.Summary by CodeRabbit