Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make BagVerifier extendable #126

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

make BagVerifier extendable #126

wants to merge 2 commits into from

Conversation

rvanheest
Copy link

This PR aims to make BagVerifier extendable. In order to do so, I removed the final keyword from class level and added it to the methods isValid, isComplete, checkHashes, getExecutor and getManifestVerifier. This way, the class is extendable, but still ensures that the implementations of these methods is not changed (as these are defined in https://tools.ietf.org/html/draft-kunze-bagit). Besides, I gave the method checkHashes access level protected, such that not only other classes within the same package, but also classes that extend BagVerifier can call this. Since this method was not part of the public API, I decided against public access level here, such that it remains outside the public API.

The reason for making BagVerifier extendable comes from a use case we have in our project. Besides 'complete' and 'valid', we define 'virtually valid'. This requires us to partially copy a large chunk of this class and also copy all functionality defined in checkHashes. Rather than copying, we would prefer to just extend the BagVerifier class and call checkHashes. That way we can still rely on this library and not end up with duplicate code.

As an example, as well as a way to check this functionality works at compile time, I added ExtendedBagVerifier to the integration folder. It is not clear to me, though, whether this folder is picked up by Gradle for compilation/testing, so I'm not sure if this is actually the correct place to put this file and ensure that it gets checked on compile time.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.282% when pulling a56225c on rvanheest-DANS-KNAW:extendable-verifier into 2b7002e on LibraryOfCongress:master.

Copy link
Contributor

@jscancella jscancella left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I specially made the classes final to denote that the classes were not designed to be extended. If you merge this PR in I would recommend extensive testing to ensure any assumptions made during the original design still hold and extended classes wouldn't break functionality.

Personally I don't see a need for this since you can simply made a "virtual" bag by copying to a new bag object and removing files from manifests. See the example I gave in
#125 (comment)

// verification rules for a bag, as defined by your project
// NOTE: this cannot override the BagVerifier.isComplete or BagVerifier.isValid, as they're defined
// by "The BagIt File Packaging Format" (https://tools.ietf.org/html/draft-kunze-bagit).
public class ExtendedBagVerifier extends BagVerifier {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class doesn't actually test anything. The src/test and src/integration are for junit tests. The integration is meant to test against the bagit-suite as well as the combination of the readers and writers. You should probably put this in the src/test directory following the package name and have junit test something. That way gradle will test it as well as it being tested during compile time.

@rvanheest
Copy link
Author

@jscancella I'm not really sure what you mean by

If you merge this PR in I would recommend extensive testing to ensure any assumptions made during the original design still hold and extended classes wouldn't break functionality.

Rather than having a final class, I now made all public methods final. Since extended classes aren't able to override the public methods, they are not able to break current functionality. Of course, testing is good, but I don't know if one can test that overriding is not possible. This would generally result in compile errors, while I read your comment as 'creating unit tests to verify this'.

I only now see your comment in #125 (comment) (missed it because of holidays). Indeed we could also do it that way, but that would require extra disk space (note that for large bags, you need a lot of extra space!). That's exactly why I wanted to do it like this.

For now we decided to keep 'virtually valid' on the backburner. We'll first look at other things while we keep this one in mind. Until then, this PR may be unnecessary. We'll see how far we come without it :-)

@jscancella
Copy link
Contributor

@rvanheest I would recommend you take a look at Effective Java 3rd Edition by Joshua Bloch since he helped design the language while at SUN. In he mentions many items that recommend against your proposal with reasons why they should be avoided. Specifically I am thinking of chapter 4 on classes and interfaces (and more specifically item 19 - Design and document for inheritance or else prohibit it).

As for having virtually valid bags take up more space, I would recommend you not store them as bags. Instead I would use a object store with built in redundancy and de-duplication (like swift or ceph.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants