From 3f2f2ccb7fcf4f46467878265d777477d10ae9d2 Mon Sep 17 00:00:00 2001 From: Justin Coyne Date: Wed, 14 Aug 2019 10:17:26 -0500 Subject: [PATCH] Add a way to do virtual merge through an API --- .rubocop.yml | 4 + .rubocop_todo.yml | 8 -- app/controllers/application_controller.rb | 8 ++ app/controllers/objects_controller.rb | 14 ++ app/services/constituent_service.rb | 43 ++++++ app/services/item_query_service.rb | 36 ++++++ .../reset_content_metadata_service.rb | 15 +++ config/routes.rb | 2 +- spec/rails_helper.rb | 3 + spec/requests/virtual_merge_spec.rb | 53 ++++++++ spec/services/constituent_service_spec.rb | 122 ++++++++++++++++++ 11 files changed, 299 insertions(+), 9 deletions(-) create mode 100644 app/services/constituent_service.rb create mode 100644 app/services/item_query_service.rb create mode 100644 app/services/reset_content_metadata_service.rb create mode 100644 spec/requests/virtual_merge_spec.rb create mode 100644 spec/services/constituent_service_spec.rb diff --git a/.rubocop.yml b/.rubocop.yml index 4a6c6cbbb..f7996f0af 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -21,6 +21,10 @@ Metrics/BlockLength: - 'config/routes.rb' - 'config/initializers/dor_config.rb' +RSpec/DescribeClass: + Exclude: + - 'spec/requests/**/*' + RSpec/ExampleLength: Max: 8 Enabled: false diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index aa00078f7..2e5d1fe6c 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -123,14 +123,6 @@ RSpec/ContextWording: - 'spec/services/registration_service_spec.rb' - 'spec/services/thumbnail_service_spec.rb' -# Offense count: 4 -RSpec/DescribeClass: - Exclude: - - 'spec/requests/about_spec.rb' - - 'spec/requests/authorization_spec.rb' - - 'spec/requests/metadata_refresh_spec.rb' - - 'spec/requests/metadata_spec.rb' - # Offense count: 32 # Cop supports --auto-correct. # Configuration parameters: SkipBlocks, EnforcedStyle. diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 553a1b55d..ad0c85cfd 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -3,6 +3,14 @@ class ApplicationController < ActionController::API include ActionController::MimeResponds + rescue_from ActionController::ParameterMissing do |exception| + render json: { + errors: [ + { title: 'bad request', detail: exception.message } + ] + }, status: :bad_request + end + before_action :check_auth_token # Since Basic auth was already using the Authorization header, we used something diff --git a/app/controllers/objects_controller.rb b/app/controllers/objects_controller.rb index 2d115d148..405c574c7 100644 --- a/app/controllers/objects_controller.rb +++ b/app/controllers/objects_controller.rb @@ -28,6 +28,20 @@ def create end end + # Handles updates to the record. + # Presently this only needs to handle the merge object use case. + # Do this by providing: constituent_ids => ['druid:123', 'druid:345'] + def update + # validate that the constituent_ids parameter is an present, raises ActionController::ParameterMissing + params.require(:constituent_ids) + filtered_params = params.permit(constituent_ids: []) + raise ActionController::ParameterMissing, 'constituent_ids must be an array' unless filtered_params[:constituent_ids] + + # Update the constituent relationship + ConstituentService.new(parent_druid: params[:id]).add(child_druids: filtered_params[:constituent_ids]) + head :no_content + end + def publish PublishMetadataService.publish(@item) head :created diff --git a/app/services/constituent_service.rb b/app/services/constituent_service.rb new file mode 100644 index 000000000..584dbcaeb --- /dev/null +++ b/app/services/constituent_service.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +# Adds a constituent relationship between a parent work and child works +# by taking the followin actions: +# 1. altering the contentMD of the parent +# 2. add isConstituentOf assertions to the RELS-EXT of the children +# 3. saving the parent and the children +class ConstituentService + # @param [String] parent_druid the identifier of the parent object + def initialize(parent_druid:) + @parent_druid = parent_druid + end + + # This resets the contentMetadataDS of the parent and then adds the child resources. + # Typically this is only called one time (with a list of all the pids) because + # subsequent calls will erase the previous changes. + # @param [Array] child_druids the identifiers of the child objects + def add(child_druids:) + ResetContentMetadataService.new(druid: parent_druid).reset + + child_druids.each do |child_druid| + add_constituent(child_druid: child_druid) + end + parent.save! + end + + private + + attr_reader :parent_druid + + def add_constituent(child_druid:) + child = ItemQueryService.find_modifiable_work(child_druid) + child.contentMetadata.ng_xml.search('//resource').each do |resource| + parent.contentMetadata.add_virtual_resource(child.id, resource) + end + child.add_relationship :is_constituent_of, parent + child.save! + end + + def parent + @parent ||= ItemQueryService.find_modifiable_work(parent_druid) + end +end diff --git a/app/services/item_query_service.rb b/app/services/item_query_service.rb new file mode 100644 index 000000000..8a3075ec7 --- /dev/null +++ b/app/services/item_query_service.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +# Responsible for retrieving information based on the given work (Dor::Item). +class ItemQueryService + # @param [String] id - The id of the work + # @param [#exists?, #find] work_relation - How we will query some of the related information + def initialize(id:, work_relation: default_work_relation) + @id = id + @work_relation = work_relation + end + + delegate :allows_modification?, to: :work + + # @raises [RuntimeError] if the item is not modifiable + def self.find_modifiable_work(druid) + query_service = ItemQueryService.new(id: druid) + query_service.work do |work| + raise "Item #{work.pid} is not open for modification" unless query_service.allows_modification? + end + end + + def work(&block) + @work ||= work_relation.find(id) + return @work unless block_given? + + @work.tap(&block) + end + + private + + attr_reader :id, :work_relation + + def default_work_relation + Dor::Item + end +end diff --git a/app/services/reset_content_metadata_service.rb b/app/services/reset_content_metadata_service.rb new file mode 100644 index 000000000..1118df0b8 --- /dev/null +++ b/app/services/reset_content_metadata_service.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +# Clears the contentMetadata datastream to the default, wiping out any members. +class ResetContentMetadataService + def initialize(druid:, type: 'image') + @druid = druid + @type = type + end + + def reset + work = ItemQueryService.find_modifiable_work(@druid) + work.contentMetadata.content = "" + work.save! + end +end diff --git a/config/routes.rb b/config/routes.rb index 123735a18..a71b4f4f4 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -20,7 +20,7 @@ get 'catkey', to: 'marcxml#catkey' end - resources :objects, only: [:create] do + resources :objects, only: [:create, :update] do member do post 'initialize_workspace', to: 'workspaces#create' # deprecated post 'publish' diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 3f09745d8..871056290 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -95,3 +95,6 @@ def fixture_dir def read_fixture(fname) File.read(File.join(fixture_dir, fname)) end + +# Creates a `not_change` matcher +RSpec::Matchers.define_negated_matcher :not_change, :change diff --git a/spec/requests/virtual_merge_spec.rb b/spec/requests/virtual_merge_spec.rb new file mode 100644 index 000000000..ac170fff8 --- /dev/null +++ b/spec/requests/virtual_merge_spec.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'Virtual merge of objects' do + let(:payload) { { sub: 'argo' } } + let(:jwt) { JWT.encode(payload, Settings.dor.hmac_secret, 'HS256') } + let(:parent_id) { 'druid:mk420bs7601' } + let(:child1_id) { 'druid:child1' } + let(:child2_id) { 'druid:child2' } + + let(:object) { Dor::Item.new(pid: parent_id) } + let(:service) { instance_double(ConstituentService, add: true) } + + before do + allow(Dor).to receive(:find).and_return(object) + allow(ConstituentService).to receive(:new).with(parent_druid: parent_id).and_return(service) + end + + context 'when constituent_ids is provided' do + it 'merges the objects' do + put "/v1/objects/#{parent_id}", + params: { constituent_ids: [child1_id, child2_id] }, + headers: { 'X-Auth' => "Bearer #{jwt}" } + expect(service).to have_received(:add).with(child_druids: [child1_id, child2_id]) + expect(response).to be_successful + end + end + + context 'when constituent_ids is not provided' do + it 'renders an error' do + put "/v1/objects/#{parent_id}", + params: { title: 'New name' }, + headers: { 'X-Auth' => "Bearer #{jwt}" } + expect(service).not_to have_received(:add) + expect(response).to be_bad_request + json = JSON.parse(response.body) + expect(json['errors'][0]['detail']).to eq 'param is missing or the value is empty: constituent_ids' + end + end + + context 'when constituent_ids is not an array' do + it 'renders an error' do + put "/v1/objects/#{parent_id}", + params: { constituent_ids: child1_id }, + headers: { 'X-Auth' => "Bearer #{jwt}" } + expect(service).not_to have_received(:add) + expect(response).to be_bad_request + json = JSON.parse(response.body) + expect(json['errors'][0]['detail']).to eq 'param is missing or the value is empty: constituent_ids must be an array' + end + end +end diff --git a/spec/services/constituent_service_spec.rb b/spec/services/constituent_service_spec.rb new file mode 100644 index 000000000..aaf6b6078 --- /dev/null +++ b/spec/services/constituent_service_spec.rb @@ -0,0 +1,122 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe ConstituentService do + let(:parent) do + Dor::Item.new.tap do |item| + item.contentMetadata.content = <<~XML + + + + + + XML + end + end + + let(:child1) do + Dor::Item.new.tap do |item| + item.contentMetadata.content = <<~XML + + + + + + + XML + end + end + + let(:child2) do + Dor::Item.new.tap do |item| + item.contentMetadata.content = <<~XML + + + + + + + XML + end + end + + describe '#add' do + subject(:add) { instance.add(child_druids: [child1.id, child2.id]) } + + let(:instance) do + described_class.new(parent_druid: parent.id) + end + let(:namespaceless) { parent.id.sub('druid:', '') } + let(:client) { instance_double(Dor::Services::Client::Object, version: version_client) } + let(:version_client) { instance_double(Dor::Services::Client::ObjectVersion, close: true) } + + before do + allow(parent).to receive_messages(id: 'druid:parent1', save!: true) + allow(child1).to receive_messages(id: 'druid:child1', save!: true) + allow(child2).to receive_messages(id: 'druid:child2', save!: true) + + # Used in ContentMetadataDS#add_virtual_resource + allow(parent.contentMetadata).to receive(:pid).and_return('druid:parent1') + + allow(ItemQueryService).to receive(:find_modifiable_work).with('druid:parent1').and_return(parent) + allow(ItemQueryService).to receive(:find_modifiable_work).with('druid:child1').and_return(child1) + allow(ItemQueryService).to receive(:find_modifiable_work).with('druid:child2').and_return(child2) + + allow(Dor::Services::Client).to receive(:object).and_return(client) + end + + context 'when the parent is open for modification' do + before do + add + end + + it 'merges objects' do + expect(parent.contentMetadata.content).to be_equivalent_to <<~XML + + + + + + + + + + XML + expect(child1.object_relations[:is_constituent_of]).to eq [parent] + expect(child2.object_relations[:is_constituent_of]).to eq [parent] + end + end + + context 'when the parent is closed for modification' do + before do + allow(ItemQueryService).to receive(:find_modifiable_work).with(parent.id).and_raise('nope') + end + + it 'merges nothing' do + expect { add }.to raise_error(RuntimeError).and(not_change { parent.contentMetadata.content }) + + expect(child1.object_relations[:is_constituent_of]).to be_empty + end + end + + context 'when the child is closed for modification' do + before do + allow(ItemQueryService).to receive(:find_modifiable_work).with(child2.id).and_raise('not modifiable') + end + + it 'merges all the chidren before an error is encountered' do + expect { add }.to raise_error RuntimeError + expect(parent.contentMetadata.content).to be_equivalent_to <<~XML + + + + + + XML + expect(child1.object_relations[:is_constituent_of]).to eq [parent] + expect(child2.object_relations[:is_constituent_of]).to be_empty + end + end + end +end