Skip to content
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.

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

Minimum Pagination Requirements #165

Closed
black-tea opened this issue Nov 6, 2018 · 15 comments
Closed

Minimum Pagination Requirements #165

black-tea opened this issue Nov 6, 2018 · 15 comments
Labels
Provider Specific to the Provider API
Milestone

Comments

@black-tea
Copy link
Contributor

When a provider decides to paginate the payload, the current MDS guidelines specify that pagination must comply with the JSON API specification. The specification requires that links be formatted a particular way, but allows that:

Keys MUST either be omitted or have a null value to indicate that a particular link is unavailable

suggesting that there is no minimum set of required links. In practice, so far I've always seen 'next', but there has been varying levels of implementation for the others. In theory, it seems plausible that a provider could just have both a first and last page, which would require a different method of iterating through page results than using next.

I thought it might be helpful to provide some additional guidance so that we can ensure a standard method for iterating through pages.

Suggestion
Given the flexibility in the JSON API spec above, if a Provider does decide to paginate, we should require at a minimum the next link.

Additional Questions

  1. At the end of paginating, should we explicitly require that it include a null value, or should we allow the next key to be omitted (which would be allowable under the current spec)?
  2. Should we be requiring more than just the next link? Should re require a last link? Should we require all four types of links?

I think that, at the very least, this would be helpful to provide some guidance. I know we've discussed this a bit with @asadowns. Any other thoughts?

@thekaveman
Copy link
Collaborator

See #160, which added the requirement for next, and #162 which allows it to be null.

I'm not convinced first and last are all that useful as URLs - it is really difficult to automatically expand the range between those (to get all possible pages) for all the different ways providers can do URL pagination. In my mind, that is the point of prev and next -- providers can implement URL pagination however they want, and we just keep asking for next.

A related thought I had on this: transition first and last to be bool indicating if the current page is either the first or the last page. Not sure how useful this is either. Maybe we remove those keys entirely in a future release?

@black-tea
Copy link
Contributor Author

@thekaveman Wonderful! Thanks for pointing that out - sorry I missed it.

I agree with your assessment of the benefit of first and last - I just wanted to check to make sure there wasn't something else that I was missing. Though I personally don't see any downside to providers including first and last, even if we are always keying next. By allowing it, we would stay pretty closely aligned with the JSON API pagination specs.

As far as the bool value, would it provide any additional benefit compared to having the next link return null when we hit the last page? Also, wouldn't it be a slight violation of the spirit of the JSON API spec:

first: the first page of data
last: the last page of data

As for requiring next at a minimum, I don't see a PR anywhere adding this to the README -- I'd be happy to put one together. If we are requiring next at a minimum, does that make sense to require the value of last page next link to be null (as opposed to the next key itself being absent)?

@thekaveman
Copy link
Collaborator

Though I personally don't see any downside to providers including first and last, even if we are always keying next. By allowing it, we would stay pretty closely aligned with the JSON API pagination specs.

Totally. No harm in those being there, and keeps it closer to a known spec. The bool value was just a random idea, ignore that 😏

I don't see a PR anywhere adding this to the README -- I'd be happy to put one together.

👍

If we are requiring next at a minimum, does that make sense to require the value of last page next link to be null (as opposed to the next key itself being absent)?

It gets tricky if we "sometimes" allow next to be absent - JSON Schema is not going to see a difference between validating a random page in the middle without the next key, vs. truly the last page of data without the next key.

Another option in that case is to simply leave the links object out entirely, as it is not required in the top-level schema.

But I am by no means saying I am the last word on this.

black-tea added a commit that referenced this issue Nov 6, 2018
Addresses part of #165, by adding an explicit requirement to the README that paginated payloads should include a `next` key at a minimum.
@thekaveman thekaveman added the Provider Specific to the Provider API label Nov 7, 2018
@thekaveman thekaveman added this to the 0.2.1 milestone Nov 7, 2018
@hunterowens
Copy link
Collaborator

reading through the relevent JSON API docs, my interpretation is as follows

  • If no pagination, the API can either not return a links object inside the top level object or return a links object with the only key being self.

  • if pagination, there MUST be a links object, with all 4 keys present or omitted to show the link is unavailable. see that Keys MUST either be omitted or have a null value to indicate that a particular link is unavailable.

This means that links: {'next': null} and links: {} are equivalent per spec. @black-tea @thekaveman correct me if I'm wrong.

Assuming that we are always hitting next, the required behavior should be once next is {null} or missing, it means that next cannot be provided, therefore we have reached end of the dataset. Thoughts?

@hunterowens
Copy link
Collaborator

