Skip to content

Conversation

@abrassel
Copy link
Contributor

@abrassel abrassel commented Jun 19, 2024

PR Checklist

  • A description of the changes is added to the description of this PR.
  • If there is a related issue, make sure it is linked to this PR.
  • If you've fixed a bug or added code that should be tested, add tests!

Description of changes

Addresses #21

When performing a tables.list operation, it seems that the server returns the empty string "" instead of None as documented in the spec.

Fixed this by making the paginated request return null instead.

Testing

On a default run of unity catalog, did this manual test:

import unitycatalog

client = unitycatalog.Unitycatalog(base_url="http://127.0.0.1:8080/api/2.1/unity-catalog/")
response = client.tables.list(catalog_name="unity", schema_name="default")

assert response.next_page_token is None, f"This will fail, with a non-empty token: {response.next_page_token}"

response = client.tables.list(catalog_name="unity", schema_name="default", page_token=response.next_page_token)
assert len(response.tables) > 0, "This will fail, because response has no tables"
assert response.next_page_token is None

Any tips for testing this effectively?

@abrassel
Copy link
Contributor Author

@tdas can you review this PR?

@abrassel abrassel force-pushed the fix_next_page_token branch from 433cabf to 02c88af Compare June 19, 2024 06:17
Copy link
Contributor

@ramesh-db ramesh-db left a comment

Choose a reason for hiding this comment

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

@abrassel thank you for fixing this.

Can you add a test for the fix? Will merge after the test.

@abrassel
Copy link
Contributor Author

@ramesh-db Thanks for the review! Do you have a code pointer for this repo's testing framework?

@ravivj-db
Copy link
Collaborator

ravivj-db commented Jun 22, 2024

Hi @abrassel , you can checkout the file BaseTableCRUDTest and SdkTableCRUDTest. Ideally we would have wanted to add this check directly to the BaseTableCRUDTest, however we have currently kept the return type of listTables as List<TableInfo> (which omits the next page token) to keep compatibility between Sdk and Cli Tests (in CLI we are not handling next page tokens). A workaround would be to create a small ListTablesTest at the level of SdkTableCRUDTest (which directly extends BaseCRUDTest or BaseServerTest) and just creates a couple of tables and Tests the output of the listTables. Then it can read the response as ListTableResponse and check the nextPageToken field. We can extend this test to test out various pagination scenarios.

@abrassel
Copy link
Contributor Author

Hi @ramesh-db I added a test. We should be good to go now.

@tdas tdas merged commit 9a17587 into unitycatalog:main Jun 28, 2024
dennyglee pushed a commit that referenced this pull request Sep 2, 2024
* start of login page

* env and google auth button

* merge with main, remove params reference

* start of okta auth

* initial commit for handling auth token (#67)

* start of login with keycloak

* handle google sign in with token

* more google auth

* profile dropdown

* merge with main

* merge with main

* convert to axios

* start of readme instructions

* get current user endpoint (#70)

clean up some other endpoints

* commenting out UI until repositories are merged

* clean up current user (#74)

* yarn lock file

* remove keycloak for now, node version error in jwt-decode dependency

* commit yarn lock

* remove state as useEffect dependency, comment out currentUser call for now

---------

Co-authored-by: Xiang Xu <xiang.xu@databricks.com>
tdas pushed a commit that referenced this pull request Sep 5, 2024
* start of login page

* env and google auth button

* merge with main, remove params reference

* start of okta auth

* initial commit for handling auth token (#67)

* start of login with keycloak

* handle google sign in with token

* more google auth

* profile dropdown

* merge with main

* merge with main

* convert to axios

* start of readme instructions

* get current user endpoint (#70)

clean up some other endpoints

* commenting out UI until repositories are merged

* clean up current user (#74)

* yarn lock file

* remove keycloak for now, node version error in jwt-decode dependency

* commit yarn lock

* remove state as useEffect dependency, comment out currentUser call for now

---------

Co-authored-by: Xiang Xu <xiang.xu@databricks.com>
rtyler pushed a commit to rtyler/unitycatalog that referenced this pull request Sep 5, 2024
* start of login page

* env and google auth button

* merge with main, remove params reference

* start of okta auth

* initial commit for handling auth token (unitycatalog#67)

* start of login with keycloak

* handle google sign in with token

* more google auth

* profile dropdown

* merge with main

* merge with main

* convert to axios

* start of readme instructions

* get current user endpoint (unitycatalog#70)

clean up some other endpoints

* commenting out UI until repositories are merged

* clean up current user (unitycatalog#74)

* yarn lock file

* remove keycloak for now, node version error in jwt-decode dependency

* commit yarn lock

* remove state as useEffect dependency, comment out currentUser call for now

---------

Co-authored-by: Xiang Xu <xiang.xu@databricks.com>
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.

5 participants