deposit_bag_validator class added #122

ndushay · 2018-03-06T22:27:26Z

This is a Shameless Green implementation of Moab::DepositBagValidator, to support the validate_bag robot in sul-dlss/preservation_robots (which will be a replacement for the old robot code in sul-dlss/sdr-preservation-core).

The logic represented by this code was originally in:

sul-dlss/sdr-preservation-core/lib/sdr_ingest/validate_bag <-- old preservation robots
sul-dlss/archive-utils/lib/bagit_bag <-- gem only used by sdr-preservation-robots
sul-dlss/archive-utils/lib/file_fixity
sul-dlss/archive-utils/lib/fixity

The code's new home is here because the common_accessioning robots ultimately calls (via dor-services preservable concern ...) Moab::Bagger in this moab-versioning gem to create the deposit bag. So it makes sense to have the bag validation code live here as well.

Please check for:

code has at least as much actual validation of the bag as the original
code makes sense (suggestions for improvements welcome!)
tests check what they should
test code makes sense (ditto)
does it make sense for this code to live in moab-versioning gem?
world peace

closes #117
connects to sul-dlss/preservation_robots#11

SaravShah · 2018-03-07T00:22:26Z

ndushay · 2018-03-07T01:20:42Z

In case it helps, some vague parallels here:

Methods:

#validation_errors --> was sdr-preservation-core.validate_bag#validate_bag
L67 --> sdr-preservation-core.bagit_bag#verify_pathname
#bag_dir_exists? --> was archive-utils BagitBag.open_bag
#verify_version --> was sdr-preservation-core.validate_bag#verify_version_number
(last_)version_id_from_xxx --> was sdr-preservation-core.validate_bag verify_version_id, vmfile_version_id
#verify_version_in_xml_file --> sdr-preservation-core.validate_bag#verify_version_id
#verify_tagmanifests --> archive-utils.BagitBag#verify_tagfile_manifests
#verify_payload_manifests --> archive-utils.BagitBag#verify_payload_manifests
#checksums_hash_from_manifest_files --> was archive-utils.BagitBag#read_manifest_files
#generate_tagmanifest_checksums_hash --> archive-utils: BagitBag generate_tagfile_checksums, Fixity.generate_checksums
#generate_payload_checksums --> archive-utils: BagitBag generate_payload_checksums
#generate_checksums_hash --> archive-utils: Fixity.generate_checksums
#generated_checksums --> archive-utils: Fixity.fixity_from_file
#digester_hash --> archive-utils Fixity.get_digesters
#verify_manifest_checksums --> archive-utils BagitBag verify_manifests and BagitBag manifest_diff
#checksums_diff_hash --> archive-utils FileFixity diff
#verify_payload_size --> archive-utils BagitBag verify_payload_size
#bag_info_payload_size --> archive-utils BagitBag info_payload_size, BagitBag read_bag_info_text
#generated_payload_size --> BagitBag bag_payload_size
#checksum_types_from_manifest_checksums_hash <-- I created this so we could stop generating unnec checksums
#single_error_hash <-- akin to MV storage_object_validator
#error_code_msg <-- akin to MV storage_object_validator

SaravShah

Seems like you got the main test cases for errors.
There are some edge cases I can think of but I think they might be too edge casey. I went back and forth from your methods and the methods in which you abstracted them and everything looks good.

PS. I don't mind the name payload but if others want it to be changed then thats fine.

Edit: Went through specs. This looks good to me. I would like to see this get tested, maybe inserting this code in place of the existing validate-bag step in the sdrIngestWF.

code has at least as much actual validation of the bag as the original
code makes sense (suggestions for improvements welcome!)
tests check what they should
test code makes sense (ditto)

SaravShah · 2018-03-07T00:36:07Z

lib/moab/deposit_bag_validator.rb

