フォームのバリデーションとニコ連携の画面変更 (#090) (#355)

Reviewed-on: #355
Co-authored-by: miteruzo <miteruzo@naver.com>
Co-committed-by: miteruzo <miteruzo@naver.com>

backend/spec/requests/error_responses_spec.rb

@@ -0,0 +1,47 @@
+require 'rails_helper'
+
+RSpec.describe 'error responses', type: :request do
+  describe 'manual input errors' do
+    it 'returns a stable payload for bad requests' do
+      get '/tags/name/%20/deerjikists'
+
+      expect(response).to have_http_status(:bad_request)
+      expect(json).to include(
+          'type' => 'bad_request',
+          'message' => be_present,
+          'errors' => {},
+          'base_errors' => [be_present])
+    end
+
+    it 'returns a stable field-error payload for unprocessable requests' do
+      member = create(:user, :member)
+      tag = create(:tag, :general, name: 'error_response_tag')
+      sign_in_as(member)
+
+      patch "/tags/#{ tag.id }", params: { category: 'nico' }
+
+      expect(response).to have_http_status(:unprocessable_entity)
+      expect(json).to include(
+          'type' => 'validation_error',
+          'message' => '入力内容を確認してください．',
+          'base_errors' => [])
+      expect(json.fetch('errors')).to include(
+          'category' => ['ニコタグは変更できません．'])
+    end
+  end
+
+  describe 'model validation errors' do
+    it 'returns field messages for model errors' do
+      user = create(:user)
+      sign_in_as(user)
+
+      put "/users/#{ user.id }", params: { name: 'a' * 256 }
+
+      expect(response).to have_http_status(:unprocessable_entity)
+      expect(json).to include(
+          'type' => 'validation_error',
+          'message' => '入力内容を確認してください．')
+      expect(json.fetch('errors').fetch('name')).to include(be_present)
+    end
+  end
+end

backend/spec/requests/materials_spec.rb

@@ -141,16 +141,21 @@ RSpec.describe 'Materials API', type: :request do
     context 'when logged in' do
       before { sign_in_as(guest_user) }
 
-      it 'returns 400 when tag is blank' do
+      it 'returns 422 when tag is blank' do
         post '/materials', params: { tag: '   ', file: dummy_upload }
 
-        expect(response).to have_http_status(:bad_request)
+        expect(response).to have_http_status(:unprocessable_entity)
+        expect(json.fetch('errors')).to include(
+            'tag' => ['タグは必須です．'])
       end
 
-      it 'returns 400 when both file and url are blank' do
+      it 'returns 422 when both file and url are blank' do
         post '/materials', params: { tag: 'material_create_blank' }
 
-        expect(response).to have_http_status(:bad_request)
+        expect(response).to have_http_status(:unprocessable_entity)
+        expect(json.fetch('errors')).to include(
+            'file' => ['ファイルまたは URL は必須です．'],
+            'url' => ['ファイルまたは URL は必須です．'])
       end
 
       it 'creates a material with an attached file' do
@@ -261,21 +266,26 @@ RSpec.describe 'Materials API', type: :request do
         expect(response).to have_http_status(:not_found)
       end
 
-      it 'returns 400 when tag is blank' do
+      it 'returns 422 when tag is blank' do
         put "/materials/#{ material.id }", params: {
           tag: '   ',
           file: dummy_upload
         }
 
-        expect(response).to have_http_status(:bad_request)
+        expect(response).to have_http_status(:unprocessable_entity)
+        expect(json.fetch('errors')).to include(
+            'tag' => ['タグは必須です．'])
       end
 
-      it 'returns 400 when both file and url are blank' do
+      it 'returns 422 when both file and url are blank' do
         put "/materials/#{ material.id }", params: {
           tag: 'material_update_no_payload'
         }
 
-        expect(response).to have_http_status(:bad_request)
+        expect(response).to have_http_status(:unprocessable_entity)
+        expect(json.fetch('errors')).to include(
+            'file' => ['ファイルまたは URL は必須です．'],
+            'url' => ['ファイルまたは URL は必須です．'])
       end
 
       it 'updates tag, url, file, and updated_by_user' do

backend/spec/requests/nico_tags_spec.rb

@@ -3,12 +3,68 @@ require 'rails_helper'
 
 RSpec.describe 'NicoTags', type: :request do
   describe 'GET /tags/nico' do
-    it 'returns tags and next_cursor when overflowing limit' do
-      create_list(:tag, 21, :nico)
-      get '/tags/nico', params: { limit: 20 }
+    it 'returns paginated tags and total count' do
+      create_list(:tag, 3, :nico)
+
+      get '/tags/nico', params: { page: 2, limit: 2 }
+
       expect(response).to have_http_status(:ok)
-      expect(json['tags'].size).to eq(20)
-      expect(json['next_cursor']).to be_present
+      expect(json['tags'].size).to eq(1)
+      expect(json['count']).to eq(3)
+    end
+
+    it 'filters by nico tag name, linked tag name, and link status' do
+      linked = create(:tag, :nico)
+      linked.tag_name.update!(name: 'nico:search_linked')
+      unlinked = create(:tag, :nico)
+      unlinked.tag_name.update!(name: 'nico:search_unlinked')
+      other = create(:tag, :nico)
+      other.tag_name.update!(name: 'nico:other')
+      destination = create(:tag, :general)
+      destination.tag_name.update!(name: 'destination_search')
+      NicoTagRelation.create!(nico_tag: linked, tag: destination)
+      NicoTagRelation.create!(nico_tag: other, tag: create(:tag, :general))
+
+      get '/tags/nico', params: {
+        name: 'search_',
+        linked_tag: 'destination_',
+        link_status: 'linked'
+      }
+
+      expect(response).to have_http_status(:ok)
+      expect(json['count']).to eq(1)
+      expect(json.fetch('tags').map { |tag| tag['id'] }).to eq([linked.id])
+
+      get '/tags/nico', params: { name: 'search_', link_status: 'unlinked' }
+
+      expect(json['count']).to eq(1)
+      expect(json.fetch('tags').map { |tag| tag['id'] }).to eq([unlinked.id])
+    end
+
+    it 'sorts by name and timestamps' do
+      older = create(:tag, :nico)
+      older.tag_name.update!(name: 'nico:a')
+      older.update_columns(created_at: 2.days.ago)
+      newer = create(:tag, :nico)
+      newer.tag_name.update!(name: 'nico:b')
+      newer.update_columns(created_at: 1.day.ago)
+      older_post_tag =
+        PostTag.create!(post: Post.create!(url: 'https://example.com/nico-older'), tag: older)
+      older_post_tag.update_columns(created_at: 1.hour.ago)
+      newer_post_tag =
+        PostTag.create!(post: Post.create!(url: 'https://example.com/nico-newer'), tag: newer)
+      newer_post_tag.update_columns(created_at: 2.hours.ago)
+
+      get '/tags/nico', params: { order: 'name:desc' }
+      expect(json.fetch('tags').map { |tag| tag['id'] }).to eq([newer.id, older.id])
+
+      get '/tags/nico', params: { order: 'created_at:asc' }
+      expect(json.fetch('tags').map { |tag| tag['id'] }).to eq([older.id, newer.id])
+
+      get '/tags/nico', params: { order: 'updated_at:desc' }
+      expect(json.fetch('tags').map { |tag| tag['id'] }).to eq([older.id, newer.id])
+      expect(Time.zone.parse(json.fetch('tags').first.fetch('recent_post_tag_created_at')))
+        .to be_within(1.second).of(older_post_tag.created_at)
     end
   end
 
@@ -75,7 +131,7 @@ RSpec.describe 'NicoTags', type: :request do
       expect(versions.last.created_by_user_id).to eq(admin.id)
     end
 
-    it '400 when linked tag normalises to nico tag' do
+    it 'returns 422 when linked tag normalises to nico tag' do
       sign_in_as(member)
 
       other_nico = create(:tag, :nico, name: 'nico:linked_ng')
@@ -87,7 +143,37 @@ RSpec.describe 'NicoTags', type: :request do
         patch "/tags/nico/#{nico_tag.id}", params: { tags: 'linked_ng_alias' }
       }.not_to change(NicoTagVersion, :count)
 
