Skip to content
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

sane_ancestor_ids and simplify specs #495

Merged
merged 4 commits into from
Aug 10, 2020
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
4 changes: 2 additions & 2 deletions lib/ancestry/class_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ def check_ancestry_integrity! options = {}
scope.find_each do |node|
begin
# ... check validity of ancestry column
if !node.valid? and !node.errors[node.class.ancestry_column].blank?
if !node.sane_ancestor_ids?
raise Ancestry::AncestryIntegrityException.new("Invalid format for ancestry column of node #{node.id}: #{node.read_attribute node.ancestry_column}.")
end
# ... check that all ancestors exist
Expand Down Expand Up @@ -140,7 +140,7 @@ def restore_ancestry_integrity!
# For each node ...
scope.find_each do |node|
# ... set its ancestry to nil if invalid
if !node.valid? and !node.errors[node.class.ancestry_column].blank?
if !node.sane_ancestor_ids?
node.without_ancestry_callbacks do
node.update_attribute :ancestor_ids, []
end
Expand Down
4 changes: 4 additions & 0 deletions lib/ancestry/instance_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,10 @@ def ancestry_changed?
end
end

def sane_ancestor_ids?
valid? || errors[self.ancestry_base_class.ancestry_column].blank?
end
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will probably get merged back in when this concept changes. But probably good enough for now


def ancestors depth_options = {}
return self.ancestry_base_class.none unless ancestors?
self.ancestry_base_class.scope_depth(depth_options, depth).ordered_by_ancestry.ancestors_of(self)
Expand Down
2 changes: 1 addition & 1 deletion lib/ancestry/materialized_path.rb
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ def subtree_of(object)
def siblings_of(object)
t = arel_table
node = to_node(object)
where(t[ancestry_column].eq(node[ancestry_column]))
where(t[ancestry_column].eq(node[ancestry_column].presence))
end

def ordered_by_ancestry(order = nil)
Expand Down
8 changes: 4 additions & 4 deletions test/concerns/orphan_strategies_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -81,12 +81,12 @@ def test_orphan_adopt_strategy
n5 = model.create!(:parent => n4) #create child with parent=n4, depth = 3
n2.destroy # delete a node with desecendants
assert_equal(model.find(n3.id).parent,n1, "orphan's not parentified" )
assert_equal(model.find(n5.id).ancestor_ids,[n1.id,n4.id], "ancestry integrity not maintained")
assert_equal(model.find(n5.id).ancestor_ids, [n1.id,n4.id], "ancestry integrity not maintained")
n1.destroy # delete a root node with desecendants
assert_nil(model.find(n3.id).ancestry," new root node has no empty ancestry string")
assert_equal(model.find(n3.id).valid?,true," new root node is not valid")
assert_nil(model.find(n3.id).parent_id," Children of the deleted root not rootfied")
assert_equal(model.find(n5.id).ancestor_ids,[n4.id],"ancestry integrity not maintained")
assert_equal(model.find(n3.id).valid?, true, " new root node is not valid")
assert_nil(model.find(n3.id).parent_id, " Children of the deleted root not rootfied")
assert_equal(model.find(n5.id).ancestor_ids, [n4.id], "ancestry integrity not maintained")
end
end

Expand Down
12 changes: 8 additions & 4 deletions test/concerns/scopes_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@ class ScopesTest < ActiveSupport::TestCase
def test_scopes
AncestryTestDatabase.with_model :depth => 3, :width => 3 do |model, roots|
# Roots assertion
assert_equal roots.map(&:first), model.roots.to_a
assert_equal roots.map(&:first).sort, model.roots.to_a.sort

# All roots are root's siblings (want to change this)
a_root = roots.first.first
assert_equal model.siblings_of(a_root).sort, roots.map(&:first).sort

model.all.each do |test_node|
# Assertions for ancestors_of named scope
Expand Down Expand Up @@ -55,12 +59,12 @@ def test_order_by
AncestryTestDatabase.with_model :depth => 3, :width => 3 do |model, roots|
# not thrilled with this. mac postgres has odd sorting requirements
if ENV["DB"].to_s =~ /pg/ && RUBY_PLATFORM !~ /x86_64-darwin/
expected = model.all.sort_by { |m| [m.ancestry.to_s.gsub('/',''), m.id.to_i] }
expected = model.all.sort_by { |m| [m.ancestor_ids.map(&:to_s).join, m.id.to_i] }
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the goal is to focus on a single format agnostic field

