TemporalParamConverter returns null if it has to convert empty string

[blazmrak]

Closes #40833

[blazmrak]

I'm having problem getting the extensions/resteasy-reactive/rest/deployment/ test suite to run locally to add the tests for temporal types. It fails with No config found for interface io.quarkus.resteasy.reactive.common.runtime.ResteasyReactiveConfig for each test. I run the suite with ./mvnw test -f extensions/resteasy-reactive/rest/deployment/. Am I missing something obvious?

[gsmet]

Have you installed the artifacts locally with ./mvnw -Dquickly?

[blazmrak]

Have you installed the artifacts locally with ./mvnw -Dquickly?

Ah... It was in my face... It failed the first time for Gradle Model. I was going to deal with it after I got everything working for the PR :D I ran it again now and it seems like I need JDK 17 installed for it to run. I'll fix this tomorrow and try again. Thanks!

[blazmrak]

@FroMage @geoand I was updating the tests, when I realized that just adding String.isEmpty() check to the converter and returning null would just make it a footgun. I have added a try/catch around the delegate.convert() in OptionalConverter and return empty if it conversion throws when converting an empty String.

This breaks custom converters that throw on empty string and where users also use Optional as a parameter and expect to throw and not handle Optional on empty string. My guess would be that this is very uncommon.

The upside is, that this applies to all types, not just temporal - e.g. @RestForm Optional<Integer> should now work as well.

[FroMage]

This feels wrong to me. It special-cases Optional, and does not solve the issue for any type that is not optional. Indeed the tests you added expect a 400 for a null Date.

In my opinion there are two alternatives possible:

• Either we decide that foo= is equivalent to foo not being set, and any value extracted for it should be null, and we cannot make the difference between passing an empty string and not passing any value, in which case it's ResteasyReactiveRequestContext.get*Param() should not return empty strings, but null values, and then every further ParamConverter will work properly, or
• We declare that foo= should set it to an empty string (as is the case now, as opposed to a null value) and so every ParamConverter that exists has to handle empty strings specially, such as temporal values.

I'm not entirely sure what to do, TBH. Perhaps @geoand has an opinion?

[blazmrak]

This feels wrong to me. It special-cases Optional, and does not solve the issue for any type that is not optional. Indeed the tests you added expect a 400 for a null Date.

I was about to disagree, because I thought I was being smart with null safety, but I completely forgot, that if no parameter is set, it will be null anyways.

The problem of making foo= null is that you might have a required field that can be an empty string, that would break. This feels like bad implementation, but someone might rely on this?

The problem with foo= being an empty string is that we likely don't control every param converter. I don't think that e.g. Integer has a param converter as it relies on Jakarta standard to use valueOf I'm guessing although I'm not familiar with the codebase and could be completely wrong 😅

[FroMage]

Yeah, for strings, we might be able to use an empty string to disinguish:

• <nothing> →@RestQuery String foo will be null because it not set in the request
• foo= → @RestQuery String foo will be "" (empty string) because it is set to empty

So, really, IMO the interesting difference is between:

