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

Source Google Analytics (GA4): Fix Pagination #26126

Merged
merged 20 commits into from
May 31, 2023
Merged

Conversation

mpetrykin
Copy link
Contributor

What

Fix the pagination mechanism. Problem is described in 26066 issue.

How

  1. Add next_page_offset property to report config, that increase in condition current next_page_offset or first offset less than rowCount from response.
  2. Add offset and limit fields to request body parameters that allowed users define them (it is possible from documentation).

@github-actions
Copy link
Contributor

github-actions bot commented May 16, 2023

Before Merging a Connector Pull Request

Wow! What a great pull request you have here! 🎉

To merge this PR, ensure the following has been done/considered for each connector added or updated:

  • PR name follows PR naming conventions
  • Breaking changes are considered. If a Breaking Change is being introduced, ensure an Airbyte engineer has created a Breaking Change Plan and you've followed all steps in the Breaking Changes Checklist
  • Connector version has been incremented in the Dockerfile and metadata.yaml according to our Semantic Versioning for Connectors guidelines
  • Secrets in the connector's spec are annotated with airbyte_secret
  • All documentation files are up to date. (README.md, bootstrap.md, docs.md, etc...)
  • Changelog updated in docs/integrations/<source or destination>/<name>.md with an entry for the new version. See changelog example
  • You, or an Airbyter, have run /test successfully on this PR - or on a non-forked branch
  • You, or an Airbyter, have run /publish successfully on this PR - or on a non-forked branch
  • You've updated the connector's metadata.yaml file new!

If the checklist is complete, but the CI check is failing,

  1. Check for hidden checklists in your PR description

  2. Toggle the github label checklist-action-run on/off to re-run the checklist CI.

Copy link
Collaborator

@bazarnov bazarnov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another change is needed, we should not have the multi-types in JSON Schema, as this will cause issues during data normalization, please refer to the comments bellow.

@bazarnov
Copy link
Collaborator

bazarnov commented May 18, 2023

/test connector=connectors/source-google-analytics-data-api

🕑 connectors/source-google-analytics-data-api https://github.com/airbytehq/airbyte/actions/runs/5013631854
❌ connectors/source-google-analytics-data-api https://github.com/airbytehq/airbyte/actions/runs/5013631854
🐛 https://gradle.com/s/zatfaci7dtc3w

Build Failed

Test summary info:

=========================== short test summary info ============================
FAILED test_core.py::TestConnection::test_check[inputs0] - docker.errors.Cont...
FAILED test_core.py::TestBasicRead::test_read[inputs0] - docker.errors.Contai...
FAILED test_full_refresh.py::TestFullRefresh::test_sequential_reads[inputs0]
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:578: The previous and actual discovered catalogs are identical.
======== 3 failed, 35 passed, 2 skipped, 1 warning in 828.97s (0:13:48) ========

@bazarnov
Copy link
Collaborator

@marcosmarxm I think we're good to go with this contribution, once CAT is passed.

Copy link
Member

@marcosmarxm marcosmarxm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some comments @mpetrykin

@marcosmarxm
Copy link
Member

marcosmarxm commented May 23, 2023

/test connector=connectors/source-google-analytics-data-api

🕑 connectors/source-google-analytics-data-api https://github.com/airbytehq/airbyte/actions/runs/5059002651
❌ connectors/source-google-analytics-data-api https://github.com/airbytehq/airbyte/actions/runs/5059002651
🐛 https://gradle.com/s/yw6txj3sn5kfk

Build Failed

Test summary info:

=========================== short test summary info ============================
FAILED test_core.py::TestConnection::test_check[inputs0] - docker.errors.Cont...
FAILED test_core.py::TestBasicRead::test_read[inputs0] - docker.errors.Contai...
FAILED test_full_refresh.py::TestFullRefresh::test_sequential_reads[inputs0]
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:578: The previous and actual discovered catalogs are identical.
======== 3 failed, 35 passed, 2 skipped, 1 warning in 968.73s (0:16:08) ========

@mpetrykin
Copy link
Contributor Author

/test connector=connectors/source-google-analytics-data-api

@bazarnov bazarnov removed their assignment May 24, 2023
Copy link
Member

@marcosmarxm marcosmarxm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nit comments. I'll run tests locally and check what is not working

@jrolom jrolom removed the bounty label May 29, 2023
@mpetrykin
Copy link
Contributor Author

@marcosmarxm hi, sorry to ask, did you manage to run the tests locally and find the problem or haven't had time yet?

@marcosmarxm
Copy link
Member

Not yet @mpetrykin I'm going to test and run today.

@sh4sh
Copy link
Contributor

sh4sh commented May 31, 2023

/test connector=connectors/source-google-analytics-data-api

🕑 connectors/source-google-analytics-data-api https://github.com/airbytehq/airbyte/actions/runs/5135617899
✅ connectors/source-google-analytics-data-api https://github.com/airbytehq/airbyte/actions/runs/5135617899
Python tests coverage:

Name                                                Stmts   Miss  Cover
-----------------------------------------------------------------------
source_google_analytics_data_api/__init__.py            2      0   100%
source_google_analytics_data_api/authenticator.py      44      2    95%
source_google_analytics_data_api/utils.py              26      2    92%
source_google_analytics_data_api/source.py            247     31    87%
source_google_analytics_data_api/api_quota.py          94     12    87%
-----------------------------------------------------------------------
TOTAL                                                 413     47    89%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:594: The previous and actual discovered catalogs are identical.
============ 39 passed, 2 skipped, 1 warning in 1102.76s (0:18:22) =============

@sh4sh sh4sh mentioned this pull request May 31, 2023
@sh4sh sh4sh dismissed stale reviews from marcosmarxm and grubberr May 31, 2023 17:56

changes resolved

@sh4sh sh4sh self-requested a review May 31, 2023 17:57
@sh4sh sh4sh merged commit 746ccb4 into airbytehq:master May 31, 2023
52 of 68 checks passed
marcosmarxm added a commit to natalia-miinto/airbyte that referenced this pull request Jun 8, 2023
* Fix paggination and add offset and limit to acceptable parameters in request body

* Change next_page_token and add tests

* Update dockerImageTag

* Update PR version

* Remove minimum, maximum, pattern fields

* Remove pattern limit and offset from test_source.py

* Remove offset and limit string type

* Remove offset and limit string type

* Increase limit number to 100000 and remove limit and offset from parameters

* Change return type value of next_page_token from int to dict

* Change return type value of next_page_token from int to dict

* Change page_size to offset and add constant PAGE_SIZE equals 100000

* Add comment to PAGE_SIZE constant and add constant to unit tests

* Remove offset and limit from PivotReport

* Import PAGE_SIZE in unit_tests from source.py

---------

Co-authored-by: Marcos Marx <marcosmarxm@users.noreply.github.com>
Co-authored-by: sh4sh <6833405+sh4sh@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants