fix(low-code cdk pagination): Fix the offset strategy so that it resets back to 0 when a stream is an incremental data feed #202
Conversation
…ack to 0 properly
brianjlai
changed the title
|
Warning Rate limit exceeded@brianjlai has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 32 minutes and 35 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe pull request introduces modifications to pagination strategies in the Airbyte CDK, focusing on enhancing the flexibility of reset methods in the Changes
Sequence DiagramsequenceDiagram
participant Paginator
participant OffsetIncrement
participant StopCondition
Paginator->>OffsetIncrement: reset(reset_value)
alt reset_value is None
OffsetIncrement-->>Paginator: Set offset to 0
else reset_value is not an integer
OffsetIncrement--xPaginator: Raise ValueError
end
Paginator->>StopCondition: reset(reset_value)
alt reset_value is provided
StopCondition->>StopCondition: Call delegate reset with value
else reset_value is None
StopCondition->>StopCondition: Call delegate reset without value
end
What do you think about these changes? They seem to add more flexibility to how reset values are handled! Wdyt? The sequence diagram might help visualize the new flow. Cheers! 🚀 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 1
🧹 Nitpick comments (2)
airbyte_cdk/sources/declarative/requesters/paginators/strategies/stop_condition.py (1)
56-59: Consider using a more concise implementation?The current implementation works correctly, but we could make it more concise using Python's optional parameter handling, wdyt?
- if reset_value: - self._delegate.reset(reset_value) - else: - self._delegate.reset() + self._delegate.reset(reset_value) if reset_value is not None else self._delegate.reset()airbyte_cdk/sources/declarative/requesters/paginators/strategies/offset_increment.py (1)
Line range hint
86-92: How about improving the error message?The error message could be more descriptive to help users understand the valid input range, wdyt?
if reset_value is None: self._offset = 0 elif not isinstance(reset_value, int): raise ValueError( - f"Reset value {reset_value} for OffsetIncrement pagination strategy was not an integer" + f"Reset value {reset_value} for OffsetIncrement pagination strategy must be either None or an integer" ) else:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
airbyte_cdk/sources/declarative/requesters/paginators/strategies/offset_increment.py(1 hunks)airbyte_cdk/sources/declarative/requesters/paginators/strategies/stop_condition.py(1 hunks)unit_tests/sources/declarative/requesters/paginators/test_default_paginator.py(3 hunks)unit_tests/sources/declarative/requesters/paginators/test_offset_increment.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-the-guardian-api' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
🔇 Additional comments (2)
unit_tests/sources/declarative/requesters/paginators/test_offset_increment.py (1)
107-108: LGTM!The test implementation is clean and covers all edge cases including the new None value handling.
unit_tests/sources/declarative/requesters/paginators/test_default_paginator.py (1)
364-396: Add assertions to validate the test case?The test sets up the paginator correctly but doesn't validate its behavior. Should we add some assertions to verify that:
- The paginator correctly stops when the cursor condition is met
- The offset resets to 0 when reset is called
- The page size is correctly applied
Here's a suggested implementation, wdyt?
response = requests.Response() response._content = json.dumps({"updated_at": "2024-01-02"}).encode("utf-8") # Verify initial state assert paginator.get_request_params() == {"limit": 2, "offset": 0} # Verify pagination stops when cursor condition is met token = paginator.next_page_token(response, 2, {"updated_at": "2023-12-31"}) assert token is None # Verify offset resets to 0 paginator.reset() assert paginator.get_request_params() == {"limit": 2, "offset": 0}✅ Verification successful
The suggested assertions look good and align with existing test patterns! 👍
The proposed assertions cover the key behaviors we typically test for paginators in the codebase:
- Initial state validation
- Cursor-based stop condition
- Reset functionality
- Page size verification
I noticed we could also add one more assertion to make it even more robust, wdyt?
# Verify pagination continues when cursor condition is not met token = paginator.next_page_token(response, 2, {"updated_at": "2024-01-03"}) assert token == 2 # Next offset should be 2🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find test files containing DefaultPaginator tests rg -l "DefaultPaginator" --type py "unit_tests/" # Look for similar test patterns rg "test.*paginator.*cursor" --type py "unit_tests/" -B 2 -A 2 # Search for assertion patterns in paginator tests rg "assert.*paginator" --type py "unit_tests/" -B 1 -A 1Length of output: 11753
unit_tests/sources/declarative/requesters/paginators/test_default_paginator.py
Outdated
Show resolved
Hide resolved
Fixes https://github.com/airbytehq/oncall/issues/7210
What
We were observing this error for low-code connectors using the offset pagination strategy:
How
After investigating this a bit, I realized that this was only happening when we used an incremental stream where
is_data_feedwas enabled.And that is because the
StopConditionPaginationStrategyDecoratorthat gets instantiated for data feeds is not implemented correctly for reseting back to 0. We accept optional values, but the problem is that the decorator can still accidentally pass inNonewhen the surroundingreset()passes None. Instead we need to callreset()with no incoming parameter. I also fixed this in theOffsetPaginationStrategyas well just to cover all our bases.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests
Bug Fixes
Nonereset values in offset increment and stop condition strategies