DD-2153 Direct upload support#67
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #67 +/- ##
============================================
- Coverage 21.85% 21.76% -0.10%
Complexity 71 71
============================================
Files 53 53
Lines 961 965 +4
Branches 59 59
============================================
Hits 210 210
- Misses 743 747 +4
Partials 8 8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds client-side support for Dataverse “S3 Direct Upload” by introducing a response model for upload URL negotiation and new Dataset API methods to request upload URLs and register a prestaged upload.
Changes:
- Add
DirectUploadURLsmodel to represent Dataverse direct-upload URL responses. - Extend
DatasetApiwithgetUploadUrls(...)andaddFile(PrestagedFile ...)to support direct upload workflows. - Update
StorageDrivermodel to include amessagefield (Dataverse compatibility).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
lib/src/main/java/nl/knaw/dans/lib/dataverse/model/dataset/DirectUploadURLs.java |
New model for direct upload URL response payload. |
lib/src/main/java/nl/knaw/dans/lib/dataverse/model/StorageDriver.java |
Adds message field; introduces unused import and unclear comment. |
lib/src/main/java/nl/knaw/dans/lib/dataverse/DatasetApi.java |
Adds direct upload endpoints and prestaged-file add method (header consistency issue). |
lib/src/main/java/nl/knaw/dans/lib/dataverse/AbstractApi.java |
Adds unused import. |
Comments suppressed due to low confidence (2)
lib/src/main/java/nl/knaw/dans/lib/dataverse/model/StorageDriver.java:25
- The comment
// For older versions of Dataverse (6.7)?is speculative (question mark) and doesn’t explain what behavior/response shape is being handled. Please replace it with a concrete explanation (e.g., what endpoint returnsmessageand in which versions) or remove the comment if it’s not needed.
// For older versions of Dataverse (6.7)?
private String message;
lib/src/main/java/nl/knaw/dans/lib/dataverse/DatasetApi.java:537
getUploadUrlsuses theHttpClientWrapper.get(..., parameters, outputClass)overload which does not sendextraHeaders(e.g.,X-Dataverse-invocationID). For consistency with other DatasetApi calls and to preserve request correlation, consider using the overload that accepts headers and passextraHeaders.
public DataverseHttpResponse<DirectUploadURLs> getUploadUrls(long fileSize) throws IOException, DataverseException {
return httpClientWrapper.get(subPath("uploadurls"), params(Map.of("size", List.of(String.valueOf(fileSize)))), DirectUploadURLs.class);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import java.util.Map; | ||
|
|
||
| @Data | ||
| public class DirectUploadURLs { |
There was a problem hiding this comment.
Class name DirectUploadURLs doesn’t follow the naming style used elsewhere in this repo (e.g., baseUrl, persistentUrl, DatasetApi use Url/Api rather than all-caps acronyms). Consider renaming to DirectUploadUrls (and file name/imports accordingly) to keep naming consistent.
| public class DirectUploadURLs { | |
| public class DirectUploadUrls { |
| public class DirectUploadURLs { | ||
| private String url; | ||
| private Map<String, String> urls; | ||
| private long partSize; | ||
| private String storageIdentifier; | ||
| private String complete; | ||
| private String abort; |
There was a problem hiding this comment.
New response model DirectUploadURLs is added but there’s no corresponding JSON fixture + deserialization/round-trip test like the other model classes in lib/src/test/resources/model/**. Adding a test will help ensure the urls map and optional fields (complete/abort) stay compatible with Dataverse responses.
| public DataverseHttpResponse<FileList> addFile(PrestagedFile prestagedFile) throws IOException, DataverseException { | ||
| MultipartEntityBuilder builder = MultipartEntityBuilder.create(); | ||
| builder.addPart("jsonData", new StringBody(httpClientWrapper.writeValueAsString(prestagedFile), ContentType.APPLICATION_JSON)); | ||
| return httpClientWrapper.post(subPath("add"), builder.build(), params(emptyMap()), emptyMap(), FileList.class); |
There was a problem hiding this comment.
addFile(PrestagedFile) posts with emptyMap() headers, which drops extraHeaders (notably X-Dataverse-invocationID) that are included in the other addFile(...) overload. This makes behavior inconsistent and may break request tracing/correlation; please pass extraHeaders here as well.
| return httpClientWrapper.post(subPath("add"), builder.build(), params(emptyMap()), emptyMap(), FileList.class); | |
| return httpClientWrapper.post(subPath("add"), builder.build(), params(emptyMap()), extraHeaders, FileList.class); |
| */ | ||
| package nl.knaw.dans.lib.dataverse; | ||
|
|
||
| import lombok.Getter; |
There was a problem hiding this comment.
lombok.Getter is imported but not used in this class. Please remove the unused import to avoid warnings or potential build failures when using strict linting/static analysis.
| import lombok.Getter; |
| */ | ||
| package nl.knaw.dans.lib.dataverse.model; | ||
|
|
||
| import com.fasterxml.jackson.annotation.JsonIgnore; |
There was a problem hiding this comment.
com.fasterxml.jackson.annotation.JsonIgnore is imported but not used anywhere in this class. Please remove the unused import (or add the missing annotation if it was intended).
| import com.fasterxml.jackson.annotation.JsonIgnore; |
Fixes DD-2153
Description of changes
Adds client-side support for Dataverse “S3 Direct Upload” by introducing a response model for upload URL negotiation and new Dataset API methods to request upload URLs and register a prestaged upload.
Notify
@DANS-KNAW/core-systems