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/10/02 18:53:00 UTC
[GitHub] [cxf] andymc12 opened a new pull request #703: Fix systests to account for fix in CXF-8346
andymc12 opened a new pull request #703:
URL: https://github.com/apache/cxf/pull/703
This should resolve 4 errors/failures in systests that resulted from runtime changes in CXF-8346 / PR #697.
The first change is to remove query parameters. These should need to be "replaced" as the query parameter was specified on the original request. By replacing them, it actually appended them, making the request URI look something like: `http://localhost:8080/bookstore/books/check2?a=b?a=b` which is not allowed (the extra question mark).
The other changes were to re-acquire the Response object rather than just re-using them. This was a clear test bug, since it was previously checking the response from the first request twice rather than checking the response from both requests.
This should resolve 8 failures in the master builds since these 4 tests are run twice (once for Java 8 and once for Java 11).
----------------------------------------------------------------
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 pull request #703: Fix systests to account for fix in CXF-8346
Posted by GitBox <gi...@apache.org>.
andymc12 commented on pull request #703:
URL: https://github.com/apache/cxf/pull/703#issuecomment-705054481
No worries @amarkevich!
There are still some failures in osgi/itests, but they pass for me locally. Maybe it's an intermittent issue, or something specific to the env/JDK/etc? I'll go ahead and merge these changes, as they definitely fix more tests than they break. :). Thanks!
----------------------------------------------------------------
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 pull request #703: Fix systests to account for fix in CXF-8346
Posted by GitBox <gi...@apache.org>.
andymc12 commented on pull request #703:
URL: https://github.com/apache/cxf/pull/703#issuecomment-703341976
> sorry for not having the Github + Jenkins in place to help with test cases validation, we are working on that
no worries. sorry for not testing these changes with the systests locally ahead of time.
----------------------------------------------------------------
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 #703: Fix systests to account for fix in CXF-8346
Posted by GitBox <gi...@apache.org>.
andymc12 commented on a change in pull request #703:
URL: https://github.com/apache/cxf/pull/703#discussion_r499303451
##########
File path: systests/jaxrs/src/test/java/org/apache/cxf/systest/jaxrs/JAXRSClientServerBookTest.java
##########
@@ -926,15 +926,12 @@ public void testCachingExpiresUsingETag() throws Exception {
// Now make a second call. The value in the clients cache will have expired, so it should call
// out to the service, which will return 304, and the client will re-use the cached payload
Review comment:
I took that to mean that the client should keep on using the same `Book` entity that it read from the first Response, but now that you mention it, it isn't very clear.
I think we can either update the text of the comment to indicate that the client process should re-use the previously obtained entity, or we could add `response.bufferEntity()` and then pull the entity from the original response - something like this:
```
Response response2 = target.request().get();
assertEquals(304, response2.getStatus());
assertFalse(response2.hasEntity());
Book book2 = response.readEntity(Book.class);
assertEquals(123L, book2.getId());
cacheControlFeature.close();
```
Wdyt?
----------------------------------------------------------------
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 #703: Fix systests to account for fix in CXF-8346
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #703:
URL: https://github.com/apache/cxf/pull/703#discussion_r499307465
##########
File path: systests/jaxrs/src/test/java/org/apache/cxf/systest/jaxrs/BookServer20.java
##########
@@ -227,7 +227,7 @@ public void filter(ContainerRequestContext context) throws IOException {
} else if (path.endsWith("books/check2")) {
replaceStream(context);
} else if (path.endsWith("books/checkNQuery")) {
- URI requestURI = URI.create(path.replace("NQuery", "2?a=b"));
+ URI requestURI = URI.create(path.replace("NQuery", "2"));
Review comment:
> Hmm... maybe I don't fully understand. I think the test was broken from the start since it was effectively setting the RequestUri to `/bookstore/books/check2?a=b?a=b` - the previous behavior (prior to #697) was to simply ignore the query string entirely which was why the test passed even though the URI was invalid. After #697 the test failed because we were actually checking the query string.
I think the test case was fine, let me try to illustrate what was happening before the change and after, for simplicity I will just include the URI into method arguments.
So before:
```
containerRequestContext.setRequestUri("/bookstore/books/check2?a=b")
m.get(Message.REQUEST_URI) -> /bookstore/books/check2
m.get(Message.QUERY_STRING) -> a=b
```
After the change:
```
containerRequestContext.setRequestUri("/bookstore/books/check2?a=b")
m.get(Message.REQUEST_URI) -> /bookstore/books/check2?a=b
m.get(Message.QUERY_STRING) -> a=b
```
As you can see, `Message.REQUEST_URI` has raw path before, but now - full URI. And that causing the issues. Unfortunately I don't know all the places but it seems like raw URI is being used to find the matching methods to invoke fe.
----------------------------------------------------------------
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 #703: Fix systests to account for fix in CXF-8346
Posted by GitBox <gi...@apache.org>.
andymc12 commented on a change in pull request #703:
URL: https://github.com/apache/cxf/pull/703#discussion_r499305938
##########
File path: systests/jaxrs/src/test/java/org/apache/cxf/systest/jaxrs/BookServer20.java
##########
@@ -227,7 +227,7 @@ public void filter(ContainerRequestContext context) throws IOException {
} else if (path.endsWith("books/check2")) {
replaceStream(context);
} else if (path.endsWith("books/checkNQuery")) {
- URI requestURI = URI.create(path.replace("NQuery", "2?a=b"));
+ URI requestURI = URI.create(path.replace("NQuery", "2"));
Review comment:
> I think although your change does fix the test, the flow is broken by and large, please correct me if I am wrong. The user should be able to change the Request URI in pre-matching filter, including the query string component, but she won't be able to do that from now on.
Hmm... maybe I don't fully understand. I think the test was broken from the start since it was effectively setting the RequestUri to `/bookstore/books/check2?a=b?a=b` - the previous behavior (prior to #697) was to simply ignore the query string entirely which was why the test passed even though the URI was invalid. After #697 the test failed because we were actually checking the query string.
Do you think we need to explicitly set `Message.QUERY_STRING` when the user calls `containerRequestContext.setRequestUri(newUri)`? In that case then we might also need to do some checking and "URI merging" in other places where JAX-RS related code is expecting to see the full URI (including the query string) - which is the point you were making, I think.
Do you know which places in the code are expecting `Message.REQUEST_URI` to be the raw URI? I'd like to take a look to determine whether we should change those parts to expect REQUEST_URI to be the full URI or whether we should change the JAX-RS parts to merge the REQUEST_URI with the QUERY_STRING when checked.
Thanks for the review and the analysis.
----------------------------------------------------------------
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 #703: Fix systests to account for fix in CXF-8346
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #703:
URL: https://github.com/apache/cxf/pull/703#discussion_r500639771
##########
File path: systests/jaxrs/src/test/java/org/apache/cxf/systest/jaxrs/BookServer20.java
##########
@@ -227,7 +227,7 @@ public void filter(ContainerRequestContext context) throws IOException {
} else if (path.endsWith("books/check2")) {
replaceStream(context);
} else if (path.endsWith("books/checkNQuery")) {
- URI requestURI = URI.create(path.replace("NQuery", "2?a=b"));
+ URI requestURI = URI.create(path.replace("NQuery", "2"));
Review comment:
Thanks @andymc12 , now we have better understanding of the impact.
----------------------------------------------------------------
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 #703: Fix systests to account for fix in CXF-8346
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #703:
URL: https://github.com/apache/cxf/pull/703#discussion_r499179674
##########
File path: systests/jaxrs/src/test/java/org/apache/cxf/systest/jaxrs/JAXRSClientServerBookTest.java
##########
@@ -926,15 +926,12 @@ public void testCachingExpiresUsingETag() throws Exception {
// Now make a second call. The value in the clients cache will have expired, so it should call
// out to the service, which will return 304, and the client will re-use the cached payload
Review comment:
Seems like ` ... and the client will re-use the cached payload` is not correct anymore?
----------------------------------------------------------------
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 #703: Fix systests to account for fix in CXF-8346
Posted by GitBox <gi...@apache.org>.
reta commented on pull request #703:
URL: https://github.com/apache/cxf/pull/703#issuecomment-703162944
@andymc12 sorry for not having the Github + Jenkins in place to help with test cases validation, we are working on that
----------------------------------------------------------------
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 #703: Fix systests to account for fix in CXF-8346
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #703:
URL: https://github.com/apache/cxf/pull/703#discussion_r500681806
##########
File path: systests/rs-security/src/test/java/org/apache/cxf/systest/jaxrs/security/oidc/OIDCNegativeTest.java
##########
@@ -113,12 +114,7 @@ public void testImplicitFlowPromptNone() throws Exception {
client.path("authorize-implicit/");
Response response = client.get();
- try {
- response.readEntity(OAuthAuthorizationData.class);
- fail("Failure expected on a bad prompt");
- } catch (Exception ex) {
- // expected
- }
+ assertNull(response.readEntity(OAuthAuthorizationData.class));
Review comment:
👍
----------------------------------------------------------------
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 #703: Fix systests to account for fix in CXF-8346
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #703:
URL: https://github.com/apache/cxf/pull/703#discussion_r499180730
##########
File path: systests/jaxrs/src/test/java/org/apache/cxf/systest/jaxrs/BookServer20.java
##########
@@ -227,7 +227,7 @@ public void filter(ContainerRequestContext context) throws IOException {
} else if (path.endsWith("books/check2")) {
replaceStream(context);
} else if (path.endsWith("books/checkNQuery")) {
- URI requestURI = URI.create(path.replace("NQuery", "2?a=b"));
+ URI requestURI = URI.create(path.replace("NQuery", "2"));
Review comment:
I was debugging this failure and it seems like the change with respect to Request URI
```
HttpUtils.resetRequestURI(m, requestUri.toString());
```
violates some CXF flow (it does not mean the flow is correct, it just probably means we need changes in other places).
In the nutshell, my findings are as such:
- CXF assumes the `m.get(Message.REQUEST_URI)` has only URI raw path component
- CXF assumes the `m.get(Message.QUERY_STRING)` has only URI query string component
The fact that we now set the full URI (with a query string) breaks the way CXF treats the `m.get(Message.REQUEST_URI)` and test fails because as you correctly pointing out, the query string is messed up `/bookstore/books/check2?a=b?a=b`.
I think although your change does fix the test, the flow is broken by and large, please correct me if I am wrong. The user should be able to change the Request URI in pre-matching filter, including the query string component, but she won't be able to do that from now on.
----------------------------------------------------------------
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 pull request #703: Fix systests to account for fix in CXF-8346
Posted by GitBox <gi...@apache.org>.
andymc12 commented on pull request #703:
URL: https://github.com/apache/cxf/pull/703#issuecomment-705000123
I ran into a merge conflict - and I think I've resolved it. I'd like to wait until Jenkins has finished this CI build before merging, but no need to re-review unless you want to check my merge-conflict-resolution. Thanks!
----------------------------------------------------------------
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 pull request #703: Fix systests to account for fix in CXF-8346
Posted by GitBox <gi...@apache.org>.
andymc12 commented on pull request #703:
URL: https://github.com/apache/cxf/pull/703#issuecomment-704629566
> LGTM but can you also check these tests that are also failing in systests/rs-security?
>
> OIDCNegativeTest.testImplicitFlowNoNonce
> OIDCNegativeTest.testImplicitFlowPromptNone
Yes, it looks like these tests were expecting the response to throw an exception rather than return null. The javadoc is not clear what to expect (null vs ProcessingException that wraps a NoContentException), but the TCK expects that if the passed-in type can be null (i.e. not a primitive), then the readEntity method must return null. I updated the test to assertNull, and it is passing locally for me.
----------------------------------------------------------------
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 #703: Fix systests to account for fix in CXF-8346
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #703:
URL: https://github.com/apache/cxf/pull/703#discussion_r500639994
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ContainerRequestContextImpl.java
##########
@@ -123,8 +123,9 @@ public void setRequestUri(URI requestUri) throws IllegalStateException {
protected void doSetRequestUri(URI requestUri) throws IllegalStateException {
checkNotPreMatch();
- // The JAX-RS TCK requires the full uri toString() rather than just the raw path:
- HttpUtils.resetRequestURI(m, requestUri.toString());
+ // TODO: The JAX-RS TCK requires the full uri toString() rather than just the raw path, but
+ // changing to toString() seems to have adverse effects downstream. Needs more investigation.
+ HttpUtils.resetRequestURI(m, requestUri.getRawPath());
Review comment:
👍
----------------------------------------------------------------
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 #703: Fix systests to account for fix in CXF-8346
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #703:
URL: https://github.com/apache/cxf/pull/703#discussion_r499306024
##########
File path: systests/jaxrs/src/test/java/org/apache/cxf/systest/jaxrs/JAXRSClientServerBookTest.java
##########
@@ -926,15 +926,12 @@ public void testCachingExpiresUsingETag() throws Exception {
// Now make a second call. The value in the clients cache will have expired, so it should call
// out to the service, which will return 304, and the client will re-use the cached payload
Review comment:
👍 to adding these assertions, thanks!
----------------------------------------------------------------
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 merged pull request #703: Fix systests to account for fix in CXF-8346
Posted by GitBox <gi...@apache.org>.
andymc12 merged pull request #703:
URL: https://github.com/apache/cxf/pull/703
----------------------------------------------------------------
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 #703: Fix systests to account for fix in CXF-8346
Posted by GitBox <gi...@apache.org>.
andymc12 commented on a change in pull request #703:
URL: https://github.com/apache/cxf/pull/703#discussion_r500623000
##########
File path: systests/jaxrs/src/test/java/org/apache/cxf/systest/jaxrs/BookServer20.java
##########
@@ -227,7 +227,7 @@ public void filter(ContainerRequestContext context) throws IOException {
} else if (path.endsWith("books/check2")) {
replaceStream(context);
} else if (path.endsWith("books/checkNQuery")) {
- URI requestURI = URI.create(path.replace("NQuery", "2?a=b"));
+ URI requestURI = URI.create(path.replace("NQuery", "2"));
Review comment:
Ah, that makes sense. For now, I'm reverting that portion of the change in ContainerRequestContextImpl - I added a TODO so that we won't lose sight of it, but that should get the tests passing again. Now I'll take a look at the OIDC tests that are failing. It's up to you whether we should merge this now to get the JAX-RS systests back to 100% passing or wait until we get the OIDC tests passing too. Thanks again!
----------------------------------------------------------------
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 #703: Fix systests to account for fix in CXF-8346
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #703:
URL: https://github.com/apache/cxf/pull/703#discussion_r500682339
##########
File path: systests/rs-security/src/test/java/org/apache/cxf/systest/jaxrs/security/oidc/OIDCNegativeTest.java
##########
@@ -196,12 +192,7 @@ public void testImplicitFlowNoNonce() throws Exception {
client.path("authorize-implicit/");
Response response = client.get();
- try {
- response.readEntity(OAuthAuthorizationData.class);
- fail("Failure expected on no nonce");
- } catch (Exception ex) {
- // expected
- }
+ assertNull(response.readEntity(OAuthAuthorizationData.class));
Review comment:
👍
----------------------------------------------------------------
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] amarkevich commented on pull request #703: Fix systests to account for fix in CXF-8346
Posted by GitBox <gi...@apache.org>.
amarkevich commented on pull request #703:
URL: https://github.com/apache/cxf/pull/703#issuecomment-705008148
@andymc12 Sorry for mess up - found failing tests recently with wrong reason
----------------------------------------------------------------
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] coheigea commented on pull request #703: Fix systests to account for fix in CXF-8346
Posted by GitBox <gi...@apache.org>.
coheigea commented on pull request #703:
URL: https://github.com/apache/cxf/pull/703#issuecomment-703428931
LGTM but can you also check these tests that are also failing in systests/rs-security?
OIDCNegativeTest.testImplicitFlowNoNonce
OIDCNegativeTest.testImplicitFlowPromptNone
----------------------------------------------------------------
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 #703: Fix systests to account for fix in CXF-8346
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #703:
URL: https://github.com/apache/cxf/pull/703#discussion_r499179689
##########
File path: systests/jaxrs/src/test/java/org/apache/cxf/systest/jaxrs/JAXRSClientServerBookTest.java
##########
@@ -840,7 +840,7 @@ public void testCaching() throws Exception {
assertTrue(cacheControl.toString().contains("max-age=100000"));
// Now make a second call. This should be retrieved from the client's cache
- target.request().get();
+ response = target.request().get();
Review comment:
👍
##########
File path: systests/jaxrs/src/test/java/org/apache/cxf/systest/jaxrs/JAXRSClientServerBookTest.java
##########
@@ -883,7 +883,7 @@ public void testCachingExpires() throws Exception {
// Now make a second call. The value in the cache will have expired, so
// it should call the service again
Thread.sleep(1500L);
- target.request().get();
+ response = target.request().get();
Review comment:
👍
----------------------------------------------------------------
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