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 metadata-patch.md to describe replace operation better #304

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tdonohue
Copy link
Member

@tdonohue tdonohue commented Feb 6, 2025

This PR is based on the discussion in DSpace/DSpace#9610 (comment)

Per RFC Section 4.3 (replace) and RFC section 4.1 (add), we recommend using the add operation (with an array of values) to replace all values. The replace operation should only be used to replace individual values.

This small PR corrects some incorrect documentation in our REST Contract

@tdonohue tdonohue added 1 APPROVAL pull request only requires a single approval to merge. port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release port to dspace-8_x This PR needs to be ported to `dspace-8_x` branch for next bug-fix release labels Feb 6, 2025
@tdonohue
Copy link
Member Author

tdonohue commented Feb 6, 2025

@atarix83 or @abollini : If you have a chance, I'd appreciate your review on this small change to our REST Contract. I'm trying to better document the replace operation, as there was confusion in today's meeting in the discussion of DSpace/DSpace#9610 (comment)

@tdonohue tdonohue requested review from abollini and atarix83 February 6, 2025 16:42
Copy link

@minurmin minurmin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The updated description is clear and explicates intended usage of PATCH add to replace values.

However, based on testing with a DSpace 7 -based installation, this does not reflect the present behaviour of the REST API. Instead, when calling PATCH add just one value is added and other values are shifted, not replaced - even if one uses path /metadata/field.name instead of single value /metadata/field.name/n.

Example: consider a community that has dc.title values
[{"value": "Artikkelit","language": "fi","place": 0}, {"value": "Articles","language": "en","place": 1}]

Then, using e.g. https://github.com/the-library-code/dspace-rest-python try to update values

d.api_patch(url=...,operation=DSpaceClient.PatchOperation.ADD, path='/metadata/dc.title', value= [ { "value": "Artikkelit (new)", "language": "fi" }, { "value": "Articles (new)", "language": "en" }])

This results to PATCH add API request that has path /metadata/dc.title and json array as value. However, this results to following structure:

[
  {"value": "Artikkelit (new)","language": "fi","place": 0},
  {"value": "Artikkelit","language": "fi","place": 1}, 
  {"value": "Articles","language": "en","place": 2}
]

I.e. only one value is added and old values are shifted! So, the note about replacing values with add should probably be rephrased - or bug fixed such that the API complies with the contract.

Assuming that
https://github.com/DSpace/DSpace/blob/main/dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/patch/operation/DSpaceObjectMetadataAddOperation.java is responsible for PATCH add it seems to contain only functionality to add one value at a time.

Compare with https://github.com/DSpace/DSpace/blob/main/dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/patch/operation/DSpaceObjectMetadataReplaceOperation.java that has more complex logic in place - even without suggested DSpace/DSpace#9610 "multiple values" addition.

Because of this DSpace/dspace-angular#3078 requires metadata remove operation first if implemented with multiple add operations instead of one "multiple values" replace call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 APPROVAL pull request only requires a single approval to merge. port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release port to dspace-8_x This PR needs to be ported to `dspace-8_x` branch for next bug-fix release
Projects
Status: 👀 Under Review
Development

Successfully merging this pull request may close these issues.

2 participants