Skip to content

Conversation

kamipo
Copy link
Member

@kamipo kamipo commented Aug 26, 2020

If a table is joined multiple times, those tables are aliased other than
the first one.

It happens easily on self referential associations, and in that case
currently there is no way to work custom attribute (type casting) and
attribute alias resolution for aliased tables in where conditions.

To address the issue, it will allow where references association names
as table aliases. If association names are referenced in where, those
names are used for joined table alias names.

class Comment < ActiveRecord::Base
  enum label: [:default, :child]
  has_many :children, class_name: "Comment", foreign_key: :parent_id
end

# ... FROM comments LEFT OUTER JOIN comments children ON ... WHERE children.label = 1
Comment.includes(:children).where("children.label": "child")

Fixes #39727.

If a table is joined multiple times, those tables are aliased other than
the first one.

It happens easily on self referential associations, and in that case
currently there is no way to work custom attribute (type casting) and
attribute alias resolution for aliased tables in `where` conditions.

To address the issue, it will allow `where` references association names
as table aliases. If association names are referenced in `where`, those
names are used for joined table alias names.

```ruby
class Comment < ActiveRecord::Base
  enum label: [:default, :child]
  has_many :children, class_name: "Comment", foreign_key: :parent_id
end

# ... FROM comments LEFT OUTER JOIN comments children ON ... WHERE children.label = 1
Comment.includes(:children).where("children.label": "child")
```

Fixes rails#39727.
@kamipo kamipo merged commit 57da1d0 into rails:master Aug 31, 2020
@kamipo kamipo deleted the where_references_association_name branch August 31, 2020 02:46
@simi
Copy link
Contributor

simi commented Aug 31, 2020

