Skip to content

Implement select/group in AR query plugin#285

Merged
shioyama merged 1 commit intomasterfrom
select
Sep 28, 2018
Merged

Implement select/group in AR query plugin#285
shioyama merged 1 commit intomasterfrom
select

Conversation

@shioyama
Copy link
Copy Markdown
Owner

Fixes #275

@shioyama
Copy link
Copy Markdown
Owner Author

Note: select is quite tricky, because in ActiveRecord attribute methods on the results of a select query are handled by method_misisng in a sparsely-documented feature dating back all the way to the early days of Rails.

The tricky part is that Mobility has already defined these accessor methods, so it can't easily delegate to method missing like AR does internally in this case. I'm not yet sure how to handle this.

Most stuff works simply by using the "as " format, where attribute name here is the name of the translated attribute. But the accessors (i.e. post.title) do not, for the reason above.


def as(*)
super
.extend(::Arel::Expressions)
Copy link
Copy Markdown
Owner Author

@shioyama shioyama Sep 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is necessary due to this line. We might not need it though.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems we do need it.

@shioyama shioyama force-pushed the select branch 2 times, most recently from d08348d to b8d9cbd Compare September 26, 2018 04:52
# TODO: Improve this.
def read(locale, **)
attr_set = model.instance_variable_get(:@attributes)
if attr_set && attr_set.key?(attribute)
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not work right now, although it may pass tests. Aside from being an ugly hack, the problem is that @attributes may have a value for a key that is a translated attribute, but the value may not be the one we want to return here.

E.g. with the json backend, if your column name is the same as the name of the translated attribute, then you will have e.g. a value of {} for title (empty json hash). This patch to read here would return this value instead of nil (no translation for the current locale), which is obviously wrong.

I'm pretty stuck with this one. AR sets the value of the SELECT using AS in the corresponding key of @attributes, but this may be set for other reasons (as mentioned above) so its presence is not a reliable indicator that we should actually return it.

We could use a different name in the AS clause, which would make it unambiguous, but this would make combining with group and other things not work (I think)...

# TODO: Improve this.
def read(locale, **)
attr_set = model.instance_variable_get(:@attributes)
if attr_set && attr_set.key?(attr_alias = Query.attribute_alias(attribute))
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've switched to using an alias when selecting, so SELECT ... AS __mobility_title__. This avoids conflicts with other stuff in ActiveRecord, but still passes grouping specs.

I still don't like the instance_variable_get here...

@shioyama shioyama force-pushed the select branch 4 times, most recently from 4073bc7 to a66ddd7 Compare September 26, 2018 13:25
@shioyama shioyama changed the title [WIP] Implement select/group in AR query plugin Implement select/group in AR query plugin Sep 27, 2018
@shioyama shioyama force-pushed the select branch 2 times, most recently from 95d2bcf to 6621ecb Compare September 28, 2018 01:03
@shioyama shioyama merged commit 71346ac into master Sep 28, 2018
@shioyama shioyama temporarily deployed to github-pages September 28, 2018 04:32 Inactive
@shioyama shioyama deleted the select branch October 1, 2018 00:26
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

Successfully merging this pull request may close these issues.

1 participant