-
Notifications
You must be signed in to change notification settings - Fork 37
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
Add special response/error for case where deep pagination is not accessible #458
Comments
Since there seems to exist a general solution for this problem, is there a reason to put it on clients to deal with it? Couldn't those with backends that work like just rewrite the If I'm not misremembering this, the About data consistency: OPTIMADE doesn't impose any requirements here (but maybe it should), so I expect many providers not to handle this consistently even when their paging "works". I suspect it can never be a MUST requirement to handle both paging of huge result sets and data consistency, but if your database can do it, you can of course also append |
It's not so much "putting it on them" but making it possible full stop (in an automatic way across databases), which I think requires the error.
I guess they could, but we either enforce that you should always be able to get all data for a query through next links or we provide a special error message that allows providers to opt out and lets the clients handle it how they please. Markus has been responding in nomad-coe/nomad#45 about ways we can get around the root problem in ES. |
I don't see any reason to object against a "new" error code as long as there is one provider who says "this would be useful for us", so feel free to PR this if this is the case. Nevertheless, to "make it possible" to use for clients, you still need the database to implement the error code. When they do that, I'd think most databases would realize they could as well fix the pagination issue in their implementation so the client doesn't have to bother with that error code.
So, it seems in this case the change may just be to swap out what ES feature is used for paging the results, so it seems at least for that implementation, no error code will be needed. |
Okay, lets see how it goes then. Quick follow-up Q before I close this as I'm now implementing
I think the |
... no? These are REST query parameters. They are always "strings" when they arrive through your framework's handling of GET parameters, so quoting them would be strange, it would mean you specify the page to be the string |
But |
You mean non-url safe? As far as I know, all characters can be represented via url-encoding, and are then url-decoded as part of resolving the GET parameters. Enclosing the URL query parameters with quotes will neither help or prevent that; you'll just end up with a string with quotes around it in the end. |
Yes, sorry, sloppy language. I'm not sure I'm making myself clear. Its only the value that needs to be in quotes, in the same way that it would be for a filter query, e.g.,
I don't think there is anything stopping an ID containing e.g., Likewise, we seem to be saying the definition above that the |
I think it may just be me lousy at explaining :-) A properly URL encoded version of the query part of a REST request communicating that the parameter
A reasonable REST framework will url-decode this and have no trouble parsing it into some form of GET key-value data structure corresponding to the following JSON:
If you instead put the query parameter value in quotes before you URL-encode the request, the URL-encoded version is:
and the REST framework would parse it into (also JSON, with \ escaping the double quotes present in the string):
which seems to provide little benefit over not including the quotes. |
Right, I'm with you now, thanks for the full explanation. I have been handcrafting queries for too long to appreciate that & would not be valid but %26% is... |
I find this very strange. When I use a filter, I have to use quotes like: ps. |
The contexts and respective left hand sides in your examples refer to different things. In the filter case, the identifiers are property names, which refer to things with data types; whereas HTTP GET parameter names usually aren't seen as things with data types ("they are all strings"). Sure, an API specification could still decide to say that the REST parameters are to be quoted. The outcome is that clients need to do an unnecessary extra layer of escaping ( By the way, for OPTIMADE, wouldn't this also mean the whole filter expression has to be enclosed in quotes? So, then one needs to escape the quotes inside the filter string, and double escape (i.e., three backslashes) double quotes inside strings inside the filter expression. I can't believe anyone wants that. Anyway, aside all this, we already defined - at least via examples - these things to not have quotes. Enforcing quotes now is a client breaking change (widely so for the filter, unless you mean that is for some reason exempt from quoting?), so, if you genuinely mean to propose this behavior, it would IMO have to be for OPTIMADE V2. |
I am not sure if any of the current clients are using page_above or page_below, but if they do, it would indeed be a breaking change. |
Some database backends have limitations on the depth of pagination into a filter, for example Elasticsearch by default does not allow random access to pages of results beyond the first 10000 results.
For example, querying for the 10001th result in NOMAD for a given filter
"nsites > 0"
:One typical workaround here is to specify a sort field (say, ID) and direction, then begin a new query that filters out the results from the first page by querying for
id > <last_id_on_prev_page>
.Is this problem ubiquitous enough that we could add a specific error type to the specification that clients could employ in order to apply client-side workarounds to this problem?
One issue to mention is the potential for data inconsistency between queries, but we don't concern ourselves with that in the spec atm...
The text was updated successfully, but these errors were encountered: