[GitHub] [cxf] reta opened a new pull request #692: CXF-8227: Failure to comply with JAX-RS spec with ContainerRequestContext and WriterInterceptorContext

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] reta opened a new pull request #692: CXF-8227: Failure to comply with JAX-RS spec with ContainerRequestContext and WriterInterceptorContext

GitBox

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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] andymc12 commented on a change in pull request #692: CXF-8227: Failure to comply with JAX-RS spec with ContainerRequestContext and WriterInterceptorContext

GitBox

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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] reta commented on a change in pull request #692: CXF-8227: Failure to comply with JAX-RS spec with ContainerRequestContext and WriterInterceptorContext

GitBox
In reply to this post by GitBox

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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] reta commented on a change in pull request #692: CXF-8227: Failure to comply with JAX-RS spec with ContainerRequestContext and WriterInterceptorContext

GitBox
In reply to this post by GitBox

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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] reta commented on pull request #692: CXF-8227: Failure to comply with JAX-RS spec with ContainerRequestContext and WriterInterceptorContext

GitBox
In reply to this post by GitBox

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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] reta merged pull request #692: CXF-8227: Failure to comply with JAX-RS spec with ContainerRequestContext and WriterInterceptorContext

GitBox
In reply to this post by GitBox

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:
[hidden email]