Add Atomic Operations Support #2979
Conversation
|
Nice. Will take a look soon! |
| } | ||
| fullPathBuilder.append(ref.getType()); | ||
|
|
||
| // Only relationship operations or resource update operation should have the id |
There was a problem hiding this comment.
Is this true? What about remove? That looks like it can also have an ID.
There was a problem hiding this comment.
For some reason I recall that the delete needed to be performed on the collection endpoint and needed to have data specified with the type and id. So later on if the data was omitted I will attempt to generate this when specified from the ref.
// If data is not provided, but it is to remove a resource the data is optional
// and should be generated from the ref
if ((data == null || JsonNodeType.NULL.equals(data.getNodeType()))
&& OperationCode.REMOVE.equals(operation.getOperationCode())) {
data = requestScope.getMapper().getObjectMapper().createObjectNode().put("type", ref.getType())
.put("id", ref.getId() != null ? ref.getId() : ref.getLid());
}There was a problem hiding this comment.
The JSON API core spec has this example of a DELETE with the ID in the path:
DELETE /photos/1 HTTP/1.1
Accept: application/vnd.api+json
The operations spec has this example of specifying the ID in the operation REF (note that data is missing):
{
"atomic:operations": [{
"op": "remove",
"ref": {
"type": "articles",
"id": "13"
}
}]
}handleDeleteOp does not append the ID to the fullPath if data is missing. However, if data is present it appends the ID. Not sure what is more common, but the path will be different depending on the presence of data. The spec example shows data missing. This looks to me like it might not work.
There was a problem hiding this comment.
You're correct I'm not sure why I ended up not specifying the id in the ref for the remove operation. I shall change it to use the id in the remove operation and not generate the data.
| Resource resource = mapper.forAtomicOperations() | ||
| .readResource(operation.getData()); | ||
| if (resource.getType() != null) { | ||
| if (resource.getAttributes() == null || resource.getAttributes().isEmpty()) { |
There was a problem hiding this comment.
Why do we care about attributes being null/empty here?
There was a problem hiding this comment.
This is to tell whether it was an relationship operation. If it is a relationship operation and the attributes would be null or empty.
There was a problem hiding this comment.
The spec says if the operations is targeting a resource, then ref/href MUST be used. The same goes for deleting a resource (non-relationship). Should this be an error instead (instead of inferring the ref)?
There was a problem hiding this comment.
After looking at the spec again it looks like you are correct that for delete resource scenario the ref/href is mandatory. I have updated the code to only infer the ref for the add and update resource scenario where the ref/href is optional. The code thus will not attempt to infer for the delete resource and any relationship operations scenario.
|
|
||
| Map<String, Object> productAttributes = response.path("'atomic:results'[3].data.attributes"); | ||
| assertEquals("Product1", productAttributes.get("commonName")); | ||
| assertEquals("Product1 Description", productAttributes.get("description")); |
There was a problem hiding this comment.
There are a number of patch extenstion requests in ResourceIT (you can search for JSONAPI_CONTENT_TYPE_WITH_JSON_PATCH_EXTENSION). They also verify that a subsequent GET request has the correct values. I recommend doing a GET here and making sure that is as expected.
There was a problem hiding this comment.
I have updated the test cases.
| Map<String, Object> attributes = response.path("'atomic:results'[1].data.attributes"); | ||
| assertEquals("Foo2", attributes.get("commonName")); | ||
| assertEquals("Updated Description", attributes.get("description")); | ||
|
|
There was a problem hiding this comment.
Same comment. Do another round trip request and verify the response matches what we expect (to confirm the database was updated correctly).
There was a problem hiding this comment.
I have updated the test cases.
| ), | ||
| atomicOperation(AtomicOperationCode.add, | ||
| ref( | ||
| type("group/com.example.operationsrel1/products"), |
There was a problem hiding this comment.
Is this a valid way of referencing type? This looks more like a href. It doesn't seem like a ref is a very powerful way of building a path - and to me, it looks like type is referring to type in the core spec.
There was a problem hiding this comment.
I'm actually not sure. I thought the intent was that the ref could always be expressed as a href and vice-versa. It's not really that clear from the spec why there were 2 ways to express the target of the operation.
There was a problem hiding this comment.
Agree the spec is confusing. My take is that some/most JSON-API APIs only expose models at the root of the URL path (/books, /invoices). Then the REF objects can be used to navigate to most things.
The type of the REF then is really the root collection.
There was a problem hiding this comment.
I have updated the test case to use the href instead of the ref.
|
One thing that's a little vague in the spec is the use of LID. I assume it means that an ID in a create op is referenced in LID in subsequent ops. We might also want to test that scenario to make sure it works as intended. |
|
Have added the test cases for LID. Initially I thought that LID was specific to the atomic operations spec but I see it's actually part of the base 1.1 spec. I think I should have addressed all the outstanding issues. |
Resolves #2915
Description
Adds support for JSON API Atomic Operations.
How Has This Been Tested?
Added the unit and integration tests
License
I confirm that this contribution is made under an Apache 2.0 license and that I have the authority necessary to make this contribution on behalf of its copyright owner.