🤔 I think I do understand the original problem in here, but are you sure that aliasing part is not breaking change and if so, it is worth it? Also why not to alias everytime to keep the where API the same aka Comment.includes(:children).where(children: {"label": "child")? Sure, that would be definitely breaking change.

What are we looking here for is finally way to specify in where API if we would like to let rails resolve the table name or we would like to provide it on our own. symbol (rails resolves table/column name) vs string (let it be, just pass it to query) would be IMHO great way to do that, but we should stick that under configuration flag since that would be enormous breaking change, but still IMHO good default for new apps. I can try to start this discussion at community board.

PS: Also this seems to me similar kind of change in where API to #39613 which was (sadly) reverted.

@kamipo
Copy link
Member Author

kamipo commented Sep 1, 2020

🤔 I think I do understand the original problem in here, but are you sure that aliasing part is not breaking change and if so, it is worth it? Also why not to alias everytime to keep the where API the same aka Comment.includes(:children).where(children: {"label": "child")? Sure, that would be definitely breaking change.

I'd not like to break existing apps, referencing original alias candidate name still should be valid call if people don't rely on custom type casting.

e.g.

# ... FROM comments LEFT OUTER JOIN comments children_comments ON ... WHERE children_comments.label = 1
Comment.includes(:children).where("children_comments.label": Comment.labels[:child])

@KapilSachdev
Copy link
Contributor

I think a CHANGELOG entry would be great as this is a new behaviour.
I'm not sure if documentation needs to be added or not, or where in case yes, to help discover this aliasing behaviour.

kamipo added a commit that referenced this pull request Sep 1, 2020
@kamipo
Copy link
Member Author

kamipo commented Sep 1, 2020

👍
Added a CHANGELOG entry 7b9ca16.

@simi
Copy link
Contributor

simi commented Sep 1, 2020

@kamipo as I mentioned, IMHO this added alias already breaks compatibility in some cases. Also change in where "hash" keys logic should be probably better discussed (as asked in #39613). I think there's much more we can do if we decide to do changes in where "hash" keys handling, but adding this will make it harder in the future since we will need to support this behaviour as well.

@tenderlove as one requesting better discussion in #39613 WDYT?

@simi
Copy link
Contributor

simi commented Sep 2, 2020

@kamipo looking at this again, wouldn't be better to allow aliasing for *joins methods with some kind of API - joins(:children, alias: {comment: :children} and just use it internally for this case ? This solution seems like a common bugfix, but actually it is big (at least in my eyes) change in AR API and overall behaviour.

kamipo added a commit to kamipo/rails that referenced this pull request Dec 4, 2020
As the API doc shows, `references` should be given table names.

https://api.rubyonrails.org/classes/ActiveRecord/QueryMethods.html#method-i-references

However, people (and test cases in the our codebase) sometimes give
association names for `references`.

Since the API example and all test cases in the codebase use symbols as
arguments, so I regarded symbols as user given values in rails#40106.

But as rails#40743 indicates, people also use string association names, it
would have unexpected side-effect especially if singular association
names given.

As much as possible to avoid to use user given values, mark internal
references for `where` and use only them for join aliases.

Fixes rails#40743.
alpaca-tc added a commit to alpaca-tc/rails that referenced this pull request Feb 9, 2021
related: rails#39465, rails#40106

Fixed the attributes couldn't cast type in the where clause
when the grandchild table is aliased.

PR rails#40106 allows aliased table in where clause, but lookup
of alias grandchild table isn't working.
@jon-sully
Copy link

Hey @kamipo — I just concluded a few hours of spelunking to figure out why a default scope on one of my models wasn't following the table aliasing that this PR introduced... long story short the default scope was set in part by calling arel_table which hard-coded the table name as the "true" table name rather than the alias. A fun find for sure.

Anyway, in the process I learned lots more about this particular feature-set and outside of my scoping breaking it partially, it is so cool. Being able to specify associations in the .where clause after so long of that not being an option is fantastic. Doing it by not adjusting the WHERE clause but instead aliasing the join table to match the association name is super clever. Absolutely love it.

I know there've been a number of issues popping up here from folks getting into trouble with it all, so I just wanted to post back to you and say thank you for developing this. I think it's a super neat feature and really appreciate it. 🙂

@sholden
Copy link
Contributor

sholden commented Apr 5, 2021

Hi @jon-sully were you able to figure out your issue with default scopes? I just got to this pull while debugging something that sounds like it may be what you hit. I have a default scope on a model, and Arel is trying to merge that default disabled = FALSE condition with the foreign key join condition, and then add a subsequent where clause against the joined table.

The foreign key equality condition and the subsequent where clause both correctly use the join alias, but the default scope condition tries to reference the original table name, generating invalid SQL.

Does this sound familiar? If so, did you resolve it?

@jon-sully
Copy link

Hey @sholden 👋🏻

My context was a little special — an older project I'm working on is using permanent_records which gives the class scope not_deleted but doesn't make it a default_scope for me, I was doing that myself. We can see that library's implementation is

where arel_table[:deleted_at].eq(nil)

So where I was doing

default_scope { not_deleted }

It would fail — for the reasons we both mentioned. I instead just made my own default scope that does basically the some thing but without all the arel-level (unnecessary?) fanciness 😛. Truly this simple for me:

default_scope { where deleted_at: nil }

And that worked perfectly.

If you're seeing a JOIN statement with multiple ONs where one of those ON/ANDs isn't using the alias name, I'd presume that something along the way is calling the model's arel_table. You may have to monkey patch that thing if it's forcing your default_scope or override your own. Otherwise you can always write your own custom join as a string.. but that's not the most Rails thing ever 😅

Hope that helps!

kamipo added a commit to kamipo/rails that referenced this pull request Apr 9, 2021
If a model which has a user-defined `self.default_scope` is joined with
table alias, a user-defined `self.default_scope` ignores table alias.

This problem has potentially existed for a long time, but it has not
become apparent because it is difficult to meet this condition.

Since rails#40106, table alias is easily used if association names are used
in `where`.

So user-defined `self.default_scope` should be evaluated in the current
aliased relation.

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

Successfully merging this pull request may close these issues.

Conditions on nested joins associations break automatic type conversion and cause schema queries
5 participants