Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Add JAX-RS PATCH annotation #36

Merged
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,12 @@

### Enhancements
* Disable the stagemonitor web widget in the service generated by the archetype, as the default configuration.
* Added toString implementation to DelegatingGenericResponse and BuiltGenericResponse.
* Added a PATCH annotation because JAX-rs does not have one.

### Defects Corrected
* Cleaned up dependencies and fixed few minor issues with generated code in archetype.

### Enhancements
* Added toString implementation to DelegatingGenericResponse and BuiltGenericResponse.

## 2.4 - 16 Feb 2017

### Additions
Expand Down
2 changes: 2 additions & 0 deletions CONTRIBUTORS.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
* Jason Gassel [@jpeg][jason-gassel]
* Andy Nelson [@anelson425][andy-nelson]
* Bryan Baugher [@bbaugher][bryan-baugher]
* Eric Christensen [@EricChristensen][eric-christensen]

[john-leacox]: https://github.com/johnlcox
[jacob-williams]: https://github.com/brokensandals
Expand All @@ -25,3 +26,4 @@
[jason-gassel]: https://github.com/jpeg
[andy-nelson]: https://github.com/anelson425
[bryan-baugher]: https://github.com/bbaugher
[eric-christensen]: https://github.com/EricChristensen
1 change: 1 addition & 0 deletions checkstyle-suppressions.xml
Original file line number Diff line number Diff line change
Expand Up @@ -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" />
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep that is the reason.

</suppressions>
4 changes: 4 additions & 0 deletions client/beadledom-client-example/example-api/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@
<name>Beadledom Resteasy Client Example API</name>

<dependencies>
<dependency>
<groupId>com.cerner.beadledom</groupId>
<artifactId>beadledom-jaxrs</artifactId>
</dependency>
<dependency>
<groupId>com.fasterxml.jackson.core</groupId>
<artifactId>jackson-annotations</artifactId>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.cerner.beadledom.client.example;

import com.cerner.beadledom.client.example.model.JsonOne;
import com.cerner.beadledom.jaxrs.PATCH;

