Support Multi-Search #321

ellnix · 2024-02-09T09:10:13Z

Pull Request

Related issue

Fixes #254

I thought I'd make a draft PR to discuss and review API decisions. I discussed the method signature of a theoretical multi_search in #254 (comment), if there is no problem I will proceed with this one:

MeiliSearch::Rails.multi_search(
  'book_production' => {q: 'paper', class_name: 'Book', **book_options}, # Index with a model
  Product => {q: 'thing', **product_options}, # Model with implied index
  'blurbs' => { q: 'happy' }, # Index not backed by a model, results will be simple hashes
  **other_searches
)

Initially I expected that the return type would be a simple array, however this might not be ideal since it

does not provide a performant way to use only the results of a single search
does not provide a way to access search metadata provided by meilisearch

I am thinking of either a simple hash or a hash-like class with convenience methods.

codecov · 2024-02-09T09:11:34Z

Codecov Report

Attention: Patch coverage is 97.36842% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 90.22%. Comparing base (b0bfab1) to head (9c186c9).

Files	Patch %	Lines
lib/meilisearch/rails/multi_search/result.rb	95.55%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #321      +/-   ##
==========================================
+ Coverage   89.47%   90.22%   +0.75%     
==========================================
  Files          11       13       +2     
  Lines         684      757      +73     
==========================================
+ Hits          612      683      +71     
- Misses         72       74       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ellnix

Should be ready for review (but I don't think it should be merged in this state), please see the comments I left.

lib/meilisearch/rails/multi_search/result.rb

ellnix · 2024-02-12T15:45:28Z

lib/meilisearch/rails/multi_search/result.rb

+
+      private
+
+      def load_results(klass, result)


Need some guidance on how pagination would be expected to work in multi search.
Paginate the results of each search, paginate the search as a whole, or both? Waiting on a review before going through with a solution.

As far as I know and per the docs: https://www.meilisearch.com/docs/reference/api/multi_search

You handle the pagination by each "block" of returned data. Because essentially the multi_search is just a way of making multiple requests at once.

That checks out, since each search can have its own pagination options. This would mean that displaying all of the results together would not make much sense, since they would never be on the same "page".

Perhaps I should expect that #each_result should be the main way to use a MultiSearchResult? It might make most sense in the documentation.

To be honest, I am quite unsure about how the users are going to make use of this feature, so I would prefer to have some external output before applying more changes to it. But I'm on your side to whichever option you may prefer.

ellnix · 2024-02-12T15:47:52Z

lib/meilisearch/rails/multi_search.rb

+module MeiliSearch
+  module Rails
+    class << self
+      def multi_search(searches)


Placed the multi search method as MeiliSearch::Rails.multi_search for the time being, but it does not feel like we should expect our users to write it all out.

Maybe ApplicationRecord.multi_search, or give all models the same, generic, multi_search method?

I think for convenience, we could add it to ApplicationRecord indeed.

brunoocasali

I think it is a good starting point to have this feature on rails!

I made some comments, let me know if you agree with them

lib/meilisearch/rails/multi_search.rb

brunoocasali · 2024-02-14T18:15:15Z

lib/meilisearch/rails/multi_search/result.rb

+
+      private
+
+      def load_results(klass, result)


As far as I know and per the docs: https://www.meilisearch.com/docs/reference/api/multi_search

You handle the pagination by each "block" of returned data. Because essentially the multi_search is just a way of making multiple requests at once.

spec/multi_search_spec.rb

spec/multi_search/result_spec.rb

brunoocasali

LGTM!

curquiza · 2024-02-23T15:56:20Z

@ellnix merge when you want 😊

ellnix · 2024-02-23T15:59:57Z

@ellnix merge when you want 😊

I do not think this is ready yet, I have to implement Bruno's suggestions and also some things I left out myself, for instance it's still not tested for MongoDB and Sequel.

curquiza · 2024-02-23T16:05:46Z

I was confused by the LGTM of Bruno 😄

brunoocasali · 2024-02-26T14:53:00Z

Hey @ellnix I thought we were ok with releasing like this, most of my suggestions are not logic-related but style ones.

Plus about this comment:

"for instance it's still not tested for MongoDB and Sequel."

I'm not even sure we still can say that we support MongoDB in the first place, there is no "real" test coverage for MongoDB.
So I don't think you need to make this PR wait if you see it is not working for mongo (we could work on that later)

ellnix · 2024-02-26T14:58:44Z

I'm not even sure we still can say that we support MongoDB in the first place, there is no "real" test coverage for MongoDB. So I don't think you need to make this PR wait if you see it is not working for mongo (we could work on that later)

Yeah, I thought I could somehow test them but that was my bad. Anyway the other changes I wanted to be done are done, this is ready to merge if it looks good to you now that pagination is implemented.

brunoocasali

It should be good now, with the pagination :)

