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

Define client-initiated pagination #184

Conversation

alien-mcl
Copy link
Member

This is a derivative of a PR #182 that covers #102 :

  • added skip/take predicates that can be used for IriTemplate variable mappings

@alien-mcl alien-mcl mentioned this pull request Feb 11, 2019
"@type": "rdf:Property",
"label": "skip",
"comment": "Instructs to skip N elements of the set.",
"range": "xsd:integer",

Choose a reason for hiding this comment

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

What's the domain of those properties?

Choose a reason for hiding this comment

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

shall we add "domain": "hydra:IriTemplateMapping"?

Copy link
Member Author

Choose a reason for hiding this comment

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

No domain is specified, enabling any kind of resource to have that property. It's purpose it to have a property that could be used indeed in IriTemplateMapping's property relation. Domain of hydra:IriTemplateMapping would be incorrect here.

@tpluscode
Copy link
Contributor

I remember how we were discussing this. skip/take is but just one possibility. The other common would be explicit page number.

I think we can address both by extending the IRI Template description. I'll continue on #102

@alien-mcl
Copy link
Member Author

I've decided on using skip/take (alternatives are offset/limit) as more generic than pageIndex/itemsPerPage. The latter can be modeled with the former (you can recalculate), but not the opposite as the latter assumes that each page has same number of items - this assumptions is no more with offset/limit approach.

@alien-mcl
Copy link
Member Author

This approach also removes the issue of page identifiers - skip/take will always be a number of collection elements.
The only thing I'm unhappy is the logic of the item - it can be either a single resource matching manages block, but in case it is not provided it could mean a single RDF statement.

@tpluscode
Copy link
Contributor

I don't see how either is more "generic" than the other. What's more, in my experience pageIndex/itemsPerPage approach is more common in API design.

I think we should not be deciding for and against either. Rather, provide a flexible enough set of primitives so that any design in possible to be described with Hydra.

Such is what I started discussing on #102, to base reduce the entire feature to IRI Templates. That way collection design is just one of possible applications of a more generic feature.

@alien-mcl
Copy link
Member Author

I don't see how either is more "generic" than the other.

As I wrote, you can express pageIndex/itemsPerPage with offset/limit, but not the opposite (limit can change on each request).

What's more, in my experience pageIndex/itemsPerPage approach is more common in API design.

For desktop views, your right. But for mobiles, not really.
In streaming-line approach you'd favor skip/take as usually there is only more button, so the only thing to do us to add previous take and skip values and you've got another skip for next request (you would even omit take part and leave it for server to decide).

@tpluscode
Copy link
Contributor

As I wrote, you can express pageIndex/itemsPerPage with offset/limit, but not the opposite (limit can change on each request).

That is not the point. If I have an API which has collection URLs in the form of /collection{?pageIndex,pageSize} there is nothing to express differently. Because whose responsibility it would be to translate one form to the other? The generic client's?

The design is what it is and we must allow to describe both and possible any other, even if it's going to be specific to the given API (if it should use custom descriptions).

For desktop views, your right. But for mobiles, not really.

We should not discuss desktop vs mobile. We are designing a generic API Description vocabulary. If it favours one solution (skip/take) over another (page index/page size) then we have failed.

In streaming-line approach you'd favor skip/take as usually there is only more button [...]

This argument is moot as such "streaming" it's already easier done by following the next link. This PR will not change it.

@alien-mcl
Copy link
Member Author

I've added another predicate for identifying a specific page. I've removed the range as the page identifier could be virtually anything (from number as in indexed pages to GUID).
I assumed that hydra:take will work as the page size. I was thinking about changing it to something more generic, i.e. hydra:limit - works better for both approaches.

@angelo-v
Copy link

I've added another predicate for identifying a specific page.

hydra:page still sounds, as if it links to some kind of page resource.

How about hydra:pageReference?

Or at least clarify the comment: "A server-known reference to a specific page to provide."

But again: Since it is a server-known reference, how can the paging be client-initiated? This actually only makes sense, if the client can assume that it is a page number it can simply increment. UUIDs etc. won't work out, because the client cannot know what to put in there.

@tpluscode
Copy link
Contributor

This actually only makes sense, if the client can assume that it is a page number it can simply increment. UUIDs etc. won't work out, because the client cannot know what to put in there.

