From e716d81bc25c520252f42daa0496ea456f559f39 Mon Sep 17 00:00:00 2001 From: Bryan Hockey Date: Wed, 21 Aug 2019 14:57:48 -0500 Subject: [PATCH 01/26] Fix long comment lines in IncomingStorage --- lib/chipmunk/incoming_storage.rb | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/lib/chipmunk/incoming_storage.rb b/lib/chipmunk/incoming_storage.rb index f01402ca..d5d4eeaa 100644 --- a/lib/chipmunk/incoming_storage.rb +++ b/lib/chipmunk/incoming_storage.rb @@ -3,17 +3,19 @@ require "pathname" module Chipmunk - # IncomingStorage is responsible for the business rules of writing to and reading from - # Volumes earmarked for its use. It is specifically concerned with initial upload of bags, - # and makes them available to other parts of the system via its methods. + # IncomingStorage is responsible for the business rules of writing to and reading + # from Volumes earmarked for its use. It is specifically concerned with initial + # upload of bags, and makes them available to other parts of the system via its + # methods. class IncomingStorage # Create an IncomingStorage instance. - # @param volume [Volume] The Volume from which the deposited packages should be ingested - # @param paths [PathBuilder] A PathBuilder that returns a path on disk to the user upload - # location for a deposit, for a given package. - # @param links [PathBuilder] A PathBuilder that returns an rsync destination to which the - # user should upload, for a given package. + # @param volume [Volume] The Volume from which the deposited packages should be + # ingested + # @param paths [PathBuilder] A PathBuilder that returns a path on disk to the user + # upload location for a deposit, for a given package. + # @param links [PathBuilder] A PathBuilder that returns an rsync destination to + # which the user should upload, for a given package. def initialize(volume:, paths:, links:) @volume = volume @paths = paths From 5e4a25ef63cf8274804d8900b9e66fead3e0bfca Mon Sep 17 00:00:00 2001 From: Bryan Hockey Date: Thu, 15 Aug 2019 12:58:57 -0500 Subject: [PATCH 02/26] Remove cruft from spec_helper, rails_helper --- spec/rails_helper.rb | 45 -------------------------------------------- spec/spec_helper.rb | 22 ---------------------- 2 files changed, 67 deletions(-) diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 05efcb36..8a685d58 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -1,59 +1,14 @@ # frozen_string_literal: true -# This file is copied to spec/ when you run 'rails generate rspec:install' require "spec_helper" ENV["RAILS_ENV"] ||= "test" require File.expand_path("../config/environment", __dir__) -# Prevent database truncation if the environment is production abort("The Rails environment is running in production mode!") if Rails.env.production? require "rspec/rails" -# Add additional requires below this line. Rails is not loaded until this point! -# Requires supporting ruby files with custom matchers and macros, etc, in -# spec/support/ and its subdirectories. Files matching `spec/**/*_spec.rb` are -# run as spec files by default. This means that files in spec/support that end -# in _spec.rb will both be required and run as specs, causing the specs to be -# run twice. It is recommended that you do not name files matching this glob to -# end with _spec.rb. You can configure this pattern with the --pattern -# option on the command line or in ~/.rspec, .rspec or `.rspec-local`. -# -# The following line is provided for convenience purposes. It has the downside -# of increasing the boot-up time by auto-requiring all files in the support -# directory. Alternatively, in the individual `*_spec.rb` files, manually -# require only the support files necessary. -# -# Dir[Rails.root.join('spec/support/**/*.rb')].each { |f| require f } - -# Checks for pending migration and applies them before tests are run. -# If you are not using ActiveRecord, you can remove this line. ActiveRecord::Migration.maintain_test_schema! RSpec.configure do |config| - # Remove this line if you're not using ActiveRecord or ActiveRecord fixtures - config.fixture_path = "#{::Rails.root}/spec/fixtures" - - # If you're not using ActiveRecord, or you'd prefer not to run each of your - # examples within a transaction, remove the following line or assign false - # instead of true. config.use_transactional_fixtures = true - - # RSpec Rails can automatically mix in different behaviours to your tests - # based on their file location, for example enabling you to call `get` and - # `post` in specs under `spec/controllers`. - # - # You can disable this behaviour by removing the line below, and instead - # explicitly tag your specs with their type, e.g.: - # - # RSpec.describe UsersController, :type => :controller do - # # ... - # end - # - # The different available types are documented in the features, such as in - # https://relishapp.com/rspec/rspec-rails/docs config.infer_spec_type_from_file_location! - - # Filter lines from Rails gems in backtraces. - config.filter_rails_from_backtrace! - # arbitrary gems may also be filtered via: - # config.filter_gems_from_backtrace("gem name") end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 97815134..d30f04f4 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -6,38 +6,16 @@ require "chipmunk" RSpec.configure do |config| - # rspec-expectations config goes here. You can use an alternate - # assertion/expectation library such as wrong or the stdlib/minitest - # assertions if you prefer. config.expect_with :rspec do |expectations| - # This option will default to `true` in RSpec 4. It makes the `description` - # and `failure_message` of custom matchers include text for helper methods - # defined using `chain`, e.g.: - # be_bigger_than(2).and_smaller_than(4).description - # # => "be bigger than 2 and smaller than 4" - # ...rather than: - # # => "be bigger than 2" expectations.include_chain_clauses_in_custom_matcher_descriptions = true end - # rspec-mocks config goes here. You can use an alternate test double - # library (such as bogus or mocha) by changing the `mock_with` option here. config.mock_with :rspec do |mocks| - # Prevents you from mocking or stubbing a method that does not exist on - # a real object. This is generally recommended, and will default to - # `true` in RSpec 4. mocks.verify_partial_doubles = true end - # This option will default to `:apply_to_host_groups` in RSpec 4 (and will - # have no way to turn it off -- the option exists only for backwards - # compatibility in RSpec 3). It causes shared context metadata to be - # inherited by the metadata hash of host groups and examples, rather than - # triggering implicit auto-inclusion in groups with matching metadata. config.shared_context_metadata_behavior = :apply_to_host_groups - config.filter_run_excluding integration: true unless ENV["RUN_INTEGRATION"] - config.example_status_persistence_file_path = "spec/examples.txt" config.after(:each) do # Reset the unique generator used for usernames for each example From c08a156ce0e83107950078a3480b7af49067adfe Mon Sep 17 00:00:00 2001 From: Bryan Hockey Date: Thu, 29 Aug 2019 23:14:25 -0500 Subject: [PATCH 03/26] Add compliant bag fixture --- .../14d25bcd-deaf-4c94-add7-c189fdca4692/bag-info.txt | 3 +++ .../fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/bagit.txt | 2 ++ .../14d25bcd-deaf-4c94-add7-c189fdca4692/chipmunk-info.txt | 6 ++++++ .../14d25bcd-deaf-4c94-add7-c189fdca4692/data/samplefile | 1 + .../14d25bcd-deaf-4c94-add7-c189fdca4692/manifest-md5.txt | 1 + .../14d25bcd-deaf-4c94-add7-c189fdca4692/manifest-sha1.txt | 1 + .../fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/meta.xml | 0 .../tagmanifest-md5.txt | 6 ++++++ .../tagmanifest-sha1.txt | 6 ++++++ 9 files changed, 26 insertions(+) create mode 100644 spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/bag-info.txt create mode 100644 spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/bagit.txt create mode 100644 spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/chipmunk-info.txt create mode 100644 spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/data/samplefile create mode 100644 spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/manifest-md5.txt create mode 100644 spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/manifest-sha1.txt create mode 100644 spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/meta.xml create mode 100644 spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/tagmanifest-md5.txt create mode 100644 spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/tagmanifest-sha1.txt diff --git a/spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/bag-info.txt b/spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/bag-info.txt new file mode 100644 index 00000000..74896c84 --- /dev/null +++ b/spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/bag-info.txt @@ -0,0 +1,3 @@ +Bag-Software-Agent: BagIt Ruby Gem (http://bagit.rubyforge.org) +Bagging-Date: 2017-06-19 +Payload-Oxum: 11.1 diff --git a/spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/bagit.txt b/spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/bagit.txt new file mode 100644 index 00000000..c4aebb43 --- /dev/null +++ b/spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/bagit.txt @@ -0,0 +1,2 @@ +BagIt-Version: 0.97 +Tag-File-Character-Encoding: UTF-8 diff --git a/spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/chipmunk-info.txt b/spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/chipmunk-info.txt new file mode 100644 index 00000000..152b8126 --- /dev/null +++ b/spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/chipmunk-info.txt @@ -0,0 +1,6 @@ +External-Identifier: 32334b0c-3977-4ae4-b5dd-57fedb60a789 +Chipmunk-Content-Type: audio +Bag-ID: 14d25bcd-deaf-4c94-add7-c189fdca4692 +Metadata-Tagfile: meta.xml +Metadata-Type: yes +Metadata-URL: http://what.ever diff --git a/spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/data/samplefile b/spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/data/samplefile new file mode 100644 index 00000000..bb215b5b --- /dev/null +++ b/spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/data/samplefile @@ -0,0 +1 @@ +Hello Bag! diff --git a/spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/manifest-md5.txt b/spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/manifest-md5.txt new file mode 100644 index 00000000..98a00b26 --- /dev/null +++ b/spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/manifest-md5.txt @@ -0,0 +1 @@ +71da23dbd1935f78b24c790c47992691 data/samplefile diff --git a/spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/manifest-sha1.txt b/spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/manifest-sha1.txt new file mode 100644 index 00000000..6053ea85 --- /dev/null +++ b/spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/manifest-sha1.txt @@ -0,0 +1 @@ +6570a827884d8936ea2f8b084705c24aa6f9d7ee data/samplefile diff --git a/spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/meta.xml b/spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/meta.xml new file mode 100644 index 00000000..e69de29b diff --git a/spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/tagmanifest-md5.txt b/spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/tagmanifest-md5.txt new file mode 100644 index 00000000..27832f76 --- /dev/null +++ b/spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/tagmanifest-md5.txt @@ -0,0 +1,6 @@ +22340b9567d070bf6022b85e68948727 bag-info.txt +9e5ad981e0d29adc278f6a294b8c2aca bagit.txt +16f51a4d0d65ed218dbc53783320d92a chipmunk-info.txt +a7a2803e003c74159725db55d91a5981 manifest-md5.txt +c5f316f266735235e4c62c88983ac711 manifest-sha1.txt +d41d8cd98f00b204e9800998ecf8427e meta.xml diff --git a/spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/tagmanifest-sha1.txt b/spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/tagmanifest-sha1.txt new file mode 100644 index 00000000..71705a82 --- /dev/null +++ b/spec/support/fixtures/14d25bcd-deaf-4c94-add7-c189fdca4692/tagmanifest-sha1.txt @@ -0,0 +1,6 @@ +e29dcf1ce489cb9fe75631a16918c1206330f01b bag-info.txt +e2924b081506bac23f5fffe650ad1848a1c8ac1d bagit.txt +8854f01854401499b0304345cdfec9ea6b64d7f3 chipmunk-info.txt +ea0b21f9502fc9155bc065faaab275fe9dfd637f manifest-md5.txt +0f1e283905e1e0c5082ce6b9dd34e8c3d589b9ce manifest-sha1.txt +da39a3ee5e6b4b0d3255bfef95601890afd80709 meta.xml From 1c1adb6dc71893b8bb2b9e563818deb8570712f0 Mon Sep 17 00:00:00 2001 From: Bryan Hockey Date: Thu, 29 Aug 2019 22:46:45 -0500 Subject: [PATCH 04/26] Define steps in deposit artifact scenario (v2) --- features/step_definitions/sort_these_asap.rb | 24 ++++++++++++++++---- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/features/step_definitions/sort_these_asap.rb b/features/step_definitions/sort_these_asap.rb index f2cc5828..cf46e387 100644 --- a/features/step_definitions/sort_these_asap.rb +++ b/features/step_definitions/sort_these_asap.rb @@ -1,23 +1,37 @@ # frozen_string_literal: true Given("I am a content steward") do - pending # Write code here that turns the phrase above into concrete actions + make_me_a("content_manager").on_all("audio") end Given("an audio object that is not in the repository") do - pending # Write code here that turns the phrase above into concrete actions + @bag = Chipmunk::Bag.new(fixture("14d25bcd-deaf-4c94-add7-c189fdca4692")) end When("I deposit the object as a bag") do - pending # Write code here that turns the phrase above into concrete actions + api_post( + "/v2/artifacts", + id: @bag.external_id, + content_type: @bag.content_type, + storage_format: "bag" + ) + + api_post("/v2/artifacts/#{@bag.external_id}/revisions") + deposit = JSON.parse(last_response.body) + FileUtils.cp_r @bag.bag_dir, deposit["upload_link"].split(":").last + api_post("/v2/deposits/#{deposit["id"]}/complete") # Should this be a put? end Then("it is preserved as an artifact") do - pending # Write code here that turns the phrase above into concrete actions + @artifact = Artifact.find(@bag.external_id) + expect(Services.storage.for(@artifact)).to be_valid end Then("the artifact has the identifier from within the bag") do - pending # Write code here that turns the phrase above into concrete actions + api_get( + "/v2/artifacts/#{@bag.external_id}/" + ) + expect(last_response.successful?).to be true end Given("a preserved audio artifact") do From a2cb7d1890e3516d4f9c9ad1223ca32a871fa133 Mon Sep 17 00:00:00 2001 From: Bryan Hockey Date: Thu, 29 Aug 2019 22:43:24 -0500 Subject: [PATCH 05/26] Add Artifact --- app/controllers/v2/artifacts_controller.rb | 47 +++++++++++++++++ app/models/artifact.rb | 52 +++++++++++++++++++ app/policies/artifacts_policy.rb | 24 +++++++++ .../v2/artifacts/_artifact.json.jbuilder | 8 +++ app/views/v2/artifacts/create.json.jbuilder | 3 ++ app/views/v2/artifacts/index.json.jbuilder | 3 ++ app/views/v2/artifacts/show.json.jbuilder | 3 ++ config/initializers/services.rb | 3 ++ db/migrate/20190802204940_create_artifacts.rb | 14 +++++ spec/fabricators/artifact_fabricactor.rb | 7 +++ spec/models/artifact_spec.rb | 42 +++++++++++++++ 11 files changed, 206 insertions(+) create mode 100644 app/controllers/v2/artifacts_controller.rb create mode 100644 app/models/artifact.rb create mode 100644 app/policies/artifacts_policy.rb create mode 100644 app/views/v2/artifacts/_artifact.json.jbuilder create mode 100644 app/views/v2/artifacts/create.json.jbuilder create mode 100644 app/views/v2/artifacts/index.json.jbuilder create mode 100644 app/views/v2/artifacts/show.json.jbuilder create mode 100644 db/migrate/20190802204940_create_artifacts.rb create mode 100644 spec/fabricators/artifact_fabricactor.rb create mode 100644 spec/models/artifact_spec.rb diff --git a/app/controllers/v2/artifacts_controller.rb b/app/controllers/v2/artifacts_controller.rb new file mode 100644 index 00000000..6c495876 --- /dev/null +++ b/app/controllers/v2/artifacts_controller.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +module V2 + class ArtifactsController < ResourceController + + collection_policy ArtifactsPolicy + + def self.of_any_type + AnyArtifact.new + end + + def show + @artifact = Artifact.find(params[:id]) + render json: @artifact, status: 200 + end + + def create + collection_policy.new(current_user).authorize! :new? + # We don't explicitly check for :save? permissions + + if duplicate = Artifact.find_by(id: params[:id]) + resource_policy.new(current_user, duplicate).authorize! :show? + head 303, location: v2_artifact_path(duplicate) + else + @artifact = new_artifact(params) + if @artifact.valid? + @artifact.save! + render json: @artifact, status: 201, location: v2_artifact_path(@artifact) + else + render json: @artifact.errors, status: :unprocessable_entity + end + end + end + + private + + def new_artifact(params) + Artifact.new( + id: params[:id], + user: current_user, + format: params[:format], + content_type: params[:content_type] + ) + end + + end +end diff --git a/app/models/artifact.rb b/app/models/artifact.rb new file mode 100644 index 00000000..4ae65817 --- /dev/null +++ b/app/models/artifact.rb @@ -0,0 +1,52 @@ +class Artifact < ApplicationRecord + + class AnyArtifact + def to_resources + Artifact.content_types.map {|t| Checkpoint::Resource::AllOfType.new(t) } + end + + def resource_type + "Artifact" + end + + def resource_id + Checkpoint::Resource::ALL + end + end + + def self.resource_types + content_types + end + + def self.content_types + Rails.application.config.validation["bagger_profile"].keys + + Rails.application.config.validation["external"].keys + end + + def self.of_any_type + AnyArtifact.new + end + + alias_method :identifier, :id + + + # Each artifact belongs to a single user + belongs_to :user + # Deposits are collections of zero or more revisions + has_many :revisions + # Revisions are added to artifacts via deposits + has_many :deposits + + validates :id, presence: true, + format: { with: Services.uuid_format, + message: "must be a valid v4 uuid." } + + validates :user, presence: true + validates :format, presence: true # TODO this is a controlled vocabulary + validates :content_type, presence: true # TODO this is a controlled vocabulary + + def stored? + revisions.any? + end + +end diff --git a/app/policies/artifacts_policy.rb b/app/policies/artifacts_policy.rb new file mode 100644 index 00000000..7f66bd85 --- /dev/null +++ b/app/policies/artifacts_policy.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +class ArtifactsPolicy < CollectionPolicy + def index? + can?(:show, Artifact.of_any_type) + end + + def new? + can?(:save, Artifact.of_any_type) + end + + def base_scope + Artifact.all + end + + def resolve + # Via the role map resolver, a user has access to: + # * All artifacts, if the user is an administrator + # * Artifacts of content types for which the user is a content manager + # * Artifacts of content types for which the user is authorized viewer + # * Any specific artifacts for which the user is granted access + ViewableResources.for(user, scope) + end +end diff --git a/app/views/v2/artifacts/_artifact.json.jbuilder b/app/views/v2/artifacts/_artifact.json.jbuilder new file mode 100644 index 00000000..11194aec --- /dev/null +++ b/app/views/v2/artifacts/_artifact.json.jbuilder @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +# TODO: consider returning paths or urls +json.id artifact.id +json.user artifact.user.username +json.content_type artifact.content_type +json.created_at artifact.created_at.to_formatted_s(:default) +json.updated_at artifact.updated_at.to_formatted_s(:default) diff --git a/app/views/v2/artifacts/create.json.jbuilder b/app/views/v2/artifacts/create.json.jbuilder new file mode 100644 index 00000000..5adbbde3 --- /dev/null +++ b/app/views/v2/artifacts/create.json.jbuilder @@ -0,0 +1,3 @@ +# frozen_string_literal: true + +json.partial! "v2/artifacts/artifact", expand: true, artifact: @artifact diff --git a/app/views/v2/artifacts/index.json.jbuilder b/app/views/v2/artifacts/index.json.jbuilder new file mode 100644 index 00000000..ca0d54c9 --- /dev/null +++ b/app/views/v2/artifacts/index.json.jbuilder @@ -0,0 +1,3 @@ +# frozen_string_literal: true + +json.partial! "v2/artifacts/artifact", expand: false, collection: @artifacts, as: :artifact diff --git a/app/views/v2/artifacts/show.json.jbuilder b/app/views/v2/artifacts/show.json.jbuilder new file mode 100644 index 00000000..5adbbde3 --- /dev/null +++ b/app/views/v2/artifacts/show.json.jbuilder @@ -0,0 +1,3 @@ +# frozen_string_literal: true + +json.partial! "v2/artifacts/artifact", expand: true, artifact: @artifact diff --git a/config/initializers/services.rb b/config/initializers/services.rb index 720a34ad..4acb0a22 100644 --- a/config/initializers/services.rb +++ b/config/initializers/services.rb @@ -79,3 +79,6 @@ def assign_db(lhs, rhs) end Services.register(:notary) { Keycard::Notary.default } +Services.register(:uuid_format) do + /\A[0-9A-F]{8}-[0-9A-F]{4}-4[0-9A-F]{3}-[89AB][0-9A-F]{3}-[0-9A-F]{12}\z/i +end diff --git a/db/migrate/20190802204940_create_artifacts.rb b/db/migrate/20190802204940_create_artifacts.rb new file mode 100644 index 00000000..0e2c79bc --- /dev/null +++ b/db/migrate/20190802204940_create_artifacts.rb @@ -0,0 +1,14 @@ +class CreateArtifacts < ActiveRecord::Migration[5.1] + def change + create_table :artifacts, id: :uuid do |t| + t.string :content_type, null: false + t.string :format, null: false + t.string :storage_volume, null: true + t.references :user, null: false, index: false + t.timestamps + end + + add_index :artifacts, :user_id, unique: false + add_foreign_key :artifacts, :users + end +end diff --git a/spec/fabricators/artifact_fabricactor.rb b/spec/fabricators/artifact_fabricactor.rb new file mode 100644 index 00000000..9a22d6d9 --- /dev/null +++ b/spec/fabricators/artifact_fabricactor.rb @@ -0,0 +1,7 @@ +Fabricator(:artifact) do + id { SecureRandom.uuid } + user { Fabricate(:user) } + format { ["bag", "bag:versioned"].sample } + content_type { ["digital", "audio"].sample } + revisions [] +end diff --git a/spec/models/artifact_spec.rb b/spec/models/artifact_spec.rb new file mode 100644 index 00000000..1a36f97a --- /dev/null +++ b/spec/models/artifact_spec.rb @@ -0,0 +1,42 @@ +require "rails_helper" + +RSpec.describe Artifact, type: :model do + let(:id) { SecureRandom.uuid } + let(:user) { Fabricate.create(:user) } + let(:content_type) { ["digital", "audio"].sample } + let(:format) { ["bag", "bag:versioned"].sample } + + it "has a valid fabricator" do + expect(Fabricate.create(:artifact)).to be_valid + end + + it "has a valid fabriactor that doesn't save to the database" do + expect(Fabricate.build(:artifact)).to be_valid + end + + describe "#id" do + it "must be unique" do + expect do + 2.times { Fabricate(:artifact, id: id) } + end.to raise_error ActiveRecord::RecordNotUnique + end + + it "must be a UUIDv4" do + expect(Fabricate.build(:artifact, id: "foo")).to_not be_valid + end + end + + describe "#stored?" do + it "is false if there are exactly zero revisions" do + expect(Fabricate.build(:artifact, revisions: []).stored?).to be false + end + it "is true if it has 1 or more revisions" do + artifact = Fabricate.build( + :artifact, + revisions: [ Fabricate.build(:revision) ] + ) + expect(artifact.stored?).to be true + end + end + +end From 50c415eca7e134794ab9ae402004737ca342eedf Mon Sep 17 00:00:00 2001 From: Bryan Hockey Date: Thu, 29 Aug 2019 22:43:51 -0500 Subject: [PATCH 06/26] Add routes for v2 deposit scenario --- config/routes.rb | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/config/routes.rb b/config/routes.rb index 566a455e..9739e71e 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -21,6 +21,13 @@ resources :audits, only: [:index, :create, :show] end + namespace :v2 do + resources :artifacts, only: [:index, :show, :create] + resources :deposits, only: [:index, :show, :create] + post "/artifacts/:artifact_id/revisions", controller: :deposits, action: :create + post "/deposits/:id/complete", controller: :deposits, action: :ready + end + get "/login", to: "login#new", as: "login" post "/login", to: "login#create", as: "login_as" match "/logout", to: "login#destroy", as: "logout", via: [:get, :post] @@ -29,7 +36,6 @@ !request.xhr? && request.format.html? end - root to: "application#fallback_index_html", constraints: ->(request) do - !request.xhr? && request.format.html? + root to: "application#fallback_index_html", constraints: ->(request) do !request.xhr? && request.format.html? end end From c19e7b7d102c97f56f7bc861beb38e3901846e2d Mon Sep 17 00:00:00 2001 From: Bryan Hockey Date: Thu, 29 Aug 2019 22:45:14 -0500 Subject: [PATCH 07/26] Add Revision --- app/controllers/v2/revisions_controller.rb | 5 +++++ app/models/revision.rb | 4 ++++ db/migrate/20190813153819_create_revisions.rb | 14 ++++++++++++++ spec/fabricators/revision_fabricator.rb | 4 ++++ spec/models/revision_spec.rb | 12 ++++++++++++ 5 files changed, 39 insertions(+) create mode 100644 app/controllers/v2/revisions_controller.rb create mode 100644 app/models/revision.rb create mode 100644 db/migrate/20190813153819_create_revisions.rb create mode 100644 spec/fabricators/revision_fabricator.rb create mode 100644 spec/models/revision_spec.rb diff --git a/app/controllers/v2/revisions_controller.rb b/app/controllers/v2/revisions_controller.rb new file mode 100644 index 00000000..83fe4e59 --- /dev/null +++ b/app/controllers/v2/revisions_controller.rb @@ -0,0 +1,5 @@ +module V2 + class RevisionsController < ResourceController + + end +end diff --git a/app/models/revision.rb b/app/models/revision.rb new file mode 100644 index 00000000..d7796e4e --- /dev/null +++ b/app/models/revision.rb @@ -0,0 +1,4 @@ +class Revision < ApplicationRecord + belongs_to :artifact + belongs_to :deposit +end diff --git a/db/migrate/20190813153819_create_revisions.rb b/db/migrate/20190813153819_create_revisions.rb new file mode 100644 index 00000000..dcfae8be --- /dev/null +++ b/db/migrate/20190813153819_create_revisions.rb @@ -0,0 +1,14 @@ +class CreateRevisions < ActiveRecord::Migration[5.1] + def change + create_table :revisions do |t| + t.string :artifact_id, null: false, index: false + t.belongs_to :deposit, null: false, index: false + t.timestamps + end + + add_index :revisions, :artifact_id, unique: false + add_index :revisions, :deposit_id, unique: true + add_foreign_key :revisions, :artifacts + add_foreign_key :revisions, :deposits + end +end diff --git a/spec/fabricators/revision_fabricator.rb b/spec/fabricators/revision_fabricator.rb new file mode 100644 index 00000000..63bb5545 --- /dev/null +++ b/spec/fabricators/revision_fabricator.rb @@ -0,0 +1,4 @@ +Fabricator(:revision) do + artifact + deposit +end diff --git a/spec/models/revision_spec.rb b/spec/models/revision_spec.rb new file mode 100644 index 00000000..c3cd84f8 --- /dev/null +++ b/spec/models/revision_spec.rb @@ -0,0 +1,12 @@ +require "rails_helper" + +RSpec.describe Revision, type: :model do + it "has a valid fabricator" do + expect(Fabricate.create(:revision)).to be_valid + end + + it "has a valid fabriactor that doesn't save to the database" do + expect(Fabricate.build(:revision)).to be_valid + end + +end From 7a2dc5acce65d805af2787fc60e757ecbc4f8353 Mon Sep 17 00:00:00 2001 From: Bryan Hockey Date: Thu, 29 Aug 2019 22:46:08 -0500 Subject: [PATCH 08/26] Add Deposit --- app/controllers/v2/deposits_controller.rb | 31 +++++++++++++ app/models/deposit.rb | 49 ++++++++++++++++++++ app/views/v2/deposits/_deposit.json.jbuilder | 10 ++++ app/views/v2/deposits/create.json.jbuilder | 3 ++ app/views/v2/deposits/index.json.jbuilder | 3 ++ app/views/v2/deposits/show.json.jbuilder | 3 ++ db/migrate/20190807173150_create_deposits.rb | 15 ++++++ spec/fabricators/deposit_fabricator.rb | 5 ++ spec/models/deposit_spec.rb | 41 ++++++++++++++++ 9 files changed, 160 insertions(+) create mode 100644 app/controllers/v2/deposits_controller.rb create mode 100644 app/models/deposit.rb create mode 100644 app/views/v2/deposits/_deposit.json.jbuilder create mode 100644 app/views/v2/deposits/create.json.jbuilder create mode 100644 app/views/v2/deposits/index.json.jbuilder create mode 100644 app/views/v2/deposits/show.json.jbuilder create mode 100644 db/migrate/20190807173150_create_deposits.rb create mode 100644 spec/fabricators/deposit_fabricator.rb create mode 100644 spec/models/deposit_spec.rb diff --git a/app/controllers/v2/deposits_controller.rb b/app/controllers/v2/deposits_controller.rb new file mode 100644 index 00000000..b8fe3e09 --- /dev/null +++ b/app/controllers/v2/deposits_controller.rb @@ -0,0 +1,31 @@ +module V2 + class DepositsController < ResourceController + + def create + # TODO policy check + @deposit = Deposit.create!( + artifact: Artifact.find(params[:artifact_id]), + user: current_user, + status: :started + ) + # TODO: don't pick a specific template + render "v2/deposits/show", status: 201, location: v2_deposit_path(@deposit) + end + + def ready + # TODO policy check + @deposit = Deposit.find(params[:id]) + case @deposit.status + when Deposit.statuses[:started] + @deposit.update!(status: Deposit.statuses[:ingesting]) + FinishDepositJob.perform_later(@deposit) + render json: @deposit, status: 200 + when Deposit.statuses[:ingesting] + render json: @deposit, status: 200 + else # :completed, :failed, :cancelled + head 422 # TODO think about this response + end + end + + end +end diff --git a/app/models/deposit.rb b/app/models/deposit.rb new file mode 100644 index 00000000..4f16e657 --- /dev/null +++ b/app/models/deposit.rb @@ -0,0 +1,49 @@ +class Deposit < ApplicationRecord + + # Each deposit is created by a single user + belongs_to :user + # A deposit is an attempt to append a revision to a single artifact + belongs_to :artifact + + # TODO Could not get the attributes api to work with a custom type + enum status: { + started: "started", + canceled: "cancelled", + ingesting: "ingesting", + failed: "failed", + completed: "completed" + } + + validates :user, presence: true + validates :artifact, presence: true + validates :status, presence: true # TODO this is a controlled vocabulary + + def identifier + id.to_s + end + + def username + user.username + end + + def content_type + artifact.content_type + end + + def storage_format + artifact.storage_format + end + + def complete! + update!(status: "completed") + end + + def fail!(errors) + update!(status: "failed", error: errors.join("\n")) + end + + def upload_link + Services.incoming_storage.upload_link(self) + end + +end diff --git a/app/views/v2/deposits/_deposit.json.jbuilder b/app/views/v2/deposits/_deposit.json.jbuilder new file mode 100644 index 00000000..d06ea938 --- /dev/null +++ b/app/views/v2/deposits/_deposit.json.jbuilder @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +# TODO: consider returning paths or urls +json.id deposit.id +json.artifact deposit.artifact_id +json.user deposit.user.username +json.status deposit.status +json.upload_link deposit.upload_link +json.created_at deposit.created_at.to_formatted_s(:default) +json.updated_at deposit.updated_at.to_formatted_s(:default) diff --git a/app/views/v2/deposits/create.json.jbuilder b/app/views/v2/deposits/create.json.jbuilder new file mode 100644 index 00000000..ed993be4 --- /dev/null +++ b/app/views/v2/deposits/create.json.jbuilder @@ -0,0 +1,3 @@ +# frozen_string_literal: true + +json.partial! "v2/deposits/deposit", expand: true, deposit: @deposit diff --git a/app/views/v2/deposits/index.json.jbuilder b/app/views/v2/deposits/index.json.jbuilder new file mode 100644 index 00000000..f8bffb12 --- /dev/null +++ b/app/views/v2/deposits/index.json.jbuilder @@ -0,0 +1,3 @@ +# frozen_string_literal: true + +json.partial! "v2/deposits/deposit", expand: false, collection: @deposits, as: :deposit diff --git a/app/views/v2/deposits/show.json.jbuilder b/app/views/v2/deposits/show.json.jbuilder new file mode 100644 index 00000000..ed993be4 --- /dev/null +++ b/app/views/v2/deposits/show.json.jbuilder @@ -0,0 +1,3 @@ +# frozen_string_literal: true + +json.partial! "v2/deposits/deposit", expand: true, deposit: @deposit diff --git a/db/migrate/20190807173150_create_deposits.rb b/db/migrate/20190807173150_create_deposits.rb new file mode 100644 index 00000000..ea698aac --- /dev/null +++ b/db/migrate/20190807173150_create_deposits.rb @@ -0,0 +1,15 @@ +class CreateDeposits < ActiveRecord::Migration[5.1] + def change + create_table :deposits do |t| + t.string :artifact_id, null: false, index: false + t.belongs_to :user, null: false, index: false + t.string :status, null: false + t.text :error, null: false, default: "" + t.timestamps + end + + add_index :deposits, :user_id, unique: false + add_foreign_key :deposits, :artifacts + add_foreign_key :deposits, :users + end +end diff --git a/spec/fabricators/deposit_fabricator.rb b/spec/fabricators/deposit_fabricator.rb new file mode 100644 index 00000000..970608d6 --- /dev/null +++ b/spec/fabricators/deposit_fabricator.rb @@ -0,0 +1,5 @@ +Fabricator(:deposit) do + user + artifact + status { Deposit.statuses[:started] } +end diff --git a/spec/models/deposit_spec.rb b/spec/models/deposit_spec.rb new file mode 100644 index 00000000..7bd33f0f --- /dev/null +++ b/spec/models/deposit_spec.rb @@ -0,0 +1,41 @@ +require "rails_helper" + +RSpec.describe Deposit, type: :model do + it "has a valid fabricator" do + expect(Fabricate.create(:deposit)).to be_valid + end + + it "has a valid fabriactor that doesn't save to the database" do + expect(Fabricate.build(:deposit)).to be_valid + end + + describe "#storage_format" do + let(:deposit) { Fabricate.build(:deposit) } + + it "uses the artifact's storage_format" do + expect(deposit.storage_format).to eql(deposit.artifact.storage_format) + end + end + + describe "#fail!" do + let(:deposit) { Fabricate.create(:deposit) } + let(:error) { ["some", "errors"] } + + it "updates with status failed" do + expect { deposit.fail!(error) } + .to change(deposit, :status) + .to("failed") + end + + it "updates with the error" do + expect { deposit.fail!(error) } + .to change(deposit, :error) + .to("some\nerrors") + end + + it "saves the record" do + deposit.fail!(error) + expect(deposit.changed?).to be false + end + end +end From 45d71bce5a104158ddfe2631128e0c87e7ede570 Mon Sep 17 00:00:00 2001 From: Bryan Hockey Date: Thu, 15 Aug 2019 10:36:32 -0500 Subject: [PATCH 09/26] Include broken DepositStatus The thought was to add DepositStatus as a PORO, and use it via the Rails attributes API and app/attributse/DepositStatusType, but I couldn't get it to work. --- app/attributes/deposit_status_type.rb | 27 ++++++++++++ lib/chipmunk.rb | 1 + lib/chipmunk/deposit_status.rb | 61 +++++++++++++++++++++++++++ 3 files changed, 89 insertions(+) create mode 100644 app/attributes/deposit_status_type.rb create mode 100644 lib/chipmunk/deposit_status.rb diff --git a/app/attributes/deposit_status_type.rb b/app/attributes/deposit_status_type.rb new file mode 100644 index 00000000..9adab796 --- /dev/null +++ b/app/attributes/deposit_status_type.rb @@ -0,0 +1,27 @@ +class DepositStatusType < ActiveRecord::Type::Value + def cast(value) + puts "#cast(#{value} #[#{value.class}]) from #{caller[0]}" + # if value.is_a? Chipmunk::DepositStatus + # value + # else + # Chipmunk::DepositStatus.new(value) + # end + super(value.to_s) + end + + # this is the default implementation + # def deserialize(value_from_db) + # cast(value_from_db) + # end + def deserialize(value) + puts "#deserialize(#{value} #[#{value.class}]) from #{caller[0]}" + Chipmunk::DepositStatus.new(value) + end + + # So this just doesn't get fucking called + def serialize(deposit_status) + puts "#serialize(#{deposit_status} #[#{deposit_status.class}]) from #{caller[0]}" + deposit_status.to_s + end + +end diff --git a/lib/chipmunk.rb b/lib/chipmunk.rb index 896cfff7..d30627be 100644 --- a/lib/chipmunk.rb +++ b/lib/chipmunk.rb @@ -9,6 +9,7 @@ module Chipmunk require_relative "chipmunk/validatable" require_relative "chipmunk/bag" +require_relative "chipmunk/deposit_status" require_relative "chipmunk/resolvers" require_relative "chipmunk/incoming_storage" require_relative "chipmunk/package_storage" diff --git a/lib/chipmunk/deposit_status.rb b/lib/chipmunk/deposit_status.rb new file mode 100644 index 00000000..71efc332 --- /dev/null +++ b/lib/chipmunk/deposit_status.rb @@ -0,0 +1,61 @@ +module Chipmunk + + class DepositStatus + + VALUES = [ + :started, + :canceled, + :ingesting, + :failed, + :completed + ].freeze + + # Define a constructor for each value, e.g. DepositStatus.started + class << self + VALUES.each do |value| + define_method(value) do + new(value) + end + end + end + + # Define a predicate method #value? for each value, e.g. #completed? + VALUES.each do |value| + define_method(:"#{value}?") do + self.value == value + end + end + + def initialize(value) + @value = value.to_sym + unless VALUES.include?(@value) + raise ArgumentError, "Invalid value #{value}, expected one of #{VALUES}" + end + end + + def eql?(other) + value == other.value + end + + def to_s + value.to_s + end + + def to_sym + value + end + + def alive? + started? || ingesting? + end + + def dead? + !alive? + end + + private + attr_reader :value + + end + +end From 8eb271f965d6cf2487f0752315d44f3251fcea20 Mon Sep 17 00:00:00 2001 From: Bryan Hockey Date: Thu, 29 Aug 2019 22:46:26 -0500 Subject: [PATCH 10/26] dump schema --- db/schema.rb | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/db/schema.rb b/db/schema.rb index ba5d20d1..fac2936d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,10 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20190531175502) do +ActiveRecord::Schema.define(version: 20190813153819) do + +# Could not dump table "artifacts" because of following StandardError +# Unknown type 'uuid' for column 'id' create_table "audits", force: :cascade do |t| t.integer "user_id" @@ -20,6 +23,15 @@ t.index ["user_id"], name: "index_audits_on_user_id" end + create_table "deposits", force: :cascade do |t| + t.string "artifact_id", null: false + t.integer "user_id", null: false + t.string "status", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["user_id"], name: "index_deposits_on_user_id" + end + create_table "events", force: :cascade do |t| t.integer "package_id" t.string "event_type" @@ -58,6 +70,15 @@ t.index ["package_id"], name: "index_queue_items_on_package_id" end + create_table "revisions", force: :cascade do |t| + t.string "artifact_id", null: false + t.integer "deposit_id", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["artifact_id"], name: "index_revisions_on_artifact_id" + t.index ["deposit_id"], name: "index_revisions_on_deposit_id", unique: true + end + create_table "users", force: :cascade do |t| t.string "username", null: false t.string "email", null: false From 64d111423148e40cbbfedcbeaf732a0d0cb4b00a Mon Sep 17 00:00:00 2001 From: Bryan Hockey Date: Mon, 12 Aug 2019 02:41:02 -0500 Subject: [PATCH 11/26] Add Package#identifier #to_param alias --- app/models/package.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/package.rb b/app/models/package.rb index b04bfecd..6e3b3303 100644 --- a/app/models/package.rb +++ b/app/models/package.rb @@ -14,6 +14,7 @@ class Package < ApplicationRecord def to_param bag_id end + alias_method :identifier, :to_param validates :bag_id, presence: true, length: { minimum: 6 } validates :user_id, presence: true From 61bbef1119a9cab659287271cb8e47495ebb3c46 Mon Sep 17 00:00:00 2001 From: Bryan Hockey Date: Mon, 12 Aug 2019 15:32:51 -0500 Subject: [PATCH 12/26] UploadPath, UserUploadPath rely on #identifier --- lib/chipmunk/upload_path.rb | 2 +- lib/chipmunk/user_upload_path.rb | 2 +- spec/chipmunk/incoming_storage_spec.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/chipmunk/upload_path.rb b/lib/chipmunk/upload_path.rb index b4641c86..71da7a08 100644 --- a/lib/chipmunk/upload_path.rb +++ b/lib/chipmunk/upload_path.rb @@ -12,7 +12,7 @@ def initialize(root_path) end def for(package) - File.join(root_path, package.bag_id) + File.join(root_path, package.identifier) end private diff --git a/lib/chipmunk/user_upload_path.rb b/lib/chipmunk/user_upload_path.rb index 1b15c462..e742c654 100644 --- a/lib/chipmunk/user_upload_path.rb +++ b/lib/chipmunk/user_upload_path.rb @@ -12,7 +12,7 @@ def initialize(root_path) end def for(package) - File.join(root_path, package.user.username, package.bag_id) + File.join(root_path, package.user.username, package.identifier) end private diff --git a/spec/chipmunk/incoming_storage_spec.rb b/spec/chipmunk/incoming_storage_spec.rb index 78e23996..2812c945 100644 --- a/spec/chipmunk/incoming_storage_spec.rb +++ b/spec/chipmunk/incoming_storage_spec.rb @@ -5,7 +5,7 @@ let(:volume) { Chipmunk::Volume.new(name: "incoming", package_type: package_type, root_path: "/incoming") } let(:path_builder) { Chipmunk::UploadPath.new("/") } let(:uploader) { instance_double("User", username: "uploader") } - let(:unstored_package) { instance_double("Package", stored?: false, user: uploader, bag_id: "abcdef-123456") } + let(:unstored_package) { instance_double("Package", stored?: false, user: uploader, bag_id: "abcdef-123456", identifier: "abcdef-123456") } let(:stored_package) { instance_double("Package", stored?: true) } subject(:storage) do From 8fc0c6f8cbda266ad79e9b113ccad8206d5affeb Mon Sep 17 00:00:00 2001 From: Bryan Hockey Date: Thu, 8 Aug 2019 23:04:52 -0500 Subject: [PATCH 13/26] Use reader and writer objects in Volume * Introduces the concept of a Reader, used to load instances from disk. This immediately replaces Volume's call to Bag.new with delegation to the volume's reader, an instance of Bag::Reader This conceptoffers a point from which we can more effectively handle different types of stored objects. * Similarly, we introduce the concept of a Writer, implement Bag::MoveWriter, and use it in our volumes. The main driver here is to have the ability to read and write v1 bags, versioned bags, and in the future OCFL objects in a consistent way. This allows us to use PackageStorage for all of these object types, avoiding having something like RevisionStorage and OCFLStorage. This approach was chosen because the responsibility for simply writing the object felt like it belonged in Volume, and so we inject it as a dependency. Meanwhile, the high-level rules around writing, especially where it fits in the lifecycle, belong to PackageStorage. --- config/initializers/services.rb | 34 +++++++++--- lib/chipmunk/bag.rb | 2 + lib/chipmunk/bag/move_writer.rb | 13 +++++ lib/chipmunk/bag/reader.rb | 13 +++++ lib/chipmunk/package_storage.rb | 31 ++++++----- lib/chipmunk/volume.rb | 20 ++++--- spec/chipmunk/bag/move_writer_spec.rb | 21 ++++++++ spec/chipmunk/bag_spec.rb | 7 +++ spec/chipmunk/incoming_storage_spec.rb | 5 +- spec/chipmunk/package_storage_spec.rb | 74 ++++++++++++++++++++------ spec/chipmunk/volume_spec.rb | 18 +++++-- 11 files changed, 187 insertions(+), 51 deletions(-) create mode 100644 lib/chipmunk/bag/move_writer.rb create mode 100644 lib/chipmunk/bag/reader.rb create mode 100644 spec/chipmunk/bag/move_writer_spec.rb diff --git a/config/initializers/services.rb b/config/initializers/services.rb index 4acb0a22..efe66863 100644 --- a/config/initializers/services.rb +++ b/config/initializers/services.rb @@ -39,7 +39,8 @@ def assign_db(lhs, rhs) Chipmunk::IncomingStorage.new( volume: Chipmunk::Volume.new( name: "incoming", - package_type: Chipmunk::Bag, + reader: Chipmunk::Bag::Reader.new, + writer: Chipmunk::Bag::MoveWriter.new, root_path: Chipmunk.config.upload.upload_path ), paths: Chipmunk::UploadPath.new("/"), @@ -48,8 +49,18 @@ def assign_db(lhs, rhs) end Services.register(:storage) do Chipmunk::PackageStorage.new(volumes: [ - Chipmunk::Volume.new(name: "test", package_type: Chipmunk::Bag, root_path: Rails.root.join("spec/support/fixtures")), - Chipmunk::Volume.new(name: "bags", package_type: Chipmunk::Bag, root_path: Chipmunk.config.upload.storage_path) + Chipmunk::Volume.new( + name: "test", + reader: Chipmunk::Bag::Reader.new, + writer: Chipmunk::Bag::MoveWriter.new, + root_path: Rails.root.join("spec/support/fixtures") + ), + Chipmunk::Volume.new( + name: "bags", + reader: Chipmunk::Bag::Reader.new, + writer: Chipmunk::Bag::MoveWriter.new, + root_path: Chipmunk.config.upload.storage_path + ) ]) end else @@ -57,7 +68,8 @@ def assign_db(lhs, rhs) Chipmunk::IncomingStorage.new( volume: Chipmunk::Volume.new( name: "incoming", - package_type: Chipmunk::Bag, + reader: Chipmunk::Bag::Reader.new, + writer: Chipmunk::Bag::MoveWriter.new, root_path: Chipmunk.config.upload.upload_path ), paths: Chipmunk::UserUploadPath.new("/"), @@ -66,8 +78,18 @@ def assign_db(lhs, rhs) end Services.register(:storage) do Chipmunk::PackageStorage.new(volumes: [ - Chipmunk::Volume.new(name: "root", package_type: Chipmunk::Bag, root_path: "/"), # For migration purposes - Chipmunk::Volume.new(name: "bags", package_type: Chipmunk::Bag, root_path: Chipmunk.config.upload.storage_path) + Chipmunk::Volume.new( + name: "root", + reader: Chipmunk::Bag::Reader.new, + writer: Chipmunk::Bag::MoveWriter.new, + root_path: "/" # For migration purposes + ), + Chipmunk::Volume.new( + name: "bags", + reader: Chipmunk::Bag::Reader.new, + writer: Chipmunk::Bag::MoveWriter.new, + root_path: Chipmunk.config.upload.storage_path + ) ]) end end diff --git a/lib/chipmunk/bag.rb b/lib/chipmunk/bag.rb index c469190a..ee7af33c 100644 --- a/lib/chipmunk/bag.rb +++ b/lib/chipmunk/bag.rb @@ -118,5 +118,7 @@ def respond_to_missing?(name, include_private = false) end require_relative "bag/profile" +require_relative "bag/reader" +require_relative "bag/move_writer" require_relative "bag/tag" require_relative "bag/validator" diff --git a/lib/chipmunk/bag/move_writer.rb b/lib/chipmunk/bag/move_writer.rb new file mode 100644 index 00000000..ebff00d0 --- /dev/null +++ b/lib/chipmunk/bag/move_writer.rb @@ -0,0 +1,13 @@ +module Chipmunk + class Bag + + # Writes a bag by moving the source via file rename. + class MoveWriter + def write(bag, path) + FileUtils.mkdir_p(path) + File.rename(bag.path, path) + end + end + + end +end diff --git a/lib/chipmunk/bag/reader.rb b/lib/chipmunk/bag/reader.rb new file mode 100644 index 00000000..35782931 --- /dev/null +++ b/lib/chipmunk/bag/reader.rb @@ -0,0 +1,13 @@ +module Chipmunk + class Bag + class Reader + def at(path) + Chipmunk::Bag.new(path) + end + + def format + Bag.format + end + end + end +end diff --git a/lib/chipmunk/package_storage.rb b/lib/chipmunk/package_storage.rb index 48c9355d..8e309248 100644 --- a/lib/chipmunk/package_storage.rb +++ b/lib/chipmunk/package_storage.rb @@ -26,28 +26,27 @@ def for(package) # Move the source archive into preservation storage and update the package's # storage_volume and storage_path accordingly. def write(package, source) - if package.format == Chipmunk::Bag.format - move_bag(package, source) - else - raise Chipmunk::UnsupportedFormatError, "Package #{package.bag_id} has invalid format: #{package.format}" - end + check_format!(package) + storage_path = storage_path_for(package) + volume = destination_volume(package) + volume.write(source, storage_path) + package.update(storage_volume: volume.name, storage_path: storage_path) end private - def move_bag(package, source) - bag_id = package.bag_id - prefixes = bag_id.match(/^(..)(..)(..).*/) - raise "bag_id too short: #{bag_id}" unless prefixes - - storage_path = File.join("/", prefixes[1..3], bag_id) - volume = destination_volume(package) - dest_path = volume.expand(storage_path) + def check_format!(package) + unless package.format == Chipmunk::Bag.format + raise Chipmunk::UnsupportedFormatError, + "Package #{package.identifier} has invalid format: #{package.format}" + end + end - FileUtils.mkdir_p(dest_path) - File.rename(source.path, dest_path) + def storage_path_for(package) + prefixes = package.identifier.match(/^(..)(..)(..).*/) + raise "identifier too short: #{package.identifier}" unless prefixes - package.update(storage_volume: volume.name, storage_path: storage_path) + File.join("/", prefixes[1..3], package.identifier) end # We are defaulting everything to "bags" for now as the simplest resolution strategy. diff --git a/lib/chipmunk/volume.rb b/lib/chipmunk/volume.rb index 33831129..a211751f 100644 --- a/lib/chipmunk/volume.rb +++ b/lib/chipmunk/volume.rb @@ -15,14 +15,16 @@ class Volume # Create a new Volume. # # @param name [String] the name of the Volume; coerced to String - # @param package_type [Class] the storage class for this volume (Chipmunk::Bag) + # @param reader [Bag::Reader] A reader that can return the an instance of the + # stored object given a path. # @param root_path [String|Pathname] the path to the storage root for this Volume; # must be absolute; coerced to Pathname # @raise [ArgumentError] if the name is blank or root_path is relative - def initialize(name:, package_type:, root_path:) + def initialize(name:, root_path:, reader: Chipmunk::Bag::Reader.new, writer:) @name = name.to_s - @package_type = package_type @root_path = Pathname(root_path) + @reader = reader + @writer = writer validate! end @@ -47,7 +49,11 @@ def expand(path) def get(path) raise Chipmunk::PackageNotFoundError unless include?(path) - package_type.new(expand(path)) + reader.at(expand(path)) + end + + def write(object, path) + writer.write(object, expand(path)) end def include?(path) @@ -57,7 +63,7 @@ def include?(path) # @!attribute [r] # @return [String] the format name of the items in this volume def format - package_type.format + reader.format end private @@ -66,6 +72,8 @@ def validate! raise ArgumentError, "Volume name must not be blank" if name.strip.empty? raise ArgumentError, "Volume format must not be blank" if format.to_s.strip.empty? raise ArgumentError, "Volume root_path must be absolute (#{root_path})" unless root_path.absolute? + raise ArgumentError, "Volume must specify a reader" unless reader + raise ArgumentError, "Volume must specify a writer" unless writer end # Remove any leading slashes so Pathname joins properly @@ -73,6 +81,6 @@ def trim(path) path.to_s.sub(/^\/*/, "") end - attr_reader :package_type + attr_reader :reader, :writer end end diff --git a/spec/chipmunk/bag/move_writer_spec.rb b/spec/chipmunk/bag/move_writer_spec.rb new file mode 100644 index 00000000..e0f7714f --- /dev/null +++ b/spec/chipmunk/bag/move_writer_spec.rb @@ -0,0 +1,21 @@ +# These tests were ported over from PackageStorage's specs. +RSpec.describe Chipmunk::Bag::MoveWriter do + let(:bag) { double(:bag, path: "/some/bag/path") } + let(:path) { "/bag/goes/in/here" } + subject(:writer) { described_class.new } + + before(:each) do + allow(FileUtils).to receive(:mkdir_p) + allow(File).to receive(:rename) + end + + it "ensures the desination directory exists" do + expect(FileUtils).to receive(:mkdir_p).with(path) + writer.write(bag, path) + end + + it "moves the source to the destination directory" do + expect(File).to receive(:rename).with(bag.path, path) + writer.write(bag, path) + end +end diff --git a/spec/chipmunk/bag_spec.rb b/spec/chipmunk/bag_spec.rb index 8e8b9695..53cf7684 100644 --- a/spec/chipmunk/bag_spec.rb +++ b/spec/chipmunk/bag_spec.rb @@ -35,6 +35,13 @@ end end + describe "#format" do + it "has the same format as the class" do + expect(stored_bag.format).to eql(described_class.format) + expect(stored_bag.format).to eql("bag") + end + end + describe "#id" do it "returns the Bag-ID from the chipmunk-info.txt" do expect(stored_bag.id).to eql("14d25bcd-deaf-4c94-add7-c189fdca4692") diff --git a/spec/chipmunk/incoming_storage_spec.rb b/spec/chipmunk/incoming_storage_spec.rb index 2812c945..efa9d878 100644 --- a/spec/chipmunk/incoming_storage_spec.rb +++ b/spec/chipmunk/incoming_storage_spec.rb @@ -1,8 +1,9 @@ # frozen_string_literal: true RSpec.describe Chipmunk::IncomingStorage do - let(:package_type) { double("SomePackageFormat", format: "some-pkg") } - let(:volume) { Chipmunk::Volume.new(name: "incoming", package_type: package_type, root_path: "/incoming") } + let(:reader) { double(:reader, format: "some-pkg") } + let(:writer) { double(:writer, format: "some-pkg") } + let(:volume) { Chipmunk::Volume.new(name: "incoming", writer: writer, reader: reader, root_path: "/incoming") } let(:path_builder) { Chipmunk::UploadPath.new("/") } let(:uploader) { instance_double("User", username: "uploader") } let(:unstored_package) { instance_double("Package", stored?: false, user: uploader, bag_id: "abcdef-123456", identifier: "abcdef-123456") } diff --git a/spec/chipmunk/package_storage_spec.rb b/spec/chipmunk/package_storage_spec.rb index ee46fd07..3f1e3f23 100644 --- a/spec/chipmunk/package_storage_spec.rb +++ b/spec/chipmunk/package_storage_spec.rb @@ -13,6 +13,33 @@ def self.format end end + class FakeBagReader + def format + "bag" + end + + def at(path) + FakeBag.new(path) + end + end + + class FakeZipReader + def format + "zip" + end + + def at(path) + FakeZip.new(path) + end + end + + class FakeBagWriter + def write(_obj, _path) + nil + end + end + FakeZipWriter = FakeBagWriter + # TODO: Set up a "test context" that has realistic, but test-focused # services registered. This will offload setup of the environment from # various tests. This may be as simple as registering test components over @@ -22,8 +49,23 @@ def self.format # inclusion of specific contexts in tests that need them. In this group, # the volumes and volume manager can be considered environmental, while the # packages are scenario data. - let(:bags) { Chipmunk::Volume.new(name: "bags", package_type: FakeBag, root_path: "/bags") } - let(:zips) { Chipmunk::Volume.new(name: "zips", package_type: FakeZip, root_path: "/zips") } + let(:bags) do + Chipmunk::Volume.new( + name: "bags", + root_path: "/bags", + reader: FakeBagReader.new, + writer: FakeBagWriter.new + ) + end + + let(:zips) do + Chipmunk::Volume.new( + name: "zips", + root_path: "/zips", + reader: FakeZipReader.new, + writer: FakeZipWriter.new + ) + end before(:each) do allow(bags).to receive(:include?).with("/a-bag").and_return true @@ -71,21 +113,19 @@ def self.format context "with a good bag" do subject(:storage) { described_class.new(volumes: [bags]) } - let(:package) { spy(:package, format: "bag", bag_id: "abcdef-123456") } + let(:package) { spy(:package, format: "bag", identifier: "abcdef-123456") } let(:disk_bag) { double(:bag, path: "/uploaded/abcdef-123456") } - before(:each) do - allow(FileUtils).to receive(:mkdir_p).with("/bags/ab/cd/ef/abcdef-123456") - allow(File).to receive(:rename).with("/uploaded/abcdef-123456", "/bags/ab/cd/ef/abcdef-123456") - end - - it "ensures the destination directory exists" do - expect(FileUtils).to receive(:mkdir_p) - storage.write(package, disk_bag) - end - it "moves the source bag to the destination directory" do - expect(File).to receive(:rename) + # TODO: This test is probably less specific than originally intended; setting + # an expection also adds an implicit allow(...) for the expectation. Here, + # originally it just expected File.rename to be called, regardless of args. + # Indeed, there seems to be a bit of a mismatch with these arguments across + # the few files concerned with it; somtimes they're bags, sometimes they're + # paths. We should be careful to make that clear so no issues get past + # testing. For the time being, I have left the expectation with the original + # specificity. + expect(bags).to receive(:write) storage.write(package, disk_bag) end @@ -104,7 +144,7 @@ def self.format context "with a badly identified bag (shorter than 6 chars)" do subject(:storage) { described_class.new(volumes: [bags]) } - let(:package) { double(:package, format: "bag", bag_id: "ab12") } + let(:package) { double(:package, format: "bag", identifier: "ab12") } let(:disk_bag) { double(:bag, path: "/uploaded/ab12") } it "raises an exception" do @@ -115,7 +155,7 @@ def self.format context "with an unsupported archive format" do subject(:storage) { described_class.new(volumes: [bags]) } - let(:package) { double(:package, format: "junk", bag_id: "id") } + let(:package) { double(:package, format: "junk", identifier: "id") } let(:archive) { double(:archive) } it "raises an Unsupported Format error" do @@ -129,6 +169,6 @@ def stored_package(format:, storage_volume: "test", storage_path: "/path") end def unstored_package(format:, id:) - double(:package, stored?: false, storage_volume: nil, storage_path: nil, format: format, bag_id: id) + double(:package, stored?: false, storage_volume: nil, storage_path: nil, format: format, identifier: id) end end diff --git a/spec/chipmunk/volume_spec.rb b/spec/chipmunk/volume_spec.rb index 0da68047..130c3933 100644 --- a/spec/chipmunk/volume_spec.rb +++ b/spec/chipmunk/volume_spec.rb @@ -3,12 +3,20 @@ require "pathname" RSpec.describe Chipmunk::Volume do - subject(:volume) { described_class.new(name: name, package_type: package_type, root_path: root_path) } + subject(:volume) do + described_class.new( + name: name, + root_path: root_path, + reader: reader, + writer: writer + ) + end context "when given valid attributes" do let(:name) { "vol" } let(:proxy) { double(:storage_proxy) } - let(:package_type) { double("SomeFormat", new: proxy, format: "some-pkg") } + let(:reader) { double(:reader, at: proxy, format: "some-pkg") } + let(:writer) { double(:writer, write: nil, format: "some-pkg") } let(:root_path) { "/path" } it "has the correct name" do @@ -57,7 +65,8 @@ context "when given a blank name" do let(:name) { "" } - let(:package_type) { double("SomeFormat", new: nil, format: "some-pkg") } + let(:reader) { double(:reader, at: nil, format: "some-pkg") } + let(:writer) { double(:writer, write: nil, format: "some-pkg") } let(:root_path) { "/path" } it "raises an argument error that the name must not be blank" do @@ -67,7 +76,8 @@ context "when given a relative path" do let(:name) { "vol" } - let(:package_type) { double("SomeFormat", new: nil, format: "some-pkg") } + let(:reader) { double(:reader, at: nil, format: "some-pkg") } + let(:writer) { double(:writer, write: nil, format: "some-pkg") } let(:root_path) { "relative/path" } it "raises an argument error that path must be absolute" do From ca765381cb2fdb36bc46bcf111a4546f1d05a125 Mon Sep 17 00:00:00 2001 From: Bryan Hockey Date: Thu, 15 Aug 2019 13:01:53 -0500 Subject: [PATCH 14/26] Move test Services config to rails_helper --- config/initializers/services.rb | 87 +++++++++++---------------------- spec/rails_helper.rb | 30 ++++++++++++ 2 files changed, 58 insertions(+), 59 deletions(-) diff --git a/config/initializers/services.rb b/config/initializers/services.rb index efe66863..59595837 100644 --- a/config/initializers/services.rb +++ b/config/initializers/services.rb @@ -31,67 +31,36 @@ def assign_db(lhs, rhs) end end -Services = Canister.new # TODO: consult the environment-specific configuration for a set of volumes -# TODO: Separate normal and test contexts -if Rails.env.test? - Services.register(:incoming_storage) do - Chipmunk::IncomingStorage.new( - volume: Chipmunk::Volume.new( - name: "incoming", - reader: Chipmunk::Bag::Reader.new, - writer: Chipmunk::Bag::MoveWriter.new, - root_path: Chipmunk.config.upload.upload_path - ), - paths: Chipmunk::UploadPath.new("/"), - links: Chipmunk::UploadPath.new(Chipmunk.config.upload["rsync_point"]) - ) - end - Services.register(:storage) do - Chipmunk::PackageStorage.new(volumes: [ - Chipmunk::Volume.new( - name: "test", - reader: Chipmunk::Bag::Reader.new, - writer: Chipmunk::Bag::MoveWriter.new, - root_path: Rails.root.join("spec/support/fixtures") - ), - Chipmunk::Volume.new( - name: "bags", - reader: Chipmunk::Bag::Reader.new, - writer: Chipmunk::Bag::MoveWriter.new, - root_path: Chipmunk.config.upload.storage_path - ) - ]) - end -else - Services.register(:incoming_storage) do - Chipmunk::IncomingStorage.new( - volume: Chipmunk::Volume.new( - name: "incoming", - reader: Chipmunk::Bag::Reader.new, - writer: Chipmunk::Bag::MoveWriter.new, - root_path: Chipmunk.config.upload.upload_path - ), - paths: Chipmunk::UserUploadPath.new("/"), - links: Chipmunk::UploadPath.new(Chipmunk.config.upload["rsync_point"]) +Services = Canister.new +Services.register(:incoming_storage) do + Chipmunk::IncomingStorage.new( + volume: Chipmunk::Volume.new( + name: "incoming", + reader: Chipmunk::Bag::Reader.new, + writer: Chipmunk::Bag::MoveWriter.new, + root_path: Chipmunk.config.upload.upload_path + ), + paths: Chipmunk::UserUploadPath.new("/"), + links: Chipmunk::UploadPath.new(Chipmunk.config.upload["rsync_point"]) + ) +end + +Services.register(:storage) do + Chipmunk::PackageStorage.new(volumes: [ + Chipmunk::Volume.new( + name: "root", + reader: Chipmunk::Bag::Reader.new, + writer: Chipmunk::Bag::MoveWriter.new, + root_path: "/" # For migration purposes + ), + Chipmunk::Volume.new( + name: "bags", + reader: Chipmunk::Bag::Reader.new, + writer: Chipmunk::Bag::MoveWriter.new, + root_path: Chipmunk.config.upload.storage_path ) - end - Services.register(:storage) do - Chipmunk::PackageStorage.new(volumes: [ - Chipmunk::Volume.new( - name: "root", - reader: Chipmunk::Bag::Reader.new, - writer: Chipmunk::Bag::MoveWriter.new, - root_path: "/" # For migration purposes - ), - Chipmunk::Volume.new( - name: "bags", - reader: Chipmunk::Bag::Reader.new, - writer: Chipmunk::Bag::MoveWriter.new, - root_path: Chipmunk.config.upload.storage_path - ) - ]) - end + ]) end Services.register(:checkpoint) do diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 8a685d58..f342955d 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -12,3 +12,33 @@ config.use_transactional_fixtures = true config.infer_spec_type_from_file_location! end + +Services.register(:incoming_storage) do + Chipmunk::IncomingStorage.new( + volume: Chipmunk::Volume.new( + name: "incoming", + reader: Chipmunk::Bag::Reader.new, + writer: Chipmunk::Bag::MoveWriter.new, + root_path: Chipmunk.config.upload.upload_path + ), + paths: Chipmunk::UploadPath.new("/"), + links: Chipmunk::UploadPath.new(Chipmunk.config.upload["rsync_point"]) + ) +end + +Services.register(:storage) do + Chipmunk::PackageStorage.new(volumes: [ + Chipmunk::Volume.new( + name: "test", + reader: Chipmunk::Bag::Reader.new, + writer: Chipmunk::Bag::MoveWriter.new, + root_path: Rails.root.join("spec/support/fixtures") + ), + Chipmunk::Volume.new( + name: "bags", + reader: Chipmunk::Bag::Reader.new, + writer: Chipmunk::Bag::MoveWriter.new, + root_path: Chipmunk.config.upload.storage_path + ) + ]) +end From 535b3b83ded62afb7852da8130427ecdb1de1ccb Mon Sep 17 00:00:00 2001 From: Bryan Hockey Date: Wed, 21 Aug 2019 15:09:36 -0500 Subject: [PATCH 15/26] Move package.stored? failure case to BagMoveJob This check was concerned with the order in which finishing a deposit takes place or with preventing duplicate deposits. IncomingStorage is not the correct place for this check. Moved to BagMoveJob. --- app/jobs/bag_move_job.rb | 5 ++++- lib/chipmunk/incoming_storage.rb | 2 -- spec/chipmunk/incoming_storage_spec.rb | 6 ------ spec/jobs/bag_move_job_spec.rb | 21 +++++++++++++++++++++ 4 files changed, 25 insertions(+), 9 deletions(-) diff --git a/app/jobs/bag_move_job.rb b/app/jobs/bag_move_job.rb index b2e3471f..e83b09af 100644 --- a/app/jobs/bag_move_job.rb +++ b/app/jobs/bag_move_job.rb @@ -30,7 +30,10 @@ def ingest! end def validate - package.valid_for_ingest?(errors) + if package.stored? + errors << "Package #{package} is already stored" + end + !package.stored? && package.valid_for_ingest?(errors) end def move_bag diff --git a/lib/chipmunk/incoming_storage.rb b/lib/chipmunk/incoming_storage.rb index d5d4eeaa..332250a7 100644 --- a/lib/chipmunk/incoming_storage.rb +++ b/lib/chipmunk/incoming_storage.rb @@ -23,8 +23,6 @@ def initialize(volume:, paths:, links:) end def for(package) - raise Chipmunk::PackageAlreadyStoredError, package if package.stored? - volume.get(upload_path(package)) end diff --git a/spec/chipmunk/incoming_storage_spec.rb b/spec/chipmunk/incoming_storage_spec.rb index efa9d878..23432cc1 100644 --- a/spec/chipmunk/incoming_storage_spec.rb +++ b/spec/chipmunk/incoming_storage_spec.rb @@ -23,12 +23,6 @@ end end - context "with a package that is already in preservation" do - it "raises an already-stored error" do - expect { storage.for(stored_package) }.to raise_error(Chipmunk::PackageAlreadyStoredError) - end - end - context "with a package that is uploaded to a user's directory (not yet in preservation)" do let(:incoming_bag) { double(:bag) } diff --git a/spec/jobs/bag_move_job_spec.rb b/spec/jobs/bag_move_job_spec.rb index 3fbedc13..561d349e 100644 --- a/spec/jobs/bag_move_job_spec.rb +++ b/spec/jobs/bag_move_job_spec.rb @@ -77,6 +77,27 @@ end end + context "with a package that is already in preservation" do + subject(:run_job) { described_class.perform_now(queue_item) } + before(:each) { allow(package).to receive(:stored?).and_return true } + + it "does not move the bag" do + expect(Services.storage).not_to receive(:write).with(package, anything) + run_job + end + + it "updates the queue_item to status 'failed'" do + run_job + expect(queue_item.status).to eql("failed") + end + + it "records the validation error" do + run_job + expect(queue_item.error).to match(/already stored/) + end + + end + context "when the package is invalid" do subject(:run_job) { described_class.perform_now(queue_item) } From 883031686623e82e83d73b4e1616cffbd5e66a60 Mon Sep 17 00:00:00 2001 From: Bryan Hockey Date: Wed, 21 Aug 2019 15:12:31 -0500 Subject: [PATCH 16/26] BagMoveJob spec housekeeping * Checking whether or not the bag moved was not sufficiently paranoid * Removed some duplicate tests --- spec/jobs/bag_move_job_spec.rb | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/spec/jobs/bag_move_job_spec.rb b/spec/jobs/bag_move_job_spec.rb index 561d349e..478de4e1 100644 --- a/spec/jobs/bag_move_job_spec.rb +++ b/spec/jobs/bag_move_job_spec.rb @@ -41,7 +41,7 @@ end it "moves the bag" do - expect(Services.storage).to receive(:write).with(queue_item.package, bag) + expect(Services.storage).to receive(:write) run_job end @@ -82,7 +82,7 @@ before(:each) { allow(package).to receive(:stored?).and_return true } it "does not move the bag" do - expect(Services.storage).not_to receive(:write).with(package, anything) + expect(Services.storage).not_to receive(:write) run_job end @@ -109,7 +109,7 @@ end it "does not move the bag" do - expect(Services.storage).not_to receive(:write).with(package, anything) + expect(Services.storage).not_to receive(:write) run_job end @@ -122,11 +122,6 @@ run_job expect(queue_item.error).to match(/my error/) end - - it "does not move the bag" do - expect(Services.storage).not_to receive(:write).with(package, anything) - run_job - end end context "when validation raises an exception" do From 9d49cb4be220d3847e42f7c7af773dbb6ae34aa5 Mon Sep 17 00:00:00 2001 From: Bryan Hockey Date: Wed, 21 Aug 2019 17:06:43 -0500 Subject: [PATCH 17/26] Add Package#username Remove demeter violation for user.username in UserUploadPath --- app/models/package.rb | 4 ++++ lib/chipmunk/user_upload_path.rb | 2 +- spec/chipmunk/incoming_storage_spec.rb | 3 +-- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/app/models/package.rb b/app/models/package.rb index 6e3b3303..7daf8198 100644 --- a/app/models/package.rb +++ b/app/models/package.rb @@ -26,6 +26,10 @@ def self.policy_class PackagePolicy end + def username + user.username + end + def upload_link Services.incoming_storage.upload_link(self) end diff --git a/lib/chipmunk/user_upload_path.rb b/lib/chipmunk/user_upload_path.rb index e742c654..13fcb5b1 100644 --- a/lib/chipmunk/user_upload_path.rb +++ b/lib/chipmunk/user_upload_path.rb @@ -12,7 +12,7 @@ def initialize(root_path) end def for(package) - File.join(root_path, package.user.username, package.identifier) + File.join(root_path, package.username, package.identifier) end private diff --git a/spec/chipmunk/incoming_storage_spec.rb b/spec/chipmunk/incoming_storage_spec.rb index 23432cc1..541f6b1a 100644 --- a/spec/chipmunk/incoming_storage_spec.rb +++ b/spec/chipmunk/incoming_storage_spec.rb @@ -5,8 +5,7 @@ let(:writer) { double(:writer, format: "some-pkg") } let(:volume) { Chipmunk::Volume.new(name: "incoming", writer: writer, reader: reader, root_path: "/incoming") } let(:path_builder) { Chipmunk::UploadPath.new("/") } - let(:uploader) { instance_double("User", username: "uploader") } - let(:unstored_package) { instance_double("Package", stored?: false, user: uploader, bag_id: "abcdef-123456", identifier: "abcdef-123456") } + let(:unstored_package) { instance_double("Package", stored?: false, username: "uploader", bag_id: "abcdef-123456", identifier: "abcdef-123456") } let(:stored_package) { instance_double("Package", stored?: true) } subject(:storage) do From a45e127164a661f7fafc0804bca4cdf91bb4879b Mon Sep 17 00:00:00 2001 From: Bryan Hockey Date: Tue, 27 Aug 2019 01:52:47 -0500 Subject: [PATCH 18/26] Rebuild validations as multiple validators * Remove Chipmunk::Validatable (replaced) * Remove Chipmunk::Bag::Validator (replaced) * Add ValidatorService * Add ValidationResult * BagMoveJob relies on ValidatorService and does not interrogate Package * Moved Package's validation logic out of the class --- app/jobs/bag_move_job.rb | 7 +- app/models/package.rb | 35 +--- config/initializers/services.rb | 2 + lib/chipmunk.rb | 3 +- lib/chipmunk/bag.rb | 1 - lib/chipmunk/bag/validator.rb | 83 --------- lib/chipmunk/validatable.rb | 38 ---- lib/chipmunk/validation_result.rb | 20 +++ lib/chipmunk/validation_service.rb | 50 ++++++ lib/chipmunk/validator.rb | 1 + lib/chipmunk/validator/bag_consistency.rb | 38 ++++ lib/chipmunk/validator/bag_matches_package.rb | 29 +++ lib/chipmunk/validator/bagger_profile.rb | 39 ++++ lib/chipmunk/validator/external.rb | 41 +++++ lib/chipmunk/validator/format.rb | 19 ++ lib/chipmunk/validator/validator.rb | 77 ++++++++ spec/chipmunk/bag/validator_spec.rb | 168 ------------------ spec/chipmunk/validation_result_spec.rb | 30 ++++ .../validator/bag_consistency_spec.rb | 69 +++++++ .../validator/bag_matches_package_spec.rb | 53 ++++++ .../chipmunk/validator/bagger_profile_spec.rb | 17 ++ spec/chipmunk/validator/external_spec.rb | 65 +++++++ spec/chipmunk/validator/format_spec.rb | 20 +++ spec/chipmunk/validator/validator_spec.rb | 74 ++++++++ spec/jobs/bag_move_job_spec.rb | 72 +------- spec/models/package_spec.rb | 120 ------------- 26 files changed, 653 insertions(+), 518 deletions(-) delete mode 100644 lib/chipmunk/bag/validator.rb delete mode 100644 lib/chipmunk/validatable.rb create mode 100644 lib/chipmunk/validation_result.rb create mode 100644 lib/chipmunk/validation_service.rb create mode 100644 lib/chipmunk/validator.rb create mode 100644 lib/chipmunk/validator/bag_consistency.rb create mode 100644 lib/chipmunk/validator/bag_matches_package.rb create mode 100644 lib/chipmunk/validator/bagger_profile.rb create mode 100644 lib/chipmunk/validator/external.rb create mode 100644 lib/chipmunk/validator/format.rb create mode 100644 lib/chipmunk/validator/validator.rb delete mode 100644 spec/chipmunk/bag/validator_spec.rb create mode 100644 spec/chipmunk/validation_result_spec.rb create mode 100644 spec/chipmunk/validator/bag_consistency_spec.rb create mode 100644 spec/chipmunk/validator/bag_matches_package_spec.rb create mode 100644 spec/chipmunk/validator/bagger_profile_spec.rb create mode 100644 spec/chipmunk/validator/external_spec.rb create mode 100644 spec/chipmunk/validator/format_spec.rb create mode 100644 spec/chipmunk/validator/validator_spec.rb diff --git a/app/jobs/bag_move_job.rb b/app/jobs/bag_move_job.rb index e83b09af..153891f0 100644 --- a/app/jobs/bag_move_job.rb +++ b/app/jobs/bag_move_job.rb @@ -30,10 +30,9 @@ def ingest! end def validate - if package.stored? - errors << "Package #{package} is already stored" - end - !package.stored? && package.valid_for_ingest?(errors) + result = Services.validation.validate(package) + errors.concat result.errors + result.valid? end def move_bag diff --git a/app/models/package.rb b/app/models/package.rb index 7daf8198..dddf369d 100644 --- a/app/models/package.rb +++ b/app/models/package.rb @@ -19,7 +19,7 @@ def to_param validates :bag_id, presence: true, length: { minimum: 6 } validates :user_id, presence: true validates :external_id, presence: true - validates :format, presence: true + validates :format, presence: true, format: /bag/ # Declare the policy class to use for authz def self.policy_class @@ -45,39 +45,6 @@ def storage_location storage_path end - def storage_location= - raise "storage_location is not writable; use storage_volume and storage_path" - end - - # TODO: This is nasty... but the storage factory checks that the package is stored, - # so we have to make the storage proxy manually here. Once the ingest and preservation - # responsibilities are clarified, this will fall out. See PFDR-184. - def valid_for_ingest?(errors = []) - if stored? - errors << "Package #{bag_id} is already stored" - elsif format != Chipmunk::Bag.format - errors << "Package #{bag_id} has invalid format: #{format}" - elsif !incoming_storage.include?(self) - errors << "Bag #{bag_id} does not exist in incoming storage." - end - - return false unless errors.empty? - - Chipmunk::Bag::Validator.new(self, errors, incoming_storage.for(self)).valid? - end - - def external_validation_cmd - ext_cmd = Rails.application.config.validation["external"][content_type.to_s] - return unless ext_cmd - - path = incoming_storage.for(self).path - [ext_cmd, external_id, path].join(" ") - end - - def bagger_profile - Rails.application.config.validation["bagger_profile"][content_type.to_s] - end - def resource_type content_type end diff --git a/config/initializers/services.rb b/config/initializers/services.rb index 59595837..ad0f76da 100644 --- a/config/initializers/services.rb +++ b/config/initializers/services.rb @@ -63,6 +63,8 @@ def assign_db(lhs, rhs) ]) end +Services.register(:validation) { Chipmunk::ValidationService.new } + Services.register(:checkpoint) do Checkpoint::Authority.new(agent_resolver: KCV::AgentResolver.new, credential_resolver: Chipmunk::RoleResolver.new, diff --git a/lib/chipmunk.rb b/lib/chipmunk.rb index d30627be..ef48e146 100644 --- a/lib/chipmunk.rb +++ b/lib/chipmunk.rb @@ -6,7 +6,6 @@ module Chipmunk require "semantic_logger" require_relative "chipmunk/errors" -require_relative "chipmunk/validatable" require_relative "chipmunk/bag" require_relative "chipmunk/deposit_status" @@ -16,3 +15,5 @@ module Chipmunk require_relative "chipmunk/upload_path" require_relative "chipmunk/user_upload_path" require_relative "chipmunk/volume" +require_relative "chipmunk/validator" +require_relative "chipmunk/validation_service" diff --git a/lib/chipmunk/bag.rb b/lib/chipmunk/bag.rb index ee7af33c..ba287c4f 100644 --- a/lib/chipmunk/bag.rb +++ b/lib/chipmunk/bag.rb @@ -121,4 +121,3 @@ def respond_to_missing?(name, include_private = false) require_relative "bag/reader" require_relative "bag/move_writer" require_relative "bag/tag" -require_relative "bag/validator" diff --git a/lib/chipmunk/bag/validator.rb b/lib/chipmunk/bag/validator.rb deleted file mode 100644 index 2868386e..00000000 --- a/lib/chipmunk/bag/validator.rb +++ /dev/null @@ -1,83 +0,0 @@ -# frozen_string_literal: true - -class Chipmunk::Bag - class Validator - include Chipmunk::Validatable - - attr_reader :errors - - # TODO: Decide exactly what this should take... Package, Bag, or what? - # @param package [Package] - # @param bag [Chipmunk::Package] If nil, the bag does not yet exist. - def initialize(package, errors = [], bag = nil) - @package = package - @bag = bag - @errors = errors - end - - # TODO: Separate ingest and storage validation: see PFDR-184 and Package#valid_for_ingest? - # Remove this when it has a proper home. - # validates "bag must exist on disk at src_path", - # condition: -> { File.exist?(src_path) }, - # error: -> { "Bag does not exist at upload location #{src_path}" } - - validates "bag on disk must be valid", - condition: -> { bag.valid? }, - error: -> { "Error validating bag:\n" + indent_array(bag.errors.full_messages) } - - { "External-Identifier" => :external_id, - "Bag-ID" => :bag_id, - "Chipmunk-Content-Type" => :content_type }.each_pair do |file_key, db_key| - validates "#{file_key} in bag on disk matches bag in database", - precondition: -> { [bag.chipmunk_info[file_key], package.public_send(db_key)] }, - condition: ->(file_val, db_val) { file_val == db_val }, - error: lambda {|file_val, db_val| - "uploaded #{file_key} '#{file_val}'" \ - " does not match expected value '#{db_val}'" - } - end - - validates "Bag ID in bag on disk matches bag in database", - condition: -> { bag.chipmunk_info["Bag-ID"] == package.bag_id }, - error: lambda { - "uploaded Bag-ID '#{bag.chipmunk_info["Bag-ID"]}'" \ - " does not match intended ID '#{package.bag_id}'" - } - - metadata_tags = ["Metadata-URL", "Metadata-Type", "Metadata-Tagfile"] - - validates "chipmunk-info.txt has metadata tags", - error: -> { "Some (but not all) metadata tags #{metadata_tags} missing in chipmunk-info.txt" }, - condition: lambda { - metadata_tags.all? {|tag| bag.chipmunk_info.key?(tag) } || - metadata_tags.none? {|tag| bag.chipmunk_info.key?(tag) } - } - - validates "bag on disk has referenced metadata files", - only_if: -> { bag.chipmunk_info["Metadata-Tagfile"] }, - error: -> { "Missing referenced metadata #{bag.chipmunk_info["Metadata-Tagfile"]}" }, - condition: lambda { - bag.tag_files.map {|f| File.basename(f) } - .include?(bag.chipmunk_info["Metadata-Tagfile"]) - } - - validates "bag on disk passes external validation", - only_if: -> { package.external_validation_cmd }, - precondition: -> { Open3.capture3(package.external_validation_cmd) }, - condition: ->(_, _, status) { status.exitstatus.zero? }, - error: ->(_, stderr, _) { "Error validating content\n" + stderr } - - validates "bag on disk meets bagger profile", - only_if: -> { package.bagger_profile }, - condition: -> { Profile.new(package.bagger_profile).valid?(bag.bag_info, errors: errors) }, - error: -> { "Not valid according to bagger profile" } - - private - - def indent_array(array, width = 2) - array.map {|s| " " * width + s }.join("\n") - end - - attr_reader :package, :bag - end -end diff --git a/lib/chipmunk/validatable.rb b/lib/chipmunk/validatable.rb deleted file mode 100644 index d71bc2f4..00000000 --- a/lib/chipmunk/validatable.rb +++ /dev/null @@ -1,38 +0,0 @@ -# frozen_string_literal: true - -module Chipmunk - module Validatable - - def self.included(base) - base.extend(ClassMethods) - end - - module ClassMethods - def validators - @validators ||= [] - end - - def validates(_description = "", only_if: -> { true }, precondition: -> {}, condition:, error:) - validators << lambda do - return true unless instance_exec(&only_if) - - precond_result = instance_exec(&precondition) - if instance_exec(*precond_result, &condition) - true - else - errors << instance_exec(*precond_result, &error) - false - end - end - end - - end - - def valid? - self.class.validators.reduce(true) do |result, validator| - result && instance_exec(&validator) - end - end - - end -end diff --git a/lib/chipmunk/validation_result.rb b/lib/chipmunk/validation_result.rb new file mode 100644 index 00000000..b5f856ec --- /dev/null +++ b/lib/chipmunk/validation_result.rb @@ -0,0 +1,20 @@ +module Chipmunk + + # The result of a validation attempt + class ValidationResult + + def initialize(errors) + @errors = [errors].flatten.compact + @errors.freeze + end + + # @return [Array] + attr_reader :errors + + # @return [Boolean] + def valid? + errors.empty? + end + end + +end diff --git a/lib/chipmunk/validation_service.rb b/lib/chipmunk/validation_service.rb new file mode 100644 index 00000000..4d5a6265 --- /dev/null +++ b/lib/chipmunk/validation_service.rb @@ -0,0 +1,50 @@ +require_relative "validation_result" + +module Chipmunk + + class ValidationService + + # @return [ValidationResult] + def validate(validatable) + case validatable + when Package + validate_package(validatable) + else + ValidationResult.new(["Did not recognize type #{validatable.class}"]) + end + end + + private + + def validate_package(package) + if incoming_storage.include?(package) + sip = incoming_storage.for(package) + ValidationResult.new(errors(package_validators(package), sip)) + else + return ValidationResult.new(["Could not find an uploaded sip"]) + end + end + + def package_validators(package) + [ + Validator::BagConsistency.new, + Validator::BagMatchesPackage.new(package), + Validator::External.new(package), + Validator::BaggerProfile.new(package) + ] + end + + # @param validators [Array] + # @param validatable [Object] The object being validated, e.g. a Bag + def errors(validators, validatable) + validators.reduce([]) do |errors, validator| + errors + validator.errors(validatable) + end + end + + def incoming_storage + Services.incoming_storage + end + end + +end diff --git a/lib/chipmunk/validator.rb b/lib/chipmunk/validator.rb new file mode 100644 index 00000000..e668dc59 --- /dev/null +++ b/lib/chipmunk/validator.rb @@ -0,0 +1 @@ +Dir[File.join(__dir__, 'validator', '**', '*.rb')].each { |file| require_relative file } diff --git a/lib/chipmunk/validator/bag_consistency.rb b/lib/chipmunk/validator/bag_consistency.rb new file mode 100644 index 00000000..79e24915 --- /dev/null +++ b/lib/chipmunk/validator/bag_consistency.rb @@ -0,0 +1,38 @@ +require_relative "validator" + +module Chipmunk + module Validator + + # Validates the internal consistency of a bag, with no external information. + class BagConsistency < Validator + + METADATA_TAGS = ["Metadata-URL", "Metadata-Type", "Metadata-Tagfile"].freeze + + validates "bag on disk must be valid", + condition: proc {|bag| bag.valid? }, + error: proc {|bag| "Error validating bag:\n" + indent_array(bag.errors.full_messages) } + + validates "chipmunk-info.txt has metadata tags", + error: proc { "Some (but not all) metadata tags #{METADATA_TAGS} missing in chipmunk-info.txt" }, + condition: proc {|bag| + METADATA_TAGS.all? {|tag| bag.chipmunk_info.key?(tag) } || + METADATA_TAGS.none? {|tag| bag.chipmunk_info.key?(tag) } + } + + validates "bag on disk has referenced metadata files", + only_if: proc {|bag| bag.chipmunk_info["Metadata-Tagfile"] }, + error: proc {|bag| "Missing referenced metadata #{bag.chipmunk_info["Metadata-Tagfile"]}" }, + condition: proc {|bag| + bag.tag_files.map {|f| File.basename(f) } + .include?(bag.chipmunk_info["Metadata-Tagfile"]) + } + + private + + def indent_array(array, width = 2) + array.map {|s| " " * width + s }.join("\n") + end + end + + end +end diff --git a/lib/chipmunk/validator/bag_matches_package.rb b/lib/chipmunk/validator/bag_matches_package.rb new file mode 100644 index 00000000..4868d7d6 --- /dev/null +++ b/lib/chipmunk/validator/bag_matches_package.rb @@ -0,0 +1,29 @@ +require_relative "validator" + +module Chipmunk + module Validator + + # Validate that the bag is indeed the one specified by the Package + class BagMatchesPackage < Validator + + # @param package [Package] + def initialize(package) + @package = package + end + + { + "External-Identifier" => :external_id, + "Bag-ID" => :bag_id, + "Chipmunk-Content-Type" => :content_type, + }.each_pair do |file_key, db_key| + validates "#{file_key} in bag on disk matches bag in database", + condition: proc {|bag| bag.chipmunk_info[file_key] == package.public_send(db_key) }, + error: proc {"uploaded #{file_key} does not match expected value #{package.public_send(db_key)}"} + end + + private + attr_reader :package + + end + end +end diff --git a/lib/chipmunk/validator/bagger_profile.rb b/lib/chipmunk/validator/bagger_profile.rb new file mode 100644 index 00000000..df6cf1cc --- /dev/null +++ b/lib/chipmunk/validator/bagger_profile.rb @@ -0,0 +1,39 @@ +require_relative "validator" + +module Chipmunk + module Validator + + # Validate that the bag obeys the spec set by the profile + class BaggerProfile < Validator + + # @param [Package] + def initialize(package) + @package = package + end + + validates "bag on disk meets bagger profile", + only_if: proc { uri }, + precondition: proc {|bag| + [].tap {|errors| profile.valid?(bag.bag_info, errors: errors) } + }, + condition: proc {|bag, errors| errors.empty? }, + error: proc {|bag, errors| errors } + + private + + attr_reader :package + + def uri + @uri ||= Rails.application.config + .validation["bagger_profile"][package.content_type.to_s] + end + + def profile + if uri + Bag::Profile.new(uri) + end + end + end + + end +end diff --git a/lib/chipmunk/validator/external.rb b/lib/chipmunk/validator/external.rb new file mode 100644 index 00000000..8bddf24f --- /dev/null +++ b/lib/chipmunk/validator/external.rb @@ -0,0 +1,41 @@ +require_relative "validator" +require "open3" + +module Chipmunk + module Validator + + # Validate that some external validation passes + class External < Validator + + # @param command [String] The fully specified command, including any reference + # to the object to be validated. + def initialize(package) + @package = package + end + + def command + if ext_bin + path = Services.incoming_storage.for(package).path + [ext_bin, package.identifier, path].join(" ") + end + end + + validates "the object passes external validation", + only_if: proc {|validatable| !command.nil? && validatable.valid? }, + precondition: proc { Open3.capture3(command) }, + condition: proc {|_, _, _, status| status.exitstatus.zero? }, + error: proc {|_, _, stderr, _| "Error validating content\n" + stderr } + + private + + attr_reader :package + + def ext_bin + @ext_bin ||= Rails.application.config + .validation["external"][package.content_type.to_s] + end + + end + + end +end diff --git a/lib/chipmunk/validator/format.rb b/lib/chipmunk/validator/format.rb new file mode 100644 index 00000000..47191335 --- /dev/null +++ b/lib/chipmunk/validator/format.rb @@ -0,0 +1,19 @@ +require_relative "validator" + +module Chipmunk + module Validator + class Format < Validator + def initialize(format) + @format = format + end + + validates "the format matches", + condition: proc {|sip| sip.format == format }, + error: proc {|sip| "SIP #{sip.identifier} has invalid format: #{sip.format}" } + + private + + attr_reader :format + end + end +end diff --git a/lib/chipmunk/validator/validator.rb b/lib/chipmunk/validator/validator.rb new file mode 100644 index 00000000..78107c61 --- /dev/null +++ b/lib/chipmunk/validator/validator.rb @@ -0,0 +1,77 @@ +module Chipmunk + module Validator + + + # A validator runs compile-time validations against a validatable object + # passed to #valid? or #errors during run-time. Validations are created + # with the DSL defined in this class via the ::validates method. Subclasses + # should define their own initializers and instance variables to facilitate + # run-time configuration of the validations. The validations themselves + # are run in instance context via instance_exec. + # + # This replaces Chipmunk::Validatable + class Validator + class << self + def validations + @validations ||= [] + end + + # Define a validation that instances of this validator will run + # @param _desc [String] A description of the check being performed. This + # string is discarded; it is for documentation purposes only. + # @param only_if [Proc] The condition will only be checked if + # this block evaluates to true, which it does by default. The object + # being validated is passed to this block. + # @param precondition [Proc] This will be evaluated prior to the condition + # and the error, and its result will be passed in expanded form to + # those procs. + # @param condition [Proc] The primary condition of this validation. + # The object being validated is passed to this block. + # @param error [Proc] A block to build out the error message; it must + # return a string. The object being validated is passed to this block. + def validates(_desc = "", only_if: proc { true }, precondition: proc {}, condition:, error:) + validations << lambda do |validatable| + return unless instance_exec(validatable, &only_if) + precond_result = instance_exec(validatable, &precondition) + unless instance_exec(validatable, *precond_result, &condition) + instance_exec(validatable, *precond_result, &error) + end + end + end + end + + # Whether or not the validatable is valid. + # @param validatable [Object] An object that can be validated by this validator. + # @return [Boolean] + def valid?(validatable) + errors(validatable).empty? + end + + # An array of errors from the validation process. An empty array indicates + # a valid object. + # @param validatable [Object] An object that can be validated by this validator. + # @return [Array] An unordered list of errors. + def errors(validatable) + validated[validatable] ||= generate_errors(validatable) + end + + private + + # Actually run the validations, returning an array of errors. nil return + # values from the validations are removed. + def generate_errors(validatable) + self.class.validations.reduce([]) do |errs, validation| + errs << instance_exec(validatable, &validation) + end.compact + end + + # A cache of objects that have already been validated by this validator + # and their errors. + def validated + @validated = {} + end + + end + end + +end diff --git a/spec/chipmunk/bag/validator_spec.rb b/spec/chipmunk/bag/validator_spec.rb deleted file mode 100644 index e4e180d4..00000000 --- a/spec/chipmunk/bag/validator_spec.rb +++ /dev/null @@ -1,168 +0,0 @@ -# frozen_string_literal: true - -require "rails_helper" -require "open3" - -RSpec.describe Chipmunk::Bag::Validator do - def exitstatus(status) - double(:exitstatus, exitstatus: status) - end - - let(:queue_item) { Fabricate(:queue_item) } - let(:package) { queue_item.package } - let(:good_tag_files) { [fixture("marc.xml")] } - - let(:chipmunk_info_db) do - { - "External-Identifier" => package.external_id, - "Chipmunk-Content-Type" => package.content_type, - "Bag-ID" => package.bag_id - } - end - - let(:chipmunk_info_with_metadata) do - chipmunk_info_db.merge( - "Metadata-Type" => "MARC", - "Metadata-URL" => "http://what.ever", - "Metadata-Tagfile" => "marc.xml" - ) - end - - # default (good case) - let(:fakebag) { double("fake bag", valid?: true, path: "/incoming/bag") } - let(:ext_validation_result) { ["", "", exitstatus(0)] } - let(:bag_info) { { "Foo" => "bar", "Baz" => "quux" } } - let(:tag_files) { good_tag_files } - let(:chipmunk_info) { chipmunk_info_with_metadata } - - let(:errors) { [] } - - around(:each) do |example| - old_profile = Rails.application.config.validation["bagger_profile"] - profile_uri = "file://" + Rails.root.join("spec", "support", "fixtures", "test-profile.json").to_s - Rails.application.config.validation["bagger_profile"] = { "digital" => profile_uri, "audio" => profile_uri } - example.run - Rails.application.config.validation["bagger_profile"] = old_profile - end - - describe "#valid?" do - let(:validator) { described_class.new(package, errors, fakebag) } - - before(:each) do - allow(Services.incoming_storage).to receive(:for).and_return(fakebag) - allow(fakebag).to receive(:chipmunk_info).and_return(chipmunk_info) - allow(fakebag).to receive(:tag_files).and_return(tag_files) - allow(fakebag).to receive(:bag_info).and_return(bag_info) - allow(Open3).to receive(:capture3).and_return(ext_validation_result) - end - - shared_examples_for "an invalid item" do |error_pattern| - it "records the validation error" do - validator.valid? - expect(errors).to include a_string_matching error_pattern - end - - it "returns false" do - expect(validator.valid?).to be false - end - end - - context "when the bag is valid" do - context "and its metadata matches the queue item" do - it "returns true" do - expect(validator.valid?).to be true - end - end - - context "but its external ID does not match the queue item" do - let(:chipmunk_info) { chipmunk_info_with_metadata.merge("External-Identifier" => "something-different") } - - it_behaves_like "an invalid item", /External-Identifier/ - end - - context "but its bag ID does not match the queue item" do - let(:chipmunk_info) { chipmunk_info_with_metadata.merge("Bag-ID" => "something-different") } - - it_behaves_like "an invalid item", /Bag-ID/ - end - - context "but its package type does not match the queue item" do - let(:chipmunk_info) { chipmunk_info_with_metadata.merge("Chipmunk-Content-Type" => "something-different") } - - it_behaves_like "an invalid item", /Chipmunk-Content-Type/ - end - - context "but does not include the referenced metadata file" do - let(:tag_files) { [] } - - it_behaves_like "an invalid item", /Missing.*marc.xml/ - end - - context "but does not any include descriptive metadata tags" do - let(:chipmunk_info) { chipmunk_info_db } - let(:tag_files) { [] } - - it "returns true" do - expect(validator.valid?).to be true - end - end - - context "but has only some descriptive metadata tags" do - let(:chipmunk_info) do - chipmunk_info_db.merge( - "Metadata-URL" => "http://what.ever", - "Metadata-Tagfile" => "marc.xml" - ) - end - - it_behaves_like "an invalid item", /Metadata-Type/ - end - - context "but external validation fails" do - around(:each) do |example| - old_ext_validation = Rails.application.config.validation["external"] - Rails.application.config.validation["external"] = { package.content_type => "something" } - example.run - Rails.application.config.validation["external"] = old_ext_validation - end - - let(:chipmunk_info) { chipmunk_info_with_metadata } - let(:ext_validation_result) { ["external output", "external error", exitstatus(1)] } - - it_behaves_like "an invalid item", /external error/ - end - - context "but the package type has no external validation command" do - around(:each) do |example| - old_ext_validation = Rails.application.config.validation["external"] - Rails.application.config.validation["external"] = {} - example.run - Rails.application.config.validation["external"] = old_ext_validation - end - - it "does not try to run external validation" do - expect(Open3).not_to receive(:capture3) - validator.valid? - end - end - end - - context "when the bag is invalid" do - let(:bag_errors) { double("bag_errors", full_messages: ["injected error"]) } - let(:fakebag) { double("fake bag", valid?: false, errors: bag_errors) } - - it_behaves_like "an invalid item", /Error validating.*\n injected error$/ - - it "does not try to run external validation" do - expect(Open3).not_to receive(:capture3) - validator.valid? - end - end - - context "with a bagger profile and bag not valid according to the profile" do - let(:bag_info) { { "Baz" => "quux" } } - - it_behaves_like "an invalid item", /Foo.*required/ - end - end -end diff --git a/spec/chipmunk/validation_result_spec.rb b/spec/chipmunk/validation_result_spec.rb new file mode 100644 index 00000000..c187ddcf --- /dev/null +++ b/spec/chipmunk/validation_result_spec.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +RSpec.describe Chipmunk::ValidationResult do + describe "#valid?" do + it "is valid when there are no errors" do + expect(described_class.new([]).valid?).to be true + end + it "is invalid when there are errors" do + expect(described_class.new([1]).valid?).to be false + end + end + + describe "#errors" do + it "returns the errors" do + expect(described_class.new(["error"]).errors).to eql(["error"]) + end + + it "handles un-nested errors" do + expect(described_class.new("error").errors).to eql(["error"]) + end + + it "handles overly nested errors" do + expect(described_class.new([["error"]]).errors).to eql(["error"]) + end + + it "removes nils" do + expect(described_class.new([nil, "error"]).errors).to eql(["error"]) + end + end +end diff --git a/spec/chipmunk/validator/bag_consistency_spec.rb b/spec/chipmunk/validator/bag_consistency_spec.rb new file mode 100644 index 00000000..cfd207c7 --- /dev/null +++ b/spec/chipmunk/validator/bag_consistency_spec.rb @@ -0,0 +1,69 @@ +RSpec.describe Chipmunk::Validator::BagConsistency do + let(:validator) { described_class.new } + let(:tag_files) { [fixture("marc.xml")] } + let(:validity) { true } + let(:bag_errors) { double(:empty_bag_errors) } + let(:bag) do + double( + :bag, + valid?: validity, + errors: bag_errors, + tag_files: tag_files, + chipmunk_info: chipmunk_info + ) + end + let(:chipmunk_info) do + { + "Metadata-Type" => "MARC", + "Metadata-URL" => "http://what.ever", + "Metadata-Tagfile" => "marc.xml" + } + end + + context "when bag#valid? is false" do + let(:validity) { false } + let(:bag_errors) { double("bag_errors", full_messages: ["injected error"]) } + + it "is invalid" do + expect(validator.valid?(bag)).to be false + end + + it "reports the errors from the bag" do + expect(validator.errors(bag)).to include( + a_string_matching( /Error validating.*\n injected error$/) + ) + end + end + + context "when the bag does not include the referenced metadata file" do + let(:tag_files) { [] } + + it "reports missing tag files" do + expect(validator.errors(bag)) + .to include(a_string_matching(/Missing.*marc.xml/)) + end + end + + context "when the bag does not any include descriptive metadata tags" do + let(:tag_files) { [] } + let(:chipmunk_info) { {} } + + it "is valid" do + expect(validator.valid?(bag)).to be true + end + end + + context "when the bag has only some descriptive metadata tags" do + let(:chipmunk_info) do + { + "Metadata-URL" => "http://what.ever", + "Metadata-Tagfile" => "marc.xml" + } + end + + it "reports missing metadata tag" do + expect(validator.errors(bag)) + .to include(a_string_matching(/Metadata-Type/)) + end + end +end diff --git a/spec/chipmunk/validator/bag_matches_package_spec.rb b/spec/chipmunk/validator/bag_matches_package_spec.rb new file mode 100644 index 00000000..2ce36c3e --- /dev/null +++ b/spec/chipmunk/validator/bag_matches_package_spec.rb @@ -0,0 +1,53 @@ +require "securerandom" + +RSpec.describe Chipmunk::Validator::BagMatchesPackage do + let(:validator) { described_class.new(package) } + let(:bag) { double(:bag, chipmunk_info: chipmunk_info) } + let(:package) do + double( + :package, + bag_id: SecureRandom.uuid, + external_id: SecureRandom.uuid, + content_type: "audio" + ) + end + let(:good_chipmunk_info) do + { + "External-Identifier" => package.external_id, + "Chipmunk-Content-Type" => package.content_type, + "Bag-ID" => package.bag_id + } + end + + context "when everything matches" do + let(:chipmunk_info) { good_chipmunk_info } + it { expect(validator.valid?(bag)).to be true } + end + + context "when its external ID does not match" do + let(:chipmunk_info) { good_chipmunk_info.merge("External-Identifier" => "something-different") } + + it "reports the error" do + expect(validator.errors(bag)) + .to contain_exactly(a_string_matching(/External-Identifier/)) + end + end + + context "when its bag ID does not match the queue item" do + let(:chipmunk_info) { good_chipmunk_info.merge("Bag-ID" => "something-different") } + + it "reports the error" do + expect(validator.errors(bag)) + .to contain_exactly(a_string_matching(/Bag-ID/)) + end + end + + context "when its package type does not match the queue item" do + let(:chipmunk_info) { good_chipmunk_info.merge("Chipmunk-Content-Type" => "something-different") } + + it "reports the error" do + expect(validator.errors(bag)) + .to contain_exactly(a_string_matching(/Chipmunk-Content-Type/)) + end + end +end diff --git a/spec/chipmunk/validator/bagger_profile_spec.rb b/spec/chipmunk/validator/bagger_profile_spec.rb new file mode 100644 index 00000000..2d2f6135 --- /dev/null +++ b/spec/chipmunk/validator/bagger_profile_spec.rb @@ -0,0 +1,17 @@ +RSpec.describe Chipmunk::Validator::BaggerProfile do + let(:validator) { described_class.new(package) } + let(:bag) { double(:bag, bag_info: { "Baz" => "quux" }) } + let(:package) { double(:package, content_type: "audio") } + + around(:each) do |example| + old_profile = Rails.application.config.validation["bagger_profile"]["audio"] + test_profile = "file://" + fixture("test-profile.json") + Rails.application.config.validation["bagger_profile"]["audio"] = test_profile.to_s + example.run + Rails.application.config.validation["bagger_profile"]["audio"] = old_profile + end + + it "tests the bag against the profile" do + expect(validator.errors(bag)).to include(a_string_matching(/Foo.*required/)) + end +end diff --git a/spec/chipmunk/validator/external_spec.rb b/spec/chipmunk/validator/external_spec.rb new file mode 100644 index 00000000..00e581b1 --- /dev/null +++ b/spec/chipmunk/validator/external_spec.rb @@ -0,0 +1,65 @@ +RSpec.describe Chipmunk::Validator::External do + let(:content_type) { "mycontenttype" } + let(:package) { double(:package, identifier: SecureRandom.uuid, content_type: content_type) } + let(:bag) { double(:bag, valid?: true, path: "/incoming/bag") } + let(:validator) { described_class.new(package) } + + before(:each) do + allow(Services.incoming_storage).to receive(:for).and_return(bag) + end + + context "when there is no external command configured" do + around(:each) do |example| + old_ext_validation = Rails.application.config.validation["external"] + Rails.application.config.validation["external"] = {} + example.run + Rails.application.config.validation["external"] = old_ext_validation + end + + it "is valid" do + expect(validator.valid?(bag)).to be true + end + end + + context "when the external command succeeds" do + around(:each) do |example| + old_ext_validation = Rails.application.config.validation["external"] + Rails.application.config.validation["external"] = { content_type => "/bin/true" } + example.run + Rails.application.config.validation["external"] = old_ext_validation + end + + it "constructs the command with the configured executable" do + expect(validator.command).to match(/^\/bin\/true/) + end + + it "includes the path to the source archive" do + expect(validator.command).to match("/incoming/bag") + end + + it "is valid" do + expect(validator.valid?(bag)).to be true + end + end + + context "when the external command fails" do + around(:each) do |example| + old_ext_validation = Rails.application.config.validation["external"] + Rails.application.config.validation["external"] = { content_type => "ls /nondir" } + example.run + Rails.application.config.validation["external"] = old_ext_validation + end + + it { expect(validator.valid?(bag)).to be false } + + it "reports the error" do + expect(validator.errors(bag)).to include(a_string_matching(/cannot access/)) + end + + it "skips this validator if the bag is invalid" do + bag = double(:invalid_bag, valid?: false) + expect(validator.valid?(bag)).to be true + end + end + +end diff --git a/spec/chipmunk/validator/format_spec.rb b/spec/chipmunk/validator/format_spec.rb new file mode 100644 index 00000000..38c25dd0 --- /dev/null +++ b/spec/chipmunk/validator/format_spec.rb @@ -0,0 +1,20 @@ +RSpec.describe Chipmunk::Validator::Format do + let(:format) { "someformat" } + let(:validator) { described_class.new(format) } + + context "when the format matches" do + let(:sip) { double(:sip, format: format) } + it "is valid" do + expect(validator.valid?(sip)).to be true + end + end + + context "when the format diverges" do + let(:sip) { double(:sip, identifier: "someid", format: "sandwich") } + + it "reports the error" do + expect(validator.errors(sip)) + .to include("SIP someid has invalid format: sandwich") + end + end +end diff --git a/spec/chipmunk/validator/validator_spec.rb b/spec/chipmunk/validator/validator_spec.rb new file mode 100644 index 00000000..993e9880 --- /dev/null +++ b/spec/chipmunk/validator/validator_spec.rb @@ -0,0 +1,74 @@ + +RSpec.describe Chipmunk::Validator::Validator do + class OnlyIfValidator < described_class + def initialize(only_if: true) + @only_if = only_if + end + + validates "For testing only_if", + only_if: proc { @only_if }, + precondition: proc { true }, + condition: proc { false }, + error: proc { "injected_error" } + end + + class PrecondValidator < described_class + def initialize(precond) + @precond = precond + end + + validates "For testing preconditions", + only_if: proc { true }, + precondition: proc { @precond }, + condition: proc {|_, precond_result| precond_result }, + error: proc {|_, precond_result| "#{precond_result} injected_error" } + end + + class BlockParamValidator < described_class + validates "For testing that blocks receive the object being validated", + only_if: proc {|v| v.called_by << :only_if; true }, + precondition: proc {|v| v.called_by << :precondition; true }, + condition: proc {|v| v.called_by << :condition; false }, + error: proc {|v| v.called_by << :error; "some_error" } + end + + let(:validatable) { double(:validatable) } + + context "when only_if is true" do + let(:failure) { OnlyIfValidator.new(only_if: true) } + it "runs the validation" do + expect(failure.valid?(validatable)).to be false + end + end + + context "when only_if is false" do + let(:success) { OnlyIfValidator.new(only_if: false) } + it "skips the validation" do + expect(success.valid?(validatable)).to be true + end + end + + context "with a precondition" do + let(:failure) { PrecondValidator.new(false) } + it "passes the precondition to the condition" do + expect(failure.valid?(validatable)).to be false + end + + it "passes the precondition to the error" do + expect(failure.errors(validatable)).to include("false injected_error") + end + end + + describe "each block receives the object being validated" do + let(:validatable) { double(:validatable, called_by: []) } + let(:failure) { BlockParamValidator.new } + + it "passes the validatable to each block" do + failure.valid?(validatable) + expect(validatable.called_by) + .to contain_exactly(:only_if, :precondition, :condition, :error) + end + end + + +end diff --git a/spec/jobs/bag_move_job_spec.rb b/spec/jobs/bag_move_job_spec.rb index 478de4e1..71561d87 100644 --- a/spec/jobs/bag_move_job_spec.rb +++ b/spec/jobs/bag_move_job_spec.rb @@ -6,26 +6,11 @@ let(:queue_item) { Fabricate(:queue_item) } let(:package) { queue_item.package } - let(:chipmunk_info_db) do - { - "External-Identifier" => package.external_id, - "Chipmunk-Content-Type" => package.content_type, - "Bag-ID" => package.bag_id - } - end - - let(:chipmunk_info_good) do - chipmunk_info_db.merge( - "Metadata-Type" => "MARC", - "Metadata-URL" => "http://what.ever", - "Metadata-Tagfile" => "marc.xml" - ) - end - describe "#perform" do let(:bag) { double(:bag, path: "/uploaded/bag") } before(:each) do + allow(Services.validation).to receive(:validate).with(package).and_return(validation_result) allow(Services.incoming_storage).to receive(:for).with(package).and_return(bag) allow(Services.storage).to receive(:write).with(package, bag) do |pkg, _bag| pkg.storage_volume = "bags" @@ -35,10 +20,7 @@ context "when the package is valid" do subject(:run_job) { described_class.perform_now(queue_item) } - - before(:each) do - allow(queue_item.package).to receive(:valid_for_ingest?).and_return true - end + let(:validation_result) { double(:validation_result, errors: [], valid?: true) } it "moves the bag" do expect(Services.storage).to receive(:write) @@ -77,36 +59,9 @@ end end - context "with a package that is already in preservation" do - subject(:run_job) { described_class.perform_now(queue_item) } - before(:each) { allow(package).to receive(:stored?).and_return true } - - it "does not move the bag" do - expect(Services.storage).not_to receive(:write) - run_job - end - - it "updates the queue_item to status 'failed'" do - run_job - expect(queue_item.status).to eql("failed") - end - - it "records the validation error" do - run_job - expect(queue_item.error).to match(/already stored/) - end - - end - context "when the package is invalid" do subject(:run_job) { described_class.perform_now(queue_item) } - - before(:each) do - allow(queue_item.package).to receive(:valid_for_ingest?) do |errors| - errors << "my error" - false - end - end + let(:validation_result) { double(valid?: false, errors: ["my error"]) } it "does not move the bag" do expect(Services.storage).not_to receive(:write) @@ -124,26 +79,5 @@ end end - context "when validation raises an exception" do - subject(:run_job) { described_class.perform_now(queue_item) } - - before(:each) do - allow(queue_item.package).to receive(:valid_for_ingest?).and_raise("test validation failure") - end - - it "re-raises the exception" do - expect { run_job }.to raise_exception(/test validation failure/) - end - - it "records the exception" do - run_job rescue StandardError - expect(queue_item.error).to match(/test validation failure/) - end - - it "records the stack trace" do - run_job rescue StandardError - expect(queue_item.error).to match(__FILE__) - end - end end end diff --git a/spec/models/package_spec.rb b/spec/models/package_spec.rb index 398692fd..611ec310 100644 --- a/spec/models/package_spec.rb +++ b/spec/models/package_spec.rb @@ -36,126 +36,6 @@ expect(request.upload_link).to eq(File.join(upload_link, uuid)) end - # TODO: This group's ugliness is because we have odd and temporary coupling between - # Package and Bag::Validator. Once the ingest/storage phases are separated, this will - # clean up considerably. See PFDR-184. - describe "#valid_for_ingest?" do - context "with an unstored bag" do - let(:package) { Fabricate.build(:unstored_package) } - let(:validator) { double(:validator) } - let(:disk_bag) { double(:bag) } - - before(:each) do - allow(Services.incoming_storage).to receive(:include?).with(package).and_return(true) - allow(Services.incoming_storage).to receive(:for).with(package).and_return(disk_bag) - allow(Chipmunk::Bag::Validator).to receive(:new).with(package, anything, disk_bag).and_return(validator) - end - - it "validates the bag with its validator" do - expect(validator).to receive(:valid?) - package.valid_for_ingest? - end - end - - context "with a stored bag" do - let(:package) { Fabricate.build(:stored_package) } - let(:result) { package.valid_for_ingest? } - - it "fails" do - expect(result).to be false - end - - it "does not create or use the bag validator" do - expect(Chipmunk::Bag::Validator).not_to receive(:new) - result - end - end - - context "with a bag with a bad path" do - let(:package) { Fabricate.build(:stored_package) } - let(:result) { package.valid_for_ingest? } - - before(:each) do - allow(Services.incoming_storage).to receive(:include?).and_return(false) - end - - it "fails" do - expect(result).to be false - end - - it "does not create or use the bag validator" do - expect(Chipmunk::Bag::Validator).not_to receive(:new) - result - end - end - - context "with a plain zip" do - let(:package) { Fabricate.build(:stored_package) } - let(:result) { package.valid_for_ingest? } - - it "fails" do - expect(result).to be false - end - - it "does not create or use the bag validator" do - expect(Chipmunk::Bag::Validator).not_to receive(:new) - result - end - end - end - - describe "#external_validation_cmd" do - let(:package) { Fabricate.build(:package) } - let(:bag) { double(:bag, path: "/incoming/bag") } - - before(:each) do - allow(Services.incoming_storage).to receive(:for).and_return(bag) - end - - context "when there is an external command configured" do - around(:each) do |example| - old_ext_validation = Rails.application.config.validation["external"] - Rails.application.config.validation["external"] = { package.content_type => "/bin/true" } - example.run - Rails.application.config.validation["external"] = old_ext_validation - end - - it "returns a command starting with the configured executable" do - expect(package.external_validation_cmd).to match(/^\/bin\/true/) - end - - it "includes the path to the source archive" do - expect(package.external_validation_cmd).to match("/incoming/bag") - end - end - - context "when there is no external command configured" do - around(:each) do |example| - old_ext_validation = Rails.application.config.validation["external"] - Rails.application.config.validation["external"] = {} - example.run - Rails.application.config.validation["external"] = old_ext_validation - end - - it "returns nil" do - expect(package.external_validation_cmd).to be_nil - end - end - end - - describe "#bagger_profile" do - around(:each) do |example| - old_profile = Rails.application.config.validation["bagger_profile"]["audio"] - Rails.application.config.validation["bagger_profile"]["audio"] = "foo" - example.run - Rails.application.config.validation["bagger_profile"]["audio"] = old_profile - end - - it "returns a bagger profile" do - expect(Fabricate.build(:package).bagger_profile).not_to be_nil - end - end - describe "#to_param" do it "uses the bag id" do bag_id = "made_up" From b21fad25167a7e44eee0594d56ae67e883256f31 Mon Sep 17 00:00:00 2001 From: Bryan Hockey Date: Thu, 29 Aug 2019 04:00:34 -0500 Subject: [PATCH 19/26] Treat Package, Deposit as depositable --- lib/chipmunk/incoming_storage.rb | 24 ++++++++++----------- spec/models/deposit_spec.rb | 4 ++++ spec/models/package_spec.rb | 4 ++++ spec/support/examples/a_depositable_item.rb | 11 ++++++++++ 4 files changed, 31 insertions(+), 12 deletions(-) create mode 100644 spec/support/examples/a_depositable_item.rb diff --git a/lib/chipmunk/incoming_storage.rb b/lib/chipmunk/incoming_storage.rb index 332250a7..1d60e254 100644 --- a/lib/chipmunk/incoming_storage.rb +++ b/lib/chipmunk/incoming_storage.rb @@ -10,34 +10,34 @@ module Chipmunk class IncomingStorage # Create an IncomingStorage instance. - # @param volume [Volume] The Volume from which the deposited packages should be - # ingested + # @param volume [Volume] The Volume from which the deposited depositable items + # should be ingested # @param paths [PathBuilder] A PathBuilder that returns a path on disk to the user - # upload location for a deposit, for a given package. + # upload location for a deposit, for a given depositable item. # @param links [PathBuilder] A PathBuilder that returns an rsync destination to - # which the user should upload, for a given package. + # which the user should upload, for a given depositable item. def initialize(volume:, paths:, links:) @volume = volume @paths = paths @links = links end - def for(package) - volume.get(upload_path(package)) + def for(depositable) + volume.get(upload_path(depositable)) end - def include?(package) - volume.include?(upload_path(package)) + def include?(depositable) + volume.include?(upload_path(depositable)) end - def upload_link(package) - links.for(package) + def upload_link(depositable) + links.for(depositable) end private - def upload_path(package) - paths.for(package) + def upload_path(depositable) + paths.for(depositable) end attr_reader :volume, :paths, :links diff --git a/spec/models/deposit_spec.rb b/spec/models/deposit_spec.rb index 7bd33f0f..226f799e 100644 --- a/spec/models/deposit_spec.rb +++ b/spec/models/deposit_spec.rb @@ -1,6 +1,10 @@ require "rails_helper" RSpec.describe Deposit, type: :model do + it_behaves_like "a depositable item" do + let(:instance) { Fabricate.build(:deposit) } + end + it "has a valid fabricator" do expect(Fabricate.create(:deposit)).to be_valid end diff --git a/spec/models/package_spec.rb b/spec/models/package_spec.rb index 611ec310..262e220b 100644 --- a/spec/models/package_spec.rb +++ b/spec/models/package_spec.rb @@ -8,6 +8,10 @@ let(:storage_path) { Rails.application.config.upload["storage_path"] } let(:uuid) { "6d11833a-d5fd-44f8-9205-277218578901" } + it_behaves_like "a depositable item" do + let(:instance) { Fabricate.build(:package) } + end + [:bag_id, :user_id, :external_id, :format, :content_type].each do |field| it "#{field} is required" do expect(Fabricate.build(:package, field => nil)).not_to be_valid diff --git a/spec/support/examples/a_depositable_item.rb b/spec/support/examples/a_depositable_item.rb new file mode 100644 index 00000000..73da96c0 --- /dev/null +++ b/spec/support/examples/a_depositable_item.rb @@ -0,0 +1,11 @@ +RSpec.shared_examples "a depositable item" do + let(:instance) { described_class.new } + + it "#identifier is a String" do + expect(instance.identifier).to be_a_kind_of(String) + end + + it "#username is a String" do + expect(instance.username).to be_a_kind_of(String) + end +end From 723a7b72a7958cbf99afbe91c3e90911e8676cab Mon Sep 17 00:00:00 2001 From: Bryan Hockey Date: Thu, 29 Aug 2019 23:09:49 -0500 Subject: [PATCH 20/26] Prefer storage_format over format Rails overwrites "format" in the request's parameter hash based on the body of the request, e.g. json. We didn't notice this before because we set the format of the created Package to Chipmunk::Bag.format. Now that we want to get the format from the user, we run into this conflict. This works around the issue by renaming the field to storage_format It also removes the paranoid format check from Volume. That sort of check is now part of validation. --- app/controllers/v2/artifacts_controller.rb | 2 +- app/models/artifact.rb | 2 +- app/models/package.rb | 7 ++++ db/migrate/20190802204940_create_artifacts.rb | 2 +- db/schema.rb | 1 + lib/chipmunk/bag.rb | 12 +++++-- lib/chipmunk/bag/reader.rb | 4 +-- lib/chipmunk/package_storage.rb | 18 ++++------ lib/chipmunk/validation_service.rb | 11 +++--- lib/chipmunk/validator/format.rb | 19 ---------- lib/chipmunk/validator/storage_format.rb | 21 +++++++++++ lib/chipmunk/volume.rb | 6 ++-- spec/chipmunk/package_storage_spec.rb | 36 +++++++++---------- spec/chipmunk/validator/format_spec.rb | 20 ----------- .../chipmunk/validator/storage_format_spec.rb | 20 +++++++++++ spec/chipmunk/volume_spec.rb | 8 +++-- spec/fabricators/artifact_fabricactor.rb | 2 +- spec/models/artifact_spec.rb | 2 +- 18 files changed, 105 insertions(+), 88 deletions(-) delete mode 100644 lib/chipmunk/validator/format.rb create mode 100644 lib/chipmunk/validator/storage_format.rb delete mode 100644 spec/chipmunk/validator/format_spec.rb create mode 100644 spec/chipmunk/validator/storage_format_spec.rb diff --git a/app/controllers/v2/artifacts_controller.rb b/app/controllers/v2/artifacts_controller.rb index 6c495876..eaf5387d 100644 --- a/app/controllers/v2/artifacts_controller.rb +++ b/app/controllers/v2/artifacts_controller.rb @@ -38,7 +38,7 @@ def new_artifact(params) Artifact.new( id: params[:id], user: current_user, - format: params[:format], + storage_format: params[:storage_format], content_type: params[:content_type] ) end diff --git a/app/models/artifact.rb b/app/models/artifact.rb index 4ae65817..b2801a46 100644 --- a/app/models/artifact.rb +++ b/app/models/artifact.rb @@ -42,7 +42,7 @@ def self.of_any_type message: "must be a valid v4 uuid." } validates :user, presence: true - validates :format, presence: true # TODO this is a controlled vocabulary + validates :storage_format, presence: true # TODO this is a controlled vocabulary validates :content_type, presence: true # TODO this is a controlled vocabulary def stored? diff --git a/app/models/package.rb b/app/models/package.rb index dddf369d..5ce149ca 100644 --- a/app/models/package.rb +++ b/app/models/package.rb @@ -26,6 +26,13 @@ def self.policy_class PackagePolicy end + # Rails overrides the format param on requests, so we rename this to storage_format. + # This alias is for backwards compatibility + # alias_method :storage_format, :format + def storage_format + format + end + def username user.username end diff --git a/db/migrate/20190802204940_create_artifacts.rb b/db/migrate/20190802204940_create_artifacts.rb index 0e2c79bc..3bf227d1 100644 --- a/db/migrate/20190802204940_create_artifacts.rb +++ b/db/migrate/20190802204940_create_artifacts.rb @@ -2,8 +2,8 @@ class CreateArtifacts < ActiveRecord::Migration[5.1] def change create_table :artifacts, id: :uuid do |t| t.string :content_type, null: false - t.string :format, null: false t.string :storage_volume, null: true + t.string :storage_format, null: false t.references :user, null: false, index: false t.timestamps end diff --git a/db/schema.rb b/db/schema.rb index fac2936d..534eff6e 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -27,6 +27,7 @@ t.string "artifact_id", null: false t.integer "user_id", null: false t.string "status", null: false + t.text "error", default: "", null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false t.index ["user_id"], name: "index_deposits_on_user_id" diff --git a/lib/chipmunk/bag.rb b/lib/chipmunk/bag.rb index ba287c4f..39e6e3e1 100644 --- a/lib/chipmunk/bag.rb +++ b/lib/chipmunk/bag.rb @@ -6,13 +6,21 @@ module Chipmunk class Bag include SemanticLogger::Loggable + class << self + def format + "bag" + end + alias_method :storage_format, :format + end + def initialize(path, info = {}, _create = false) @bag = BagIt::Bag.new(path, info, _create) end - def self.format - "bag" + def format + self.class.format end + alias_method :storage_format, :format def path bag_dir.to_s diff --git a/lib/chipmunk/bag/reader.rb b/lib/chipmunk/bag/reader.rb index 35782931..1d6b11bb 100644 --- a/lib/chipmunk/bag/reader.rb +++ b/lib/chipmunk/bag/reader.rb @@ -5,8 +5,8 @@ def at(path) Chipmunk::Bag.new(path) end - def format - Bag.format + def storage_format + Bag.storage_format end end end diff --git a/lib/chipmunk/package_storage.rb b/lib/chipmunk/package_storage.rb index 8e309248..196ece67 100644 --- a/lib/chipmunk/package_storage.rb +++ b/lib/chipmunk/package_storage.rb @@ -26,22 +26,14 @@ def for(package) # Move the source archive into preservation storage and update the package's # storage_volume and storage_path accordingly. def write(package, source) - check_format!(package) - storage_path = storage_path_for(package) volume = destination_volume(package) + storage_path = storage_path_for(package) volume.write(source, storage_path) package.update(storage_volume: volume.name, storage_path: storage_path) end private - def check_format!(package) - unless package.format == Chipmunk::Bag.format - raise Chipmunk::UnsupportedFormatError, - "Package #{package.identifier} has invalid format: #{package.format}" - end - end - def storage_path_for(package) prefixes = package.identifier.match(/^(..)(..)(..).*/) raise "identifier too short: #{package.identifier}" unless prefixes @@ -50,9 +42,11 @@ def storage_path_for(package) end # We are defaulting everything to "bags" for now as the simplest resolution strategy. - def destination_volume(_package) + def destination_volume(package) volumes["bags"].tap do |volume| raise Chipmunk::VolumeNotFoundError, "Cannot find destination volume: bags" if volume.nil? + + unsupported_format!(volume, package) if volume.storage_format != package.storage_format end end @@ -60,12 +54,12 @@ def volume_for(package) volumes[package.storage_volume].tap do |volume| raise Chipmunk::VolumeNotFoundError, package.storage_volume if volume.nil? - unsupported_format!(volume, package) if volume.format != package.format + unsupported_format!(volume, package) if volume.storage_format != package.storage_format end end def unsupported_format!(volume, package) - raise Chipmunk::UnsupportedFormatError, "Volume #{volume.name} does not support #{package.format}" + raise Chipmunk::UnsupportedFormatError, "Volume #{volume.name} does not support #{package.storage_format}" end attr_reader :volumes diff --git a/lib/chipmunk/validation_service.rb b/lib/chipmunk/validation_service.rb index 4d5a6265..93f6473b 100644 --- a/lib/chipmunk/validation_service.rb +++ b/lib/chipmunk/validation_service.rb @@ -8,7 +8,7 @@ class ValidationService def validate(validatable) case validatable when Package - validate_package(validatable) + validate_with(validatable, package_validators(validatable)) else ValidationResult.new(["Did not recognize type #{validatable.class}"]) end @@ -16,10 +16,10 @@ def validate(validatable) private - def validate_package(package) - if incoming_storage.include?(package) - sip = incoming_storage.for(package) - ValidationResult.new(errors(package_validators(package), sip)) + def validate_with(validatable, validators) + if incoming_storage.include?(validatable) + sip = incoming_storage.for(validatable) + ValidationResult.new(errors(validators, sip)) else return ValidationResult.new(["Could not find an uploaded sip"]) end @@ -27,6 +27,7 @@ def validate_package(package) def package_validators(package) [ + Validator::StorageFormat.new(package.storage_format), Validator::BagConsistency.new, Validator::BagMatchesPackage.new(package), Validator::External.new(package), diff --git a/lib/chipmunk/validator/format.rb b/lib/chipmunk/validator/format.rb deleted file mode 100644 index 47191335..00000000 --- a/lib/chipmunk/validator/format.rb +++ /dev/null @@ -1,19 +0,0 @@ -require_relative "validator" - -module Chipmunk - module Validator - class Format < Validator - def initialize(format) - @format = format - end - - validates "the format matches", - condition: proc {|sip| sip.format == format }, - error: proc {|sip| "SIP #{sip.identifier} has invalid format: #{sip.format}" } - - private - - attr_reader :format - end - end -end diff --git a/lib/chipmunk/validator/storage_format.rb b/lib/chipmunk/validator/storage_format.rb new file mode 100644 index 00000000..fd7d4bc0 --- /dev/null +++ b/lib/chipmunk/validator/storage_format.rb @@ -0,0 +1,21 @@ +require_relative "validator" + +module Chipmunk + module Validator + class StorageFormat < Validator + def initialize(storage_format) + @storage_format = storage_format + end + + validates "the storage_format matches", + condition: proc {|sip| sip.storage_format == storage_format }, + error: proc {|sip| + "SIP #{sip.identifier} has invalid storage_format: #{sip.storage_format}" + } + + private + + attr_reader :storage_format + end + end +end diff --git a/lib/chipmunk/volume.rb b/lib/chipmunk/volume.rb index a211751f..15aab345 100644 --- a/lib/chipmunk/volume.rb +++ b/lib/chipmunk/volume.rb @@ -62,15 +62,15 @@ def include?(path) # @!attribute [r] # @return [String] the format name of the items in this volume - def format - reader.format + def storage_format + reader.storage_format end + alias_method :format, :storage_format private def validate! raise ArgumentError, "Volume name must not be blank" if name.strip.empty? - raise ArgumentError, "Volume format must not be blank" if format.to_s.strip.empty? raise ArgumentError, "Volume root_path must be absolute (#{root_path})" unless root_path.absolute? raise ArgumentError, "Volume must specify a reader" unless reader raise ArgumentError, "Volume must specify a writer" unless writer diff --git a/spec/chipmunk/package_storage_spec.rb b/spec/chipmunk/package_storage_spec.rb index 3f1e3f23..065774fb 100644 --- a/spec/chipmunk/package_storage_spec.rb +++ b/spec/chipmunk/package_storage_spec.rb @@ -2,19 +2,19 @@ RSpec.describe Chipmunk::PackageStorage do FakeBag = Struct.new(:storage_path) do - def self.format + def self.storage_format "bag" end end FakeZip = Struct.new(:storage_path) do - def self.format + def self.storage_format "zip" end end class FakeBagReader - def format + def storage_format "bag" end @@ -24,7 +24,7 @@ def at(path) end class FakeZipReader - def format + def storage_format "zip" end @@ -72,14 +72,14 @@ def write(_obj, _path) allow(zips).to receive(:include?).with("/a-zip").and_return true end - context "with two formats registered: bag and zip" do + context "with two storage_formats registered: bag and zip" do let(:storage) { described_class.new(volumes: [bags, zips]) } - let(:formats) { { bag: FakeBag, zip: FakeZip } } - let(:bag) { stored_package(format: "bag", storage_volume: "bags", storage_path: "/a-bag") } - let(:zip) { stored_package(format: "zip", storage_volume: "zips", storage_path: "/a-zip") } - let(:transient) { unstored_package(format: "bag", id: "abcdef-123456") } - let(:badvolume) { stored_package(format: "bag", storage_volume: "notfound") } + let(:storage_formats) { { bag: FakeBag, zip: FakeZip } } + let(:bag) { stored_package(storage_format: "bag", storage_volume: "bags", storage_path: "/a-bag") } + let(:zip) { stored_package(storage_format: "zip", storage_volume: "zips", storage_path: "/a-zip") } + let(:transient) { unstored_package(storage_format: "bag", id: "abcdef-123456") } + let(:badvolume) { stored_package(storage_format: "bag", storage_volume: "notfound") } let(:bag_proxy) { storage.for(bag) } let(:zip_proxy) { storage.for(zip) } @@ -113,7 +113,7 @@ def write(_obj, _path) context "with a good bag" do subject(:storage) { described_class.new(volumes: [bags]) } - let(:package) { spy(:package, format: "bag", identifier: "abcdef-123456") } + let(:package) { spy(:package, storage_format: "bag", identifier: "abcdef-123456") } let(:disk_bag) { double(:bag, path: "/uploaded/abcdef-123456") } it "moves the source bag to the destination directory" do @@ -144,7 +144,7 @@ def write(_obj, _path) context "with a badly identified bag (shorter than 6 chars)" do subject(:storage) { described_class.new(volumes: [bags]) } - let(:package) { double(:package, format: "bag", identifier: "ab12") } + let(:package) { double(:package, storage_format: "bag", identifier: "ab12") } let(:disk_bag) { double(:bag, path: "/uploaded/ab12") } it "raises an exception" do @@ -152,10 +152,10 @@ def write(_obj, _path) end end - context "with an unsupported archive format" do + context "with an unsupported archive storage_format" do subject(:storage) { described_class.new(volumes: [bags]) } - let(:package) { double(:package, format: "junk", identifier: "id") } + let(:package) { double(:package, storage_format: "junk", identifier: "id") } let(:archive) { double(:archive) } it "raises an Unsupported Format error" do @@ -164,11 +164,11 @@ def write(_obj, _path) end end - def stored_package(format:, storage_volume: "test", storage_path: "/path") - double(:package, stored?: true, format: format.to_s, storage_volume: storage_volume, storage_path: storage_path) + def stored_package(storage_format:, storage_volume: "test", storage_path: "/path") + double(:package, stored?: true, storage_format: storage_format.to_s, storage_volume: storage_volume, storage_path: storage_path) end - def unstored_package(format:, id:) - double(:package, stored?: false, storage_volume: nil, storage_path: nil, format: format, identifier: id) + def unstored_package(storage_format:, id:) + double(:package, stored?: false, storage_volume: nil, storage_path: nil, storage_format: storage_format, identifier: id) end end diff --git a/spec/chipmunk/validator/format_spec.rb b/spec/chipmunk/validator/format_spec.rb deleted file mode 100644 index 38c25dd0..00000000 --- a/spec/chipmunk/validator/format_spec.rb +++ /dev/null @@ -1,20 +0,0 @@ -RSpec.describe Chipmunk::Validator::Format do - let(:format) { "someformat" } - let(:validator) { described_class.new(format) } - - context "when the format matches" do - let(:sip) { double(:sip, format: format) } - it "is valid" do - expect(validator.valid?(sip)).to be true - end - end - - context "when the format diverges" do - let(:sip) { double(:sip, identifier: "someid", format: "sandwich") } - - it "reports the error" do - expect(validator.errors(sip)) - .to include("SIP someid has invalid format: sandwich") - end - end -end diff --git a/spec/chipmunk/validator/storage_format_spec.rb b/spec/chipmunk/validator/storage_format_spec.rb new file mode 100644 index 00000000..5a79ee4c --- /dev/null +++ b/spec/chipmunk/validator/storage_format_spec.rb @@ -0,0 +1,20 @@ +RSpec.describe Chipmunk::Validator::StorageFormat do + let(:storage_format) { "somestorage_format" } + let(:validator) { described_class.new(storage_format) } + + context "when the storage_format matches" do + let(:sip) { double(:sip, storage_format: storage_format) } + it "is valid" do + expect(validator.valid?(sip)).to be true + end + end + + context "when the storage_format diverges" do + let(:sip) { double(:sip, identifier: "someid", storage_format: "sandwich") } + + it "reports the error" do + expect(validator.errors(sip)) + .to include("SIP someid has invalid storage_format: sandwich") + end + end +end diff --git a/spec/chipmunk/volume_spec.rb b/spec/chipmunk/volume_spec.rb index 130c3933..32ee6cba 100644 --- a/spec/chipmunk/volume_spec.rb +++ b/spec/chipmunk/volume_spec.rb @@ -15,8 +15,8 @@ context "when given valid attributes" do let(:name) { "vol" } let(:proxy) { double(:storage_proxy) } - let(:reader) { double(:reader, at: proxy, format: "some-pkg") } - let(:writer) { double(:writer, write: nil, format: "some-pkg") } + let(:reader) { double(:reader, at: proxy, storage_format: "some-pkg") } + let(:writer) { double(:writer, write: nil) } let(:root_path) { "/path" } it "has the correct name" do @@ -27,6 +27,10 @@ expect(volume.format).to eq "some-pkg" end + it "has the correct storage_format" do + expect(volume.storage_format).to eq "some-pkg" + end + it "has the correct root path" do expect(volume.root_path).to eq Pathname("/path") end diff --git a/spec/fabricators/artifact_fabricactor.rb b/spec/fabricators/artifact_fabricactor.rb index 9a22d6d9..1c6443fd 100644 --- a/spec/fabricators/artifact_fabricactor.rb +++ b/spec/fabricators/artifact_fabricactor.rb @@ -1,7 +1,7 @@ Fabricator(:artifact) do id { SecureRandom.uuid } user { Fabricate(:user) } - format { ["bag", "bag:versioned"].sample } + storage_format { ["bag", "bag:versioned"].sample } content_type { ["digital", "audio"].sample } revisions [] end diff --git a/spec/models/artifact_spec.rb b/spec/models/artifact_spec.rb index 1a36f97a..c5f483b9 100644 --- a/spec/models/artifact_spec.rb +++ b/spec/models/artifact_spec.rb @@ -4,7 +4,7 @@ let(:id) { SecureRandom.uuid } let(:user) { Fabricate.create(:user) } let(:content_type) { ["digital", "audio"].sample } - let(:format) { ["bag", "bag:versioned"].sample } + let(:storage_format) { ["bag", "bag:versioned"].sample } it "has a valid fabricator" do expect(Fabricate.create(:artifact)).to be_valid From 6e4d312337e1e8bfea528703d778ee788c8bef9d Mon Sep 17 00:00:00 2001 From: Bryan Hockey Date: Thu, 29 Aug 2019 23:16:29 -0500 Subject: [PATCH 21/26] Reduce BagMoveJob's knowledge of PackageStorage, QueueItem * Provide hook in PackageStorage for on-success behavior * Use said hook * Define methods on QueueItem to replace calls to update() previously used in BagMoveJob. These changes allow for greater reuse of PackageStorage. --- app/jobs/bag_move_job.rb | 14 ++++++-------- app/models/queue_item.rb | 8 ++++++++ lib/chipmunk/package_storage.rb | 12 ++++++++++-- spec/chipmunk/package_storage_spec.rb | 17 +++++++++-------- spec/jobs/bag_move_job_spec.rb | 16 ++++++++++------ 5 files changed, 43 insertions(+), 24 deletions(-) diff --git a/app/jobs/bag_move_job.rb b/app/jobs/bag_move_job.rb index 153891f0..29f93222 100644 --- a/app/jobs/bag_move_job.rb +++ b/app/jobs/bag_move_job.rb @@ -37,23 +37,21 @@ def validate def move_bag source = incoming_storage.for(package) - package_storage.write(package, source) + package_storage.write(package, source) do |volume, storage_path| + package.storage_volume = volume.name + package.storage_path = storage_path + end end def record_success queue_item.transaction do - queue_item.status = :done - queue_item.save! + queue_item.success! package.save! end end def record_failure - queue_item.transaction do - queue_item.error = errors.join("\n\n") - queue_item.status = :failed - queue_item.save! - end + queue_item.fail!(errors) end def log_exception(exception) diff --git a/app/models/queue_item.rb b/app/models/queue_item.rb index 1dcc2a1e..0bdca2bb 100644 --- a/app/models/queue_item.rb +++ b/app/models/queue_item.rb @@ -19,6 +19,14 @@ def user package.user end + def success! + update!(status: :done) + end + + def fail!(errors) + update!(status: :failed, error: errors.join("\n\n")) + end + scope :for_package, ->(package_id) { where(package_id: package_id) unless package_id.blank? } scope :for_packages, ->(package_scope) { joins(:package).merge(package_scope) } end diff --git a/lib/chipmunk/package_storage.rb b/lib/chipmunk/package_storage.rb index 196ece67..0267c6f5 100644 --- a/lib/chipmunk/package_storage.rb +++ b/lib/chipmunk/package_storage.rb @@ -20,7 +20,15 @@ def initialize(volumes:) def for(package) raise Chipmunk::PackageNotStoredError, package unless package.stored? - volume_for(package).get(package.storage_path) + # This is backwards compatibility for Package#storage_path. Otherwise, we + # construct the storage path from the package's identifier. Package#storage_path + # is safe to remove now, but is nontrivial to do so. + storage_path = if package.respond_to?(:storage_path) + package.storage_path + else + storage_path_for(package) + end + volume_for(package).get(storage_path) end # Move the source archive into preservation storage and update the package's @@ -29,7 +37,7 @@ def write(package, source) volume = destination_volume(package) storage_path = storage_path_for(package) volume.write(source, storage_path) - package.update(storage_volume: volume.name, storage_path: storage_path) + yield volume, storage_path end private diff --git a/spec/chipmunk/package_storage_spec.rb b/spec/chipmunk/package_storage_spec.rb index 065774fb..7671830b 100644 --- a/spec/chipmunk/package_storage_spec.rb +++ b/spec/chipmunk/package_storage_spec.rb @@ -126,18 +126,19 @@ def write(_obj, _path) # testing. For the time being, I have left the expectation with the original # specificity. expect(bags).to receive(:write) - storage.write(package, disk_bag) + storage.write(package, disk_bag) {} end - it "sets the storage_volume" do - expect(package).to receive(:update).with(a_hash_including(storage_volume: "bags")) - storage.write(package, disk_bag) + it "yields the storage_volume" do + storage.write(package, disk_bag) do |actual_storage_volume, _| + expect(actual_storage_volume).to eql(bags) + end end - it "sets the storage_path with three levels of hierarchy" do - expect(package).to receive(:update) - .with(a_hash_including(storage_path: "/ab/cd/ef/abcdef-123456")) - storage.write(package, disk_bag) + it "yields the storage_path" do + storage.write(package, disk_bag) do |_, actual_storage_path| + expect(actual_storage_path).to eql("/ab/cd/ef/abcdef-123456") + end end end diff --git a/spec/jobs/bag_move_job_spec.rb b/spec/jobs/bag_move_job_spec.rb index 71561d87..df78e67b 100644 --- a/spec/jobs/bag_move_job_spec.rb +++ b/spec/jobs/bag_move_job_spec.rb @@ -8,14 +8,13 @@ describe "#perform" do let(:bag) { double(:bag, path: "/uploaded/bag") } + let(:volume) { double(:volume, name: "bags") } before(:each) do allow(Services.validation).to receive(:validate).with(package).and_return(validation_result) allow(Services.incoming_storage).to receive(:for).with(package).and_return(bag) - allow(Services.storage).to receive(:write).with(package, bag) do |pkg, _bag| - pkg.storage_volume = "bags" - pkg.storage_path = "/storage/path/to/#{pkg.bag_id}" - end + allow(Services.storage).to receive(:write).with(package, bag) + .and_yield(volume, "/storage/path/to/#{package.bag_id}") end context "when the package is valid" do @@ -32,12 +31,17 @@ expect(queue_item.status).to eql("done") end - # TODO: Make sure that the destination volume is set properly, not literally; see PFDR-185 - it "sets the package storage_volume to root" do + it "sets the package storage_volume" do run_job expect(queue_item.package.storage_volume).to eql("bags") end + it "sets the package storage_path" do + run_job + expect(package.storage_path) + .to eql("/storage/path/to/#{package.bag_id}") + end + context "but the move fails" do before(:each) do allow(Services.storage).to receive(:write).with(package, bag).and_raise "test move failed" From 58b04790c4fba2a015dea9442dabf96dcdd00179 Mon Sep 17 00:00:00 2001 From: Bryan Hockey Date: Thu, 29 Aug 2019 23:20:06 -0500 Subject: [PATCH 22/26] ValidationService recognizes Deposit --- lib/chipmunk/validation_service.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/lib/chipmunk/validation_service.rb b/lib/chipmunk/validation_service.rb index 93f6473b..5c99b7c7 100644 --- a/lib/chipmunk/validation_service.rb +++ b/lib/chipmunk/validation_service.rb @@ -9,6 +9,8 @@ def validate(validatable) case validatable when Package validate_with(validatable, package_validators(validatable)) + when Deposit + validate_with(validatable, deposit_validators(validatable)) else ValidationResult.new(["Did not recognize type #{validatable.class}"]) end @@ -25,6 +27,15 @@ def validate_with(validatable, validators) end end + # TODO: decide which validators we need + def deposit_validators(deposit) + [ + Validator::BagConsistency.new, + Validator::External.new(deposit), + Validator::BaggerProfile.new(deposit) + ] + end + def package_validators(package) [ Validator::StorageFormat.new(package.storage_format), From 386faea333746313abc24c5c65919ec62147f142 Mon Sep 17 00:00:00 2001 From: Bryan Hockey Date: Thu, 29 Aug 2019 23:15:19 -0500 Subject: [PATCH 23/26] Add FinishDeposit, FinishDepositJob --- app/jobs/finish_deposit_job.rb | 5 ++++ lib/chipmunk.rb | 1 + lib/chipmunk/finish_deposit.rb | 54 ++++++++++++++++++++++++++++++++++ 3 files changed, 60 insertions(+) create mode 100644 app/jobs/finish_deposit_job.rb create mode 100644 lib/chipmunk/finish_deposit.rb diff --git a/app/jobs/finish_deposit_job.rb b/app/jobs/finish_deposit_job.rb new file mode 100644 index 00000000..a2e66750 --- /dev/null +++ b/app/jobs/finish_deposit_job.rb @@ -0,0 +1,5 @@ +class FinishDepositJob < ApplicationJob + def perform(deposit) + Chipmunk::FinishDeposit.new(deposit).run + end +end diff --git a/lib/chipmunk.rb b/lib/chipmunk.rb index ef48e146..4460d6a2 100644 --- a/lib/chipmunk.rb +++ b/lib/chipmunk.rb @@ -17,3 +17,4 @@ module Chipmunk require_relative "chipmunk/volume" require_relative "chipmunk/validator" require_relative "chipmunk/validation_service" +require_relative "chipmunk/finish_deposit" diff --git a/lib/chipmunk/finish_deposit.rb b/lib/chipmunk/finish_deposit.rb new file mode 100644 index 00000000..01c32845 --- /dev/null +++ b/lib/chipmunk/finish_deposit.rb @@ -0,0 +1,54 @@ +module Chipmunk + class FinishDeposit + def initialize(deposit) + @deposit = deposit + @errors = [] + end + + def run + ingest! or record_failure + end + + private + + attr_reader :deposit, :errors + + def ingest! + validate and move_sip and record_success + rescue StandardError => error + log_exception(error) + end + + def validate + result = Services.validation.validate(deposit) + errors.concat result.errors + result.valid? + end + + def move_sip + sip = Services.incoming_storage.for(deposit) + Services.storage.write(deposit.artifact, sip) do |volume| + deposit.artifact.storage_volume = volume.name + end + end + + def record_success + deposit.transaction do + Revision.create!(deposit: deposit, artifact: deposit.artifact) + deposit.complete! + deposit.artifact.save! + end + end + + def record_failure + deposit.fail!(errors) + end + + def log_exception(exception) + errors << "#{exception.backtrace.first}: #{exception.message} (#{exception.class})" + errors << exception.backtrace.drop(1).map {|s| "\t#{s}" } + record_failure + end + + end +end From 28b645e190f224457195d49844b6a8b76d7beb43 Mon Sep 17 00:00:00 2001 From: Bryan Hockey Date: Thu, 29 Aug 2019 23:54:38 -0500 Subject: [PATCH 24/26] don't swear in deposit status --- app/attributes/deposit_status_type.rb | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/app/attributes/deposit_status_type.rb b/app/attributes/deposit_status_type.rb index 9adab796..22621286 100644 --- a/app/attributes/deposit_status_type.rb +++ b/app/attributes/deposit_status_type.rb @@ -1,26 +1,15 @@ +# frozen_string_literal: true + class DepositStatusType < ActiveRecord::Type::Value def cast(value) - puts "#cast(#{value} #[#{value.class}]) from #{caller[0]}" - # if value.is_a? Chipmunk::DepositStatus - # value - # else - # Chipmunk::DepositStatus.new(value) - # end super(value.to_s) end - # this is the default implementation - # def deserialize(value_from_db) - # cast(value_from_db) - # end def deserialize(value) - puts "#deserialize(#{value} #[#{value.class}]) from #{caller[0]}" Chipmunk::DepositStatus.new(value) end - # So this just doesn't get fucking called def serialize(deposit_status) - puts "#serialize(#{deposit_status} #[#{deposit_status.class}]) from #{caller[0]}" deposit_status.to_s end From 9cd6d462bc7508d13c6038cbd80bf0e0ca3d79da Mon Sep 17 00:00:00 2001 From: Bryan Hockey Date: Fri, 30 Aug 2019 00:01:16 -0500 Subject: [PATCH 25/26] Rubocop Rubocoping all at once is far from ideal. --- app/controllers/v2/artifacts_controller.rb | 2 +- app/controllers/v2/deposits_controller.rb | 2 ++ app/controllers/v2/revisions_controller.rb | 2 ++ app/jobs/finish_deposit_job.rb | 2 ++ app/models/artifact.rb | 7 ++++--- app/models/deposit.rb | 8 +++++--- app/models/revision.rb | 2 ++ config/initializers/services.rb | 2 +- config/routes.rb | 3 ++- db/migrate/20190802204940_create_artifacts.rb | 2 ++ db/migrate/20190807173150_create_deposits.rb | 2 ++ db/migrate/20190813153819_create_revisions.rb | 2 ++ lib/chipmunk/bag/move_writer.rb | 2 ++ lib/chipmunk/bag/reader.rb | 2 ++ lib/chipmunk/deposit_status.rb | 3 +++ lib/chipmunk/finish_deposit.rb | 10 ++++++---- lib/chipmunk/validation_result.rb | 2 ++ lib/chipmunk/validation_service.rb | 4 +++- lib/chipmunk/validator.rb | 4 +++- lib/chipmunk/validator/bag_consistency.rb | 2 ++ lib/chipmunk/validator/bag_matches_package.rb | 7 +++++-- lib/chipmunk/validator/bagger_profile.rb | 6 ++++-- lib/chipmunk/validator/external.rb | 4 +++- lib/chipmunk/validator/storage_format.rb | 2 ++ lib/chipmunk/validator/validator.rb | 4 +++- spec/chipmunk/bag/move_writer_spec.rb | 3 +++ spec/chipmunk/incoming_storage_spec.rb | 12 ++++++++++-- spec/chipmunk/package_storage_spec.rb | 2 +- spec/chipmunk/validator/bag_consistency_spec.rb | 14 ++++++++------ .../chipmunk/validator/bag_matches_package_spec.rb | 3 +++ spec/chipmunk/validator/bagger_profile_spec.rb | 2 ++ spec/chipmunk/validator/external_spec.rb | 3 ++- spec/chipmunk/validator/storage_format_spec.rb | 3 +++ spec/chipmunk/validator/validator_spec.rb | 12 +++++++----- spec/chipmunk/volume_spec.rb | 4 ++-- spec/fabricators/artifact_fabricactor.rb | 2 ++ spec/fabricators/deposit_fabricator.rb | 2 ++ spec/fabricators/revision_fabricator.rb | 2 ++ spec/jobs/bag_move_job_spec.rb | 3 ++- spec/models/artifact_spec.rb | 7 ++++--- spec/models/deposit_spec.rb | 2 ++ spec/models/revision_spec.rb | 3 ++- spec/support/examples/a_depositable_item.rb | 2 ++ 43 files changed, 126 insertions(+), 43 deletions(-) diff --git a/app/controllers/v2/artifacts_controller.rb b/app/controllers/v2/artifacts_controller.rb index eaf5387d..5c6d3852 100644 --- a/app/controllers/v2/artifacts_controller.rb +++ b/app/controllers/v2/artifacts_controller.rb @@ -18,7 +18,7 @@ def create collection_policy.new(current_user).authorize! :new? # We don't explicitly check for :save? permissions - if duplicate = Artifact.find_by(id: params[:id]) + if (duplicate = Artifact.find_by(id: params[:id])) resource_policy.new(current_user, duplicate).authorize! :show? head 303, location: v2_artifact_path(duplicate) else diff --git a/app/controllers/v2/deposits_controller.rb b/app/controllers/v2/deposits_controller.rb index b8fe3e09..0edc8304 100644 --- a/app/controllers/v2/deposits_controller.rb +++ b/app/controllers/v2/deposits_controller.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module V2 class DepositsController < ResourceController diff --git a/app/controllers/v2/revisions_controller.rb b/app/controllers/v2/revisions_controller.rb index 83fe4e59..ac6fd783 100644 --- a/app/controllers/v2/revisions_controller.rb +++ b/app/controllers/v2/revisions_controller.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module V2 class RevisionsController < ResourceController diff --git a/app/jobs/finish_deposit_job.rb b/app/jobs/finish_deposit_job.rb index a2e66750..98f8b1d4 100644 --- a/app/jobs/finish_deposit_job.rb +++ b/app/jobs/finish_deposit_job.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + class FinishDepositJob < ApplicationJob def perform(deposit) Chipmunk::FinishDeposit.new(deposit).run diff --git a/app/models/artifact.rb b/app/models/artifact.rb index b2801a46..3f5a90a7 100644 --- a/app/models/artifact.rb +++ b/app/models/artifact.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + class Artifact < ApplicationRecord class AnyArtifact @@ -29,7 +31,6 @@ def self.of_any_type alias_method :identifier, :id - # Each artifact belongs to a single user belongs_to :user # Deposits are collections of zero or more revisions @@ -38,8 +39,8 @@ def self.of_any_type has_many :deposits validates :id, presence: true, - format: { with: Services.uuid_format, - message: "must be a valid v4 uuid." } + format: { with: Services.uuid_format, + message: "must be a valid v4 uuid." } validates :user, presence: true validates :storage_format, presence: true # TODO this is a controlled vocabulary diff --git a/app/models/deposit.rb b/app/models/deposit.rb index 4f16e657..6c0f5a0d 100644 --- a/app/models/deposit.rb +++ b/app/models/deposit.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + class Deposit < ApplicationRecord # Each deposit is created by a single user @@ -7,10 +9,10 @@ class Deposit < ApplicationRecord # TODO Could not get the attributes api to work with a custom type enum status: { - started: "started", - canceled: "cancelled", + started: "started", + canceled: "cancelled", ingesting: "ingesting", - failed: "failed", + failed: "failed", completed: "completed" } diff --git a/app/models/revision.rb b/app/models/revision.rb index d7796e4e..8a50a9b8 100644 --- a/app/models/revision.rb +++ b/app/models/revision.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + class Revision < ApplicationRecord belongs_to :artifact belongs_to :deposit diff --git a/config/initializers/services.rb b/config/initializers/services.rb index ad0f76da..7143417e 100644 --- a/config/initializers/services.rb +++ b/config/initializers/services.rb @@ -73,5 +73,5 @@ def assign_db(lhs, rhs) Services.register(:notary) { Keycard::Notary.default } Services.register(:uuid_format) do - /\A[0-9A-F]{8}-[0-9A-F]{4}-4[0-9A-F]{3}-[89AB][0-9A-F]{3}-[0-9A-F]{12}\z/i + /\A[0-9A-F]{8}-[0-9A-F]{4}-4[0-9A-F]{3}-[89AB][0-9A-F]{3}-[0-9A-F]{12}\z/i end diff --git a/config/routes.rb b/config/routes.rb index 9739e71e..6b1a9601 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -36,6 +36,7 @@ !request.xhr? && request.format.html? end - root to: "application#fallback_index_html", constraints: ->(request) do !request.xhr? && request.format.html? + root to: "application#fallback_index_html", constraints: ->(request) do + !request.xhr? && request.format.html? end end diff --git a/db/migrate/20190802204940_create_artifacts.rb b/db/migrate/20190802204940_create_artifacts.rb index 3bf227d1..52501572 100644 --- a/db/migrate/20190802204940_create_artifacts.rb +++ b/db/migrate/20190802204940_create_artifacts.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + class CreateArtifacts < ActiveRecord::Migration[5.1] def change create_table :artifacts, id: :uuid do |t| diff --git a/db/migrate/20190807173150_create_deposits.rb b/db/migrate/20190807173150_create_deposits.rb index ea698aac..5286c770 100644 --- a/db/migrate/20190807173150_create_deposits.rb +++ b/db/migrate/20190807173150_create_deposits.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + class CreateDeposits < ActiveRecord::Migration[5.1] def change create_table :deposits do |t| diff --git a/db/migrate/20190813153819_create_revisions.rb b/db/migrate/20190813153819_create_revisions.rb index dcfae8be..8b2862bd 100644 --- a/db/migrate/20190813153819_create_revisions.rb +++ b/db/migrate/20190813153819_create_revisions.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + class CreateRevisions < ActiveRecord::Migration[5.1] def change create_table :revisions do |t| diff --git a/lib/chipmunk/bag/move_writer.rb b/lib/chipmunk/bag/move_writer.rb index ebff00d0..6b04cf54 100644 --- a/lib/chipmunk/bag/move_writer.rb +++ b/lib/chipmunk/bag/move_writer.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module Chipmunk class Bag diff --git a/lib/chipmunk/bag/reader.rb b/lib/chipmunk/bag/reader.rb index 1d6b11bb..459eed01 100644 --- a/lib/chipmunk/bag/reader.rb +++ b/lib/chipmunk/bag/reader.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module Chipmunk class Bag class Reader diff --git a/lib/chipmunk/deposit_status.rb b/lib/chipmunk/deposit_status.rb index 71efc332..41e2fe32 100644 --- a/lib/chipmunk/deposit_status.rb +++ b/lib/chipmunk/deposit_status.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module Chipmunk class DepositStatus @@ -54,6 +56,7 @@ def dead? end private + attr_reader :value end diff --git a/lib/chipmunk/finish_deposit.rb b/lib/chipmunk/finish_deposit.rb index 01c32845..281e058b 100644 --- a/lib/chipmunk/finish_deposit.rb +++ b/lib/chipmunk/finish_deposit.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module Chipmunk class FinishDeposit def initialize(deposit) @@ -6,7 +8,7 @@ def initialize(deposit) end def run - ingest! or record_failure + ingest! || record_failure end private @@ -14,9 +16,9 @@ def run attr_reader :deposit, :errors def ingest! - validate and move_sip and record_success - rescue StandardError => error - log_exception(error) + validate && move_sip && record_success + rescue StandardError => e + log_exception(e) end def validate diff --git a/lib/chipmunk/validation_result.rb b/lib/chipmunk/validation_result.rb index b5f856ec..294e43dc 100644 --- a/lib/chipmunk/validation_result.rb +++ b/lib/chipmunk/validation_result.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module Chipmunk # The result of a validation attempt diff --git a/lib/chipmunk/validation_service.rb b/lib/chipmunk/validation_service.rb index 5c99b7c7..e33cdcbf 100644 --- a/lib/chipmunk/validation_service.rb +++ b/lib/chipmunk/validation_service.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require_relative "validation_result" module Chipmunk @@ -23,7 +25,7 @@ def validate_with(validatable, validators) sip = incoming_storage.for(validatable) ValidationResult.new(errors(validators, sip)) else - return ValidationResult.new(["Could not find an uploaded sip"]) + ValidationResult.new(["Could not find an uploaded sip"]) end end diff --git a/lib/chipmunk/validator.rb b/lib/chipmunk/validator.rb index e668dc59..dcfe8711 100644 --- a/lib/chipmunk/validator.rb +++ b/lib/chipmunk/validator.rb @@ -1 +1,3 @@ -Dir[File.join(__dir__, 'validator', '**', '*.rb')].each { |file| require_relative file } +# frozen_string_literal: true + +Dir[File.join(__dir__, "validator", "**", "*.rb")].each {|file| require_relative file } diff --git a/lib/chipmunk/validator/bag_consistency.rb b/lib/chipmunk/validator/bag_consistency.rb index 79e24915..abbfda56 100644 --- a/lib/chipmunk/validator/bag_consistency.rb +++ b/lib/chipmunk/validator/bag_consistency.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require_relative "validator" module Chipmunk diff --git a/lib/chipmunk/validator/bag_matches_package.rb b/lib/chipmunk/validator/bag_matches_package.rb index 4868d7d6..d68b8dc8 100644 --- a/lib/chipmunk/validator/bag_matches_package.rb +++ b/lib/chipmunk/validator/bag_matches_package.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require_relative "validator" module Chipmunk @@ -14,14 +16,15 @@ def initialize(package) { "External-Identifier" => :external_id, "Bag-ID" => :bag_id, - "Chipmunk-Content-Type" => :content_type, + "Chipmunk-Content-Type" => :content_type }.each_pair do |file_key, db_key| validates "#{file_key} in bag on disk matches bag in database", condition: proc {|bag| bag.chipmunk_info[file_key] == package.public_send(db_key) }, - error: proc {"uploaded #{file_key} does not match expected value #{package.public_send(db_key)}"} + error: proc { "uploaded #{file_key} does not match expected value #{package.public_send(db_key)}" } end private + attr_reader :package end diff --git a/lib/chipmunk/validator/bagger_profile.rb b/lib/chipmunk/validator/bagger_profile.rb index df6cf1cc..f73fc8f2 100644 --- a/lib/chipmunk/validator/bagger_profile.rb +++ b/lib/chipmunk/validator/bagger_profile.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require_relative "validator" module Chipmunk @@ -16,8 +18,8 @@ def initialize(package) precondition: proc {|bag| [].tap {|errors| profile.valid?(bag.bag_info, errors: errors) } }, - condition: proc {|bag, errors| errors.empty? }, - error: proc {|bag, errors| errors } + condition: proc {|_bag, errors| errors.empty? }, + error: proc {|_bag, errors| errors } private diff --git a/lib/chipmunk/validator/external.rb b/lib/chipmunk/validator/external.rb index 8bddf24f..9f84959b 100644 --- a/lib/chipmunk/validator/external.rb +++ b/lib/chipmunk/validator/external.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require_relative "validator" require "open3" @@ -24,7 +26,7 @@ def command only_if: proc {|validatable| !command.nil? && validatable.valid? }, precondition: proc { Open3.capture3(command) }, condition: proc {|_, _, _, status| status.exitstatus.zero? }, - error: proc {|_, _, stderr, _| "Error validating content\n" + stderr } + error: proc {|_, _, stderr, _| "Error validating content\n" + stderr } private diff --git a/lib/chipmunk/validator/storage_format.rb b/lib/chipmunk/validator/storage_format.rb index fd7d4bc0..df2db129 100644 --- a/lib/chipmunk/validator/storage_format.rb +++ b/lib/chipmunk/validator/storage_format.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require_relative "validator" module Chipmunk diff --git a/lib/chipmunk/validator/validator.rb b/lib/chipmunk/validator/validator.rb index 78107c61..1745a1e9 100644 --- a/lib/chipmunk/validator/validator.rb +++ b/lib/chipmunk/validator/validator.rb @@ -1,7 +1,8 @@ +# frozen_string_literal: true + module Chipmunk module Validator - # A validator runs compile-time validations against a validatable object # passed to #valid? or #errors during run-time. Validations are created # with the DSL defined in this class via the ::validates method. Subclasses @@ -32,6 +33,7 @@ def validations def validates(_desc = "", only_if: proc { true }, precondition: proc {}, condition:, error:) validations << lambda do |validatable| return unless instance_exec(validatable, &only_if) + precond_result = instance_exec(validatable, &precondition) unless instance_exec(validatable, *precond_result, &condition) instance_exec(validatable, *precond_result, &error) diff --git a/spec/chipmunk/bag/move_writer_spec.rb b/spec/chipmunk/bag/move_writer_spec.rb index e0f7714f..f55f06ce 100644 --- a/spec/chipmunk/bag/move_writer_spec.rb +++ b/spec/chipmunk/bag/move_writer_spec.rb @@ -1,7 +1,10 @@ +# frozen_string_literal: true + # These tests were ported over from PackageStorage's specs. RSpec.describe Chipmunk::Bag::MoveWriter do let(:bag) { double(:bag, path: "/some/bag/path") } let(:path) { "/bag/goes/in/here" } + subject(:writer) { described_class.new } before(:each) do diff --git a/spec/chipmunk/incoming_storage_spec.rb b/spec/chipmunk/incoming_storage_spec.rb index 541f6b1a..44dbc72b 100644 --- a/spec/chipmunk/incoming_storage_spec.rb +++ b/spec/chipmunk/incoming_storage_spec.rb @@ -5,8 +5,16 @@ let(:writer) { double(:writer, format: "some-pkg") } let(:volume) { Chipmunk::Volume.new(name: "incoming", writer: writer, reader: reader, root_path: "/incoming") } let(:path_builder) { Chipmunk::UploadPath.new("/") } - let(:unstored_package) { instance_double("Package", stored?: false, username: "uploader", bag_id: "abcdef-123456", identifier: "abcdef-123456") } - let(:stored_package) { instance_double("Package", stored?: true) } + let(:unstored_package) do + instance_double( + "Package", + stored?: false, + username: "uploader", + bag_id: "abcdef-123456", + identifier: "abcdef-123456" +) + end + let(:stored_package) { instance_double("Package", stored?: true) } subject(:storage) do described_class.new( diff --git a/spec/chipmunk/package_storage_spec.rb b/spec/chipmunk/package_storage_spec.rb index 7671830b..1bd8d219 100644 --- a/spec/chipmunk/package_storage_spec.rb +++ b/spec/chipmunk/package_storage_spec.rb @@ -75,7 +75,7 @@ def write(_obj, _path) context "with two storage_formats registered: bag and zip" do let(:storage) { described_class.new(volumes: [bags, zips]) } - let(:storage_formats) { { bag: FakeBag, zip: FakeZip } } + let(:storage_formats) { { bag: FakeBag, zip: FakeZip } } let(:bag) { stored_package(storage_format: "bag", storage_volume: "bags", storage_path: "/a-bag") } let(:zip) { stored_package(storage_format: "zip", storage_volume: "zips", storage_path: "/a-zip") } let(:transient) { unstored_package(storage_format: "bag", id: "abcdef-123456") } diff --git a/spec/chipmunk/validator/bag_consistency_spec.rb b/spec/chipmunk/validator/bag_consistency_spec.rb index cfd207c7..08052a33 100644 --- a/spec/chipmunk/validator/bag_consistency_spec.rb +++ b/spec/chipmunk/validator/bag_consistency_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + RSpec.describe Chipmunk::Validator::BagConsistency do let(:validator) { described_class.new } let(:tag_files) { [fixture("marc.xml")] } @@ -14,9 +16,9 @@ end let(:chipmunk_info) do { - "Metadata-Type" => "MARC", - "Metadata-URL" => "http://what.ever", - "Metadata-Tagfile" => "marc.xml" + "Metadata-Type" => "MARC", + "Metadata-URL" => "http://what.ever", + "Metadata-Tagfile" => "marc.xml" } end @@ -30,7 +32,7 @@ it "reports the errors from the bag" do expect(validator.errors(bag)).to include( - a_string_matching( /Error validating.*\n injected error$/) + a_string_matching(/Error validating.*\n injected error$/) ) end end @@ -56,8 +58,8 @@ context "when the bag has only some descriptive metadata tags" do let(:chipmunk_info) do { - "Metadata-URL" => "http://what.ever", - "Metadata-Tagfile" => "marc.xml" + "Metadata-URL" => "http://what.ever", + "Metadata-Tagfile" => "marc.xml" } end diff --git a/spec/chipmunk/validator/bag_matches_package_spec.rb b/spec/chipmunk/validator/bag_matches_package_spec.rb index 2ce36c3e..0c548967 100644 --- a/spec/chipmunk/validator/bag_matches_package_spec.rb +++ b/spec/chipmunk/validator/bag_matches_package_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require "securerandom" RSpec.describe Chipmunk::Validator::BagMatchesPackage do @@ -21,6 +23,7 @@ context "when everything matches" do let(:chipmunk_info) { good_chipmunk_info } + it { expect(validator.valid?(bag)).to be true } end diff --git a/spec/chipmunk/validator/bagger_profile_spec.rb b/spec/chipmunk/validator/bagger_profile_spec.rb index 2d2f6135..9d8546ee 100644 --- a/spec/chipmunk/validator/bagger_profile_spec.rb +++ b/spec/chipmunk/validator/bagger_profile_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + RSpec.describe Chipmunk::Validator::BaggerProfile do let(:validator) { described_class.new(package) } let(:bag) { double(:bag, bag_info: { "Baz" => "quux" }) } diff --git a/spec/chipmunk/validator/external_spec.rb b/spec/chipmunk/validator/external_spec.rb index 00e581b1..74f1c96e 100644 --- a/spec/chipmunk/validator/external_spec.rb +++ b/spec/chipmunk/validator/external_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + RSpec.describe Chipmunk::Validator::External do let(:content_type) { "mycontenttype" } let(:package) { double(:package, identifier: SecureRandom.uuid, content_type: content_type) } @@ -61,5 +63,4 @@ expect(validator.valid?(bag)).to be true end end - end diff --git a/spec/chipmunk/validator/storage_format_spec.rb b/spec/chipmunk/validator/storage_format_spec.rb index 5a79ee4c..c9ff28e7 100644 --- a/spec/chipmunk/validator/storage_format_spec.rb +++ b/spec/chipmunk/validator/storage_format_spec.rb @@ -1,9 +1,12 @@ +# frozen_string_literal: true + RSpec.describe Chipmunk::Validator::StorageFormat do let(:storage_format) { "somestorage_format" } let(:validator) { described_class.new(storage_format) } context "when the storage_format matches" do let(:sip) { double(:sip, storage_format: storage_format) } + it "is valid" do expect(validator.valid?(sip)).to be true end diff --git a/spec/chipmunk/validator/validator_spec.rb b/spec/chipmunk/validator/validator_spec.rb index 993e9880..7b5a7c5d 100644 --- a/spec/chipmunk/validator/validator_spec.rb +++ b/spec/chipmunk/validator/validator_spec.rb @@ -1,3 +1,4 @@ +# frozen_string_literal: true RSpec.describe Chipmunk::Validator::Validator do class OnlyIfValidator < described_class @@ -6,7 +7,7 @@ def initialize(only_if: true) end validates "For testing only_if", - only_if: proc { @only_if }, + only_if: proc { @only_if }, # rubocop:disable RSpec/InstanceVariable precondition: proc { true }, condition: proc { false }, error: proc { "injected_error" } @@ -19,14 +20,14 @@ def initialize(precond) validates "For testing preconditions", only_if: proc { true }, - precondition: proc { @precond }, + precondition: proc { @precond }, # rubocop:disable RSpec/InstanceVariable condition: proc {|_, precond_result| precond_result }, error: proc {|_, precond_result| "#{precond_result} injected_error" } end class BlockParamValidator < described_class validates "For testing that blocks receive the object being validated", - only_if: proc {|v| v.called_by << :only_if; true }, + only_if: proc {|v| v.called_by << :only_if; true }, precondition: proc {|v| v.called_by << :precondition; true }, condition: proc {|v| v.called_by << :condition; false }, error: proc {|v| v.called_by << :error; "some_error" } @@ -36,6 +37,7 @@ class BlockParamValidator < described_class context "when only_if is true" do let(:failure) { OnlyIfValidator.new(only_if: true) } + it "runs the validation" do expect(failure.valid?(validatable)).to be false end @@ -43,6 +45,7 @@ class BlockParamValidator < described_class context "when only_if is false" do let(:success) { OnlyIfValidator.new(only_if: false) } + it "skips the validation" do expect(success.valid?(validatable)).to be true end @@ -50,6 +53,7 @@ class BlockParamValidator < described_class context "with a precondition" do let(:failure) { PrecondValidator.new(false) } + it "passes the precondition to the condition" do expect(failure.valid?(validatable)).to be false end @@ -69,6 +73,4 @@ class BlockParamValidator < described_class .to contain_exactly(:only_if, :precondition, :condition, :error) end end - - end diff --git a/spec/chipmunk/volume_spec.rb b/spec/chipmunk/volume_spec.rb index 32ee6cba..9adc67d3 100644 --- a/spec/chipmunk/volume_spec.rb +++ b/spec/chipmunk/volume_spec.rb @@ -68,7 +68,7 @@ end context "when given a blank name" do - let(:name) { "" } + let(:name) { "" } let(:reader) { double(:reader, at: nil, format: "some-pkg") } let(:writer) { double(:writer, write: nil, format: "some-pkg") } let(:root_path) { "/path" } @@ -79,7 +79,7 @@ end context "when given a relative path" do - let(:name) { "vol" } + let(:name) { "vol" } let(:reader) { double(:reader, at: nil, format: "some-pkg") } let(:writer) { double(:writer, write: nil, format: "some-pkg") } let(:root_path) { "relative/path" } diff --git a/spec/fabricators/artifact_fabricactor.rb b/spec/fabricators/artifact_fabricactor.rb index 1c6443fd..fe2f7358 100644 --- a/spec/fabricators/artifact_fabricactor.rb +++ b/spec/fabricators/artifact_fabricactor.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + Fabricator(:artifact) do id { SecureRandom.uuid } user { Fabricate(:user) } diff --git a/spec/fabricators/deposit_fabricator.rb b/spec/fabricators/deposit_fabricator.rb index 970608d6..9968168e 100644 --- a/spec/fabricators/deposit_fabricator.rb +++ b/spec/fabricators/deposit_fabricator.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + Fabricator(:deposit) do user artifact diff --git a/spec/fabricators/revision_fabricator.rb b/spec/fabricators/revision_fabricator.rb index 63bb5545..23d326ec 100644 --- a/spec/fabricators/revision_fabricator.rb +++ b/spec/fabricators/revision_fabricator.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + Fabricator(:revision) do artifact deposit diff --git a/spec/jobs/bag_move_job_spec.rb b/spec/jobs/bag_move_job_spec.rb index df78e67b..eeb312bd 100644 --- a/spec/jobs/bag_move_job_spec.rb +++ b/spec/jobs/bag_move_job_spec.rb @@ -19,6 +19,7 @@ context "when the package is valid" do subject(:run_job) { described_class.perform_now(queue_item) } + let(:validation_result) { double(:validation_result, errors: [], valid?: true) } it "moves the bag" do @@ -65,6 +66,7 @@ context "when the package is invalid" do subject(:run_job) { described_class.perform_now(queue_item) } + let(:validation_result) { double(valid?: false, errors: ["my error"]) } it "does not move the bag" do @@ -82,6 +84,5 @@ expect(queue_item.error).to match(/my error/) end end - end end diff --git a/spec/models/artifact_spec.rb b/spec/models/artifact_spec.rb index c5f483b9..e5ef3f1e 100644 --- a/spec/models/artifact_spec.rb +++ b/spec/models/artifact_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require "rails_helper" RSpec.describe Artifact, type: :model do @@ -22,7 +24,7 @@ end it "must be a UUIDv4" do - expect(Fabricate.build(:artifact, id: "foo")).to_not be_valid + expect(Fabricate.build(:artifact, id: "foo")).not_to be_valid end end @@ -33,10 +35,9 @@ it "is true if it has 1 or more revisions" do artifact = Fabricate.build( :artifact, - revisions: [ Fabricate.build(:revision) ] + revisions: [Fabricate.build(:revision)] ) expect(artifact.stored?).to be true end end - end diff --git a/spec/models/deposit_spec.rb b/spec/models/deposit_spec.rb index 226f799e..f937e64d 100644 --- a/spec/models/deposit_spec.rb +++ b/spec/models/deposit_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require "rails_helper" RSpec.describe Deposit, type: :model do diff --git a/spec/models/revision_spec.rb b/spec/models/revision_spec.rb index c3cd84f8..94fa9d3e 100644 --- a/spec/models/revision_spec.rb +++ b/spec/models/revision_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require "rails_helper" RSpec.describe Revision, type: :model do @@ -8,5 +10,4 @@ it "has a valid fabriactor that doesn't save to the database" do expect(Fabricate.build(:revision)).to be_valid end - end diff --git a/spec/support/examples/a_depositable_item.rb b/spec/support/examples/a_depositable_item.rb index 73da96c0..29201914 100644 --- a/spec/support/examples/a_depositable_item.rb +++ b/spec/support/examples/a_depositable_item.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + RSpec.shared_examples "a depositable item" do let(:instance) { described_class.new } From 91712af13f5f5e036e3c117cf2df5ff94d807e74 Mon Sep 17 00:00:00 2001 From: Bryan Hockey Date: Sat, 31 Aug 2019 17:39:45 -0500 Subject: [PATCH 26/26] Force Travis-CI to actually run migrations Previously, this relied on Rails's automatic behavior to migrate the database, which seems to build the database from schema.rb. As schema.rb is not created correctly when the PK of a table is a uuid, it would skip creating the artifacts table. By forcing the migrations to run, we better test the production setup, and avoid this issue. --- .travis.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.travis.yml b/.travis.yml index 5b4a59e5..81d45a43 100644 --- a/.travis.yml +++ b/.travis.yml @@ -5,6 +5,9 @@ rvm: - 2.5 - 2.6 +env: + - RACK_ENV=test RAILS_ENV=test + branches: only: - master @@ -17,6 +20,7 @@ before_install: before_script: - bundle exec rake checkpoint:migrate - bundle exec rake keycard:migrate + - bin/rails db:migrate script: - bundle exec rspec