• Was set to nothing (either null or "" (empty string)
• Was not set

Note that this problem only happens for strings, because we have confusion between null (not set) and "" (empty string) (set to empty value).

This does not apply to integers, booleans or any other type where the empty value is distinct from the "not set" value.

For example, given an unboxed integer:

• <nothing> →@RestQuery int foo will be 0 because it not set in the request
• foo= → @RestQuery int foo will be 0 because it is set to empty

And given a boxed integer:

• <nothing> →@RestQuery Integer foo will be null because it not set in the request
• foo= → @RestQuery Integer foo will be null because it is set to empty

So I think that String is a very special case, and every other type will not be able to make the difference between "not set" and "set to empty value", because in both cases they should get a null or zero value (for primitives).

Therefore, I suggest we turn both <nothing> and foo= into null for String, LocalTime and in fact every non-primitive, and the zero-value for primitives.

This is what makes the most sense. And if people want to differenciate between <nothing> and foo= we will need a different API.

[blazmrak]

So I think that String is a very special case

Yep, completely agree. There are custom converters as well, but I would be surprised if anyone is relying on this behavior.

Therefore, I suggest we turn both and foo= into null for String, LocalTime and in fact every non-primitive, and the zero-value for primitives.

This makes the most sense to me as well. I'll make the changes shortly.

[blazmrak]

I have done the changes. I have two more questions:

1. Do we leave the array parameters (single = false) as is, or do we make them null as well? Leaving them as is would make List<String> params be null instead of empty, however it would make e.g. List<Integer> not throw. I'm leaning towards not returning null for !single.
2. I don't like where the tests for this change are, but I have no idea where else to put them. Do I make another test class or did I miss some place where RequestContext is tested or are you fine with where they are? I don't know the codebase, but to me it looks like the tests are meant to be testing the converter 😄

[FroMage]

1. Do we leave the array parameters (single = false) as is, or do we make them null as well? Leaving them as is would make List<String> params be null instead of empty, however it would make e.g. List<Integer> not throw. I'm leaning towards not returning null for !single.

I think we should make the array parameters empty collections, if it's not already the case. I'm not sure how it makes sense to turn foo=&foo= into collections of two empty elements, especially given it can't work for primitives, and the previous reasoning that we applied to String.

2. I don't like where the tests for this change are, but I have no idea where else to put them. Do I make another test class or did I miss some place where RequestContext is tested or are you fine with where they are? I don't know the codebase, but to me it looks like the tests are meant to be testing the converter 😄

That's fine IMO.

[FroMage]

The TCK complains about cookie params only, perhaps there's a bug in your PR, only for cookies?

As for RuntimeParamConverterTest.sendEmptyParameter I think it's a pretty convoluted test, for example it creates a param converter for Optional<Integer> which returns null in some cases. That's not realistic IMO, so this test should be adapted for the new behaviour.

[blazmrak]

I think we should make the array parameters empty collections, if it's not already the case. I'm not sure how it makes sense to turn foo=&foo= into collections of two empty elements, especially given it can't work for primitives, and the previous reasoning that we applied to String.

What do we do with foo=&foo=1? If I understood you correctly, you would expect it to be become [1] and not [null, 1].

[FroMage]

What do we do with foo=&foo=1? If I understood you correctly, you would expect it to be become [1] and not [null, 1]

Yeah, this is the only thing that makes sense for arrays of primitives anyways.

[blazmrak]

The TCK complains about cookie params only, perhaps there's a bug in your PR, only for cookies?

It seems to be an issue with query parameters for the test setup... When client makes the request on the server, query parameter todo is always null for some reason (it should have been setcookies). How can it pass the query parameter tests (I tried even with String param locally, same as it is in ecks), but fails on ecks?

[blazmrak]

It took me more than I'm willing to admit, but I have confirmed this... As soon as I return null for val.isEmpty() in getQueryParameter, the tests for cookies fail. TCK tests send empty todo= somewhere and expect to get back other stuff 😄 https://github.com/quarkusio/resteasy-reactive-testsuite/blob/c8b7402a9fdd6aa4e267a6e9e12d936b246e2a75/tests/src/test/java/com/sun/ts/tests/jaxrs/ee/rs/cookieparam/JAXRSClient0143.java#L368

Do I make a PR to TCK as well? This needs to be changed to include the same response as the else block, todo=null is never expected anywhere https://github.com/quarkusio/resteasy-reactive-testsuite/blob/c8b7402a9fdd6aa4e267a6e9e12d936b246e2a75/tests/src/test/java/com/sun/ts/tests/jaxrs/ee/rs/cookieparam/CookieParamTest.java#L89

[FroMage]

Do I make a PR to TCK as well?

yeah, it's not right that a query param change would break cookie tests. Thta's just bad testing.

[blazmrak]

yeah, it's not right that a query param change would break cookie tests. Thta's just bad testing.

Well, it was just a defensive branch that was never hit. I opened the pull request there: quarkusio/resteasy-reactive-testsuite#8

Once that is merged, the test should go through.

[cescoffier]

@blazmrak Can you rebase, the other PR has been merged.

[blazmrak]

@cescoffier I have rebased the changes. Sorry, I forgot about this PR. It was a while ago, but I think I still have to implement the handling of the arrays. I'll take a look over this PR again today and make the changes if necessary.

[blazmrak]

@cescoffier I have implemented the feature and it passes the tests for vertx. However, I'm losing the battle with TCK tests... I have no idea in what world is this wrong:

11-12-2024 20:55:04:  Harness - [HttpRequest] Dispatching request: 'GET /jaxrs_ee_rs_cookieparam_web/CookieParamTest?todo= HTTP/1.1' to target server at 'localhost:8081'
2024-11-12 20:55:04,399 SEVERE [com.sun.ts.lib.uti.TestUtil] (main) [WebValidatorBase] Unable to find the following search string in the server's response: 'ParamEntityWithFromString=CookieParamTest' at index: 0
[WebValidatorBase] Server's response:
-------------------------------------------
other stuff
-------------------------------------------

GET /jaxrs_ee_rs_cookieparam_web/CookieParamTest?todo= sends empty todo parameter. Old implementation would return other stuff, because the parameter would be an empty string. The patch I submitted to the test suite does the same, because todo param is now null instead of an empty string. The tests have not been touched, wtf is going on???

Edit: fuck me... https://github.com/blazmrak/resteasy-reactive-testsuite/blob/7ae50ccdff78f07049e62d5f8da9af981cd2012b/tests/src/test/java/com/sun/ts/tests/jaxrs/ee/rs/cookieparam/CookieParamTest.java#L100

I don't know how I could overlook that, nor how did I get the suite to pass with that change... I have probably undone the changes for query params when I was figuring the test suite out smh. Sorry for this fuckup... I have made the suite actually pass this time by moving the content of else if todo == "" to if todo == null block. Do I submit another PR to TCK suite?

[cescoffier]

@FroMage any idea about the TCK?

[FroMage]

Do I submit another PR to TCK suite?

Sure.

[blazmrak]

@FroMage I have submitted a PR

[FroMage]

I've restarted the tests

[blazmrak]

@FroMage Same failure... The commit hash has to be updated to 2b263617d93e3f161bf092bb6425405e9d3c01d0 probably, unless the pipeline does it automatically somehow?

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit f119848.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

[FroMage]

@geoand you OK with this one? Shall I merge it?

[geoand]

I think there was an update to the TCK in the meantime, that would be nice to include.

Otherwise, it's good

[blazmrak]

I think there was an update to the TCK in the meantime, that would be nice to include.

@geoand This PR should be on the latest commit quarkusio/resteasy-reactive-testsuite@2b26361

<resteasy-reactive-testsuite.repo.ref>2b263617d93e3f161bf092bb6425405e9d3c01d0</resteasy-reactive-testsuite.repo.ref>

[geoand]

The latest commit on the testsuite is 0231b4a4c070d4899216bd6aa77c9a30f2129dd3 from a few days ago

[blazmrak]

@geoand that is my commit, 2b26361 is your merge commit to main. Without that patch, TCK was failing the CI. This PR only passed CI after I updated the TCK commit hash here: f119848

[geoand]

Ah, good point!