#428

backend/spec/services/post_create_plan_spec.rb

@@ -18,9 +18,9 @@ RSpec.describe PostCreatePlan do
       ainu_name = create(:tag_name, name: 'shared_name',
                                       language_code: 'ain', script_code: 'Kana')
       ainu = create(:tag, primary_tag_name: ainu_name, category: :character)
+      primary_tag_name_for(ainu, 'ja').update!(name: 'ainu_in_japanese', auto_generated: false)
       nipponese = create(:tag, primary_name: 'temporary_nipponese', category: :general)
-      # The DB permits this identity; validation has its own contract spec.
-      primary_tag_name_for(nipponese, 'ja').update_columns(name: 'shared_name')
+      primary_tag_name_for(nipponese, 'ja').update!(name: 'shared_name')
       ainu_parent_name = create(:tag_name, name: 'ainu_parent',
                                             language_code: 'ain', script_code: 'Kana')
       ainu_parent = create(:tag, primary_tag_name: ainu_parent_name,

backend/spec/services/shared_resource_id_allocator_mysql_spec.rb

@@ -141,6 +141,40 @@ RSpec.describe 'Shared resource IDs across MySQL connections', type: :model do
     result(first)
   end
 
+  [false, true].each do |inner_rollback|
+    [false, true].each do |outer_rollback|
+      it "keeps savepoint=#{ inner_rollback } lock through outer rollback=#{ outer_rollback }" do
+        connection = ApplicationRecord.connection
+        database = connection.pool.db_config.database
+        lock_name = "btrc:ids:#{ Digest::SHA256.hexdigest(database)[0, 16] }:tags"
+        quoted_lock = connection.quote(lock_name)
+        owner = connection.select_value('SELECT CONNECTION_ID()').to_i
+
+        connection.transaction do
+          connection.transaction(requires_new: true) do
+            create_resource(:external)
+            raise ActiveRecord::Rollback if inner_rollback
+          end
+
+          expect(connection.select_value("SELECT IS_USED_LOCK(#{ quoted_lock })").to_i)
+              .to eq(owner)
+          contender = worker do |other|
+            other.select_value("SELECT GET_LOCK(#{ quoted_lock }, 0)").to_i
+          end
+          expect(result(contender)).to eq(0)
+
+          SharedResourceIdAllocator.lock_space!(ExternalTag, 'tags')
+          raise ActiveRecord::Rollback if outer_rollback
+        end
+
+        expect(connection.select_value("SELECT IS_FREE_LOCK(#{ quoted_lock })").to_i)
+            .to eq(1)
+        expect(ExternalTag.exists?(name: "#{ @prefix }_external"))
+            .to eq(!inner_rollback && !outer_rollback)
+      end
+    end
+  end
+
   it 'raises on a real lock acquisition failure without inserting a record' do
     first, (_id, _connection_id, lock_name) = hold_creation(:external, rollback: true)
     stub_const('SharedResourceIdAllocator::LOCK_TIMEOUT', 0)

backend/spec/services/shared_resource_id_allocator_spec.rb

@@ -101,7 +101,7 @@ RSpec.describe SharedResourceIdAllocator, 'lock lifecycle' do
   let(:record) { build(:external_tag) }
   let(:connection) { double('connection') }
   let(:transaction) { double('transaction', open?: true) }
-  let(:callbacks) { { } }
+  let(:callbacks) { [] }
   let(:sql) { [] }
 
   before do
@@ -128,19 +128,24 @@ RSpec.describe SharedResourceIdAllocator, 'lock lifecycle' do
       end
     end
     allow(ExternalTag).to receive(:connection).and_return(connection)
-    allow(ExternalTag).to receive(:current_transaction).and_return(transaction)
+    allow(connection).to receive(:current_transaction).and_return(transaction)
+    allow(connection).to receive(:add_transaction_record) { callbacks << _1 }
+    allow(connection).to receive(:disconnect!)
     allow(ExternalTag).to receive(:uncached).and_yield
-    [:after_commit, :after_rollback].each do |event|
-      allow(transaction).to receive(event) { |&callback| callbacks[event] = callback }
-    end
   end
 
-  [:after_commit, :after_rollback].each do |event|
+  def finish_transaction event
+    allow(transaction).to receive(:open?).and_return(false)
+    callbacks.shift.public_send(event)
+    allow(transaction).to receive(:open?).and_return(true)
+  end
+
+  [:committed!, :rolledback!].each do |event|
     it "holds the shared lock until #{ event }" do
       described_class.assign!(record)
       expect(record.id).to eq(9)
       expect(sql.grep(/RELEASE_LOCK/)).to be_empty
-      callbacks.fetch(event).call
+      finish_transaction(event)
       expect(sql.grep(/RELEASE_LOCK/).size).to eq(1)
     end
   end
@@ -155,7 +160,7 @@ RSpec.describe SharedResourceIdAllocator, 'lock lifecycle' do
   it 'releases a successfully acquired lock when allocation fails and rolls back' do
     allow(connection).to receive(:select_all).and_raise('query failed')
     expect { described_class.assign!(record) }.to raise_error(RuntimeError, 'query failed')
-    callbacks.fetch(:after_rollback).call
+    finish_transaction(:rolledback!)
     expect(sql.grep(/RELEASE_LOCK/).size).to eq(1)
   end
 
@@ -164,18 +169,17 @@ RSpec.describe SharedResourceIdAllocator, 'lock lifecycle' do
     described_class.assign!(record)
     expect(record.id).to eq(50)
     expect(sql.grep(/FOR UPDATE/)).to be_empty
-    callbacks.fetch(:after_commit).call
+    finish_transaction(:committed!)
   end
 
   it 'uses the same named lock within each space and different locks between spaces' do
     locks = [Tag, ExternalTag, TagDescription, TagGroup, TagDescriptionVersion, TagGroupVersion].map do |model|
       allow(model).to receive(:connection).and_return(connection)
-      allow(model).to receive(:current_transaction).and_return(transaction)
       allow(model).to receive(:uncached).and_yield
       record = model.new(id: 50)
       described_class.assign!(record)
       statement = sql.grep(/GET_LOCK/).last
-      callbacks.fetch(:after_commit).call
+      finish_transaction(:committed!)
       statement
     end
     expect(locks[0]).to eq(locks[1])
@@ -187,7 +191,21 @@ RSpec.describe SharedResourceIdAllocator, 'lock lifecycle' do
   it 'reports lock release failures' do
     described_class.assign!(record)
     allow(connection).to receive(:select_value).with(/RELEASE_LOCK/).and_return(nil)
-    expect { callbacks.fetch(:after_commit).call }
+    expect { finish_transaction(:committed!) }
         .to raise_error(ActiveRecord::ActiveRecordError, /lock release failed/)
+    expect(connection).to have_received(:disconnect!)
+  end
+
+  it 'keeps the lock and transfers cleanup when a savepoint rolls back' do
+    described_class.assign!(record)
+    callbacks.shift.rolledback!
+
+    expect(callbacks.size).to eq(1)
+    expect(sql.grep(/RELEASE_LOCK/)).to be_empty
+    described_class.assign!(record)
+    expect(sql.grep(/GET_LOCK/).size).to eq(1)
+
+    finish_transaction(:committed!)
+    expect(sql.grep(/RELEASE_LOCK/).size).to eq(1)
   end
 end

backend/spec/services/tag_name_transcription_spec.rb

@@ -17,7 +17,7 @@ RSpec.describe TagNameTranscription do
       'ガギグゲゴダヂヅデド' => 'カキクケコタチチュテト',
       'バビブベボパピプペポ' => 'パピプペポパピプペポ',
       'ザジズゼゾ' => 'チャチチュチェチョ',
-      'ジャジュジョ' => 'チヤチユチヨ',
+      'ジャジュジョ' => 'チャチュチョ',
       'アㇰアㇱアッアンアㇺアㇷ゚' => 'アㇰアㇱアッアンアㇺアㇷ゚',
       'アㇻイㇼウㇽエㇾオㇿ' => 'アㇻイㇼウㇽエㇾオㇿ',
       '漢Aあ' => 'カンAア' }.each do |source, expected|
