[TCK Challenge] JAXRSClientIT#replaceQueryTest4() failure should not be expected

[jamezp]

The ee.jakarta.tck.ws.rs.api.rs.core.uribuilder.JAXRSClientIT#replaceQueryTest4 test expects an IllegalArgumentException is thrown because an invalid URI characters are passed as an argument. However, it seems like it shouldn't throw a message, but instead encode the invalid values. From the UriBuilder Java Doc:

Builder methods perform contextual encoding of characters not permitted in the corresponding URI component following the rules of the application/x-www-form-urlencoded media type for query parameters and RFC 3986 for all other components. Note that only characters not permitted in a particular component are subject to encoding so, e.g., a path supplied to one of the path methods may contain matrix parameters or multiple path segments since the separators are legal characters and will not be encoded. Percent encoded values are also recognized where allowed and will not be double encoded.

I interrupt that as the character $ in the test should be encoded to %24 and not throw an IllegalArgumentException. The method UriBuilder.replaceQuery() doesn't really specify whether or not the parameter should be encoded. Given that, it seems it should default to the statement above for "Builder methods".

As a side note it does look like this test has been in the source for quite a while. For some reason it's not been executing in the platform tests for Jakarta EE 9.1 though. @scottmarlow looked at the various places where ignored tests are listed, but couldn't find a reason it was not running.

[scottmarlow]

FYI, Platform TCK test results from EE 9.1 are here, note that replaceQueryTest3 is included but not replaceQueryTest4. I searched for replaceQueryTest in the Platform TCK (9.1.x branch) but didn't see any reason why replaceQueryTest4 would be excluded, I must be missing the way replaceQueryTest4 is excluded from Platform TCK but not really sure. This is just background info for anyone looking into the Platform TCK aspect of this challenge.

[alwin-joseph]

I see that this test never ran or was excluded in platform tck and in old standalone TCK since it did not have @ in the @testName but was not part of the excluded list. This was missed out while the test got migrated.

[jamezp]

Ah-ha! Thanks for that clue @alwin-joseph. Does this mean the test was not intended to be ran in the current 3.1 TCK?

[alwin-joseph]

Apparently the test passes with Jersey 3.1-M3 where IllegalArgumentException is thrown for this test. I believe it is worthwhile a discussion to verify if the test is valid before we disable or modify the test.

[jamezp]

That makes sense to me. Given the java doc I would say that not requiring an IllegalArgumentException being thrown is valid. "Builder methods perform contextual encoding of characters not permitted". However, the java doc for the UriBuilder.replaceQuery() does say "IllegalArgumentException - if query cannot be parsed.". It doesn't explicitly state that the character cannot be encoded so the default java doc would seem to be the reference on whether or not to encode the argument.

[jansupol]

I interrupt that as the character $ in the test should be encoded to %24 and not throw an IllegalArgumentException

The test tries to verify that a query parameter is invalid. The query parameter is quite a long string name$*()^@!+-]}[{|<>,./:;'#1==x?&name2=%20y. So it is not just about the first $. At the moment I do not know whether it is a valid query param or not, I would need to check the RFC.

Jersey throws:

Illegal character "|" at position 35 is not allowed as a start of a name in a path template "name%24%2A%28%29%5E%40%21+-%5D}%5B{|<>,./:;'#1==x?&name2=%20y".

There might be an issue with the Jersey UriBuilder, but it would have been quite interesting to know what the encoded query param should look like by the RFC.

[jamezp]

Right yes, the $ was just an example of one of the invalid characters. It could be valid if the invalid characters are encoded. It's definitely invalid as-is though.

TBH I'd almost hesitate to say what Jersey is doing is wrong. If you were to do something like URI.create("http://localhost:8080?ame$*()^@!+-]}[{|<>,./:;'#1==x?&name2=%20y it would throw an IllegalArgumentException. I assume this is why the replaceQuery() says "IllegalArgumentException - if query cannot be parsed.".

However, it's the java doc on the UriBuilder I'm questioning since the method doesn't explicitly state anything about encoding the parameter passed in. RESTEasy is encoding the parameters invalid characters and has been for as long as I can tell. The issue is just now coming up because the test wasn't ran before in previous TCK's. Given the ambiguity in the java doc, I'm hesitant to change RESTEasy because it breaks backwards compatibility.

[jansupol]

Maybe the IllegalArgumentException is untestable i.e. there is no clear definition of an input upon which the IAE should be thrown? Perhaps the best solution is to remove the test?

[jamezp]

My vote would personally be either remove or ignore it in case the specification wants to clarify later.

[spericas]

+1 on removal

[alwin-joseph]

Created https://github.com/jakartaee/rest/pull/1107 to disable/skip the challenged test.