@black-tea just reading the changes suggested in #166, I would suggest that for those changes we additionally add that either next: null or last be required so that we know we are on the last page of the data.

@thekaveman
Copy link
Collaborator

Here is what is currently valid under the JSON spec:

next with a pointer to the next page:

{
    "version": "x.y.z",
    "data": { },
    "links": {
        "next": "https://..."
    }
}

next is null, this is the last page

{
    "version": "x.y.z",
    "data": { },
    "links": {
        "next": null
    }
}

links is omitted, so we aren't paging -> this is the last page

{
    "version": "x.y.z",
    "data": { },
}

Importantly, what is invalid is a missing next key:

{
    "version": "x.y.z",
    "data": { },
    "links": { }
}

Even if there are other keys present, this is still invalid without next:

{
    "version": "x.y.z",
    "data": { },
    "links": { 
        "prev": "https://...",
        "last": null, 
    }
}

@black-tea
Copy link
Contributor Author

Thanks @hunterowens for the suggestion to #166 and @thekaveman for clarifying what we have so far. Based on our conversations with some providers who are saying it is difficult to implement the last link, and discussion above about using next, my preference is to have all the pagination behavior discoverable through next (rather than also keying last).

I also think that a null value would I think be a more explicit method of communicating that we are on the last page, as opposed to a missing next key, so my preference is to enforce that when we finish paginating, the value of the next key is null. This would be a slightly more restrictive condition than the current JSON API spec as I understand it, but I think it would be better.

I think that @thekaveman and I are interpreting the spec to not require all 4 links, which is why I think we can get everything by pushing a tighter standard on the next link. Thoughts?

@thekaveman
Copy link
Collaborator

thekaveman commented Nov 8, 2018

@black-tea are you saying you don't want this behavior supported, to indicate the last page?

links is omitted, so we aren't paging -> this is the last page

{
    "version": "x.y.z",
    "data": { },
}

In other words, are you saying: If paging is being done at all (even if it is the last page), links.next must always be present?

@black-tea
Copy link
Contributor Author

@thekaveman I'm suggesting that that behavior would be supported if they decide not to paginate, which we would know by the omission of the links key. However, I was suggesting that if links is present (from paginating), there should always be a next key, even if it is the last page.

I was suggesting that this behavior should not be supported:
{ "version": "x.y.z", "data": { }, "links": { } }
Is that too strict?

@thekaveman
Copy link
Collaborator

{ "version": "x.y.z", "data": { }, "links": { } } is not currently supported.

If links is there, next has to be there. That was #160 and #162.

I'll try to rephrase my question. For the purposes of signalling that "this is the last page of data, you don't have to request anything else right now", there are currently 2 different options:

  1. Use { "links": { "next": null } }

  2. Omit links entirely

My question is, are you suggesting we only support option 1? I personally don't see a problem allowing the second option, but I don't feel very strongly if option 1 is the only supported option.

@black-tea
Copy link
Contributor Author

Yes - Sorry - I meant to articulate that I support keeping that restriction as is. For checking the last page, I was just suggesting that we enforce allowing only (1) and not (2). I'm largely in the same camp as you though I think forcing { "links": { "next": null } } on the last page would just add some clarity.

@thekaveman
Copy link
Collaborator

Definitely agree with the clarity that { "links": { "next": null } } provides.

From a JSON Schema perspective though, we've already covered that. I don't know if there's anything else we can do in the schema to only allow (1) and not (2), since (2) has to be supported for those providers that choose not to page at all.

@black-tea
Copy link
Contributor Author

@thekaveman agreed. From a JSON Schema perspective they would be indistinguishable. I was thinking more about adding it to the README to enforce in-practice.

  1. I'll amend my original PR to add that requirement to the README.
  2. @hunterowens did you have any other strong feelings about having all of the links present or do you feel that the current guidance on requirements would be sufficient (and we can close out)?

black-tea added a commit that referenced this issue Nov 8, 2018
Following discussion #165, suggestion adds a explicit requirement for the last page of any paginated payload to return a value of `null` for the `next` key.
hunterowens pushed a commit that referenced this issue Nov 8, 2018
* Add `next` link requirement

Addresses part of #165, by adding an explicit requirement to the README that paginated payloads should include a `next` key at a minimum.

* Revision to for null value on last page

Following discussion #165, suggestion adds a explicit requirement for the last page of any paginated payload to return a value of `null` for the `next` key.
@thekaveman
Copy link
Collaborator

I think we're good to close this right @black-tea @hunterowens?

@black-tea
Copy link
Contributor Author

I think so, given that @hunterowens approved my PR with recommended changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Provider Specific to the Provider API
Projects
None yet
Development

No branches or pull requests

3 participants