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

pagy_url_for in url mode gets duplicated page param #149

Closed
r-sierra opened this issue Mar 28, 2019 · 1 comment
Closed

pagy_url_for in url mode gets duplicated page param #149

r-sierra opened this issue Mar 28, 2019 · 1 comment

Comments

@r-sierra
Copy link

r-sierra commented Mar 28, 2019

Hey guys,
I'm trying to paginate an API, with the latest release(2.1.4)and the links generated by pagy_headers_hash are incorrect. Doing a GET request to http://localhost:3000/items.json?page=2 I expected pagy_headers_hash to return:

"Link": {
  "first":"http://localhost:3000/items?page=1",
  "prev":"http://localhost:3000/items?page=1",
  "next":"http://localhost:3000/items?page=3",
  "last":"http://localhost:3000/items?page=9"
  }

but instead got:

"Link": {
  "first":"http://localhost:3000/items?page=2?page=1",
  "prev":"http://localhost:3000/items?page=2?page=1",
  "next":"http://localhost:3000/items?page=2?page=3",
  "last":"http://localhost:3000/items?page=2?page=9"
}

After reading the code it seems that the problem is around this line:

url_str = pagy_url_for(Frontend::MARKER, pagy, :url)

which calls:
"#{request.send(path_or_url)}?#{Rack::Utils.build_nested_query(pagy_get_params(params))}#{p_vars[:anchor]}"

And request.send(:url) will return the full URL with its query string according to Rack documentation. Hence the double parameters.

Please let me know if you need any additional information.
Excellent work by the way! Awesome gem,
Thanks

@ddnexus ddnexus changed the title pagy_headers_hash not working as expected pagy_url_for in url mode gets duplicated page param Mar 29, 2019
@ddnexus ddnexus added the bug label Mar 29, 2019
ddnexus added a commit that referenced this issue Mar 29, 2019
- used concatenated request.base_url instead of request.url
- added better tests
- fixed overriding of pagy_url_for in items extra
@ddnexus
Copy link
Owner

ddnexus commented Mar 29, 2019

@r-sierra Yes, that's a bug, and, as you find-out, it is due to an improper use of request.url.

I forgot to add tests to check the full url when I pushed the headers extra. Adding better tests exposed also another missing thing: the items extra overrides the pagy_url_for but it used the old pagy_url_for implementation, so that has been fixed as well now.

Thank you.

@ddnexus ddnexus added the fixed label Mar 29, 2019
@ddnexus ddnexus closed this as completed Mar 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants