Fix pagination description based on new feedback #9917
Conversation
open-api/rest-catalog-open-api.yaml
Outdated
| signaling to the service that the response should be paginated. | ||
| An opaque token that allows clients to make use of pagination for list APIs | ||
| (e.g. ListTables). Clients may initiate the first paginated request by sending an empty | ||
| query parameter `pageToken` to the server e.g. `GET /tables?pageToken` or `GET /tables?pageToken=` |
There was a problem hiding this comment.
I don't think this should have multiple "examples" that conflict with one another. If an example is necessary (and I don't think that it is) then there should be only one recommendation in a spec.
There was a problem hiding this comment.
Sounds good, will include one example then with the = form, based on your recommendation from prior pagination pr.
There was a problem hiding this comment.
To be clear, I prefer removing these examples because this is an area that should be covered by the OpenAPI spec. The purpose of OpenAPI is to allow us to specify these things without ambiguity. I would prefer relying on that rather than giving an example here at all.
But if you want to have an example, there should be only one example.
There was a problem hiding this comment.
I think your point is fair I only see one example of this in the rest spec https://github.com/apache/iceberg/blob/main/open-api/rest-catalog-open-api.yaml#L198
So for now will remove these examples based on your feedback.
There was a problem hiding this comment.
@rahil-c I did see those examples too and I think the difference there is there may be some confusion about how namespaces are encoded where as here, an empty query parameter really isn't ambiguous and is covered by standard URL specification.
open-api/rest-catalog-open-api.yaml
Outdated
| Servers that support pagination should identify the `pageToken` parameter and return a | ||
| `next-page-token` in the response if there are more results available. After the initial | ||
| request, the value of `next-page-token` from each response must be used as the `pageToken` | ||
| parameter value for the next request. The server must omit or return `null` value for the |
There was a problem hiding this comment.
A spec should not specify two options. I think the service should send null.
There was a problem hiding this comment.
Ok just want to clarify what we mean by the service should send null (does it mean the same as omitting the field). Since in the last line of the pagination description i think we wrote this
Servers that do not support pagination should ignore the
pageTokenparameter and return
all results in a single response. Thenext-page-tokenmust be omitted or set tonull
If setting null on service side means the same as service is omitting the next-page-token field in the response, something like this
ListIcebergNameResponse {
"namespaces" : [...]
}
then would it be clearer to just write the service should omit next-page-token when no more results are available?
There was a problem hiding this comment.
Sending null and omitting the field are different. This should explicitly send null for next-page-token. The idea is to always be clear about what, exactly, should be sent. Clients need to interpret both null and missing to stop further requests. But for a service what to send must always be clear.
There was a problem hiding this comment.
Thank you for the clarification will fix this.
open-api/rest-catalog-open-api.yaml
Outdated
| `next-page-token` in the last response. | ||
|
|
||
| Servers that support pagination must return all results in a single response with the value | ||
| of `next-page-token` omitted or set to `null` if the query parameter `pageToken` is not set in the |
open-api/rest-catalog-open-api.yaml
Outdated
| request. | ||
|
|
||
| Servers that do not support pagination should ignore the `pageToken` parameter and return | ||
| all results in a single response. The `next-page-token` must be omitted or set to `null` |
There was a problem hiding this comment.
So above I think you mentioned to use the service should send null. but here you mentioned to use must be omitted. Just want to confirm which option we should say across the description to be consistent( I think omission makes sense to me)
There was a problem hiding this comment.
This is for services that do not support pagination, like all existing ones. In that case, they should not send the key. The difference is between not supporting pagination and ending paginated request exchanges.
There was a problem hiding this comment.
will fix this thanks for clarifying this.
|
Was wondering if you @nastra or @amogh-jahagirdar can take a look whenever you get a chance, appreciate it. |
|
Look good. Thanks, @rahil-c! |
…9917) Co-authored-by: Rahil Chertara <[email protected]>
3 participants
Incorporated feedback from recent pagination spec fix prs into this one.
cc @jackye1995 @rdblue @danielcweeks @geruh @nastra