Open ankane opened 4 years ago
This will be great. I recently had to create the graphql-searchkick gem to work around using Searchkick with GraphQL pagination. Any chance this will support lazy limit
and offset
calls as well?
@chadwilken Yeah, all the current options are available as methods.
@ankane --
Are there any plans to deprecate or discontinue the current interface?
Over here at Leafly, we've had really great success moving from Searchkick 3 -> 4 -> and now to 5. Pretty minimal changes on our side, but super huge value in terms of insulating us from the ES DSL and preparing us for smooth upgrades for ES major versions.
If the old interface were to be deprecated and/or removed, it would be, I think, a major upheaval for us -- We have many, many queries constructing hashes internally and converting these to the relation-based interface would not be straight-forward.
Presumably something we could plan for if we knew it were coming, so I wanted to check in :smile: .
Cheers and thank you for the great library!
Currently my team is trying to streamline our search code as we have been using both Searchkick.search
and Model.search
for example:
options = {
models: [SomeModel],
debug: debug,
execute: execute,
match: :text_middle,
fields: [
...
],
boost_by_recency: {
start_time: { scale: "21d", decay: 0.4 }
},
boost_where: {
state: { value: "posted", factor: 2 }
}
}
Searchkick.search(search_text, model: nil, **options)
But after reading this thread it appears we should be converting all this to Model.search
can you advise if this is the case @ankane ?
@philipbjorge Passing options to search
will still be supported, since all existing Searchkick code uses this approach. This could change in a future major release, but there are no plans to change it now.
@daande Either should be fine (Model.search
calls Searchkick.search
internally).
@ankane Kindly asking to add the possibility to get Searchkick::Results
from Searchkick::Relation
, something more legal than relation.send(:private_execute)
. I.e. something similar to ActiveRecord::Relation#to_a
.
Also we noticed that delegate_missing_to :private_execute triggers the query execution upon any random relation.respond_to?(:foo)
call. This way the Searchkick::Relation
gets the loaded status which forbids the further .page().per()
-like chaining.
Hey @stengineering0, thanks for reporting the respond_to?
issue. Fixed in the commit above. fwiw, you can still call non-mutating methods like page
and per_page
on a loaded relation (just not page!
or per_page!
).
Also, I'm not sure I understand the ask about Searchkick::Results
(you can already call to_a
on the relation). Can you share code for an example you're thinking of?
@ankane
fwiw, you can still call non-mutating methods like page and per_page on a loaded relation (just not page! or per_page!).
Definitively I saw the Relation loaded
exception after relation.respond_to?(:each)
and relation.page(1)
. I bet the current implementation like
def page(value)
clone.page!(value)
end
will clone self
entirely, including @execute
variable - which effectively forwards the loaded status to the new relation )
Also, I'm not sure I understand the ask about Searchkick::Results (you can already call to_a on the relation). Can you share code for an example you're thinking of?
I mean that sometimes might be better to operate by true Searchkick::Results
rather than proxy - just to be sure down the stack that query is executed already and will never be changed, as well as the resulting object stops replying true
on respond_to?(:page)
calls )
Well, honestly we have a huge framework around Rails controllers, including third-party gems like active_model_serializers
. And we face a tons of issues somewhere in the deep once Searchkick.search
started to respond Relation
instead of Results
. Most of them are about broken duck-typing checks because the framework is trying to process Searchkick and ActiveRecord result sets in similar way.
We can fix all of these and perhaps we will do this some day, but for now the easiest way to go ahead is doing like following at the beginning of action dispatcher:
def index
collection = User.search(...).send(:private_execute)
respond_with(collection)
end
For sure this approach effectively switches back to the legacy non-lazy searching - but it's a way cheaper for certain cases )
My bad, that was another bug. Fixed in the commit above.
Can you explain more about the issue you're seeing with active_model_serializers
?
In short, as per previous Searchkick interface we used the singular .search
call for ElasticSearch-backed collections. Such call absorbed all the things like filtering, pagination etc. Then a number of AMS-based Responder classes come into the game, one of them is trying to add total_count
meta info into response, the other will apply pagination if respond_with
collection is a kind of ActiveRecord relation, etc.
Most of those responders rely on duck-typing, i.e. them take total_count
if collection object responds on this method, the same for pagination and .respond_to?(:page)
pair. These all have been designed for and worked well while Model.search
replied Searchkick::Results
object, but now with relation:
Relation
respond to total_count
as per delegation to Results
, but once called this method triggers the ES query execution and switches the relation status to loaded
state;.respond_to?(:page)
, and get true
from Relation
which was false
with Results
. This way we apply the pagination to already loaded relation and get corresponding error.This is just one example, we face a number of similar issues - which can be fixed one-by-one some day for sure.. But for now we decided to turn Searchkick::Relation
back into Searchkick::Results
on framework level, this is collection.send(:private_execute)
-like one-liner which gives us the time to do the things without rush. Will be nice to get the same by some legal .to_results
method or whatever )
The current plan is
for Searchkick 5to have a new query API similar to Active Record.You can still use pass options to
search
for backwards compatibility, but queries will be performed lazily instead of eagerly.Edit: This wasn't included in Searchkick 5, but there's experimental support for some methods in the latest release. If you have any feedback, please comment below.