-      expect(response).to have_http_status(:bad_request)
+      expect(response).to have_http_status(:unprocessable_entity)
+      expect(json.fetch('errors')).to include(
+          'tags' => ['ニコニコ・タグ同士は連携できません．'])
+    end
+
+    it 'returns the tags field error when a nico tag is specified directly' do
+      sign_in_as(member)
+
+      patch "/tags/nico/#{nico_tag.id}", params: { tags: 'nico:linked_ng' }
+
+      expect(response).to have_http_status(:unprocessable_entity)
+      expect(json.fetch('errors')).to include(
+          'tags' => ['ニコニコ・タグ同士は連携できません．'])
+    end
+
+    it 'returns tag name validation errors on the tags field and rolls back created tags' do
+      sign_in_as(member)
+      TagNameSanitisationRule.create!(
+        priority: 1,
+        source_pattern: 'invalid',
+        replacement: 'valid'
+      )
+      nico_tag
+
+      expect {
+        patch "/tags/nico/#{nico_tag.id}", params: { tags: 'created_first invalid' }
+      }.not_to change(TagName, :count)
+
+      expect(response).to have_http_status(:unprocessable_entity)
+      expect(json.fetch('errors').fetch('tags')).to include(
+          a_string_including('タグ名 “invalid”:', '名前に使用できない文字が含まれてゐます．'))
     end
   end
 end

