-
Notifications
You must be signed in to change notification settings - Fork 43
feat(declarative): Add interpolation support for CursorPagination page_size #878
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
Changes from all commits
c75bc79
f091b89
ebe431e
2ab1416
8fb98a6
3226f7c
afad0ce
54d4e2d
39ac22e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1554,14 +1554,18 @@ def create_concurrent_cursor_from_incrementing_count_cursor( | |||||||||||||||||
| f"Expected {model_type.__name__} component, but received {incrementing_count_cursor_model.__class__.__name__}" | ||||||||||||||||||
| ) | ||||||||||||||||||
|
|
||||||||||||||||||
| interpolated_start_value = ( | ||||||||||||||||||
| InterpolatedString.create( | ||||||||||||||||||
| incrementing_count_cursor_model.start_value, # type: ignore | ||||||||||||||||||
| start_value: Union[int, str, None] = incrementing_count_cursor_model.start_value | ||||||||||||||||||
| # Pydantic Union type coercion can convert int 0 to string '0' depending on Union order. | ||||||||||||||||||
| # We need to handle both int and str representations of numeric values. | ||||||||||||||||||
| # Evaluate the InterpolatedString and convert to int for the ConcurrentCursor. | ||||||||||||||||||
| if start_value is not None: | ||||||||||||||||||
| interpolated_start_value = InterpolatedString.create( | ||||||||||||||||||
| str(start_value), # Ensure we pass a string to InterpolatedString.create | ||||||||||||||||||
| parameters=incrementing_count_cursor_model.parameters or {}, | ||||||||||||||||||
| ) | ||||||||||||||||||
| if incrementing_count_cursor_model.start_value | ||||||||||||||||||
| else 0 | ||||||||||||||||||
| ) | ||||||||||||||||||
| evaluated_start_value: int = int(interpolated_start_value.eval(config=config)) | ||||||||||||||||||
| else: | ||||||||||||||||||
| evaluated_start_value = 0 | ||||||||||||||||||
|
|
||||||||||||||||||
| cursor_field = self._get_catalog_defined_cursor_field( | ||||||||||||||||||
| stream_name=stream_name, | ||||||||||||||||||
|
|
@@ -1593,7 +1597,7 @@ def create_concurrent_cursor_from_incrementing_count_cursor( | |||||||||||||||||
| connector_state_converter=connector_state_converter, | ||||||||||||||||||
| cursor_field=cursor_field, | ||||||||||||||||||
| slice_boundary_fields=None, | ||||||||||||||||||
| start=interpolated_start_value, # type: ignore # Having issues w/ inspection for GapType and CursorValueType as shown in existing tests. Confirmed functionality is working in practice | ||||||||||||||||||
| start=evaluated_start_value, # type: ignore # Having issues w/ inspection for GapType and CursorValueType as shown in existing tests. Confirmed functionality is working in practice | ||||||||||||||||||
| end_provider=connector_state_converter.get_end_provider(), # type: ignore # Having issues w/ inspection for GapType and CursorValueType as shown in existing tests. Confirmed functionality is working in practice | ||||||||||||||||||
| ) | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
@@ -1745,10 +1749,16 @@ def create_cursor_pagination( | |||||||||||||||||
| self._UNSUPPORTED_DECODER_ERROR.format(decoder_type=type(inner_decoder)) | ||||||||||||||||||
| ) | ||||||||||||||||||
|
|
||||||||||||||||||
| # Pydantic v1 Union type coercion can convert int to string depending on Union order. | ||||||||||||||||||
| # If page_size is a string that represents an integer (not an interpolation), convert it back. | ||||||||||||||||||
| page_size = model.page_size | ||||||||||||||||||
| if isinstance(page_size, str) and page_size.isdigit(): | ||||||||||||||||||
| page_size = int(page_size) | ||||||||||||||||||
|
|
||||||||||||||||||
| return CursorPaginationStrategy( | ||||||||||||||||||
| cursor_value=model.cursor_value, | ||||||||||||||||||
| decoder=decoder_to_use, | ||||||||||||||||||
| page_size=model.page_size, | ||||||||||||||||||
| page_size=page_size, | ||||||||||||||||||
| stop_condition=model.stop_condition, | ||||||||||||||||||
| config=config, | ||||||||||||||||||
| parameters=model.parameters or {}, | ||||||||||||||||||
|
|
@@ -2917,8 +2927,14 @@ def create_offset_increment( | |||||||||||||||||
| else None | ||||||||||||||||||
| ) | ||||||||||||||||||
|
|
||||||||||||||||||
| # Pydantic v1 Union type coercion can convert int to string depending on Union order. | ||||||||||||||||||
| # If page_size is a string that represents an integer (not an interpolation), convert it back. | ||||||||||||||||||
| page_size = model.page_size | ||||||||||||||||||
| if isinstance(page_size, str) and page_size.isdigit(): | ||||||||||||||||||
| page_size = int(page_size) | ||||||||||||||||||
|
Comment on lines
+2933
to
+2934
|
||||||||||||||||||
| if isinstance(page_size, str) and page_size.isdigit(): | |
| page_size = int(page_size) | |
| if isinstance(page_size, str): | |
| try: | |
| page_size = int(page_size) | |
| except ValueError: | |
| # Leave page_size as-is if it's not a plain integer string (e.g., interpolation). | |
| pass |
Copilot
AI
Jan 23, 2026
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.
Same issue as above: isdigit() only detects positive digits, so some Union-coerced integer strings (e.g. "-1") won’t be converted back to int. Consider using an int() cast with try/except (and/or explicit positive validation) for more complete coercion handling.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -112,3 +112,43 @@ def test_last_record_is_node_if_no_records(): | |
| response = requests.Response() | ||
| next_page_token = strategy.next_page_token(response, 0, None) | ||
| assert next_page_token is None | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "page_size_input, config, expected_page_size", | ||
| [ | ||
| pytest.param(100, {}, 100, id="static_integer"), | ||
| pytest.param("100", {}, 100, id="static_string"), | ||
| pytest.param( | ||
| "{{ config['page_size'] }}", {"page_size": 50}, 50, id="interpolated_from_config" | ||
| ), | ||
| pytest.param("{{ config.get('page_size', 100) }}", {}, 100, id="interpolated_with_default"), | ||
| pytest.param( | ||
| "{{ config.get('page_size', 100) }}", | ||
| {"page_size": 200}, | ||
| 200, | ||
| id="interpolated_override_default", | ||
| ), | ||
| pytest.param(None, {}, None, id="none_page_size"), | ||
| ], | ||
| ) | ||
| def test_interpolated_page_size(page_size_input, config, expected_page_size): | ||
| """Test that page_size supports interpolation from config.""" | ||
| strategy = CursorPaginationStrategy( | ||
| page_size=page_size_input, | ||
| cursor_value="token", | ||
| config=config, | ||
| parameters={}, | ||
| ) | ||
| assert strategy.get_page_size() == expected_page_size | ||
|
Comment on lines
+117
to
+143
|
||
|
|
||
|
|
||
| def test_interpolated_page_size_raises_on_non_integer(): | ||
| """Test that initialization raises an exception when interpolation resolves to a non-integer.""" | ||
| with pytest.raises(Exception, match="is of type .* Expected"): | ||
| CursorPaginationStrategy( | ||
| page_size="{{ config['page_size'] }}", | ||
| cursor_value="token", | ||
| config={"page_size": "invalid"}, | ||
| parameters={}, | ||
| ) | ||
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.
The new Pydantic-v1 Union coercion workaround only converts numeric strings back to int using
str.isdigit(), which does not handle valid integer string representations like negative numbers (e.g. "-1") or strings with leading/trailing whitespace. If the goal is to robustly undo Union coercion, consider using a saferint()cast with try/except (and/or explicitly validating page_size must be > 0) instead ofisdigit().