From d147b956ebcf32b65e80e990a14332630c87c31b Mon Sep 17 00:00:00 2001 From: Abdelkader Boudih Date: Fri, 19 Jan 2024 00:48:30 +0100 Subject: [PATCH] fix --- lib/closure_tree/has_closure_tree.rb | 4 - lib/closure_tree/has_closure_tree_root.rb | 2 - lib/closure_tree/numeric_order_support.rb | 78 +++++++------- lib/closure_tree/support.rb | 7 +- spec/spec_helper.rb | 7 +- spec/support/models.rb | 13 +++ spec/support/schema.rb | 121 +++++++++++----------- test/closure_tree/pool_test.rb | 7 +- test/test_helper.rb | 6 +- 9 files changed, 125 insertions(+), 120 deletions(-) diff --git a/lib/closure_tree/has_closure_tree.rb b/lib/closure_tree/has_closure_tree.rb index b0bc5b1a..a45462cc 100644 --- a/lib/closure_tree/has_closure_tree.rb +++ b/lib/closure_tree/has_closure_tree.rb @@ -31,10 +31,6 @@ def has_closure_tree(options = {}) include ClosureTree::DeterministicOrdering if _ct.order_option? include ClosureTree::NumericDeterministicOrdering if _ct.order_is_numeric? - - connection_pool.release_connection - rescue StandardError => e - raise e unless ClosureTree.configuration.database_less end alias_method :acts_as_tree, :has_closure_tree diff --git a/lib/closure_tree/has_closure_tree_root.rb b/lib/closure_tree/has_closure_tree_root.rb index 70e327d1..26f9d535 100644 --- a/lib/closure_tree/has_closure_tree_root.rb +++ b/lib/closure_tree/has_closure_tree_root.rb @@ -77,8 +77,6 @@ def has_closure_tree_root(assoc_name, options = {}) @closure_tree_roots[assoc_name][assoc_map] = root end - - connection_pool.release_connection end end end diff --git a/lib/closure_tree/numeric_order_support.rb b/lib/closure_tree/numeric_order_support.rb index 0223a415..351720a7 100644 --- a/lib/closure_tree/numeric_order_support.rb +++ b/lib/closure_tree/numeric_order_support.rb @@ -1,53 +1,42 @@ module ClosureTree module NumericOrderSupport - - def self.adapter_for_connection(connection) - das = WithAdvisoryLock::DatabaseAdapterSupport.new(connection) - if das.postgresql? - ::ClosureTree::NumericOrderSupport::PostgreSQLAdapter - elsif das.mysql? - ::ClosureTree::NumericOrderSupport::MysqlAdapter - else - ::ClosureTree::NumericOrderSupport::GenericAdapter - end - end - module MysqlAdapter - def reorder_with_parent_id(parent_id, minimum_sort_order_value = nil) - return if parent_id.nil? && dont_order_roots + module_function def reorder_with_parent_id(ct, parent_id, minimum_sort_order_value = nil) + return if parent_id.nil? && ct.dont_order_roots + min_where = if minimum_sort_order_value - "AND #{quoted_order_column} >= #{minimum_sort_order_value}" + "AND #{ct.quoted_order_column} >= #{minimum_sort_order_value}" else "" end - connection.execute 'SET @i = 0' - connection.execute <<-SQL.squish - UPDATE #{quoted_table_name} - SET #{quoted_order_column} = (@i := @i + 1) + #{minimum_sort_order_value.to_i - 1} - WHERE #{where_eq(parent_column_name, parent_id)} #{min_where} - ORDER BY #{nulls_last_order_by} + ct.connection.execute 'SET @i = 0' + ct.connection.execute <<-SQL.squish + UPDATE #{ct.quoted_table_name} + SET #{ct.quoted_order_column} = (@i := @i + 1) + #{minimum_sort_order_value.to_i - 1} + WHERE #{ct.where_eq(ct.parent_column_name, parent_id)} #{min_where} + ORDER BY #{ct.nulls_last_order_by} SQL end end module PostgreSQLAdapter - def reorder_with_parent_id(parent_id, minimum_sort_order_value = nil) - return if parent_id.nil? && dont_order_roots + module_function def reorder_with_parent_id(ct, parent_id, minimum_sort_order_value = nil) + return if parent_id.nil? && ct.dont_order_roots min_where = if minimum_sort_order_value - "AND #{quoted_order_column} >= #{minimum_sort_order_value}" + "AND #{ct.quoted_order_column} >= #{minimum_sort_order_value}" else "" end - connection.execute <<-SQL.squish - UPDATE #{quoted_table_name} - SET #{quoted_order_column(false)} = t.seq + #{minimum_sort_order_value.to_i - 1} + ct.connection.execute <<-SQL.squish + UPDATE #{ct.quoted_table_name} + SET #{ct.quoted_order_column(false)} = t.seq + #{minimum_sort_order_value.to_i - 1} FROM ( - SELECT #{quoted_id_column_name} AS id, row_number() OVER(ORDER BY #{order_by}) AS seq - FROM #{quoted_table_name} - WHERE #{where_eq(parent_column_name, parent_id)} #{min_where} + SELECT #{ct.quoted_id_column_name} AS id, row_number() OVER(ORDER BY #{ct.order_by}) AS seq + FROM #{ct.quoted_table_name} + WHERE #{ct.where_eq(ct.parent_column_name, parent_id)} #{min_where} ) AS t - WHERE #{quoted_table_name}.#{quoted_id_column_name} = t.id and - #{quoted_table_name}.#{quoted_order_column(false)} is distinct from t.seq + #{minimum_sort_order_value.to_i - 1} + WHERE #{ct.quoted_table_name}.#{ct.quoted_id_column_name} = t.id and + #{ct.quoted_table_name}.#{ct.quoted_order_column(false)} is distinct from t.seq + #{minimum_sort_order_value.to_i - 1} SQL end @@ -57,18 +46,31 @@ def rows_updated(result) end module GenericAdapter - def reorder_with_parent_id(parent_id, minimum_sort_order_value = nil) - return if parent_id.nil? && dont_order_roots - scope = model_class. - where(parent_column_sym => parent_id). - order(nulls_last_order_by) + module_function def reorder_with_parent_id(ct, parent_id, minimum_sort_order_value = nil) + return if parent_id.nil? && ct.dont_order_roots + binding.irb + scope = ct. + where(ct.parent_column_sym => parent_id). + order(ct.nulls_last_order_by) if minimum_sort_order_value - scope = scope.where("#{quoted_order_column} >= #{minimum_sort_order_value}") + scope = scope.where("#{ct.quoted_order_column} >= #{minimum_sort_order_value}") end scope.each_with_index do |ea, idx| ea.update_order_value(idx + minimum_sort_order_value.to_i) end end end + + + module_function def adapter_for_connection(ct, parent_id, minimum_sort_order_value = nil) + das = WithAdvisoryLock::DatabaseAdapterSupport.new(ct.connection) + if das.postgresql? + PostgreSQLAdapter.reorder_with_parent_id(ct, parent_id, minimum_sort_order_value = nil) + elsif das.mysql? + MysqlAdapter.reorder_with_parent_id(ct, parent_id, minimum_sort_order_value = nil) + else + GenericAdapter.reorder_with_parent_id(ct, parent_id, minimum_sort_order_value = nil) + end + end end end diff --git a/lib/closure_tree/support.rb b/lib/closure_tree/support.rb index e887090e..0e406bd9 100644 --- a/lib/closure_tree/support.rb +++ b/lib/closure_tree/support.rb @@ -26,9 +26,6 @@ def initialize(model_class, options) :numeric_order => false }.merge(options) raise ArgumentError, "name_column can't be 'path'" if options[:name_column] == 'path' - if order_is_numeric? - extend NumericOrderSupport.adapter_for_connection(connection) - end end def hierarchy_class_for_model @@ -54,6 +51,10 @@ def hash hierarchy_class end + def reorder_with_parent_id(parent_id, minimum_sort_order_value = nil) + NumericOrderSupport.adapter_for_connection(self, parent_id, minimum_sort_order_value) + end + def hierarchy_table_name # We need to use the table_name, not something like ct_class.to_s.demodulize + "_hierarchies", # because they may have overridden the table name, which is what we want to be consistent with diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 1c02f255..1803df2d 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -29,6 +29,7 @@ ActiveRecord::Base.configurations = { default_env: { primary: { + primary: true, url: primary_database_url, properties: { allowPublicKeyRetrieval: true } # for JRuby madness }, @@ -99,14 +100,8 @@ def sqlite? # Require our gem require 'closure_tree' -ActiveRecord::Tasks::DatabaseTasks.drop_current(:primary) -ActiveRecord::Tasks::DatabaseTasks.create_current(:primary) -ActiveRecord::Tasks::DatabaseTasks.drop_current(:secondary) -ActiveRecord::Tasks::DatabaseTasks.create_current(:secondary) - # Load test helpers require_relative 'support/schema' -require_relative 'support/models' require_relative 'support/helpers' require_relative 'support/exceed_query_limit' require_relative 'support/query_counter' diff --git a/spec/support/models.rb b/spec/support/models.rb index eee68410..cb937a15 100644 --- a/spec/support/models.rb +++ b/spec/support/models.rb @@ -1,5 +1,18 @@ # frozen_string_literal: true + + +class ApplicationRecord < ActiveRecord::Base + self.abstract_class = true + + connects_to database: { writing: :primary, reading: :primary } +end + +class SecondDatabaseRecord < ActiveRecord::Base + self.abstract_class = true + + connects_to database: { writing: :secondary, reading: :secondary } +end class Tag < ApplicationRecord has_closure_tree dependent: :destroy, order: :name before_destroy :add_destroyed_tag diff --git a/spec/support/schema.rb b/spec/support/schema.rb index 665f46fb..95cbdf19 100644 --- a/spec/support/schema.rb +++ b/spec/support/schema.rb @@ -1,19 +1,21 @@ # frozen_string_literal: true -class ApplicationRecord < ActiveRecord::Base - self.abstract_class = true - - connects_to database: { writing: :primary, reading: :primary } -end - -class SecondDatabaseRecord < ActiveRecord::Base - self.abstract_class = true - - connects_to database: { writing: :secondary, reading: :secondary } +require_relative 'models' +ApplicationRecord.establish_connection +if sqlite? + ActiveRecord::Tasks::DatabaseTasks.drop(:primary, 'test') + ActiveRecord::Tasks::DatabaseTasks.create(:primary, 'test') + ActiveRecord::Tasks::DatabaseTasks.drop(:secondary, 'test') + ActiveRecord::Tasks::DatabaseTasks.create(:secondary, 'test') +else + ActiveRecord::Tasks::DatabaseTasks.drop(:primary) + ActiveRecord::Tasks::DatabaseTasks.create(:primary) + ActiveRecord::Tasks::DatabaseTasks.drop(:secondary) + ActiveRecord::Tasks::DatabaseTasks.create(:secondary) end ActiveRecord::Schema.define(version: 0) do - connection.create_table 'tags', force: :cascade do |t| + connection.create_table Tag.table_name, force: :cascade do |t| t.string 'name' t.string 'title' t.references 'parent' @@ -21,13 +23,17 @@ class SecondDatabaseRecord < ActiveRecord::Base t.timestamps null: false end - create_table 'tag_hierarchies', id: false, force: :cascade do |t| + create_table Tag.hierarchy_class.table_name, id: false, force: :cascade do |t| t.references 'ancestor', null: false t.references 'descendant', null: false t.integer 'generations', null: false end - create_table 'uuid_tags', id: false, force: :cascade do |t| + add_index Tag.hierarchy_class.table_name, %i[ancestor_id descendant_id generations], unique: true, + name: 'tag_anc_desc_idx' + add_index Tag.hierarchy_class.table_name, [:descendant_id], name: 'tag_desc_idx' + + create_table UUIDTag.table_name, id: false, force: :cascade do |t| t.string 'uuid', primary_key: true t.string 'name' t.string 'title' @@ -36,95 +42,99 @@ class SecondDatabaseRecord < ActiveRecord::Base t.timestamps null: false end - create_table 'uuid_tag_hierarchies', id: false, force: :cascade do |t| + create_table UUIDTag.hierarchy_class.table_name, id: false, force: :cascade do |t| t.string 'ancestor_id', null: false t.string 'descendant_id', null: false t.integer 'generations', null: false end - create_table 'destroyed_tags', force: :cascade do |t| + create_table DestroyedTag.table_name, force: :cascade do |t| t.string 'name' end - add_index 'tag_hierarchies', %i[ancestor_id descendant_id generations], unique: true, - name: 'tag_anc_desc_idx' - add_index 'tag_hierarchies', [:descendant_id], name: 'tag_desc_idx' - - create_table 'groups', force: :cascade do |t| + create_table Group.table_name, force: :cascade do |t| t.string 'name', null: false end - create_table 'groupings', force: :cascade do |t| + create_table Grouping.table_name, force: :cascade do |t| t.string 'name', null: false end - create_table 'user_sets', force: :cascade do |t| + create_table UserSet.table_name, force: :cascade do |t| t.string 'name', null: false end - create_table 'teams', force: :cascade do |t| + create_table Team.table_name, force: :cascade do |t| t.string 'name', null: false end - create_table 'users', force: :cascade do |t| + create_table User.table_name, force: :cascade do |t| t.string 'email' t.references 'referrer' t.integer 'group_id' t.timestamps null: false end - create_table 'contracts', force: :cascade do |t| + create_table User.hierarchy_class.table_name, id: false, force: :cascade do |t| + t.references 'ancestor', null: false + t.references 'descendant', null: false + t.integer 'generations', null: false + end + + add_index User.hierarchy_class.table_name, %i[ancestor_id descendant_id generations], unique: true, + name: 'ref_anc_desc_idx' + add_index User.hierarchy_class.table_name, [:descendant_id], name: 'ref_desc_idx' + + create_table Contract.table_name, force: :cascade do |t| t.references 'user', null: false t.references 'contract_type' t.string 'title' end - create_table 'contract_types', force: :cascade do |t| + create_table ContractType.table_name, force: :cascade do |t| t.string 'name', null: false end - create_table 'referral_hierarchies', id: false, force: :cascade do |t| - t.references 'ancestor', null: false - t.references 'descendant', null: false - t.integer 'generations', null: false - end - - create_table 'labels', force: :cascade do |t| + create_table Label.table_name, force: :cascade do |t| t.string 'name' t.string 'type' t.integer 'column_whereby_ordering_is_inferred' t.references 'mother' end - create_table 'label_hierarchies', id: false, force: :cascade do |t| + create_table Label.hierarchy_class.table_name, id: false, force: :cascade do |t| t.references 'ancestor', null: false t.references 'descendant', null: false t.integer 'generations', null: false end - create_table 'cuisine_types', force: :cascade do |t| + add_index Label.hierarchy_class.table_name, %i[ancestor_id descendant_id generations], unique: true, + name: 'lh_anc_desc_idx' + add_index Label.hierarchy_class.table_name, [:descendant_id], name: 'lh_desc_idx' + + create_table CuisineType.table_name, force: :cascade do |t| t.string 'name' t.references 'parent' end - create_table 'cuisine_type_hierarchies', id: false, force: :cascade do |t| + create_table CuisineType.hierarchy_class.table_name, id: false, force: :cascade do |t| t.references 'ancestor', null: false t.references 'descendant', null: false t.integer 'generations', null: false end - create_table 'namespace_types', force: :cascade do |t| + create_table Namespace::Type.table_name, force: :cascade do |t| t.string 'name' t.references 'parent' end - create_table 'namespace_type_hierarchies', id: false, force: :cascade do |t| + create_table Namespace::Type.hierarchy_class.table_name, id: false, force: :cascade do |t| t.references 'ancestor', null: false t.references 'descendant', null: false t.integer 'generations', null: false end - create_table 'metal', force: :cascade do |t| + create_table Metal.table_name, force: :cascade do |t| t.references 'parent' t.string 'metal_type' t.string 'value' @@ -132,43 +142,38 @@ class SecondDatabaseRecord < ActiveRecord::Base t.integer 'sort_order' end - create_table 'metal_hierarchies', id: false, force: :cascade do |t| + create_table Metal.hierarchy_class.table_name, id: false, force: :cascade do |t| t.references 'ancestor', null: false t.references 'descendant', null: false t.integer 'generations', null: false end - add_index 'label_hierarchies', %i[ancestor_id descendant_id generations], unique: true, - name: 'lh_anc_desc_idx' - add_index 'label_hierarchies', [:descendant_id], name: 'lh_desc_idx' - add_index 'referral_hierarchies', %i[ancestor_id descendant_id generations], unique: true, - name: 'ref_anc_desc_idx' - add_index 'referral_hierarchies', [:descendant_id], name: 'ref_desc_idx' - - add_foreign_key(:tags, :tags, column: 'parent_id', on_delete: :cascade) - add_foreign_key(:users, :users, column: 'referrer_id', on_delete: :cascade) - add_foreign_key(:labels, :labels, column: 'mother_id', on_delete: :cascade) - add_foreign_key(:metal, :metal, column: 'parent_id', on_delete: :cascade) - add_foreign_key(:tag_hierarchies, :tags, column: 'ancestor_id', on_delete: :cascade) - add_foreign_key(:tag_hierarchies, :tags, column: 'descendant_id', on_delete: :cascade) + add_foreign_key(Tag.table_name, Tag.table_name, column: 'parent_id', on_delete: :cascade) + add_foreign_key(User.table_name, User.table_name, column: 'referrer_id', on_delete: :cascade) + add_foreign_key(Label.table_name, Label.table_name, column: 'mother_id', on_delete: :cascade) + add_foreign_key(Metal.table_name, Metal.table_name, column: 'parent_id', on_delete: :cascade) + add_foreign_key(Tag.hierarchy_class.table_name, Tag.table_name, column: 'ancestor_id', on_delete: :cascade) + add_foreign_key(Tag.hierarchy_class.table_name, Tag.table_name, column: 'descendant_id', on_delete: :cascade) end SecondDatabaseRecord.establish_connection SecondDatabaseRecord.connection_pool.with_connection do |connection| ActiveRecord::Schema.define(version: 0) do - connection.create_table 'menu_items', force: :cascade do |t| + connection.create_table MenuItem.table_name, force: :cascade do |t| t.string 'name' t.references 'parent' t.timestamps null: false end - connection.create_table 'menu_item_hierarchies', id: false, force: :cascade do |t| + connection.create_table MenuItem.hierarchy_class.table_name, id: false, force: :cascade do |t| t.references 'ancestor', null: false t.references 'descendant', null: false t.integer 'generations', null: false end - connection.add_foreign_key(:menu_items, :menu_items, column: 'parent_id', on_delete: :cascade) - connection.add_foreign_key(:menu_item_hierarchies, :menu_items, column: 'ancestor_id', on_delete: :cascade) - connection.add_foreign_key(:menu_item_hierarchies, :menu_items, column: 'descendant_id', on_delete: :cascade) + connection.add_foreign_key(MenuItem.table_name, MenuItem.table_name, column: 'parent_id', on_delete: :cascade) + connection.add_foreign_key(MenuItem.hierarchy_class.table_name, MenuItem.table_name, column: 'ancestor_id', + on_delete: :cascade) + connection.add_foreign_key(MenuItem.hierarchy_class.table_name, MenuItem.table_name, column: 'descendant_id', + on_delete: :cascade) end end diff --git a/test/closure_tree/pool_test.rb b/test/closure_tree/pool_test.rb index db5dfc61..0250cf7e 100644 --- a/test/closure_tree/pool_test.rb +++ b/test/closure_tree/pool_test.rb @@ -9,8 +9,7 @@ class TypeDuplicate < ActiveRecord::Base has_closure_tree end - refute ActiveRecord::Base.connection_pool.active_connection? - # +false+ in AR 4, +nil+ in AR 5 + assert_nil TypeDuplicate.connection_pool.active_connection? end it 'returns connection to the pool after has_closure_tree setup with order' do @@ -19,7 +18,7 @@ class MetalDuplicate < ActiveRecord::Base has_closure_tree order: 'sort_order', name_column: 'value' end - refute ActiveRecord::Base.connection_pool.active_connection? + refute MetalDuplicate.connection_pool.active_connection? end it 'returns connection to the pool after has_closure_tree_root setup' do @@ -28,6 +27,6 @@ class GroupDuplicate < ActiveRecord::Base has_closure_tree_root :root_user end - refute ActiveRecord::Base.connection_pool.active_connection? + refute GroupDuplicate.connection_pool.active_connection? end end diff --git a/test/test_helper.rb b/test/test_helper.rb index f1c547a3..06053b9f 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -67,12 +67,8 @@ class Spec Thread.abort_on_exception = true require 'closure_tree' -ActiveRecord::Tasks::DatabaseTasks.drop_current(:primary) -ActiveRecord::Tasks::DatabaseTasks.create_current(:primary) -ActiveRecord::Tasks::DatabaseTasks.drop_current(:secondary) -ActiveRecord::Tasks::DatabaseTasks.create_current(:secondary) + require_relative '../spec/support/schema' -require_relative '../spec/support/models' puts "Testing with #{env_db} database"