-
Notifications
You must be signed in to change notification settings - Fork 495
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
Update get file endpoint to add a datasetVersion optional query parameter and extend its payload #10299
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
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.
This comment has been minimized.
This comment has been minimized.
2 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.
...java/edu/harvard/iq/dataverse/engine/command/impl/GetLatestPublishedFileMetadataCommand.java
Outdated
Show resolved
Hide resolved
...ava/edu/harvard/iq/dataverse/engine/command/impl/GetLatestAccessibleFileMetadataCommand.java
Outdated
Show resolved
Hide resolved
...ava/edu/harvard/iq/dataverse/engine/command/impl/GetLatestAccessibleFileMetadataCommand.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I left a few comments w.r.t. some changes, but nothing major.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…ommand call in GetLatestAccessibleFileMetadataCommand
…hedFileMetadata method to avoid extra command call in GetLatestAccessibleFileMetadataCommand
This comment has been minimized.
This comment has been minimized.
1 similar comment
📦 Pushed preview images as
🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name. |
Thanks. I have updated the PR with the requested changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to go. All change requests addressed.
What this PR does / why we need it:
The API endpoint
api/files/{id}
has been extended to support the following optional query parameters:includeDeaccessioned
: Indicates whether or not to consider deaccessioned dataset versions in the latest file search. (Default:false
).returnDatasetVersion
: Indicates whether or not to include the dataset version of the file in the response. (Default:false
).A new endpoint
api/files/{id}/versions/{datasetVersionId}
has been created. This endpoint returns the file metadata present in the requested dataset version. To specify the dataset version, you can use:latest-published
, or:latest
, or:draft
or1.0
or any other available version identifier.The endpoint supports the
includeDeaccessioned
andreturnDatasetVersion
optional query parameters, as does theapi/files/{id}
endpoint.Which issue(s) this PR closes:
Special notes for your reviewer:
DataFile's
getLatestPublishedFileMetadata
andgetLatestFileMetadata
methods have been refactored to reduce duplication and improve readability and logic reusability.As I mentioned in the frontend weekly meeting, I discovered that these methods include deaccessioned dataset versions in the lookup, regardless of the user permissions, which may expose file metadata of deaccessioned dataset versions publicly.
It is important to review this behavior in the future, as we should only expose deaccessioned file metadatas if the user has permissions and requires deaccessioned versions explicitly ("includeDeaccessioned" parameter).
In order not to overextend this PR, I finally chose not to fix the issue in this PR, keeping this behavior as it is for now, since the mentioned methods are used in many parts of the code and modifying them would require a more exhaustive review, since there are cases in which obtaining latest files in a deaccessioned state may be necessary (for example when handling new file versions or editing them).
So although I have refactored the
getLatestFileMetadata
andgetLatestPublishedFileMetadata
methods, I am not using them in this API extension. I am using commands instead, where the user permissions are checked and the includeDeaccessioned optional query param is managed, as in get dataset commands.files/{id}/draft
endpoint is no longer available in favor of the new endpointfiles/{id}/versions/{datasetVersionId}
, which can use the version identifier:draft
(files/{id}/versions/:draft
) to obtain the same result.Suggestions on how to test this:
Follow the new docs to call the extended API endpoint. For example:
api/files/{id}:
curl -H "X-Dataverse-key:xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" "http://localhost:8080/api/files/:persistentId/?persistentId=doi:10.5072/FK2/J8SJZB&returnDatasetVersion=true&includeDeaccessioned=true"
api/files/{id}/versions/{datasetVersionId}:
curl -H "X-Dataverse-key:xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" "http://localhost:8080/api/files/:persistentId/versions/:draft?persistentId=doi:10.5072/FK2/J8SJZB&returnDatasetVersion=true&includeDeaccessioned=true"
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
None
Is there a release notes update needed for this change?:
Yes. Added.
Additional documentation:
None