Skip to content

Add pagination to open api spec for listing of namespaces, tables, views #9660

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.

Sign up for GitHub

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

Merged
merged 14 commits into from
Feb 29, 2024
Merged

Add pagination to open api spec for listing of namespaces, tables, views #9660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
231b4dd
Add pagination to open api spec for listing of namespaces, tables, views
Feb 2, 2024
a3dc720
Rev for pagination in open api spec
Feb 7, 2024
a1e20ad
Fix writing in pagination descriptions
Feb 12, 2024
4128211
Use opaque token
Feb 12, 2024
25656b2
Rev on pagination pr writing
Feb 13, 2024
e0307a1
Rev on pagination pr writing py file
Feb 13, 2024
c78f251
Rev on description for pageSize
Feb 13, 2024
e25c70a
revise pageToken description
Feb 14, 2024
6965325
minor rev and make int for pageSize
Feb 15, 2024
77df1a5
minor rev and remove emtpy val true for pageSize
Feb 16, 2024
aab3caa
use only PageToken instead of NextPageToken
Feb 19, 2024
eed29cc
address jack and nastra comments
Feb 21, 2024
918a1ad
address final comments
Feb 21, 2024
ef4af36
remove they and use min 1 for pageSize
Feb 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions open-api/rest-catalog-open-api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,13 @@ class Namespace(BaseModel):
)


class PageToken(BaseModel):
__root__: str = Field(
...,
description='An opaque token which allows clients to make use of pagination for a list API (e.g. ListTables). Clients will initiate the first paginated request by sending an empty `pageToken` e.g. `GET /tables?pageToken` or `GET /tables?pageToken=` signaling to the service that the response should be paginated.\nServers that support pagination will recognize `pageToken` and return a `next-page-token` in response if there are more results available. After the initial request, it is expected that the value of `next-page-token` from the last response is used in the subsequent request. Servers that do not support pagination will ignore `next-page-token` and return all results.',
)


class TableIdentifier(BaseModel):
namespace: Namespace
name: str
Expand Down Expand Up @@ -581,10 +588,12 @@ class GetNamespaceResponse(BaseModel):


class ListTablesResponse(BaseModel):
next_page_token: Optional[PageToken] = Field(None, alias='next-page-token')
identifiers: Optional[List[TableIdentifier]] = Field(None, unique_items=True)


class ListNamespacesResponse(BaseModel):
next_page_token: Optional[PageToken] = Field(None, alias='next-page-token')
namespaces: Optional[List[Namespace]] = Field(None, unique_items=True)


Expand Down
42 changes: 42 additions & 0 deletions open-api/rest-catalog-open-api.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,8 @@ paths:
If `parent` is not provided, all top-level namespaces should be listed.
operationId: listNamespaces
parameters:
- $ref: '#/components/parameters/page-token'
- $ref: '#/components/parameters/page-size'
- name: parent
in: query
description:
Expand Down Expand Up @@ -448,6 +450,9 @@ paths:
summary: List all table identifiers underneath a given namespace
description: Return all table identifiers under this namespace
operationId: listTables
parameters:
- $ref: '#/components/parameters/page-token'
- $ref: '#/components/parameters/page-size'
responses:
200:
$ref: '#/components/responses/ListTablesResponse'
Expand Down Expand Up @@ -1070,6 +1075,9 @@ paths:
summary: List all view identifiers underneath a given namespace
description: Return all view identifiers under this namespace
operationId: listViews
parameters:
- $ref: '#/components/parameters/page-token'
- $ref: '#/components/parameters/page-size'
responses:
200:
$ref: '#/components/responses/ListTablesResponse'
Expand Down Expand Up @@ -1482,6 +1490,25 @@ components:
explode: false
example: "vended-credentials,remote-signing"

page-token:
name: pageToken
in: query
required: false
allowEmptyValue: true
schema:
$ref: '#/components/schemas/PageToken'

page-size:
name: pageSize
in: query
description:
For servers that support pagination, this signals an upper bound of the number of results that a client will receive.
For servers that do not support pagination, clients may receive results larger than the indicated `pageSize`.
required: false
schema:
type: integer
minimum: 1

##############################
# Application Schema Objects #
##############################
Expand Down Expand Up @@ -1581,6 +1608,17 @@ components:
type: string
example: [ "accounting", "tax" ]

PageToken:
description:
An opaque token which allows clients to make use of pagination for a list API (e.g. ListTables).
Clients will initiate the first paginated request by sending an empty `pageToken` e.g. `GET /tables?pageToken` or `GET /tables?pageToken=`
Copy link
Contributor

Choose a reason for hiding this comment

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

@rahil-c, in a spec you always want to be specific, not give options. Options make the spec more complicated. Here, only the second form should be added. The first form with no = is not recommended.

Copy link
Contributor

@jackye1995 jackye1995 Mar 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Trying to remember what was decided during the community sync discussion, https://www.youtube.com/watch?v=uAQVGd5zV4I, starting at 45:04, seems like you said we should support both there. But maybe it's a misunderstanding of what Rahil meant there when he was talking about the first option of value not present.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rdblue @jackye1995 Yea I think based on the community sync I thought we were suppose to be flexible for what users can do hence I included the two options in the description.

I can remove the first form if needed.

signaling to the service that the response should be paginated.

Servers that support pagination will recognize `pageToken` and return a `next-page-token` in response if there are more results available.
After the initial request, it is expected that the value of `next-page-token` from the last response is used in the subsequent request.
Servers that do not support pagination will ignore `next-page-token` and return all results.
Copy link
Contributor

Choose a reason for hiding this comment

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

Servers that do not support pagination will ignore pageToken right? This should be the query param not the response field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix this.

type: string

TableIdentifier:
type: object
required:
Expand Down Expand Up @@ -3169,6 +3207,8 @@ components:
ListTablesResponse:
type: object
properties:
next-page-token:
$ref: '#/components/schemas/PageToken'
identifiers:
type: array
uniqueItems: true
Expand All @@ -3178,6 +3218,8 @@ components:
ListNamespacesResponse:
type: object
properties:
next-page-token:
$ref: '#/components/schemas/PageToken'
namespaces:
type: array
uniqueItems: true
Expand Down