12141 - Storage Driver API#12182
Merged
Merged
Conversation
jp-tosca
added
GREI Re-arch
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses #12141 by moving Dataverse Storage Driver endpoints out of the Admin API namespace and exposing them under the Dataverses API.
Changes:
- Moved storage driver GET/PUT/DELETE endpoints from
/api/admin/dataverse/{alias}/storageDriverto/api/dataverses/{identifier}/storageDriver. - Replaced the former “list storage drivers” admin endpoint with a dataverse-scoped endpoint intended to list allowed drivers.
- Updated integration tests and admin docs/release notes to reflect the endpoint move.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/edu/harvard/iq/dataverse/api/UtilIT.java | Updates helper methods to call the new dataverses storage driver endpoints (but list path currently mismatches new API). |
| src/test/java/edu/harvard/iq/dataverse/api/S3AccessIT.java | Updates calls to the storage-driver listing helper to include a dataverse alias. |
| src/test/java/edu/harvard/iq/dataverse/api/DatasetsIT.java | Updates storage-driver listing helper usage for dataset storage driver test. |
| src/main/java/edu/harvard/iq/dataverse/engine/command/impl/SetDataverseStorageDriverCommand.java | New command to set a collection’s storage driver by label. |
| src/main/java/edu/harvard/iq/dataverse/engine/command/impl/GetDataverseStorageDriverCommand.java | New command to fetch direct/effective storage driver id with release-based permission requirements. |
| src/main/java/edu/harvard/iq/dataverse/engine/command/impl/GetDataverseAllowedStorageDriverCommand.java | New command to list drivers (currently returns all drivers; permission model may need alignment). |
| src/main/java/edu/harvard/iq/dataverse/engine/command/impl/DeleteDataverseStorageDriverComman.java | New command to reset a collection’s configured storage driver (class name contains a typo). |
| src/main/java/edu/harvard/iq/dataverse/engine/command/impl/SetDataverseMetadataLanguageCommand.java | Removes an unused import. |
| src/main/java/edu/harvard/iq/dataverse/api/Dataverses.java | Adds new /storageDriver and /allowedStorageDrivers endpoints under /api/dataverses/{identifier}; removes unrelated commented-out logo code; introduces unused imports. |
| src/main/java/edu/harvard/iq/dataverse/api/Admin.java | Removes the old admin storage driver endpoints. |
| src/main/java/edu/harvard/iq/dataverse/api/Info.java | Adds imports but currently leaves them unused (compile-breaking). |
| doc/sphinx-guides/source/admin/dataverses-datasets.rst | Updates curl examples to the new endpoint locations; superuser-only wording likely no longer matches implementation. |
| doc/release-notes/12141-storage-driver-endpoints.md | Adds release note for breaking endpoint changes; contains typos and path accuracy issues. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import com.google.common.collect.Lists; | ||
| import com.google.api.client.util.ArrayMap; | ||
| import com.google.api.client.util.Data; | ||
| import com.google.api.services.storage.Storage.AnywhereCaches.Get; |
There was a problem hiding this comment.
These newly added imports are unused in this file and will fail compilation (Java treats unused imports as errors). Please remove them unless they’re actually needed.
Suggested change
| import com.google.api.services.storage.Storage.AnywhereCaches.Get; |
This comment has been minimized.
This comment has been minimized.
4 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
stevenwinship
moved this from Ready for Review ⏩
to In Review 🔎
in IQSS Dataverse Project
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
@stevenwinship this is ready to review, thanks! |
| GetDataverseAllowedStorageDriverCommand getAllowedStorageDriversCommand = new GetDataverseAllowedStorageDriverCommand(request, dv); | ||
| return ok(execCommand(getAllowedStorageDriversCommand)); | ||
| } catch (WrappedResponse wr) { | ||
| return handleWrappedResponse(wr); |
Check warning
Code scanning / CodeQL
Information exposure through an error message Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To fix this without changing endpoint behavior more than necessary, avoid passing the wrapped response through handleWrappedResponse(wr) in this method and instead:
- Log the exception server-side with context (
id) and stack trace. - Return a generic error response to the client (same status family, but non-revealing message).
In src/main/java/edu/harvard/iq/dataverse/api/Dataverses.java, update the catch (WrappedResponse wr) block in listStorageDrivers(...) (around lines 2255–2257). Replace the direct handleWrappedResponse(wr) return with a sanitized error(...) response (e.g., BAD_REQUEST with generic text) and add a logger.log(...) call. No new imports are needed because Logger/Level are already imported and used in this file pattern.
Suggested changeset
1
src/main/java/edu/harvard/iq/dataverse/api/Dataverses.java
| @@ -2253,7 +2253,8 @@ | ||
| GetDataverseAllowedStorageDriverCommand getAllowedStorageDriversCommand = new GetDataverseAllowedStorageDriverCommand(request, dv); | ||
| return ok(execCommand(getAllowedStorageDriversCommand)); | ||
| } catch (WrappedResponse wr) { | ||
| return handleWrappedResponse(wr); | ||
| logger.log(Level.WARNING, "Failed to list allowed storage drivers for dataverse: " + id, wr); | ||
| return error(Response.Status.BAD_REQUEST, "Unable to list allowed storage drivers."); | ||
| } | ||
| } | ||
|
|
Copilot is powered by AI and may make mistakes. Always verify output.
This comment has been minimized.
This comment has been minimized.
stevenwinship
approved these changes
Apr 1, 2026
github-project-automation
Bot
moved this from In Review 🔎
to Ready for QA ⏩
in IQSS Dataverse Project
Contributor
|
@jp-tosca Could you solve the conflict here? Thank you! |
|
📦 Pushed preview images as 🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name. |
github-project-automation
Bot
moved this from QA ✅
to Merged 🚀
in IQSS Dataverse Project
ErykKul
added a commit
to IQSS/dataverse-client-javascript
that referenced
this pull request
May 5, 2026
PR #12182 merged on dataverse develop and moved the per-collection
storage-driver endpoint:
OLD: PUT /api/admin/dataverse/{alias}/storageDriver
NEW: PUT /api/dataverses/{alias}/storageDriver
The CI integration tests on PR #403 now run against a Dataverse
container that includes the move, so setStorageDriverViaApi was
hitting the old admin path and getting 404, which cascaded into
every dataset/file test that depends on the directUploadTestCollection
having LocalStack as its storage driver.
Fix: update setStorageDriverViaApi to use the new public endpoint.
The endpoint still requires X-Dataverse-Key for write operations
(superuser only), so authentication is unchanged.
ErykKul
added a commit
to IQSS/dataverse-frontend
that referenced
this pull request
May 5, 2026
Pulls the latest prerelease of the SDK published from PR #403 after its CI went green following the IQSS/dataverse#12182 storage-driver endpoint move. This version ships: - The tree node listing helpers (listDatasetTreeNode + iterateDatasetTreeNode) that the tree-view track will consume. - The public re-export of DataverseApiAuthMechanism, replacing the current deep import in src/standalone-uploader/index.tsx. - The server-driven S3 tagging (FileUploadDestination.tagging) that removes the duplicate client-side flag. Lockfile updated; no behaviour change in this commit (consumer code still uses the existing deep import + inline axios; follow-up commits will wire in the new public surfaces).
This file contains hidden or 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
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.
What this PR does / why we need it:
Which issue(s) this PR closes:
Special notes for your reviewer:
Suggestions on how to test this:
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?:
Additional documentation: