Response for individual operations

[mzellho]

This PR fixes the return of Response for void operations (Issue #424).

[hbelmiro]

Thank you for this contribution @mzellho.
I left a few comments and I think we also need tests for vendorExtensions.x-java-is-response-void = true.

[hbelmiro]

@mzellho I'm just waiting for a test case related to vendorExtensions.x-java-is-response-void = true. Something like:

    @Test
    void testXJavaIsResponseVoidReturnResponseFalseVoid() throws NoSuchMethodException {
        var method = XJavaIsReturnResponseVoidReturnResponseFalseVoidApi.class.getMethod("hello");
        assertThat(method.getReturnType())
                .isEqualTo(Void.TYPE);
    }

The rest looks good to me.

[mzellho]

Hi @hbelmiro,

I spent a lot of time thinking about how to test the vendor extension - long story short: I can't. As stated in my debugging notes in #386, we always end up in an if statement in the AbstractJavaJAXRSServerCodegen of openapi-generator if we don't specify a response in the spec and that's exactly where the vendor extension is added. So, even with this PR we totally rely on what AbstractJavaJAXRSServerCodegen has added elsewhere and likely are vulnerable to changes there.

Now, having slept over this, I really think that this PR at the very least should not be based on the vendor extension but compare for void (which is set as a return type in the very same if in AbstractJavaJAXRSServerCodegen) in our api.qute.

But, to be completely honest, I even think that we shouldn't add behavior that cannot be configured here. I feel that it would be a lot better and more importantly more user-friendly to revert what we did here and either:

a) provide a new config property (boolean) to "return Response if original Response had been void" (of course, this should be applied for mutiny as well)

or

b) provide a new config property (List) to "name operations that should return Response"

Of course, the generator feature return-response would override these settings if set to true.

WDYT?

[hbelmiro]

Hi @mzellho.

Now, having slept over this, I really think that this PR at the very least should not be based on the vendor extension but compare for void (which is set as a return type in the very same if in AbstractJavaJAXRSServerCodegen) in our api.qute.

It makes sense to me.
I would go with:

b) provide a new config property (List) to "name operations that should return Response"

Or even change the current return-response option to accept both boolean and List. Something like:

...return-response = true # return `Response` for all operations
...return-response = false # return type/void for all operations
...return-response = op1,op2,op3 # return `Response` only for op1, op2, and op3

WDYT?

[mzellho]

Hey @hbelmiro, thanks for the feedback. I would also prefer (b) and am confident that there'll be some time for it before the weekend.

As return-response is a feature of openapi-generator I would prefer not to mix it up with custom behavior, but how about adding a Map<String, String> config property instead that allows to specify full-qualified class names as the desired response type per operation, with any undefined operation falling back to default generator behavior (e.g. op2 of below configuration would use the calculated response type).

quarkus.openapi-generator.codegen.spec.myspec_yaml.responses.op1=jakarta.ws.rs.core.Response # override Response
quarkus.openapi-generator.codegen.spec.myspec_yaml.responses.op3=void

If mutiny is flagged to true, the responses would of course be Uni.
If return-response is flagged to true, any specified response in the new config property will be ignored.

[hbelmiro]

@mzellho return-response is not from openapi-generator. I implemented this feature in #219.
Do you see any other drawback besides that?
A Map<String, String> config property would be a good solution, but I'd still prefer to stick with one single property for simplicity. Users wouldn't have to know/worry about overwriting properties.

[mzellho]

@hbelmiro Just to be safe, isn't return-response forwarded to the jaxrs generator as an additional property?

[hbelmiro]

@hbelmiro Just to be safe, isn't return-response forwarded to the jaxrs generator as an additional property?

@mzellho It is. And then we read its value in the qute template, just how we do with other custom properties.

[lordvlad]

I feel that it would be a lot better and more importantly more user-friendly to revert what we did here

I don't think it is more user-friendly to have an additional option for this. I get the point that this behavior possibly diverges from standard openapi-generator behavior. But then again, since this project is using it's own qute templates, you could argue that it's already diverging from standard openapi-generator behavior.

Correct me if I'm wrong, but I don't think this PR, as it is, would break any expectations or worsen developer experience in any way. If you don't care for the response, there's no difference in having a void method or simply discarding the return value. On the other hand, if you do care for the response (and there is no type info given) its much more user-friendly to already have the Response object instead of having to look for ways to turn the void into a response. I don't think there is any value in keeping the void return type, but I'd love to be corrected.

As for fixing the tests, I feel this would actually suffice:

Now, having slept over this, I really think that this PR at the very least should not be based on the vendor extension but compare for void (which is set as a return type in the very same if in AbstractJavaJAXRSServerCodegen) in our api.qute.

[hbelmiro]

@mzellho since @lordvlad's comment above makes a lot of sense, would you mind changing this PR to always return Response?

I did it myself.

[hbelmiro]

https://github.com/all-contributors please add @mzellho for code
https://github.com/all-contributors please add @mzellho for test