Hence my comments above and under #102. It is more important to ensure that the server can instruct the client what are the allowed value for the pageReference. In natural language those could be

  • natural number where 1 <= N < 100
  • a letter [A-Za-z] (think of a lexical index)
  • anything else (requiring custom client code, which is fine)

To get this right we should also first ensure that we have a solid and share understanding for how the templates are supposed to be used. Please compare to #152

@angelo-v
Copy link

To get this right we should also first ensure that we have a solid and share understanding for how the templates are supposed to be used.

Since this is more complicated, I propose to only merge the skip/take approach for now (which adds at least some value for APIs using that approach) and work further on alternatives afterwards.

@alien-mcl
Copy link
Member Author

Since it is a server-known reference, how can the paging be client-initiated?

Good point - skip/take approach is free of that issue as it relies only on number of items within the set (which will be always a natural number).

anything else (requiring custom client code, which is fine)

This is related to #82 . We could allow to provide an explicit list of allowed literals or a link to the endpoint that would provide those.

@tpluscode
Copy link
Contributor

Good point - skip/take approach is free of that issue as it relies only on number of items within the set (which will be always a natural number).

This is irrelevant. Like I said, not every API will want to follow the skip/take design and I'm pretty sure that it's a less common approach in practice outside of implementations close to a query language, like OData. I find it harmful that Hydra should inadvertently favour one possible approach over another.

This statement is also inaccurate. Just like with pageReference or any other alternative, it's not nearly enough to just tell the client

these are the skip and take variables, deal with them

The server will most likely still impose restrictions, such as Page size cannot be greater then 50.

If we take a step back and look at ways to extend the vocab around templates, we will be able to represent any design

@alien-mcl
Copy link
Member Author

My proposal is:

  • add hydra:offset (replacement for hydra:skip introduced here) with range of xsd:nonNegativeInteger
  • add hydra:limit (replacement for hydra:take introduced here) with range of xsd:nonNegativeInteger
  • add hydra:pageIndex with range of xsd:nonNegativeInteger
  • add hydra:pageReference with range unspecified for future detailed specification (custom indices)

This way we'll cover 99% of paging use cases (pageIndex/limit fits not too bad, offset/limit fits well) and we're still enabled for future custom usages including GUID or letter indices. The latter will require some more concept i.e. variable constraints, allowed literals/individuals, etc.

…ges:

- skip -> offset
- take -> limit
- page -> pageIndex
- pageReference added
Copy link

@vddesai1871 vddesai1871 left a comment

Choose a reason for hiding this comment

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

How pageReference will be used in the current state of vocab? I don't see any reason to add it now. It will just end up confusing newcomers.

"vs:term_status": "testing"
},
{
"@id": "hydra:pageReference",

Choose a reason for hiding this comment

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

Is it only me or pageReference still does not make much sense? Can't we just go with offset, limit and pageIndex for now and create another issue or PR before we add it to the vocab.

Copy link
Member Author

Choose a reason for hiding this comment

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

We wouldn't like to limit possible usages only to numeric pages - that's why pageIndex. I've recently added a subPropertyOf relation between pageIndex and pageReference - it feels natural this way.
Both predicates has similar semantics - here client you can point to a specific page of the set. The extra thing that comes with pageIndex is it's cleanness, which covers most use cases.
While it is not possible to provide i.e. min/max range or other means of expressing allowed values for pageReference, this predicate still can be used as it is now with some success.

@alien-mcl
Copy link
Member Author

Is it OK to merge this one? Are there any objection against this solution?

@angelo-v
Copy link

Is it OK to merge this one? Are there any objection against this solution?

I am fine with it.

@tpluscode
Copy link
Contributor

Yup, do it.

@alien-mcl alien-mcl merged commit 5035af4 into HydraCG:master Mar 19, 2019
@angelo-v
Copy link

@alien-mcl There is a problem with the rendering of html page:

hydra:pageIndex
Instructs to provide a specific page of the collection at a given index.

Range: undefined

Subproperty of: [object Object]

Status: testing

http://www.hydra-cg.com/spec/latest/core/#hydra:pageIndex

@tpluscode
Copy link
Contributor

@alien-mcl please turn @angelo-v's comment into a new issue

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.

4 participants