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

DS-3908 Metadata patch support #2313

Merged
merged 7 commits into from Mar 8, 2019

Conversation

@cwilper
Copy link
Contributor

commented Jan 2, 2019

This implements PATCH support for DSO metadata, in the REST API.

https://jira.duraspace.org/browse/DS-3908

With this code, all DSpace object types support PATCHing their metadata by authorized users, as described by the REST docs in the related PR: DSpace/Rest7Contract#46

ITs have been updated to support a series of authorized/unauthorized metadata PATCH requests for each DSO type, which can be found in the metadata-patch-suite.json file.

@cwilper cwilper force-pushed the atmire:DS-3908_Metadata_patch_support branch from fda61b7 to 185766d Feb 26, 2019
Copy link
Member

left a comment

@cwilper : I gave this a code review today. It's excellent overall (and I appreciate some of the refactoring here to make the Patch code shared among all DSOs).

I have a few minor suggestions inline (mostly requires for JavaDocs and minor updates). I haven't had a chance to (manually) test it out yet, but it looks like you have good Integration Tests which is nice to see.

@paulo-graca

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

I've tried to run:
curl --data '[{ "op": "add", "path": "/metadata/dc.description.abstract/0", "value": [ { "value": "Some description" } ] },{ "op": "add", "path": "/metadata/dc.description.abstract/-", "value": [ { "value": "Some description 2" } ] }]' -H "Authorization: Bearer ..." -H "content-type: application/json" -X PATCH http://HOST/api/core/items/ITEM_UUID
and got the following error:

{  
   "timestamp":1551714121150,
   "status":400,
   "error":"Bad Request",
   "exception":"java.lang.IllegalArgumentException",
   "message":"com.fasterxml.jackson.databind.JsonMappingException: Can not deserialize instance of org.dspace.app.rest.model.MetadataValueRest out of START_ARRAY token\n at [Source: N/A; line: -1, column: -1] (through reference chain: org.dspace.app.rest.model.MetadataRest[\"dc.description.abstract\"]->java.util.ArrayList[0])",
   "trace":"java.lang.IllegalArgumentException: com.fasterxml.jackson.databind.JsonMappingException: Can not deserialize instance of org.dspace.app.rest.model.MetadataValueRest out of START_ARRAY token\n at [Source: N/A; line: -1, column: -1] (through reference chain: org.dspace.app.rest.model.MetadataRest[\"dc.description.abstract\"]->java.util.ArrayList[0])\n\tat org.
@cwilper

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

@paulo-graca I think your PATCH request is trying to do something impossible. The first "add" is trying to set the first (position 0) dc.description.abstract value to be an array. The first value should be a value, not an array of values. One way to "fix" it would be to omit the "/0" part of your path, in which case you are effectively setting dc.description.abstract to an array of size 1. The second "add" is similar in that it is trying to set the value of a particular position in an array to an array. You could fix it by omitting the leading and trailing "[" and "]" characters from the "value":....you should be providing an object when the path specifies a position in the array. An array should only be provided as the value when you are specifying the path to the "key" only, e.g. "path": "/metadata/dc.description.abstract" <-- the value for this should be an array. See the examples in this file: https://github.com/DSpace/DSpace/blob/185766d8d242a9b855005e33bcc1f32e0d0f2206/dspace-spring-rest/src/test/resources/org/dspace/app/rest/test/metadata-patch-suite.json

@paulo-graca

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

I'm just trying to follow the examples in the Rest contract:
https://github.com/DSpace/Rest7Contract/blob/master/metadata-patch.md#adding-metadata

@paulo-graca

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

@cwilper my fault, I didn't notice the brackets in values when manipulating specific metadata values.

Copy link
Contributor

left a comment

The code looks good to me, It's accordant to the Rest Contract. I don't have any more comments besides what @tdonohue already noticed.

Copy link
Member

left a comment

👍 This looks good to me now, too. Thanks @cwilper !

Merging immediately. (As a sidenote, I'm hoping the refactors here won't affect the Angular UI in any way -- I don't think they should, as Angular isn't using PATCH for anything yet. But I'll hold off for a few days on updating the REST API demo site just in case.)

@tdonohue tdonohue merged commit efbffe4 into DSpace:master Mar 8, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.07%) to 29.93%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.