Repository navigation
fix(manifest-server): descriptor can be dict - #745
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@pedro/fix-type#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 pedro/fix-typeHelpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
📝 WalkthroughWalkthroughThe typing for StreamReadSlices.slice_descriptor in airbyte_cdk/manifest_server/api_models/stream.py was expanded from Optional[str] to Optional[Union[Dict[str, Any], str]], and Union was added to imports. No runtime logic or control flow changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Would you like to add a brief docstring or pydantic validator clarifying accepted shapes for slice_descriptor to guide downstream users, wdyt? ✨ 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
🧹 Nitpick comments (1)
airbyte_cdk/manifest_server/api_models/stream.py (1)
62-62: Makeslice_descriptortruly optional and consider accepting any mapping-like input
- Default the field to
Noneso Pydantic treats it as optional at validation time (it’s currently required despite theOptionalannotation)- Optionally switch
Dict[str, Any]toMapping[str, Any]to accept mapping-like inputs beyond plain dicts- We ran a grep over all
.slice_descriptorreferences and found no string-only operations (e.g., no.split,.upper, etc.), so widening tostrorMappingis safe- Can we confirm that our JSON Schema now emits
oneOf: [string, object]for this field?Proposed minimal diff:
- slice_descriptor: Optional[Union[Dict[str, Any], str]] # We're seeing strings at runtime + slice_descriptor: Optional[Union[Dict[str, Any], str]] = None # We're seeing strings at runtimeWdyt?
📜 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)
airbyte_cdk/manifest_server/api_models/stream.py(2 hunks)
⏰ 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). (1)
- GitHub Check: Check: source-shopify
🔇 Additional comments (1)
airbyte_cdk/manifest_server/api_models/stream.py (1)
9-9: Import update looks good for 3.9+ compatibilityAdding Union here is the right call given the broadened annotation and the project’s likely Python version support. LGTM, wdyt?
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a type annotation issue in the manifest server API models where the slice_descriptor field was incorrectly typed as only accepting strings, when it can actually be either a dictionary or a string at runtime.
- Updates the type annotation for
slice_descriptorfromOptional[str]toOptional[Union[Dict[str, Any], str]]
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Initially I changed this type to be a string (differing from the one declared by the TestReader response) because that's what I was observing the type to be in my tests with simple manifests. Running in prod for real users I noticed an error when returning the response because the descriptor was actually a dict (as declared)
So, instead of overriding to string, this declares the type as a union to support both responses.
Summary by CodeRabbit