Skip to content
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

Merged
merged 33 commits into from
Feb 23, 2024

Conversation

GPortas
Copy link
Contributor

@GPortas GPortas commented Feb 6, 2024

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 or 1.0 or any other available version identifier.

The endpoint supports the includeDeaccessioned and returnDatasetVersion optional query parameters, as does the api/files/{id} endpoint.

Which issue(s) this PR closes:

Special notes for your reviewer:

DataFile's getLatestPublishedFileMetadata and getLatestFileMetadata 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 and getLatestPublishedFileMetadata 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 endpoint files/{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

@coveralls
Copy link

coveralls commented Feb 6, 2024

Coverage Status

coverage: 20.275% (-0.001%) from 20.276%
when pulling f49a48a on 10280-get-file-api-extension
into 82585f9 on develop.

This comment has been minimized.

This comment has been minimized.

1 similar comment

This comment has been minimized.

This comment has been minimized.

1 similar comment

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.

2 similar comments

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Member

@qqmyers qqmyers left a 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.

1 similar comment

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.

1 similar comment
Copy link

📦 Pushed preview images as

ghcr.io/gdcc/dataverse:10280-get-file-api-extension
ghcr.io/gdcc/configbaker:10280-get-file-api-extension

🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name.

@GPortas GPortas requested a review from qqmyers February 23, 2024 10:17
@GPortas GPortas assigned qqmyers and unassigned GPortas Feb 23, 2024
@GPortas
Copy link
Contributor Author

GPortas commented Feb 23, 2024

Looks good. I left a few comments w.r.t. some changes, but nothing major.

Thanks. I have updated the PR with the requested changes.

Copy link
Member

@qqmyers qqmyers left a 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.

@sekmiller sekmiller self-assigned this Feb 23, 2024
@sekmiller sekmiller merged commit de45c13 into develop Feb 23, 2024
20 checks passed
@sekmiller sekmiller deleted the 10280-get-file-api-extension branch February 23, 2024 21:21
@pdurbin pdurbin added this to the 6.2 milestone Feb 23, 2024
@qqmyers qqmyers removed their assignment Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pm.GREI-d-2.7.1 NIH, yr2, aim7, task1: R&D UI modules for creating datasets and supporting publishing workflows pm.GREI-d-2.7.2 NIH, yr2, aim7, task2: Implement UI modules for creating datasets and publishing workflows Size: 10 A percentage of a sprint. 7 hours. SPA: File Page SPA These changes are required for the Dataverse SPA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update get file endpoint to add a datasetVersion optional query parameter and extend its payload
5 participants