diff --git a/lib/concepts/school/operations/create.rb b/lib/concepts/school/operations/create.rb index df1ff5ad6..9923cbf86 100644 --- a/lib/concepts/school/operations/create.rb +++ b/lib/concepts/school/operations/create.rb @@ -2,12 +2,19 @@ class School class Create + LOCK_NAMESPACE = Zlib.crc32(name) + class << self def call(school_params:, creator_id:, token:) response = OperationResponse.new response[:school] = build_school(school_params.merge!(creator_id:)) School.transaction do + # Serialise concurrent creates by the same creator. + # The loser blocks here until the winner's transaction commits, + # so its uniqueness validation sees the winner's school + # rather than hitting the partial unique index on creator_id. + acquire_advisory_lock_for_creator(creator_id) response[:school].save! SchoolOnboardingService.new(response[:school]).onboard(token:) @@ -17,7 +24,12 @@ def call(school_params:, creator_id:, token:) rescue ProfileApiClient::UnauthorizedError => e # Do not log noise to sentry. # TODO: consider returning a separate error here to distinguish from other errors and return 401 from the API, not 422 - Rails.logger.warn { "Failed to onboard school #{response[:school].id}: user is unauthorized" } + Rails.logger.warn { "Failed to onboard school #{response[:school]&.id}: user is unauthorized" } + failure(response, e) + rescue ActiveRecord::RecordInvalid => e + # A double submit loses the advisory lock race and fails the creator_id + # uniqueness validation as expected, so keep it out of Sentry. + Sentry.capture_exception(e) unless response[:school]&.errors&.of_kind?(:creator_id, :taken) failure(response, e) rescue StandardError => e Sentry.capture_exception(e) @@ -26,9 +38,15 @@ def call(school_params:, creator_id:, token:) private + def acquire_advisory_lock_for_creator(creator_id) + lock_key = Zlib.crc32("#{School::Create::LOCK_NAMESPACE}:#{creator_id}") + School.connection.execute("SELECT pg_advisory_xact_lock(#{lock_key})") + end + def failure(response, error) - response[:error] = response[:school].errors.presence || [error.message] - response[:error_types] = response[:school].errors.details + school_errors = response[:school]&.errors + response[:error] = school_errors.presence || [error.message] + response[:error_types] = school_errors&.details || {} response end diff --git a/spec/concepts/school/create_spec.rb b/spec/concepts/school/create_spec.rb index c924a391f..4074187bb 100644 --- a/spec/concepts/school/create_spec.rb +++ b/spec/concepts/school/create_spec.rb @@ -27,6 +27,13 @@ allow(ProfileApiClient).to receive(:create_school).and_return(true) end + it 'acquires an advisory lock keyed on the creator' do + allow(School.connection).to receive(:execute).and_call_original + described_class.call(school_params:, creator_id:, token:) + lock_key = Zlib.crc32("#{School::Create::LOCK_NAMESPACE}:#{creator_id}") + expect(School.connection).to have_received(:execute).with("SELECT pg_advisory_xact_lock(#{lock_key})") + end + it 'returns a successful operation response' do response = described_class.call(school_params:, creator_id:, token:) expect(response.success?).to be(true) @@ -51,6 +58,41 @@ expect(response[:school].creator_id).to eq(creator_id) end + context 'when the creator already has an active school' do + # The advisory lock serialises concurrent requests: the loser only validates + # once the winner has committed, so it sees the winner's school. + before do + allow(Sentry).to receive(:capture_exception) + create(:school, creator_id:, reference: '999999') + end + + it 'does not create a second school' do + expect { described_class.call(school_params:, creator_id:, token:) }.not_to change(School, :count) + end + + it 'returns a failed operation response' do + response = described_class.call(school_params:, creator_id:, token:) + expect(response.failure?).to be(true) + end + + it 'returns a validation error rather than a database uniqueness violation' do + response = described_class.call(school_params:, creator_id:, token:) + expect(response[:error_types][:creator_id]).to eq([{ error: :taken, value: creator_id }]) + end + + it 'does not onboard the second school' do + allow(SchoolOnboardingService).to receive(:new) + described_class.call(school_params:, creator_id:, token:) + expect(SchoolOnboardingService).not_to have_received(:new) + end + + it 'does not capture the error in Sentry' do + allow(Sentry).to receive(:capture_exception) + described_class.call(school_params:, creator_id:, token:) + expect(Sentry).not_to have_received(:capture_exception) + end + end + context 'when creation fails' do let(:school_params) { {} }