From 0d7ab973e15b476ed42471395027e329f95c8951 Mon Sep 17 00:00:00 2001
From: "yuuji.yaginuma" <yuuji.yaginuma@gmail.com>
Date: Fri, 3 Nov 2017 10:09:43 +0900
Subject: Remove unused classes
* `HasManyThroughCantDissociateNewRecords` and `HasManyThroughCantAssociateNewRecords`
are no longer used since f6b12c1.
* `ReadOnlyAssociation` is no longer used since 0da426b.
---
activerecord/lib/active_record/associations.rb | 30 --------------------------
1 file changed, 30 deletions(-)
(limited to 'activerecord')
diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb
index bc7e288500..661605d3e5 100644
--- a/activerecord/lib/active_record/associations.rb
+++ b/activerecord/lib/active_record/associations.rb
@@ -140,26 +140,6 @@ module ActiveRecord
class HasOneThroughCantAssociateThroughHasOneOrManyReflection < ThroughCantAssociateThroughHasOneOrManyReflection #:nodoc:
end
- class HasManyThroughCantAssociateNewRecords < ActiveRecordError #:nodoc:
- def initialize(owner = nil, reflection = nil)
- if owner && reflection
- super("Cannot associate new records through '#{owner.class.name}##{reflection.name}' on '#{reflection.source_reflection.class_name rescue nil}##{reflection.source_reflection.name rescue nil}'. Both records must have an id in order to create the has_many :through record associating them.")
- else
- super("Cannot associate new records.")
- end
- end
- end
-
- class HasManyThroughCantDissociateNewRecords < ActiveRecordError #:nodoc:
- def initialize(owner = nil, reflection = nil)
- if owner && reflection
- super("Cannot dissociate new records through '#{owner.class.name}##{reflection.name}' on '#{reflection.source_reflection.class_name rescue nil}##{reflection.source_reflection.name rescue nil}'. Both records must have an id in order to delete the has_many :through record associating them.")
- else
- super("Cannot dissociate new records.")
- end
- end
- end
-
class ThroughNestedAssociationsAreReadonly < ActiveRecordError #:nodoc:
def initialize(owner = nil, reflection = nil)
if owner && reflection
@@ -189,16 +169,6 @@ module ActiveRecord
end
end
- class ReadOnlyAssociation < ActiveRecordError #:nodoc:
- def initialize(reflection = nil)
- if reflection
- super("Cannot add to a has_many :through association. Try adding to #{reflection.through_reflection.name.inspect}.")
- else
- super("Read-only reflection error.")
- end
- end
- end
-
# This error is raised when trying to destroy a parent instance in N:1 or 1:1 associations
# (has_many, has_one) when there is at least 1 child associated instance.
# ex: if @project.tasks.size > 0, DeleteRestrictionError will be raised when trying to destroy @project
--
cgit v1.2.3
From e3a295664681ea0480a9c95f6bfc776a113ff926 Mon Sep 17 00:00:00 2001
From: Ryuta Kamizono <kamipo@gmail.com>
Date: Wed, 1 Nov 2017 10:36:47 +0900
Subject: Ensure `apply_join_dependency` for `collection_cache_key` if
eager-loading is needed
Fixes #30315.
---
activerecord/lib/active_record/collection_cache_key.rb | 3 +++
activerecord/lib/active_record/relation/calculations.rb | 4 ++--
activerecord/lib/active_record/relation/finder_methods.rb | 2 +-
activerecord/test/cases/collection_cache_key_test.rb | 10 ++++++++++
activerecord/test/schema/schema.rb | 1 +
5 files changed, 17 insertions(+), 3 deletions(-)
(limited to 'activerecord')
diff --git a/activerecord/lib/active_record/collection_cache_key.rb b/activerecord/lib/active_record/collection_cache_key.rb
index f3e6516414..88b398ad45 100644
--- a/activerecord/lib/active_record/collection_cache_key.rb
+++ b/activerecord/lib/active_record/collection_cache_key.rb
@@ -12,6 +12,9 @@ module ActiveRecord
timestamp = collection.max_by(×tamp_column)._read_attribute(timestamp_column)
end
else
+ if collection.eager_loading?
+ collection = collection.send(:apply_join_dependency)
+ end
column_type = type_for_attribute(timestamp_column.to_s)
column = connection.column_name_from_arel_node(collection.arel_attribute(timestamp_column))
select_values = "COUNT(*) AS #{connection.quote_column_name("size")}, MAX(%s) AS timestamp"
diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb
index 116bddce85..11256ab3d9 100644
--- a/activerecord/lib/active_record/relation/calculations.rb
+++ b/activerecord/lib/active_record/relation/calculations.rb
@@ -130,7 +130,7 @@ module ActiveRecord
# end
def calculate(operation, column_name)
if has_include?(column_name)
- relation = apply_join_dependency(construct_join_dependency)
+ relation = apply_join_dependency
relation.distinct! if operation.to_s.downcase == "count"
relation.calculate(operation, column_name)
@@ -180,7 +180,7 @@ module ActiveRecord
end
if has_include?(column_names.first)
- relation = apply_join_dependency(construct_join_dependency)
+ relation = apply_join_dependency
relation.pluck(*column_names)
else
relation = spawn
diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb
index 707245bab2..18566b5662 100644
--- a/activerecord/lib/active_record/relation/finder_methods.rb
+++ b/activerecord/lib/active_record/relation/finder_methods.rb
@@ -394,7 +394,7 @@ module ActiveRecord
)
end
- def apply_join_dependency(join_dependency)
+ def apply_join_dependency(join_dependency = construct_join_dependency)
relation = except(:includes, :eager_load, :preload).joins!(join_dependency)
if using_limitable_reflections?(join_dependency.reflections)
diff --git a/activerecord/test/cases/collection_cache_key_test.rb b/activerecord/test/cases/collection_cache_key_test.rb
index c137693211..19d6464a22 100644
--- a/activerecord/test/cases/collection_cache_key_test.rb
+++ b/activerecord/test/cases/collection_cache_key_test.rb
@@ -73,6 +73,16 @@ module ActiveRecord
assert_equal last_developer_timestamp.to_s(ActiveRecord::Base.cache_timestamp_format), $3
end
+ test "cache_key for relation with includes" do
+ comments = Comment.includes(:post).where("posts.type": "Post")
+ assert_match(/\Acomments\/query-(\h+)-(\d+)-(\d+)\z/, comments.cache_key)
+ end
+
+ test "cache_key for loaded relation with includes" do
+ comments = Comment.includes(:post).where("posts.type": "Post").load
+ assert_match(/\Acomments\/query-(\h+)-(\d+)-(\d+)\z/, comments.cache_key)
+ end
+
test "it triggers at most one query" do
developers = Developer.where(name: "David")
diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb
index 8f872c38ba..a4505a4892 100644
--- a/activerecord/test/schema/schema.rb
+++ b/activerecord/test/schema/schema.rb
@@ -191,6 +191,7 @@ ActiveRecord::Schema.define do
t.string :resource_id
t.string :resource_type
t.integer :developer_id
+ t.datetime :updated_at
t.datetime :deleted_at
t.integer :comments
end
--
cgit v1.2.3
From 8a2ee3d8c6921ce34e597658e3f2b43b41423d1d Mon Sep 17 00:00:00 2001
From: Ryuta Kamizono <kamipo@gmail.com>
Date: Mon, 16 Oct 2017 01:33:40 +0900
Subject: Ensure `apply_join_dependency` for `update_all` and `delete_all` if
eager-loading is needed
If a relation has eager-loading values, `count` and `exists?` works
properly, but `update_all` and `delete_all` doesn't work due to missing
`apply_join_dependency`. It should be applied to work consistently.
Fixes #28863.
---
activerecord/lib/active_record/relation.rb | 10 ++++++
activerecord/test/cases/persistence_test.rb | 49 ++++++++++++++++++-----------
2 files changed, 40 insertions(+), 19 deletions(-)
(limited to 'activerecord')
diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb
index 997cfe4b5e..7615fb6ee9 100644
--- a/activerecord/lib/active_record/relation.rb
+++ b/activerecord/lib/active_record/relation.rb
@@ -359,6 +359,11 @@ module ActiveRecord
def update_all(updates)
raise ArgumentError, "Empty list of attributes to change" if updates.blank?
+ if eager_loading?
+ relation = apply_join_dependency
+ return relation.update_all(updates)
+ end
+
stmt = Arel::UpdateManager.new
stmt.set Arel.sql(@klass.send(:sanitize_sql_for_assignment, updates))
@@ -423,6 +428,11 @@ module ActiveRecord
raise ActiveRecordError.new("delete_all doesn't support #{invalid_methods.join(', ')}")
end
+ if eager_loading?
+ relation = apply_join_dependency
+ return relation.delete_all
+ end
+
stmt = Arel::DeleteManager.new
stmt.from(table)
diff --git a/activerecord/test/cases/persistence_test.rb b/activerecord/test/cases/persistence_test.rb
index 6cbe18cc8c..f088c064f5 100644
--- a/activerecord/test/cases/persistence_test.rb
+++ b/activerecord/test/cases/persistence_test.rb
@@ -94,27 +94,31 @@ class PersistenceTest < ActiveRecord::TestCase
end
def test_delete_all_with_joins_and_where_part_is_hash
- where_args = { toys: { name: "Bone" } }
- count = Pet.joins(:toys).where(where_args).count
+ pets = Pet.joins(:toys).where(toys: { name: "Bone" })
- assert_equal count, 1
- assert_equal count, Pet.joins(:toys).where(where_args).delete_all
+ assert_equal true, pets.exists?
+ assert_equal pets.count, pets.delete_all
+ end
+
+ def test_delete_all_with_joins_and_where_part_is_not_hash
+ pets = Pet.joins(:toys).where("toys.name = ?", "Bone")
+
+ assert_equal true, pets.exists?
+ assert_equal pets.count, pets.delete_all
end
def test_delete_all_with_left_joins
- where_args = { toys: { name: "Bone" } }
- count = Pet.left_joins(:toys).where(where_args).count
+ pets = Pet.left_joins(:toys).where(toys: { name: "Bone" })
- assert_equal count, 1
- assert_equal count, Pet.left_joins(:toys).where(where_args).delete_all
+ assert_equal true, pets.exists?
+ assert_equal pets.count, pets.delete_all
end
- def test_delete_all_with_joins_and_where_part_is_not_hash
- where_args = ["toys.name = ?", "Bone"]
- count = Pet.joins(:toys).where(where_args).count
+ def test_delete_all_with_includes
+ pets = Pet.includes(:toys).where(toys: { name: "Bone" })
- assert_equal count, 1
- assert_equal count, Pet.joins(:toys).where(where_args).delete_all
+ assert_equal true, pets.exists?
+ assert_equal pets.count, pets.delete_all
end
def test_increment_attribute
@@ -496,17 +500,24 @@ class PersistenceTest < ActiveRecord::TestCase
end
def test_update_all_with_joins
- where_args = { toys: { name: "Bone" } }
- count = Pet.left_joins(:toys).where(where_args).count
+ pets = Pet.joins(:toys).where(toys: { name: "Bone" })
- assert_equal count, Pet.joins(:toys).where(where_args).update_all(name: "Bob")
+ assert_equal true, pets.exists?
+ assert_equal pets.count, pets.update_all(name: "Bob")
end
def test_update_all_with_left_joins
- where_args = { toys: { name: "Bone" } }
- count = Pet.left_joins(:toys).where(where_args).count
+ pets = Pet.left_joins(:toys).where(toys: { name: "Bone" })
+
+ assert_equal true, pets.exists?
+ assert_equal pets.count, pets.update_all(name: "Bob")
+ end
+
+ def test_update_all_with_includes
+ pets = Pet.includes(:toys).where(toys: { name: "Bone" })
- assert_equal count, Pet.left_joins(:toys).where(where_args).update_all(name: "Bob")
+ assert_equal true, pets.exists?
+ assert_equal pets.count, pets.update_all(name: "Bob")
end
def test_update_all_with_non_standard_table_name
--
cgit v1.2.3
From e617fb57f5a388d5f0a47fd5e576588dd10066b0 Mon Sep 17 00:00:00 2001
From: Ryuta Kamizono <kamipo@gmail.com>
Date: Mon, 30 Oct 2017 23:55:48 +0900
Subject: Fix preloading polymorphic association when through association has
already loaded
If through association has already loaded, `source_type` is ignored to
loaded through records. The loaded records should be filtered by
`source_type` in that case.
Fixes #30904.
---
.../associations/preloader/through_association.rb | 20 ++++++++++++++++----
.../associations/nested_through_associations_test.rb | 11 +++++++++++
2 files changed, 27 insertions(+), 4 deletions(-)
(limited to 'activerecord')
diff --git a/activerecord/lib/active_record/associations/preloader/through_association.rb b/activerecord/lib/active_record/associations/preloader/through_association.rb
index fa32cc5553..5bd49b041a 100644
--- a/activerecord/lib/active_record/associations/preloader/through_association.rb
+++ b/activerecord/lib/active_record/associations/preloader/through_association.rb
@@ -13,18 +13,30 @@ module ActiveRecord
end
def associated_records_by_owner(preloader)
+ already_loaded = owners.first.association(through_reflection.name).loaded?
through_scope = through_scope()
- preloader.preload(owners,
- through_reflection.name,
- through_scope)
+ unless already_loaded
+ preloader.preload(owners, through_reflection.name, through_scope)
+ end
through_records = owners.map do |owner|
center = owner.association(through_reflection.name).target
[owner, Array(center)]
end
- reset_association(owners, through_reflection.name, through_scope)
+ if already_loaded
+ if source_type = reflection.options[:source_type]
+ through_records.map! do |owner, center|
+ center = center.select do |record|
+ record[reflection.foreign_type] == source_type
+ end
+ [owner, center]
+ end
+ end
+ else
+ reset_association(owners, through_reflection.name, through_scope)
+ end
middle_records = through_records.flat_map(&:last)
diff --git a/activerecord/test/cases/associations/nested_through_associations_test.rb b/activerecord/test/cases/associations/nested_through_associations_test.rb
index 4cf5a9ffc4..11ee45917f 100644
--- a/activerecord/test/cases/associations/nested_through_associations_test.rb
+++ b/activerecord/test/cases/associations/nested_through_associations_test.rb
@@ -579,6 +579,17 @@ class NestedThroughAssociationsTest < ActiveRecord::TestCase
assert !c.post_taggings.empty?
end
+ def test_polymorphic_has_many_through_when_through_association_has_already_loaded
+ cake_designer = CakeDesigner.create!(chef: Chef.new)
+ drink_designer = DrinkDesigner.create!(chef: Chef.new)
+ department = Department.create!(chefs: [cake_designer.chef, drink_designer.chef])
+ Hotel.create!(departments: [department])
+ hotel = Hotel.includes(:chefs, :cake_designers, :drink_designers).take
+
+ assert_equal [cake_designer], hotel.cake_designers
+ assert_equal [drink_designer], hotel.drink_designers
+ end
+
def test_polymorphic_has_many_through_joined_different_table_twice
cake_designer = CakeDesigner.create!(chef: Chef.new)
drink_designer = DrinkDesigner.create!(chef: Chef.new)
--
cgit v1.2.3
From e0bef22665f93e88f6b2f3ac6bd55543ed0d6343 Mon Sep 17 00:00:00 2001
From: Ryuta Kamizono <kamipo@gmail.com>
Date: Mon, 6 Nov 2017 06:40:36 +0900
Subject: Fix preloading polymorphic multi-level through association
This is partially fixed by e617fb57 when through association has already
loaded. Otherwise, second level through association should respect
`preload_scope`.
Fixes #30242.
Closes #30076.
[Ryuta Kamizono & CicholGricenchos]
---
.../associations/preloader/through_association.rb | 8 +++++++-
.../cases/associations/nested_through_associations_test.rb | 11 +++++++++++
2 files changed, 18 insertions(+), 1 deletion(-)
(limited to 'activerecord')
diff --git a/activerecord/lib/active_record/associations/preloader/through_association.rb b/activerecord/lib/active_record/associations/preloader/through_association.rb
index 5bd49b041a..b16fca7dc9 100644
--- a/activerecord/lib/active_record/associations/preloader/through_association.rb
+++ b/activerecord/lib/active_record/associations/preloader/through_association.rb
@@ -40,7 +40,11 @@ module ActiveRecord
middle_records = through_records.flat_map(&:last)
- reflection_scope = reflection_scope() if reflection.scope
+ if preload_scope
+ reflection_scope = reflection_scope().merge(preload_scope)
+ elsif reflection.scope
+ reflection_scope = reflection_scope()
+ end
preloaders = preloader.preload(middle_records,
source_reflection.name,
@@ -70,6 +74,8 @@ module ActiveRecord
rhs_records
end
end
+ end.tap do
+ reset_association(middle_records, source_reflection.name, preload_scope)
end
end
diff --git a/activerecord/test/cases/associations/nested_through_associations_test.rb b/activerecord/test/cases/associations/nested_through_associations_test.rb
index 11ee45917f..65d30d011b 100644
--- a/activerecord/test/cases/associations/nested_through_associations_test.rb
+++ b/activerecord/test/cases/associations/nested_through_associations_test.rb
@@ -579,6 +579,17 @@ class NestedThroughAssociationsTest < ActiveRecord::TestCase
assert !c.post_taggings.empty?
end
+ def test_polymorphic_has_many_through_when_through_association_has_not_loaded
+ cake_designer = CakeDesigner.create!(chef: Chef.new)
+ drink_designer = DrinkDesigner.create!(chef: Chef.new)
+ department = Department.create!(chefs: [cake_designer.chef, drink_designer.chef])
+ Hotel.create!(departments: [department])
+ hotel = Hotel.includes(:cake_designers, :drink_designers).take
+
+ assert_equal [cake_designer], hotel.cake_designers
+ assert_equal [drink_designer], hotel.drink_designers
+ end
+
def test_polymorphic_has_many_through_when_through_association_has_already_loaded
cake_designer = CakeDesigner.create!(chef: Chef.new)
drink_designer = DrinkDesigner.create!(chef: Chef.new)
--
cgit v1.2.3
From ffc1ed9bd54f3cf746f7ddf798e000fc025edef6 Mon Sep 17 00:00:00 2001
From: Ryuta Kamizono <kamipo@gmail.com>
Date: Mon, 6 Nov 2017 08:29:40 +0900
Subject: `scoping` should respect current class and STI constraint (#29199)
A relation includes `klass`, so it can not be used as it is if current
class is different from `current_scope.klass`. It should be created new
relation by current class to respect the klass and STI constraint.
Fixes #17603.
Fixes #23576.
---
activerecord/lib/active_record/scoping/named.rb | 8 +++++++-
activerecord/test/cases/scoping/relation_scoping_test.rb | 14 ++++++++++++++
activerecord/test/models/comment.rb | 4 ++++
3 files changed, 25 insertions(+), 1 deletion(-)
(limited to 'activerecord')
diff --git a/activerecord/lib/active_record/scoping/named.rb b/activerecord/lib/active_record/scoping/named.rb
index 6fa096c1fe..310af72c41 100644
--- a/activerecord/lib/active_record/scoping/named.rb
+++ b/activerecord/lib/active_record/scoping/named.rb
@@ -24,8 +24,14 @@ module ActiveRecord
# You can define a scope that applies to all finders using
# {default_scope}[rdoc-ref:Scoping::Default::ClassMethods#default_scope].
def all
+ current_scope = self.current_scope
+
if current_scope
- current_scope.clone
+ if self == current_scope.klass
+ current_scope.clone
+ else
+ relation.merge!(current_scope)
+ end
else
default_scoped
end
diff --git a/activerecord/test/cases/scoping/relation_scoping_test.rb b/activerecord/test/cases/scoping/relation_scoping_test.rb
index f3b84d88c2..116f8e83aa 100644
--- a/activerecord/test/cases/scoping/relation_scoping_test.rb
+++ b/activerecord/test/cases/scoping/relation_scoping_test.rb
@@ -240,6 +240,20 @@ class RelationScopingTest < ActiveRecord::TestCase
assert_nil SpecialComment.current_scope
end
+ def test_scoping_respects_current_class
+ Comment.unscoped do
+ assert_equal "a comment...", Comment.all.what_are_you
+ assert_equal "a special comment...", SpecialComment.all.what_are_you
+ end
+ end
+
+ def test_scoping_respects_sti_constraint
+ Comment.unscoped do
+ assert_equal comments(:greetings), Comment.find(1)
+ assert_raises(ActiveRecord::RecordNotFound) { SpecialComment.find(1) }
+ end
+ end
+
def test_circular_joins_with_scoping_does_not_crash
posts = Post.joins(comments: :post).scoping do
Post.first(10)
diff --git a/activerecord/test/models/comment.rb b/activerecord/test/models/comment.rb
index 740aa593ac..5ab433f2d9 100644
--- a/activerecord/test/models/comment.rb
+++ b/activerecord/test/models/comment.rb
@@ -60,6 +60,10 @@ end
class SpecialComment < Comment
default_scope { where(deleted_at: nil) }
+
+ def self.what_are_you
+ "a special comment..."
+ end
end
class SubSpecialComment < SpecialComment
--
cgit v1.2.3
From edcc0a714d911468caf7db0879fc7a3d1185ee3c Mon Sep 17 00:00:00 2001
From: Bogdan Gusiev <agresso@gmail.com>
Date: Mon, 6 Nov 2017 17:32:54 +0200
Subject: Refactor Preloader Code
---
.../associations/preloader/association.rb | 4 +-
.../associations/preloader/has_many_through.rb | 10 --
.../associations/preloader/through_association.rb | 107 ++++++++-------------
3 files changed, 40 insertions(+), 81 deletions(-)
(limited to 'activerecord')
diff --git a/activerecord/lib/active_record/associations/preloader/association.rb b/activerecord/lib/active_record/associations/preloader/association.rb
index 607d376a08..b0c41c7d5f 100644
--- a/activerecord/lib/active_record/associations/preloader/association.rb
+++ b/activerecord/lib/active_record/associations/preloader/association.rb
@@ -17,7 +17,7 @@ module ActiveRecord
end
def run(preloader)
- associated_records_by_owner(preloader).each do |owner, records|
+ associated_records_by_owner(preloader) do |owner, records|
associate_records_to_owner(owner, records)
end
end
@@ -41,7 +41,7 @@ module ActiveRecord
end
owners.each_with_object({}) do |owner, result|
- result[owner] = records[convert_key(owner[owner_key_name])] || []
+ yield(owner, records[convert_key(owner[owner_key_name])] || [])
end
end
diff --git a/activerecord/lib/active_record/associations/preloader/has_many_through.rb b/activerecord/lib/active_record/associations/preloader/has_many_through.rb
index 0639fdca44..3e17d07a33 100644
--- a/activerecord/lib/active_record/associations/preloader/has_many_through.rb
+++ b/activerecord/lib/active_record/associations/preloader/has_many_through.rb
@@ -5,16 +5,6 @@ module ActiveRecord
class Preloader
class HasManyThrough < CollectionAssociation #:nodoc:
include ThroughAssociation
-
- def associated_records_by_owner(preloader)
- records_by_owner = super
-
- if reflection_scope.distinct_value
- records_by_owner.each_value(&:uniq!)
- end
-
- records_by_owner
- end
end
end
end
diff --git a/activerecord/lib/active_record/associations/preloader/through_association.rb b/activerecord/lib/active_record/associations/preloader/through_association.rb
index b16fca7dc9..1bb7e98231 100644
--- a/activerecord/lib/active_record/associations/preloader/through_association.rb
+++ b/activerecord/lib/active_record/associations/preloader/through_association.rb
@@ -13,86 +13,45 @@ module ActiveRecord
end
def associated_records_by_owner(preloader)
- already_loaded = owners.first.association(through_reflection.name).loaded?
- through_scope = through_scope()
-
- unless already_loaded
- preloader.preload(owners, through_reflection.name, through_scope)
- end
-
- through_records = owners.map do |owner|
- center = owner.association(through_reflection.name).target
- [owner, Array(center)]
- end
+ already_loaded = owners.first.association(through_reflection.name).loaded?
+ through_scope = through_scope()
+ reflection_scope = target_reflection_scope
+ through_preloaders = preloader.preload(owners, through_reflection.name, through_scope)
+ middle_records = through_preloaders.flat_map(&:preloaded_records)
+ preloaders = preloader.preload(middle_records, source_reflection.name, reflection_scope)
+ @preloaded_records = preloaders.flat_map(&:preloaded_records)
- if already_loaded
- if source_type = reflection.options[:source_type]
- through_records.map! do |owner, center|
- center = center.select do |record|
+ owners.each do |owner|
+ through_records = Array(owner.association(through_reflection.name).target)
+ if already_loaded
+ if source_type = reflection.options[:source_type]
+ through_records = through_records.select do |record|
record[reflection.foreign_type] == source_type
end
- [owner, center]
end
+ else
+ owner.association(through_reflection.name).reset if through_scope
end
- else
- reset_association(owners, through_reflection.name, through_scope)
- end
-
- middle_records = through_records.flat_map(&:last)
-
- if preload_scope
- reflection_scope = reflection_scope().merge(preload_scope)
- elsif reflection.scope
- reflection_scope = reflection_scope()
- end
-
- preloaders = preloader.preload(middle_records,
- source_reflection.name,
- reflection_scope)
-
- @preloaded_records = preloaders.flat_map(&:preloaded_records)
-
- middle_to_pl = preloaders.each_with_object({}) do |pl, h|
- pl.owners.each { |middle|
- h[middle] = pl
- }
- end
-
- through_records.each_with_object({}) do |(lhs, center), records_by_owner|
- pl_to_middle = center.group_by { |record| middle_to_pl[record] }
-
- records_by_owner[lhs] = pl_to_middle.flat_map do |pl, middles|
- rhs_records = middles.flat_map { |r|
- r.association(source_reflection.name).target
- }.compact
-
- # Respect the order on `reflection_scope` if it exists, else use the natural order.
- if reflection_scope && !reflection_scope.order_values.empty?
- @id_map ||= id_to_index_map @preloaded_records
- rhs_records.sort_by { |rhs| @id_map[rhs] }
- else
- rhs_records
- end
+ result = through_records.flat_map do |record|
+ association = record.association(source_reflection.name)
+ target = association.target
+ association.reset if preload_scope
+ target
end
- end.tap do
- reset_association(middle_records, source_reflection.name, preload_scope)
+ result.compact!
+ if reflection_scope
+ result.sort_by! { |rhs| preload_index[rhs] } if reflection_scope.order_values.any?
+ result.uniq! if reflection_scope.distinct_value
+ end
+ yield(owner, result)
end
end
private
- def id_to_index_map(ids)
- id_map = {}
- ids.each_with_index { |id, index| id_map[id] = index }
- id_map
- end
-
- def reset_association(owners, association_name, should_reset)
- # Don't cache the association - we would only be caching a subset
- if should_reset
- owners.each { |owner|
- owner.association(association_name).reset
- }
+ def preload_index
+ @preload_index ||= @preloaded_records.each_with_object({}).with_index do |(id, result), index|
+ result[id] = index
end
end
@@ -133,6 +92,16 @@ module ActiveRecord
scope unless scope.empty_scope?
end
+
+ def target_reflection_scope
+ if preload_scope
+ reflection_scope.merge(preload_scope)
+ elsif reflection.scope
+ reflection_scope
+ else
+ nil
+ end
+ end
end
end
end
--
cgit v1.2.3
From 01c703248396528d9f3398d0f9d0143831e9d0dc Mon Sep 17 00:00:00 2001
From: Keenan Brock <keenan@thebrocks.net>
Date: Thu, 9 Mar 2017 11:48:44 -0500
Subject: Properly check transaction in persistence
```
[NoMethodError]: undefined method `state' for nil:NilClass Method:[rescue in block in refresh]
```
In `within_new_transaction`, there is the possibility that
`begin_transaction` returns a `nil`. (i.e.: so `transaction = nil`)
So this method is checking `transaction` for nil in 2 spots.
Unfortunately, there is one line that is not checking `transaction` for `nil`
That line, `commit_transaction`, throws an exception for us in AR 5.0.0.1
The problem with the method is finally realized in the error checking itself.
it calls `transaction.state` (i.e.: nil.state) and that is the final exception
raised.
The actual underlying (user) issue is hidden by this line.
Solution is test transaction for nil.
---
.../lib/active_record/connection_adapters/abstract/transaction.rb | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
(limited to 'activerecord')
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
index 147e16e9fa..d9ac8db6a8 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
@@ -240,7 +240,7 @@ module ActiveRecord
rollback_transaction if transaction
else
begin
- commit_transaction
+ commit_transaction if transaction
rescue Exception
rollback_transaction(transaction) unless transaction.state.completed?
raise
--
cgit v1.2.3
From 90fe2a42f0b68a66e970169d38e91a0126de7d3e Mon Sep 17 00:00:00 2001
From: bogdanvlviv <bogdanvlviv@gmail.com>
Date: Tue, 26 Sep 2017 00:10:17 +0300
Subject: Fix `bin/rails db:migrate` with specified `VERSION`
Ensure that `bin/rails db:migrate` with specified `VERSION` reverts
all migrations only if `VERSION` is `0`.
Raise error if target migration doesn't exist.
---
activerecord/CHANGELOG.md | 8 ++
activerecord/lib/active_record/migration.rb | 2 +-
.../lib/active_record/railties/databases.rake | 19 ++-
.../lib/active_record/tasks/database_tasks.rb | 15 ++-
activerecord/test/cases/migrator_test.rb | 20 +++
.../test/cases/tasks/database_tasks_test.rb | 149 ++++++++++++++++++++-
6 files changed, 201 insertions(+), 12 deletions(-)
(limited to 'activerecord')
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 4aa7ecfc7e..e6a41ec281 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,11 @@
+* Fix `bin/rails db:migrate` with specified `VERSION`.
+ `bin/rails db:migrate` with empty VERSION behaves as without `VERSION`.
+ Check a format of `VERSION`: Allow a migration version number
+ or name of a migration file. Raise error if format of `VERSION` is invalid.
+ Raise error if target migration doesn't exist.
+
+ *bogdanvlviv*
+
* Fixed a bug where column orders for an index weren't written to
db/schema.rb when using the sqlite adapter.
diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb
index 1485687053..d12a979a7f 100644
--- a/activerecord/lib/active_record/migration.rb
+++ b/activerecord/lib/active_record/migration.rb
@@ -1224,7 +1224,7 @@ module ActiveRecord
# Return true if a valid version is not provided.
def invalid_target?
- !target && @target_version && @target_version > 0
+ @target_version && @target_version != 0 && !target
end
def execute_migration_in_transaction(migration, direction)
diff --git a/activerecord/lib/active_record/railties/databases.rake b/activerecord/lib/active_record/railties/databases.rake
index 723272b4b2..3bca2982e0 100644
--- a/activerecord/lib/active_record/railties/databases.rake
+++ b/activerecord/lib/active_record/railties/databases.rake
@@ -97,16 +97,27 @@ db_namespace = namespace :db do
task up: [:environment, :load_config] do
raise "VERSION is required" if !ENV["VERSION"] || ENV["VERSION"].empty?
- version = ENV["VERSION"] ? ENV["VERSION"].to_i : nil
- ActiveRecord::Migrator.run(:up, ActiveRecord::Tasks::DatabaseTasks.migrations_paths, version)
+ ActiveRecord::Tasks::DatabaseTasks.check_target_version
+
+ ActiveRecord::Migrator.run(
+ :up,
+ ActiveRecord::Tasks::DatabaseTasks.migrations_paths,
+ ActiveRecord::Tasks::DatabaseTasks.target_version
+ )
db_namespace["_dump"].invoke
end
# desc 'Runs the "down" for a given migration VERSION.'
task down: [:environment, :load_config] do
raise "VERSION is required - To go down one migration, use db:rollback" if !ENV["VERSION"] || ENV["VERSION"].empty?
- version = ENV["VERSION"] ? ENV["VERSION"].to_i : nil
- ActiveRecord::Migrator.run(:down, ActiveRecord::Tasks::DatabaseTasks.migrations_paths, version)
+
+ ActiveRecord::Tasks::DatabaseTasks.check_target_version
+
+ ActiveRecord::Migrator.run(
+ :down,
+ ActiveRecord::Tasks::DatabaseTasks.migrations_paths,
+ ActiveRecord::Tasks::DatabaseTasks.target_version
+ )
db_namespace["_dump"].invoke
end
diff --git a/activerecord/lib/active_record/tasks/database_tasks.rb b/activerecord/lib/active_record/tasks/database_tasks.rb
index ff388ff1f6..4657e51e6d 100644
--- a/activerecord/lib/active_record/tasks/database_tasks.rb
+++ b/activerecord/lib/active_record/tasks/database_tasks.rb
@@ -164,13 +164,12 @@ module ActiveRecord
end
def migrate
- raise "Empty VERSION provided" if ENV["VERSION"] && ENV["VERSION"].empty?
+ check_target_version
verbose = ENV["VERBOSE"] ? ENV["VERBOSE"] != "false" : true
- version = ENV["VERSION"] ? ENV["VERSION"].to_i : nil
scope = ENV["SCOPE"]
verbose_was, Migration.verbose = Migration.verbose, verbose
- Migrator.migrate(migrations_paths, version) do |migration|
+ Migrator.migrate(migrations_paths, target_version) do |migration|
scope.blank? || scope == migration.scope
end
ActiveRecord::Base.clear_cache!
@@ -178,6 +177,16 @@ module ActiveRecord
Migration.verbose = verbose_was
end
+ def check_target_version
+ if target_version && !(Migration::MigrationFilenameRegexp.match?(ENV["VERSION"]) || /\A\d+\z/.match?(ENV["VERSION"]))
+ raise "Invalid format of target version: `VERSION=#{ENV['VERSION']}`"
+ end
+ end
+
+ def target_version
+ ENV["VERSION"].to_i if ENV["VERSION"] && !ENV["VERSION"].empty?
+ end
+
def charset_current(environment = env)
charset ActiveRecord::Base.configurations[environment]
end
diff --git a/activerecord/test/cases/migrator_test.rb b/activerecord/test/cases/migrator_test.rb
index ee10be119c..1047ba1367 100644
--- a/activerecord/test/cases/migrator_test.rb
+++ b/activerecord/test/cases/migrator_test.rb
@@ -66,6 +66,26 @@ class MigratorTest < ActiveRecord::TestCase
list = [ActiveRecord::Migration.new("Foo", 1), ActiveRecord::Migration.new("Bar", 2)]
ActiveRecord::Migrator.new(:up, list, 3).run
end
+
+ assert_raises(ActiveRecord::UnknownMigrationVersionError) do
+ list = [ActiveRecord::Migration.new("Foo", 1), ActiveRecord::Migration.new("Bar", 2)]
+ ActiveRecord::Migrator.new(:up, list, -1).run
+ end
+
+ assert_raises(ActiveRecord::UnknownMigrationVersionError) do
+ list = [ActiveRecord::Migration.new("Foo", 1), ActiveRecord::Migration.new("Bar", 2)]
+ ActiveRecord::Migrator.new(:up, list, 0).run
+ end
+
+ assert_raises(ActiveRecord::UnknownMigrationVersionError) do
+ list = [ActiveRecord::Migration.new("Foo", 1), ActiveRecord::Migration.new("Bar", 2)]
+ ActiveRecord::Migrator.new(:up, list, 3).migrate
+ end
+
+ assert_raises(ActiveRecord::UnknownMigrationVersionError) do
+ list = [ActiveRecord::Migration.new("Foo", 1), ActiveRecord::Migration.new("Bar", 2)]
+ ActiveRecord::Migrator.new(:up, list, -1).migrate
+ end
end
def test_finds_migrations
diff --git a/activerecord/test/cases/tasks/database_tasks_test.rb b/activerecord/test/cases/tasks/database_tasks_test.rb
index 1495d2ab89..5a094ead42 100644
--- a/activerecord/test/cases/tasks/database_tasks_test.rb
+++ b/activerecord/test/cases/tasks/database_tasks_test.rb
@@ -357,8 +357,15 @@ module ActiveRecord
ActiveRecord::Migration.expects(:verbose=).with(ActiveRecord::Migration.verbose)
ActiveRecord::Tasks::DatabaseTasks.migrate
+ ENV["VERBOSE"] = ""
+ ENV["VERSION"] = ""
+ ActiveRecord::Migrator.expects(:migrate).with("custom/path", nil)
+ ActiveRecord::Migration.expects(:verbose=).with(true)
+ ActiveRecord::Migration.expects(:verbose=).with(ActiveRecord::Migration.verbose)
+ ActiveRecord::Tasks::DatabaseTasks.migrate
+
ENV["VERBOSE"] = "yes"
- ENV["VERSION"] = "unknown"
+ ENV["VERSION"] = "0"
ActiveRecord::Migrator.expects(:migrate).with("custom/path", 0)
ActiveRecord::Migration.expects(:verbose=).with(true)
ActiveRecord::Migration.expects(:verbose=).with(ActiveRecord::Migration.verbose)
@@ -367,15 +374,47 @@ module ActiveRecord
ENV["VERBOSE"], ENV["VERSION"] = verbose, version
end
- def test_migrate_raise_error_on_empty_version
+ def test_migrate_raise_error_on_invalid_version_format
version = ENV["VERSION"]
- ENV["VERSION"] = ""
+
+ ENV["VERSION"] = "unknown"
+ e = assert_raise(RuntimeError) { ActiveRecord::Tasks::DatabaseTasks.migrate }
+ assert_match(/Invalid format of target version/, e.message)
+
+ ENV["VERSION"] = "0.1.11"
+ e = assert_raise(RuntimeError) { ActiveRecord::Tasks::DatabaseTasks.migrate }
+ assert_match(/Invalid format of target version/, e.message)
+
+ ENV["VERSION"] = "1.1.11"
+ e = assert_raise(RuntimeError) { ActiveRecord::Tasks::DatabaseTasks.migrate }
+ assert_match(/Invalid format of target version/, e.message)
+
+ ENV["VERSION"] = "0 "
+ e = assert_raise(RuntimeError) { ActiveRecord::Tasks::DatabaseTasks.migrate }
+ assert_match(/Invalid format of target version/, e.message)
+
+ ENV["VERSION"] = "1."
+ e = assert_raise(RuntimeError) { ActiveRecord::Tasks::DatabaseTasks.migrate }
+ assert_match(/Invalid format of target version/, e.message)
+
+ ENV["VERSION"] = "1_"
e = assert_raise(RuntimeError) { ActiveRecord::Tasks::DatabaseTasks.migrate }
- assert_equal "Empty VERSION provided", e.message
+ assert_match(/Invalid format of target version/, e.message)
+
+ ENV["VERSION"] = "1_name"
+ e = assert_raise(RuntimeError) { ActiveRecord::Tasks::DatabaseTasks.migrate }
+ assert_match(/Invalid format of target version/, e.message)
ensure
ENV["VERSION"] = version
end
+ def test_migrate_raise_error_on_failed_check_target_version
+ ActiveRecord::Tasks::DatabaseTasks.stubs(:check_target_version).raises("foo")
+
+ e = assert_raise(RuntimeError) { ActiveRecord::Tasks::DatabaseTasks.migrate }
+ assert_equal "foo", e.message
+ end
+
def test_migrate_clears_schema_cache_afterward
ActiveRecord::Base.expects(:clear_cache!)
ActiveRecord::Tasks::DatabaseTasks.migrate
@@ -444,6 +483,108 @@ module ActiveRecord
end
end
+ class DatabaseTaskTargetVersionTest < ActiveRecord::TestCase
+ def test_target_version_returns_nil_if_version_does_not_exist
+ version = ENV.delete("VERSION")
+ assert_nil ActiveRecord::Tasks::DatabaseTasks.target_version
+ ensure
+ ENV["VERSION"] = version
+ end
+
+ def test_target_version_returns_nil_if_version_is_empty
+ version = ENV["VERSION"]
+
+ ENV["VERSION"] = ""
+ assert_nil ActiveRecord::Tasks::DatabaseTasks.target_version
+ ensure
+ ENV["VERSION"] = version
+ end
+
+ def test_target_version_returns_converted_to_integer_env_version_if_version_exists
+ version = ENV["VERSION"]
+
+ ENV["VERSION"] = "0"
+ assert_equal ENV["VERSION"].to_i, ActiveRecord::Tasks::DatabaseTasks.target_version
+
+ ENV["VERSION"] = "42"
+ assert_equal ENV["VERSION"].to_i, ActiveRecord::Tasks::DatabaseTasks.target_version
+
+ ENV["VERSION"] = "042"
+ assert_equal ENV["VERSION"].to_i, ActiveRecord::Tasks::DatabaseTasks.target_version
+ ensure
+ ENV["VERSION"] = version
+ end
+ end
+
+ class DatabaseTaskCheckTargetVersionTest < ActiveRecord::TestCase
+ def test_check_target_version_does_not_raise_error_on_empty_version
+ version = ENV["VERSION"]
+ ENV["VERSION"] = ""
+ assert_nothing_raised { ActiveRecord::Tasks::DatabaseTasks.check_target_version }
+ ensure
+ ENV["VERSION"] = version
+ end
+
+ def test_check_target_version_does_not_raise_error_if_version_is_not_setted
+ version = ENV.delete("VERSION")
+ assert_nothing_raised { ActiveRecord::Tasks::DatabaseTasks.check_target_version }
+ ensure
+ ENV["VERSION"] = version
+ end
+
+ def test_check_target_version_raises_error_on_invalid_version_format
+ version = ENV["VERSION"]
+
+ ENV["VERSION"] = "unknown"
+ e = assert_raise(RuntimeError) { ActiveRecord::Tasks::DatabaseTasks.check_target_version }
+ assert_match(/Invalid format of target version/, e.message)
+
+ ENV["VERSION"] = "0.1.11"
+ e = assert_raise(RuntimeError) { ActiveRecord::Tasks::DatabaseTasks.check_target_version }
+ assert_match(/Invalid format of target version/, e.message)
+
+ ENV["VERSION"] = "1.1.11"
+ e = assert_raise(RuntimeError) { ActiveRecord::Tasks::DatabaseTasks.check_target_version }
+ assert_match(/Invalid format of target version/, e.message)
+
+ ENV["VERSION"] = "0 "
+ e = assert_raise(RuntimeError) { ActiveRecord::Tasks::DatabaseTasks.check_target_version }
+ assert_match(/Invalid format of target version/, e.message)
+
+ ENV["VERSION"] = "1."
+ e = assert_raise(RuntimeError) { ActiveRecord::Tasks::DatabaseTasks.check_target_version }
+ assert_match(/Invalid format of target version/, e.message)
+
+ ENV["VERSION"] = "1_"
+ e = assert_raise(RuntimeError) { ActiveRecord::Tasks::DatabaseTasks.check_target_version }
+ assert_match(/Invalid format of target version/, e.message)
+
+ ENV["VERSION"] = "1_name"
+ e = assert_raise(RuntimeError) { ActiveRecord::Tasks::DatabaseTasks.check_target_version }
+ assert_match(/Invalid format of target version/, e.message)
+ ensure
+ ENV["VERSION"] = version
+ end
+
+ def test_check_target_version_does_not_raise_error_on_valid_version_format
+ version = ENV["VERSION"]
+
+ ENV["VERSION"] = "0"
+ assert_nothing_raised { ActiveRecord::Tasks::DatabaseTasks.check_target_version }
+
+ ENV["VERSION"] = "1"
+ assert_nothing_raised { ActiveRecord::Tasks::DatabaseTasks.check_target_version }
+
+ ENV["VERSION"] = "001"
+ assert_nothing_raised { ActiveRecord::Tasks::DatabaseTasks.check_target_version }
+
+ ENV["VERSION"] = "001_name.rb"
+ assert_nothing_raised { ActiveRecord::Tasks::DatabaseTasks.check_target_version }
+ ensure
+ ENV["VERSION"] = version
+ end
+ end
+
class DatabaseTasksStructureDumpTest < ActiveRecord::TestCase
include DatabaseTasksSetupper
--
cgit v1.2.3
From 3f1695bb9c008b7cb1840e09e640f3ec0c59a564 Mon Sep 17 00:00:00 2001
From: Ryuta Kamizono <kamipo@gmail.com>
Date: Tue, 7 Nov 2017 08:44:44 +0900
Subject: Remove useless `associated_records_by_owner`
`associated_records_by_owner` had returned customizing result before
calling `associate_records_to_owner` for through association subclasses.
Since #22115, `associate_records_to_owner` is called in the method and
not returned owner and result pairs. Removing the method will reduce
method call and block call nesting.
---
.../associations/preloader/association.rb | 22 ++++++++--------------
.../associations/preloader/through_association.rb | 4 ++--
2 files changed, 10 insertions(+), 16 deletions(-)
(limited to 'activerecord')
diff --git a/activerecord/lib/active_record/associations/preloader/association.rb b/activerecord/lib/active_record/associations/preloader/association.rb
index b0c41c7d5f..e77761692d 100644
--- a/activerecord/lib/active_record/associations/preloader/association.rb
+++ b/activerecord/lib/active_record/associations/preloader/association.rb
@@ -17,8 +17,14 @@ module ActiveRecord
end
def run(preloader)
- associated_records_by_owner(preloader) do |owner, records|
- associate_records_to_owner(owner, records)
+ records = load_records do |record|
+ owner = owners_by_key[convert_key(record[association_key_name])]
+ association = owner.association(reflection.name)
+ association.set_inverse_instance(record)
+ end
+
+ owners.each do |owner|
+ associate_records_to_owner(owner, records[convert_key(owner[owner_key_name])] || [])
end
end
@@ -33,18 +39,6 @@ module ActiveRecord
reflection.join_foreign_key
end
- def associated_records_by_owner(preloader)
- records = load_records do |record|
- owner = owners_by_key[convert_key(record[association_key_name])]
- association = owner.association(reflection.name)
- association.set_inverse_instance(record)
- end
-
- owners.each_with_object({}) do |owner, result|
- yield(owner, records[convert_key(owner[owner_key_name])] || [])
- end
- end
-
def associate_records_to_owner(owner, records)
raise NotImplementedError
end
diff --git a/activerecord/lib/active_record/associations/preloader/through_association.rb b/activerecord/lib/active_record/associations/preloader/through_association.rb
index 1bb7e98231..b1813ba66b 100644
--- a/activerecord/lib/active_record/associations/preloader/through_association.rb
+++ b/activerecord/lib/active_record/associations/preloader/through_association.rb
@@ -12,7 +12,7 @@ module ActiveRecord
reflection.source_reflection
end
- def associated_records_by_owner(preloader)
+ def run(preloader)
already_loaded = owners.first.association(through_reflection.name).loaded?
through_scope = through_scope()
reflection_scope = target_reflection_scope
@@ -43,7 +43,7 @@ module ActiveRecord
result.sort_by! { |rhs| preload_index[rhs] } if reflection_scope.order_values.any?
result.uniq! if reflection_scope.distinct_value
end
- yield(owner, result)
+ associate_records_to_owner(owner, result)
end
end
--
cgit v1.2.3
From 0c2cb880e34c943275758ca6a6ff84afa7a29fba Mon Sep 17 00:00:00 2001
From: Ryuta Kamizono <kamipo@gmail.com>
Date: Wed, 8 Nov 2017 20:08:19 +0900
Subject: Don't expose internal methods in `Preloader::ThroughAssociation`
`through_reflection` and `source_reflection` are used only in the class.
---
.../associations/preloader/through_association.rb | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
(limited to 'activerecord')
diff --git a/activerecord/lib/active_record/associations/preloader/through_association.rb b/activerecord/lib/active_record/associations/preloader/through_association.rb
index b1813ba66b..762275fbad 100644
--- a/activerecord/lib/active_record/associations/preloader/through_association.rb
+++ b/activerecord/lib/active_record/associations/preloader/through_association.rb
@@ -4,14 +4,6 @@ module ActiveRecord
module Associations
class Preloader
module ThroughAssociation #:nodoc:
- def through_reflection
- reflection.through_reflection
- end
-
- def source_reflection
- reflection.source_reflection
- end
-
def run(preloader)
already_loaded = owners.first.association(through_reflection.name).loaded?
through_scope = through_scope()
@@ -48,6 +40,13 @@ module ActiveRecord
end
private
+ def through_reflection
+ reflection.through_reflection
+ end
+
+ def source_reflection
+ reflection.source_reflection
+ end
def preload_index
@preload_index ||= @preloaded_records.each_with_object({}).with_index do |(id, result), index|
--
cgit v1.2.3
From 65aa0b7e3448f849a91b6f510b78d4303ff44dbc Mon Sep 17 00:00:00 2001
From: Ryuta Kamizono <kamipo@gmail.com>
Date: Wed, 8 Nov 2017 20:28:52 +0900
Subject: Don't expose accessors which are internal used only
---
activerecord/lib/active_record/associations/preloader.rb | 5 +++--
activerecord/lib/active_record/associations/preloader/association.rb | 4 +++-
2 files changed, 6 insertions(+), 3 deletions(-)
(limited to 'activerecord')
diff --git a/activerecord/lib/active_record/associations/preloader.rb b/activerecord/lib/active_record/associations/preloader.rb
index e1754d4a19..5a93a89d0a 100644
--- a/activerecord/lib/active_record/associations/preloader.rb
+++ b/activerecord/lib/active_record/associations/preloader.rb
@@ -166,8 +166,6 @@ module ActiveRecord
end
class AlreadyLoaded # :nodoc:
- attr_reader :owners, :reflection
-
def initialize(klass, owners, reflection, preload_scope)
@owners = owners
@reflection = reflection
@@ -178,6 +176,9 @@ module ActiveRecord
def preloaded_records
owners.flat_map { |owner| owner.association(reflection.name).target }
end
+
+ protected
+ attr_reader :owners, :reflection
end
# Returns a class containing the logic needed to load preload the data
diff --git a/activerecord/lib/active_record/associations/preloader/association.rb b/activerecord/lib/active_record/associations/preloader/association.rb
index e77761692d..19c337dc39 100644
--- a/activerecord/lib/active_record/associations/preloader/association.rb
+++ b/activerecord/lib/active_record/associations/preloader/association.rb
@@ -4,7 +4,6 @@ module ActiveRecord
module Associations
class Preloader
class Association #:nodoc:
- attr_reader :owners, :reflection, :preload_scope, :model, :klass
attr_reader :preloaded_records
def initialize(klass, owners, reflection, preload_scope)
@@ -28,6 +27,9 @@ module ActiveRecord
end
end
+ protected
+ attr_reader :owners, :reflection, :preload_scope, :model, :klass
+
private
# The name of the key on the associated records
def association_key_name
--
cgit v1.2.3
From af91eff3fa395a0a57f8002b3b92e5d053cd2d7d Mon Sep 17 00:00:00 2001
From: Ryuta Kamizono <kamipo@gmail.com>
Date: Thu, 9 Nov 2017 05:08:26 +0900
Subject: Consolidate redundant `if` and `unless` with the same condition
---
activerecord/test/cases/transaction_isolation_test.rb | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
(limited to 'activerecord')
diff --git a/activerecord/test/cases/transaction_isolation_test.rb b/activerecord/test/cases/transaction_isolation_test.rb
index b1ebccdcc3..eaafd13360 100644
--- a/activerecord/test/cases/transaction_isolation_test.rb
+++ b/activerecord/test/cases/transaction_isolation_test.rb
@@ -15,9 +15,7 @@ unless ActiveRecord::Base.connection.supports_transaction_isolation?
end
end
end
-end
-
-if ActiveRecord::Base.connection.supports_transaction_isolation?
+else
class TransactionIsolationTest < ActiveRecord::TestCase
self.use_transactional_tests = false
--
cgit v1.2.3
From 89603b41edbaaed5cab4964a30caa8bda9285879 Mon Sep 17 00:00:00 2001
From: Ryuta Kamizono <kamipo@gmail.com>
Date: Thu, 9 Nov 2017 05:27:30 +0900
Subject: `Mysql2Adapter` should pass `ConcurrentTransactionTest`
---
activerecord/test/cases/transactions_test.rb | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
(limited to 'activerecord')
diff --git a/activerecord/test/cases/transactions_test.rb b/activerecord/test/cases/transactions_test.rb
index 7fd125ab74..91032945a6 100644
--- a/activerecord/test/cases/transactions_test.rb
+++ b/activerecord/test/cases/transactions_test.rb
@@ -954,7 +954,7 @@ class TransactionsWithTransactionalFixturesTest < ActiveRecord::TestCase
end
end if Topic.connection.supports_savepoints?
-if current_adapter?(:PostgreSQLAdapter)
+if current_adapter?(:PostgreSQLAdapter, :Mysql2Adapter)
class ConcurrentTransactionTest < TransactionTest
# This will cause transactions to overlap and fail unless they are performed on
# separate database connections.
--
cgit v1.2.3
From 801fbde87deae5c01c581a6e4ef1157d27185b32 Mon Sep 17 00:00:00 2001
From: Yasuo Honda <yasuo.honda@gmail.com>
Date: Wed, 8 Nov 2017 21:22:55 +0000
Subject: Run `ConcurrentTransactionTest` if `supports_transaction_isolation?`
returns true
Not only postgresql or mysql2 adapter, Oracle enhanced adapter
whose default isolation level is read commited, passes these two test cases.
`ConcurrentTransactionTest#test_transaction_per_thread`
`ConcurrentTransactionTest#test_transaction_isolation__read_committed`
```ruby
$ ARCONN=oracle bin/test test/cases/transactions_test.rb:961 -v
Using oracle
Run options: -v --seed 18865
ConcurrentTransactionTest#test_transaction_per_thread = 0.98 s = .
Finished in 1.061036s, 0.9425 runs/s, 5.6549 assertions/s.
1 runs, 6 assertions, 0 failures, 0 errors, 0 skips
```
```ruby
$ ARCONN=oracle bin/test test/cases/transactions_test.rb:979 -v
Using oracle
Run options: -v --seed 13341
ConcurrentTransactionTest#test_transaction_isolation__read_committed = 1.85 s = .
Finished in 1.928637s, 0.5185 runs/s, 10.3700 assertions/s.
1 runs, 20 assertions, 0 failures, 0 errors, 0 skips
$
```
Also, regardless it is a file based or memory based these tests could fail with SQLite3Adapter.
(Extra CR added to make lines shorter)
```ruby
$ ARCONN=sqlite3 bin/test test/cases/transactions_test.rb:961 -v
Using sqlite3
Run options: -v --seed 18815
ConcurrentTransactionTest#test_transaction_per_thread = /home/yahonda/.rbenv/versions/2.4.1/lib/ruby/gems/2.4.0/gems/sqlite3-1.3.13/lib/sqlite3/statement.rb:108:in `step':
SQLite3::BusyException: database is locked: UPDATE "topics" SET "approved" = ?, "updated_at" = ? WHERE "topics"."id" = ? (ActiveRecord::StatementInvalid)
```
```ruby
$ ARCONN=sqlite3 bin/test test/cases/transactions_test.rb:979 -v
Using sqlite3
Run options: -v --seed 25520
ConcurrentTransactionTest#test_transaction_isolation__read_committed = 0.12 s = E
/home/yahonda/.rbenv/versions/2.4.1/lib/ruby/gems/2.4.0/gems/sqlite3-1.3.13/lib/sqlite3/statement.rb:108:in `step':
SQLite3::BusyException: database is locked: UPDATE "developers" SET "salary" = ?, "updated_at" = ?, "updated_on" = ? WHERE "developers"."id" = ? (ActiveRecord::StatementInvalid)
```
---
activerecord/test/cases/transactions_test.rb | 28 +++++++++++++---------------
1 file changed, 13 insertions(+), 15 deletions(-)
(limited to 'activerecord')
diff --git a/activerecord/test/cases/transactions_test.rb b/activerecord/test/cases/transactions_test.rb
index 91032945a6..5c8ae4d3cb 100644
--- a/activerecord/test/cases/transactions_test.rb
+++ b/activerecord/test/cases/transactions_test.rb
@@ -954,27 +954,25 @@ class TransactionsWithTransactionalFixturesTest < ActiveRecord::TestCase
end
end if Topic.connection.supports_savepoints?
-if current_adapter?(:PostgreSQLAdapter, :Mysql2Adapter)
+if ActiveRecord::Base.connection.supports_transaction_isolation?
class ConcurrentTransactionTest < TransactionTest
# This will cause transactions to overlap and fail unless they are performed on
# separate database connections.
- unless in_memory_db?
- def test_transaction_per_thread
- threads = 3.times.map do
- Thread.new do
- Topic.transaction do
- topic = Topic.find(1)
- topic.approved = !topic.approved?
- assert topic.save!
- topic.approved = !topic.approved?
- assert topic.save!
- end
- Topic.connection.close
+ def test_transaction_per_thread
+ threads = 3.times.map do
+ Thread.new do
+ Topic.transaction do
+ topic = Topic.find(1)
+ topic.approved = !topic.approved?
+ assert topic.save!
+ topic.approved = !topic.approved?
+ assert topic.save!
end
+ Topic.connection.close
end
-
- threads.each(&:join)
end
+
+ threads.each(&:join)
end
# Test for dirty reads among simultaneous transactions.
--
cgit v1.2.3
From be6e1b8f7dbce1940f47339657faab2c1fdeaa54 Mon Sep 17 00:00:00 2001
From: Ryuta Kamizono <kamipo@gmail.com>
Date: Thu, 9 Nov 2017 16:53:55 +0900
Subject: Should pass `test_no_locks_no_wait` not only on `PostgreSQLAdapter`
and `OracleAdapter`
---
activerecord/test/cases/locking_test.rb | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
(limited to 'activerecord')
diff --git a/activerecord/test/cases/locking_test.rb b/activerecord/test/cases/locking_test.rb
index 6791d50940..e857180bd1 100644
--- a/activerecord/test/cases/locking_test.rb
+++ b/activerecord/test/cases/locking_test.rb
@@ -611,14 +611,12 @@ unless in_memory_db?
end
end
- if current_adapter?(:PostgreSQLAdapter, :OracleAdapter)
- def test_no_locks_no_wait
- first, second = duel { Person.find 1 }
- assert first.end > second.end
- end
-
- private
+ def test_no_locks_no_wait
+ first, second = duel { Person.find 1 }
+ assert first.end > second.end
+ end
+ private
def duel(zzz = 5)
t0, t1, t2, t3 = nil, nil, nil, nil
@@ -646,6 +644,5 @@ unless in_memory_db?
assert t3 > t2
[t0.to_f..t1.to_f, t2.to_f..t3.to_f]
end
- end
end
end
--
cgit v1.2.3