Skip to content

feat: use new with_advisory_lock #450

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 28, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,5 @@
source 'https://rubygems.org'

gemspec

gem 'with_advisory_lock', github: 'closuretree/with_advisory_lock'
2 changes: 1 addition & 1 deletion closure_tree.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ Gem::Specification.new do |gem|
gem.required_ruby_version = '>= 3.3.0'

gem.add_runtime_dependency 'activerecord', '>= 7.1.0'
gem.add_runtime_dependency 'with_advisory_lock', '>= 5.0.0', '< 6.0.0'
gem.add_runtime_dependency 'with_advisory_lock', '>= 6.0.0'

gem.add_development_dependency 'appraisal'
gem.add_development_dependency 'database_cleaner'
Expand Down
1 change: 1 addition & 0 deletions gemfiles/activerecord_7.1.gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

source "https://rubygems.org"

gem "with_advisory_lock", github: "closuretree/with_advisory_lock"
gem "activerecord", "~> 7.1.0"
gem "railties"

Expand Down
1 change: 1 addition & 0 deletions gemfiles/activerecord_7.2.gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

source "https://rubygems.org"

gem "with_advisory_lock", github: "closuretree/with_advisory_lock"
gem "activerecord", "~> 7.2.0"
gem "railties"

Expand Down
1 change: 1 addition & 0 deletions gemfiles/activerecord_8.0.gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

source "https://rubygems.org"

gem "with_advisory_lock", github: "closuretree/with_advisory_lock"
gem "activerecord", "~> 8.0.0"
gem "railties"

Expand Down
1 change: 1 addition & 0 deletions gemfiles/activerecord_edge.gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

source "https://rubygems.org"

gem "with_advisory_lock", github: "closuretree/with_advisory_lock"
gem "activerecord", github: "rails/rails"
gem "railties", github: "rails/rails"

Expand Down
6 changes: 3 additions & 3 deletions lib/closure_tree/numeric_order_support.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@ module ClosureTree
module NumericOrderSupport

def self.adapter_for_connection(connection)
das = WithAdvisoryLock::DatabaseAdapterSupport.new(connection)
if das.postgresql?
adapter_name = connection.adapter_name.downcase
if adapter_name.include?('postgresql') || adapter_name.include?('postgis')
::ClosureTree::NumericOrderSupport::PostgreSQLAdapter
elsif das.mysql?
elsif adapter_name.include?('mysql') || adapter_name.include?('trilogy')
::ClosureTree::NumericOrderSupport::MysqlAdapter
else
::ClosureTree::NumericOrderSupport::GenericAdapter
Expand Down
10 changes: 5 additions & 5 deletions lib/closure_tree/support.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,15 @@ class Support

def initialize(model_class, options)
@model_class = model_class

# Detect if we're using SQLite and disable advisory locks
default_with_advisory_lock = !connection.adapter_name.downcase.include?('sqlite')

@options = {
:parent_column_name => 'parent_id',
:dependent => :nullify, # or :destroy or :delete_all -- see the README
:name_column => 'name',
:with_advisory_lock => true,
:with_advisory_lock => default_with_advisory_lock,
:numeric_order => false
}.merge(options)
raise ArgumentError, "name_column can't be 'path'" if options[:name_column] == 'path'
Expand All @@ -34,14 +38,10 @@ def initialize(model_class, options)
def hierarchy_class_for_model
parent_class = model_class.module_parent
hierarchy_class = parent_class.const_set(short_hierarchy_class_name, Class.new(model_class.superclass))
use_attr_accessible = use_attr_accessible?
include_forbidden_attributes_protection = include_forbidden_attributes_protection?
model_class_name = model_class.to_s
hierarchy_class.class_eval do
include ActiveModel::ForbiddenAttributesProtection if include_forbidden_attributes_protection
belongs_to :ancestor, class_name: model_class_name
belongs_to :descendant, class_name: model_class_name
attr_accessible :ancestor, :descendant, :generations if use_attr_accessible
def ==(other)
self.class == other.class && ancestor_id == other.ancestor_id && descendant_id == other.descendant_id
end
Expand Down
10 changes: 0 additions & 10 deletions lib/closure_tree/support_flags.rb
Original file line number Diff line number Diff line change
@@ -1,16 +1,6 @@
module ClosureTree
module SupportFlags

