From b19c4eff47d912ee3a038a6e0653eea7df5b67b8 Mon Sep 17 00:00:00 2001 From: Eugene Kenny Date: Mon, 13 Mar 2017 02:01:18 +0000 Subject: Sync transaction state when accessing primary key If a record is modified inside a transaction, it must check the outcome of that transaction before accessing any state which would no longer be valid if it was rolled back. For example, consider a new record that was saved inside a transaction which was later rolled back: it should be restored to its previous state so that saving it again inserts a new row into the database instead of trying to update a row that no longer exists. The `id` and `id=` methods defined on the PrimaryKey module implement this correctly, but when a model uses a custom primary key, the reader and writer methods for that attribute must check the transaction state too. The `read_attribute` and `write_attribute` methods also need to check the transaction state when accessing the primary key. --- activerecord/lib/active_record/attribute_methods/read.rb | 3 +++ activerecord/lib/active_record/attribute_methods/write.rb | 3 +++ activerecord/lib/active_record/transactions.rb | 4 ++-- 3 files changed, 8 insertions(+), 2 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/attribute_methods/read.rb b/activerecord/lib/active_record/attribute_methods/read.rb index fdc4bf6621..f7f3b7ec96 100644 --- a/activerecord/lib/active_record/attribute_methods/read.rb +++ b/activerecord/lib/active_record/attribute_methods/read.rb @@ -29,9 +29,11 @@ module ActiveRecord temp_method = "__temp__#{safe_name}" ActiveRecord::AttributeMethods::AttrNames.set_name_cache safe_name, name + sync_with_transaction_state = "sync_with_transaction_state" if name == primary_key generated_attribute_methods.module_eval <<-STR, __FILE__, __LINE__ + 1 def #{temp_method} + #{sync_with_transaction_state} name = ::ActiveRecord::AttributeMethods::AttrNames::ATTR_#{safe_name} _read_attribute(name) { |n| missing_attribute(n, caller) } end @@ -55,6 +57,7 @@ module ActiveRecord end name = self.class.primary_key if name == "id".freeze && self.class.primary_key + sync_with_transaction_state if name == self.class.primary_key _read_attribute(name, &block) end diff --git a/activerecord/lib/active_record/attribute_methods/write.rb b/activerecord/lib/active_record/attribute_methods/write.rb index 54b673c72e..9c43567a11 100644 --- a/activerecord/lib/active_record/attribute_methods/write.rb +++ b/activerecord/lib/active_record/attribute_methods/write.rb @@ -13,10 +13,12 @@ module ActiveRecord def define_method_attribute=(name) safe_name = name.unpack("h*".freeze).first ActiveRecord::AttributeMethods::AttrNames.set_name_cache safe_name, name + sync_with_transaction_state = "sync_with_transaction_state" if name == primary_key generated_attribute_methods.module_eval <<-STR, __FILE__, __LINE__ + 1 def __temp__#{safe_name}=(value) name = ::ActiveRecord::AttributeMethods::AttrNames::ATTR_#{safe_name} + #{sync_with_transaction_state} _write_attribute(name, value) end alias_method #{(name + '=').inspect}, :__temp__#{safe_name}= @@ -36,6 +38,7 @@ module ActiveRecord end name = self.class.primary_key if name == "id".freeze && self.class.primary_key + sync_with_transaction_state if name == self.class.primary_key _write_attribute(name, value) end diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb index 463bb1f314..4d883e66df 100644 --- a/activerecord/lib/active_record/transactions.rb +++ b/activerecord/lib/active_record/transactions.rb @@ -430,8 +430,8 @@ module ActiveRecord @new_record = restore_state[:new_record] @destroyed = restore_state[:destroyed] pk = self.class.primary_key - if pk && read_attribute(pk) != restore_state[:id] - write_attribute(pk, restore_state[:id]) + if pk && _read_attribute(pk) != restore_state[:id] + _write_attribute(pk, restore_state[:id]) end freeze if restore_state[:frozen?] end -- cgit v1.2.3 From 0d56480c4b547915f99f3581065e52a48bd98ca1 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Wed, 19 Jul 2017 08:18:43 +0900 Subject: Fix `find_by` with range conditions `StatementCache` doesn't support range conditions. So we need to through the args to `FinderMethods#find_by` if range value is passed. --- activerecord/lib/active_record/core.rb | 5 ++--- activerecord/lib/active_record/statement_cache.rb | 7 ++++++- 2 files changed, 8 insertions(+), 4 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/core.rb b/activerecord/lib/active_record/core.rb index 3ea7b644c2..da0331d9c3 100644 --- a/activerecord/lib/active_record/core.rb +++ b/activerecord/lib/active_record/core.rb @@ -166,8 +166,7 @@ module ActiveRecord id = ids.first - return super if id.kind_of?(Array) || - id.is_a?(ActiveRecord::Base) + return super if StatementCache.unsupported_value?(id) key = primary_key @@ -192,7 +191,7 @@ module ActiveRecord hash = args.first return super if !(Hash === hash) || hash.values.any? { |v| - v.nil? || Array === v || Hash === v || Relation === v || Base === v + StatementCache.unsupported_value?(v) } # We can't cache Post.find_by(author: david) ...yet diff --git a/activerecord/lib/active_record/statement_cache.rb b/activerecord/lib/active_record/statement_cache.rb index 1877489e55..ceb7c66766 100644 --- a/activerecord/lib/active_record/statement_cache.rb +++ b/activerecord/lib/active_record/statement_cache.rb @@ -106,6 +106,11 @@ module ActiveRecord klass.find_by_sql(sql, bind_values, preparable: true, &block) end - alias :call :execute + + def self.unsupported_value?(value) + case value + when NilClass, Array, Range, Hash, Relation, Base then true + end + end end end -- cgit v1.2.3 From 45955aed0085c453917aac2e2e5fbe5f3e73f705 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Mon, 24 Jul 2017 13:28:38 +0900 Subject: `Relation::Merger` should not fill `values` with empty values Currently `Relation#merge` will almost fill `values` with empty values (e.g. `other.order_values` is always true, it should be `other.order_values.any?`). This means that `Relation#merge` always changes `values` even if actually `values` is nothing changed. This behavior will makes `Relation#empty_scope?` fragile. So `Relation#merge` should avoid unnecessary changes. --- activerecord/lib/active_record/relation/merger.rb | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/relation/merger.rb b/activerecord/lib/active_record/relation/merger.rb index 182f654897..03824ffff9 100644 --- a/activerecord/lib/active_record/relation/merger.rb +++ b/activerecord/lib/active_record/relation/merger.rb @@ -135,19 +135,17 @@ module ActiveRecord if other.reordering_value # override any order specified in the original relation relation.reorder! other.order_values - elsif other.order_values + elsif other.order_values.any? # merge in order_values from relation relation.order! other.order_values end - relation.extend(*other.extending_values) unless other.extending_values.blank? + extensions = other.extensions - relation.extensions + relation.extending!(*extensions) if extensions.any? end def merge_single_values - if relation.from_clause.empty? - relation.from_clause = other.from_clause - end - relation.lock_value ||= other.lock_value + relation.lock_value ||= other.lock_value if other.lock_value unless other.create_with_value.blank? relation.create_with_value = (relation.create_with_value || {}).merge(other.create_with_value) @@ -155,11 +153,15 @@ module ActiveRecord end def merge_clauses - CLAUSE_METHODS.each do |method| - clause = relation.get_value(method) - other_clause = other.get_value(method) - relation.set_value(method, clause.merge(other_clause)) + if relation.from_clause.empty? && !other.from_clause.empty? + relation.from_clause = other.from_clause end + + where_clause = relation.where_clause.merge(other.where_clause) + relation.where_clause = where_clause unless where_clause.empty? + + having_clause = relation.having_clause.merge(other.having_clause) + relation.having_clause = having_clause unless having_clause.empty? end end end -- cgit v1.2.3 From f0e6ecc9966f5a569313658bedff54bde8517e65 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Mon, 24 Jul 2017 15:38:40 +0900 Subject: `get_value` and `set_value` in `Relation` are no longer used externally --- .../lib/active_record/relation/query_methods.rb | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb index 812c8e7e3f..ee49930c90 100644 --- a/activerecord/lib/active_record/relation/query_methods.rb +++ b/activerecord/lib/active_record/relation/query_methods.rb @@ -901,16 +901,17 @@ module ActiveRecord @arel ||= build_arel end - # Returns a relation value with a given name - def get_value(name) # :nodoc: - @values[name] || default_value_for(name) - end + protected + # Returns a relation value with a given name + def get_value(name) # :nodoc: + @values[name] || default_value_for(name) + end - # Sets the relation value with the given name - def set_value(name, value) # :nodoc: - assert_mutability! - @values[name] = value - end + # Sets the relation value with the given name + def set_value(name, value) # :nodoc: + assert_mutability! + @values[name] = value + end private -- cgit v1.2.3