From 6c11cbe5363c542493a574f42d3a431f1eb817ce Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Fri, 9 Mar 2018 21:40:49 +1100 Subject: [PATCH] fix: ensure matrix is updated when pacticipant is deleted --- lib/pact_broker/api.rb | 2 +- .../api/resources/base_resource.rb | 9 +- lib/pact_broker/api/resources/pact.rb | 7 +- lib/pact_broker/api/resources/pacticipant.rb | 10 +- lib/pact_broker/api/resources/tag.rb | 6 +- .../api/resources/verifications.rb | 4 + lib/pact_broker/api/resources/version.rb | 4 +- lib/pact_broker/matrix/repository.rb | 40 +++++- lib/pact_broker/matrix/row.rb | 42 ++---- lib/pact_broker/matrix/service.rb | 8 +- spec/features/publish_verification_spec.rb | 1 - spec/features/update_matrix_spec.rb | 127 ++++++++++++++++++ .../lib/pact_broker/matrix/repository_spec.rb | 20 +++ spec/lib/pact_broker/matrix/row_spec.rb | 20 +-- spec/support/test_data_builder.rb | 2 + 15 files changed, 251 insertions(+), 51 deletions(-) create mode 100644 spec/features/update_matrix_spec.rb diff --git a/lib/pact_broker/api.rb b/lib/pact_broker/api.rb index 68e8939ca..73bea53e5 100644 --- a/lib/pact_broker/api.rb +++ b/lib/pact_broker/api.rb @@ -41,7 +41,7 @@ module PactBroker # Pacticipants add ['pacticipants'], Api::Resources::Pacticipants, {resource_name: "pacticipants"} add ['pacticipants', 'label', :label_name], PactBroker::Api::Resources::PacticipantsForLabel, {resource_name: "pacticipants_for_label"} - add ['pacticipants', :name], Api::Resources::Pacticipant, {resource_name: "pacticipant"} + add ['pacticipants', :pacticipant_name], Api::Resources::Pacticipant, {resource_name: "pacticipant"} add ['pacticipants', :pacticipant_name, 'versions'], Api::Resources::Versions, {resource_name: "pacticipant_versions"} add ['pacticipants', :pacticipant_name, 'versions', :pacticipant_version_number], Api::Resources::Version, {resource_name: "pacticipant_version"} add ['pacticipants', :pacticipant_name, 'latest-version', :tag], Api::Resources::Version, {resource_name: "latest_tagged_pacticipant_version"} diff --git a/lib/pact_broker/api/resources/base_resource.rb b/lib/pact_broker/api/resources/base_resource.rb index cc8532921..e533b1c51 100644 --- a/lib/pact_broker/api/resources/base_resource.rb +++ b/lib/pact_broker/api/resources/base_resource.rb @@ -40,8 +40,12 @@ def initialize PactBroker.configuration.before_resource.call(self) end + def update_matrix_after_request? + false + end + def finish_request - if !request.get? && !request.head? + if update_matrix_after_request? matrix_service.refresh(identifier_from_path) end PactBroker.configuration.after_resource.call(self) @@ -155,6 +159,9 @@ def contract_validation_errors? contract, params invalid end + def with_matrix_refresh &block + matrix_service.refresh(identifier_from_path, &block) + end end end end diff --git a/lib/pact_broker/api/resources/pact.rb b/lib/pact_broker/api/resources/pact.rb index 47268fb83..bb7f27ee4 100644 --- a/lib/pact_broker/api/resources/pact.rb +++ b/lib/pact_broker/api/resources/pact.rb @@ -87,7 +87,9 @@ def to_html end def delete_resource - pact_service.delete(pact_params) + with_matrix_refresh do + pact_service.delete(pact_params) + end true end @@ -101,6 +103,9 @@ def pact_params @pact_params ||= PactBroker::Pacts::PactParams.from_request request, path_info end + def update_matrix_after_request? + request.put? || request.patch? + end end end end diff --git a/lib/pact_broker/api/resources/pacticipant.rb b/lib/pact_broker/api/resources/pacticipant.rb index e958b1558..36293ffe2 100644 --- a/lib/pact_broker/api/resources/pacticipant.rb +++ b/lib/pact_broker/api/resources/pacticipant.rb @@ -45,7 +45,9 @@ def resource_exists? end def delete_resource - pacticipant_service.delete pacticipant_name + with_matrix_refresh do + pacticipant_service.delete pacticipant_name + end true end @@ -60,7 +62,11 @@ def pacticipant end def pacticipant_name - identifier_from_path[:name] + identifier_from_path[:pacticipant_name] + end + + def update_matrix_after_request? + request.patch? end end end diff --git a/lib/pact_broker/api/resources/tag.rb b/lib/pact_broker/api/resources/tag.rb index d6cd2ed77..ebdfb6269 100644 --- a/lib/pact_broker/api/resources/tag.rb +++ b/lib/pact_broker/api/resources/tag.rb @@ -22,6 +22,7 @@ def from_json @tag = tag_service.create identifier_from_path # Make it return a 201 by setting the Location header response.headers["Location"] = tag_url(base_url, tag) + matrix_service.refresh_tags(identifier_from_path) end response.body = to_json end @@ -39,10 +40,11 @@ def tag end def delete_resource - tag_service.delete identifier_from_path + matrix_service.refresh_tags(identifier_from_path) do + tag_service.delete identifier_from_path + end true end - end end diff --git a/lib/pact_broker/api/resources/verifications.rb b/lib/pact_broker/api/resources/verifications.rb index 4e85e8100..ace44a90b 100644 --- a/lib/pact_broker/api/resources/verifications.rb +++ b/lib/pact_broker/api/resources/verifications.rb @@ -61,6 +61,10 @@ def next_verification_number def decorator_for model PactBroker::Api::Decorators::VerificationDecorator.new(model) end + + def update_matrix_after_request? + request.post? + end end end end diff --git a/lib/pact_broker/api/resources/version.rb b/lib/pact_broker/api/resources/version.rb index c87f7a8f1..1ab04e392 100644 --- a/lib/pact_broker/api/resources/version.rb +++ b/lib/pact_broker/api/resources/version.rb @@ -25,7 +25,9 @@ def to_json end def delete_resource - version_service.delete version + with_matrix_refresh do + version_service.delete version + end true end diff --git a/lib/pact_broker/matrix/repository.rb b/lib/pact_broker/matrix/repository.rb index 21295cff0..31f24142d 100644 --- a/lib/pact_broker/matrix/repository.rb +++ b/lib/pact_broker/matrix/repository.rb @@ -22,9 +22,45 @@ class Repository GROUP_BY_PROVIDER = [:consumer_name, :consumer_version_number, :provider_name] GROUP_BY_PACT = [:consumer_name, :provider_name] + # Use a block when the refresh is caused by a resource deletion + # This allows us to store the correct object ids for use afterwards def refresh params - PactBroker::Matrix::Row.refresh(params) - PactBroker::Matrix::HeadRow.refresh(params) + criteria = find_ids_for_pacticipant_names(params) + yield if block_given? + PactBroker::Matrix::Row.refresh(criteria) + PactBroker::Matrix::HeadRow.refresh(criteria) + end + + # Only need to update the HeadRow table when tags change + # because it only changes which rows are the latest tagged ones - + # it doesn't change the actual values in the underlying matrix. + def refresh_tags params + criteria = find_ids_for_pacticipant_names(params) + yield if block_given? + PactBroker::Matrix::HeadRow.refresh(criteria) + end + + def find_ids_for_pacticipant_names params + criteria = {} + + if params[:consumer_name] || params[:provider_name] + if params[:consumer_name] + pacticipant = PactBroker::Domain::Pacticipant.where(name_like(:name, params[:consumer_name])).single_record + criteria[:consumer_id] = pacticipant.id if pacticipant + end + + if params[:provider_name] + pacticipant = PactBroker::Domain::Pacticipant.where(name_like(:name, params[:provider_name])).single_record + criteria[:provider_id] = pacticipant.id if pacticipant + end + end + + if params[:pacticipant_name] + pacticipant = PactBroker::Domain::Pacticipant.where(name_like(:name, params[:pacticipant_name])).single_record + criteria[:pacticipant_id] = pacticipant.id if pacticipant + end + + criteria end # Return the latest matrix row (pact/verification) for each consumer_version_number/provider_version_number diff --git a/lib/pact_broker/matrix/row.rb b/lib/pact_broker/matrix/row.rb index a6df45246..2a60de1ce 100644 --- a/lib/pact_broker/matrix/row.rb +++ b/lib/pact_broker/matrix/row.rb @@ -19,42 +19,28 @@ class Row < Sequel::Model(:materialized_matrix) include PactBroker::Repositories::Helpers include PactBroker::Logging - def refresh params - logger.debug("Refreshing #{model.table_name} for #{params}") + def refresh ids + logger.debug("Refreshing #{model.table_name} for #{ids}") db = model.db table_name = model.table_name - source_view_name = model.table_name.to_s.gsub('materialized_', '').to_sym - if params[:consumer_name] || params[:provider_name] - criteria = {} - if params[:consumer_name] - pacticipant = PactBroker::Domain::Pacticipant.where(name_like(:name, params[:consumer_name])).single_record - criteria[:consumer_id] = pacticipant.id if pacticipant + if ids[:pacticipant_id] + db.transaction do + db[table_name].where(consumer_id: ids[:pacticipant_id]).or(provider_id: ids[:pacticipant_id]).delete + new_rows = db[source_view_name].where(consumer_id: ids[:pacticipant_id]).or(provider_id: ids[:pacticipant_id]).distinct + db[table_name].insert(new_rows) end - - if params[:provider_name] - pacticipant = PactBroker::Domain::Pacticipant.where(name_like(:name, params[:provider_name])).single_record - criteria[:provider_id] = pacticipant.id if pacticipant - end - if criteria.any? - db.transaction do - db[table_name].where(criteria).delete - db[table_name].insert(db[source_view_name].where(criteria).distinct) - end + elsif ids.any? + db.transaction do + db[table_name].where(ids).delete + db[table_name].insert(db[source_view_name].where(ids).distinct) end end + end - if params[:pacticipant_name] - pacticipant = PactBroker::Domain::Pacticipant.where(name_like(:name, params[:pacticipant_name])).single_record - if pacticipant - db.transaction do - db[table_name].where(consumer_id: pacticipant.id).or(provider_id: pacticipant.id).delete - new_rows = db[source_view_name].where(consumer_id: pacticipant.id).or(provider_id: pacticipant.id).distinct - db[table_name].insert(new_rows) - end - end - end + def source_view_name + model.table_name.to_s.gsub('materialized_', '').to_sym end def matching_selectors selectors diff --git a/lib/pact_broker/matrix/service.rb b/lib/pact_broker/matrix/service.rb index 8efca8d8b..abecfdff9 100644 --- a/lib/pact_broker/matrix/service.rb +++ b/lib/pact_broker/matrix/service.rb @@ -9,8 +9,12 @@ module Service extend PactBroker::Repositories extend PactBroker::Services - def refresh params - matrix_repository.refresh params + def refresh params, &block + matrix_repository.refresh(params, &block) + end + + def refresh_tags params, &block + matrix_repository.refresh_tags(params, &block) end def find criteria, options = {} diff --git a/spec/features/publish_verification_spec.rb b/spec/features/publish_verification_spec.rb index 871ccf92e..67a4277d5 100644 --- a/spec/features/publish_verification_spec.rb +++ b/spec/features/publish_verification_spec.rb @@ -10,7 +10,6 @@ subject { post path, verification_content, {'CONTENT_TYPE' => 'application/json' }; last_response } - before do td.create_provider("Provider") .create_consumer("Consumer") diff --git a/spec/features/update_matrix_spec.rb b/spec/features/update_matrix_spec.rb new file mode 100644 index 000000000..19c5c1e37 --- /dev/null +++ b/spec/features/update_matrix_spec.rb @@ -0,0 +1,127 @@ +=begin + +Things that should update the matrix (ignoring tags): +* pact created +* pact content updated +* pact deleted +* verification created +* verification deleted (not yet implemented) + +It's easier to update the matrix at the resource level, so we actually update the matrix when: +* pacticipant deleted +* version deleted +* pact deleted + +Things that should update the head matrix +* All of the above +* tag created +* tag deleted + +=end + +describe "Deleting a resource that affects the matrix" do + + let(:td) { TestDataBuilder.new } + let(:response_body_json) { JSON.parse(subject.body) } + + subject { delete path; last_response } + + before do + td.create_pact_with_hierarchy("Foo", "1", "Bar") + .create_consumer_version_tag("prod") + .create_verification(provider_version: "2") + end + + context "deleting a pact" do + let(:path) { "/pacts/provider/Bar/consumer/Foo/version/1" } + + it "deletes the relevant lines from the matrix" do + expect{ subject }.to change{ PactBroker::Matrix::Row.count }.by(-1) + end + end + + context "deleting a pacticipant" do + let(:path) { "/pacticipants/Bar" } + + it "deletes the relevant lines from the matrix" do + expect{ subject }.to change{ PactBroker::Matrix::Row.count }.by(-1) + end + end + + context "deleting a version" do + let(:path) { "/pacticipants/Foo/versions/1" } + + it "deletes the relevant lines from the matrix" do + expect{ subject }.to change{ PactBroker::Matrix::Row.count }.by(-1) + end + end + + context "deleting a tag" do + let(:path) { "/pacticipants/Foo/versions/1/tags/prod" } + + it "deletes the relevant lines from the matrix" do + expect{ subject }.to change{ PactBroker::Matrix::HeadRow.count }.by(-1) + end + end +end + +describe "Creating a resource that affects the matrix" do + + let(:td) { TestDataBuilder.new } + let(:response_body_json) { JSON.parse(subject.body) } + + subject { put(path, nil, {'CONTENT_TYPE' => 'application/json'}); last_response } + + context "creating a tag" do + before do + td.create_pact_with_hierarchy("Foo", "1", "Bar") + end + + let(:path) { "/pacticipants/Foo/versions/1/tags/prod" } + + it "adds the relevant lines to the head matrix" do + expect{ subject }.to change{ PactBroker::Matrix::HeadRow.count }.by(1) + end + + it "does not add any lines to the matrix" do + expect{ subject }.to change{ PactBroker::Matrix::Row.count }.by(0) + end + end + + context "creating a pact" do + let(:pact_content) { load_fixture('foo-bar.json') } + let(:path) { "/pacts/provider/Bar/consumer/Foo/versions/1.2.3" } + + subject { put path, pact_content, {'CONTENT_TYPE' => 'application/json'}; last_response } + + it "adds the relevant lines to the matrix" do + expect{ subject }.to change{ PactBroker::Matrix::Row.count }.by(1) + end + + it "adds the relevant lines to the head matrix" do + expect{ subject }.to change{ PactBroker::Matrix::HeadRow.count }.by(1) + end + end + + context "creating a verification" do + let(:td) { TestDataBuilder.new } + let(:path) { "/pacts/provider/Bar/consumer/Foo/pact-version/#{pact.pact_version_sha}/verification-results" } + let(:verification_content) { load_fixture('verification.json') } + let(:parsed_response_body) { JSON.parse(subject.body) } + let(:pact) { td.pact } + + subject { post path, verification_content, {'CONTENT_TYPE' => 'application/json' }; last_response } + + before do + td.create_pact_with_hierarchy("Foo", "1", "Bar") + end + + it "updates the relevant lines in the matrix" do + expect{ subject }.to change{ PactBroker::Matrix::Row.first.provider_version_number }.from(nil).to("4.5.6") + end + + it "updates the relevant lines in the head matrix" do + expect{ subject }.to change{ PactBroker::Matrix::HeadRow.first.provider_version_number }.from(nil).to("4.5.6") + end + end +end diff --git a/spec/lib/pact_broker/matrix/repository_spec.rb b/spec/lib/pact_broker/matrix/repository_spec.rb index 6e5e34697..616125460 100644 --- a/spec/lib/pact_broker/matrix/repository_spec.rb +++ b/spec/lib/pact_broker/matrix/repository_spec.rb @@ -19,6 +19,26 @@ def shorten_rows rows rows.collect{ |r| shorten_row(r) } end + describe "refresh" do + before do + td.create_pact_with_hierarchy("Foo", "1", "Bar") + Row.refresh(pacticipant_id: td.provider.id) + end + + context "when deleting an object in the block" do + it "removes the relevant lines from the matrix" do + expect(Row.count).to_not eq 0 + Repository.new.refresh(pacticipant_name: "Bar") { PactBroker::Pacticipants::Service.delete("Bar") } + expect(Row.count).to eq 0 + end + + it "yields the block" do + Repository.new.refresh(pacticipant_name: "Bar") { PactBroker::Pacticipants::Service.delete("Bar") } + expect(PactBroker::Domain::Pacticipant.where(name: "Bar").count).to eq 0 + end + end + end + describe "find" do before do # A1 - B1 diff --git a/spec/lib/pact_broker/matrix/row_spec.rb b/spec/lib/pact_broker/matrix/row_spec.rb index ef8948922..75a364d26 100644 --- a/spec/lib/pact_broker/matrix/row_spec.rb +++ b/spec/lib/pact_broker/matrix/row_spec.rb @@ -14,8 +14,8 @@ module Matrix td.create_pact_with_hierarchy("Foo", "1", "Bar") end - context "with only a consumer_name" do - subject { Row.refresh(consumer_name: "Foo") } + context "with only a consumer_id" do + subject { Row.refresh(consumer_id: td.consumer.id) } it "refreshes the data for the consumer" do subject @@ -23,8 +23,8 @@ module Matrix end end - context "with only a provider_name" do - subject { Row.refresh(provider_name: "Bar") } + context "with only a provider_id" do + subject { Row.refresh(provider_id: td.provider.id) } it "refreshes the data for the provider" do subject @@ -32,8 +32,8 @@ module Matrix end end - context "with both consumer_name and provider_name" do - subject { Row.refresh(provider_name: "Bar", consumer_name: "Foo") } + context "with both consumer_id and provider_id" do + subject { Row.refresh(provider_id: td.provider.id, consumer_id: td.consumer.id) } it "refreshes the data for the consumer and provider" do subject @@ -43,17 +43,17 @@ module Matrix context "when there was existing data" do it "deletes the existing data before inserting the new data" do - Row.refresh(provider_name: "Bar", consumer_name: "Foo") + Row.refresh(provider_id: td.provider.id, consumer_id: td.consumer.id) expect(Row.count).to eq 1 td.create_consumer_version("2") .create_pact - Row.refresh(provider_name: "Bar", consumer_name: "Foo") + Row.refresh(provider_id: td.provider.id, consumer_id: td.consumer.id) expect(Row.count).to eq 2 end end - context "with a pacticipant_name" do - subject { Row.refresh(pacticipant_name: "Bar") } + context "with a pacticipant_id" do + subject { Row.refresh(pacticipant_id: td.provider.id) } it "refreshes the data for both consumer and provider roles" do subject diff --git a/spec/support/test_data_builder.rb b/spec/support/test_data_builder.rb index c505e2e9b..fe22fa716 100644 --- a/spec/support/test_data_builder.rb +++ b/spec/support/test_data_builder.rb @@ -55,6 +55,7 @@ def refresh_matrix params[:provider_name] = provider.name if provider matrix_service.refresh(params) # Row is not used in production code, but the tests depend on it + # Technically this code is expecting ids, but it will work with names too PactBroker::Matrix::Row.refresh(params) end end @@ -197,6 +198,7 @@ def create_tag tag_name, params = {} def create_consumer_version_tag tag_name, params = {} params.delete(:comment) @tag = PactBroker::Domain::Tag.create(name: tag_name, version: @consumer_version) + refresh_matrix self end