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

Inconsistent results with #or in ActiveRecord::Relation with respect to documentation #24055

Closed
abhishekjain16 opened this issue Mar 4, 2016 · 26 comments
Assignees

Comments

@abhishekjain16
Copy link
Contributor

According to documentation:

# The two relations must be structurally compatible: they must be scoping the same model, and

In order to use #or Neither relation may have a #limit, #offset, or #distinct set.

But, I could use all of the above like:

>> Post.where(id: 1).distinct.or(Post.where(id: 2).distinct)
=> SELECT DISTINCT "posts".* FROM "posts" WHERE ("posts"."id" = ? OR "posts"."id" = ?)  [["id", 1], ["id", 2]]

OR

>> Post.where(id: 1).limit(1).or(Post.where(id: 2).limit(1))
=> SELECT  "posts".* FROM "posts" WHERE ("posts"."id" = ? OR "posts"."id" = ?) LIMIT ?  [["id", 1], ["id", 2], ["LIMIT", 1]]

Not sure if this is intended behaviour to support #limit, #offset, or #distinct.

If yes, then we need to improve the documentation about above use case.
If no, then should it be considered as bug?

I would be happy to look into it and send PR for either case.
But, wanted to confirm first for the intended behaviour.

@abhishekjain16 abhishekjain16 changed the title Inconsistent results with #or in ActiveRecord::Relation Inconsistent results with #or in ActiveRecord::Relation with respect to documentation Mar 4, 2016
@sikachu
Copy link
Member

sikachu commented Mar 4, 2016

I discussed this with @matthewd and I think this is a side effect that occurred from recent refactoring. I think we still want to base this per documentation, and actually stop you from able to pass in relation that has #limit, #offset, or #distinct.

@sgrif do you want to take a stab at this?

@sikachu sikachu assigned sgrif and unassigned matthewd Mar 4, 2016
@abhishekjain16
Copy link
Contributor Author

@sikachu Should #order also be included in the list as it gives similar behaviour as above(when used with same parameters in both the relation) else it gives
ArgumentError: Relation passed to #or must be structurally compatible. Incompatible values: [:order]

Just like #limit, #offset, or #distinct.

@matthewd
Copy link
Member

matthewd commented Mar 4, 2016

That's covered by "they must differ only by #where"

@abhishekjain16
Copy link
Contributor Author

But do they work when they are added to both the relations.

Not sure if documentation is complete. I think this case should be mentioned in documentation if this is supposed to work or we should stop passing in relation that has #limit, #offset, or #distinct.

@sgrif sgrif closed this as completed Mar 7, 2016
@sgrif sgrif reopened this Mar 7, 2016
@sgrif
Copy link
Contributor

sgrif commented Mar 7, 2016

I recall that we wanted to reserve the right to make it more conservative in the future

@abhishekjain16
Copy link
Contributor Author

@sgrif So we want to stop passing in relation that has #limit, #offset or #distinct when using #or right?

@robmathews
Copy link

robmathews commented Jun 23, 2016

Why can't we allow :references? I have scopes I'm trying to combine together. Couple of points:

  1. I'd like not to care about if one does eager loading and other doesn't. Load or throw out the loading, I could easily fix it up at another level, but if .or() throws an error then I'm back to the bad old days of using to_sql
  2. Some of the scopes have where clauses that reference associations, which means they cause LEFT OUTER JOINS, and cause ActiveRecord::QueryMethods#structurally_incompatible_values_for_or to complain about :references. This in turn causes or!() to raise. Thing is, it's wrong - there are no conditions in the LEFT OUTER JOINS, all the conditions are still in the .where and are correctly "OR"'d together. So the check is unnecessarily gimping "or!():"

Just changing that method like this (add :references) works:

def structurally_incompatible_values_for_or(other)
  Relation::SINGLE_VALUE_METHODS.reject { |m| send("#{m}_value") == other.send("#{m}_value") } +
    (Relation::MULTI_VALUE_METHODS - [:eager_load, :references, :extending]).reject { |m| send("#{m}_values") == other.send("#{m}_values") } +
    (Relation::CLAUSE_METHODS - [:having, :where]).reject { |m| send("#{m}_clause") == other.send("#{m}_clause") }
end

@pudiva
Copy link

pudiva commented Nov 11, 2016

+1 for :references thing. if i do

x = A.left_outer_joins(:bs, :cs)

then

x.where('bs.a_id = 1 OR cs.a_id = 1')

gives expected SQL

y = x.where('bs.a_id = 1')
z = x.where('cs.a_id = 1')
y.or(z)

gives seemingly compatible (have to test further) but different SQL and

y = x.where(bs: {a_id: 1})
z = x.where(cs: {a_id: 1})
y.or(z)

gives exception

ArgumentError: Relation passed to #or must be structurally compatible. Incompatible values: [:references]

which i believe to be bug, since y and z SQLs are ok.

@pudiva
Copy link

pudiva commented Dec 25, 2016

i just managed to work around this kinda well:

i = A.joins(:b).where(something_a: 'xxx'})
j = A.joins(:b).where(b: {something_b: 'yyy'})
A.joins(:b).where(i.constraints.last.or(j.constraints.last))

