Update REST API for specifying checksums #960
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Initially clients could specify checksums for uploaded binary content via a custom `checksum` query parameter, instead of this non-standard approach, we want to refactor to follow RFC-3230 4.3.2. * Adds 'Warning' header to announce deprecation of `checksum` * Adds support for 'Digest' header as per RFC-3230 * Adds utility function for parsing Digest headers * Adds integration tests for checksum match and mismatch Resolves: https://jira.duraspace.org/browse/FCREPO-1468
| * Create a resource at a specified path, or replace triples with provided RDF. | ||
| * @param requestContentType the request content type | ||
| * @param requestBodyStream the request body stream | ||
| * @param checksumDeprecated the deprecated checksum value |
There was a problem hiding this comment.
This needs a better comment. Why deprecated? How to avoid it? What to prefer?
Testing of checksums of binary uploads should have utilized the `getStatus(final HttpUriRequest req)` instead of executing the request in a try block explicitly.
parseDigestHeader was looping through every submitted hash in the Digest header and only taking action on the sha1. Until we have interest in additional hashes we can simple use lambdas to pull just the sha1 header if it exists.
Reasoning behind deprecation of the checksum query parameter in favour of the Digest header existed only in the git commit log and JIRA ticket, without being present in the code. * Documents deprecation * Adds TODO to remove deprecated code
| */ | ||
| private String parseDigestHeader(final String digest) { | ||
| final Map<String,String> digestPairs = RFC3230_SPLITTER.split(nullToEmpty(digest)); | ||
| final String hash = digestPairs.entrySet().stream() |
There was a problem hiding this comment.
Just use findFirst here, to acquire an Optional<String>. Then below, use Optional::orElse to create your defaulting behavior. There's no need to create then check for the blank string.
Instead of generating a potentially empty string value for the hash variable in parseDigestHeader, we can utilize Optional<String> to obliviate the need for an StringUtils.isBlank() in favour of using Optional::orElse.
| .filter(s -> s.getKey().toLowerCase().equals("sha1")) | ||
| .map(Map.Entry::getValue) | ||
| .findFirst(); | ||
| return hash.map("urn:sha1:"::concat).orElse(""); |
There was a problem hiding this comment.
You can even go further and simply .map(...).orElse("") off of the .findFirst() line:
.findFirst()
.map("urn:sha1:"::concat)
.orElse("");
Then you don't even need to define the hash value or import Optional.
There was a problem hiding this comment.
I... should have seen that.
Chainable functions... are chainable.
Add reference to TODO ticket
We were not properly handling invalid digest headers so when receiving a value in the digest header that did not match RFC-3230 we were incorrectly returning a 500 Internal Server Error, rather than the correct 400 Bad Request. * Handle invalid Digest Headers * Extend integration tests to ensure proper handling of malformed header
|
Resolved with: 4aefe3e |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Initially clients could specify checksums for uploaded binary content
via a custom
checksumquery parameter, instead of this non-standardapproach, we want to refactor to follow RFC-3230 4.3.2.
Warningheader to announce deprecation ofchecksumDigestheader as per RFC-3230DigestheaderschecksumTODOhints for removal of deprecated codeResolves: https://jira.duraspace.org/browse/FCREPO-1468