Allows using relation name when querying joins/includes #13555

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
Contributor

danmcclain commented Dec 31, 2013

When joining or including records, when querying the relation, you can use either the table name or the relation name:

Example:

Author.joins(:posts_with_comments).where(posts: { id: 1})
# or
Author.joins(:posts_with_comments).where(posts_with_comments: { id: 1})
Contributor

kbrock commented Jan 7, 2014

This is nice.

  1. Will this work if there are multiple relations on a given table? (think it will)
  2. if you rebase to master, I think the build failure will clear up.
Contributor

danmcclain commented Jan 7, 2014

@kbrock The only place that it would think of that I can think of is where you join/include the 2 relations with the same table, then this would not work as expected. It should resolve out all relations used in the include/join otherwise.

Also, rebased to see if that resolves the failures

Contributor

kbrock commented Jan 7, 2014

@danmcclain Yea, that is what I thought / sounds good to me

railties seems to be a bit finicky in their builds :(.

Contributor

danmcclain commented Jan 31, 2014

@rafaelfranca Any thoughts on this?

Owner

rafaelfranca commented Jan 31, 2014

I'm neutral about this one. It seems good but It doesn't change too much to me. I'll merge if nobody has something against but we'll have to wait the 4.1 be released because I'm focused on this right now.

Contributor

danmcclain commented Jan 31, 2014

That's fine, just wanted to get some visibility on it to see if it's a reasonable commit. Thanks @rafaelfranca

@danmcclain danmcclain referenced this pull request in interexchange/postgres_ext Feb 2, 2014

@dirkkelly dirkkelly Created failing spec for inferred table name eae9144

Hi,
this would come in really handy for me -- I'm using a custom query object and am dynamically building activerecord queries from the stored model. This would solve one of my problems.

@danmcclain : just to clarify it, if I have a model Comment that references User two times, i.e.

class Comment < ...
     belongs_to :written_by, class_name: User
     belongs_to :aproved_by, class_name: User
end

I cannot do a:

  Comment.joins(:written_by).joins(:aproved_by).where(written_by: { username: 'user1' }).where(aproved_by: { username: 'user2' })?
Contributor

danmcclain commented May 13, 2014

@andreashappe This won't solve your case with the 2 different relations relating to the same object. We'd have to alias the joins, and use the alias when executing the where clause

@rafaelfranca Is this still a possible feature? I can rebase if it is

@danmcclain: thank you for the clarification. Do you by-chance have some hints howto do the Aliasing? I've looked into Arel, ActiveRecord::Associations::JoinDependency, etc. but haven't found a "real" (or "clean") solution for this problem.

(I know that this is OT, but I found this bug report while searching for a solution for that problem and maybe this helps the next user)

Contributor

danmcclain commented May 13, 2014

@andreashappe Are you using postgresql? If so, I have some resources for you.

@danmcclain : yes, postgres 9.1 or 9.3!

Contributor

kbrock commented May 13, 2014

@danmcclain
Fun! I tried to use the relation name in 4.0 and was confused that it wasn't working.
Just ran into this thread again and remembered why.

@tenderlove tenderlove commented on the diff May 13, 2014

...ecord/lib/active_record/relation/predicate_builder.rb
@@ -15,6 +15,16 @@ def self.resolve_column_aliases(klass, hash)
hash
end
+ def self.resolve_relation_names(klass, relation, hash)
+ joins_includes_values = relation.joins_values + relation.includes_values
+ hash.keys.grep(Symbol) do |key|
+ if joins_includes_values.include?(key) && association = klass.reflect_on_association(key)
+ hash[association.table_name.to_sym] = hash.delete key
+ end
+ end
+ hash
@tenderlove

tenderlove May 13, 2014

Owner

Can you make this return a new hash and not modify the existing one? I'd rather we don't mutate the parameters.

@danmcclain danmcclain Allows using relation name when querying joins/includes
When joining or including records, when querying the relation, you
can use either the table name or the relation name:

Example:
    Author.joins(:posts_with_comments).where(posts: { id: 1})
    # or
    Author.joins(:posts_with_comments).where(posts_with_comments: { id: 1})
bf8ac39
Contributor

danmcclain commented May 14, 2014

@tenderlove Rebase and updated: hash = hash.dup to avoid mutating params 👍

Owner

tenderlove commented May 14, 2014

I'm kinda worried about this. I like the feature, but I'm worried about relation names clashing with table names. Correct me if I'm wrong, but today we'll accept a symbol as a parameter, but just treat it as a table name. What do we do if there's a relation that has scopes or whatever associated with it? Won't this change behavior for people who were expecting it to be "just a table"?

Maybe we could introduce a new method?

That said, maybe I'm being too paranoid.

Owner

matthewd commented May 14, 2014

@tenderlove I think the "just a table" bit is okay, because whichever spelling you use, you're still referring to a relation (in the SQL sense) that has to have been JOINed in to the query somehow.

That said, this cannot fly as is, in my view:

Author.includes(:posts, :posts_with_comments).where(posts_with_comments: { id: 10 })

It succeeds, while referencing a different SQL relation from the one that I explicitly named. And actually, the same is true for the previously-mentioned written_by/approved_by example.


I also dislike giving special treatment for direct associations... this works:

Posts.includes(:editor).where(editor: { name: 'Bob' })

but instead of being able to use:

Posts.includes(:editor => :employer).where(editor: { employer: { name: 'ACME' } })

I need:

Posts.includes(:editor => :employer).where(companies: { name: 'ACME' })
Owner

matthewd commented May 14, 2014

To be clear, I would love to be able to refer to things by the association name... but if we're going to, I think we should handle nested association references too. And we clearly must deal with aliases properly, rather than blindly translating back to the table name... without that, I think we'd make it much easier to compose a correct-sounding-yet-very-wrong query (and it's already not a big challenge 😐)

@tenderlove: what i am seeing is, that people are confused that the underlying table Name (and not the activerecord relationship name) is used -- people i'm working with usually asume that they can use the relationship name (which matches the table name most of the time). This would make "dynamic" constructed queries easier to construct (which would help me (; )

@danmcclain: do you believe that it would be possible to move this into a new method (and in a gem altogether)? I don't know if it is even possible (rails-community wise) to change the where clause without breaking existing installations, hope this is not a showstopper :-/

@danmcclain: thank you for the CTE information. What do you think about adding alias support to active_records join semantic? Instead of "select * from comments inner join users on users.id = comments.written_by_id" -> "select * from comments inner join users as written_by on written_by.id = comments.written_by_id". If doing this, this would automatically fall in with the other solution (note: that just adding the "as #{relation_name}" wouldn't be enough as the table alias must be used (at least for postgres) instead of the table name in the join clause.

Contributor

danmcclain commented May 15, 2014

@tenderlove This still operates in the 'just a table' way, it is just smarter in finding the table name. It does not apply the underlying scopes.

So this operates the same as including/joining 2 relations with the same underlying table the old way, as in @matthewd's example:

Author.includes(:posts, :posts_with_comments).where(posts: { id: 10 })
# And
Author.includes(:posts, :posts_with_comments).where(posts_with_comments: { id: 10 })
# execute the same SQL

If both posts and posts_with_comments have the same underlying table, they operate on the same table. This PR does not change that behavior.

@andreashappe I think without a new method/gem, auto-aliasing would be a breaking change. I would be happy to work on this auto-aliasing version, if it would be a desirable feature in the future. In that way too, we could potentially apply the underlying scopes as subqueries. It's do-able, but does it make sense to do it? Are there enough people who want it?

@tenderlove Any thoughts on the alias-ing version?

Owner

matthewd commented May 15, 2014

@danmcclain the only breaking change I can see is where there's an association, with the same name as a table, where the association has a different source table.

If the association uses the same-named table, either interpretation is equal. And any association name that doesn't match a table name would previously have been an error, so no back-compat issue there.

And for that narrow case, we can detect it, and manage a transition: initially, we continue interpreting the name to mean the table, while outputting a deprecation warning... later, assuming we consider the association-name interpretation to be superior (I certainly do), we can swap it over.

@rafaelfranca rafaelfranca modified the milestone: 4.2.0, 5.0.0 Aug 18, 2014

danmcclain closed this Apr 3, 2015

@rafaelfranca rafaelfranca modified the milestone: 5.0.0 [temp], 5.0.0 Dec 30, 2015

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