+    # returns Array of tiny error hashes, allowing multiple occurrences of a single error code
+    def validation_errors
+      return [single_error_hash(BAG_DIR_NOT_FOUND, bag_dir: deposit_bag_pathname)] unless deposit_bag_pathname.exist?
+      return result_array unless required_bag_files_exist?


do we want an error code here as well?

required_bag_files_exist? is a method that adds errors to result_array if all required files aren't present. :-)

your question points out that that method is doing two things: writing errors and answering a boolean. perhaps that is bad.

SaravShah · 2018-03-09T01:01:12Z

lib/moab/deposit_bag_validator.rb

+      end
+    end
+
+    def verify_manifest_checksums(manifest_type, manifests_checksum_hash, generated_checksum_hash)


verify_manifest_checksums is the combination of verify_manifests and manifest_diff. I see manifest_diff uses FileFixity.new(file_id: file_id) and we are using an empty hash here. Just wanted some clarification

Class FileFixity def initialize(options=nil) @checksums=Hash.new options = {} if options.nil? options.each do |key,value| #instance_variable_set("@#{key}", value) send "#{key}=", value end end .....

So they are passing in file_id to this hash obj but we are leaving it empty?

So in the original, they use this "FileFixity" structure as a mean of associating a file name with its checksums. And there is a bunch of code about making FileFixity a class that can be used as a hash key ... but it never is.

When I unraveled what was going on, it appeared we needed something simpler: a hash with the relative file name as the key, and a hash of checksums as the value. No need for all the indirection.

We don't have to worry about file name clashes, because they can't happen with relative file names in the bag. And we are only verifying the checksums that exist for the file, so it seemed expedient to simply work with those.

It may be that some comments would be helpful explaining the hashes.

It may also be that I am wrong in my assessment.

A further note: FileFixity in archive-utils is similar to FileSignature in moab-versioning. Since I was already up to my neck in alligators, I did not take the next step to using existing code in moab-versioning. I saw that the bag validation code was totally separate for unknown reasons so I kept my focus there and didn't look to re-use moab-versioning existing classes. Perhaps that was a bad decision.

