diff --git a/backend/spec/db/delete_discarded_tags_spec.rb b/backend/spec/db/delete_discarded_tags_spec.rb new file mode 100644 index 0000000..3882e0d --- /dev/null +++ b/backend/spec/db/delete_discarded_tags_spec.rb @@ -0,0 +1,123 @@ +require 'rails_helper' +require_relative '../../db/migrate/20260921020000_delete_discarded_records_from_tags' +require_relative '../../db/migrate/20260921030000_delete_discarded_records_from_tag_names' + +RSpec.describe 'discarded tag cleanup migrations' do + [DeleteDiscardedRecordsFromTags, DeleteDiscardedRecordsFromTagNames].each do |migration_class| + it "rejects rollback of #{ migration_class.name }" do + expect { migration_class.new.down } + .to raise_error(ActiveRecord::IrreversibleMigration) + end + end + + context 'with legacy records' do + self.use_transactional_tests = false + + before do + record_class = Class.new(ActiveRecord::Base) do + self.abstract_class = true + end + stub_const('TagCleanupMigrationRecord', record_class) + config = ActiveRecord::Base.connection_db_config.configuration_hash + @database = "btrc_hub_test_tag_cleanup_#{ Process.pid }_#{ SecureRandom.hex(4) }" + record_class.establish_connection(config.merge(database: nil)) + @connection = record_class.lease_connection + @connection.create_database(@database) + @database_created = true + @connection.execute("USE #{ @connection.quote_table_name(@database) }") + end + + after do + @connection.drop_database(@database) if @database_created + ensure + TagCleanupMigrationRecord.remove_connection + end + + before do + @connection.create_table(:tag_names) do |t| + t.string :name, null: false, index: { unique: true } + t.bigint :canonical_id + t.datetime :discarded_at, index: true + end + @connection.add_foreign_key(:tag_names, :tag_names, column: :canonical_id) + @connection.create_table(:tags) do |t| + t.references :tag_name, null: false, foreign_key: true, index: { unique: true } + t.datetime :discarded_at, index: true + end + @connection.create_table(:nico_tag_relations) do |t| + t.references :tag, null: false, foreign_key: true + t.references :nico_tag, null: false, foreign_key: { to_table: :tags } + end + @connection.create_table(:tag_implications) do |t| + t.references :tag, null: false, foreign_key: true + t.references :parent_tag, null: false, foreign_key: { to_table: :tags } + end + [:tag_versions, :nico_tag_versions, :material_versions].each do |table| + @connection.create_table(table) do |t| + t.references :tag, null: false, foreign_key: true + end + end + + @connection.execute(<<~SQL) + INSERT INTO tag_names (id, name, canonical_id, discarded_at) VALUES + (1, 'kept', NULL, NULL), + (2, 'merged_alias', 1, NULL), + (3, 'nico:deleted', NULL, '2026-09-20'), + (4, 'nico:kept', NULL, NULL), + (5, 'deleted_name', NULL, '2026-09-20') + SQL + @connection.execute(<<~SQL) + INSERT INTO tags (id, tag_name_id, discarded_at) VALUES + (1, 1, NULL), (2, 2, '2026-09-20'), + (3, 3, '2026-09-20'), (4, 4, NULL) + SQL + @connection.execute(<<~SQL) + INSERT INTO nico_tag_relations (id, tag_id, nico_tag_id) VALUES + (1, 1, 4), (2, 2, 4), (3, 1, 3), (4, 2, 3) + SQL + @connection.execute(<<~SQL) + INSERT INTO tag_implications (id, tag_id, parent_tag_id) VALUES + (1, 1, 4), (2, 2, 1), (3, 1, 2), (4, 2, 3) + SQL + @connection.execute('INSERT INTO tag_versions (tag_id) VALUES (1), (2)') + @connection.execute('INSERT INTO nico_tag_versions (tag_id) VALUES (3), (4)') + @connection.execute('INSERT INTO material_versions (tag_id) VALUES (1), (2)') + end + + it 'removes discarded records and their links while retaining aliases and history' do + [DeleteDiscardedRecordsFromTags, DeleteDiscardedRecordsFromTagNames].each do |klass| + migration = klass.new + allow(migration).to receive(:connection).and_return(@connection) + migration.suppress_messages { migration.up } + end + + expect(@connection.select_values('SELECT id FROM tags ORDER BY id')).to eq([1, 4]) + expect(@connection.select_rows('SELECT id, canonical_id FROM tag_names ORDER BY id')) + .to eq([[1, nil], [2, 1], [4, nil]]) + expect(@connection.select_values('SELECT id FROM nico_tag_relations')).to eq([1]) + expect(@connection.select_values('SELECT id FROM tag_implications')).to eq([1]) + expect(@connection.select_values('SELECT tag_id FROM tag_versions ORDER BY tag_id')) + .to eq([1, 2]) + expect(@connection.select_values('SELECT tag_id FROM nico_tag_versions ORDER BY tag_id')) + .to eq([3, 4]) + expect(@connection.select_values('SELECT tag_id FROM material_versions ORDER BY tag_id')) + .to eq([1, 2]) + + [:tags, :tag_names].each do |table| + expect(@connection.column_exists?(table, :discarded_at)).to be(false) + expect(@connection.index_exists?(table, :discarded_at)).to be(false) + end + [:tag_versions, :nico_tag_versions, :material_versions].each do |table| + expect(@connection.foreign_key_exists?(table, :tags, column: :tag_id)).to be(false) + end + expect(@connection.foreign_key_exists?(:tags, :tag_names)).to be(true) + expect(@connection.foreign_key_exists?(:tag_names, :tag_names, column: :canonical_id)) + .to be(true) + expect(@connection.index_exists?(:tag_names, :name, unique: true)).to be(true) + expect(@connection.index_exists?(:tags, :tag_name_id, unique: true)).to be(true) + [:nico_tag_relations, :tag_implications].each do |table| + expect(@connection.foreign_keys(table).map(&:to_table)).to eq(['tags', 'tags']) + end + end + end +end diff --git a/backend/spec/models/tag_name_sanitisation_rule_spec.rb b/backend/spec/models/tag_name_sanitisation_rule_spec.rb index ea14461..b82f9fa 100644 --- a/backend/spec/models/tag_name_sanitisation_rule_spec.rb +++ b/backend/spec/models/tag_name_sanitisation_rule_spec.rb @@ -57,7 +57,7 @@ RSpec.describe TagNameSanitisationRule, type: :model do it 'deletes the source tag_name' do described_class.apply! - expect(TagName.exists?(source.id)).to be(false) + expect(TagName.unscoped.exists?(source.id)).to be(false) expect(existing.reload.name).to eq('foobar') end end @@ -75,7 +75,27 @@ RSpec.describe TagNameSanitisationRule, type: :model do described_class.apply! expected_tag_name_id = existing.canonical_id || existing.id expect(source_tag.reload.tag_name_id).to eq(expected_tag_name_id) - expect(TagName.exists?(source_tag_name_id)).to be(false) + expect(TagName.unscoped.exists?(source_tag_name_id)).to be(false) + end + end + + context 'when the sanitised name is an alias of an existing tag' do + let!(:existing_tag) { create(:tag) } + let!(:alias_name) do + TagName.create!(name: 'foobar', canonical: existing_tag.tag_name) + end + let!(:source) do + TagName.create!(name: 'tmp').tap do |tn| + tn.update_columns(name: 'foo_bar', updated_at: Time.current) + end + end + + it 'deletes only the source and preserves the alias and its canonical tag' do + described_class.apply! + + expect(TagName.unscoped.exists?(source.id)).to be(false) + expect(alias_name.reload.canonical).to eq(existing_tag.tag_name) + expect(Tag.find(existing_tag.id)).to eq(existing_tag) end end @@ -92,13 +112,15 @@ RSpec.describe TagNameSanitisationRule, type: :model do end it 'merges the source tag into the existing tag and deletes the source tag_name' do - expect(TagName.find_by(name: 'foobar')&.tag&.id).to eq(existing_tag.id) - expect(TagName.find_by(name: 'foo_bar')&.tag&.id).to eq(source_tag.id) + post = create(:post) + PostTag.create!(post:, tag: source_tag) described_class.apply! - expect(Tag.exists?(source_tag.id)).to be(false) - expect(TagName.exists?(source_tag.tag_name_id)).to be(false) + expect(Tag.unscoped.exists?(source_tag.id)).to be(false) + expect(TagName.unscoped.exists?(source_tag_name_id)).to be(false) + expect(post.reload.tags).to contain_exactly(existing_tag) + expect(existing_tag.reload.name).to eq('foobar') end end end diff --git a/backend/spec/models/tag_spec.rb b/backend/spec/models/tag_spec.rb index 79b0ac0..e532e25 100644 --- a/backend/spec/models/tag_spec.rb +++ b/backend/spec/models/tag_spec.rb @@ -173,6 +173,47 @@ RSpec.describe Tag, type: :model do end end + describe '.find_or_create_by_tag_name!' do + it 'creates a tag and name with the requested category after stripping whitespace' do + tag = nil + + expect { + tag = described_class.find_or_create_by_tag_name!( + ' lookup_new ', category: :character) + }.to change(Tag, :count).by(1).and change(TagName, :count).by(1) + + expect(tag.name).to eq('lookup_new') + expect(tag.category).to eq('character') + end + + it 'reuses the canonical tag for an alias without changing its category' do + tag = create(:tag, category: :character) + alias_name = TagName.create!(name: 'lookup_alias', canonical: tag.tag_name) + found = nil + + expect { + found = described_class.find_or_create_by_tag_name!( + alias_name.name, category: :general) + }.to change(Tag, :count).by(0).and change(TagName, :count).by(0) + + expect(found).to eq(tag) + expect(found.category).to eq('character') + end + + it 'creates a tag for an existing canonical name reached through an alias' do + canonical = create(:tag_name) + alias_name = TagName.create!(name: 'lookup_alias', canonical:) + tag = nil + + expect { + tag = described_class.find_or_create_by_tag_name!( + alias_name.name, category: :general) + }.to change(Tag, :count).by(1).and change(TagName, :count).by(0) + + expect(tag.tag_name).to eq(canonical) + end + end + describe '.merge_tags!' do let!(:target_tag) { create(:tag, category: :general) } let!(:source_tag) { create(:tag, category: :general) } @@ -185,17 +226,14 @@ RSpec.describe Tag, type: :model do context 'when merging a simple source tag' do let!(:source_post_tag) { PostTag.create!(post: post_record, tag: source_tag) } - it 'deletes the source link, links the target, and aliases the discarded source tag' do + it 'deletes the source tag, moves its post link, and keeps its name as an alias' do described_class.merge_tags!(target_tag, [source_tag]) target_link = PostTag.find_by(post: post_record, tag: target_tag) expect(PostTag.exists?(post: post_record, tag: source_tag)).to be(false) expect(target_link).to be_present - expect(source_tag.reload.post_count).to eq(0) - - expect(Tag.with_discarded.find(source_tag.id)).to be_discarded - expect(TagName.with_discarded.find(source_tag_name.id)).not_to be_discarded + expect(Tag.unscoped.exists?(source_tag.id)).to be(false) expect(source_tag_name.reload.canonical_id).to eq(target_tag.tag_name_id) expect(target_tag.reload.post_count).to eq(1) end @@ -218,17 +256,76 @@ RSpec.describe Tag, type: :model do expect(PostTag.exists?(post: post_record, tag: source_tag)).to be(false) expect(target_links).to contain_exactly(target_post_tag) - expect(source_tag.reload.post_count).to eq(0) expect(PostTagSection.where(post: post_record, tag: source_tag)).to be_empty expect(target_post_tag.reload.sections).to contain_exactly(target_section) - expect(Tag.with_discarded.find(source_tag.id)).to be_discarded - expect(TagName.with_discarded.find(source_tag_name.id)).not_to be_discarded + expect(Tag.unscoped.exists?(source_tag.id)).to be(false) expect(source_tag_name.reload.canonical_id).to eq(target_tag.tag_name_id) expect(target_tag.reload.post_count).to eq(1) end end + it 'keeps source history and records the new target alias after deleting the source' do + user = create_member_user! + source_name = source_tag.name + source_alias = TagName.create!(name: 'merge_alias', canonical: source_tag_name) + TagVersioning.ensure_snapshot!(source_tag, created_by_user: user) + original_version = source_tag.tag_versions.first + + described_class.merge_tags!(target_tag, [source_tag], created_by_user: user) + + versions = TagVersion.where(tag_id: source_tag.id).order(:version_no) + expect(versions.pluck(:version_no, :event_type)) + .to eq([[1, 'create'], [2, 'discard']]) + expect(versions.first).to eq(original_version) + expect(versions.last).to have_attributes( + name: source_name, aliases: source_alias.name, created_by_user: user) + expect(Tag.unscoped.exists?(source_tag.id)).to be(false) + + target_versions = target_tag.tag_versions.order(:version_no) + expect(target_versions.pluck(:event_type)).to eq(['create', 'update']) + expect(target_versions.last.aliases.split).to eq([source_name]) + end + + it 'deletes source relationships while preserving unrelated relationships' do + parent = create(:tag) + child = create(:tag) + nico_tag = create(:tag, :nico) + TagImplication.create!(tag: source_tag, parent_tag: parent) + TagImplication.create!(tag: child, parent_tag: source_tag) + kept_implication = TagImplication.create!(tag: target_tag, parent_tag: parent) + NicoTagRelation.create!(tag: source_tag, nico_tag:) + kept_relation = NicoTagRelation.create!(tag: target_tag, nico_tag:) + TagSimilarity.create!(tag: source_tag, target_tag:, cos: 0.5) + TagSimilarity.create!(tag: target_tag, target_tag: source_tag, cos: 0.5) + kept_similarity = TagSimilarity.create!(tag: target_tag, target_tag: parent, + cos: 0.5) + + described_class.merge_tags!(target_tag, [source_tag]) + + expect(TagImplication.all).to contain_exactly(kept_implication) + expect(NicoTagRelation.all).to contain_exactly(kept_relation) + expect(TagSimilarity.all).to contain_exactly(kept_similarity) + expect(TagVersion.where(tag_id: source_tag.id).order(:version_no).last.parent_tag_ids) + .to eq(parent.id.to_s) + end + + it 'preserves material history referencing the deleted source tag' do + source_tag.update!(category: :material) + target_tag.update!(category: :material) + material = Material.create!(tag: source_tag, url: 'https://example.com/material') + version = MaterialVersionRecorder.record!( + material:, event_type: :create, created_by_user: nil) + material.update!(tag: target_tag) + + described_class.merge_tags!(target_tag, [source_tag]) + + expect(version.reload).to have_attributes( + tag_id: source_tag.id, tag_name: source_tag_name.name, tag_category: 'material') + expect(Tag.unscoped.exists?(source_tag.id)).to be(false) + expect(material.reload.tag).to eq(target_tag) + end + context 'when source_tags includes the target itself' do let!(:source_post_tag) { PostTag.create!(post: post_record, tag: source_tag) } @@ -238,7 +335,7 @@ RSpec.describe Tag, type: :model do target_link = PostTag.find_by(post: post_record, tag: target_tag) expect(Tag.find(target_tag.id)).to be_present - expect(Tag.with_discarded.find(source_tag.id)).to be_discarded + expect(Tag.unscoped.exists?(source_tag.id)).to be(false) expect(PostTag.exists?(post: post_record, tag: source_tag)).to be(false) expect(target_link).to be_present expect(source_tag_name.reload.canonical_id).to eq(target_tag.tag_name_id) @@ -263,7 +360,7 @@ RSpec.describe Tag, type: :model do ) end - it 'still merges, but discards the source tag_name instead of aliasing it' do + it 'still merges and keeps the source name as an alias without validating it' do described_class.merge_tags!(target_tag, [source_tag]) target_link = PostTag.find_by(post: post_record, tag: target_tag) @@ -271,7 +368,8 @@ RSpec.describe Tag, type: :model do expect(PostTag.exists?(post: post_record, tag: source_tag)).to be(false) expect(target_link).to be_present - expect(Tag.with_discarded.find(source_tag.id)).to be_discarded + expect(Tag.unscoped.exists?(source_tag.id)).to be(false) + expect(source_tag_name.reload.canonical_id).to eq(target_tag.tag_name_id) expect(target_tag.reload.post_count).to eq(1) end end @@ -288,17 +386,23 @@ RSpec.describe Tag, type: :model do message: 'init') end - it 'rolls back the transaction' do + it 'rolls back earlier deletions, links, and history when a later source has a wiki' do + earlier_source = create(:tag) + earlier_name = earlier_source.tag_name source_section = create(:post_tag_section, post: post_record, tag: source_tag, begin_ms: 1000, end_ms: 2000) expect { - described_class.merge_tags!(target_tag, [source_tag]) + described_class.merge_tags!(target_tag, [earlier_source, source_tag]) }.to raise_error(ActiveRecord::RecordInvalid) - expect(Tag.with_discarded.find(source_tag.id)).not_to be_discarded - expect(TagName.with_discarded.find(source_tag_name.id)).not_to be_discarded + expect(Tag.unscoped.exists?(earlier_source.id)).to be(true) + expect(earlier_name.reload.canonical_id).to be_nil + expect(TagVersion.where(tag_id: [earlier_source.id, source_tag.id, target_tag.id])) + .to be_empty + expect(Tag.unscoped.exists?(source_tag.id)).to be(true) + expect(TagName.unscoped.exists?(source_tag_name.id)).to be(true) expect(source_post_tag.reload.tag_id).to eq(source_tag.id) expect(source_post_tag.sections).to contain_exactly(source_section) expect(PostTag.find_by(post: post_record, tag: target_tag)).to be_nil @@ -309,21 +413,45 @@ RSpec.describe Tag, type: :model do end context 'when merging a nico source tag' do - let!(:target_tag) { create(:tag, category: :nico, name: 'nico:foo') } - let!(:source_tag) { create(:tag, category: :nico, name: 'nico:bar') } + let!(:target_tag) do + create(:tag, category: :nico, tag_name: create(:tag_name, name: 'nico:foo')) + end + let!(:source_tag) do + create(:tag, category: :nico, tag_name: create(:tag_name, name: 'nico:bar')) + end let!(:source_tag_name_id) { source_tag.tag_name_id } - it 'discards the source tag_name instead of aliasing it' do + it 'deletes the source tag and name instead of keeping an alias' do described_class.merge_tags!(target_tag, [source_tag]) - discarded_source_tag = Tag.with_discarded.find(source_tag.id) - discarded_source_tag_name = TagName.with_discarded.find(source_tag_name_id) - - expect(discarded_source_tag).to be_discarded - expect(discarded_source_tag_name).to be_discarded - expect(discarded_source_tag_name.canonical_id).to be_nil + expect(Tag.unscoped.exists?(source_tag.id)).to be(false) + expect(TagName.unscoped.exists?(source_tag_name_id)).to be(false) expect(target_tag.reload.post_count).to eq(0) end + + it 'keeps nico history while deleting source links and allows recreating the name' do + linked_tag = create(:tag) + NicoTagRelation.create!(nico_tag: source_tag, tag: linked_tag) + kept_relation = NicoTagRelation.create!(nico_tag: target_tag, tag: linked_tag) + user = create_member_user! + source_name = source_tag.name + + described_class.merge_tags!(target_tag, [source_tag], created_by_user: user) + + expect(NicoTagRelation.all).to contain_exactly(kept_relation) + versions = NicoTagVersion.where(tag_id: source_tag.id).order(:version_no) + expect(versions.pluck(:version_no, :event_type)) + .to eq([[1, 'create'], [2, 'discard']]) + expect(versions.last).to have_attributes( + name: source_name, linked_tags: linked_tag.name, created_by_user: user) + + recreated = described_class.find_or_create_by_tag_name!(source_name, category: :nico) + + expect(recreated.id).not_to eq(source_tag.id) + expect(recreated.tag_name_id).not_to eq(source_tag_name_id) + expect(recreated.nico_tag_versions).to be_empty + expect(versions.reload.size).to eq(2) + end end def snapshot_tags(post) diff --git a/backend/spec/requests/posts_spec.rb b/backend/spec/requests/posts_spec.rb index 350b5e7..fb08d21 100644 --- a/backend/spec/requests/posts_spec.rb +++ b/backend/spec/requests/posts_spec.rb @@ -1252,9 +1252,7 @@ RSpec.describe 'Posts API', type: :request do context 'when nico tag already exists in tags' do before do - Tag.find_undiscard_or_create_by!( - tag_name: TagName.find_undiscard_or_create_by!(name: 'nico:nico_tag'), - category: :nico) + Tag.find_or_create_by_tag_name!('nico:nico_tag', category: :nico) end it 'returns 422 with tag field errors' do @@ -1580,9 +1578,7 @@ RSpec.describe 'Posts API', type: :request do context 'when nico tag already exists in tags' do before do - Tag.find_undiscard_or_create_by!( - tag_name: TagName.find_undiscard_or_create_by!(name: 'nico:nico_tag'), - category: :nico) + Tag.find_or_create_by_tag_name!('nico:nico_tag', category: :nico) end it 'returns 422 with tag field errors' do diff --git a/backend/spec/tasks/nico_sync_spec.rb b/backend/spec/tasks/nico_sync_spec.rb index f708608..1ec5b08 100644 --- a/backend/spec/tasks/nico_sync_spec.rb +++ b/backend/spec/tasks/nico_sync_spec.rb @@ -8,8 +8,7 @@ RSpec.describe 'nico:sync' do end def create_tag!(name, category:) - tn = TagName.find_undiscard_or_create_by!(name: name.to_s.strip) - Tag.find_undiscard_or_create_by!(tag_name_id: tn.id) { |t| t.category = category } + Tag.find_or_create_by_tag_name!(name, category:) end def link_nico_to_tag!(nico_tag, tag)