meilisearch / meilisearch-rails

Meilisearch integration for Ruby on Rails
https://www.meilisearch.com
MIT License
308 stars 47 forks source link

can't search with mongoid #250

Closed masukomi closed 1 year ago

masukomi commented 1 year ago

context: a Task model in a rails app backed with Mongoid. ms_search will never work because it always assumes columns

The relevant comment from the code:

    # The condition_key must be a valid column otherwise, the `.where` below will not work
   # Since we provide a way to customize the primary_key value, `ms_pk(meilisearch_options)` may not
   # respond with a valid database column. The blocks below prevent that from happening.

There will never be a "valid column" in MongoDB. Columns don't exist in document databases.

Expected behavior not to try and call .columns when using mongoid

Current behavior 💥 because mongoid doesn't have a columns method (MongoDB is a document database so it wouldn't make sense)

/Users/masukomi/.rbenv/versions/3.2.0/lib/ruby/gems/3.2.0/gems/meilisearch-rails-0.9.0/lib/meilisearch-rails.rb:645:in `ms_search': undefined method `columns' for Task:Class (NoMethodError)

                                     meilisearch_options[:type].columns.map(&:name).map(&:to_s).exclude?(condition_key.to_s)

Environment (please complete the following information):

masukomi commented 1 year ago

potential fix. This appears to fix the described problem but blows up when it hits #251 on the next assignment. I have NOT exhaustively tested this.

        has_virtual_column_as_pk = if defined?(::Sequel::Model) && self < Sequel::Model
                                     meilisearch_options[:type].columns.map(&:to_s)
                                   elsif defined?(::Mongoid::Document) && include?(::Mongoid::Document)
                                     meilisearch_options[:type].fields.keys.map(&:to_s)
                                   else
                                     meilisearch_options[:type].columns.map(&:name).map(&:to_s)
                                   end.exclude?(condition_key.to_s)

Note that this is hampered by a faulty algorithm above for choosing the primary key

 # condition_key gets the primary key of the document; looks for "id" on the options
        condition_key = if defined?(::Mongoid::Document) && include?(::Mongoid::Document)
                          ms_primary_key_method.in
                        else
                          ms_primary_key_method
                        end

in mongoid this will result in

 #<Mongoid::Criteria::Queryable::Key:0x00000001083bad40 @block=nil, @expanded=nil, @name=:id, @operator="$in", @strategy=:__intersect__>

which, when you call .to_s on it, will give you "id"

There are 3 problems with that though.

  1. it's nonsensical to call .in because that's for creating a query where you want to find if something is in a list
  2. it works without the .in
  3. "id" isn't the name of the primary key column in mongid anyway. it's "_id" and I believe it's ALWAYS "_id"

So, i think the correct thing to do there is to say

 # condition_key gets the primary key of the document; looks for "id" on the options
        condition_key = if defined?(::Mongoid::Document) && include?(::Mongoid::Document)
                          '_id'
                        else
                          ms_primary_key_method
                        end
brunoocasali commented 1 year ago

Hi @masukomi, thanks a lot for your issue.

Indeed the support of Mongoid is flaky, plus there is no real coverage of its features in the code https://github.com/meilisearch/meilisearch-rails/issues/175.

Unfortunately, I didn't have much time to dig into the Mongoid support yet 😓. But I would appreciate it if you or anyone else could tackle those issues.

masukomi commented 1 year ago

how do you feel about me refactoring the enclosing method. It's doing a lot of things, and it'd be much easier to test and know if i haven't broken things if i broke it down into a bunch of smaller methods.

brunoocasali commented 1 year ago

Any contribution is welcome! But since the meilisearch-rails.rb is quite big, I would like to have mongo id working before any refactoring because this is not the only place a refactoring is required.

So if we could add the Mongo support and then work in a wide refactor, it would be better because the test suite is also something that needs attention.

In any case, if you want to do it, I suggest starting small and submitting smaller PRs (they are easier to review) :)

Thanks a lot for your help!

adback03 commented 1 year ago

@masukomi did you get anywhere further on this? I'm running into the same issues. Thanks!

masukomi commented 1 year ago

@adback03 I started down the rabbit hole of fixing this but there was just too much complexity. I ended up just making a completely new library for it. Very stripped down, but (i think) easy to use, and flexible implementation. It is currently focused on Mongoid models. My plan was to release it as a new Gem, I just haven't gotten around to writing the docs or tests. Drop me an e-mail if you'd like access to the pre-release files (not gemmified yet). I've been running them for about a month now without issues.

I'll update this thread when that gem is available since i'm assuming Bruno won't have much time to focus on making it work in this gem. It may be a few weeks though (limited time).

brunoocasali commented 1 year ago

Hi @masukomi

Yes, I would like to have access to it 😄. Also, it would be nice to fix the implementation here since more people could benefit from your solution. I would love to have it integrated here because this is the best way to unite the community.

I will look into the code, but unfortunately, I can't promise I will have time to work on this maintenance right now.

In any case, thanks for helping us!

masukomi commented 1 year ago

@brunoocasali @adback03 finally got my gem into a releasable state. The new gem is called mongodb_meilisearch and the source is here on github Sorry i wasn't able to get it out sooner.

brunoocasali commented 1 year ago

I'm going to close this issue because all the MongoDB requests are going to be tracked in the issue #175.