this gives me

... WHERE (`as`.`something_a` = 'xxx' OR `cs`.`something_b` = 'yyy')

idk if teh constraints i wanted to OR could be expected to be at constraints.last of i and j, but it is working for me now

@vdhpieter
Copy link

vdhpieter commented Feb 24, 2017

I was wondering what is the status of this issue? The PR that fixes the problem is rejected but this issue is still open, so is it going to be fixed or is it intended behaviour? Because I have the same problem as @ayghor but his work around isn't working for me and the only other solution I have is using raw sql but I like to avoid that as much as possible

@hopsoft
Copy link

hopsoft commented Sep 17, 2017

I was able to work around the problem using Arel with a solution based on @ayghor's example.

A.joins(:b)
  .where(
    A.arel_table[:something_a].eq('xxx').or(B.arel_table[:something_b].eq('yyy'))
  )

@rails-bot
Copy link

rails-bot bot commented Apr 6, 2018

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 5-1-stable branch or on master, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@MSCAU
Copy link

MSCAU commented Mar 2, 2019

Rails 5.2

I cannot OR two ActiveRecord::Relations which both have .limit applied to them:

Relation passed to #or must be structurally compatible

@comments = Comment.where(status: "live")
@requested_comment = Comment.where(id: requested_id)
@comments.limit(COMMENT_PAGE_SIZE - 1).or(@requested_comment.limit(1))

This is contrary to the advice provided at https://makandracards.com/alexander-m/40723-support-or-in-active-record and at https://til.hashrocket.com/posts/d3b0c7acec-rails-5-use-limit-on-activerecords-with-or.

The Guide is painfully brief on this topic:

https://guides.rubyonrails.org/active_record_querying.html#or-conditions

Is this a bug, and - if so - what's the likelihood of a fix?

@feliperaul
Copy link

@sgrif Sean, do you think someone can take a look at this? It's biting a lot of people and makes .or pretty much useless if you want the .or query to use a simple join, which is a pretty common use case. There are 57 likes in this issue alone.

@kamipo
Copy link
Member

kamipo commented Mar 11, 2019

:references is already allowed by ea61391.

@davidalee
Copy link
Contributor

davidalee commented Mar 2, 2020

Any progress on this issue?

@edipox

This comment has been minimized.

@NAR8789
Copy link

NAR8789 commented Mar 5, 2021

Hacky workaround: do all your .joins after the .or. This hides the offending .joins from the checker.

That is, the following two lines will both fail

A.joins(:b).where(b: b_query).or(A.where(query))  # error! 😞 
A.where(query).or(A.joins(:b).where(b: b_query))  # error! 😞 

but rearrange as follows, and you can evade the checker:

A.where(query).or(A.where(b: b_query)).joins(:b)  # works 😈 

This works because all the checking happens inside the .or() method. It's blissfully unaware of shennanigans on its downstream results.

One downside of course is it doesn't read as nicely.

@robmathews
Copy link

@NAR8789 lol, I love it.

@NAR8789
Copy link

NAR8789 commented Mar 9, 2021

After reading more in-depth and better-understanding some of the comments in this thread, I think the current behavior of .or is perfectly acceptable.

It took me a long while to build enough personal context to parse that out of this thread though, so here are summaries by use case. Hopefully useful to others

I want to .or together queries that include .joins on rails >= 6.1.3

I think this should just work! As @kamipo noted, ea61391 allows :references. I thiiink that means that on rails >= 6.1.3, you can .or together .joins-containing scopes, and it will just work.

I haven't tested this myself though, and the test in the referenced commit only tests with .include, not .joins. Maybe you'll still need the rails 5 workaround below:

I want to .or together queries that include .joins on rails < 6.1.3 (probably mostly for people on rails 5)

Workaround: move your .joins out to the end of the query. See earlier comment

This gets pretty unergonomic if you have .joins-containing scopes. Consider upgrading to rails 6.1.3! Alas, I am still on rails 5.2. I rewrote my scopes to take an option to disable the internal .join, and it looks absolutely horrible (but it works).

I want to .or together queries containing .limit or .distinct

Move the .limits or .distincts to the after all your .ors, similar to the workaround for .joins for rails 5

