Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Disable query cache for lock queries #6985

Merged
merged 1 commit into from

10 participants

@sidonath

Query caching should be disabled for queries asking for a pessimistic lock. Otherwise the lock query may not reach the database, thus breaking the expected functionality.

Fixes #867

@rafaelfranca rafaelfranca commented on the diff
...ve_record/connection_adapters/abstract/query_cache.rb
@@ -83,6 +83,14 @@ def cache_sql(sql, binds)
result.collect { |row| row.dup }
end
end
+
+ def locked?(arel)
+ if arel.respond_to?(:locked)
@rafaelfranca Owner

Are there some case where locked is not defined?

@sidonath
sidonath added a note

Yes, sometimes the arel is actually a String.

Without the check QueryCacheTest#test_cache_does_not_wrap_string_results_in_arrays was failing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rafaelfranca rafaelfranca merged commit 717aa92 into from
@jjb
jjb commented

@rafaelfranca - should this also be ported to the rails 3 branch?

@rafaelfranca
Owner

@jjb I can backport to 3-2-stable, but I want to confirm with @tenderlove first.

@rafaelfranca
Owner

@jbb done at 7adc4f2

@jjb
jjb commented

whoo hoo!

@nragaz

Line 83 causes an NoMethodFound exception when there is a 'locked' scope defined on a model. The respond_to? call returns true but the method is not found.

Perhaps having a scope named 'locked' is a bad idea anyway, but that isn't documented as far as I know.

@ihoka

@nragaz Agreed on naming. Just got bitten by this when upgrading Rails. It has a weird interaction with Draper.

@steveklabnik
Collaborator

@ihoka oh?

@ihoka

@steveklabnik yes. Draper uses method_missing and respond_to? to delegate to the model.

The issue I ran into was triggered by calling a method on the wrapped model within a decorator, which internally uses associations (has_one in my case). When doing so, it spits out an "undefined method `locked' for #Class:0x007f82fb067068", like this: https://gist.github.com/7498247d20ecd191d77e.

I spent a bunch of time chasing the cause, and finally ended up renaming my "locked" named scope to something else.

@steveklabnik
Collaborator

Roger, okay. So not really something I could have fixed.

@nragaz

@steveklabnik I think the point is that suddenly relying on the behaviour of a method named locked to all relations may not be so great, especially since there's no documented list of scope and class method names to avoid. It's going to cause a bunch of weird interactions, because if you redefine locked the query_cache logic is still going to call your redefined method -- and sometimes it'll even still seem to work, but mostly it won't.

@steveklabnik
Collaborator

@nragaz in this case I'm speaking as the Draper maintainer, not as someone who works on Rails.

I agree that locked is a pretty common name to be adding to every relation.

@nragaz
@smathieu

I ran into this exact problem as well and have to rename many scopes. Would it be possible to add something similar to DangerousAttributeError for scopes?

@rafaelfranca

One of the possibles solution is add a method locked? as alias to loked here and check arel.respond_to?(:locked?). But I need feedback from @tenderlove if this make sense.

@giddie

Would be nice to get this fixed, or at least find a way to provide a slightly less obscure error message. Just got bitten by this.

@BM5k

Wasted a bunch of time on this. A better error is definitely needed :(

@grosser grosser commented on the diff
...ve_record/connection_adapters/abstract/query_cache.rb
@@ -83,6 +83,14 @@ def cache_sql(sql, binds)
result.collect { |row| row.dup }
end
end
+
+ def locked?(arel)
+ if arel.respond_to?(:locked)
+ arel.locked
@grosser
grosser added a note

FYI we have a scope called locked and this is blowing up ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 6, 2012
  1. @sidonath
This page is out of date. Refresh to see the latest.
View
10 activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb
@@ -56,7 +56,7 @@ def clear_query_cache
end
def select_all(arel, name = nil, binds = [])
- if @query_cache_enabled
+ if @query_cache_enabled && !locked?(arel)
sql = to_sql(arel, binds)
cache_sql(sql, binds) { super(sql, name, binds) }
else
@@ -83,6 +83,14 @@ def cache_sql(sql, binds)
result.collect { |row| row.dup }
end
end
+
+ def locked?(arel)
+ if arel.respond_to?(:locked)
@rafaelfranca Owner

Are there some case where locked is not defined?

@sidonath
sidonath added a note

Yes, sometimes the arel is actually a String.

Without the check QueryCacheTest#test_cache_does_not_wrap_string_results_in_arrays was failing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ arel.locked
@grosser
grosser added a note

FYI we have a scope called locked and this is blowing up ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ else
+ false
+ end
+ end
end
end
end
View
8 activerecord/test/cases/query_cache_test.rb
@@ -164,6 +164,14 @@ def test_cache_does_not_wrap_string_results_in_arrays
end
end
end
+
+ def test_cache_is_ignored_for_locked_relations
+ task = Task.find 1
+
+ Task.cache do
+ assert_queries(2) { task.lock!; task.lock! }
+ end
+ end
end
class QueryCacheExpiryTest < ActiveRecord::TestCase
Something went wrong with that request. Please try again.