Fix systests to account for fix in CXF-8346 by andymc12 · Pull Request #703 · apache/cxf

andymc12 · 2020-10-02T18:52:45Z

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).

systests/jaxrs/src/test/java/org/apache/cxf/systest/jaxrs/JAXRSClientServerBookTest.java

reta · 2020-10-03T20:31:41Z

systests/jaxrs/src/test/java/org/apache/cxf/systest/jaxrs/JAXRSClientServerBookTest.java


        // Now make a second call. This should be retrieved from the client's cache
-        target.request().get();
+        response = target.request().get();


reta · 2020-10-03T20:31:47Z

systests/jaxrs/src/test/java/org/apache/cxf/systest/jaxrs/JAXRSClientServerBookTest.java

        // it should call the service again
        Thread.sleep(1500L);
-        target.request().get();
+        response = target.request().get();


reta · 2020-10-03T20:47:46Z

systests/jaxrs/src/test/java/org/apache/cxf/systest/jaxrs/BookServer20.java

                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"));


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.

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.

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.

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!

Thanks @andymc12 , now we have better understanding of the impact.

reta · 2020-10-03T20:49:38Z

@andymc12 sorry for not having the Github + Jenkins in place to help with test cases validation, we are working on that

andymc12 · 2020-10-05T00:46:59Z

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.

coheigea · 2020-10-05T06:34:40Z

LGTM but can you also check these tests that are also failing in systests/rs-security?

OIDCNegativeTest.testImplicitFlowNoNonce
OIDCNegativeTest.testImplicitFlowPromptNone

reta · 2020-10-06T22:51:24Z

rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ContainerRequestContextImpl.java

-        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());


andymc12 · 2020-10-07T00:54:14Z

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.

reta · 2020-10-07T01:16:21Z

...s/rs-security/src/test/java/org/apache/cxf/systest/jaxrs/security/oidc/OIDCNegativeTest.java

-        } catch (Exception ex) {
-            // expected
-        }
+        assertNull(response.readEntity(OAuthAuthorizationData.class));


reta · 2020-10-07T01:18:25Z

...s/rs-security/src/test/java/org/apache/cxf/systest/jaxrs/security/oidc/OIDCNegativeTest.java

-        } catch (Exception ex) {
-            // expected
-        }
+        assertNull(response.readEntity(OAuthAuthorizationData.class));


Signed-off-by: Andy McCright <j.andrew.mccright@gmail.com>

from overwriting query parameter in pre match filters. Signed-off-by: Andy McCright <j.andrew.mccright@gmail.com>

Signed-off-by: Andy McCright <j.andrew.mccright@gmail.com>

andymc12 · 2020-10-07T15:07:01Z

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!

amarkevich · 2020-10-07T15:19:33Z

@andymc12 Sorry for mess up - found failing tests recently with wrong reason

andymc12 · 2020-10-07T16:34:19Z

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!

andymc12 requested review from coheigea and reta October 2, 2020 18:52

andymc12 self-assigned this Oct 2, 2020

andymc12 mentioned this pull request Oct 2, 2020

[CXF-8346] TCK Changes for URI and hasEntity checks #697

Merged

reta reviewed Oct 3, 2020

View reviewed changes

systests/jaxrs/src/test/java/org/apache/cxf/systest/jaxrs/JAXRSClientServerBookTest.java Outdated Show resolved Hide resolved

reta reviewed Oct 3, 2020

View reviewed changes

reta reviewed Oct 6, 2020

View reviewed changes

andymc12 force-pushed the fixSystests branch from 8a2e347 to c32f3c7 Compare October 7, 2020 00:48

reta reviewed Oct 7, 2020

View reviewed changes

reta approved these changes Oct 7, 2020

View reviewed changes

reta reviewed Oct 7, 2020

View reviewed changes

coheigea approved these changes Oct 7, 2020

View reviewed changes

andymc12 added 5 commits October 7, 2020 10:01

Fix systests to account for fix in CXF-8346

91cb6d0

Signed-off-by: Andy McCright <j.andrew.mccright@gmail.com>

Additional assertions per code review comments

13a6bf1

Signed-off-by: Andy McCright <j.andrew.mccright@gmail.com>

Reverting part of apache#697 to avoid preventing user...

78a503d

from overwriting query parameter in pre match filters. Signed-off-by: Andy McCright <j.andrew.mccright@gmail.com>

Update test to expect null rather than exception

a291094

Signed-off-by: Andy McCright <j.andrew.mccright@gmail.com>

Test fix: Need to bufferEntity if reading twice

d1ed01a

Signed-off-by: Andy McCright <j.andrew.mccright@gmail.com>

andymc12 force-pushed the fixSystests branch from d54efc8 to d1ed01a Compare October 7, 2020 15:05

andymc12 merged commit da47285 into apache:master Oct 7, 2020

Conversation

andymc12 commented Oct 2, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reta commented Oct 3, 2020

Uh oh!

andymc12 commented Oct 5, 2020

Uh oh!

coheigea commented Oct 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andymc12 commented Oct 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andymc12 commented Oct 7, 2020

Uh oh!

amarkevich commented Oct 7, 2020

Uh oh!

andymc12 commented Oct 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants