Clarify pagination description #9872
Conversation
danielcweeks
force-pushed
the
fix-pagination-desc
branch
2 times, most recently
from
99afb91 to
adc94f2
Compare
open-api/rest-catalog-open-api.yaml
Outdated
| Servers that support pagination will 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 is used as the parameter value | ||
| for the next request. An empty `next-page-token` in the response signals that pagination |
There was a problem hiding this comment.
What does it mean for this to be empty? Empty string, null, missing? Do all of them work?
I'd prefer either an explicit null or not sending back the key. Probably best to send an explicit null.
There was a problem hiding this comment.
I added clarification around what the server needs to return and what conditions the client needs to account for.
There was a problem hiding this comment.
@danielcweeks @rdblue
Do we also need to clarify what the "empty" pageToken means as well i.e(Empty string, null, missing)
There was a problem hiding this comment.
I don't think we need to clarify since an empty query parameter is standardized URL convention. Implementations may have minor variations, but I don't think that needs to be addressed here.
danielcweeks
force-pushed
the
fix-pagination-desc
branch
from
adc94f2 to
1edeae9
Compare
danielcweeks
force-pushed
the
fix-pagination-desc
branch
from
1edeae9 to
316875c
Compare
|
cc @jackye1995 |
| parameter value for the next request. The server must return `null` value for the | ||
| `next-page-token` in the last response. | ||
|
|
||
| Servers that support pagination must return all results in a single response with the value |
There was a problem hiding this comment.
How does the service distinguish this case from the initial request when pageToken is empty, but we still return the number of items determined by pageSize?
There was a problem hiding this comment.
If no token is provided, I don't think pageSize makes sense because there's no guaranteed ordering. I think it's correct to say that if pageToken is not present, all results are returned.
|
How should we deal with this? There are 2 PRs opened: #9866 |
@jackye1995 I commented on the other PR, but I think we should close #9866 as it's missing a lot of corrections that need to be addressed |
|
I am quite confused about this situation. We literally merged a spec change, and then immediately published a revision which completely rewrites the description. We could have discussed this on devlist if there is any additional disagreements instead of just putting up a PR without even informing the original author or requesting original author and reviewers for review. This is even worse given that there is already an attempt for the original author to fix the comments Ryan made after the original PR is merged. This does not sound very community friendly to me. |
@jackye1995 I wasn't aware of the other PR, they were created relatively close in time. I originally thought it would be a small clarification and it expanded as the comments came in. I don't think there's really a disagreement in terms of the behavior, it was more that there were a number of concerns raised that the content wasn't clear. |
|
@jackye1995, as @danielcweeks said, this was originally a much more narrow fix that got larger as we went along. Also, most of the reason why the text was replaced was reformatting (see the early commit, 99afb91). I think both Dan and I just didn't think it was a significant change at first -- similar to how it doesn't seem necessary to ping @nastra, @geruh, and @danielcweeks on #9866. We'll try to do a better job pinging people next time. This was an accident. |
|
I caught up with @rahil-c and discussed the plan forward here and he offered to consolidate all of the feedback across this and the other PR. Seems like best approach is just to close these two and start with a new one to incorporate all of the feedback. Just to reiterate, the plan here is not to change anything about what was agreed upon in terms of the pagination behavior, but we looked at the description, there were some aspects that expectations that were not clearly called out. This one is a little more complicated since we're trying add this in a backward compatible way and therefore we need to be clear about both old and new clients/servers and what the expectation is in each case. Thanks @rahil-c for taking this up! |
| all results in a single response. The `next-page-token` must be omitted or set to `null` | ||
| in the response. | ||
|
|
||
| Clients must interpret either `null` or missing response value of `last-page-token` as |
There was a problem hiding this comment.
Since there is no field called last-page-token we should just refer to this as next-page-token instead.
| `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` set to `null` if the query parameter `pageToken` is not set in the |
There was a problem hiding this comment.
Instead of setting the next-page-token to null we can also just omit this from the response right?
|
Closing in favor of #9917 |
4 participants
No description provided.