Stream: ibm
Topic: Handling invalid page number
Lee Surprenant (Oct 29 2019 at 16:43):
I recently added PersistenceLayer (PL) test cases for handling invalid page numbers (AbstractPagingTest.testInvalidPage0 and AbstractPagingTest.testInvalidPage4)
Lee Surprenant (Oct 29 2019 at 16:45):
https://github.com/IBM/FHIR/pull/330 updates the JDBC impl so that it doesn't throw when we get an invalid page number
Lee Surprenant (Oct 29 2019 at 16:45):
but its causing the test to fail because its expecting an exception
Lee Surprenant (Oct 29 2019 at 16:46):
what is the proper behavior for a Persistence implementation when we receive a search with an invalid page number?
Paul Bastide (Oct 29 2019 at 16:46):
what type of invalid number?
Lee Surprenant (Oct 29 2019 at 16:46):
two examples
Paul Bastide (Oct 29 2019 at 16:46):
negative, zero...
postivie?
Lee Surprenant (Oct 29 2019 at 16:47):
negative / zero is one
more than the number of pages is the other
Lee Surprenant (Oct 29 2019 at 16:47):
so lets call them 0, and 4 (the test expects 3)
Paul Bastide (Oct 29 2019 at 16:47):
more than ... empty page
Lee Surprenant (Oct 29 2019 at 16:47):
in both cases, it is a client error
Paul Bastide (Oct 29 2019 at 16:47):
less than it should throw some loose exception / outcome or return the first page
Paul Bastide (Oct 29 2019 at 16:48):
once again.... my 1000 cents
Lee Surprenant (Oct 29 2019 at 16:48):
in https://github.com/IBM/FHIR/issues/194 we define a rule-of-thumb...that PL shouldn't throw for client errors
Lee Surprenant (Oct 29 2019 at 16:49):
...but one that our impl doesn't actually follow
Lee Surprenant (Oct 29 2019 at 16:49):
so this is an interesting test for that...
Lee Surprenant (Oct 29 2019 at 16:50):
what if we have the PL return an empty result along with an OperationOutcome with a warning that the page number was invalid?
Lee Surprenant (Oct 29 2019 at 16:51):
then, for the server, the REST layer can decide what to do with that (based on the passed Prefer "return" preference or the server config)
Lee Surprenant (Oct 29 2019 at 16:51):
and for the test, we could verify that:
- no results came back; and
- the response contains the warning
Lee Surprenant (Oct 29 2019 at 16:53):
@Paul Bastide thoughts on this proposal? or you still prefer throwing in the page 0 case?
Paul Bastide (Oct 29 2019 at 16:54):
I like the idea. I think that makes sense
Paul Bastide (Oct 29 2019 at 16:54):
it's a parseable result
Lee Surprenant (Oct 29 2019 at 16:54):
@Albert(Xu) Wang @John Timm ? ^
Lee Surprenant (Oct 29 2019 at 16:55):
i also kind of like paul's idea of returning page 1 if they ask for a negative or 0 page. any objections to this?
Lee Surprenant (Oct 29 2019 at 16:56):
something totally unexpected, but potentially handy, would be to interpret negative pages as index number from the back (like negative python array indices) :-)
John Timm (Oct 29 2019 at 17:36):
I support the option of using 1 if the page number is negative or zero
Lee Surprenant (Oct 29 2019 at 17:41):
and in the case of page number > last page, we just return 0 results?
John Timm (Oct 29 2019 at 17:43):
The reason why we no longer see an exception is because pagination happens on a List of resource ids, previously, we had a problem where when the list was empty, it was generating invalid SQL and ultimately throwing a FHIRPersistenceException. Now an empty list is returned if there are no resource ids for a given page (i.e. an invalid offset).
John Timm (Oct 29 2019 at 17:44):
should we have logic like this:
if (page < min) page = min
if (page > max) page = max
?
Lee Surprenant (Oct 29 2019 at 17:47):
i like the page<min check
Lee Surprenant (Oct 29 2019 at 17:47):
i think we're in a greement there
Lee Surprenant (Oct 29 2019 at 17:48):
for page > max, I think we're better off returning empty
Lee Surprenant (Oct 29 2019 at 17:48):
but i don't feel that strongly
John Timm (Oct 29 2019 at 17:48):
That's fair
Lee Surprenant (Oct 29 2019 at 17:49):
it would enable lazy folks to just keep incrementing page number til they don't get anything
Lee Surprenant (Oct 29 2019 at 17:49):
(as opposed to checking either the total or the lastpage like they should)
John Timm (Oct 29 2019 at 17:49):
Have we decided the best place to put the logic? I can add the change to my PR. Also, we will need to tweak the unit tests accordingly.
Lee Surprenant (Oct 29 2019 at 17:49):
no, we havn't determined where yet. yes, if you can add to your PR that would be ideal
John Timm (Oct 29 2019 at 17:51):
sure, i'll dig in and figure out where to put it
John Timm (Oct 29 2019 at 17:52):
so then on the search result bundle what do we put for prev / next if page > max = no results
John Timm (Oct 29 2019 at 17:52):
will we just keep reporting imaginary pages?
John Timm (Oct 29 2019 at 17:53):
I guess it would keep things consistent
John Timm (Oct 29 2019 at 17:53):
but it seems a bit odd to have self = 5000, pref = 4999 and next = 5001 when we are already way out of range of the actually paged data
Lee Surprenant (Oct 29 2019 at 17:54):
good question. i was mostly focused on whats the proper response from the Persistence Layer's perspective
Lee Surprenant (Oct 29 2019 at 17:55):
if we want, we return success=false
Lee Surprenant (Oct 29 2019 at 17:55):
and REST layer could map the OperationOutcome to a 400 error
John Timm (Oct 29 2019 at 17:56):
I'd be tempted to do that for all invalid page numbers... negative, zero, > max, etc.
John Timm (Oct 29 2019 at 17:57):
I'm just trying to make sure things are (mostly) consistent across the board
Lee Surprenant (Oct 29 2019 at 17:57):
yeah
Lee Surprenant (Oct 29 2019 at 17:58):
does spec define behavior for invalid page numbers?
Lee Surprenant (Oct 29 2019 at 18:00):
https://www.hl7.org/fhir/http.html#paging
Lee Surprenant (Oct 29 2019 at 18:02):
doesn't really help
John Timm (Oct 29 2019 at 18:02):
there is no official _page parameter
John Timm (Oct 29 2019 at 18:02):
only _count
John Timm (Oct 29 2019 at 18:03):
and if _count=0 then it should be interpreted as _summary=count
Lee Surprenant (Oct 29 2019 at 18:04):
yeah, i guess the _page parameter was our invention
Lee Surprenant (Oct 29 2019 at 18:04):
and what matters is the links in the response
Lee Surprenant (Oct 29 2019 at 18:05):
anyway, i think the cop-out on this issue is https://github.com/IBM/FHIR/issues/254 :-)
Lee Surprenant (Oct 29 2019 at 18:06):
lenient: < 1 -> 1; > last -> last
strict: < 1 or > last -> 400 error
Lee Surprenant (Oct 29 2019 at 18:07):
we already hide prev and next links for the first/last entry. so on that front we could just exclude them both if we have an invalid page number
John Timm (Oct 29 2019 at 18:08):
we need the lenient/strict behavior for invalid count as well
John Timm (Oct 29 2019 at 18:09):
and also we need _count=0 to invoke _summary=count
Lee Surprenant (Oct 29 2019 at 18:09):
I think 254 can cover _count validation, but wanna open new related issue for handling _count=0 ?
John Timm (Oct 29 2019 at 18:11):
@Lee Surprenant
254 can cover page and new issue for count or vice versa?
Lee Surprenant (Oct 29 2019 at 19:02):
sorry, i just meant 254 can cover the "validation" (and use of handling pref) for all parameters (_count, _page, and all others)
Lee Surprenant (Oct 29 2019 at 19:03):
so open a new issue for handling _count=0
Last updated: Apr 12 2022 at 19:14 UTC
