From 589fbb0aaec359b23d8e3a9c0cd9bab57b4ea4c4 Mon Sep 17 00:00:00 2001 From: miteruzo Date: Tue, 14 Jul 2026 00:54:43 +0900 Subject: [PATCH] #409 --- backend/db/seeds.rb | 37 +++ backend/spec/db/seeds_spec.rb | 65 +++++ backend/spec/models/post_spec.rb | 68 +++++ .../models/post_url_sanitisation_rule_spec.rb | 265 ++++++++++++++++++ backend/spec/tasks/nico_export_spec.rb | 2 +- 5 files changed, 436 insertions(+), 1 deletion(-) create mode 100644 backend/spec/db/seeds_spec.rb create mode 100644 backend/spec/models/post_spec.rb create mode 100644 backend/spec/models/post_url_sanitisation_rule_spec.rb diff --git a/backend/db/seeds.rb b/backend/db/seeds.rb index 067d073..c88f60a 100644 --- a/backend/db/seeds.rb +++ b/backend/db/seeds.rb @@ -8,6 +8,43 @@ # MovieGenre.find_or_create_by!(name: genre_name) # end +post_url_sanitisation_rules = [ + { priority: 10, + source_pattern: '\Ahttps?://youtu\.be/([^/?#]+)(?:[?#].*)?\z', + replacement: 'https://www.youtube.com/watch?v=\1' }, + { priority: 20, + source_pattern: '\Ahttps?://(?:www\.|m\.)?youtube\.com/live/([^/?#]+)(?:[?#].*)?\z', + replacement: 'https://www.youtube.com/watch?v=\1' }, + { priority: 30, + source_pattern: '\Ahttps?://(?:www\.|m\.)?youtube\.com/shorts/([^/?#]+)(?:[?#].*)?\z', + replacement: 'https://www.youtube.com/watch?v=\1' }, + { priority: 40, + source_pattern: '\Ahttps?://(?:www\.|m\.)?youtube\.com/embed/([^/?#]+)(?:[?#].*)?\z', + replacement: 'https://www.youtube.com/watch?v=\1' }, + { priority: 50, + source_pattern: + '\Ahttps?://(?:www\.|m\.)?youtube\.com/watch\?(?:[^#&]+&)*v=([^&#]+)(?:[&#].*)?\z', + replacement: 'https://www.youtube.com/watch?v=\1' }, + { priority: 60, + source_pattern: '\Ahttps?://nico\.ms/([^/?#]+)(?:[?#].*)?\z', + replacement: 'https://www.nicovideo.jp/watch/\1' }, + { priority: 70, + source_pattern: '\Ahttps?://(?:www\.)?nicovideo\.jp/watch/([^?#/]+)(?:[?#].*)?\z', + replacement: 'https://www.nicovideo.jp/watch/\1' } +] + +post_url_sanitisation_rule_scope = PostUrlSanitisationRule.unscoped + +post_url_sanitisation_rules.each do |attributes| + priority = attributes.fetch(:priority) + source_pattern = attributes.fetch(:source_pattern) + + next if post_url_sanitisation_rule_scope.exists?(priority:) + next if post_url_sanitisation_rule_scope.exists?(source_pattern:) + + post_url_sanitisation_rule_scope.create!(attributes) +end + material_sync_source_uri = ENV['MATERIAL_SYNC_SOURCE_URI'] material_sync_source_file_id = ENV['MATERIAL_SYNC_SOURCE_FILE_ID'] diff --git a/backend/spec/db/seeds_spec.rb b/backend/spec/db/seeds_spec.rb new file mode 100644 index 0000000..57eba67 --- /dev/null +++ b/backend/spec/db/seeds_spec.rb @@ -0,0 +1,65 @@ +require 'rails_helper' + +RSpec.describe 'database seeds' do + before do + PostUrlSanitisationRule.unscoped.delete_all + end + + it 'registers the initial post URL sanitisation rules' do + load_seeds + + urls = { + 'https://youtu.be/abc123?si=share' => youtube_url('abc123'), + 'https://www.youtube.com/live/abc123?t=10' => youtube_url('abc123'), + 'https://youtube.com/shorts/abc123?feature=share' => youtube_url('abc123'), + 'https://m.youtube.com/embed/abc123' => youtube_url('abc123'), + 'https://youtube.com/watch?feature=share&v=abc123&t=10' => youtube_url('abc123'), + 'https://nico.ms/sm123?from=share#fragment' => nico_url('sm123'), + 'https://www.nicovideo.jp/watch/sm123?ref=share#fragment' => nico_url('sm123') + } + + expect(PostUrlSanitisationRule.count).to eq(7) + urls.each do |url, canonical_url| + expect(PostUrlSanitisationRule.sanitise(url)).to eq(canonical_url) + end + end + + it 'does not overwrite or restore an existing rule' do + rule = PostUrlSanitisationRule.create!( + priority: 10, + source_pattern: '\Ahttps://example\.com/custom\z', + replacement: 'https://example.com/replacement' + ) + rule.discard! + original_attributes = rule.reload.attributes + + 2.times { load_seeds } + + persisted_rule = PostUrlSanitisationRule.unscoped.find(10) + expect(persisted_rule.attributes).to eq(original_attributes) + expect(PostUrlSanitisationRule.unscoped.count).to eq(7) + end + + it 'does not duplicate a rule moved to another priority' do + source_pattern = '\Ahttps?://youtu\.be/([^/?#]+)(?:[?#].*)?\z' + PostUrlSanitisationRule.create!( + priority: 80, + source_pattern:, + replacement: 'https://example.com/custom/\1' + ) + + load_seeds + + rules = PostUrlSanitisationRule.unscoped + expect(rules.where(source_pattern:).count).to eq(1) + expect(rules.find(80).replacement).to eq('https://example.com/custom/\1') + end + + def load_seeds + load Rails.root.join('db/seeds.rb') + end + + def youtube_url(video_id) = "https://www.youtube.com/watch?v=#{ video_id }" + + def nico_url(video_id) = "https://www.nicovideo.jp/watch/#{ video_id }" +end diff --git a/backend/spec/models/post_spec.rb b/backend/spec/models/post_spec.rb new file mode 100644 index 0000000..627b28f --- /dev/null +++ b/backend/spec/models/post_spec.rb @@ -0,0 +1,68 @@ +require 'rails_helper' + +RSpec.describe Post, type: :model do + before do + PostUrlSanitisationRule.unscoped.delete_all + end + + describe 'URL normalisation' do + it 'normalises the HTTP URL before applying sanitisation rules' do + PostUrlSanitisationRule.create!( + priority: 10, + source_pattern: '\\Ahttps://example\\.com/videos/([^/]+)\\z', + replacement: 'https://example.com/watch/\\1' + ) + + post = described_class.create!( + title: 'normalised URL', + url: ' https://EXAMPLE.com/videos/123/ ' + ) + + expect(post.url).to eq('https://example.com/watch/123') + end + + it 'does not normalise an unchanged URL when another attribute changes' do + post = create(:post) + post.update_column(:url, 'https://EXAMPLE.com/unchanged/') + + post.update!(title: 'updated title') + + expect(post.reload.url).to eq('https://EXAMPLE.com/unchanged/') + end + + it 'validates the sanitised URL length' do + path = 'a' * 375 + + PostUrlSanitisationRule.create!( + priority: 10, + source_pattern: '\\Ahttps://example\\.com/(a+)\\z', + replacement: 'https://example.com/\\1\\1' + ) + + post = described_class.new( + title: 'long URL', + url: "https://example.com/#{ path }" + ) + + expect(post).to be_invalid + expect(post.errors.details.fetch(:url)).to include( + error: :too_long, + count: 768 + ) + end + + it 'validates uniqueness after sanitisation' do + PostUrlSanitisationRule.create!( + priority: 10, + source_pattern: '\\Ahttps://example\\.com/alias\\z', + replacement: 'https://example.com/canonical' + ) + create(:post, url: 'https://example.com/canonical') + + post = described_class.new(title: 'duplicate URL', url: 'https://example.com/alias') + + expect(post).to be_invalid + expect(post.errors.details.fetch(:url)).to include(error: :taken, value: post.url) + end + end +end diff --git a/backend/spec/models/post_url_sanitisation_rule_spec.rb b/backend/spec/models/post_url_sanitisation_rule_spec.rb new file mode 100644 index 0000000..bf5f7df --- /dev/null +++ b/backend/spec/models/post_url_sanitisation_rule_spec.rb @@ -0,0 +1,265 @@ +require 'rails_helper' + +RSpec.describe PostUrlSanitisationRule, type: :model do + before do + described_class.unscoped.delete_all + end + + describe 'validations' do + it 'requires a source pattern' do + rule = described_class.new(priority: 10, source_pattern: nil, replacement: '') + + expect(rule).to be_invalid + expect(rule.errors.details.fetch(:source_pattern)).to eq([{ error: :blank }]) + end + + it 'requires a unique source pattern' do + described_class.create!(priority: 10, source_pattern: 'source', replacement: 'first') + rule = described_class.new( + priority: 20, + source_pattern: 'source', + replacement: 'second' + ) + + expect(rule).to be_invalid + expect(rule.errors.details.fetch(:source_pattern)).to include( + error: :taken, + value: 'source' + ) + end + + it 'rejects an invalid regexp' do + rule = described_class.new(priority: 10, source_pattern: '[', replacement: '') + + expect(rule).to be_invalid + expect(rule.errors[:source_pattern]).to include('変な正規表現だね〜（笑）') + end + end + + describe '.sanitise' do + it 'applies each active rule once in priority order' do + described_class.create!(priority: 30, source_pattern: 'c', replacement: 'd') + described_class.create!(priority: 10, source_pattern: 'a', replacement: 'aa') + described_class.create!(priority: 20, source_pattern: 'aa', replacement: 'c') + discarded = described_class.create!( + priority: 5, + source_pattern: '.', + replacement: 'discarded' + ) + discarded.discard! + + expect(described_class.sanitise('a')).to eq('d') + end + + it 'canonicalises the initial YouTube and Nico URL forms' do + create_initial_rules + + urls = { + 'https://youtu.be/abc123?si=share' => youtube_url('abc123'), + 'https://www.youtube.com/live/abc123?t=10' => youtube_url('abc123'), + 'https://youtube.com/shorts/abc123?feature=share' => youtube_url('abc123'), + 'https://m.youtube.com/embed/abc123' => youtube_url('abc123'), + 'https://youtube.com/watch?feature=share&v=abc123&t=10' => youtube_url('abc123'), + 'https://nico.ms/sm123?from=share#fragment' => nico_url('sm123'), + 'https://www.nicovideo.jp/watch/sm123?ref=share#fragment' => nico_url('sm123') + } + + urls.each do |url, canonical_url| + expect(described_class.sanitise(url)).to eq(canonical_url) + end + end + end + + describe '.apply!' do + it 'locks posts in ID order and updates through temporary URLs' do + first = create(:post, url: 'https://example.com/source/1') + second = create(:post, url: 'https://example.com/source/2') + create_source_rule + sql = capture_sql { described_class.apply! } + + lock_sql = sql.find { _1.match?(/SELECT .*posts.*FOR UPDATE/i) } + expect(lock_sql).to match(/ORDER BY .*posts.*id.* ASC/i) + expect(sql.grep(/post-url-sanitising\.invalid/).size).to eq(2) + expect(first.reload.url).to eq('https://example.com/canonical/1') + expect(second.reload.url).to eq('https://example.com/canonical/2') + end + + it 'loads and compiles rules only once' do + create(:post, url: 'https://example.com/source/1') + create(:post, url: 'https://example.com/source/2') + create_source_rule + + expect(described_class).to receive(:rules).once.and_call_original + + described_class.apply! + end + + it 'does not change post versions, version_no, or updated_at' do + post = create(:post, url: 'https://example.com/source/1') + post.update_columns(version_no: 7, updated_at: 1.day.ago) + original_updated_at = post.reload.updated_at + create_source_rule + + expect { described_class.apply! }.not_to change(PostVersion, :count) + + post.reload + expect(post.url).to eq('https://example.com/canonical/1') + expect(post.version_no).to eq(7) + expect(post.updated_at).to eq(original_updated_at) + end + + invalid_urls = { + 'a blank URL' => '', + 'an unparseable URL' => 'https://[', + 'a non-HTTP URL' => 'ftp://example.com/file', + 'an HTTP URL without a host' => 'https:/path' + } + + invalid_urls.each do |description, sanitised_url| + it "rolls back every update when sanitisation produces #{ description }" do + valid_post = create(:post, url: 'https://example.com/source/1') + invalid_post = create(:post, url: 'https://example.com/invalid') + create_source_rule + described_class.create!( + priority: 20, + source_pattern: '\\Ahttps://example\\.com/invalid\\z', + replacement: sanitised_url + ) + + expect { described_class.apply! } + .to raise_error(described_class::InvalidUrlError) { |error| + expect(error.invalid_rows).to eq([ + { post_id: invalid_post.id, + original_url: 'https://example.com/invalid', + sanitised_url: } + ]) + } + expect(valid_post.reload.url).to eq('https://example.com/source/1') + expect(invalid_post.reload.url).to eq('https://example.com/invalid') + end + end + + it 'rolls back every update when sanitisation produces a URL longer than 768 characters' do + path = 'a' * 375 + original_url = "https://example.com/#{ path }" + sanitised_url = "https://example.com/#{ path }#{ path }" + valid_post = create(:post, url: 'https://example.com/source/1') + invalid_post = create(:post, url: original_url) + create_source_rule + described_class.create!( + priority: 20, + source_pattern: '\\Ahttps://example\\.com/(a+)\\z', + replacement: 'https://example.com/\\1\\1' + ) + + expect { described_class.apply! } + .to raise_error(described_class::InvalidUrlError) { |error| + expect(error.invalid_rows).to eq([ + { post_id: invalid_post.id, + original_url:, + sanitised_url: } + ]) + } + expect(valid_post.reload.url).to eq('https://example.com/source/1') + expect(invalid_post.reload.url).to eq(original_url) + end + + it 'rejects sanitised URL collisions case-insensitively without changing posts' do + source = create(:post, url: 'https://example.com/source/Foo') + canonical = create(:post, url: 'https://example.com/canonical/foo') + create_source_rule + + expect { described_class.apply! } + .to raise_error(described_class::UrlConflictError) { |error| + expect(error.conflicts).to contain_exactly( + { url: 'https://example.com/canonical/Foo', + post_id: source.id, + original_url: 'https://example.com/source/Foo' }, + { url: 'https://example.com/canonical/foo', + post_id: canonical.id, + original_url: 'https://example.com/canonical/foo' } + ) + } + expect(source.reload.url).to eq('https://example.com/source/Foo') + expect(canonical.reload.url).to eq('https://example.com/canonical/foo') + expect(Post.count).to eq(2) + end + + it 'converts a persisted URL constraint race into UrlConflictError' do + post = create(:post, url: 'https://example.com/source/1') + create_source_rule + conflict = { url: 'https://example.com/canonical/1', + post_id: post.id, + original_url: post.url } + database_error = ActiveRecord::RecordNotUnique.new('duplicate URL') + allow_any_instance_of(ActiveRecord::Relation) + .to receive(:update_all).and_raise(database_error) + allow(described_class).to receive(:build_persisted_conflicts).and_return([conflict]) + + expect { described_class.apply! } + .to raise_error(described_class::UrlConflictError) { |error| + expect(error.conflicts).to eq([conflict]) + expect(error.cause).to equal(database_error) + } + expect(post.reload.url).to eq('https://example.com/source/1') + end + + it 're-raises an unidentified RecordNotUnique error' do + post = create(:post, url: 'https://example.com/source/1') + create_source_rule + database_error = ActiveRecord::RecordNotUnique.new('another unique constraint') + allow_any_instance_of(ActiveRecord::Relation) + .to receive(:update_all).and_raise(database_error) + allow(described_class).to receive(:build_persisted_conflicts).and_return([]) + + expect { described_class.apply! }.to raise_error(database_error) + expect(post.reload.url).to eq('https://example.com/source/1') + end + end + + def capture_sql + statements = [] + subscriber = lambda do |_name, _start, _finish, _id, payload| + binds = payload.fetch(:binds, []).map { _1.value_for_database.to_s } + statements << ([payload.fetch(:sql)] + binds).join(' ') + end + ActiveSupport::Notifications.subscribed(subscriber, 'sql.active_record') { yield } + statements + end + + def create_source_rule + described_class.create!( + priority: 10, + source_pattern: '\\Ahttps://example\\.com/source/(.+)\\z', + replacement: 'https://example.com/canonical/\\1' + ) + end + + def create_initial_rules + rules = [ + ['\\Ahttps?://youtu\\.be/([^/?#]+)(?:[?#].*)?\\z', youtube_url('\\1')], + ['\\Ahttps?://(?:www\\.|m\\.)?youtube\\.com/live/([^/?#]+)(?:[?#].*)?\\z', + youtube_url('\\1')], + ['\\Ahttps?://(?:www\\.|m\\.)?youtube\\.com/shorts/([^/?#]+)(?:[?#].*)?\\z', + youtube_url('\\1')], + ['\\Ahttps?://(?:www\\.|m\\.)?youtube\\.com/embed/([^/?#]+)(?:[?#].*)?\\z', + youtube_url('\\1')], + ['\\Ahttps?://(?:www\\.|m\\.)?youtube\\.com/watch\\?(?:[^#&]+&)*' \ + 'v=([^&#]+)(?:[&#].*)?\\z', youtube_url('\\1')], + ['\\Ahttps?://nico\\.ms/([^/?#]+)(?:[?#].*)?\\z', nico_url('\\1')], + ['\\Ahttps?://(?:www\\.)?nicovideo\\.jp/watch/([^?#/]+)(?:[?#].*)?\\z', + nico_url('\\1')] + ] + rules.each_with_index do |(source_pattern, replacement), index| + described_class.create!( + priority: (index + 1) * 10, + source_pattern:, + replacement: + ) + end + end + + def youtube_url(video_id) = "https://www.youtube.com/watch?v=#{ video_id }" + + def nico_url(video_id) = "https://www.nicovideo.jp/watch/#{ video_id }" +end diff --git a/backend/spec/tasks/nico_export_spec.rb b/backend/spec/tasks/nico_export_spec.rb index e6ed87c..0221f99 100644 --- a/backend/spec/tasks/nico_export_spec.rb +++ b/backend/spec/tasks/nico_export_spec.rb @@ -62,7 +62,7 @@ RSpec.describe 'nico:export' do it 'deduplicates video ids' do create_post('https://www.nicovideo.jp/watch/sm12345') - create_post('https://www.nicovideo.jp/watch/sm12345?from=1') + create_post('https://sp.nicovideo.jp/watch/sm12345') expect(Open3).to receive(:capture3) do |_env, *args, **_kwargs| expect(args.drop(3)).to eq(['sm12345'])