diff --git a/backend/app/controllers/posts_controller.rb b/backend/app/controllers/posts_controller.rb index 48cf816..26e358e 100644 --- a/backend/app/controllers/posts_controller.rb +++ b/backend/app/controllers/posts_controller.rb @@ -140,10 +140,11 @@ class PostsController < ApplicationController original_created_from = params[:original_created_from] original_created_before = params[:original_created_before] parent_post_ids = parse_parent_post_ids + resized_thumbnail = thumbnail.present? ? Post.resized_thumbnail_attachment(thumbnail) : nil post = Post.new(title:, url:, thumbnail_base: nil, uploaded_user: current_user, original_created_from:, original_created_before:) - post.thumbnail.attach(thumbnail) if thumbnail.present? + post.thumbnail.attach(resized_thumbnail) if resized_thumbnail ApplicationRecord.transaction do post.save! @@ -156,8 +157,6 @@ class PostsController < ApplicationController sync_parent_posts!(post, parent_post_ids) - post.resized_thumbnail! - PostVersionRecorder.record!(post:, event_type: :create, created_by_user: current_user) end @@ -167,6 +166,8 @@ class PostsController < ApplicationController render_validation_error fields: { tags: 'ニコニコ・タグは直接指定できません.' } rescue Tag::DeprecatedTagNormalisationError render_unprocessable_entity '廃止済みタグは付与できません.', field: :tags + rescue MiniMagick::Error + render_validation_error fields: { thumbnail: ['サムネイル画像の変換に失敗しました.'] } rescue ArgumentError => e render_validation_error fields: { parent_post_ids: [e.message] } rescue ActiveRecord::RecordInvalid => e diff --git a/backend/app/controllers/tags_controller.rb b/backend/app/controllers/tags_controller.rb index f345a02..cb07ddd 100644 --- a/backend/app/controllers/tags_controller.rb +++ b/backend/app/controllers/tags_controller.rb @@ -245,10 +245,7 @@ class TagsController < ApplicationController return render_unprocessable_entity('カテゴリは必須です.', field: :category) if category.blank? return render_unprocessable_entity '廃止状態は必須です.', field: :deprecated unless params.key?(:deprecated) - if (name != tag.name && - tag.in?([Tag.tagme, Tag.bot, Tag.no_deerjikist, Tag.video, Tag.niconico])) - return render_unprocessable_entity 'システム・タグの名称は変更できません.', field: :name - end + return unless validate_tag_rename(tag, name) alias_names = params[:aliases].to_s.split.uniq parent_names = params[:parent_tags].to_s.split.uniq @@ -274,7 +271,7 @@ class TagsController < ApplicationController else tag.update!(category:, deprecated_at: deprecated ? Time.current : nil) end - tag.tag_name.update!(name:) + rename_tag_name!(tag, name) if name_changed alias_names << old_name if name_changed alias_names.delete(name) @@ -310,6 +307,8 @@ class TagsController < ApplicationController return render_unprocessable_entity 'ニコタグは廃止できません.', field: :deprecated end + return unless validate_tag_rename(tag, name) + if tag.nico? || (category.present? && category == 'nico') return render_unprocessable_entity 'ニコタグは変更できません.', field: :category end @@ -321,7 +320,7 @@ class TagsController < ApplicationController name_changed = name.present? && name != old_name wiki_page = tag.tag_name.wiki_page if name_changed - tag.tag_name.update!(name:) if name.present? + rename_tag_name!(tag, name) if name_changed tag.update!(category:) if category.present? if deprecated_given && tag.deprecated? != deprecated tag.update!(deprecated_at: deprecated ? Time.current : nil) @@ -535,6 +534,67 @@ class TagsController < ApplicationController created_by_user:) end + def validate_tag_rename tag, name + return true if name.blank? || name == tag.name + + if tag.in?([Tag.tagme, Tag.bot, Tag.no_deerjikist, Tag.video, Tag.niconico]) + render_unprocessable_entity 'システム・タグの名称は変更できません.', field: :name + return false + end + + target_tag_name = TagName.with_discarded.find_by(name:) + return true if target_tag_name.nil? + return true if target_tag_name.canonical_id? + + render_unprocessable_entity 'その名前は既に使はれてゐます.', field: :name + false + end + + def rename_tag_name! tag, name + return if name == tag.name + + current_tag_name = tag.tag_name + target_tag_name = TagName.with_discarded.find_by(name:) + + if target_tag_name.nil? + current_tag_name.update!(name:) + return + end + + promote_tag_alias!( + tag, + current_tag_name:, + promoted_tag_name: target_tag_name) + end + + def promote_tag_alias! tag, current_tag_name:, promoted_tag_name: + old_owner_tag = promoted_tag_name.canonical&.tag + + if old_owner_tag && old_owner_tag != tag + TagVersioning.ensure_snapshot!(old_owner_tag, created_by_user: current_user) + end + + promoted_tag_name.undiscard! if promoted_tag_name.discarded? + promoted_tag_name.update!(canonical: nil) + + TagName.with_discarded + .where(canonical_id: current_tag_name.id) + .where.not(id: promoted_tag_name.id) + .find_each do |alias_tag_name| + alias_tag_name.update!(canonical: promoted_tag_name) + end + + current_tag_name.wiki_page&.update!(tag_name: promoted_tag_name) + tag.update!(tag_name: promoted_tag_name) + current_tag_name.association(:wiki_page).reset + current_tag_name.association(:tag).reset + current_tag_name.reload.update!(canonical: promoted_tag_name) + + return unless old_owner_tag && old_owner_tag != tag + + record_tag_version!(old_owner_tag.reload, event_type: :update, created_by_user: current_user) + end + def update_aliases! tag, alias_names alias_names = alias_names.uniq diff --git a/backend/app/models/post.rb b/backend/app/models/post.rb index d354314..b2835b0 100644 --- a/backend/app/models/post.rb +++ b/backend/app/models/post.rb @@ -1,5 +1,19 @@ class Post < ApplicationRecord require 'mini_magick' + require 'stringio' + + def self.resized_thumbnail_attachment(upload) + upload.rewind + image = MiniMagick::Image.read(upload.read) + image.resize '180x180' + image.format 'jpg' + + { io: StringIO.new(image.to_blob), + filename: 'resized_thumbnail.jpg', + content_type: 'image/jpeg' } + ensure + upload.rewind + end belongs_to :uploaded_user, class_name: 'User', optional: true @@ -87,12 +101,7 @@ class Post < ApplicationRecord def resized_thumbnail! return unless thumbnail.attached? - image = MiniMagick::Image.read(thumbnail.download) - image.resize '180x180' - thumbnail.purge - thumbnail.attach(io: File.open(image.path), - filename: 'resized_thumbnail.jpg', - content_type: 'image/jpeg') + thumbnail.attach(self.class.resized_thumbnail_attachment(StringIO.new(thumbnail.download))) end private diff --git a/backend/app/models/tag_implication.rb b/backend/app/models/tag_implication.rb index a629764..888713c 100644 --- a/backend/app/models/tag_implication.rb +++ b/backend/app/models/tag_implication.rb @@ -6,6 +6,7 @@ class TagImplication < ApplicationRecord validates :parent_tag_id, presence: true validate :parent_tag_mustnt_be_itself + validate :parent_tag_mustnt_create_cycle private @@ -14,4 +15,27 @@ class TagImplication < ApplicationRecord errors.add :parent_tag_id, '親タグは子タグと同一であってはなりません.' end end + + def parent_tag_mustnt_create_cycle + return if tag_id.blank? || parent_tag_id.blank? + return if errors[:parent_tag_id].present? + + seen = { } + stack = [parent_tag_id] + + until stack.empty? + current_id = stack.pop + next if seen[current_id] + + seen[current_id] = true + + if current_id == tag_id + errors.add :parent_tag_id, '親タグに子孫タグを指定すると循環します.' + errors.add :base, 'タグの親子関係が循環します.' + return + end + + stack.concat(TagImplication.where(tag_id: current_id).pluck(:parent_tag_id)) + end + end end diff --git a/backend/spec/models/tag_implication_spec.rb b/backend/spec/models/tag_implication_spec.rb new file mode 100644 index 0000000..a29cc1c --- /dev/null +++ b/backend/spec/models/tag_implication_spec.rb @@ -0,0 +1,36 @@ +require 'rails_helper' + +RSpec.describe TagImplication, type: :model do + it 'rejects a parent tag that would create a cycle' do + child = create(:tag, name: 'tag_implication_cycle_child') + parent = create(:tag, name: 'tag_implication_cycle_parent') + + described_class.create!(tag: child, parent_tag: parent) + + implication = described_class.new(tag: parent, parent_tag: child) + + expect(implication).not_to be_valid + expect(implication.errors[:parent_tag_id]).to include( + '親タグに子孫タグを指定すると循環します.' + ) + expect(implication.errors[:base]).to be_present + end + + it 'terminates even when existing data already contains a cycle' do + child = create(:tag, name: 'tag_implication_existing_cycle_child') + parent = create(:tag, name: 'tag_implication_existing_cycle_parent') + ancestor = create(:tag, name: 'tag_implication_existing_cycle_ancestor') + + described_class.create!(tag: parent, parent_tag: ancestor) + described_class.insert_all!( + [ + { tag_id: ancestor.id, parent_tag_id: parent.id, + created_at: Time.current, updated_at: Time.current } + ] + ) + + implication = described_class.new(tag: child, parent_tag: parent) + + expect(implication).to be_valid + end +end diff --git a/backend/spec/models/tag_spec.rb b/backend/spec/models/tag_spec.rb index 014cdde..1b2549c 100644 --- a/backend/spec/models/tag_spec.rb +++ b/backend/spec/models/tag_spec.rb @@ -54,7 +54,17 @@ RSpec.describe Tag, type: :model do first = create(:tag, name: 'expand_cycle_first') second = create(:tag, name: 'expand_cycle_second') TagImplication.create!(tag: first, parent_tag: second) - TagImplication.create!(tag: second, parent_tag: first) + now = Time.current + TagImplication.insert_all!( + [ + { + tag_id: second.id, + parent_tag_id: first.id, + created_at: now, + updated_at: now + } + ] + ) expect(described_class.expand_parent_tags([first])).to contain_exactly(first, second) end diff --git a/backend/spec/requests/posts_spec.rb b/backend/spec/requests/posts_spec.rb index 18d5b73..b640f1a 100644 --- a/backend/spec/requests/posts_spec.rb +++ b/backend/spec/requests/posts_spec.rb @@ -1,4 +1,5 @@ require 'rails_helper' +require 'base64' require 'set' include ActiveSupport::Testing::TimeHelpers @@ -8,6 +9,11 @@ RSpec.describe 'Posts API', type: :request do # resized_thumbnail! が MiniMagick 依存でコケやすいので request spec ではスタブしとくのが無難。 before do allow_any_instance_of(Post).to receive(:resized_thumbnail!).and_return(true) + allow(Post).to receive(:resized_thumbnail_attachment).and_return( + io: StringIO.new('dummy'), + filename: 'resized_thumbnail.jpg', + content_type: 'image/jpeg' + ) end def create_nico_tag!(name) @@ -19,6 +25,18 @@ RSpec.describe 'Posts API', type: :request do Rack::Test::UploadedFile.new(StringIO.new('dummy'), 'image/jpeg', original_filename: 'dummy.jpg') end + def real_thumbnail_upload + gif = + Base64.decode64( + 'R0lGODdhAQABAIABAP///wAAACwAAAAAAQABAAACAkQBADs=') + + Rack::Test::UploadedFile.new( + StringIO.new(gif), + 'image/gif', + original_filename: 'thumbnail.gif' + ) + end + def post_write_params params = { } { parent_post_ids: '' }.merge(params) end @@ -696,6 +714,66 @@ RSpec.describe 'Posts API', type: :request do expect(json['tags'][0]).to have_key('name') end + it '201 when posting manually with a thumbnail' do + sign_in_as(member) + allow(Post).to receive(:resized_thumbnail_attachment).and_call_original + + post '/posts', params: post_write_params( + title: 'thumbnail post', + url: 'https://example.com/thumbnail-post', + tags: 'spec_tag', + thumbnail: real_thumbnail_upload + ) + + expect(response).to have_http_status(:created) + post_record = Post.find(json.fetch('id')) + expect(post_record.thumbnail).to be_attached + expect(post_record.thumbnail.blob.content_type).to eq('image/jpeg') + expect { post_record.thumbnail.download }.not_to raise_error + end + + it 'resizes the thumbnail before the create transaction begins' do + sign_in_as(member) + + open_transactions = [] + baseline_open_transactions = Post.connection.open_transactions + allow(Post).to receive(:resized_thumbnail_attachment) do |_upload| + open_transactions << Post.connection.open_transactions + { io: StringIO.new('dummy'), + filename: 'resized_thumbnail.jpg', + content_type: 'image/jpeg' } + end + + post '/posts', params: post_write_params( + title: 'transaction post', + url: 'https://example.com/transaction-post', + tags: 'spec_tag', + thumbnail: dummy_upload + ) + + expect(response).to have_http_status(:created) + expect(open_transactions).to eq([baseline_open_transactions]) + end + + it 'returns 422 and does not create a post when thumbnail resize fails' do + sign_in_as(member) + allow(Post).to receive(:resized_thumbnail_attachment).and_raise(MiniMagick::Error) + + expect { + post '/posts', params: post_write_params( + title: 'broken thumbnail post', + url: 'https://example.com/broken-thumbnail-post', + tags: 'spec_tag', + thumbnail: dummy_upload + ) + }.not_to change(Post, :count) + + expect(response).to have_http_status(:unprocessable_entity) + expect(json.fetch('errors')).to include( + 'thumbnail' => ['サムネイル画像の変換に失敗しました.'] + ) + end + it '201 and creates post + tags when member and tags have aliases' do sign_in_as(member) diff --git a/backend/spec/requests/tag_children_spec.rb b/backend/spec/requests/tag_children_spec.rb index 8e1b91b..69bb10a 100644 --- a/backend/spec/requests/tag_children_spec.rb +++ b/backend/spec/requests/tag_children_spec.rb @@ -56,6 +56,17 @@ RSpec.describe "TagChildren", type: :request do expect(response).to have_http_status(:no_content) end + + it 'returns 422 and does not create relation when the new link makes a cycle' do + TagImplication.create!(tag: parent, parent_tag: child) + + expect { + do_request + }.not_to change(TagImplication, :count) + + expect(response).to have_http_status(:unprocessable_entity) + expect(TagImplication.where(tag: child, parent_tag: parent)).not_to exist + end end context "when Tag.find raises (invalid ids)" do diff --git a/backend/spec/requests/tags_spec.rb b/backend/spec/requests/tags_spec.rb index 927b4a7..0055364 100644 --- a/backend/spec/requests/tags_spec.rb +++ b/backend/spec/requests/tags_spec.rb @@ -581,6 +581,58 @@ RSpec.describe 'Tags API', type: :request do expect(response).to have_http_status(:ok) expect(wiki_page.reload.wiki_versions.count).to eq(before_wiki_version_count) end + + it 'full update で旧名 alias が残った tag を PATCH で旧名へ戻せる' do + put "/tags/#{ tag.id }", params: { + name: 'patch_roundtrip_target', + category: 'general', + aliases: 'unko', + parent_tags: '', + deprecated: '0', + } + + expect(response).to have_http_status(:ok) + expect(TagName.find_by!(name: 'spec_tag').canonical).to eq(tag.reload.tag_name) + + patch "/tags/#{ tag.id }", params: { name: 'spec_tag' } + + expect(response).to have_http_status(:ok) + + tag.reload + + expect(tag.name).to eq('spec_tag') + expect(tag.tag_name.canonical_id).to be_nil + expect(TagName.find_by!(name: 'patch_roundtrip_target').canonical).to eq(tag.tag_name) + end + + it '別 tag の正規名には変更できない' do + wiki_page = + Wiki::Commit.create_content!( + tag_name: tag.tag_name, + body: 'patch collision wiki', + created_by_user: member_user, + message: 'init') + + patch "/tags/#{ tag.id }", params: { name: 'unknown' } + + expect(response).to have_http_status(:unprocessable_entity) + expect(json.fetch('errors')).to include( + 'name' => ['その名前は既に使はれてゐます.'] + ) + + expect(tag.reload.name).to eq('spec_tag') + expect(tag.tag_name.aliases.map(&:name)).to contain_exactly('unko') + expect(wiki_page.reload.tag_name).to eq(tag.tag_name) + end + + it 'system tag の name は変更できない' do + system_tag = Tag.bot + + patch "/tags/#{ system_tag.id }", params: { name: 'patch_system_tag_renamed' } + + expect(response).to have_http_status(:unprocessable_entity) + expect(system_tag.reload.name).to eq('bot操作') + end end end @@ -745,7 +797,17 @@ RSpec.describe 'Tags API', type: :request do ) TagImplication.create!(tag: first, parent_tag: root_material) TagImplication.create!(tag: second, parent_tag: first) - TagImplication.create!(tag: first, parent_tag: second) + now = Time.current + TagImplication.insert_all!( + [ + { + tag_id: first.id, + parent_tag_id: second.id, + created_at: now, + updated_at: now + } + ] + ) get '/tags/with-depth', params: { parent: root_material.id } @@ -1029,6 +1091,42 @@ RSpec.describe 'Tags API', type: :request do expect(versions.second.created_by_user_id).to eq(member_user.id) end + it '同じ tag の旧 alias へ戻しても rename できる' do + put "/tags/#{ tag.id }", params: { + name: 'put_roundtrip_b', + category: 'general', + aliases: 'unko', + parent_tags: '', + deprecated: '0', + } + + expect(response).to have_http_status(:ok) + + put "/tags/#{ tag.id }", params: { + name: 'spec_tag', + category: 'general', + aliases: 'unko', + parent_tags: '', + deprecated: '0', + } + + expect(response).to have_http_status(:ok) + + tag.reload + + expect(tag.name).to eq('spec_tag') + expect(TagName.find_by!(name: 'put_roundtrip_b').canonical).to eq(tag.tag_name) + expect(tag.tag_name.aliases.map(&:name)).to contain_exactly('put_roundtrip_b', 'unko') + expect(tag.tag_name.aliases.map(&:name)).not_to include('spec_tag') + expect(alias_tn.reload.canonical).to eq(tag.tag_name) + + version = tag.tag_versions.order(:version_no).last + + expect(version.event_type).to eq('update') + expect(version.name).to eq('spec_tag') + expect(version.aliases.split).to contain_exactly('put_roundtrip_b', 'unko') + end + it 'parent tag の snapshot も作成する' do old_parent = Tag.create!( tag_name: TagName.create!(name: 'put_snapshot_old_parent'), @@ -1153,6 +1251,48 @@ RSpec.describe 'Tags API', type: :request do ) end + it 'wiki を持つ tag を旧 alias へ戻しても wiki を新 canonical へ移す' do + wiki_page = + Wiki::Commit.create_content!( + tag_name: tag.tag_name, + body: 'wiki body before', + created_by_user: member_user, + message: 'init') + + expect { + put "/tags/#{ tag.id }", params: { + name: 'put_wiki_roundtrip_b', + category: 'general', + aliases: 'unko', + parent_tags: '', + deprecated: '0', + } + + put "/tags/#{ tag.id }", params: { + name: 'spec_tag', + category: 'general', + aliases: 'unko', + parent_tags: '', + deprecated: '0', + } + } + .to change(TagVersion, :count).by(3) + .and change(WikiVersion, :count).by(2) + + expect(response).to have_http_status(:ok) + + tag.reload + + expect(wiki_page.reload.tag_name).to eq(tag.tag_name) + expect(TagName.find_by!(name: 'put_wiki_roundtrip_b').wiki_page).to be_nil + expect(TagName.find_by!(name: 'put_wiki_roundtrip_b').canonical).to eq(tag.tag_name) + + versions = wiki_page.wiki_versions.order(:version_no).last(2) + + expect(versions.map(&:event_type)).to eq(['update', 'update']) + expect(versions.map(&:title)).to eq(['put_wiki_roundtrip_b', 'spec_tag']) + end + it '別名を他 tag から奪った場合、奪はれた側の tag version も作成する' do old_owner = Tag.create!( tag_name: TagName.create!(name: 'put_alias_old_owner'), @@ -1191,9 +1331,49 @@ RSpec.describe 'Tags API', type: :request do expect(old_owner_versions.second.aliases.split).not_to include('put_stolen_alias') end - it 'parent_tags に指定すると循環する tag は 422 にする' do - pending '#332 で対応予定' + it '別 tag の alias 名を rename で奪へる' do + old_owner = Tag.create!( + tag_name: TagName.create!(name: 'put_alias_collision_owner'), + category: :general + ) + stolen_alias = TagName.create!( + name: 'put_alias_collision_name', + canonical: old_owner.tag_name + ) + wiki_page = + Wiki::Commit.create_content!( + tag_name: tag.tag_name, + body: 'put collision wiki', + created_by_user: member_user, + message: 'init') + put "/tags/#{ tag.id }", params: { + name: 'put_alias_collision_name', + category: 'general', + aliases: 'unko', + parent_tags: '', + deprecated: '0', + } + + expect(response).to have_http_status(:ok) + + tag.reload + old_owner.reload + stolen_alias.reload + + expect(tag.name).to eq('put_alias_collision_name') + expect(stolen_alias.canonical_id).to be_nil + expect(TagName.find_by!(name: 'spec_tag').canonical).to eq(tag.tag_name) + expect(old_owner.tag_name.aliases.map(&:name)).not_to include('put_alias_collision_name') + + old_owner_versions = old_owner.tag_versions.order(:version_no) + + expect(old_owner_versions.last.event_type).to eq('update') + expect(old_owner_versions.last.aliases.split).not_to include('put_alias_collision_name') + expect(wiki_page.reload.tag_name).to eq(tag.tag_name) + end + + it 'parent_tags に指定すると循環する tag は 422 にする' do child = Tag.create!( tag_name: TagName.create!(name: 'put_cycle_child'), category: :general