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

standardize requesting a specific page/index in search api #381

Closed
joshfix opened this issue Jan 31, 2019 · 13 comments
Closed

standardize requesting a specific page/index in search api #381

joshfix opened this issue Jan 31, 2019 · 13 comments
Assignees
Labels
prio: should-have would be very good to have in the release
Milestone

Comments

@joshfix
Copy link
Contributor

joshfix commented Jan 31, 2019

sat-api supports requesting specific pages (with a page parameter), however WFS uses a startIndex parameter. The current spec (I believe) also defines a start and next parameter. We should standardize what parameters to use for pagination and determine whether it's implementation is optional or required.

@cholmes
Copy link
Contributor

cholmes commented Feb 4, 2019

I think it'd be good to stay aligned with WFS 3.

Here's the yaml bit from WFS 3 - https://github.com/opengeospatial/WFS_FES/blob/75d7c7839aca8c27ad9f8590727b37e110cfa0e7/placeholder-additional-conformance-class/parameters/startIndex.yaml

Ideally we can use that fragment directly in our little spec build system, and keep it as optional.

@cholmes cholmes added this to the 0.7.0 milestone Feb 4, 2019
@m-mohr
Copy link
Collaborator

m-mohr commented Feb 4, 2019

If not specified, does it return all elements or what number of elements does it return? Can it be specified?

I personally don't like the WFS approach as it depends on implementation details. I'd go another way (see next paragraph) and also propose it to WFS. StartIndex seems to be dropped anyway in Core, see WFS: opengeospatial/ogcapi-features#75

Pagination should not be used by default. If a client wants to use pagination it needs to send a parameter such as "count" (number of elements) with the request. That's actually what clients want to specify, not an index. In this case the endpoint returns the requested subset of elements and adds a link with next (and prev, maybe also last and first) rel types to the links array. The clients can follow these links to the other datasets. If a server does not support pagination, it should return an error and the client can decide whether it tries again without pagination or not as just returning the full list of elements instead of an error is critical as some applications may have problems with big datasets, which may lead to a slow down/crash/... in the worst case.

As I just saw, that is mostly what you also proposed here, @cholmes : opengeospatial/ogcapi-features#23 - It seems they agreed on this, but don't have it in the specification yet... We should push them to update?!

As client-side searching/sorting/ordering only works with all elements (i.e. it doesn't work with pagination) this needs to be enabled server-side and therefore needs to be part of the API.

@cholmes cholmes added the prio: should-have would be very good to have in the release label Feb 14, 2019
@joshfix
Copy link
Contributor Author

joshfix commented Mar 7, 2019

So it looks like wfs3 is going to keep startIndex around as an extension. I think I agree with @m-mohr and I'm not a huge fan of it, but I guess that's what we should go with?

@cholmes
Copy link
Contributor

cholmes commented Mar 13, 2019

What @m-mohr described is what WFS 3 says now. The 'count' parameter in the above comment is called 'limit' in WFS land. It's specified at https://cdn.rawgit.com/opengeospatial/WFS_FES/3.0.0-draft.1/docs/17-069.html#_response_6 So the only required 'parameter' is 'limit'. next and prev are links, next required, prev optional.

That should be consistent with what we have in our spec.

I'm ok if we want to have a 'paging' extension. If WFS 3 had the startIndex extension done I'd insist on that, and we can align with that in the future. @joshfix - if you want to make the paging extension with the openapi fragment that'd be great, and we can just align on what you and Matt have.

cc/ @hgs-msmith and @hgs-truthe01 - are you all doing random access paging? If so would be good to sound in.

@m-mohr
Copy link
Collaborator

m-mohr commented Mar 13, 2019

That sounds reasonable. Limit with next/prev is a very lightweight approach and can optionally be extended with a startIndex from an extension. That's what we will also implement in the openEO API, so +1 from us for that apprach.

@hgs-msmith
Copy link
Contributor

@cholmes, our current implementation is based on STAC 0.5.x, so we are supporting the page and limit parameters, allowing you to effectively do random access paging.

A couple of comments:

  • Preferably, we should align with WFS 3. The fact that I also prefer next and prev as links helps persuade me here.
  • I consider paging to be a core feature, so I would prefer that it is not an extension. I know it complicates things for implementors, but I think 'correctness' of the response wins out over this consideration. If the matching record set is greater than 'limit' then returning only 'limit' number of records without a way to access the additional records is not a correct response.

@matthewhanson
Copy link
Collaborator

We talked about this in Tuesdays meeting and while we want to align with WFS3 they use startIndex, which is only a problem because it's camelCase and would be the only parameter in all of STAC that uses camelCase (note it hasn't been a problem elsewhere because the only shared parameters are 1 word.

So our solution is to use page and try and get WFS3 to adopt page.

This would give us page and limit, where limit is the the max number of results returned, or effectively the page size.

@m-mohr
Copy link
Collaborator

m-mohr commented Apr 8, 2019

@matthewhanson Is there already a WFS issue on it? Seems things get rolling in their repo again (and there's the Hackathon soon). We shouldn't miss out to actually propose page.

@cholmes
Copy link
Contributor

cholmes commented Apr 8, 2019

+1 on getting in WFS issue, I don't believe there's one already. Let's get the PR in though, with the OpenAPI snippet that we need for it, and then we can propose to WFS as something 'real' that we're using, not just an idea. Could even make a PR on their repo, add a new 'extension' at https://github.com/opengeospatial/WFS_FES/tree/master/extensions

@cholmes
Copy link
Contributor

cholmes commented Apr 20, 2019

Assigning to you Matt, as you were going to take a crack at this.

@matthewhanson
Copy link
Collaborator

Yep, going to get final version of #432 complete then will issue PR for this

@cholmes cholmes closed this as completed May 2, 2019
@m-mohr
Copy link
Collaborator

m-mohr commented May 31, 2019

@matthewhanson @cholmes Seems like WFS people now decided to go without page parameter? Have we reached out to them and what was the response? If we really want to get them adopt the page parameter, now (i.e. before the OGC hackathon in June) is probably the last time to get it into the WFS3 spec...

@cholmes
Copy link
Contributor

cholmes commented May 31, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
prio: should-have would be very good to have in the release
Projects
None yet
Development

No branches or pull requests

5 participants