[Java][Spring] do webflux controllers the right way

[ilya1st]

Hello, there is a problem with mustache templates for spring-boot-webflux reactive case.
ResponseEntity<Mono|Flux> is wrong way for this case.
Right way is to use @RestController annotation and return directly Flux or Mono objects from controllers.

Here is patchset to make generator do it right.

[jmini]

Java Technical Committee:
@bbdouglas (2017/07) @JFCote (2017/08) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09) @jeff9finger (2018/01)
(we should have a Spring Technical Committee)

cc @macjohnny

[cbornet]

It’s better to return a ResponseEntity to give the possibility for the implementer to set the Http status code and headers

[ilya1st]

@cbornet that is wrong way cause with ResponseEntity you have to set http status code only once, before data is fetched and computed(before you know them). And cannot change them to status you need. But you can set them with WebExchange e.g.

exchange.getResponse().setStatusCode(HttpStatus.NOT_FOUND);

After you fetch your data. And this is more flexible way than using ResponseEntity and exception handlers

[macjohnny]

@ilya1st it might be helpful for reviewing to optimize the PR. could you please

• cherry-pick the changes to the mustache templates
• re-generate only the java-spring samples
• add a variation of the java-spring samples with reactive enabled, i.e. here https://github.com/OpenAPITools/openapi-generator/blob/master/bin/spring-all-pestore.sh
• and then force-push to your branch to update the PR

[cbornet]

@ilya1st I'm not saying that the current implementation is OK but that we should keep the ResponseEntity. So return Mono<ResponseEntity<Foo>> and Mono<ResponseEntity<Flux<Foo>>>
The ServerWebExchange could also be passed (you would have to put it in the method arguments) but it's not as handy as ResponseEntity.

[cbornet]

Actually the ServerWebExchange is already passed (we use it for the examples). Which is a good thing since it's the layer that gives the most flexibility (you can force the payload with untyped/raw data, etc...)

[ilya1st]

@macjohnny I've reset branch to commit with mustache fixes.
There are some diffuculties with generating sources cause I have to work on Windows 7 machine and git marks some files 755 there :)

[wing328]

I've updated the Petstore samples via e5f222d. Let's see how it goes.

[cbornet]

@wing328 Note that there are still changes required on this PR

[wing328]

@cbornet thanks for the heads-up. I'm just trying to resolve the Shippable CI error. Let me update the samples one more time to see how it goes.

[ilya1st]

@cbornet We had to write our generator based on this generator class for our internal projects for webflux. There are some issues with wrong work of delegate methods and classes for example. There are to much issues to make PRs for them.

For webflux case code looks slack-baked. May be better idea for springboot webflux case write separate generator from scratch? For now templates are too overloaded with cases to prevent erros there.
I can do this job.

[IsaacDeLaRosa]

@cbornet Is support for Mono<ResponseEntity<Foo>> (or Flux) controllers coming soon? if so will these be the required params and the right way to use it?

import org.openapitools.codegen.config.CodegenConfigurator
import org.openapitools.codegen.DefaultGenerator
def swaggerSourceFile = 'ignoreThis'
def swaggerTargetFolder = 'src/generated/java'

task generateApi {
    inputs.file("$projectDir/$swaggerSourceFile")
    outputs.dir("$projectDir/$swaggerTargetFolder")
    doLast {
        def config = new CodegenConfigurator()
        config.setInputSpec("file:///$projectDir/$swaggerSourceFile")
        config.setOutputDir("$projectDir")
        config.setGeneratorName('spring')
        config.setAdditionalProperties([
                'interfaceOnly' : 'true',
                'reactive' : 'true',
                'apiPackage'    : 'ignoreThis',
                'modelPackage'  : 'ignoreThis',
                'sourceFolder'  : swaggerTargetFolder
        ])
        new DefaultGenerator().opts(config.toClientOptInput()).generate()
    }
}

[cbornet]

@IsaacDeLaRosa see #913 . As for the gradle part, have you tried the gradle plugin ?

[cbornet]

Workaround with the current templates : if you need to set the status code or the headers reactively, you can do so by setting them on the ServerWebExchange in the reactive body. Eg:

    @Override
    public ResponseEntity<Mono<Void>> deletePet(Long petId, String apiKey, ServerWebExchange exchange) {
        Mono<Void> result = Mono.empty()
                .doOnSuccess(it -> exchange.getResponse().setStatusCode(HttpStatus.FORBIDDEN))
                .doOnSuccess(it -> exchange.getResponse().getHeaders().add("foo", "bar"))
                .then();
        return ResponseEntity.status(HttpStatus.OK).body(result);
    }

[jrobison153]

Chiming in here in agreement with the OP. We've been looking at the generated Spring Web Reactive interfaces and the response type wrappings seem superfluous, this is using version openapi-generator:3.3.1. For an API that returns a collection of type Foo, we get a Java interface with the following response signature

Mono<ResponseEntity<Flux<Foo>>>

From all of our testing this behaves identically to Flux<Foo> from a reactive client's perspective. The additional Mono<ResponseEntity> adds additional code to each controller to add the type wrappings.

Is there a reason for this or can it be simplified to Flux<Foo> and Mono<Foo>. The ServerWebExchange is already present and gives all the control over the HTTP session needed. It seems the generated code could be simplified

[cbornet]

The same about ResponseEntity could be said for the non-reactive generation. I agree that it could be an option not to wrap into ResponseEntity.

[jrobison153]

Thanks for the quick response @cbornet, is this something you would be willing to take a PR for? If so would you want it conditional on option or just outright make the generator always create Flux and Mono response types?

[cbornet]

It shall be conditional with an option at least for backward compat. We can discuss on what should be the default. I don't know when I can find enough time for this...

[cbornet]

I'm closing this PR as the webflux gen has been fixed in another one.
@jrobison153 please open another issue for the optional ResponseEntity.

[jrobison153]

will do, cheers