def use_attr_accessible?
defined?(ActiveModel::MassAssignmentSecurity) &&
model_class.respond_to?(:accessible_attributes) &&
! model_class.accessible_attributes.empty?
end

def include_forbidden_attributes_protection?
defined?(ActiveModel::ForbiddenAttributesProtection) &&
model_class.ancestors.include?(ActiveModel::ForbiddenAttributesProtection)
end

def order_option?
order_by.present?
Expand Down
11 changes: 8 additions & 3 deletions test/closure_tree/matcher_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,14 @@ class MatcherTest < ActiveSupport::TestCase
end

test "advisory_lock option" do
assert_closure_tree User, with_advisory_lock: true
assert_closure_tree Label, ordered: true, with_advisory_lock: true
assert_closure_tree Metal, ordered: :sort_order, with_advisory_lock: true
# SQLite doesn't support advisory locks, so skip these tests when using SQLite
if ActiveRecord::Base.connection.adapter_name.downcase.include?('sqlite')
skip "SQLite doesn't support advisory locks"
else
assert_closure_tree User, with_advisory_lock: true
assert_closure_tree Label, ordered: true, with_advisory_lock: true
assert_closure_tree Metal, ordered: :sort_order, with_advisory_lock: true
end
end

test "without_advisory_lock option" do
Expand Down
5 changes: 0 additions & 5 deletions test/support/tag_examples.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,6 @@ def self.included(mod)
end

describe 'class setup' do
it 'has correct accessible_attributes' do
if @tag_class._ct.use_attr_accessible?
assert_equal(%w[parent name title].sort, @tag_class.accessible_attributes.to_a.sort)
end
end

it 'should build hierarchy classname correctly' do
assert_equal @tag_hierarchy_class, @tag_class.hierarchy_class
Expand Down
29 changes: 28 additions & 1 deletion test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,37 @@ def sqlite?
env_db == :sqlite3
end

ActiveRecord::Base.connection.recreate_database('closure_tree_test') unless sqlite?
# For PostgreSQL and MySQL, we need to create/reset the database structure
unless sqlite?
begin
if ActiveRecord::Base.connection.adapter_name.downcase.include?('postgresql')
# PostgreSQL requires disconnecting before dropping the database
ActiveRecord::Base.connection.disconnect!
# Connect to postgres database to drop/create closure_tree_test
if connection_config.is_a?(String)
# Parse the DATABASE_URL and change database to postgres
postgres_url = connection_config.gsub(/\/closure_tree_test/, '/postgres')
ActiveRecord::Base.establish_connection(postgres_url)
else
ActiveRecord::Base.establish_connection(connection_config.merge(database: 'postgres'))
end
ActiveRecord::Base.connection.drop_database('closure_tree_test') rescue nil
ActiveRecord::Base.connection.create_database('closure_tree_test')
ActiveRecord::Base.connection.disconnect!
ActiveRecord::Base.establish_connection(connection_config)
else
# MySQL can recreate directly
ActiveRecord::Base.connection.recreate_database('closure_tree_test')
end
rescue => e
puts "Warning: Could not recreate database: #{e.message}"
end
end
puts "Testing with #{env_db} database, ActiveRecord #{ActiveRecord.gem_version} and #{RUBY_ENGINE} #{RUBY_ENGINE_VERSION} as #{RUBY_VERSION}"

DatabaseCleaner.strategy = :truncation
# Allow DatabaseCleaner to work with our test database
DatabaseCleaner.allow_remote_database_url = true

module Minitest
class Spec
Expand Down
Loading