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

New Query API #1395

Open
ankane opened this issue Mar 18, 2020 · 11 comments
Open

New Query API #1395

ankane opened this issue Mar 18, 2020 · 11 comments

Comments

@ankane
Copy link
Owner

ankane commented Mar 18, 2020

The current plan is for Searchkick 5 to have a new query API similar to Active Record.

Product.search("apples").where(in_stock: true).page(10).per(50)

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.

@ankane ankane mentioned this issue Mar 18, 2020
41 tasks
@chadwilken
Copy link

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?

@ankane
Copy link
Owner Author

ankane commented Jun 11, 2020

@chadwilken Yeah, all the current options are available as methods.

@ankane ankane mentioned this issue Feb 22, 2022
13 tasks
@philipbjorge
Copy link

philipbjorge commented Mar 1, 2022

@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 😄 .

Cheers and thank you for the great library!

@daande
Copy link

daande commented Mar 11, 2022

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 ?

@ankane
Copy link
Owner Author

ankane commented Mar 12, 2022

@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).

@stengineering0
Copy link

@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.

@ankane
Copy link
Owner Author

ankane commented Oct 7, 2022

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?

@stengineering0
Copy link

@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 )

@stengineering0
Copy link

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 )

@ankane
Copy link
Owner Author

ankane commented Oct 7, 2022

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?

@stengineering0
Copy link

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:

  1. 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;
  2. Then on the same relation we check .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 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants