🎉 Source Notion: Adds pagination to Users stream #11452

lgomezm · 2022-03-27T02:28:49Z

What

Notion Users stream does not handle pagination. That is, it only outputs the first page of user records, which defaults to 100 (https://developers.notion.com/reference/pagination).

How

Adds correct handling of pagination for that kind of stream.
Overwrites the request_params stream method.

🚨 User Impact 🚨

Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.

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 by running the /publish command described here
After the new connector version is published, connector version bumped in the seed directory 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

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.

itaseskii · 2022-03-27T20:13:06Z

airbyte-integrations/connectors/source-notion/unit_tests/test_streams.py

+
+    # When getting pages after the first pull.
+    inputs = {"stream_slice": None, "stream_state": None, "next_page_token": {"next_cursor": "123"}}
+    expected_params = {"start_cursor": "123", "page_size": 100}


Instead of using a dummy start_cursor value maybe use a self documenting value that will be provided from Notion such as "next_cursor": "fe2cc560-036c-44cd-90e8-294d5a74cebc"

You're right. Updated in fda250e.

itaseskii · 2022-03-27T20:17:06Z

airbyte-integrations/connectors/source-notion/unit_tests/test_streams.py

+    """
+
+    response_body = {
+        "object": "list",


In the original response from Notion both object and type are part of the results array. In my opinion It would be better if the tests are kept consistent with the actual implementation

Done in fda250e.

itaseskii · 2022-03-27T20:20:43Z

airbyte-integrations/connectors/source-notion/unit_tests/test_streams.py

+        "has_more": True,
+        "type": "user"
+    }
+    url = "https://api.notion.com/v1/users?page_size=100"


Is there a need to declare the url variable? couldn't the url be passed directly to requests_mock.get()?

Moved to requests_mock.get() in fda250e.

itaseskii · 2022-03-27T20:32:55Z

airbyte-integrations/connectors/source-notion/unit_tests/test_streams.py

+    stream = Users(config=MagicMock())
+
+    user_ids = set()
+    for record in stream.read_records(sync_mode=SyncMode.full_refresh):


Is there a need to fill the user_ids set before asserting that the set is of length 220? wouldn't the following snippet do the job? Might also save you some memory from allocating an additional data structure.

records = stream.read_records(sync_mode=SyncMode.full_refresh): records_length = sum(1 for record in records) assert records_length == 220

Updated in fda250e.

lgomezm · 2022-03-28T14:30:38Z

Hi @itaseskii. I've addressed your comments. Please take a look again when you get a chance and let me know if there's anything else I should do. Thanks for reviewing!

lgomezm · 2022-03-28T14:32:55Z

This is the unit test execution:

itaseskii · 2022-03-29T09:22:23Z

Hey @lgomezm I have went through you changes and I have no further comments, great job! :)

lgomezm · 2022-04-18T15:04:55Z

@marcosmarxm @alafanechere Hi guys. This PR has been open for a while now. Could you take a look? Thanks in advance!

alafanechere

Hi @lgomezm,
Thank you for your contribution. I made minor suggestions around the tests. I'll bump the version myself as I can commit to your branch.

alafanechere · 2022-04-22T09:28:47Z

airbyte-integrations/connectors/source-notion/unit_tests/test_streams.py

+def test_users_request_params(patch_base_class):
+    stream = Users(config=MagicMock())
+
+    # No next_page_token. First pull
+    inputs = {"stream_slice": None, "stream_state": None, "next_page_token": None}
+    expected_params = {"page_size": 100}
+    assert stream.request_params(**inputs) == expected_params
+
+    # When getting pages after the first pull.
+    inputs = {"stream_slice": None, "stream_state": None, "next_page_token": {"next_cursor": "123"}}
+    expected_params = {"start_cursor": "123", "page_size": 100}
+    assert stream.request_params(**inputs) == expected_params


nit: I suggest leverage parametrize wherever you can.

Suggested change

def test_users_request_params(patch_base_class):

stream = Users(config=MagicMock())

# No next_page_token. First pull

inputs = {"stream_slice": None, "stream_state": None, "next_page_token": None}

expected_params = {"page_size": 100}

assert stream.request_params(**inputs) == expected_params

# When getting pages after the first pull.

inputs = {"stream_slice": None, "stream_state": None, "next_page_token": {"next_cursor": "123"}}

expected_params = {"start_cursor": "123", "page_size": 100}

assert stream.request_params(**inputs) == expected_params

@pytest.mark.parametrize("next_page_token, expected_params", [(None, {"page_size": 100}), ({"next_cursor": "123"}, {"start_cursor": "123", "page_size": 100})])

def test_users_request_params(patch_base_class, next_page_token, expected_params):

stream = Users(config=MagicMock())

inputs = {"stream_slice": None, "stream_state": None, "next_page_token": next_page_token}

assert stream.request_params(**inputs) == expected_params

alafanechere · 2022-04-22T09:31:48Z

airbyte-integrations/connectors/source-notion/unit_tests/test_streams.py

+    """
+    Test shows that Users stream uses pagination as per Notion API docs.
+    """
+


nit: I would suggest to use a function to generate your requests/responses sequence

alafanechere · 2022-04-22T10:28:59Z

/publish connector=connectors/source-notion auto-bump-version=false

🕑 connectors/source-notion https://github.com/airbytehq/airbyte/actions/runs/2207187005
❌ Failed to publish connectors/source-notion

alafanechere · 2022-04-22T13:56:17Z

/publish connector=connectors/source-notion auto-bump-version=false

🕑 connectors/source-notion https://github.com/airbytehq/airbyte/actions/runs/2208150419
🚀 Successfully published connectors/source-notion
✅ connectors/source-notion https://github.com/airbytehq/airbyte/actions/runs/2208150419

alafanechere

Thank you @lgomezm for this improvements. I had to update some schemas too as it looks like notion API evolved a bit.

lgomezm added 2 commits March 26, 2022 21:21

Handled pagination on Users stream

2c6f0fd

Added unit tests

11cfa83

github-actions bot added the area/connectors Connector related issues label Mar 27, 2022

itaseskii reviewed Mar 27, 2022

View reviewed changes

Refactored unit test

fda250e

aharelick linked an issue Apr 18, 2022 that may be closed by this pull request

Pagination #12115

Closed

aharelick added the community label Apr 21, 2022

alafanechere self-assigned this Apr 22, 2022

alafanechere approved these changes Apr 22, 2022

View reviewed changes

alafanechere added 2 commits April 22, 2022 11:37

Merge branch 'master' into lgomez/notion_user_pagination

6d85961

bump version

1fbb529

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

update schemas

ac5965c

update source specs

e241d21

alafanechere approved these changes Apr 22, 2022

View reviewed changes

alafanechere merged commit c16af25 into airbytehq:master Apr 22, 2022

lgomezm deleted the lgomez/notion_user_pagination branch April 22, 2022 14:22

octavia-squidington-iii mentioned this pull request Apr 25, 2022

Bump Airbyte version from 0.36.2-alpha to 0.36.3-alpha #12306

Merged

suhomud pushed a commit that referenced this pull request May 23, 2022

🎉 Source Notion: Adds pagination to Users stream (#11452)

b438dfd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🎉 Source Notion: Adds pagination to Users stream #11452

🎉 Source Notion: Adds pagination to Users stream #11452

lgomezm commented Mar 27, 2022

itaseskii Mar 27, 2022

lgomezm Mar 28, 2022

itaseskii Mar 27, 2022

lgomezm Mar 28, 2022

itaseskii Mar 27, 2022

lgomezm Mar 28, 2022

itaseskii Mar 27, 2022

lgomezm Mar 28, 2022

lgomezm commented Mar 28, 2022

lgomezm commented Mar 28, 2022

itaseskii commented Mar 29, 2022

lgomezm commented Apr 18, 2022

alafanechere left a comment

alafanechere Apr 22, 2022

alafanechere Apr 22, 2022

alafanechere commented Apr 22, 2022 •

edited by github-actions bot

Loading

alafanechere commented Apr 22, 2022 •

edited by github-actions bot

Loading

alafanechere left a comment

🎉 Source Notion: Adds pagination to Users stream #11452

🎉 Source Notion: Adds pagination to Users stream #11452

Conversation

lgomezm commented Mar 27, 2022

What

How

Recommended reading order

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

Tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgomezm commented Mar 28, 2022

lgomezm commented Mar 28, 2022

itaseskii commented Mar 29, 2022

lgomezm commented Apr 18, 2022

alafanechere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alafanechere commented Apr 22, 2022 • edited by github-actions bot Loading

alafanechere commented Apr 22, 2022 • edited by github-actions bot Loading

alafanechere left a comment

Choose a reason for hiding this comment

alafanechere commented Apr 22, 2022 •

edited by github-actions bot

Loading

alafanechere commented Apr 22, 2022 •

edited by github-actions bot

Loading