diff --git a/db/migrations/20180315_create_verification_sequence.rb b/db/migrations/20180315_create_verification_sequence.rb new file mode 100644 index 000000000..c4744b7db --- /dev/null +++ b/db/migrations/20180315_create_verification_sequence.rb @@ -0,0 +1,14 @@ +Sequel.migration do + up do + create_table(:verification_sequence_number) do + Integer :value, null: false + end + + start = (from(:verifications).max(:number) || 0) + 100 + from(:verification_sequence_number).insert(value: start) + end + + down do + drop_table(:verification_sequence_number) + end +end diff --git a/lib/pact_broker/api/resources/verifications.rb b/lib/pact_broker/api/resources/verifications.rb index ace44a90b..a90825fac 100644 --- a/lib/pact_broker/api/resources/verifications.rb +++ b/lib/pact_broker/api/resources/verifications.rb @@ -55,7 +55,7 @@ def pact end def next_verification_number - @next_verification_number ||= verification_service.next_number_for(pact) + @next_verification_number ||= verification_service.next_number end def decorator_for model diff --git a/lib/pact_broker/verifications/repository.rb b/lib/pact_broker/verifications/repository.rb index 0ab454529..b735b4409 100644 --- a/lib/pact_broker/verifications/repository.rb +++ b/lib/pact_broker/verifications/repository.rb @@ -2,6 +2,7 @@ require 'pact_broker/domain/verification' require 'pact_broker/verifications/latest_verifications_by_consumer_version' require 'pact_broker/verifications/all_verifications' +require 'pact_broker/verifications/sequence' module PactBroker module Verifications @@ -10,6 +11,14 @@ class Repository include PactBroker::Repositories::Helpers include PactBroker::Repositories + # Ideally this would just be a sequence, but Sqlite and MySQL don't support sequences + # in the way we need to use them ie. determining what the next number will be before we + # create the record, because Webmachine wants to set the URL of the resource that is about + # to be created *before* we actually create it. + def next_number + Sequence.next_val + end + def create verification, provider_version_number, pact provider = pacticipant_repository.find_by_name(pact.provider_name) version = version_repository.find_by_pacticipant_id_and_number_or_create(provider.id, provider_version_number) @@ -18,10 +27,6 @@ def create verification, provider_version_number, pact verification.save end - def verification_count_for_pact pact - PactBroker::Domain::Verification.where(pact_version_id: pact_version_id_for(pact)).count - end - def find consumer_name, provider_name, pact_version_sha, verification_number PactBroker::Domain::Verification .select_all_qualified diff --git a/lib/pact_broker/verifications/sequence.rb b/lib/pact_broker/verifications/sequence.rb new file mode 100644 index 000000000..9ab9faa00 --- /dev/null +++ b/lib/pact_broker/verifications/sequence.rb @@ -0,0 +1,34 @@ + +require 'sequel' + +module PactBroker + module Verifications + class Sequence < Sequel::Model(:verification_sequence_number) + + dataset_module do + # The easiest way to implement a cross database compatible sequence. + # Sad, I know. + def next_val + db.transaction do + for_update.first + select_all.update(value: Sequel[:value]+1) + row = first + if row + row.value + else + # The first row should have been created in the migration, so this code + # should only ever be executed in a test context. + # There would be a risk of a race condition creating two rows if this + # code executed in prod, as I don't think you can lock an empty table + # to prevent another record being inserted. + max_verification_number = PactBroker::Domain::Verification.max(:number) + value = max_verification_number ? max_verification_number + 100 : 1 + insert(value: value) + value + end + end + end + end + end + end +end diff --git a/lib/pact_broker/verifications/service.rb b/lib/pact_broker/verifications/service.rb index 3de11fe58..eb8c0b5e4 100644 --- a/lib/pact_broker/verifications/service.rb +++ b/lib/pact_broker/verifications/service.rb @@ -12,8 +12,8 @@ module Service extend PactBroker::Repositories extend PactBroker::Services - def next_number_for pact - verification_repository.verification_count_for_pact(pact) + 1 + def next_number + verification_repository.next_number end def create next_verification_number, params, pact diff --git a/spec/lib/pact_broker/api/resources/verifications_spec.rb b/spec/lib/pact_broker/api/resources/verifications_spec.rb index ab934eb45..b458db962 100644 --- a/spec/lib/pact_broker/api/resources/verifications_spec.rb +++ b/spec/lib/pact_broker/api/resources/verifications_spec.rb @@ -46,7 +46,7 @@ module Resources before do allow(Pacts::Service).to receive(:find_pact).and_return(pact) - allow(PactBroker::Verifications::Service).to receive(:next_number_for).and_return(next_verification_number) + allow(PactBroker::Verifications::Service).to receive(:next_number).and_return(next_verification_number) allow(PactBroker::Api::Decorators::VerificationDecorator).to receive(:new).and_return(decorator) end diff --git a/spec/lib/pact_broker/verifications/repository_spec.rb b/spec/lib/pact_broker/verifications/repository_spec.rb index 65f54bd0c..dd516c6c9 100644 --- a/spec/lib/pact_broker/verifications/repository_spec.rb +++ b/spec/lib/pact_broker/verifications/repository_spec.rb @@ -3,35 +3,6 @@ module PactBroker module Verifications describe Repository do - - describe "#verification_count_for_pact" do - let!(:pact_1) do - TestDataBuilder.new - .create_consumer("Consumer") - .create_provider("Provider") - .create_consumer_version("1.2.3") - .create_pact - .create_verification(number: 1) - .create_verification(number: 2) - .and_return(:pact) - end - let!(:pact_2) do - TestDataBuilder.new - .create_consumer("Foo") - .create_provider("Bar") - .create_consumer_version("4.5.6") - .create_pact - .create_verification(number: 1) - .and_return(:pact) - end - - subject { Repository.new.verification_count_for_pact(pact_1) } - - it "returns the number of verifications for the given pact" do - expect(subject).to eq 2 - end - end - describe "#find" do let!(:pact) do builder = TestDataBuilder.new diff --git a/spec/lib/pact_broker/verifications/sequence_spec.rb b/spec/lib/pact_broker/verifications/sequence_spec.rb new file mode 100644 index 000000000..5a51ea69c --- /dev/null +++ b/spec/lib/pact_broker/verifications/sequence_spec.rb @@ -0,0 +1,47 @@ +require 'pact_broker/verifications/sequence' + +module PactBroker + module Verifications + describe Sequence do + describe "#next_val", migration: true do + + before do + PactBroker::Database.migrate + end + + context "when there is a row in the verification_sequence_number table" do + before do + Sequence.select_all.delete + Sequence.insert(value: 1) + end + + it "increments the value and returns it" do + expect(Sequence.next_val).to eq 2 + end + end + + context "when there is no row in the verification_sequence_number table and no existing verifications" do + before do + Sequence.select_all.delete + end + + it "inserts and returns the value 1" do + expect(Sequence.next_val).to eq 1 + end + end + + context "when there is no row in the verification_sequence_number table and there are existing verifications" do + before do + Sequence.select_all.delete + TestDataBuilder.new.create_pact_with_hierarchy("A", "1", "B") + .create_verification(provider_version: "2") + end + + it "inserts a number that is guaranteed to be higher than any of the existing verification numbers from when we tried to do this without a sequence" do + expect(Sequence.next_val).to eq 101 + end + end + end + end + end +end diff --git a/spec/lib/pact_broker/verifications/service_spec.rb b/spec/lib/pact_broker/verifications/service_spec.rb index 7b42b9920..3fa4909cd 100644 --- a/spec/lib/pact_broker/verifications/service_spec.rb +++ b/spec/lib/pact_broker/verifications/service_spec.rb @@ -8,19 +8,6 @@ module Verifications subject { PactBroker::Verifications::Service } - describe "#next_number_for" do - - let(:pact) { double(:pact) } - - before do - allow_any_instance_of(PactBroker::Verifications::Repository).to receive(:verification_count_for_pact).and_return(2) - end - - it "returns the number for the next build to be recorded for a pact" do - expect(subject.next_number_for(pact)).to eq 3 - end - end - describe "#create" do let(:params) { {'success' => true, 'providerApplicationVersion' => '4.5.6'} } let(:pact) { TestDataBuilder.new.create_pact_with_hierarchy.and_return(:pact) }