bagit-profiles / bagit-profiles-validator

A simple Python module for validating BagIt Profiles.
The Unlicense
12 stars 6 forks source link

Raise targeted exceptions in `validate()` method #24

Closed helrond closed 4 years ago

helrond commented 4 years ago

Currently the methods called by validate() all raise a ProfileValidationError, which make it hard to target specific errors.

I'd like to implement more specific exceptions, so that, for example, validate_bagit_profile_accept_bagit_versions() would raise an AcceptBagItVersionsValidationError, which would be a subclass of ProfileValidationError.

One point of discussion: the validate() function as currently written seems designed to produce a comprehensive list of errors, rather than raising the first exception it encounters. If my read on that is true, it may be necessary to implement an additional validation method that would raise exceptions.

tdilauro commented 4 years ago

Currently the methods called by validate() all raise a ProfileValidationError, which make it hard to target specific errors.

ProfileValidator.validate, which validates a bag using the profile, currently catches all of those exceptions and doesn't re-raise them, so you will not see them. Only the functions used to validate the profile throw uncaught exceptions that you could handle yourself. I didn't write this part, but presumably profile validation errors raise an exception so that one does not blithely forge ahead validating a bag with an invalid profile.

I would add that the exceptions can be distinguished by the associated value, which corresponds to argument to self._fail. Further, the variable msg in the scope of the for loop (and, thus, the current exception handler) could be use for the same targeting.

I'd like to implement more specific exceptions, so that, for example, validate_bagit_profile_accept_bagit_versions() would raise an AcceptBagItVersionsValidationError, which would be a subclass of ProfileValidationError.

That would be one way to do it, but once an exception is raised, control will pass back to the scope containing the handler, so ProfileValidator.validate would not run to completion. Is there any particular reason why it would not be acceptable to allow validate to complete and then inspect the report?

Because exceptions are such a blunt instrument, I would be interested to hear your use case(s) to see if there other approaches that get you to what you need.

One point of discussion: the validate() function as currently written seems designed to produce a comprehensive list of errors, rather than raising the first exception it encounters. If my read on that is true, it may be necessary to implement an additional validation method that would raise exceptions.

Here are a couple of alternatives that spring to mind:

helrond commented 4 years ago

Hi @tdilauro thanks for your response. My main use case here is that I'm using bagit-profiles in a Django application, and want to grab and persist validation error messages to the database. Based on what you're saying above, it seems like that should be possible right now by targeting the report attribute of an instantiated ProfileValidator. Is that true?

tdilauro commented 4 years ago

Close. There is no ProfileValidator. The Profile object has a report attribute that is initially None, but gets set to a new ProfileValidationReport at the beginning of each run of Profile.validate. You should capture either a reference to or the contents of that report as soon as validate completes, as subsequent calls to validate will overwrite the report attribute.

Have a look at ProfileValidationReport and this test to get a better sense of how it works. Currently, if validate returns success, then there are no error messages in the report.

helrond commented 4 years ago

Got it - I think that takes care of my use case, thank you!

Digging into this a bit more it looks like we were using an older version of the library, which didn't help. I've updated to 1.3.1 now and things are working much better.

tdilauro commented 4 years ago

Just FYI, I am thinking about some improvements and there is a discussion over on Issue #25. Since I know you're using the validator, your thoughts would be welcome.