import javax.ws.rs.Consumes;
import javax.ws.rs.GET;
Expand All @@ -23,4 +24,9 @@ public interface ResourceOne {
@Produces("application/json")
@Consumes("application/json")
JsonOne echo(JsonOne json);

@PATCH
@Produces("application/json")
@Consumes("application/json")
JsonOne patch(JsonOne json);
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.cerner.beadledom.client.example;

import com.cerner.beadledom.client.example.model.JsonTwo;
import com.cerner.beadledom.jaxrs.PATCH;

import javax.ws.rs.Consumes;
import javax.ws.rs.GET;
Expand All @@ -23,4 +24,9 @@ public interface ResourceTwo {
@Produces("application/json")
@Consumes("application/json")
JsonTwo echo(JsonTwo json);

@PATCH
@Produces("application/json")
@Consumes("application/json")
JsonTwo patch(JsonTwo json);
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
import com.fasterxml.jackson.databind.annotation.JsonPOJOBuilder;
import com.google.auto.value.AutoValue;

/**
Expand All @@ -10,21 +12,40 @@
* @author John Leacox
*/
@AutoValue
@JsonDeserialize(builder = AutoValue_JsonOne.Builder.class)
public abstract class JsonOne {

public static Builder builder() {
return new AutoValue_JsonOne.Builder();
}

/**
* Creates a new instance of JsonOne.
*/
@JsonCreator
public static JsonOne create(
@JsonProperty("one") String one,
@JsonProperty("hello") String hello) {
return new AutoValue_JsonOne(one, hello);
return builder()
.setOne(one)
.setHello(hello)
.build();
}

@JsonProperty("one")
public abstract String getOne();

@JsonProperty("hello")
public abstract String getHello();

@AutoValue.Builder
@JsonPOJOBuilder(withPrefix = "set")
public abstract static class Builder {

public abstract Builder setOne(String one);

public abstract Builder setHello(String hello);

public abstract JsonOne build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
import com.fasterxml.jackson.databind.annotation.JsonPOJOBuilder;
import com.google.auto.value.AutoValue;

/**
Expand All @@ -10,21 +12,40 @@
* @author John Leacox
*/
@AutoValue
@JsonDeserialize(builder = AutoValue_JsonTwo.Builder.class)
public abstract class JsonTwo {

public static Builder builder() {
return new AutoValue_JsonTwo.Builder();
}

/**
* Creates a new instance of JsonOne.
*/
@JsonCreator
public static JsonTwo create(
@JsonProperty("two") String two,
@JsonProperty("hello") String hello) {
return new AutoValue_JsonTwo(two, hello);
return builder()
.setTwo(two)
.setHello(hello)
.build();
}

@JsonProperty("two")
public abstract String getTwo();

@JsonProperty("hello")
public abstract String getHello();

@AutoValue.Builder
@JsonPOJOBuilder(withPrefix = "set")
public abstract static class Builder {

public abstract Builder setTwo(String two);

public abstract Builder setHello(String hello);

public abstract JsonTwo build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,12 @@ public JsonOne get() {
public JsonOne echo(JsonOne json) {
return json;
}

@Override
public JsonOne patch(JsonOne json) {
return json.builder()
.setHello("Hola")
.setOne("New Json")
.build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,12 @@ public JsonTwo get() {
public JsonTwo echo(JsonTwo json) {
return json;
}

@Override
public JsonTwo patch(JsonTwo json) {
return json.builder()
.setHello("Hola")
.setTwo("New Json")
.build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,15 @@ class ClientServiceSpec(contextRoot: String, servicePort: Int)
val resourceOne = injector.getInstance(classOf[ResourceOne])
val resourceTwo = injector.getInstance(classOf[ResourceTwo])

val jsonNewOne = JsonOne.create("New Json", "Hola")
val jsonOne = JsonOne.create("LocalOne", "Hi")
resourceOne.echo(jsonOne) mustBe jsonOne
resourceOne.patch(jsonOne) mustBe jsonNewOne

val jsonNewTwo = JsonTwo.create("New Json", "Hola")
Copy link
Contributor

Choose a reason for hiding this comment

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

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

val jsonTwo = JsonTwo.create("LocalTwo", "Howdy")
resourceTwo.echo(jsonTwo) mustBe jsonTwo
resourceTwo.patch(jsonTwo) mustBe jsonNewTwo
}

it("each client gets its own unique object mapper") {
Expand All @@ -53,7 +57,6 @@ class ClientServiceSpec(contextRoot: String, servicePort: Int)
mapperTwo.isEnabled(SerializationFeature.INDENT_OUTPUT) must be(true)
}


it("provides default object mapper") {
val injector = getInjector(List(new ResourceOneModule, new ResourceTwoModule))

Expand Down
14 changes: 14 additions & 0 deletions jaxrs/src/main/java/com/cerner/beadledom/jaxrs/PATCH.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package com.cerner.beadledom.jaxrs;

import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

import javax.ws.rs.HttpMethod;

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

Choose a reason for hiding this comment

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

could you please add some javadoc to this.

}
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ public class FakeModel {
@JsonProperty("inner_models")
public List<FakeInnerModel> innerModels;

public FakeModel() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope all tests pass even after model change


}

public FakeModel(
String id, String name, int times, List<String> tags, List<FakeInnerModel> innerModels) {
this.id = id;
Expand All @@ -37,6 +41,32 @@ public FakeModel(
this.innerModels = innerModels;
}

public FakeModel setId(String id) {
this.id = id;
return this;
}

public FakeModel setName(String name) {
this.name = name;
return this;
}

public FakeModel setTimes(int times) {
this.times = times;
return this;
}

public FakeModel setTags(List<String> tags) {
this.tags = tags;
return this;
}

public FakeModel setInnerModels(
List<FakeInnerModel> innerModels) {
this.innerModels = innerModels;
return this;
}

@JsonProperty("id")
public String getId() {
return id;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package com.cerner.beadledom.jaxrs.provider;

import com.cerner.beadledom.jaxrs.PATCH;

import javax.ws.rs.Consumes;
import javax.ws.rs.Path;
import javax.ws.rs.Produces;
import javax.ws.rs.core.MediaType;
import javax.ws.rs.core.Response;

@Path("/fakeResource")
public class FakeResource {

@PATCH
@Path("/Patch")
@Produces(MediaType.APPLICATION_JSON)
@Consumes(MediaType.APPLICATION_JSON)
public Response fakePatch(FakeModel model) {
model.setId("newId");
model.setName("newName");
return Response.ok(model).build();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra newline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}
77 changes: 77 additions & 0 deletions jaxrs/src/test/scala/com/cerner/beadledom/jaxrs/PATCHSpec.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
package com.cerner.beadledom.jaxrs

import com.cerner.beadledom.jaxrs.provider.{FakeModel, FakeResource}

import com.fasterxml.jackson.databind.ObjectMapper
import com.fasterxml.jackson.jaxrs.json.JacksonJsonProvider

import org.jboss.resteasy.core.Dispatcher
import org.jboss.resteasy.mock.{MockDispatcherFactory, MockHttpRequest, MockHttpResponse}
import org.jboss.resteasy.plugins.server.resourcefactory.POJOResourceFactory
import org.scalatest._
import org.scalatest.mock.MockitoSugar

import java.io._

import scala.collection.JavaConverters._
import javax.ws.rs.core.MediaType

/**
* Spec tests for {@link PATCH}.
*
* @author Eric Christensen
*/
class PATCHSpec extends FunSpec with BeforeAndAfterAll with ShouldMatchers with MockitoSugar {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@EricChristensen EricChristensen May 15, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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


val dispatcher: Dispatcher = MockDispatcherFactory.createDispatcher()
val noDefaults: POJOResourceFactory = new POJOResourceFactory(classOf[FakeResource])
dispatcher.getRegistry.addResourceFactory(noDefaults)
dispatcher.getProviderFactory.registerProvider(classOf[JacksonJsonProvider])

describe("PATCH") {
it("calls PATCH resource without error") {

val model = new FakeModel()
.setId("id")
.setName("name")
.setTimes(10)
.setTags(List.empty[String].asJava)
.setInnerModels(List.empty[FakeModel.FakeInnerModel].asJava)

val mapper = new ObjectMapper
val request: MockHttpRequest = MockHttpRequest.create("PATCH", "/fakeResource/Patch")
request.contentType(MediaType.APPLICATION_JSON)
request.content(new ByteArrayInputStream(mapper.writeValueAsBytes(model)))

val response: MockHttpResponse = new MockHttpResponse()
dispatcher.invoke(request, response)

response.getStatus shouldBe 200
}

it("should change the request model fields") {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


val model = new FakeModel()
.setId("id")
.setName("name")
.setTimes(10)
.setTags(List.empty[String].asJava)
.setInnerModels(List.empty[FakeModel.FakeInnerModel].asJava)

val mapper = new ObjectMapper
val request: MockHttpRequest = MockHttpRequest.create("PATCH", "/fakeResource/Patch")
request.contentType(MediaType.APPLICATION_JSON)
request.content(new ByteArrayInputStream(mapper.writeValueAsBytes(model)))

val response: MockHttpResponse = new MockHttpResponse()
dispatcher.invoke(request, response)

response.getStatus shouldBe 200

val newModel: FakeModel = mapper.readValue(response.getContentAsString, classOf[FakeModel])
newModel.name shouldBe "newName"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

newModel.id shouldBe "newId"
newModel.times shouldBe 10
}
}
}