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.

Sign up for GitHub

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

Jump to bottom

ActiveRecord 4.2.3 regression: unscope mutates source relation's where_values #20722

Closed
felixbuenemann opened this issue Jun 27, 2015 · 14 comments
Closed

ActiveRecord 4.2.3 regression: unscope mutates source relation's where_values #20722

felixbuenemann opened this issue Jun 27, 2015 · 14 comments
Assignees
@sgrif
Labels
activerecord regression
Milestone
4.2.4

Comments

@felixbuenemann
Copy link
Contributor

There is a regression in ActiveRecord 4.2.3 compared to 4.2.2 related to unscope.

Calling unscope on a relation modifies the where_values in the input relation instead of only in the returned relation:

active = Project.active
active.where_values.size
=> 1
active.bind_values.size
=> 1
active.to_sql
=> "SELECT \"projects\".* FROM \"projects\" WHERE \"projects\".\"deleted\" = 'f'"
all = active.unscope(where: :deleted)
active.where_values.size
=> 0 # WRONG should not be modified!
active.bind_values.size
=> 1
active.to_sql
=> "SELECT \"projects\".* FROM \"projects\""
active.to_a
  Project Load (1.7ms)  SELECT "projects".* FROM "projects"  [["deleted", "f"]]
PG::ProtocolViolation: ERROR:  bind message supplies 1 parameters, but prepared statement "a1" requires 0
: SELECT "projects".* FROM "projects"
@felixbuenemann
Copy link
Contributor Author

I'm currently working on a tescase using the ar template, will post a gist asap.

@felixbuenemann
Copy link
Contributor Author

While writing a testcase I noticed that unscope was already mutating the source relation before 4.2.3, which I think is caused by incomplete dereferencing in spawn. The difference is that 4.2.3 only modifies the where_values on the source relation, while 4.2.2 also modifies the bind_values.

There is no indication in the docs that unscope is supposed to modify the source relation, so I think thats also a bug.

@felixbuenemann
Copy link
Contributor Author

Here's the testcase: https://gist.github.com/21dc7e7496839ae06e07

On rails 4.2.3 it fails with the following exception:

BugTest#test_unscope_should_not_modify_source_relation:
ActiveRecord::StatementInvalid: SQLite3::RangeException: bind or column index out of range: SELECT COUNT(*) FROM "posts"

Interestingly it also fails on older versions, eg. 4.2.2, 4.1.0, 4.0.0 because both the published_posts and all_posts relations are modified on unscope:

Finished in 0.041740s, 23.9579 runs/s, 71.8736 assertions/s.

  1) Failure:
BugTest#test_unscope_should_not_modify_source_relation [ar-gh-30722-testcase.rb:46]:
Expected: 1
  Actual: 2

So it seems unscope has always modified the source relation in addition to the target/spawned relation.

@felixbuenemann
Copy link
Contributor Author

This might be related to #20721.

@fenec
Copy link
Contributor

fenec commented Jun 28, 2015

related to #20718. because of changes from here: c99bea1
maybe @sgrif can help with this issue

@senny senny added this to the 4.2.4 milestone Jun 29, 2015
@senny
Copy link
Member

senny commented Jun 29, 2015

@felixbuenemann the title mentions regression but since it also fails on older versions that's not really the case. Is it possible to create a gist that started failing on 4.2.3?

@sgrif sgrif self-assigned this Jun 29, 2015
@felixbuenemann
Copy link
Contributor Author

@senny well it fails with an exception on 4.2.3 which it didn't before. I only noticed in the process that unscope was already mutating the source previously. I could change the gist to assert on the size of where_values and bind_values as written in the initial post.

@sgrif
Copy link
Contributor

sgrif commented Jun 29, 2015

The current gist is fine, I spent the morning on this and the related issues. I have a fix pretty much done, just had to run into meetings. Should have it finished tonight or tomorrow.

@felixbuenemann
Copy link
Contributor Author

@sgrif Thanks for the update!

sgrif added a commit that referenced this issue Jun 30, 2015
@sgrif
Correct through associations using scopes
bc6ac86 
The changes introduced to through associations in c80487e were quite
interesting. Changing `relation.merge!(scope)` to `relation =
relation.merge(scope)` should in theory never cause any changes in
behavior. The subtle breakage led to a surprising conclusion.

The old code wasn't doing anything! Since `merge!` calls
`instance_exec` when given a proc, and most scopes will look something
like `has_many :foos, -> { where(foo: :bar) }`, if we're not capturing
the return value, it's a no-op. However, removing the `merge` causes
`unscope` to break.

While we're merging in the rest of the chain elsewhere, we were never
merging in `unscope` values, causing a breakage on associations where a
default scope was being unscoped in an association scope (yuk!). This is
subtly related to #20722, since it appears we were previously relying on
this mutability.

Fixes #20721.
Fixes #20727.
sgrif added a commit that referenced this issue Jun 30, 2015
@sgrif
Correct through associations using scopes
449241c 
The changes introduced to through associations in c80487e were quite
interesting. Changing `relation.merge!(scope)` to `relation =
relation.merge(scope)` should in theory never cause any changes in
behavior. The subtle breakage led to a surprising conclusion.

The old code wasn't doing anything! Since `merge!` calls
`instance_exec` when given a proc, and most scopes will look something
like `has_many :foos, -> { where(foo: :bar) }`, if we're not capturing
the return value, it's a no-op. However, removing the `merge` causes
`unscope` to break.

While we're merging in the rest of the chain elsewhere, we were never
merging in `unscope` values, causing a breakage on associations where a
default scope was being unscoped in an association scope (yuk!). This is
subtly related to #20722, since it appears we were previously relying on
this mutability.

Fixes #20721.
Fixes #20727.
@sgrif
Copy link
Contributor

sgrif commented Jun 30, 2015

This was fixed by 449241c

@sgrif sgrif closed this as completed Jun 30, 2015
@felixbuenemann
Copy link
Contributor Author

@sgrif Please reopen. I am still getting the same exception with the testcase gist posted above on 4-2-stable at 449241c.

@sgrif
Copy link
Contributor

sgrif commented Jul 2, 2015

I'll confirm when I can, but that test was passing for me.

@sgrif sgrif reopened this Jul 2, 2015
sgrif added a commit that referenced this issue Jul 19, 2015
@sgrif
Don't mutate the parent relation when unscope is called
21e05a0 
This was already fixed on master as part of a routine refactoring to
remove all mutation of any `_values` attributes, but this particular
case was still causing a bug on 4-2-stable.

Fixes #20722.
@sgrif
Copy link
Contributor

sgrif commented Jul 19, 2015

Fixed in 21e05a0

@sgrif sgrif closed this as completed Jul 19, 2015
@felixbuenemann
Copy link
Contributor Author

Thanks, works fine on 4-2-stable!

@tobypinder tobypinder mentioned this issue Jul 21, 2015
Closed
chewi added a commit to yakara-ltd/squeel that referenced this issue Jun 17, 2016
@chewi
The where_unscoping test case does apply to Rails 4.2
6bc745c 
Perhaps the original author thought it didn't because there was no
`where_unscoping` override defined for 4.2 but it inherits from 4.1.

It doesn't hurt to do the test anyway, except that the unscope call
had to be changed to `unscope!` in order for the SQL check to pass. It
should have been `unscope!` all along and it only worked because of
rails/rails#20722.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sgrif sgrif

Labels
activerecord regression
Projects
None yet
Milestone
4.2.4
Development

No branches or pull requests
4 participants
@senny @fenec @felixbuenemann @sgrif