This is pretty reasonable if you think about trying to write equivalent SQL: LIMIT and DISTINCT can only apply to whole queries, not parts of queries. (unless you use UNION, but I think that's outside the intended scope of the .or operator. My mental model for .or is that it resembles SQL OR or boolean ||--it combines predicates, not result sets.)

@MSCAU 's example is illustrative here

@live_comments = Comment.where(status: "live")
@requested_comment = Comment.where(id: requested_id)
@live_comments.limit(COMMENT_PAGE_SIZE - 1).or(@requested_comment.limit(1))

Rewrite that last line like so.

@live_comments.or(@requested_comment).limit(COMMENT_PAGE_SIZE)

This works and I would argue is "prettier". (whoops, this doesn't quite work... needs more tweaking to reorder requested_comment to the top, to ensure it's within the bounds of the limit. Still useful for exploration though...)

But beauty is in the eye of the beholder--I think I find the latter expression prettier because it matches my mental model. I am thinking of the inputs to .or as predicates. I'd wager a guess MSCAU's thinks of them as collections:

More specifically, I'm guessing @MSCAU thinks of @live_comments and @requested_comment as collections, and .or as union. Under that mental model, I should be able to combine 10 @live_comments and 1 @requested_comment, and get 11 total comments.

I think .or, treats @live_comments and @requested_comment more like predicates. Predicates are properties of individual elements. They are things you might express as clauses of a .where or or a .select.

  • status: "live" is a predicate. An individual comment can be status: "live".
  • id: requested_id is a predicate. An individual comment has the requested id or it does not.
  • limit(10) can't be a predicate. It's meaningless applied to an individual comment. You can't tell looking at an individual comment whether your whole result set contains 4 comments or 7 comments or 11 comments.

Under this mental model, it's impossible to limit the subqueries on the arms of the or, because it's not something I can do by testing individual elements.

In sum, I think this makes more intuitive sense if you think about it in terms of SQL OR. It combines boolean predicates that test individual rows, not whole sets (i.e. not whole SELECTs). You can't apply LIMIT or DISTINCT to a subclause of OR.

Subcase: I read the TIL or makandracards articles @MSCAU mentioned. Why don't they work?

To the best of my understanding, those articles are just wrong. The included code doesn't work, and the underlying assumed concept is wrong. I'm guessing whoever originated them made a faulty assumption based on the "structurally compatible" bit of the error message, and then published without bothering to test.

I agree though, that the "structurally compatible" wording in the error message encourages wrong fixes. In particular, when I encountered this I also first reached towards making my subqueries "compatible" by making sure they had identical .joins clauses.

I have a legitimate case where I really do want the SQL UNION behavior

Alas, this is probably outside the scope of the .or operator. Make two queries and union them in memory, or write raw sql.

@MSCAU
Copy link

MSCAU commented Mar 9, 2021

Thanks, @NAR8789. It's been two years since I touched any of this code so I'll have to dive back in to make any sense of it :) Your comment is really well-written. If you have any learning / reference material to recommend, I could use it!

@henrik
Copy link
Contributor

henrik commented Jul 14, 2021

Example of making joins conditional in Rails 5:

scope :complex_stuff, -> {
  foo(joins: false).or(other_scope).joins_for_foo.any_other_joins_go_here
}

scope :foo, ->(joins: true) {
  base = joins ? joins_for_foo : self
  base.where("stuff")
}

scope :joins_for_foo, -> { joins(:bar) }

scope :other_scope, -> { where("other stuff") }

I've also added a reminder to simplify this when this project has been updated to Rails 6.1+.

@TiesWestendorp
Copy link

TiesWestendorp commented Aug 24, 2022

Working on Rails 6.1.6.1, and it seems that the above is not true for me. What I'm trying to achieve is a.or(b).or(c) where:

def a
  SomeModel.where(foo: true)
end

def b
  SomeModel.where(bar: true)
end

def c
  SomeModel.joins(<<-SQL
    INNER JOIN
    ...
  SQL).where(something happens with the joined table)
end

I can get it to work by moving the join outside of c, and to the back of the or calls, but that would require me to change the INNER JOIN to a LEFT OUTER JOIN, since otherwise it would filter out rows from a and b that it shouldn't. Also, this is a less specific query than that would be generated with the INNER JOIN applied to just c, so I have the feeling that this alternative is a bit slower.

@Vinay50
Copy link

Vinay50 commented Apr 25, 2023

@robmathews Do you know where below method can be added to rails application?

def structurally_incompatible_values_for_or(other)
  Relation::SINGLE_VALUE_METHODS.reject { |m| send("#{m}_value") == other.send("#{m}_value") } +
    (Relation::MULTI_VALUE_METHODS - [:eager_load, :references, :extending]).reject { |m| send("#{m}_values") == other.send("#{m}_values") } +
    (Relation::CLAUSE_METHODS - [:having, :where]).reject { |m| send("#{m}_clause") == other.send("#{m}_clause") }
end

@robmathews
Copy link

Looks like it might have been handled a long time ago. At least, reading down the thread, someone closed it and made a PR.

@sergioisidoro
Copy link
Contributor

sergioisidoro commented May 3, 2023

I thiiink that means that on rails >= 6.1.3, you can .or together .joins-containing scopes, and it will just work.

Just tested this on Rails 7.x (7.0.4), and it fails.
@NAR8789 trick of pushing the joins outside of the .or worked perfectly tho - #24055 (comment)

My case:

joins(:employments).where(employments: { company: other_user.companies }).or(joins(:teams_users).where(teams_users: { project: other_user.projects })).joins(:teams_users).joins(:employments).distinct

Fails with ArgumentError: Relation passed to #or must be structurally compatible. Incompatible values: [:joins]

where(employments: { company: other_user.companies }).or(where(teams_users: { project: other_user.projects })).joins(:teams_users).joins(:employments).distinct

Succeeds!
(Edit: That should have been a left_joins tho)

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

No branches or pull requests