I didn't try locally, but looking closely to the tests, they seem good enough!

LGTM

ellnix · 2024-02-27T19:44:00Z

bors merge

meili-bors · 2024-02-27T19:45:46Z

Build succeeded:

integration-tests (Rails 6.1 with Ruby 2.7)
integration-tests (Rails 6.1 with Ruby 3.0)
integration-tests (Rails 6.1 with Ruby 3.1)
integration-tests (Rails 7 with Ruby 2.7)
integration-tests (Rails 7 with Ruby 3.0)
integration-tests (Rails 7 with Ruby 3.1)
linter-check
smoke-test

ellnix force-pushed the support-multi-search branch 3 times, most recently from 6222e8e to 1eb2b84 Compare February 12, 2024 15:33

ellnix marked this pull request as ready for review February 12, 2024 15:39

ellnix commented Feb 12, 2024

View reviewed changes

ellnix requested a review from brunoocasali February 12, 2024 15:48

brunoocasali reviewed Feb 14, 2024

View reviewed changes

brunoocasali added the enhancement New feature or request label Feb 14, 2024

brunoocasali self-requested a review February 15, 2024 19:46

brunoocasali previously approved these changes Feb 19, 2024

View reviewed changes

ellnix added 7 commits February 23, 2024 17:15

Write multi-search specs

3b2a8e4

Write simple multi-search implementation

502fc30

Fix context leak in multi-search spec

edd61ac

Refactor multi-search with rubocop

d3dd92a

Add MultiSearchResult

e67fe25

Fix linter problems

ef6b019

Implement multi-search pagination and refactor

b0248af

ellnix dismissed brunoocasali’s stale review via b0248af February 23, 2024 17:00

ellnix force-pushed the support-multi-search branch from cc9f4aa to b0248af Compare February 23, 2024 17:00

Fix linter problems

9c186c9

ellnix requested a review from brunoocasali February 23, 2024 17:41

brunoocasali approved these changes Feb 27, 2024

View reviewed changes

ellnix closed this Feb 27, 2024

meili-bors bot merged commit b5983bd into meilisearch:main Feb 27, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Multi-Search #321

Support Multi-Search #321

ellnix commented Feb 9, 2024

codecov bot commented Feb 9, 2024 •

edited

Loading

ellnix left a comment •

edited

Loading

ellnix Feb 12, 2024

brunoocasali Feb 14, 2024

ellnix Feb 14, 2024

brunoocasali Feb 15, 2024

ellnix Feb 12, 2024

brunoocasali Feb 15, 2024

brunoocasali left a comment

brunoocasali Feb 14, 2024

brunoocasali left a comment

curquiza commented Feb 23, 2024

ellnix commented Feb 23, 2024

curquiza commented Feb 23, 2024

brunoocasali commented Feb 26, 2024

ellnix commented Feb 26, 2024 •

edited

Loading

brunoocasali left a comment

ellnix commented Feb 27, 2024

meili-bors bot commented Feb 27, 2024

Support Multi-Search #321

Support Multi-Search #321

Conversation

ellnix commented Feb 9, 2024

Pull Request

Related issue

codecov bot commented Feb 9, 2024 • edited Loading

Codecov Report

ellnix left a comment • edited Loading

Choose a reason for hiding this comment

ellnix Feb 12, 2024

Choose a reason for hiding this comment

brunoocasali Feb 14, 2024

Choose a reason for hiding this comment

ellnix Feb 14, 2024

Choose a reason for hiding this comment

brunoocasali Feb 15, 2024

Choose a reason for hiding this comment

ellnix Feb 12, 2024

Choose a reason for hiding this comment

brunoocasali Feb 15, 2024

Choose a reason for hiding this comment

brunoocasali left a comment

Choose a reason for hiding this comment

brunoocasali Feb 14, 2024

Choose a reason for hiding this comment

brunoocasali left a comment

Choose a reason for hiding this comment

curquiza commented Feb 23, 2024

ellnix commented Feb 23, 2024

curquiza commented Feb 23, 2024

brunoocasali commented Feb 26, 2024

ellnix commented Feb 26, 2024 • edited Loading

brunoocasali left a comment

Choose a reason for hiding this comment

ellnix commented Feb 27, 2024

meili-bors bot commented Feb 27, 2024

codecov bot commented Feb 9, 2024 •

edited

Loading

ellnix left a comment •

edited

Loading

ellnix commented Feb 26, 2024 •

edited

Loading