thoughtbot / administrate

A Rails engine that helps you put together a super-flexible admin dashboard.
http://administrate-demo.herokuapp.com
MIT License
5.9k stars 1.12k forks source link

Searching translated attributes yields "column does not exist" error #1837

Open sedubois opened 3 years ago

sedubois commented 3 years ago

Our app's content is stored in DB and translated using Mobility. For instance a Thought model has translates :quote and ThoughtDashboard has quote: Field::Text. Listing, showing and editing the thoughts' quotes in the different languages from the admin dashboard works as expected (*). Behind the scenes Mobility automatically generates the required getter and setter methods which it maps to ActiveRecord associations.

When using the administrate search box, records whose searchable attributes contain the provided text should be returned, whether these attributes are translated or not.

This error is raised when performing a search:

ActionView::Template::Error: PG::UndefinedColumn: ERROR:  column thoughts.quote does not exist
LINE 1: ...me" AS CHAR(256))) LIKE '%thought%' OR LOWER(CAST("thoughts"...
                                                             ^

It appears that because the attribute is Field::Text, Administrate assumes that the data is stored in a text column in the same table. However the data may well be stored in the same table but using JSON, or in a key-value polymorphic table, or (as in our case) in a belongs-to table, or why not even in other ways.

Although Mobility has various backend strategies, it provides a unified querying API, e.g. thought.quote "just works" and shields us from worrying about "implementation details" regarding how the translation is actually stored. This is why I can simply rely on Field::Text in the dashboard to display and edit the attribute. Unfortunately this assumption no longer holds when performing a search.

Exact-match searches can be done with Thought.i18n.where(quote: "blah") and partial case-insensitive matches with Arel predicates: Thought.i18n { quote.matches("%thought%") }. Mobility also has an integration with Ransack allowing for queries like Thought.ransack(quote_cont: 'foo').result. An important improvement would be having real full-text search (e.g. using pg_search or searchkick) where fuzzy matches are returned by order or decreasing relevance. Maybe it would be too much for Administrate to do all of this out of the box, but making it possible by plugging in an API would be great. Or can this somehow already be done by overriding some methods?

(*) As in the main app, the admin routes are scoped to set the proper I18n.locale, and there is a language switcher.

pablobm commented 3 years ago

This is going to be a tricky one. Administrate's search currently makes some assumptions that don't hold well here. Ideally we would want to create a custom field type (eg: Field::MobilityI18nText) that described how to search it. However this is not currently supported, and it's not trivial to add.

An alternative would be creating your own search, instead of using Administrate's default one. Remove Administrate's search field, use your own one, send queries to a custom action, handle it, and render the result with an Administrate index view. I think that should work, but I haven't tried.

sedubois commented 3 years ago

@pablobm OK below is a custom solution to get ILIKE search working for translated models.

# app/controllers/admin/application_controller.rb
module Admin
  class ApplicationController < Administrate::ApplicationController
    #...

    def index
      # copied from superclass, with following changes:
+     return super unless resource_class < Translatable
      search_term = params[:search].to_s.strip
-     resources = Administrate::Search.new(scoped_resource,
-                                          dashboard_class,
-                                          search_term).run
+     resources = search_term.blank? ? scoped_resource.all : i18n_search(search_term)
      resources = apply_collection_includes(resources)
      resources = order.apply(resources)
      resources = resources.page(params[:page]).per(records_per_page)
      page = Administrate::Page::Collection.new(dashboard, order: order)

      render locals: {
        resources: resources,
        search_term: search_term,
        page: page,
        show_search_bar: show_search_bar?,
      }
    end

    #...
    def i18n_search(search_term)
      search_term = sanitize_sql_like(search_term)
      attributes = dashboard_class::ATTRIBUTE_TYPES.select { |_, field| field.searchable? }

      resource_class.i18n do
        def match((attribute, field), search_term)
          # TODO add API to fields to allow matchers for other column types, e.g. `.eq(search_term)`
          instance_eval(attribute.to_s).matches("%#{search_term}%")
        end

        attributes.reduce(nil) do |memo, nxt|
          memo.nil? ? match(nxt, search_term) : memo.or(match(nxt, search_term))
        end
      end
    end
  end
