Not properly handling Optional param in JAX-RS method

[EricWittmann]

First reported here: quarkusio/quarkus#5376

When processing the JAX-RS arguments, we should handle java.util.Optional appropriately.

[EricWittmann]

I'm labeling this as both a bug (the report in quarkus is that the build fails) and also an enhancement, because we should mark the query parameter as not required in this case. And perhaps mark any method arguments that are query parameters as required if not using java.util.Optional.

Thoughts on the latter point, @MikeEdgar and @msavy ?

[machi1990]

@EricWittmann thanks for opening this. Also, the build fails when Optional is a return type.

[MikeEdgar]

And perhaps mark any method arguments that are query parameters as required if not using java.util.Optional

I would stay away from marking it required when Optional is not used. There are other scenarios where non-mandatory parameters are expressed in JAX-RS without Optional (e.g. @DefaultValue). It might be safer to use @Optional as a hint to set "required": false if no other implicit/explicit value has been set using OAI annotations or bean validation annotations like @NotBlank.

Aside from that, I think the reason the build is failing in the original issue is due to something other than the use of Optional. The stack trace indicates that it's getting an out of range exception in the component that is looking for getters/setters in a schema model. The bug there is that TypeResolver isn't doing any bounds check before calling charAt and substring. I'm guessing there is a class somewhere in the hierarchy with a method name like "get" or "set". The handling is still a bug though.

[machi1990]

Aside from that, I think the reason the build is failing in the original issue is due to something other than the use of Optional. The stack trace indicates that it's getting an out of range exception in the component that is looking for getters/setters in a schema model. The bug there is that TypeResolver isn't doing any bounds check before calling charAt and substring. I'm guessing there is a class somewhere in the hierarchy with a method name like "get" or "set". The handling is still a bug though.

Exactly, and the class here is Optional, the method causing this is Optional#get().

[MikeEdgar]

Ahh, right. I forgot about these classes being in the index. So it is the same issue.

[MikeEdgar]

@EricWittmann, @msavy - a few additional cases this should probably cover. What are your thoughts?

1. When optional type is a @FormParam or @MatrixParam, the generated property should be "nullable": true and should not be in the form/matrix parameter schema's array of required properties unless otherwise specified.
2. When optional type is a @RequestBody, the generated requestBody should be "required": false unless otherwise specified.
3. When optional type is a JAX-RS resource method return type, the generated schema(s) should be "nullable": true unless otherwise specified.
4. When optional type is a schema property, the property should be "nullable": true unless otherwise specified.
5. An optional @BeanParam will not affect any of the parameters contained within it, but it should properly unwrap the bean's type from the Optional.

[EricWittmann]

This all sounds good to me. I was going to add that for (4) the property should be excluded from the required list, but it occurred to me that I'm not sure if we ever include anything in the required list when generating a schema from beans (unless annotated explicitly).

[MikeEdgar]

The only time the required list is modified during the scan is when a @NotNull annotation is encountered. In the case where a @NotNull on an Optional is scanned, I think @NotNull is more indicative of the intent, even if using those two together won't really function as expected at run time.

[gsmet]

FTR, we just had a second report of this very issue in the Quarkus bugtracker.

[EricWittmann]

@MikeEdgar has a PR addressing the issue. I'll review it today and hopefully we can get it merged asap.

[gsmet]

@EricWittmann do you think it's worth releasing something now to have it fixed in Quarkus 1.0 Final?

Code freeze is this evening so we don't have much time.

Not sure how risky the fix is though compared to fixing an issue for 2 users who reported it. And how much work it is.

[EricWittmann]

I think the change is pretty low risk, and the test coverage is quite good in this project. I'm pretty comfortable if a new release were done. What do you think @MikeEdgar ?

[EricWittmann]

Also it's a good question for @kenfinnigan - since I think he would need to do the release.

[kenfinnigan]

I have no opinion, I just click a button

[MikeEdgar]

I think it's a good time for a release. There are quite a few bug fixes and improvements since 1.1.19 and the schema reference option is now enabled by default.

[EricWittmann]

@MikeEdgar Anything loose you want to toss in there before Ken clicks the button? :)

[MikeEdgar]

I do have #215 and #216 basically done, but I won't be able to get them up to Github and open the PRs before the Quarkus code freeze, unfortunately.

[EricWittmann]

OK then, @gsmet if you are comfortable accepting a new release of this into Quarkus then I think @kenfinnigan can go ahead with the release! Or I guess go ahead with it anyway, and @gsmet can pull it into Quarkus or not, at his discretion. :)

[kenfinnigan]

release triggered

[gsmet]

Cool, thanks everyone!

[gsmet]

@kenfinnigan can you create a PR when the release is done?

[machi1990]

Thanks for the fix @EricWittmann @MikeEdgar :-)