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

Rails 6 RC2 joins / arel regression #36761

Closed
glebm opened this issue Jul 25, 2019 · 20 comments · Fixed by #36805
Closed

Rails 6 RC2 joins / arel regression #36761

glebm opened this issue Jul 25, 2019 · 20 comments · Fixed by #36805
Assignees
Milestone

Comments

@glebm
Copy link
Contributor

glebm commented Jul 25, 2019

We have a query that works with all versions of Rails from 4 to 6 RC1:

https://github.com/thredded/thredded/blob/c691be6c9b4b25dea291ea4cfe7ea9b0de773c89/app/models/thredded/messageboard.rb#L131-L138

In Rails 6.2 RC2 the very first join (joins(:topics)) is no longer produced (INNER JOIN "thredded_topics" ...), resulting in an invalid SQL query.

Originally reported in thredded/thredded#822.
This was reported for PostgreSQL, not sure if other databases are also affected.

@rafaelfranca
Copy link
Member

I believe we could fix this issue, but arel is private API of the framework and there is no guarantee that query will always work.

@rafaelfranca rafaelfranca added this to the 6.0.0 milestone Jul 25, 2019
@rafaelfranca
Copy link
Member

@kamipo any idea of which change would cause this?

@kamipo
Copy link
Member

kamipo commented Jul 25, 2019

Allowing Arel node for joins is to apply implicit through table joins.
(e.g. joining "posts" table for author -> (though posts) -> comments association)

scope.joins!(join(foreign_table, constraint))

Before #36304, Arel node (i.e. implicit through table) joins are always applied after association joins (e.g. joins(:comments)), it is not a matter for INNER JOIN somehow, but a problem for LEFT JOIN #36293.

What people want is that user supplied joins order is completely preserved (#36558, #34328, #24281, #12953, and more).
But for now, at least as long as using Arel which is private API directly, there is no guarantee that joins order is preserved, as @rafaelfranca said.

@glebm
Copy link
Contributor Author

glebm commented Jul 25, 2019

It's not the order, the join is completely missing now, which sounds like a bug? Ah, nevermind, the join is there but it is now last.

glebm added a commit to thredded/thredded that referenced this issue Jul 25, 2019
Rails 6.2.0.rc2 changed join order when combining Arel and Rails joins.
Worked around by using only Arel joins in the affected query.
See rails/rails#36761

Fixes #822
@glebm
Copy link
Contributor Author

glebm commented Jul 25, 2019

Is a fix feasible? Arel is a private API but is widely used (it used to be public and there is no built-in alternative).

glebm added a commit to thredded/thredded that referenced this issue Jul 25, 2019
Rails 6.2.0.rc2 changed join order when combining Arel and Rails joins.
Worked around by using only Arel joins in the affected query.
See rails/rails#36761

Fixes #822
@rafaelfranca
Copy link
Member

it used to be public

It was never public.

and there is no built-in alternative

Built-in alternative are strings.

Is a fix feasible?

Maybe, I'll let @kamipo decide.

@glebm
Copy link
Contributor Author

glebm commented Jul 25, 2019

Built-in alternative are strings.

Strings are not composable, parameterizable, or cross-DB compatible (important for library authors).

It was never public.

It wasn't tagged as a private API in the rubydoc, the methods were documented with examples, and the Arel gem itself was available standalone.

Lots of people thought (and probably still think) that it's public (e.g. https://thoughtbot.com/blog/using-arel-to-compose-sql-queries#arel).

This is what docs looked like:

image

@rafaelfranca
Copy link
Member

That is not the official Rails documentation. You will not be able to find arel_table documented in our documentation which is what define what is public api or not https://api.rubyonrails.org/v5.2.3/

glebm added a commit to thredded/thredded that referenced this issue Jul 25, 2019
Rails 6.2.0.rc2 changed join order when combining Arel and Rails joins.
Worked around by using only Arel joins in the affected query.
See rails/rails#36761

Fixes #822
@glebm
Copy link
Contributor Author

glebm commented Jul 25, 2019

Yeah, I'm not arguing that it was ever officially public, just highlighting why people thought that it was (this is why lots of code uses Arel).

glebm added a commit to thredded/thredded that referenced this issue Jul 25, 2019
Rails 6.2.0.rc2 changed join order when combining Arel and Rails joins.
Worked around by using only Arel joins in the affected query.
See rails/rails#36761

Fixes #822
@glebm
Copy link
Contributor Author

glebm commented Jul 25, 2019

@kamipo Working around this is gnarly: thredded/thredded@67630c1

@rafaelfranca
Copy link
Member

I think we should try to fix if we can, but maybe Kamipo has a better idea of the cost of fixing this.

glebm added a commit to thredded/thredded that referenced this issue Jul 25, 2019
Rails 6.2.0.rc2 changed join order when combining Arel and Rails joins.
Worked around by using only Arel joins in the affected query.
See rails/rails#36761

Fixes #822
glebm added a commit to thredded/thredded that referenced this issue Jul 25, 2019
Rails 6.2.0.rc2 changed join order when combining Arel and Rails joins.
Worked around by using only Arel joins in the affected query.
See rails/rails#36761

Fixes #822
@inopinatus
Copy link
Contributor

inopinatus commented Jul 26, 2019

It was public, and it was intended as an ORM builder. It boils my head that the Rails core team not only co-opted Arel, decided it was wizards-only and should be locked away in the tower, and is now lying about its historical status. Do you think we can't read the old README?

It's totally against the spirit of the multi-story model of software that Yehuda Katz and Ernie Miller spoke about at RailsConf 2014, talks that Sean Griffin then cited as inspiration for the Attributes API.

Some things can only be composably/neatly achieved with Arel and this holier-than-thou line so often parroted about it being a "private API" is just a load of elitist, revisionist claptrap. Arel is incredibly useful at times and is sometimes the single most impressive component of Rails. The advice should be that it is powerful, dangerous, that all Arel-based expressions should have tests, and that breaking changes may occur, but not that it is somehow an untouchable, unknowable, unspeakable wizards-only secret.

@rafaelfranca
Copy link
Member

I'm sorry, but Arel never was public API of the framework. it was a public gem indeed, but it was never exposed by Rails to the users to be used in Rails applications. But, you can believe in whatever you want. The fact is that Arel is not part of public API of Rails and places where it worked before can stop work without notice.

While I can see your point about it seeming to be an untouchable, unknowable, unspeakable wizards-only secret, when we say Arel is private API of the framework we are basically saying that we don't support its usage in application and issues on its direct usage will be likely closed.

We do have plans to expose it in the future though, in a different way and them we are going to support its usage. Right now it is not the reality and never was.

But like I said, I'm fine to fix this issue if @kamipo thinks it will not have a high cost in the implementation.

@inopinatus
Copy link
Contributor

That is splitting a hair finer than I can discern the difference. If Arel wasn't part of the public API of Rails, it is because it wasn't part of Rails, clearly advertising itself as a standalone component, upon which Rails just happened to depend, right up until the point it was absorbed into the bowels of ActiveRecord and vanished.

As for "never", I only had to go back to Rails 4.x to find public API methods that explicitly accept and return Arel objects, and there are still references to Arel in Rails 6.1 public docs.

One may as well suggest that GlobalID isn't part of the Rails API either, it has even fewer mentions in public and is also a separate gem.

I know that merging Arel into rails/rails made a lot of sense for maintainers, but for advanced users it was more like a Central Committee suddenly disappearing an old friend for being too difficult and too powerful. I could only file that in the drawer of Very Unhappy Outcomes. It also added barriers to being an occasional third-party contributor to Arel because I have to extract and build the docs myself now.

If there are plans to expose it in the future, please link them, so we can all contribute. I really think the multi-storey concept of software is super applicable here.

glebm added a commit to thredded/thredded that referenced this issue Jul 26, 2019
Rails 6.2.0.rc2 changed join order when combining Arel and Rails joins.
Worked around by using only Arel joins in the affected query.
See rails/rails#36761

Fixes #822
glebm added a commit to thredded/thredded that referenced this issue Jul 26, 2019
Rails 6.2.0.rc2 changed join order when combining Arel and Rails joins.
Worked around by using only Arel joins in the affected query.
See rails/rails#36761

Fixes #822
@legendetm
Copy link

Maybe it's then time to reconsider Arel as public API, because a lot of people/companies are using it. Without Arel, there's no viable alternatives to building complicated composable and parameterizable queries with ActiveRecord. Also I believe it's safer to use Arel than strings.

In my current and in my previous company, it was forbidden to use the built-in string format, everything has to be done in Arel, what cannot be done with ActiveRecord::Relation directly.
We started to use Arel everywhere in Rails 4.X, and it has been getting better with every release (exception was 5.1)

kamipo added a commit to kamipo/rails that referenced this issue Jul 29, 2019
Currently, string joins are always applied as last joins part, and Arel
join nodes are always applied as leading joins part (since rails#36304), it
makes people struggled to preserve user supplied joins order.

To mitigate this problem, preserve the order of string joins and Arel
join nodes either before or after of association joins.

Fixes rails#36761.
Fixes rails#34328.
Fixes rails#24281.
Fixes rails#12953.
@flanger001
Copy link
Contributor

@inopinatus 👏👏👏 The fact that they locked Arel away and have tried to gaslight us into believing it was never part of the public api (arel_table was literally in the Rails Guides) continues to frustrate me so much.

Arel should be public, or ActiveRecord should expose everything Arel does, full stop.

@danielricecodes
Copy link
Contributor

Maybe it's then time to reconsider Arel as public API, because a lot of people/companies are using it. Without Arel, there's no viable alternatives to building complicated composable and parameterizable queries with ActiveRecord. Also I believe it's safer to use Arel than strings.

In my current and in my previous company, it was forbidden to use the built-in string format, everything has to be done in Arel, what cannot be done with ActiveRecord::Relation directly.
We started to use Arel everywhere in Rails 4.X, and it has been getting better with every release (exception was 5.1)

Agreed. There is no better way to do time range scopes than by using Arel.

scope :started, -> {
  where(arel_table[:valid_from].lteq(Date.current))
}

AFAIK - this is perfectly valid and even encouraged in the older Rails Guides (as previously noted above).

@inopinatus
Copy link
Contributor

@danielricecodes Recently merged was #36696 making the specific example of lteq possible using a range, as in where(valid_from: ..Date.current). However, the general point remains true, because some intervals cannot be represented as a Ruby range and need Arel or SQL.

In practice, outsiders are welcome to contribute to Arel, but not welcome to use it. My feelings on this cannot be expressed politely.

glebm added a commit to thredded/thredded that referenced this issue Aug 17, 2019
This was a work-around for:
rails/rails#36761

It is no longer necessary and won't work with Rails 6.0.0
glebm added a commit to thredded/thredded that referenced this issue Aug 17, 2019
This was a work-around for:
rails/rails#36761

It is no longer necessary and won't work with Rails 6.0.0
glebm added a commit to thredded/thredded that referenced this issue Aug 17, 2019
This version of Rails won't work with this Thredded release because
the work-around for its bug has been removed.

Refs rails/rails#36761
@basvk
Copy link
Contributor

basvk commented Sep 24, 2019

I like how we are invited to use so called non-public API's to fix this kind of shenanigans:

DEPRECATION WARNING: Dangerous query method (method whose arguments are used as raw SQL) called with non-attribute argument(s): "random()". Non-attribute arguments will be disallowed in Rails 6.1. This method should not be called with user-provided values, such as request parameters or model attributes. Known-safe values can be passed by wrapping them in Arel.sql(). (called from …)

(emphasis mine).

@rafaelfranca
Copy link
Member

That is the only method of Arel that is public API at the moment.

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

Successfully merging a pull request may close this issue.

8 participants