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

Column backend does not support pluck or select #275

Closed
wenderjean opened this issue Sep 17, 2018 · 11 comments
Closed

Column backend does not support pluck or select #275

wenderjean opened this issue Sep 17, 2018 · 11 comments

Comments

@wenderjean
Copy link

Context

I'm using column backend strategy in mobility and I can perform queries based on my translated column using find_by and even where but what intrigues me is that select or pluck does not know how to handle my column name.

I'm not considering this a problem but in fact a feature not implemented, if there is an already approach to handle this I'm sorry for the issue but I really didn't find nothing about in docs and stackoverflow.

Expected Behavior

I expect mobility to handle my column name when I use select or pluck methods.

Mobility.locale = :jp
Post.all.pluck(:title) # select title_jp from posts

Actual Behavior

Post.all.pluck(:title) # ERROR:  column "title" does not exist

Possible Fix

@shioyama
Copy link
Owner

what intrigues me is that select or pluck does not know how to handle my column name.

I was actually thinking about this just a couple hours before you posted this issue. As it turns out, this is very easy to do now that querying has been refactored into a plugin. I already added order recently which is actually slightly more complicated than methods like select or pluck.

The methods I think could be easily supported are any ones that accept an arel node, so e.g. select, pluck and group should work. Any other ones you can think of? This should take less than ten lines of code for all of them, and work for all backends.

@wenderjean
Copy link
Author

wenderjean commented Sep 17, 2018

No, in fact I'm not thinking any other AR method at the moment, I have another use case that mobility could support but I think it's another feature or even another plugin, take a look:

pg_search_scope :by_title, against: :title
pg_search_scope :by_title, against: Post.mobility_translated_attributes
# Post.mobility_translated_attributes returns [:title_en, :title_jp]

@wenderjean
Copy link
Author

@shioyama can i do this PR based on your previous one (#261) or are you already working on that?

@shioyama
Copy link
Owner

shioyama commented Sep 18, 2018

I just pushed what I have to the add_new_query_methods branch, here is the commit: 9bff9b0

I lightly tested it and it seems to work, but if you want to work on it from there please go ahead! It needs a few simple specs as well.

@shioyama
Copy link
Owner

Here's the code change, to discuss:

%w[select pluck group].each do |method_name|
  define_method method_name do |*attrs|
    i18n_keys, keys = attrs.partition(&@klass.method(:mobility_attribute?))
      return super(*attrs) if i18n_keys.empty?

      base = i18n_keys.inject(self) { |query, key|
        @klass.mobility_backend_class(key).apply_scope(query, backend_node(key))
      }

      base.public_send(method_name, *(keys + i18n_keys.map(&method(:backend_node))))
    end
  end
end

As you mentioned, similar to order (#261) with the difference that pluck returns values, not a relation, so the method itself has to be called last with all the arguments.

Basically, what this does it two things:

  1. Call backend_class.apply_scope for all translated attributes (this is necessary to join translations for KeyValue and Table backends)
  2. Swap all translated attribute names for their respective Arel nodes (keys + i18n_keys.map(&method(:backend_node))

One thing I noticed is that the Table and KeyValue backends will default to using an INNER join here, which I think is not really appropriate (it will not include any results which have no translations in Mobility.locale). But this can be fixed separately in another PR.

@shioyama
Copy link
Owner

Merged #284, so this is now half-solved on the master branch.

@shioyama
Copy link
Owner

select, group and pluck are all supported now on the master branch. I'll release a new version soon.

@shioyama
Copy link
Owner

Released in 0.8.2, and supported on all backends. Let me know if you notice any issues.

@wenderjean
Copy link
Author

Sure, thanks for the support :)

@borisrorsvort
Copy link
Contributor

@shioyama seems this bug is back in mobility (1.2.9)
Capture d’écran 2022-08-18 à 11 07 02

@shioyama
Copy link
Owner

Sorry @borisrorsvort you'll have to elaborate. There are specs for pluck and select now, so basic usage is guaranteed to work. If there is a problem please provide a failing test.

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

No branches or pull requests

3 participants