(Perhaps it was a bad decision that the original ingest-robots didn't leverage the moab-versioning code for the bag validation, since they do leverage it for deposit-bag. I have no idea why the existing production code is so inconsistent and strange. -- wait -- yes, yes I do. Le sigh.)

SaravShah · 2018-03-14T23:22:54Z

spec/unit_tests/moab/deposit_bag_validator_spec.rb

+      storage_obj = instance_double(Moab::StorageObject, deposit_bag_pathname: bag_pathname, current_version_id: 0)
+      dbv = described_class.new(storage_obj)
+      code = described_class::CHECKSUM_MISMATCH
+      from_file = '6666666666666666666666666666666666666666666666666666666666666666'


jmartin-sul

i have 3 small nitpicks. if you think they're off base, or would prefer to ticket them, or think they're a good point but not worth the effort, i'm happy to just approve and merge.

nice work, definitely more readable, and likely more maintainable!

jmartin-sul · 2018-03-12T13:59:44Z

lib/moab/deposit_bag_validator.rb

+        pathname = deposit_bag_pathname.join(filename)
+        result_array << single_error_hash(REQUIRED_FILE_NOT_FOUND, file_pathname: pathname) unless pathname.exist?
+      end
+      result_array.empty? ? true : false


not totally opposed to this, but sort of assumes that required_bag_files_exist? will be called before any other checks, since it assumes result_array will be empty if it didn't add anything, right? comment maybe? is a local result array overkill?

adding comment - good catch.

empty? already returns true or false. No need to reinterpret that, right?

jmartin-sul · 2018-03-12T14:23:10Z

lib/moab/deposit_bag_validator.rb

+      nil
+    end
+
+    def verify_version_in_xml_file(file_pathname, found)


what about verify_version_from_xml_file? small difference, but every time i read the method name, it made me think it was going to read the xml, but it's doing stuff with already-extracted values.

jmartin-sul · 2018-03-19T17:07:14Z

lib/moab/deposit_bag_validator.rb

+      diff_hash = {}
+      # NOTE: these are intentionally & and | instead of && and ||
+      matching_checksum_types = (left_checksums.keys & right_checksums.keys)
+      matching_checksum_types = (left_checksums.keys | right_checksums.keys) if matching_checksum_types.empty?


what about... checksum_types_to_check? checksum_types_to_match? checksum_types_to_comp?

ndushay · 2018-03-20T20:17:23Z

@jmartin-sul I believe I have addressed your concerns. Thanks for the 👀 . The code is better with the small tweaks.

coveralls · 2018-03-20T20:19:51Z

Coverage decreased (-0.06%) to 98.342% when pulling 088b957 on new-deposit-bag-validator into df7102e on master.

ndushay added the needs review label Mar 6, 2018

ndushay force-pushed the new-deposit-bag-validator branch from 4fef405 to fa918f9 Compare March 6, 2018 22:30

SaravShah reviewed Mar 7, 2018

View reviewed changes

SaravShah reviewed Mar 9, 2018

View reviewed changes

SaravShah reviewed Mar 14, 2018

View reviewed changes

SaravShah approved these changes Mar 19, 2018

View reviewed changes

jmartin-sul requested changes Mar 20, 2018

View reviewed changes

ndushay added 5 commits March 20, 2018 13:15

.rspec gets '--require spec_helper'; remove it from individual specs

d121cd6

add fixtures for deposit_bag_validator tests

c656fc7

deposit_bag_validator class added (with specs)

824b185

.rubocop_todo.yml regenned

3864176

deposit-bag-validator: small tweaks for clarity

088b957

ndushay force-pushed the new-deposit-bag-validator branch from fa918f9 to 088b957 Compare March 20, 2018 20:16

jmartin-sul approved these changes Mar 20, 2018

View reviewed changes

jmartin-sul merged commit 37a34fb into master Mar 20, 2018

jmartin-sul deleted the new-deposit-bag-validator branch March 20, 2018 21:12

jmartin-sul removed the needs review label Mar 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deposit_bag_validator class added #122

deposit_bag_validator class added #122

ndushay commented Mar 6, 2018 •

edited

SaravShah commented Mar 7, 2018

ndushay commented Mar 7, 2018 •

edited

SaravShah left a comment •

edited

SaravShah Mar 7, 2018

ndushay Mar 8, 2018

ndushay Mar 8, 2018

SaravShah Mar 9, 2018 •

edited

SaravShah Mar 9, 2018 •

edited

ndushay Mar 9, 2018 •

edited

ndushay Mar 9, 2018 •

edited

SaravShah Mar 14, 2018

jmartin-sul left a comment

jmartin-sul Mar 12, 2018

ndushay Mar 20, 2018

atz Apr 2, 2018

jmartin-sul Mar 12, 2018

jmartin-sul Mar 19, 2018

ndushay commented Mar 20, 2018

coveralls commented Mar 20, 2018 •

edited

deposit_bag_validator class added #122

deposit_bag_validator class added #122

Conversation

ndushay commented Mar 6, 2018 • edited

SaravShah commented Mar 7, 2018

ndushay commented Mar 7, 2018 • edited

SaravShah left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SaravShah Mar 9, 2018 • edited

Choose a reason for hiding this comment

SaravShah Mar 9, 2018 • edited

Choose a reason for hiding this comment

ndushay Mar 9, 2018 • edited

Choose a reason for hiding this comment

ndushay Mar 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmartin-sul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndushay commented Mar 20, 2018

coveralls commented Mar 20, 2018 • edited

ndushay commented Mar 6, 2018 •

edited

ndushay commented Mar 7, 2018 •

edited

SaravShah left a comment •

edited

SaravShah Mar 9, 2018 •

edited

SaravShah Mar 9, 2018 •

edited

ndushay Mar 9, 2018 •

edited

ndushay Mar 9, 2018 •

edited

coveralls commented Mar 20, 2018 •

edited