@@ -26,9 +26,12 @@ RSpec.describe TagNameTranscription do
     end
   end
 
-  ['😀', 'ヴ', 'ティ', 'ㇵ', '魑', '𠮷', 'ー', '々'].each do |source|
-    it "rejects unsupported input #{ source }" do
-      expect { described_class.ainu(source) }.to raise_error(described_class::Unconvertible)
+  { '😀' => '', 'ヴ' => '', 'ティ' => 'テ', 'ㇵ' => '',
+    '魑' => '', '𠮷' => '', 'ー' => '', '々' => '',
+    'ア😀😀イ' => 'ア_イ', '😀アイ😀' => 'アイ',
+    '_😀_' => '__', 'ア_😀_イ' => 'ア___イ', '漢魑字' => 'カン_チ' }.each do |source, expected|
+    it "preserves convertible portions of #{ source }" do
+      expect(described_class.ainu(source)).to eq(expected)
     end
   end
 end

backend/spec/services/tag_version_recorder_spec.rb

@@ -64,7 +64,7 @@ RSpec.describe TagVersionRecorder do
   it 'snapshots future languages together without a request locale argument' do
     prepare_ainu_names!
 
-    version = described_class.record!(tag:, event_type: :create, created_by_user: user)
+    version = described_class.record!(tag:, event_type: :update, created_by_user: user)
 
     expect(version.names).to eq(
         'ja' => nipponese_snapshot,
@@ -75,12 +75,12 @@ RSpec.describe TagVersionRecorder do
 
   it 'creates a version for an Ainu-only change and preserves prior names' do
     prepare_ainu_names!
-    original = described_class.record!(tag:, event_type: :create, created_by_user: user)
+    original = described_class.record!(tag:, event_type: :update, created_by_user: user)
     primary_tag_name_for(tag, 'ain').update!(name: 'Renamed')
 
     updated = described_class.record!(tag:, event_type: :update, created_by_user: user)
 
-    expect(updated.version_no).to eq(2)
+    expect(updated.version_no).to eq(original.version_no + 1)
     expect(original.reload.names.fetch('ain')).to include('name' => 'Ainu name')
     expect(updated.names.fetch('ain')).to include('name' => 'Renamed')
     expect(updated.names.fetch('ja')).to eq(nipponese_snapshot)
@@ -88,11 +88,11 @@ RSpec.describe TagVersionRecorder do
 
   it 'does not create a version for an unchanged multilingual snapshot' do
     prepare_ainu_names!
-    described_class.record!(tag:, event_type: :create, created_by_user: user)
+    original = described_class.record!(tag:, event_type: :update, created_by_user: user)
 
     unchanged = described_class.record!(tag:, event_type: :update, created_by_user: user)
 
-    expect(unchanged.version_no).to eq(1)
-    expect(tag.reload.tag_versions.count).to eq(1)
+    expect(unchanged.version_no).to eq(original.version_no)
+    expect(tag.reload.tag_versions.count).to eq(original.version_no)
   end
 end