-
Notifications
You must be signed in to change notification settings - Fork 1
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 support for resumption tokens #11
Conversation
0e00591
to
4d6af89
Compare
4d6af89
to
da49e46
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! I tested it on my local for the new support for resumptionToken
, got 5 results per request, and there is a resumptionToken
element in the result if there are more records.
I just noticed that there is a rule for the resumptionToken
element when I read the doc.
I am wondering if we need to implement this? As I found there is no empty resumptionToken
element in the last response. 👀
Other than that, all looks great to me! 🌟
Nice spotting, @MelissaWu-SS! Thanks for picking that up. I'll work to add that now. |
645e3fd
to
039fa8f
Compare
@MelissaWu-SS I've added the empty Resumption Token for the "last page" of a paginated response. I was also originally not going to bother with expiring Resumption Tokens, however, I've had a change of heart. I've decided that I don't like the idea of Resumption Tokens "living forever". I've added some new functionality, which is the ability for us to expire our Tokens. The default is 1 hour. Easiest way to test the token expiry is to get your first response and grab the Resumption Token from it. Take it to a Base 64 decoder/encoder (https://www.base64encode.org/), and decode it. Change the expiry date to "an hour earlier", re-encode it, and use that as the Resumption Token. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet! Thanks for the suggestion around manually making the token expired! Otherwise I will probably wait for one hour😀
Just one minor change, other than that, the adding expiry attribute and empty resumption token element are looking great to me!🌟
$resumptionTokenElement = $this->findOrCreateElement('resumptionToken', $listRecordsElement); | ||
|
||
$resumptionTokenElement->nodeValue = $resumptionToken; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!$resumptionToken) { | |
return; | |
} |
Can we please add this for the empty resumption token scenario? 👀
I just realised it would cause an error in the decode process($decode = base64_decode($resumptionToken, true);
) if we set an empty resumption token in the last page of the response. Looks base64_decode
are not able to decode an empty string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @MelissaWu-SS!
Fixed at the source of the issue:
fad9151
Resumption Token
Resumption Tokens are a form of pagination, however, they also contain a level of validation.
Each Resumption Token should represent a specific request, including whatever filters might have been applied as part of that request, as well as representing a particular "page" in the Paginated List.
The goal is to increase reliability of pagination by making sure that each requested "page" came from a request containing the expected filters. EG: You can't send an unfiltered request for OAI Records, see that there are 10 pages, and then decide to request page=10 with some filters now applied. The Token itself would be aware that a different filter has been applied, and it would be invalid.
Consistency in config names
I realised that we had some snake case and camel case names. Switched everything to snake case to be consistent with SS framework.
09b0b38