Add JAX-RS PATCH annotation #36

EricChristensen · 2017-05-15T18:16:01Z

What was changed? Why is this necessary?

From issue 21 JAX-RS does not have a PATCH annotation for supporting HTTP patch method like it does for others like GET or POST. We should add a PATCH annotation that can be used for handling the patch HTTP method.

How was it tested?

The PATCH annotation was tested by consuming this change in a test branch of a project that already consumes beadledom. In this project, a simple resource that uses the PATCH annotation was used to manually make sure that the behavior performed as expected. Two unit tests were also added to the project to 1. Ensure that PATCH is used and behaves as expected and 2. PATCH is used and performs an operation on the server to ensure that serialization works as expected.

How to test

mvn clean install -U

Reviewers

johnlcox · 2017-05-15T18:24:48Z

jaxrs/src/test/scala/com/cerner/beadledom/jaxrs/PATCHSpec.scala

+  *
+  * @author Eric Christensen
+  */
+class PATCHSpec extends FunSpec with BeforeAndAfterAll with ShouldMatchers with MockitoSugar {


I'd like to see a test with an actual stood up service. Might fit in https://github.com/cerner/beadledom/blob/c2125c7228f9a778b56a07615f00a4d729a86270/client/beadledom-client-test/src/test/scala/com/cerner/beadledom/client/ClientServiceSpec.scala

PATCH used with an actual stood up service in the "retrieves the resources from different clients" test d582c3a

I think it should be a separate test directly in the describe("Proxied Clients") block that tests only that patch works.

kyleroush · 2017-05-15T18:31:13Z

jaxrs/src/test/scala/com/cerner/beadledom/jaxrs/PATCHSpec.scala

+        response.getStatus shouldBe 200
+      }
+
+      it("should change the request model fields") {


Why do we need to have these two test, when you are calling the function with the same input twice?

They are indeed the same function calls with the same parameters but it seemed appropriate to have two different "it" descriptors, one for just making sure that PATCH is called successfully and one to make sure that serialization takes place properly by making sure the server performed a change and returned it.

I would merge the two into one test. We are not testing two different aspects of the method here. Status code 200 means the request is served successfully. For PATCH a successful request mean that the resource is patched correctly. So, having these two checks in one test makes sense IMO.

kyleroush · 2017-05-15T18:32:06Z

jaxrs/src/test/scala/com/cerner/beadledom/jaxrs/PATCHSpec.scala

+        response.getStatus shouldBe 200
+
+        val newModel: FakeModel = mapper.readValue(response.getContentAsString, classOf[FakeModel])
+        newModel.name shouldBe "newName"


In theory, if this was a real patch shouldn't these responses be what you sent to the patch?

The PATCH resource updates the request that was sent to it. Normally the update would be to an existing resource in a database, but for the purposes of just performing an operation on the server this is fine.

kyleroush · 2017-05-15T18:34:35Z

jaxrs/src/test/java/com/cerner/beadledom/jaxrs/provider/FakeModel.java

@@ -28,6 +28,10 @@
  @JsonProperty("inner_models")
  public List<FakeInnerModel> innerModels;

+  public FakeModel() {


why did you add a default constructor? the only place I see you using it is here where you use the constructor that already exist.

The default constructer and the setters were added to make the creating of the FakeModel more like the builder pattern. It is used in the Spec at the recommendation of Brian.

We didn't break of the existing tests using this model did we? I can't imagine so but you never know.

Nope all tests pass even after model change

b-boogaard · 2017-05-16T20:47:22Z

jaxrs/src/test/java/com/cerner/beadledom/jaxrs/provider/FakeResource.java

+    model.setName("newName");
+    return Response.ok(model).build();
+  }
+


nit: extra newline

b-boogaard · 2017-05-16T20:54:44Z

I'm not sure exactly what we would say about it, but it seems like we would want to add the fact we added a @PATCH annotation to the manual for the jax-rs module

Edit:

In case you're not sure, the changes would be made under the docs module. We use reStructuredText as our markup.

EricChristensen · 2017-05-16T21:17:12Z

@b-boogaard what do you think of this? a1fac9f

b-boogaard · 2017-05-16T21:24:27Z

I think I would add it either above or below the Correlation-Id header. I think I would want to link to https://tools.ietf.org/html/rfc5789 the rfc that added PATCH to HTTP. Then give an interface like example of a resource using it, pointing out it can be used just like the others that are defined by JAX-RS.

@johnlcox @sparuvu @lal-s @nimeshsubramanian thoughts?

EricChristensen · 2017-05-16T21:58:11Z

@b-boogaard Interface example and link to RFC

b-boogaard · 2017-05-16T23:25:28Z

How about something like:

JAX-RS 2.0 (see section 3.3 for Resource Methods) does not require implementations to support the PATCH HTTP method. This is likely do to the fact that PATCH was introduced in a later rfc that added the new HTTP method to the already existing HTTP/1.1 specification.

@PATCH was added to beadledom-jaxrs to allow services to support partial updates without the need of overloading @POST. The annotation has no opinion on how the service decides to implement the resource performing the PATCH operation. Implementing services have the freedom to support JSON Patch and/or JSON Merge Patch.

As long as a service has beadledom-jaxrs as a dependency @PATCH can be used just like any of the HTTP method annotations defined by JAX-RS. Below is a small example of @PATCH being used in an interface for a resource.

code example here

I think this could use a bit a clean up but I feel like it reads a bit better. Thoughts?

sparuvu · 2017-05-17T01:59:46Z

client/beadledom-client-test/src/test/scala/com/cerner/beadledom/client/ClientServiceSpec.scala


+        val jsonNewTwo = JsonTwo.create("New Json", "Hola")


could you please differentiate jsonNewTwo from jsonNewOne (just change Hola to Hola1 and Hola2 or something simple)

sparuvu · 2017-05-17T02:09:15Z

jaxrs/src/main/java/com/cerner/beadledom/jaxrs/PATCH.java

+@Target({ElementType.METHOD})
+@Retention(RetentionPolicy.RUNTIME)
+@HttpMethod("PATCH")
+public @interface PATCH {


could you please add some javadoc to this.

sparuvu · 2017-05-17T02:15:30Z

jaxrs/src/test/scala/com/cerner/beadledom/jaxrs/PATCHSpec.scala

+        response.getStatus shouldBe 200
+      }
+
+      it("should change the request model fields") {


I would merge the two into one test. We are not testing two different aspects of the method here. Status code 200 means the request is served successfully. For PATCH a successful request mean that the resource is patched correctly. So, having these two checks in one test makes sense IMO.

sparuvu · 2017-05-17T03:03:34Z

docs/source/manual/jaxrs.rst

+    PatchObject patchObject);
+
+.. _RFC: https://tools.ietf.org/html/rfc5789
+


How about this?

PATCH -------------- `PATCH <https://github.com/ktoso/maven-git-commit-id-plugin>`_ is a HTTP verb (similar to GET, POST, DELETE, PUT) to push partial changes to the REST resource. JAX-RS by default doesn't support PATCH hence Beadledom adds the support for PATCH explicitly. For more details on the PATCH method visit the PATCH RFC_. To use the PATCH method and annotation for your resource, simply annotate your JAX-RS resource method with the PATCH annotation as shown below .. code-block:: java @PATCH @Path("path/to/patch") @Produces(MediaType.APPLICATION_JSON) public Response patch( @PathParam("id") final Long id, @ApiParam(value = "changes to make to the object with the specified id") PatchObject patchObject); .. _RFC: https://tools.ietf.org/html/rfc5789

EricChristensen · 2017-05-17T14:06:03Z

@sparuvu comments addressed here. @b-boogaard I went with Sundeeps description of the annotation in the jaxrs.rst. Concise, simple and exactly what the user needs to know. I removed the git url that was linked in the description because don't see how it relates to PATCH at all...

sparuvu · 2017-05-17T14:08:17Z

jaxrs/src/main/java/com/cerner/beadledom/jaxrs/PATCH.java

+/**
+ * Indicates that the annotated method responds to HTTP PATCH requests.
+ *
+ * @author John Leacox


I think this should be you :) and not John

If you insist :) bb8be52

sparuvu · 2017-05-17T14:09:38Z

jaxrs/src/main/java/com/cerner/beadledom/jaxrs/PATCH.java

+import javax.ws.rs.HttpMethod;
+
+/**
+ * Indicates that the annotated method responds to HTTP PATCH requests.


may be - Annotation to support the Http PATCH requests.

The current description is consistent with other jaxrs http annotation descriptions https://docs.oracle.com/javaee/7/api/javax/ws/rs/PUT.html

+1 to matching the jaxrs descriptions.

sparuvu · 2017-05-17T14:12:33Z

docs/source/manual/jaxrs.rst

+PATCH
+--------------
+
+`PATCH is a HTTP verb (similar to GET, POST, DELETE, PUT) to push partial changes to the REST resource. JAX-RS by default doesn't support PATCH hence Beadledom adds the support for PATCH explicitly.


IMO, it would be good if we could link the PATCH to this wiki article - https://en.wikipedia.org/wiki/Patch_verb

link to wiki on brian's version of rst f388ee2

sparuvu · 2017-05-17T14:30:30Z

@sparuvu comments addressed here. @b-boogaard I went with Sundeeps description of the annotation in the jaxrs.rst. Concise, simple and exactly what the user needs to know. I removed the git url that was linked in the description because don't see how it relates to PATCH at all...

I think I like what Brian has. It is more detailed and more importantly it points that implementing services have the freedom to implement whatever PATCH algorithm they want to go with.

EricChristensen · 2017-05-17T15:10:27Z

@sparuvu brian's version of jaxrs.rst bb8be52

johnlcox · 2017-05-17T15:16:01Z

checkstyle-suppressions.xml

@@ -4,4 +4,5 @@
    <suppress files="generated-sources" checks=".*"/>
    <suppress files="generated-test-sources" checks=".*"/>
    <suppress files="com.cerner.beadledom.client.resteasy.ApacheHttpClient4Dot3Engine" checks=".*"/>
+    <suppress files="com.cerner.beadledom.jaxrs.PATCH" checks="AbbreviationAsWordInName" />


I just want to confirm this is necessary because checkstyle is expecting the classname to be Patch?

If JAX-RS hadn't already set the standard for these annotations we would name it that way, but PATCH is correct in following the standard of the other JAX-RS annotations.

Yep that is the reason.

johnlcox · 2017-05-17T15:24:22Z

jaxrs/src/main/java/com/cerner/beadledom/jaxrs/PATCH.java

+/**
+ * Indicates that the annotated method responds to HTTP PATCH requests.
+ *
+ * @author Eric Christensen


Add @since 2.5 below author.

johnlcox · 2017-05-17T15:28:21Z

+1 to @b-boogaard's doc text.

johnlcox · 2017-05-17T15:31:11Z

Will it be desired for beadledom to include a class with additional mediatypes like json-patch and json-merge-patch that can be used similarly to MediaType.APPLICATION_JSON?

This questions came to mind while looking at this review, but I think it would be a separate code change if it is desired.

EricChristensen · 2017-05-17T15:34:29Z

@johnlcox we are planning to use json-merge-patch in our application so yes I would say that is a desired behavior.

b-boogaard · 2017-05-17T15:38:35Z

I think adding the media types for patch is a good idea.

EricChristensen · 2017-05-17T15:42:12Z

Is there any reason that adding the media types could not be done in this code change?

johnlcox · 2017-05-17T15:46:12Z

I'd rather see it as a separate code change because I think there needs to be some discussion on the strategy we want to use for supporting additional media types in general. This probably warrants starting with an issue for that discussion prior to any code changes.

johnlcox · 2017-05-17T15:58:44Z

+1

b-boogaard · 2017-05-17T18:12:18Z

+1

johnlcox · 2017-05-17T19:53:16Z

I logged #37 for the media type discussion.

sparuvu · 2017-05-18T13:57:18Z

+1

nimeshsubramanian · 2017-05-18T17:12:52Z

+1

Add JAX-RS PATCH annotation

e5c090e

johnlcox reviewed May 15, 2017

View reviewed changes

kyleroush reviewed May 15, 2017

View reviewed changes

EricChristensen and others added 3 commits May 15, 2017 16:52

Use PATCH on a stood up service

d582c3a

Move dependency to alphabetical spot

f093e10

little change to see config changes

203e4a6

b-boogaard reviewed May 16, 2017

View reviewed changes

extra newline

48dafb3

add to jaxrs.rst

a1fac9f

EricChristensen added 5 commits May 16, 2017 16:49

add example and link to rfc for patch description

00de0c5

remove extra underscore

7ea7dec

remove the other underscore?

20ef4a8

add period instead of <<

32acdeb

add _ back

f8347b0

sparuvu reviewed May 17, 2017

View reviewed changes

EricChristensen added 2 commits May 17, 2017 09:00

Sundeeps comments

7931147

remove mysterious url

84431c4

sparuvu reviewed May 17, 2017

View reviewed changes

link wiki

d564ac3

add proper name and brians version of rst

bb8be52

link to wiki on new rst

f388ee2

johnlcox reviewed May 17, 2017

View reviewed changes

add since 2.5 in patch javadoc

ffcd9d0

nimeshsubramanian mentioned this pull request May 17, 2017

Add JAX-RS PATCH annotation #21

Closed

b-boogaard merged commit e83ff7c into cerner:master May 18, 2017

b-boogaard added this to the 2.5 milestone May 19, 2017

b-boogaard assigned EricChristensen May 19, 2017

		PatchObject patchObject);

		.. _RFC: https://tools.ietf.org/html/rfc5789

Add JAX-RS PATCH annotation #36

Add JAX-RS PATCH annotation #36

Conversation

EricChristensen commented May 15, 2017 • edited by nimeshsubramanian Loading

What was changed? Why is this necessary?

How was it tested?

How to test

Reviewers

Choose a reason for hiding this comment

EricChristensen May 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-boogaard commented May 16, 2017 • edited Loading

EricChristensen commented May 16, 2017

b-boogaard commented May 16, 2017

EricChristensen commented May 16, 2017

b-boogaard commented May 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EricChristensen commented May 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sparuvu commented May 17, 2017

EricChristensen commented May 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnlcox commented May 17, 2017

johnlcox commented May 17, 2017

EricChristensen commented May 17, 2017

b-boogaard commented May 17, 2017

EricChristensen commented May 17, 2017

johnlcox commented May 17, 2017

johnlcox commented May 17, 2017

b-boogaard commented May 17, 2017

johnlcox commented May 17, 2017

sparuvu commented May 18, 2017

nimeshsubramanian commented May 18, 2017

EricChristensen commented May 15, 2017 •

edited by nimeshsubramanian

Loading

EricChristensen May 15, 2017 •

edited

Loading

b-boogaard commented May 16, 2017 •

edited

Loading