You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cxf.apache.org by GitBox <gi...@apache.org> on 2020/09/09 01:48:24 UTC
[GitHub] [cxf] reta opened a new pull request #692: CXF-8227: Failure to comply with JAX-RS spec with ContainerRequestContext and WriterInterceptorContext
reta opened a new pull request #692:
URL: https://github.com/apache/cxf/pull/692
For some reasons, the implementation branched off into request / message property holders. If the request was available in the message context, all properties were mapped to request attributes, otherwise to message properties container. In order to preserve the compatibility, the fix still supports request attributes however at the same time populates message properties container.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] reta commented on pull request #692: CXF-8227: Failure to comply with JAX-RS spec with ContainerRequestContext and WriterInterceptorContext
Posted by GitBox <gi...@apache.org>.
reta commented on pull request #692:
URL: https://github.com/apache/cxf/pull/692#issuecomment-689880156
Thanks a lot for the review, @andymc12 !
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] reta commented on a change in pull request #692: CXF-8227: Failure to comply with JAX-RS spec with ContainerRequestContext and WriterInterceptorContext
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #692:
URL: https://github.com/apache/cxf/pull/692#discussion_r485943617
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/PropertyHolderFactory.java
##########
@@ -41,6 +46,60 @@ public static PropertyHolder getPropertyHolder(Message m) {
void setProperty(String name, Object value);
Collection<String> getPropertyNames();
}
+
+ private static class ServletRequestPropertyHolder extends MessagePropertyHolder {
+ private static final String ENDPOINT_ADDRESS_PROPERTY = "org.apache.cxf.transport.endpoint.address";
+ private HttpServletRequest request;
Review comment:
:+1: correct, thank you!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] andymc12 commented on a change in pull request #692: CXF-8227: Failure to comply with JAX-RS spec with ContainerRequestContext and WriterInterceptorContext
Posted by GitBox <gi...@apache.org>.
andymc12 commented on a change in pull request #692:
URL: https://github.com/apache/cxf/pull/692#discussion_r485714951
##########
File path: systests/jaxrs/src/test/java/org/apache/cxf/systest/jaxrs/BookApplication.java
##########
@@ -106,6 +106,12 @@ public void setDefaultId(List<String> ids) {
public void aroundWriteTo(WriterInterceptorContext context) throws IOException,
WebApplicationException {
context.getHeaders().putSingle("BookWriter", "TheBook");
+
+ final Object property = context.getProperty("x-book");
+ if (property != null) {
+ context.getHeaders().putSingle("X-Book-Header", property);
Review comment:
```suggestion
context.getHeaders().putSingle("X-Book-Response-Header", property);
```
Just a suggestion, but IIUC, the intent is to show that property set from the request filter is propagated to the writer interceptor (in the response flow). By using a different header in the response, it makes it clear that the test is not just propagating headers from the request back to the response.
##########
File path: systests/jaxrs/src/test/java/org/apache/cxf/systest/jaxrs/JAXRSClientServerNonSpringBookTest.java
##########
@@ -186,17 +186,33 @@ public void testGetBook123Application11PerRequest() throws Exception {
doTestPerRequest("http://localhost:" + PORT + "/application11/thebooks/bookstore2/bookheaders");
assertEquals("TheBook", r.getHeaderString("BookWriter"));
}
+
+ @Test
+ public void testGetBook123Application11PerRequestWithHeader() throws Exception {
+ Response r =
+ doTestPerRequest("http://localhost:" + PORT + "/application11/thebooks/bookstore2/bookheaders",
+ "PropValue");
+ assertEquals("PropValue", r.getHeaderString("X-Book-Header"));
Review comment:
```suggestion
assertEquals("PropValue", r.getHeaderString("X-Book-Response-Header"));
```
see previous comment
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/PropertyHolderFactory.java
##########
@@ -41,6 +46,60 @@ public static PropertyHolder getPropertyHolder(Message m) {
void setProperty(String name, Object value);
Collection<String> getPropertyNames();
}
+
+ private static class ServletRequestPropertyHolder extends MessagePropertyHolder {
+ private static final String ENDPOINT_ADDRESS_PROPERTY = "org.apache.cxf.transport.endpoint.address";
+ private HttpServletRequest request;
Review comment:
```suggestion
private final HttpServletRequest request;
```
it looks like this is not intended to be changed, so we could explicitly mark it as final.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] reta merged pull request #692: CXF-8227: Failure to comply with JAX-RS spec with ContainerRequestContext and WriterInterceptorContext
Posted by GitBox <gi...@apache.org>.
reta merged pull request #692:
URL: https://github.com/apache/cxf/pull/692
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] reta commented on a change in pull request #692: CXF-8227: Failure to comply with JAX-RS spec with ContainerRequestContext and WriterInterceptorContext
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #692:
URL: https://github.com/apache/cxf/pull/692#discussion_r485964407
##########
File path: systests/jaxrs/src/test/java/org/apache/cxf/systest/jaxrs/BookApplication.java
##########
@@ -106,6 +106,12 @@ public void setDefaultId(List<String> ids) {
public void aroundWriteTo(WriterInterceptorContext context) throws IOException,
WebApplicationException {
context.getHeaders().putSingle("BookWriter", "TheBook");
+
+ final Object property = context.getProperty("x-book");
+ if (property != null) {
+ context.getHeaders().putSingle("X-Book-Header", property);
Review comment:
Completely agree, rewrote the test to reflect its purpose, thanks a lot!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org