-
Notifications
You must be signed in to change notification settings - Fork 30
fix(IncrementingCountCursor): Raise a better error when users attempt to use IncrementingCountCursor with partition router #784
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
Conversation
…r with partition router
👋 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/incrementing_count_cursor_with_partition_router_error_message#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/incrementing_count_cursor_with_partition_router_error_message Helpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
📝 WalkthroughWalkthroughAdds a validation in _build_concurrent_cursor to raise a ValueError when an IncrementingCountCursorModel is used with a non-single PartitionRouter. Updates unit tests to assert the error is raised when combining IncrementingCountCursor with ListPartitionRouter, with a duplicated test present. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant SM as StreamModel
participant F as ModelToComponentFactory
participant PR as PartitionRouter
participant CC as ConcurrentCursor
SM->>F: create_component(model)
F->>PR: resolve_partition_router(model)
Note over F,PR: New guard check
alt IncrementingCountCursorModel + non-single PR
F-->>SM: raise ValueError("Unsupported combination")
else Other combinations
F->>CC: build concurrent cursor
CC-->>F: cursor instance
F-->>SM: component instance
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
unit_tests/sources/declarative/parsers/test_model_to_component_factory.py (1)
3672-3722
: Consider verifying the error message content, wdyt?Since the PR's main objective is to provide a clearer error message when users attempt to use IncrementingCountCursor with a partition router, it would be valuable to assert the actual error message in this test. Currently, the test only verifies that a
ValueError
is raised, but doesn't check that it contains the expected helpful message.You could enhance the test like this:
- with pytest.raises(ValueError): + with pytest.raises(ValueError) as exc_info: factory.create_component( model_type=DeclarativeStreamModel, component_definition=YamlDeclarativeSource._parse(content), config=input_config, ) + assert "PartitionRouter and an IncrementingCountCursor" in str(exc_info.value) + assert "test" in str(exc_info.value) # stream name should be in the messageThis way, future changes won't accidentally break the improved error messaging without the test catching it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
(1 hunks)unit_tests/sources/declarative/parsers/test_model_to_component_factory.py
(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
unit_tests/sources/declarative/parsers/test_model_to_component_factory.py (1)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (2)
ModelToComponentFactory
(648-4185)create_component
(789-822)
⏰ 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). (12)
- GitHub Check: Check: source-pokeapi
- GitHub Check: Check: source-hardcoded-records
- GitHub Check: Check: source-shopify
- GitHub Check: Check: destination-motherduck
- GitHub Check: Check: source-intercom
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.13, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.12, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Manifest Server Docker Image Build
unit_tests/sources/declarative/parsers/test_model_to_component_factory.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
We've had a fe different customers write in with issues related to using the IncrementingCountCursor and partition routing at the same time which is not currently supported.
The old error was not very helpful mainly saying:
Expected manifest component of type DatetimeBasedCursor, but received IncrementingCountCursor instead
which is only really useful if you can reverse engineering our component parsing.Note: At the moment, this will still not be super useful for the connector builder since all errors get surfaced as
Internal Server Error: Server error : 500 Internal Server Error Internal Server Error
. But once we fix that builder bug, then this will properly flag and hopefully at least give more info on next stepsSummary by CodeRabbit