Skip to content
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

Empty String Cursor Value from Zoom API Leading to Pagination Issue in JSONResponseCursorPaginator #2012

Closed
kang8 opened this issue Nov 1, 2024 · 3 comments · Fixed by #2016
Assignees
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@kang8
Copy link
Contributor

kang8 commented Nov 1, 2024

dlt version

dlt 1.3.0

Describe the problem

I encountered an issue when using the JSONResponseCursorPaginator class in rest_client for pagination requests while interacting with Zoom's API. The update_state() method is implemented as follows:

def update_state(self, response: Response, data: Optional[List[Any]] = None) -> None:
"""Extracts the cursor value from the JSON response."""
values = jsonpath.find_values(self.cursor_path, response.json())
self._next_reference = values[0] if values else None

The problem occurs when values is equal to [""], resulting in self._next_reference being set to "" instead of None. This leads to pagination requests continuing to the next page, whereas I expect it to be None to stop further requests.

Expected behavior

The expected behavior is for self._next_reference to be set to None when values is [""], thus preventing further pagination requests. This matches the anticipated termination condition logic.

Steps to reproduce

  1. Use the JSONResponseCursorPaginator to implement pagination requests.
  2. When the value extracted from the JSON response using the specified path is [""], observe the pagination behavior.
  3. Note whether self._next_reference is set to "" instead of None.

Operating system

Linux

Runtime environment

Kubernetes

Python version

3.11

dlt data source

ZOOM api, e.g. GET /users

dlt destination

Snowflake

Other deployment details

No response

Additional information

I plan to submit a PR to address this issue.

@burnash burnash self-assigned this Nov 1, 2024
@burnash burnash added the bug Something isn't working label Nov 1, 2024
@burnash
Copy link
Collaborator

burnash commented Nov 1, 2024

Thanks for reporting this @kang8. Let me know if you'd be interested in submitting a PR with a fix.

@burnash burnash added the help wanted Extra attention is needed label Nov 1, 2024
@kang8
Copy link
Contributor Author

kang8 commented Nov 1, 2024

Yes, I'll submit a PR around tomorrow night(my timezone is UTC +08:00)

@burnash
Copy link
Collaborator

burnash commented Nov 1, 2024

Perfect, thank you!

kang8 added a commit to kang8/dlt that referenced this issue Nov 2, 2024
…ng cursor value

Adjusts the `update_state()` method to set `_next_reference` to None
when the cursor value extracted from the JSON response is an empty
string, preventing unintended pagination requests.

Fixed dlt-hub#2012
kang8 added a commit to kang8/dlt that referenced this issue Nov 2, 2024
…ng cursor value

Adjusts the `update_state()` method to set `_next_reference` to None
when the cursor value extracted from the JSON response is an empty
string, preventing unintended pagination requests.

Fixed dlt-hub#2012
kang8 added a commit to kang8/dlt that referenced this issue Nov 2, 2024
…ng cursor value

Adjusts the `update_state()` method to set `_next_reference` to None
when the cursor value extracted from the JSON response is an empty
string, preventing unintended pagination requests.

Fixed dlt-hub#2012
@burnash burnash closed this as completed in c11e447 Nov 4, 2024
@github-project-automation github-project-automation bot moved this from Todo to Done in dlt core library Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants