🐛 Source Tempo: Avoid infinite loop for non-paginated APIs #16361

bstrawson · 2022-09-06T15:44:05Z

What

Only some of the Tempo APIs support pagination. The ones that don't (accounts and customers) return all their results and ignore any parameters to request the results to be paginated. Users that have DEFAULT_ITEMS_PER_PAGE or more results will enter an infinite loop and the sync operation never completes.

How

The existing code repeatedly calls the API with different pagination parameters (limit and offset) until less than DEFAULT_ITEMS_PER_PAGE (normally 100) items are returned. At this point it terminates. This change adds a parameter paginated to each API in ENTITIES_MAP to indicate whether it is paginated or not. Where the API is paginated, behaviour is as before. However, if not paginated then only one call is made, avoiding entering an infinite loop.

🚨 User Impact 🚨

No user impact.

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub by running the /publish command described here
After the connector is published, connector added to connector index as described here
Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

Updating a connector

Community member or Airbyter

Grant edit access to maintainers (instructions)
Secrets in the connector's spec are annotated with airbyte_secret
Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
Code reviews completed
Documentation updated
- Connector's README.md
- Connector's bootstrap.md. See description and examples
- Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub and connector version bumped by running the /publish command described here

Connector Generator

Issue acceptance criteria met
PR name follows PR naming conventions
If adding a new generator, add it to the list of scaffold modules being tested
The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
Documentation which references the generator is updated as needed

Tests

Unit

Put your unit tests output here.

Integration

Put your integration tests output here.

Acceptance

Put your acceptance tests output here.

CLAassistant · 2022-09-06T15:44:15Z

All committers have signed the CLA.

…nation and support more than DEFAULT_ITEMS_PER_PAGE results.

vincentkoc

Change process to look for next in the API response payload

vincentkoc · 2022-09-08T12:11:21Z

airbyte-integrations/connectors/source-tempo/source_tempo/client.py

@@ -38,7 +38,7 @@ def lists(self, name, url, params, func, **kwargs):
            response = requests.get(f"{self.base_api_url}{url}?limit={params['limit']}&offset={params['offset']}", headers=self.headers)
            data = func(response.json())
            yield from data
-            if len(data) < self.DEFAULT_ITEMS_PER_PAGE:
+            if not self.ENTITIES_MAP[name]["paginated"] or len(data) < self.DEFAULT_ITEMS_PER_PAGE:


I'm not sure this is the safe way to handle this. As another API repo in python is listening on the next field existing in the API response in order to continue. See https://github.com/stanislavulrych/tempo-api-python-client/blob/62418b72b10c625c7fcc9b9d1ea61f09a3c6a09d/tempoapiclient/client.py#L54

I took an example API call for worklogs and can confirm the response object next is in there https://tempo-io.github.io/tempo-api-docs/v2.html#worklogs - This is the safest way and easier based on the data return vs. being explicit each time. Also there are chances when data may be more than the default and not have a page and this could cause errors.

The worklogs API call supports pagination and therefore has parameters such as offset, limit, next and previous.