else
expected = model.all.sort_by { |m| [m.ancestry.to_s, m.id.to_i] }
expected = model.all.sort_by { |m| [m.ancestor_ids.map(&:to_s), m.id.to_i] }
end
actual = model.ordered_by_ancestry_and(:id)
assert_equal expected.map { |r| [r.ancestry, r.id.to_s] }, actual.map { |r| [r.ancestry, r.id.to_s] }
assert_equal expected.map { |r| [r.ancestor_ids, r.id.to_s] }, actual.map { |r| [r.ancestor_ids, r.id.to_s] }
end
end

Expand Down
4 changes: 1 addition & 3 deletions test/concerns/update_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,7 @@ def test_node_creation_in_after_commit
end
end
model.create!(:idx => 3)
# In the error case, the ancestry on each item will only contain the parent's id,
# and not the entire ancestry tree.
assert_equal '1/2/3', children.first.ancestry
assert_equal [1,2,3], children.first.ancestor_ids
end
end
end
23 changes: 5 additions & 18 deletions test/concerns/validations_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,42 +8,29 @@ def test_ancestry_column_validation
node.send :write_attribute, model.ancestry_column, value
node.valid?; assert node.errors[model.ancestry_column].blank?
end
['1/3/', '/2/3', 'a', 'a/b', '-34', '/54'].each do |value|
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we used to focus that the format of this field was serialized correctly.

The new functionality parses these values just fine.
And since we are moving the serialization to a single method - the chances of failure is much lower than when this field was formed all over the code.

['a', 'a/b', '-34'].each do |value|
node.send :write_attribute, model.ancestry_column, value
node.valid?; assert !node.errors[model.ancestry_column].blank?
end
end
end

def test_ancestry_column_validation_alt
AncestryTestDatabase.with_model(:primary_key_format => /[a-z]+/) do |model|
node = model.create
AncestryTestDatabase.with_model(:id => :string, :primary_key_format => /[a-z]/) do |model|
node = model.create(:id => 'z')
['a', 'a/b', 'a/b/c', nil].each do |value|
node.send :write_attribute, model.ancestry_column, value
node.valid?; assert node.errors[model.ancestry_column].blank?
end
['1', '1/2', 'a/b/', '/a/b'].each do |value|
['1', '1/2', 'a-b/c'].each do |value|
node.send :write_attribute, model.ancestry_column, value
node.valid?; assert !node.errors[model.ancestry_column].blank?
end
end
end

def test_ancestry_column_validation_full_key
AncestryTestDatabase.with_model(:primary_key_format => /\A[a-z]+(\/[a-z]+)*\Z/) do |model|
node = model.create
['a', 'a/b', 'a/b/c', nil].each do |value|
node.send :write_attribute, model.ancestry_column, value
node.valid?; assert node.errors[model.ancestry_column].blank?
end
['1', '1/2', 'a/b/', '/a/b'].each do |value|
node.send :write_attribute, model.ancestry_column, value
node.valid?; assert !node.errors[model.ancestry_column].blank?
end
end
end

def test_validate_ancestry_exclude_self
def test_ancestry_validation_exclude_self
AncestryTestDatabase.with_model do |model|
parent = model.create!
child = parent.children.create!
Expand Down
5 changes: 4 additions & 1 deletion test/environment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,10 @@ def self.with_model options = {}
extra_columns = options.delete(:extra_columns)
default_scope_params = options.delete(:default_scope_params)

ActiveRecord::Base.connection.create_table 'test_nodes' do |table|
table_options={}
table_options[:id] = options.delete(:id) if options.key?(:id)

ActiveRecord::Base.connection.create_table 'test_nodes', table_options do |table|
table.string options[:ancestry_column] || :ancestry
table.integer options[:depth_cache_column] || :ancestry_depth if options[:cache_depth]
if options[:counter_cache]
Expand Down