Add checksum option to DROID API#1341
Conversation
Merge develop to main
This allows you to pass in a list of hash algorithms and the API will return the checksums for the file for that algorithm.
| } | ||
|
|
||
| private <T> Map<HashAlgorithm, String> generateHashResults(T identifier, BiFunction<HashAlgorithm, T, String> hashFunction) { | ||
| return hashAlgorithms.stream().collect(Collectors.toMap( |
There was a problem hiding this comment.
potential NPE if hashAlgorithm is not requested by the API user.
There was a problem hiding this comment.
Ah good spot. And the test was always passing in an empty list so I didn't notice. The builder will now return an empty list if hasAlgorithms isn't set and I've updated the test to pass null in to check that it works.
| String droidVersion = ResourceBundle.getBundle("options").getString("version_no"); | ||
| ContainerApi containerApi = new ContainerApi(droidCore, containerSignature); | ||
| return new DroidAPI(droidCore, containerApi.zipIdentifier(), containerApi.ole2Identifier(), containerApi.gzIdentifier(), containerVersion, droidCore.getSigFile().getVersion(), droidVersion, this.s3Client, this.httpClient, this.s3Region); | ||
| List<HashAlgorithm> hashAlgorithmsOrEmptyList = this.hashAlgorithms == null ? Collections.emptyList() : this.hashAlgorithms; |
There was a problem hiding this comment.
This might be my ignorance of Java, but on line 182, couldn't you just do
private List<HashAlgorithm> hashAlgorithms = Collections.emptyList(); or
private List<HashAlgorithm> hashAlgorithmsOrEmptyList = Collections.emptyList();?
There was a problem hiding this comment.
I think you're right, I've changed the builder so it calls private List<HashAlgorithm> hashAlgorithms = Collections.emptyList();
There was a problem hiding this comment.
You would still need the null protection in case someone directly uses Builder and passes in a null. Might be easier to null check in public DroidAPIBuilder hashAlgorithms(final List<HashAlgorithm> hashAlgorithms)
There was a problem hiding this comment.
I did think that but then I thought, if you're explicitly passing null into a builder then you deserve what happens to you. That being said, I've added a null check in the builder method.
| )); | ||
| } | ||
|
|
||
| private String getFileHash(HashAlgorithm hashAlgorithm, Path path) { |
There was a problem hiding this comment.
Is the FileInputStream staying open here?
Might be easier to use try-with-resource so it closes automatically
try (InputStream fs = new FileInputStream(path.toFile())){
return getHash(hashAlgorithm, fs);
} catch (IOException e) {
throw new RuntimeException(e);
}
There was a problem hiding this comment.
Good point, I've changed it.
| private String getS3Hash(HashAlgorithm algorithm, S3Uri s3Uri) { | ||
| String key = s3Uri.key().orElseThrow(() -> new RuntimeException("Key not found in uri " + s3Uri.uri())); | ||
| String bucket = s3Uri.bucket().orElseThrow(() -> new RuntimeException("Bucket not found in uri " + s3Uri.uri())); | ||
| ResponseInputStream<GetObjectResponse> responseInputStream = s3Client.getObject(GetObjectRequest.builder().bucket(bucket).key(key).build()); |
There was a problem hiding this comment.
similar to above, try-with-resource to close the ResponseInputStream
There was a problem hiding this comment.
I've changed this one as well.
There was a problem hiding this comment.
A couple of comments inline. Looks good. Do we need to convey the ApiResult vs APIResult breaking change to TDR in advance.
... and may I tempt you to update this page too -> https://github.com/digital-preservation/droid/wiki/How-to-use-DROID-internal-API (sorry!!)
I had another look at the "how to" page and it has been out of date for a while, feel free to ignore it for this PR. |
| } | ||
|
|
||
| private ApiResult createApiResult(IdentificationResult result, String extension, boolean extensionMismatch, URI uri) { | ||
| private IdentificationResult createIdentificationResult(uk.gov.nationalarchives.droid.core.interfaces.IdentificationResult result, String extension, boolean extensionMismatch, URI uri) { |
There was a problem hiding this comment.
Can't you just use IdentificationResult or interfaces.IdentificationResult?
There was a problem hiding this comment.
Not with Java I don't think. This is a different class with the same name as the record I created. I could rename the record but I don't really want to. I've passed in the three parameters I need instead of using this class.
There was a problem hiding this comment.
Actually, I've changed my mind, I've renamed the record APIIdentificationResult so this doesn't need the package qualifier now.
| List<String> containerPuids = Arrays.asList(ZIP_PUID, OLE2_PUID, GZIP_PUID); | ||
| return binaryResult.getResults().stream() | ||
| .map(IdentificationResult::getPuid) | ||
| .map(uk.gov.nationalarchives.droid.core.interfaces.IdentificationResult::getPuid) |
There was a problem hiding this comment.
Can't you just use IdentificationResult or interfaces.IdentificationResult?
There was a problem hiding this comment.
As above, I've renamed the record so this can be used unqualified.
| return uri; | ||
| } | ||
| public enum HashAlgorithm { | ||
| /** MD5. **/ |
There was a problem hiding this comment.
Are these comments necessary?
There was a problem hiding this comment.
Technically they are because of the checkstyle rules but I agree that this is daft so I've disabled that rule for this class.
3 participants
This allows you to pass in a list of hash algorithms and the API will
return the checksums for the file for that algorithm.