rails/rails — pull request #17820 by fw42 (2014-11-28 19:30:29+00:00)
Title: Restore query cache on rollback

Fixes https://github.com/rails/rails/issues/17449

Make sure the query cache is flushed after transaction rollback (for both real and savepoint transactions).

The changes here feel a little bit hacky, I'm looking for some suggestions how to do this cleaner and without too much monkey patching. I had a few other ideas, but @arthurnn suggested that the approach in this PR is better (more consistent with exec_insert etc.).

### Alternative 1: Just do it in Transaction and assume QueryCache is already loaded
```diff
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb b/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb
index 4a4506c..d20c02e 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb
@@ -18,7 +18,8 @@ module ActiveRecord
end
end

- attr_reader :query_cache, :query_cache_enabled
+ attr_reader :query_cache_enabled
+ attr_accessor :query_cache

def initialize(*)
super
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
index fd666c8..f247bb8 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
@@ -184,10 +184,12 @@ module ActiveRecord
end

def within_new_transaction(options = {})
+ old_query_cache = @connection.query_cache.dup
transaction = begin_transaction options
yield
rescue Exception => error
rollback_transaction if transaction
+ @connection.query_cache = old_query_cache
raise
ensure
unless error
```

### Alternative 2: Monkey-patch the transaction manager
```diff
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb b/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb
index 4a4506c..353ba4c 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb
@@ -1,9 +1,17 @@
module ActiveRecord
module ConnectionAdapters # :nodoc:
+ module TransactionManagerDirtiesQueryCache
+ def rollback_transaction(*)
+ @connection.clear_query_cache if @connection.query_cache_enabled
+ super
+ end
+ end
+
module QueryCache
class << self
def included(base) #:nodoc:
dirties_query_cache base, :insert, :update, :delete
+ base.new.transaction_manager.class.prepend(TransactionManagerDirtiesQueryCache)
end
```

### Alternative 3: Add a new Connection#rollback method that always gets called
```diff
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb
index 12b16b2..55f3e54 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb
@@ -235,7 +235,7 @@ module ActiveRecord
# Rolls back the transaction (and turns on auto-committing). Must be
# done if the transaction block raises an exception or returns false.
def rollback_db_transaction() end

+ # Rolls back non-db state of the connection, like query caches. Gets
+ # called on transaction rollback.
+ def rollback() end
+
def default_sequence_name(table, column)
nil
end
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb b/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb
index 4a4506c..b056104 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb
@@ -3,7 +3,7 @@ module ActiveRecord
module QueryCache
class << self
def included(base) #:nodoc:
- dirties_query_cache base, :insert, :update, :delete
+ dirties_query_cache base, :insert, :update, :delete, :rollback
end

def dirties_query_cache(base, *method_names)
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
index fd666c8..d3a3ea8 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
@@ -63,6 +63,7 @@ module ActiveRecord
end

def rollback
+ connection.rollback
@state.set_state(:rolledback)
end
```

Thoughts? @arthurnn @dylanahsmith @sirupsen