end

My translatable models include a Translatable module:

```rb module Translatable extend ActiveSupport::Concern included do extend Mobility default_scope { i18n } # various other useful scopes defined as well # ... end end ```

Actually the method is called i18n_search(), but except for the scope .i18n coming from Mobility it is quite generic (and I already apply it using a default_scope). I just don't know how to replace that scope to perform the Arel query on the class' default scope, so that it would also work in non-translated models. Any idea how to make that generic?

The great thing with the solution above is that thanks to Mobility's API it should work no matter how the translation data is stored (in a key-value table, in a JSON column, etc).

Anyway at least now I have something working for my app. Next steps could be to retrofit something like this in Administrate if you deem it worthwhile, and also play with having more proper full-text search with pg_search or searchkick.

sedubois commented 3 years ago

@shioyama any idea about this question above?

Actually the method is called i18n_search(), but except for the scope .i18n coming from Mobility it is quite generic (and I already apply it using a default_scope). I just don't know how to replace that scope to perform the Arel query on the class' default scope, so that it would also work in non-translated models. Any idea how to make that generic?

shioyama commented 3 years ago

Hmm I don't know administrate at all, so it's going to be hard to say very much. Under the hood the i18n scope in Mobility is doing two things, creating an Arel node for each translated attribute:

backend_class = Post.mobility_backend_class(:title)
node = backend_class.build_node(:title, :en)

You can do things like node.eq('foo') etc like any other Arel node.

Once you have the node, you the build a relation by applying the backend scope to a relation (like Post.all):

relation = backend_class.apply_scope(Post.all, node, :en)

You can then call relation.all, relation.first, relation.to_sql etc. (This second step is necessary for some backends like Table and KeyValue to join translations, which can't be done at the arel node level.)

That's pretty complicated which is why Mobility gives you the ability to just pass a block to the query scope like this:

Post.i18n do
  title.eq('foo')
end

The title in the block resolves to the node above.

If you need to do something tricky, you might need to actually build up the relation using the steps above.

sedubois commented 3 years ago

Thanks @shioyama for taking the time to reply. My question wasn't clear. In a plain Rails project (where Mobility or Administrate for that matter are not involved at all), how to make the below query work? I just don't know how to navigate the ActiveRecord/Mobility codebase to find the answer myself.

Post.??? { title.matches("%foo%") }

Supposing that such an API is available in plain ActiveRecord, using it in Administrate would allow to make queries that would "just work", regardless of whether the model is plain vanilla Rails or is translated with Mobility (or supposedly, something else).

shioyama commented 3 years ago

Hmm, so actually you made me realize that some things in the query plugin that are currently private should actually be exposed.

But to answer your question, there is no ??? which will work AFAIK in Rails, since Rails does not support block-format query. Mobility custom implements it using a class called Mobility::Plugins::ActiveRecord::Query::VirtualRow, which is actually private but I will make it public in the next release.

You can do this currently but it's going to be quite ugly:

Mobility::Plugins::ActiveRecord::Query.const_get(:VirtualRow).build_query(resource_class, Mobility.locale) do
  title.matches("%foo%")
end

I think that might do what you want. I will make the VirtualRow class public in the next release so you don't need that const_get (which gets around private_constant).

shioyama commented 3 years ago

Ok I created https://github.com/shioyama/mobility/pull/471 which will make it slightly easier to do this:

Mobility::Plugins::ActiveRecord::Query.build_query(resource_class) do
  title.matches("%foo%")
end
sedubois commented 3 years ago

there is no ??? which will work AFAIK in Rails, since Rails does not support block-format query

@shioyama Do you think a case could be made to request adding this in Rails? If there is a reason to refuse it, then the flip-side of that question would be, how to make queries in Mobility which match "the Rails way"? Or would you recommend I just give up on this idea of having the same API in both?

shioyama commented 3 years ago

Do you think a case could be made to request adding this in Rails?

This is basically what Wharel does, and I believe there is a PR somewhere to make Rails do it as well (might have been merged actually).

sedubois commented 3 years ago

You are possibly referring to this PR, but it seems far from ready for merge, and would take a long time before being available in all supported Rails versions unfortunately.