backend/spec/requests/posts_spec.rb

@@ -57,6 +57,23 @@ RSpec.describe 'Posts API', type: :request do
     post_write_params({ base_version_no: base_version.version_no }.merge(params))
   end
 
+  def count_sql_queries
+    count = 0
+
+    callback = lambda do |_name, _started, _finished, _id, payload|
+      next if payload[:cached]
+      next if ['SCHEMA', 'TRANSACTION'].include?(payload[:name])
+
+      count += 1
+    end
+
+    ActiveSupport::Notifications.subscribed(callback, 'sql.active_record') do
+      yield
+    end
+
+    count
+  end
+
   let!(:tag_name) { TagName.create!(name: 'spec_tag') }
   let!(:tag)      { Tag.create!(tag_name: tag_name, category: :general) }
 
@@ -558,6 +575,59 @@ RSpec.describe 'Posts API', type: :request do
           expect(sibling_ids).to include(sibling_post.id)
         end
       end
+
+      it 'does not issue a query per tag or related post' do
+        user = create_member_user!
+
+        tags =
+            15.times.map do |i|
+              tag_name = TagName.create!(name: "show_query_tag_#{ i }")
+              tag = Tag.create!(tag_name:, category: :general)
+              TagName.create!(name: "show_query_alias_#{ i }", canonical: tag_name)
+              PostTag.create!(post: post_record, tag:)
+              tag
+            end
+
+        tags.each_cons(2) do |parent_tag, child_tag|
+          TagImplication.create!(parent_tag:, tag: child_tag)
+        end
+
+        parent_post = Post.create!(
+          title: 'query parent post',
+          url: 'https://example.com/query-parent-post'
+        )
+        sibling_post = Post.create!(
+          title: 'query sibling post',
+          url: 'https://example.com/query-sibling-post'
+        )
+        child_post = Post.create!(
+          title: 'query child post',
+          url: 'https://example.com/query-child-post'
+        )
+
+        PostImplication.create!(post: post_record, parent_post:)
+        PostImplication.create!(post: sibling_post, parent_post:)
+        PostImplication.create!(post: child_post, parent_post: post_record)
+
+        20.times do |i|
+          related_post = Post.create!(
+            title: "query related post #{ i }",
+            url: "https://example.com/query-related-post-#{ i }"
+          )
+          PostSimilarity.create!(post: post_record,
+                                 target_post: related_post,
+                                 cos: 1.0 - (i / 100.0))
+        end
+
+        query_count =
+            count_sql_queries do
+              get "/posts/#{ post_record.id }",
+                  headers: { 'X-Transfer-Code' => user.inheritance_code }
+            end
+
+        expect(response).to have_http_status(:ok)
+        expect(query_count).to be <= 45
+      end
     end
 
     context 'when post does not exist' do
