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

elasticsearch_rails ignores page_param #123

Closed
0x0badc0de opened this issue Feb 5, 2019 · 7 comments
Closed

elasticsearch_rails ignores page_param #123

0x0badc0de opened this issue Feb 5, 2019 · 7 comments

Comments

@0x0badc0de
Copy link

Compare https://github.com/ddnexus/pagy/blob/master/lib/pagy/extras/elasticsearch_rails.rb#L15

vars[:page] ||= params[:page] || 1

and https://github.com/ddnexus/pagy/blob/master/lib/pagy/extras/array.rb#L16 (or other backends)

vars[:page] ||= params[ vars[:page_param] || VARS[:page_param] ]
@ddnexus
Copy link
Owner

ddnexus commented Feb 6, 2019

@0x0badc0de Thanks.
I think the elasticsearch_rails extra needs a refactoring. There is also #118 that has to be addressed. The only problem is that I know nothing about the elasticsearch_rails, I don't use it and I don't have much time to learn it and prepare a sample app to test it now.

However, I could give a shot at it if anyone (e.g. @Kani999, @0x0badc0de. @renshuki) that happen to use it could share a minimal rails app repo already configured with elasticsearch_rails working and a page showing the pagy output.

@0x0badc0de
Copy link
Author

@ddnexus here it is https://github.com/0x0badc0de/pagy-elasticsearch-demo let me know if it works for you.

@ddnexus
Copy link
Owner

ddnexus commented Feb 7, 2019

A setup with Docker Compose! That's great!
I will try to come out with a nice refactoring during the weekend.
Thank you!

@Kani999
Copy link

Kani999 commented Feb 7, 2019

The application by docker-compose run just fine.

I cannot debug the offset issue (#118) because of this part of code https://github.com/0x0badc0de/pagy-elasticsearch-demo/blob/master/app/controllers/notes_controller.rb#L12
The index page is loaded from the database and not from the elastic-search. I'll try to change the behavior, but I'm not familiar with docker, so I hope I won't struggle with it too much.

@Kani999
Copy link

Kani999 commented Feb 7, 2019

Ok I've run into the same error as in #118 - I've edit the index page, to load data by elasticsearch

app/controllers/notes_controller.rb

def index
    if params[:q].present?
      @pagy, @notes = pagy_elasticsearch_rails(Note.search(params[:q]).records)
    else
      @pagy, @notes = pagy_elasticsearch_rails(Note.search('*').records)
    end
end

First page runs ok
http://localhost:3000/notes

Note Load (0.7ms)  SELECT  "notes".*
FROM "notes"
 WHERE "notes"."id" IN ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10) 
LIMIT $11 
OFFSET $12 
 [["id", 14], ["id", 19], ["id", 22], ["id", 24], ["id", 25], ["id", 26], ["id", 29], ["id", 5], ["id", 8], ["id", 9], 
["LIMIT", 20],
["OFFSET", 0]]

The Second page have invalid offset so no records will be displayed
http://localhost:3000/notes?page=2

FROM "notes"
 WHERE "notes"."id" IN ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10)
 LIMIT $11 
OFFSET $12 
 [["id", 14], ["id", 19], ["id", 22], ["id", 24], ["id", 25], ["id", 26], ["id", 29], ["id", 5], ["id", 8], ["id", 9],
 ["LIMIT", 10],
 ["OFFSET", 20]]

The ["OFFSET", 20]] - do the problem i suppose

Even the page does not change (reocrds id are the same)

@ddnexus
Copy link
Owner

ddnexus commented Feb 7, 2019

I didn't write that extra, so I don't know whether there is some mistake in the doc or it is just plain wrong. It uses the same logic of the Array extra, which assumes you already have the array of items, and you extract a page from it... which doesn't seem the way to go with ESrails :)

Thanks for the extra editing. I will try it during the weekend.

@ddnexus
Copy link
Owner

ddnexus commented Feb 11, 2019

Fixed with the refactoring in branch v2.0-dev

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

3 participants