However the accounts and customers API calls do not support pagination, and do not support these parameters (eg. see https://tempo-io.github.io/tempo-api-docs/v2.html#accounts). The API ignores the limit parameter (set to 100 by default) and returns all the results at once. If you have (for example) 120 accounts, then the existing code fails to exit.

This change puts a flag against the APIs calls that are paginated and those that are not. For non-paginated API calls it doesn't try to fetch further pages and just exits, avoiding an infinite loop.

It doesn't change the behaviour for paginated API calls (such as worklogs). I don't believe that checking the response size is < DEFAULT_ITEMS_PER_PAGE is a great way to check whether to exit or request further pages. But I'm unaware of an issue with the existing implementation (for the paginated APIs) and it isn't what I am trying to fix, so I've left it as-is.

@bstrawson thanks for explination. All is good 👍

@bstrawson code changes are all good, we are just miasing one change - you need to edit the md files too when making changes. Let me know if you need me to point you the connector md with an example

@koconder Apologies for missing that. If you could point me in the direction of the md with an example, I'd be grateful.

@bstrawson you need to edit this file:

https://github.com/airbytehq/airbyte/blob/master/docs/integrations/sources/tempo.md (see the bottom)

Ok thanks. Done.

@bstrawson will get the team to check the integration test results which failed and report back. Should be able to get this review finalized soon

sajarin · 2022-09-08T20:08:57Z

/test connector=connectors/source-tempo

🕑 connectors/source-tempo https://github.com/airbytehq/airbyte/actions/runs/3017970585
❌ connectors/source-tempo https://github.com/airbytehq/airbyte/actions/runs/3017970585
🐛 https://gradle.com/s/mwc4whvibtx7o

Build Failed

Test summary info:

=========================== short test summary info ============================
FAILED test_core.py::TestSpec::test_additional_properties_is_true[inputs0] - ...
FAILED test_core.py::TestDiscovery::test_additional_properties_is_true[inputs0]
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:60: Skipping TestIncremental.test_two_sequential_reads because not found in the config
=================== 2 failed, 24 passed, 1 skipped in 29.03s ===================

sajarin · 2022-09-13T23:30:08Z

/test connector=connectors/source-tempo

🕑 connectors/source-tempo https://github.com/airbytehq/airbyte/actions/runs/3049070815
✅ connectors/source-tempo https://github.com/airbytehq/airbyte/actions/runs/3049070815
Python tests coverage:

	 Name                                                 Stmts   Miss  Cover   Missing
	 ----------------------------------------------------------------------------------
	 source_acceptance_test/base.py                          10      4    60%   15-18
	 source_acceptance_test/config.py                        83      6    93%   78-80, 84-86
	 source_acceptance_test/conftest.py                     164    164     0%   6-282
	 source_acceptance_test/plugin.py                        48     48     0%   6-104
	 source_acceptance_test/tests/test_core.py              329    111    66%   39, 50-58, 63-70, 74-75, 79-80, 164, 202-219, 228-236, 240-245, 251, 284-289, 327-334, 374-376, 379, 439-448, 477-478, 484, 487, 520-530, 543-568, 573-577
	 source_acceptance_test/tests/test_full_refresh.py       52      2    96%   34, 65
	 source_acceptance_test/tests/test_incremental.py       121     25    79%   21-23, 29-31, 36-43, 48-61, 208-216
	 source_acceptance_test/utils/asserts.py                 37      2    95%   57-58
	 source_acceptance_test/utils/common.py                  77     17    78%   15-16, 24-30, 47-54, 64, 67
	 source_acceptance_test/utils/compare.py                 62     23    63%   21-51, 68, 97-99
	 source_acceptance_test/utils/connector_runner.py       110     48    56%   23-26, 32, 36, 39-64, 67-69, 72-74, 77-79, 82-84, 87-89, 92-110, 144-146
	 source_acceptance_test/utils/json_schema_helper.py     105     13    88%   30-31, 38, 41, 65-68, 96, 120, 190-192
	 ----------------------------------------------------------------------------------
	 TOTAL                                                 1325    463    65%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:60: Skipping TestIncremental.test_two_sequential_reads because not found in the config
======================== 26 passed, 1 skipped in 26.78s ========================

vincentkoc

All changes completed, tests passing

sajarin · 2022-09-13T23:50:13Z

/publish connector=connectors/source-tempo

🕑 Publishing the following connectors:
connectors/source-tempo
https://github.com/airbytehq/airbyte/actions/runs/3049157963

Connector	Did it publish?	Were definitions generated?
connectors/source-tempo	✅	✅

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

…#16361) * 🐛 Source Tempo: Avoid infinite loop for APIs that do not support pagination and support more than DEFAULT_ITEMS_PER_PAGE results. * Source Tempo: Update changelog for version 0.2.6 * fix: update additionalProperties to true * fix: remove additionalProperties field in schema files Co-authored-by: Sajarin <sajarindider@gmail.com>

github-actions bot added the area/connectors Connector related issues label Sep 6, 2022

octavia-squidington-iii added bounty community connectors/source/tempo labels Sep 6, 2022

🐛 Source Tempo: Avoid infinite loop for APIs that do not support pagi…

a00ac71

…nation and support more than DEFAULT_ITEMS_PER_PAGE results.

bstrawson force-pushed the source-tempo/fix-infinite-loop branch from e6d7668 to a00ac71 Compare September 6, 2022 15:45

sajarin added the bounty-S Maintainer program: claimable small bounty PR label Sep 6, 2022

vincentkoc suggested changes Sep 8, 2022

View reviewed changes

sajarin added 2 commits September 8, 2022 10:18

Merge branch 'master' into source-tempo/fix-infinite-loop

b4cc921

Merge branch 'master' into source-tempo/fix-infinite-loop

2ac6667

bstrawson added 2 commits September 9, 2022 12:29

Merge branch 'airbytehq:master' into source-tempo/fix-infinite-loop

3c6b441

Source Tempo: Update changelog for version 0.2.6

9dd81a5

github-actions bot added the area/documentation Improvements or additions to documentation label Sep 9, 2022

sajarin added 3 commits September 13, 2022 12:26

fix: update additionalProperties to true

4fff469

fix: remove additionalProperties field in schema files

b201955

Merge branch 'master' into source-tempo/fix-infinite-loop

6fc7ca2

vincentkoc approved these changes Sep 13, 2022

View reviewed changes

sajarin approved these changes Sep 13, 2022

View reviewed changes

auto-bump connector version [ci skip]

f403257

sajarin merged commit 8b67f03 into airbytehq:master Sep 14, 2022

This was referenced Sep 14, 2022

Bump Airbyte version from 0.40.6 to 0.40.7 #16754

Closed

Bump Airbyte version from 0.40.6 to 0.40.7 #16785

Merged

sajarin assigned sajarin and unassigned sajarin Sep 23, 2022

vincentkoc self-assigned this Oct 1, 2022

bleonard added the reward-50 label Oct 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Source Tempo: Avoid infinite loop for non-paginated APIs #16361

🐛 Source Tempo: Avoid infinite loop for non-paginated APIs #16361

bstrawson commented Sep 6, 2022

CLAassistant commented Sep 6, 2022 •

edited

Loading

vincentkoc left a comment

vincentkoc Sep 8, 2022

bstrawson Sep 8, 2022

vincentkoc Sep 8, 2022

vincentkoc Sep 8, 2022 •

edited

Loading

bstrawson Sep 8, 2022

vincentkoc Sep 9, 2022

bstrawson Sep 9, 2022

vincentkoc Sep 13, 2022

sajarin commented Sep 8, 2022 •

edited by github-actions bot

Loading

sajarin commented Sep 13, 2022 •

edited by github-actions bot

Loading

vincentkoc left a comment

sajarin commented Sep 13, 2022 •

edited by github-actions bot

Loading

🐛 Source Tempo: Avoid infinite loop for non-paginated APIs #16361

🐛 Source Tempo: Avoid infinite loop for non-paginated APIs #16361

Conversation

bstrawson commented Sep 6, 2022

What

How

Recommended reading order

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

Tests

CLAassistant commented Sep 6, 2022 • edited Loading

vincentkoc left a comment

Choose a reason for hiding this comment

vincentkoc Sep 8, 2022

Choose a reason for hiding this comment

bstrawson Sep 8, 2022

Choose a reason for hiding this comment

vincentkoc Sep 8, 2022

Choose a reason for hiding this comment

vincentkoc Sep 8, 2022 • edited Loading

Choose a reason for hiding this comment

bstrawson Sep 8, 2022

Choose a reason for hiding this comment

vincentkoc Sep 9, 2022

Choose a reason for hiding this comment

bstrawson Sep 9, 2022

Choose a reason for hiding this comment

vincentkoc Sep 13, 2022

Choose a reason for hiding this comment

sajarin commented Sep 8, 2022 • edited by github-actions bot Loading

Build Failed

sajarin commented Sep 13, 2022 • edited by github-actions bot Loading

Build Passed

vincentkoc left a comment

Choose a reason for hiding this comment

sajarin commented Sep 13, 2022 • edited by github-actions bot Loading

CLAassistant commented Sep 6, 2022 •

edited

Loading

vincentkoc Sep 8, 2022 •

edited

Loading

sajarin commented Sep 8, 2022 •

edited by github-actions bot

Loading

sajarin commented Sep 13, 2022 •

edited by github-actions bot

Loading

sajarin commented Sep 13, 2022 •

edited by github-actions bot

Loading