-
Notifications
You must be signed in to change notification settings - Fork 30
Add JAX-RS PATCH annotation #36
Changes from 1 commit
e5c090e
d582c3a
f093e10
203e4a6
48dafb3
a1fac9f
00de0c5
7ea7dec
20ef4a8
32acdeb
f8347b0
7931147
84431c4
d564ac3
bb8be52
f388ee2
ffcd9d0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
---|---|---|
|
@@ -28,6 +28,10 @@ public class FakeModel { | |
@JsonProperty("inner_models") | ||
public List<FakeInnerModel> innerModels; | ||
|
||
public FakeModel() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
@@ -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; | ||
|
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(); | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: extra newline There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} |
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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it should be a separate test directly in the |
||
|
||
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") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.