From 83193bd99dc5f34e876837a523f8cdbdfee49774 Mon Sep 17 00:00:00 2001 From: Derek Etherton Date: Wed, 12 Mar 2025 13:00:14 -0700 Subject: [PATCH 01/13] apply patch for root ordering scope (https://github.com/ClosureTree/closure_tree/pull/442) --- lib/closure_tree/has_closure_tree.rb | 3 ++- lib/closure_tree/model.rb | 4 ++++ .../numeric_deterministic_ordering.rb | 6 ++--- lib/closure_tree/numeric_order_support.rb | 21 +++++++++++++----- lib/closure_tree/support_attributes.rb | 8 +++++++ spec/closure_tree/root_ordering.rb | 22 +++++++++++++++++++ spec/support/models.rb | 7 ++++++ spec/support/schema.rb | 8 +++++++ 8 files changed, 70 insertions(+), 9 deletions(-) create mode 100644 spec/closure_tree/root_ordering.rb diff --git a/lib/closure_tree/has_closure_tree.rb b/lib/closure_tree/has_closure_tree.rb index b0bc5b1a..4b19f780 100644 --- a/lib/closure_tree/has_closure_tree.rb +++ b/lib/closure_tree/has_closure_tree.rb @@ -11,7 +11,8 @@ def has_closure_tree(options = {}) :dont_order_roots, :numeric_order, :touch, - :with_advisory_lock + :with_advisory_lock, + :order_belong_to ) class_attribute :_ct diff --git a/lib/closure_tree/model.rb b/lib/closure_tree/model.rb index d874c5f4..1e2889a4 100644 --- a/lib/closure_tree/model.rb +++ b/lib/closure_tree/model.rb @@ -171,6 +171,10 @@ def _ct_parent_id read_attribute(_ct.parent_column_sym) end + def _ct_belong_to_id + read_attribute(_ct.belong_to_column_sym) + end + def _ct_quoted_parent_id _ct.quoted_value(_ct_parent_id) end diff --git a/lib/closure_tree/numeric_deterministic_ordering.rb b/lib/closure_tree/numeric_deterministic_ordering.rb index 8e7fe584..e62837fa 100644 --- a/lib/closure_tree/numeric_deterministic_ordering.rb +++ b/lib/closure_tree/numeric_deterministic_ordering.rb @@ -17,17 +17,17 @@ def _ct_reorder_prior_siblings_if_parent_changed attribute_method = as_5_1 ? :attribute_before_last_save : :attribute_was was_parent_id = public_send(attribute_method, _ct.parent_column_name) - _ct.reorder_with_parent_id(was_parent_id) + _ct.reorder_with_parent_id(parent_id: was_parent_id, belong_to_name: _ct.belong_to_column_sym, belong_to_id: _ct_belong_to_id) end end def _ct_reorder_siblings(minimum_sort_order_value = nil) - _ct.reorder_with_parent_id(_ct_parent_id, minimum_sort_order_value) + _ct.reorder_with_parent_id(parent_id: _ct_parent_id, minimum_sort_order_value: minimum_sort_order_value, belong_to_name: _ct.belong_to_column_sym, belong_to_id: _ct_belong_to_id) reload unless destroyed? end def _ct_reorder_children(minimum_sort_order_value = nil) - _ct.reorder_with_parent_id(_ct_id, minimum_sort_order_value) + _ct.reorder_with_parent_id(parent_id: _ct_id, minimum_sort_order_value: minimum_sort_order_value) end def self_and_descendants_preordered diff --git a/lib/closure_tree/numeric_order_support.rb b/lib/closure_tree/numeric_order_support.rb index 0223a415..a867216d 100644 --- a/lib/closure_tree/numeric_order_support.rb +++ b/lib/closure_tree/numeric_order_support.rb @@ -13,38 +13,48 @@ def self.adapter_for_connection(connection) end module MysqlAdapter - def reorder_with_parent_id(parent_id, minimum_sort_order_value = nil) + def reorder_with_parent_id(parent_id:, minimum_sort_order_value: nil, belong_to_name: nil, belong_to_id: nil) return if parent_id.nil? && dont_order_roots min_where = if minimum_sort_order_value "AND #{quoted_order_column} >= #{minimum_sort_order_value}" else "" end + belong_to_scope = if parent_id.nil? && belong_to_id + "AND #{quoted_table_name}.#{belong_to_name} = #{belong_to_id}" + 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} + WHERE #{where_eq(parent_column_name, parent_id)} #{min_where} #{belong_to_scope} ORDER BY #{nulls_last_order_by} SQL end end module PostgreSQLAdapter - def reorder_with_parent_id(parent_id, minimum_sort_order_value = nil) + def reorder_with_parent_id(parent_id:, minimum_sort_order_value: nil, belong_to_name: nil, belong_to_id: nil) return if parent_id.nil? && dont_order_roots min_where = if minimum_sort_order_value "AND #{quoted_order_column} >= #{minimum_sort_order_value}" else "" end + belong_to_scope = if parent_id.nil? && belong_to_id + "AND #{quoted_table_name}.#{belong_to_name} = #{belong_to_id}" + else + "" + end connection.execute <<-SQL.squish UPDATE #{quoted_table_name} SET #{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} + WHERE #{where_eq(parent_column_name, parent_id)} #{min_where} #{belong_to_scope} ) 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} @@ -57,7 +67,7 @@ def rows_updated(result) end module GenericAdapter - def reorder_with_parent_id(parent_id, minimum_sort_order_value = nil) + def reorder_with_parent_id(parent_id:, minimum_sort_order_value: nil, belong_to_name: nil, belong_to_id: nil) return if parent_id.nil? && dont_order_roots scope = model_class. where(parent_column_sym => parent_id). @@ -65,6 +75,7 @@ def reorder_with_parent_id(parent_id, minimum_sort_order_value = nil) if minimum_sort_order_value scope = scope.where("#{quoted_order_column} >= #{minimum_sort_order_value}") end + scope = scope.where(belong_to_name => belong_to_id) if belong_to_id scope.each_with_index do |ea, idx| ea.update_order_value(idx + minimum_sort_order_value.to_i) end diff --git a/lib/closure_tree/support_attributes.rb b/lib/closure_tree/support_attributes.rb index 38879ade..2741c290 100644 --- a/lib/closure_tree/support_attributes.rb +++ b/lib/closure_tree/support_attributes.rb @@ -36,6 +36,14 @@ def parent_column_sym parent_column_name.to_sym end + def belong_to_column_name + options[:order_belong_to] + end + + def belong_to_column_sym + belong_to_column_name&.to_sym + end + def name_column options[:name_column] end diff --git a/spec/closure_tree/root_ordering.rb b/spec/closure_tree/root_ordering.rb new file mode 100644 index 00000000..51d9b74c --- /dev/null +++ b/spec/closure_tree/root_ordering.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +require "spec_helper" + +describe Block do + describe "correct root order_value" do + let!(:group) { Group.create!(name: "TheGroup") } + let!(:user1) { User.create!(email: "1@example.com", group_id: group.id) } + let!(:user2) { User.create!(email: "2@example.com", group_id: group.id) } + let!(:block1) { Block.create!(name: "1block", user_id: user1.id) } + let!(:block2) { Block.create!(name: "2block", user_id: user2.id) } + let!(:block3) { Block.create!(name: "3block", user_id: user1.id) } + let!(:block4) { Block.create!(name: "4block", user_id: user2.id) } + let!(:block5) { Block.create!(name: "5block", user_id: user1.id) } + let!(:block6) { Block.create!(name: "6block", user_id: user2.id) } + + it "should set order_value on roots" do + assert_equal block1.self_and_siblings.pluck(:sort_order), [1,2,3] + assert_equal block2.self_and_siblings.pluck(:sort_order), [1,2,3] + end + end +end \ No newline at end of file diff --git a/spec/support/models.rb b/spec/support/models.rb index 9927af41..b9b45d7a 100644 --- a/spec/support/models.rb +++ b/spec/support/models.rb @@ -78,6 +78,13 @@ class ContractType < ActiveRecord::Base has_many :contracts, inverse_of: :contract_type end +class Block < ApplicationRecord + acts_as_tree order: :column_whereby_ordering_is_inferred, # <- symbol, and not "sort_order" + numeric_order: true, + dependent: :destroy, + order_belong_to: :user_id +end + class Label < ActiveRecord::Base # make sure order doesn't matter acts_as_tree :order => :column_whereby_ordering_is_inferred, # <- symbol, and not "sort_order" diff --git a/spec/support/schema.rb b/spec/support/schema.rb index 6d530117..cf6a0331 100644 --- a/spec/support/schema.rb +++ b/spec/support/schema.rb @@ -12,6 +12,14 @@ add_foreign_key(:tags, :tags, :column => 'parent_id') + create_table 'blocks' do |t| + t.string 'name' + t.references 'parent' + t.references 'user', null: false + t.integer 'sort_order' + t.timestamps null: false + end + create_table "tag_hierarchies", :id => false do |t| t.references "ancestor", :null => false t.references "descendant", :null => false From f106ac0292a3d0358f14020e61930b1c1fdd2ac5 Mon Sep 17 00:00:00 2001 From: derek-etherton-opslevel Date: Wed, 12 Mar 2025 13:03:42 -0700 Subject: [PATCH 02/13] Update README.md --- README.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/README.md b/README.md index d6856b9c..3ecc8403 100644 --- a/README.md +++ b/README.md @@ -1,3 +1,7 @@ +# OpsLevel fork of Closure Tree + +This fork is based on 7.4.0, the last stable gem version release (from Oct 2021). We then applied a patch from https://github.com/ClosureTree/closure_tree/pull/442, which allows us to support tenancy with determinstically ordered closure trees. + # Closure Tree ### Closure_tree lets your ActiveRecord models act as nodes in a [tree data structure](http://en.wikipedia.org/wiki/Tree_%28data_structure%29) From b816694e046a08013b5518b40fb8d87d4641cc32 Mon Sep 17 00:00:00 2001 From: Derek Etherton Date: Wed, 12 Mar 2025 13:39:09 -0700 Subject: [PATCH 03/13] remove github workflows --- .github/workflows/ci.yml | 98 ---------------------------------------- 1 file changed, 98 deletions(-) delete mode 100644 .github/workflows/ci.yml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml deleted file mode 100644 index 24edd818..00000000 --- a/.github/workflows/ci.yml +++ /dev/null @@ -1,98 +0,0 @@ ---- -name: CI - -on: - - push - - pull_request - -jobs: - rspec: - runs-on: ubuntu-20.04 - - services: - postgres: - image: 'postgres:13' - ports: ['5432:5432'] - env: - POSTGRES_PASSWORD: postgres - POSTGRES_DB: closure_tree - options: >- - --health-cmd pg_isready - --health-interval 10s - --health-timeout 5s - --health-retries 5 - - strategy: - fail-fast: false - matrix: - ruby: - - '3.0' - - '2.7' - - '2.6' - - '2.5' - rails: - - activerecord_6.1 - - activerecord_6.0 - - activerecord_5.2 - - activerecord_5.1 - - activerecord_5.0 - - activerecord_4.2 - - activerecord_edge - adapter: - - sqlite3 - - mysql2 - - postgresql - exclude: - - ruby: '2.7' - rails: activerecord_4.2 - - ruby: '3.0' - rails: activerecord_4.2 - - ruby: '3.0' - rails: activerecord_5.0 - - ruby: '3.0' - rails: activerecord_5.1 - - ruby: '3.0' - rails: activerecord_5.2 - - ruby: '2.5' - rails: activerecord_edge - - ruby: '2.6' - rails: activerecord_edge - - steps: - - name: Checkout - uses: actions/checkout@v2 - - - name: Setup Ruby - uses: ruby/setup-ruby@v1 - with: - ruby-version: ${{ matrix.ruby }} - - - name: Set DB Adapter - env: - RAILS_VERSION: ${{ matrix.rails }} - DB_ADAPTER: ${{ matrix.adapter }} - - # See: https://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu2004-README.md#mysql - run: | - if [ "${DB_ADAPTER}" = "mysql2" ]; then - sudo systemctl start mysql.service - mysql -u root -proot -e 'create database closure_tree;' - fi - - - name: Bundle - env: - RAILS_VERSION: ${{ matrix.rails }} - DB_ADAPTER: ${{ matrix.adapter }} - BUNDLE_GEMFILE: gemfiles/${{ matrix.rails }}.gemfile - run: | - gem install bundler - bundle config path vendor/bundle - bundle install --jobs 4 --retry 3 - - - name: RSpec - env: - RAILS_VERSION: ${{ matrix.rails }} - DB_ADAPTER: ${{ matrix.adapter }} - BUNDLE_GEMFILE: gemfiles/${{ matrix.rails }}.gemfile - WITH_ADVISORY_LOCK_PREFIX: ${{ github.run_id }} - run: bin/rake --trace spec:all From 9ea773da041171d6459eef92f786ea5c7714ee9d Mon Sep 17 00:00:00 2001 From: Farjaad Rawasia Date: Wed, 19 Mar 2025 13:16:30 -0400 Subject: [PATCH 04/13] Run tests using GitHub Actions --- .github/workflows/tests.yml | 76 +++++++++++++++++++++++++++++++ Appraisals | 15 ++++++ Rakefile | 9 ---- gemfiles/activerecord_7.0.gemfile | 21 +++++++++ spec/support/models.rb | 2 +- spec/support/schema.rb | 4 +- 6 files changed, 116 insertions(+), 11 deletions(-) create mode 100644 .github/workflows/tests.yml create mode 100644 gemfiles/activerecord_7.0.gemfile diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml new file mode 100644 index 00000000..4e7020ea --- /dev/null +++ b/.github/workflows/tests.yml @@ -0,0 +1,76 @@ +--- +name: Tests + +on: + push: + branches: [ "master" ] + pull_request: + branches: [ "**" ] + +jobs: + test: + runs-on: ubuntu-20.04 + + services: + postgres: + image: "postgres:13" + ports: ["5432:5432"] + env: + POSTGRES_PASSWORD: postgres + POSTGRES_DB: closure_tree + options: >- + --health-cmd pg_isready + --health-interval 10s + --health-timeout 5s + --health-retries 5 + + strategy: + fail-fast: false + matrix: + ruby: + - "3.2.5" + rails: + - activerecord_7.0 + adapter: + - sqlite3 + - mysql2 + - postgresql + + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Setup Ruby + uses: ruby/setup-ruby@v1 + with: + ruby-version: ${{ matrix.ruby }} + + - name: Set DB Adapter + env: + RAILS_VERSION: ${{ matrix.rails }} + DB_ADAPTER: ${{ matrix.adapter }} + + # See: https://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu2004-README.md#mysql + run: | + if [ "${DB_ADAPTER}" = "mysql2" ]; then + sudo systemctl start mysql.service + mysql -u root -proot -e "create database closure_tree;" + fi + + - name: Bundle + env: + RAILS_VERSION: ${{ matrix.rails }} + DB_ADAPTER: ${{ matrix.adapter }} + BUNDLE_GEMFILE: gemfiles/${{ matrix.rails }}.gemfile + run: | + gem install bundler + bundle config path vendor/bundle + bundle install --jobs 4 --retry 3 + + - name: RSpec + env: + RAILS_VERSION: ${{ matrix.rails }} + DB_ADAPTER: ${{ matrix.adapter }} + BUNDLE_GEMFILE: gemfiles/${{ matrix.rails }}.gemfile + WITH_ADVISORY_LOCK_PREFIX: ${{ github.run_id }} + run: bin/rake --trace spec:all \ No newline at end of file diff --git a/Appraisals b/Appraisals index 98d02c51..a88f6246 100644 --- a/Appraisals +++ b/Appraisals @@ -89,6 +89,21 @@ appraise 'activerecord-6.1' do end end +appraise 'activerecord-7.0' do + gem 'activerecord', '~> 7.0' + platforms :ruby do + gem 'mysql2' + gem 'pg' + gem 'sqlite3' + end + + platforms :jruby do + gem 'activerecord-jdbcmysql-adapter' + gem 'activerecord-jdbcpostgresql-adapter' + gem 'activerecord-jdbcsqlite3-adapter' + end +end + appraise 'activerecord-edge' do gem 'activerecord', github: 'rails/rails' platforms :ruby do diff --git a/Rakefile b/Rakefile index 58751bf1..bb87ad7c 100644 --- a/Rakefile +++ b/Rakefile @@ -26,12 +26,3 @@ namespace :spec do task.pattern = 'spec/generators/*_spec.rb' end end - -require 'github_changelog_generator/task' -GitHubChangelogGenerator::RakeTask.new :changelog do |config| - config.user = 'ClosureTree' - config.project = 'closure_tree' - config.issues = false - config.future_release = '5.2.0' - config.since_tag = 'v7.3.0' -end diff --git a/gemfiles/activerecord_7.0.gemfile b/gemfiles/activerecord_7.0.gemfile new file mode 100644 index 00000000..f50ebabc --- /dev/null +++ b/gemfiles/activerecord_7.0.gemfile @@ -0,0 +1,21 @@ +# This file was generated by Appraisal + +source "https://rubygems.org" + +gem "bump", "~> 0.10.0" +gem "github_changelog_generator", "~> 1.16" +gem "activerecord", "~> 7.0" + +platforms :ruby do + gem "mysql2" + gem "pg" + gem "sqlite3" +end + +platforms :jruby do + gem "activerecord-jdbcmysql-adapter" + gem "activerecord-jdbcpostgresql-adapter" + gem "activerecord-jdbcsqlite3-adapter" +end + +gemspec path: "../" diff --git a/spec/support/models.rb b/spec/support/models.rb index b9b45d7a..7c51aac1 100644 --- a/spec/support/models.rb +++ b/spec/support/models.rb @@ -78,7 +78,7 @@ class ContractType < ActiveRecord::Base has_many :contracts, inverse_of: :contract_type end -class Block < ApplicationRecord +class Block < ActiveRecord::Base acts_as_tree order: :column_whereby_ordering_is_inferred, # <- symbol, and not "sort_order" numeric_order: true, dependent: :destroy, diff --git a/spec/support/schema.rb b/spec/support/schema.rb index cf6a0331..5346a791 100644 --- a/spec/support/schema.rb +++ b/spec/support/schema.rb @@ -30,7 +30,7 @@ add_foreign_key(:tag_hierarchies, :tags, :column => 'descendant_id') create_table "uuid_tags", :id => false do |t| - t.string "uuid", :unique => true + t.string "uuid" t.string "name" t.string "title" t.string "parent_uuid" @@ -38,6 +38,8 @@ t.timestamps null: false end + add_index "uuid_tags", :uuid, unique: true + create_table "uuid_tag_hierarchies", :id => false do |t| t.string "ancestor_id", :null => false t.string "descendant_id", :null => false From c06a110ed5b842586859cc2fcc448ba213b2ead8 Mon Sep 17 00:00:00 2001 From: Farjaad Rawasia Date: Mon, 17 Mar 2025 13:22:48 -0400 Subject: [PATCH 05/13] Raise error when advisory lock cannot be acquired --- lib/closure_tree/support.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/closure_tree/support.rb b/lib/closure_tree/support.rb index 04cf06b4..dc45b64f 100644 --- a/lib/closure_tree/support.rb +++ b/lib/closure_tree/support.rb @@ -110,7 +110,7 @@ def where_eq(column_name, value) def with_advisory_lock(&block) if options[:with_advisory_lock] - model_class.with_advisory_lock(advisory_lock_name) do + model_class.with_advisory_lock!(advisory_lock_name, timeout_seconds: 5) do transaction { yield } end else From 8ae7dd14c6454ed4c6cafdaaca6eeadc26752497 Mon Sep 17 00:00:00 2001 From: Farjaad Rawasia Date: Wed, 19 Mar 2025 13:08:21 -0400 Subject: [PATCH 06/13] address comments --- lib/closure_tree/finders.rb | 4 ++-- lib/closure_tree/has_closure_tree.rb | 1 + lib/closure_tree/hierarchy_maintenance.rb | 8 ++++---- lib/closure_tree/numeric_deterministic_ordering.rb | 2 +- lib/closure_tree/support.rb | 6 ++++-- 5 files changed, 12 insertions(+), 9 deletions(-) diff --git a/lib/closure_tree/finders.rb b/lib/closure_tree/finders.rb index c95b5c12..e66ead0e 100644 --- a/lib/closure_tree/finders.rb +++ b/lib/closure_tree/finders.rb @@ -17,7 +17,7 @@ def find_or_create_by_path(path, attributes = {}) return found if found attrs = subpath.shift - _ct.with_advisory_lock do + _ct.with_advisory_lock! do # shenanigans because children.create is bound to the superclass # (in the case of polymorphism): child = self.children.where(attrs).first || begin @@ -159,7 +159,7 @@ def find_or_create_by_path(path, attributes = {}) attr_path = _ct.build_ancestry_attr_path(path, attributes) find_by_path(attr_path) || begin root_attrs = attr_path.shift - _ct.with_advisory_lock do + _ct.with_advisory_lock! do # shenanigans because find_or_create can't infer that we want the same class as this: # Note that roots will already be constrained to this subclass (in the case of polymorphism): root = roots.where(root_attrs).first || _ct.create!(self, root_attrs) diff --git a/lib/closure_tree/has_closure_tree.rb b/lib/closure_tree/has_closure_tree.rb index 4b19f780..023eb203 100644 --- a/lib/closure_tree/has_closure_tree.rb +++ b/lib/closure_tree/has_closure_tree.rb @@ -12,6 +12,7 @@ def has_closure_tree(options = {}) :numeric_order, :touch, :with_advisory_lock, + :advisory_lock_timeout_seconds, :order_belong_to ) diff --git a/lib/closure_tree/hierarchy_maintenance.rb b/lib/closure_tree/hierarchy_maintenance.rb index 3023b247..04845709 100644 --- a/lib/closure_tree/hierarchy_maintenance.rb +++ b/lib/closure_tree/hierarchy_maintenance.rb @@ -53,7 +53,7 @@ def _ct_after_save end def _ct_before_destroy - _ct.with_advisory_lock do + _ct.with_advisory_lock! do delete_hierarchy_references if _ct.options[:dependent] == :nullify self.class.find(self.id).children.find_each { |c| c.rebuild! } @@ -63,7 +63,7 @@ def _ct_before_destroy end def rebuild!(called_by_rebuild = false) - _ct.with_advisory_lock do + _ct.with_advisory_lock! do delete_hierarchy_references unless (defined? @was_new_record) && @was_new_record hierarchy_class.create!(:ancestor => self, :descendant => self, :generations => 0) unless root? @@ -89,7 +89,7 @@ def rebuild!(called_by_rebuild = false) end def delete_hierarchy_references - _ct.with_advisory_lock do + _ct.with_advisory_lock! do # The crazy double-wrapped sub-subselect works around MySQL's limitation of subselects on the same table that is being mutated. # It shouldn't affect performance of postgresql. # See http://dev.mysql.com/doc/refman/5.0/en/subquery-errors.html @@ -111,7 +111,7 @@ module ClassMethods # Rebuilds the hierarchy table based on the parent_id column in the database. # Note that the hierarchy table will be truncated. def rebuild! - _ct.with_advisory_lock do + _ct.with_advisory_lock! do cleanup! roots.find_each { |n| n.send(:rebuild!) } # roots just uses the parent_id column, so this is safe. end diff --git a/lib/closure_tree/numeric_deterministic_ordering.rb b/lib/closure_tree/numeric_deterministic_ordering.rb index e62837fa..6c36b8f9 100644 --- a/lib/closure_tree/numeric_deterministic_ordering.rb +++ b/lib/closure_tree/numeric_deterministic_ordering.rb @@ -125,7 +125,7 @@ def add_sibling(sibling, add_after = true) # Make sure self isn't dirty, because we're going to call reload: save - _ct.with_advisory_lock do + _ct.with_advisory_lock! do prior_sibling_parent = sibling.parent reorder_from_value = if prior_sibling_parent == self.parent [self.order_value, sibling.order_value].compact.min diff --git a/lib/closure_tree/support.rb b/lib/closure_tree/support.rb index dc45b64f..6fa395f5 100644 --- a/lib/closure_tree/support.rb +++ b/lib/closure_tree/support.rb @@ -23,6 +23,7 @@ def initialize(model_class, options) :dependent => :nullify, # or :destroy or :delete_all -- see the README :name_column => 'name', :with_advisory_lock => true, + :advisory_lock_timeout_seconds => nil, :numeric_order => false }.merge(options) raise ArgumentError, "name_column can't be 'path'" if options[:name_column] == 'path' @@ -108,9 +109,10 @@ def where_eq(column_name, value) end end - def with_advisory_lock(&block) + def with_advisory_lock!(&block) if options[:with_advisory_lock] - model_class.with_advisory_lock!(advisory_lock_name, timeout_seconds: 5) do + lock_options = { timeout_seconds: options[:advisory_lock_timeout_seconds] }.compact + model_class.with_advisory_lock!(advisory_lock_name, lock_options) do transaction { yield } end else From cad9d07b7d60f724c86145f2da8f53efacad6861 Mon Sep 17 00:00:00 2001 From: Farjaad Rawasia Date: Wed, 19 Mar 2025 15:26:38 -0400 Subject: [PATCH 07/13] tests --- lib/closure_tree/support.rb | 3 +++ spec/closure_tree/parallel_spec.rb | 2 +- spec/closure_tree/support_spec.rb | 28 ++++++++++++++++++++++++++-- 3 files changed, 30 insertions(+), 3 deletions(-) diff --git a/lib/closure_tree/support.rb b/lib/closure_tree/support.rb index 6fa395f5..24d08f90 100644 --- a/lib/closure_tree/support.rb +++ b/lib/closure_tree/support.rb @@ -27,6 +27,9 @@ def initialize(model_class, options) :numeric_order => false }.merge(options) raise ArgumentError, "name_column can't be 'path'" if options[:name_column] == 'path' + if !options[:with_advisory_lock] && options[:advisory_lock_timeout_seconds].present? + raise ArgumentError, "advisory_lock_timeout_seconds cannot be provided when advisory lock is disabled" + end if order_is_numeric? extend NumericOrderSupport.adapter_for_connection(connection) end diff --git a/spec/closure_tree/parallel_spec.rb b/spec/closure_tree/parallel_spec.rb index 4ec3a61a..7cdf9951 100644 --- a/spec/closure_tree/parallel_spec.rb +++ b/spec/closure_tree/parallel_spec.rb @@ -103,7 +103,7 @@ def run_workers(worker_class = FindOrCreateWorker) it 'creates dupe roots without advisory locks' do # disable with_advisory_lock: - allow(Tag).to receive(:with_advisory_lock) { |_lock_name, &block| block.call } + allow(Tag).to receive(:with_advisory_lock!) { |_lock_name, &block| block.call } run_workers # duplication from at least one iteration: expect(Tag.where(name: @names).size).to be > @iterations diff --git a/spec/closure_tree/support_spec.rb b/spec/closure_tree/support_spec.rb index 1deb27bf..d63a3dbe 100644 --- a/spec/closure_tree/support_spec.rb +++ b/spec/closure_tree/support_spec.rb @@ -1,14 +1,38 @@ require 'spec_helper' RSpec.describe ClosureTree::Support do - let(:sut) { Tag._ct } + let(:model) { Tag } + let(:options) { {} } + let(:sut) { described_class.new(model, options) } + it 'passes through table names without prefix and suffix' do expected = 'some_random_table_name' expect(sut.remove_prefix_and_suffix(expected)).to eq(expected) end + it 'extracts through table name with prefix and suffix' do expected = 'some_random_table_name' tn = ActiveRecord::Base.table_name_prefix + expected + ActiveRecord::Base.table_name_suffix expect(sut.remove_prefix_and_suffix(tn)).to eq(expected) end -end + + [ + [true, 10, { timeout_seconds: 10 }], + [true, nil, {}], + [false, nil, {}] + ].each do |with_lock, timeout, expected_options| + context "when with_advisory_lock is #{with_lock} and timeout is #{timeout}" do + let(:options) { { with_advisory_lock: with_lock, advisory_lock_timeout_seconds: timeout } } + + it "uses advisory lock with timeout options: #{expected_options}" do + if with_lock + expect(model).to receive(:with_advisory_lock!).with(anything, expected_options).and_yield + else + expect(model).not_to receive(:with_advisory_lock!) + end + + expect { |b| sut.with_advisory_lock!(&b) }.to yield_control + end + end + end +end \ No newline at end of file From 606c1576427606d3408d6421cb8a66d76448a25b Mon Sep 17 00:00:00 2001 From: Farjaad Rawasia Date: Wed, 19 Mar 2025 22:56:26 -0400 Subject: [PATCH 08/13] move lock options to support_attributes so it's alongside lock name --- lib/closure_tree/support.rb | 3 +-- lib/closure_tree/support_attributes.rb | 4 ++++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/closure_tree/support.rb b/lib/closure_tree/support.rb index 24d08f90..27020fd9 100644 --- a/lib/closure_tree/support.rb +++ b/lib/closure_tree/support.rb @@ -114,8 +114,7 @@ def where_eq(column_name, value) def with_advisory_lock!(&block) if options[:with_advisory_lock] - lock_options = { timeout_seconds: options[:advisory_lock_timeout_seconds] }.compact - model_class.with_advisory_lock!(advisory_lock_name, lock_options) do + model_class.with_advisory_lock!(advisory_lock_name, advisory_lock_options) do transaction { yield } end else diff --git a/lib/closure_tree/support_attributes.rb b/lib/closure_tree/support_attributes.rb index 2741c290..cf28553e 100644 --- a/lib/closure_tree/support_attributes.rb +++ b/lib/closure_tree/support_attributes.rb @@ -8,6 +8,10 @@ def advisory_lock_name Digest::SHA1.hexdigest("ClosureTree::#{base_class.name}")[0..32] end + def advisory_lock_options + { timeout_seconds: options[:advisory_lock_timeout_seconds] }.compact + end + def quoted_table_name connection.quote_table_name(table_name) end From 0e6fbf5915aafc30fdeb7174847032f64b03fa1a Mon Sep 17 00:00:00 2001 From: Farjaad Rawasia Date: Thu, 20 Mar 2025 11:19:41 -0400 Subject: [PATCH 09/13] set default to 5 seconds --- lib/closure_tree/support.rb | 5 +---- spec/closure_tree/parallel_spec.rb | 6 ++++++ 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/closure_tree/support.rb b/lib/closure_tree/support.rb index 27020fd9..0da20582 100644 --- a/lib/closure_tree/support.rb +++ b/lib/closure_tree/support.rb @@ -23,13 +23,10 @@ def initialize(model_class, options) :dependent => :nullify, # or :destroy or :delete_all -- see the README :name_column => 'name', :with_advisory_lock => true, - :advisory_lock_timeout_seconds => nil, + :advisory_lock_timeout_seconds => 5, :numeric_order => false }.merge(options) raise ArgumentError, "name_column can't be 'path'" if options[:name_column] == 'path' - if !options[:with_advisory_lock] && options[:advisory_lock_timeout_seconds].present? - raise ArgumentError, "advisory_lock_timeout_seconds cannot be provided when advisory lock is disabled" - end if order_is_numeric? extend NumericOrderSupport.adapter_for_connection(connection) end diff --git a/spec/closure_tree/parallel_spec.rb b/spec/closure_tree/parallel_spec.rb index 7cdf9951..664ff6ba 100644 --- a/spec/closure_tree/parallel_spec.rb +++ b/spec/closure_tree/parallel_spec.rb @@ -121,6 +121,12 @@ def work end it 'fails to deadlock while simultaneously deleting items from the same hierarchy' do + allow(User).to receive(:with_advisory_lock!).and_wrap_original do |method, *args, &block| + options = args.extract_options! + options[:timeout_seconds] = nil + method.call(*args, options, &block) + end + target = User.find_or_create_by_path((1..200).to_a.map { |ea| ea.to_s }) emails = target.self_and_ancestors.to_a.map(&:email).shuffle Parallel.map(emails, :in_threads => max_threads) do |email| From ae7b44f6afe8e7f1dac70192089c94b3dc5f053e Mon Sep 17 00:00:00 2001 From: Farjaad Rawasia Date: Thu, 20 Mar 2025 11:51:52 -0400 Subject: [PATCH 10/13] try higher timeout --- spec/closure_tree/parallel_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/closure_tree/parallel_spec.rb b/spec/closure_tree/parallel_spec.rb index 664ff6ba..8d9e94a4 100644 --- a/spec/closure_tree/parallel_spec.rb +++ b/spec/closure_tree/parallel_spec.rb @@ -123,7 +123,7 @@ def work it 'fails to deadlock while simultaneously deleting items from the same hierarchy' do allow(User).to receive(:with_advisory_lock!).and_wrap_original do |method, *args, &block| options = args.extract_options! - options[:timeout_seconds] = nil + options[:timeout_seconds] = 15 method.call(*args, options, &block) end From 43f68345e46a7f25a2e4a891f1f6cc4cd8ca8254 Mon Sep 17 00:00:00 2001 From: Farjaad Rawasia Date: Thu, 20 Mar 2025 12:25:06 -0400 Subject: [PATCH 11/13] set default to 15 seconds --- lib/closure_tree/support.rb | 2 +- spec/closure_tree/parallel_spec.rb | 6 ------ 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/lib/closure_tree/support.rb b/lib/closure_tree/support.rb index 0da20582..2874c44f 100644 --- a/lib/closure_tree/support.rb +++ b/lib/closure_tree/support.rb @@ -23,7 +23,7 @@ def initialize(model_class, options) :dependent => :nullify, # or :destroy or :delete_all -- see the README :name_column => 'name', :with_advisory_lock => true, - :advisory_lock_timeout_seconds => 5, + :advisory_lock_timeout_seconds => 15, :numeric_order => false }.merge(options) raise ArgumentError, "name_column can't be 'path'" if options[:name_column] == 'path' diff --git a/spec/closure_tree/parallel_spec.rb b/spec/closure_tree/parallel_spec.rb index 8d9e94a4..7cdf9951 100644 --- a/spec/closure_tree/parallel_spec.rb +++ b/spec/closure_tree/parallel_spec.rb @@ -121,12 +121,6 @@ def work end it 'fails to deadlock while simultaneously deleting items from the same hierarchy' do - allow(User).to receive(:with_advisory_lock!).and_wrap_original do |method, *args, &block| - options = args.extract_options! - options[:timeout_seconds] = 15 - method.call(*args, options, &block) - end - target = User.find_or_create_by_path((1..200).to_a.map { |ea| ea.to_s }) emails = target.self_and_ancestors.to_a.map(&:email).shuffle Parallel.map(emails, :in_threads => max_threads) do |email| From 46ed50ddeb09e75206f91240ecdc14f2781a27fa Mon Sep 17 00:00:00 2001 From: derek-etherton-opslevel Date: Mon, 24 Mar 2025 08:19:40 -0700 Subject: [PATCH 12/13] Update README.md --- README.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/README.md b/README.md index 3ecc8403..41162511 100644 --- a/README.md +++ b/README.md @@ -1,5 +1,13 @@ # OpsLevel fork of Closure Tree +[![Overall](https://img.shields.io/endpoint?style=flat&url=https%3A%2F%2Fapp.opslevel.com%2Fapi%2Fservice_level%2F8bksYN3Yj8yRfI1LKJNLpqXqzpXSPdvGAkJKUXqYMIA)](https://app.opslevel.com/services/closure_tree/maturity-report) +[![🔐 Security](https://img.shields.io/endpoint?style=flat&url=https%3A%2F%2Fapp.opslevel.com%2Fapi%2Fservice_level%2F8bksYN3Yj8yRfI1LKJNLpqXqzpXSPdvGAkJKUXqYMIA%2Fsecurity)](https://app.opslevel.com/services/closure_tree/maturity-report) +[![🟢 Reliability](https://img.shields.io/endpoint?style=flat&url=https%3A%2F%2Fapp.opslevel.com%2Fapi%2Fservice_level%2F8bksYN3Yj8yRfI1LKJNLpqXqzpXSPdvGAkJKUXqYMIA%2Freliability)](https://app.opslevel.com/services/closure_tree/maturity-report) +[![🔍 Observability](https://img.shields.io/endpoint?style=flat&url=https%3A%2F%2Fapp.opslevel.com%2Fapi%2Fservice_level%2F8bksYN3Yj8yRfI1LKJNLpqXqzpXSPdvGAkJKUXqYMIA%2Fobservability)](https://app.opslevel.com/services/closure_tree/maturity-report) +[![📈 Quality](https://img.shields.io/endpoint?style=flat&url=https%3A%2F%2Fapp.opslevel.com%2Fapi%2Fservice_level%2F8bksYN3Yj8yRfI1LKJNLpqXqzpXSPdvGAkJKUXqYMIA%2Fquality)](https://app.opslevel.com/services/closure_tree/maturity-report) +[![📋 Ownership](https://img.shields.io/endpoint?style=flat&url=https%3A%2F%2Fapp.opslevel.com%2Fapi%2Fservice_level%2F8bksYN3Yj8yRfI1LKJNLpqXqzpXSPdvGAkJKUXqYMIA%2Fownership)](https://app.opslevel.com/services/closure_tree/maturity-report) +[![🛠 Misc](https://img.shields.io/endpoint?style=flat&url=https%3A%2F%2Fapp.opslevel.com%2Fapi%2Fservice_level%2F8bksYN3Yj8yRfI1LKJNLpqXqzpXSPdvGAkJKUXqYMIA%2Fmisc_2)](https://app.opslevel.com/services/closure_tree/maturity-report) + This fork is based on 7.4.0, the last stable gem version release (from Oct 2021). We then applied a patch from https://github.com/ClosureTree/closure_tree/pull/442, which allows us to support tenancy with determinstically ordered closure trees. # Closure Tree From e1469cbf3824b07898bd150285bf72c71c5f9d3c Mon Sep 17 00:00:00 2001 From: Derek Etherton Date: Mon, 24 Mar 2025 12:07:56 -0700 Subject: [PATCH 13/13] add dependabot --- .github/workflows/dependabot.yml | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 .github/workflows/dependabot.yml diff --git a/.github/workflows/dependabot.yml b/.github/workflows/dependabot.yml new file mode 100644 index 00000000..f3c4cad6 --- /dev/null +++ b/.github/workflows/dependabot.yml @@ -0,0 +1,9 @@ +version: 2 +updates: + - package-ecosystem: 'bundler' + directory: '/' + open-pull-requests-limit: 10 + schedule: + interval: 'weekly' + labels: + - "dependencies" \ No newline at end of file