@@ -634,7 +704,7 @@ RSpec.describe 'Posts API', type: :request do
             category: :nico)
       end
 
-      it 'return 400' do
+      it 'returns 422 with tag field errors' do
         sign_in_as(member)
 
         post '/posts', params: post_write_params(
@@ -644,7 +714,13 @@ RSpec.describe 'Posts API', type: :request do
           thumbnail: dummy_upload
         )
 
-        expect(response).to have_http_status(:bad_request), response.body
+        expect(response).to have_http_status(:unprocessable_entity), response.body
+        expect(json).to include(
+            'type' => 'validation_error',
+            'message' => '入力内容を確認してください．',
+            'base_errors' => [])
+        expect(json.fetch('errors')).to include(
+            'tags' => ['ニコニコ・タグは直接指定できません．'])
       end
     end
 
@@ -861,7 +937,7 @@ RSpec.describe 'Posts API', type: :request do
             category: :nico)
       end
 
-      it 'return 400' do
+      it 'returns 422 with tag field errors' do
         sign_in_as(member)
 
         put "/posts/#{post_record.id}", params: post_update_params(
@@ -869,7 +945,13 @@ RSpec.describe 'Posts API', type: :request do
             title: 'updated title',
             tags: 'nico:nico_tag')
 
-        expect(response).to have_http_status(:bad_request), response.body
+        expect(response).to have_http_status(:unprocessable_entity), response.body
+        expect(json).to include(
+            'type' => 'validation_error',
+            'message' => '入力内容を確認してください．',
+            'base_errors' => [])
+        expect(json.fetch('errors')).to include(
+            'tags' => ['ニコニコ・タグは直接指定できません．'])
       end
     end
 

backend/spec/requests/tags_deerjikists_spec.rb

@@ -275,6 +275,30 @@ RSpec.describe 'Tags deerjikists API', type: :request do
         end
       end
 
+      context 'when a row is invalid' do
+        let(:payload) do
+          [
+            { platform: '', code: code1 },
+          ]
+        end
+
+        it 'returns 422 with indexed field errors and does not replace existing deerjikists' do
+          Deerjikist.create!(platform: platform1, code: code1, tag: tag)
+
+          expect {
+            do_request
+          }.not_to change { Deerjikist.where(tag: tag).map { |d| [d.platform, d.code] } }
+
+          expect(response).to have_http_status(:unprocessable_entity)
+          expect(json).to include(
+              'type' => 'validation_error',
+              'message' => '入力内容を確認してください．',
+              'base_errors' => [])
+          expect(json.fetch('errors')).to include(
+              'deerjikists.0.platform' => [be_present])
+        end
+      end
+
       context 'when youtube code is handle' do
         let(:channel_id) { 'UCabcdefghijklmnopqrstuv' }
         let(:payload) do

backend/spec/requests/users_spec.rb

@@ -90,12 +90,14 @@ RSpec.describe 'Users', type: :request do
       expect(response).to have_http_status(:unauthorized)
     end
 
-    it 'returns 400 when name is blank' do
+    it 'returns 422 when name is blank' do
       put "/users/#{user.id}",
           params: { name: '   ' },
           headers: auth_headers(user)
 
-      expect(response).to have_http_status(:bad_request)
+      expect(response).to have_http_status(:unprocessable_entity)
+      expect(json.fetch('errors')).to include(
+          'name' => ['名前は必須です．'])
     end
 
     it 'updates name and returns user slice' do