Closed tddrmllr closed 1 year ago
Seems like it's been that way for a little while, so maybe it's not an error, but I'm trying to build a test, and the descendants
method is not picking up any records.
I just barely installed the gem, ran the migration, and added has_ancestry
to the model.
Migration:
class AddAncestryToDepartments < ActiveRecord::Migration[7.0]
def change
change_table(:departments) do |t|
t.string "ancestry", collation: 'C', null: false, default: ""
t.index "ancestry"
end
end
end
Note that I added the default: ""
option, because I was getting a PG::NotNullViolation
when attempting to run the migration. Not sure if that would mess anything up, but seems unlikely. (Probably a different question about how to deal with the not null error.)
Active record class:
class Department < ApplicationRecord
has_ancestry
end
Test:
class DepartmentTest < UnitTest
attr_accessor :department
setup do
self.department = departments(:marketing_department)
end
def test_ancestry
dev = departments(:dev_department)
dev.update!(parent: department)
assert_includes department.descendants, dev # This fails. department.descendants returns []
assert_equal department, dev.parent # this passes
end
end
Hack:
class Department < ApplicationRecord
has_ancestry
def self.descendants_by_ancestry(ancestry)
arel_table[ancestry_column].matches("%#{ancestry}%", nil, true)
end
end
If I do this ☝️ the test passes.
And with a test too! Really appreciate the notes.
Looks like 4-3 broke descendants for materialized_path and materialized_path2 The main contributions in this version are #589 and #597 So I'm trying to figure out if those points introduced something or if there was a hidden error before hand.
I think #636 overlaps with this and have a good chunk of time this weekend to track it down.
The %
does not belong at the beginning of the string. But thank you for telling me this will fix it. I'm guessing that means the ancestries in the db are showing 2/
instead of /2/
.
If you are using materialized_path2, then set the default to "/"
I think this will fix the %
issue. There are probably other issues as well, but that will get you moving forward. Rails works best when the database gets the default for the column from the database and not from ruby.
If you are using materialized_path
, then the default should be nil and the column should be nullable.
I appreciate you pointing me to code that is not working and will take it from here.
Thanks again for the test and let me know if the db default fixes things for now.
Ruby: 3.0.3
ActiveRecord: 6.1.7.3
# /usr/bin/env ruby
# test/concerns/repro_638_test.rb
require_relative '../environment'
class Repro638Test < ActiveSupport::TestCase
def test_has_ancestry_in_after_save
AncestryTestDatabase.with_model do |model|
node1 = model.create!
node11 = model.create!
node11.update!(parent: node1)
assert_includes node1.descendants, node11
end
end
end
I ran on branch master
and 4-3-stable
.
I ran with defaulting the column in the db to "", "/", and nil (manually changed test/environment.rb
)
Maybe I'm running it wrong, but both branches seem to be working for me.
Do you have ideas on now to make my test more similar to yours? I do not have a department table written and I do not have your fixtures. Maybe the fixture sets the ancestry column and it is in materialized_path not materialized_path2?
DB=sqlite3 FORMAT=materialized_path UPDATE_STRATEGY=ruby ruby test/concerns/repro_638_test.rb
DB=sqlite3 FORMAT=materialized_path2 UPDATE_STRATEGY=ruby ruby test/concerns/repro_638_test.rb
Seems like t.string "ancestry", collation: 'C', null: false, default: "/"
fixed it for me.
Also, you may want to check out the migrating a model section. If you are adding a not null column to an already populated database, you'll need to add the column as nullable, update the field, and then change the null constraint
closing for now. let me know if you are still having issues. Also, if you have any suggestions for making the documentation better so you can see the update/migrating section, please let me know
The
descendants
scope doesn't seem to be working for MaterializedPath2, and it looks like it's because the SQL is missing a%
at the beginning? I'm definitely not an SQL expert, so I may be missing something here.The
descendants_by_ancestry
method currently hasbut I believe it should be
The main difference is
"#{ancestry}%"
vs"%#{ancestry}%"
.