Skip to content

Exit loop if start_after is None and add per_page to query string#7

Open
ahuynh3 wants to merge 2 commits intosinger-io:masterfrom
ahuynh3:fix-sync-cursor
Open

Exit loop if start_after is None and add per_page to query string#7
ahuynh3 wants to merge 2 commits intosinger-io:masterfrom
ahuynh3:fix-sync-cursor

Conversation

@ahuynh3
Copy link
Copy Markdown

@ahuynh3 ahuynh3 commented Sep 25, 2020

Description of change

This issue applies to streams that use cursor based pagination (i.e. the contacts stream). Currently, the starting_after variable is set to None if there's no more data to paginate through. This then gets passed to next_url, and a request is made to a URL that looks something like https://api.intercom.io/contacts?starting_after=None. This leads to the following error

tap_intercom.client.IntercomBadRequestError: client_error: Invalid starting_after param. Please try again using a starting_after value from a paginated response

The fix for this is to add a check to break out of the loop when starting_after is set to None.

In addition, add the per_page query param so that the same limit is used throughout the entire paginated result set. This limit is set in params and gets built up into querystring for the first loop through, but subsequent loops set the querystring variable to None.

Manual QA steps

Sample log output with the per_page query param addition

INFO URL for Stream contacts: https://api.intercom.io/contacts?per_page=150
INFO METRIC: {"type": "timer", "metric": "http_request_duration", "value": 1.6957898139953613, "tags": {"endpoint": "contacts", "http_status_code": 200, "status": "succeeded"}}
INFO METRIC: {"type": "counter", "metric": "record_count", "value": 150, "tags": {"endpoint": "contacts"}}
INFO Stream contacts, batch processed 150 records
INFO Write state for stream: contacts, value: 2020-09-25T23:01:36.000000Z
INFO Synced Stream: contacts, page: 1, records: 0 to 150
INFO URL for Stream contacts: https://api.intercom.io/contacts?starting_after=WzE2MDEwNTA2MzgwMDAsIjVmNmUxMTZlMTQxNzRkMDdkZjFiMmVlZSIsMl0=&per_page=150
INFO METRIC: {"type": "timer", "metric": "http_request_duration", "value": 1.154371976852417, "tags": {"endpoint": "contacts", "http_status_code": 200, "status": "succeeded"}}
INFO METRIC: {"type": "counter", "metric": "record_count", "value": 150, "tags": {"endpoint": "contacts"}}
INFO Stream contacts, batch processed 150 records
INFO Write state for stream: contacts, value: 2020-09-25T23:01:36.000000Z
INFO Synced Stream: contacts, page: 2, records: 150 to 300
INFO URL for Stream contacts: https://api.intercom.io/contacts?starting_after=WzE2MDA5Nzc5MjcwMDAsIjVmNTNkMTE3NTA1NTA0MmY1MjdlYTgyOSIsM10=&per_page=150
INFO METRIC: {"type": "timer", "metric": "http_request_duration", "value": 1.5758838653564453, "tags": {"endpoint": "contacts", "http_status_code": 200, "status": "succeeded"}}
INFO METRIC: {"type": "counter", "metric": "record_count", "value": 150, "tags": {"endpoint": "contacts"}}
INFO Stream contacts, batch processed 150 records
INFO Write state for stream: contacts, value: 2020-09-25T23:01:36.000000Z
INFO Synced Stream: contacts, page: 3, records: 300 to 450
INFO URL for Stream contacts: https://api.intercom.io/contacts?starting_after=WzE2MDA5MDQ3NDYwMDAsIjVmNWNlMzUyYzQzOTQ2Yzg4NTE5NjNmZCIsNF0=&per_page=150
INFO METRIC: {"type": "timer", "metric": "http_request_duration", "value": 1.3687028884887695, "tags": {"endpoint": "contacts", "http_status_code": 200, "status": "succeeded"}}
INFO METRIC: {"type": "counter", "metric": "record_count", "value": 150, "tags": {"endpoint": "contacts"}}
INFO Stream contacts, batch processed 150 records
...
INFO Synced Stream: contacts, pages: 244, total records: 36746
INFO FINISHED Syncing: contacts, total_records: 36746
{"bookmarks": {"contacts": "2020-09-25T22:42:53.000000Z"}}

Risks

Rollback steps

  • revert this branch

@cmerrick
Copy link
Copy Markdown
Contributor

Hi @ahuynh3, thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes.

@cmerrick
Copy link
Copy Markdown
Contributor

You did it @ahuynh3!

Thank you for signing the Singer Contribution License Agreement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants