Make OR more permissive, let folks write wrong queries if they want to

[robmathews]

Summary

I was trying to port some old code that used or_chain, and wanted to use ActiveRecord::Relation#or instead. However, I quickly ran aground, because whereas or_chain doesn't try to protect me from stupidity, the new ActiveRecord::Relation#or code does.

The real world use case was I have a UI that lets a user select from a bunch of named scopes, which I then join together like this:

query=scopes.map {|scope_name|MyClass.send scope_name}.
           inject {|query,scope_query|query.or(scope_query)}

Some of those scopes eager_load associations and have restrictions on them, and currently that's not allowed.

The patch simply removes some of the restrictions on joining and including other tables, and lets me make my own mistakes. I know that it can generate unintentionally valid but wrong sql, but that's so much better than using "#to_sql" and stitching the SQL together manually.

Here's a gist about spammy cat videos showing before and after the change, with failing/passing tests.

Other Information

There was a discussion over here about it, although that was more of a complaint about the docs.

[rails-bot]

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @matthewd (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[matthewd]

The aim isn't to protect you from stupidity, but rather to protect us from ambiguity.

The restrictiveness is a design decision to avoid locking us into a dumb/unconsidered combination of the relations' structures, in the hope we'll be able to handle some of them more intelligently in the future.

If you really want to live dangerously, the actual implementation, while involving private API, isn't particularly complicated.

The proposed patch would simply ignore the joins/references/eager_loads values from the RHS relation entirely... presumably a more useful "just have a go" would attempt to merge them... which could potentially render the [mutilated] relations compatible anyway.

[robmathews]

Well, kudos for not attempting to protect me from stupidity, that would be a Sisyphean task. ;)

Still, just closing like this doesn't leave my code base with a way forward into Rails 5.0. I'm depending on being able to combine queries with .or, and the current implementation is so limited as to be nearly useless. Are there plans afoot to make it more useful?

Thanks,
Rob.

[matthewd]

I pointed out a means for you to implement behaviour closer to what you're requesting, using minimal private API, down to the line you need to copy. If that's "just closing like this", I'm sorry, but I don't think I can offer much more: 5.0 is definitely not going to grow more permissive, and any future plans are plans only.

While this resolution might mean your application can't switch to the new functionality in 5.0, if you'd found a way to make your application work on 4.2, presumably the same (or corresponding) implementation should work just as well on the newer release.

[robmathews]

Hi Matthew, thanks for taking the time to answer.

hmm ... how does knowing which commit implemented .or would help me change it's behaviour, unless I monkey-patch Rails, which I don't want to do, for obvious reasons?

The way that I've got this working on 4.2.X is by using a backport of .or, a gem called where-or. They did accept the same patch that was just closed here. Of course, that's not the rails core team, I understand, different standards.

I'd like to make one little comment about "The proposed patch would simply ignore the joins/references/eager_loads values from the RHS relation entirely".

For eager loading, it might be quite reasonable to ignore the RHS eager load values. I say that because in practice, we are finding that having scopes eager-load tables to be problematic for performance as we assemble a larger query from those scopes. We'd rather nobody eager-loaded, and then insert eager-loading as needed on the controller paths as load tests tell us we need it. I know that's not a general statement, but I thought I'd put it out there as a practical matter.

Thanks for your time.