active_model_serializers
active_model_serializers copied to clipboard
rails-api
Prevent out of bounds pages from returning a next link.
Purpose
In the JSON:API adapter, this will prevent next link URLs from being generated for nonexistent pages if the page requested is out of bounds.
Changes
Now checking current_page against total_pages using >= instead of ==.
This also makes sure the prev link is a real page.
The self link is unmodified, it will return a link to the current page even if it doesn't exist. Seemed to best conform to the JSON:API spec.
Caveats
Related GitHub issues
Additional helpful information
@bf4 Is there anything more I can do to help get this merged? I think tests were failing on the original branch I forked which may have prevented this from getting looked at. This is now fixed since I just pulled the upstream changes.
@bf4 Would you be willing to provide any info on why this was closed and not merged? This was my first attempt at contributing to open source software, so the information would be much appreciated.
This looks fine to me. Does the spec comment on this scenario? I don't recall. hashtag 'lazy'
@bf4 I named them test_out_of_bounds_page_pagination_links_using_{serializer here}, but there isn't a specific comment outlining the reason for the test. I didn't see any other specs with comments outlining expected behavior. Is that something you'd like me to add?
I didn't see any other specs with comments outlining expected behavior
Sorry, I meant http://jsonapi.org/format/#fetching-pagination
Keys MUST either be omitted or have a null value to indicate that a particular link is unavailable.
So, nothing about out of bounds.
Ah, I see what you meant now. Yeah, it doesn't mention this specific circumstance. I decided when out of bounds: omitting the next link, providing the last page as the previous link, and providing a current link even though you are out of bounds seemed to best conform to my reading of the spirit of the spec.
It'd be good to see this merged. Is there anything I can do to help move things along?
This appears to have been addressed by this newer PR from December 2020: https://github.com/rails-api/active_model_serializers/pull/2399
A little disappointed #2171 was never merged, but glad it's fixed nonetheless.