Add throws Exception directive to Spring operation methods

[mcac0006]

PR checklist

• Read the contribution guidelines.
• Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
• Filed the PR against the correct branch: master, 3.4.x, 4.0.x. Default: master.
• Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Problem: The current spring generator generates the API interfaces but does not allow the operation methods to throw unhandled, checked exceptions, thus forcing the implementation to handle the exceptions.

In certain situations, checked exceptions are desired to be left unhandled which are then interpreted as HTTP500 responses. Construct like Spring's @ControllerAdvice would be a perfect fit to handle all unhandled exceptions and throw a standard HTTP500 response. Currently, this is not possible and all operations must write their own try { ... } catch (Exception e) { ... } construct to take care of this, generating a lot of boilerplate code.

Solution: An additional flag, unhandledException, would determine whether all operation methods would declare themselves whether they would allow throwing any kind of exception (throws Exception). Setting the flag false would avoid the throws declaration and leave everything as-is.

Reviewers

@bbdouglas @sreeshas @jfiala @lukoyanov @cbornet @jeff9finger

[cbornet]

I don't think this is a good idea:

• it adds yet another option to the generator
• throwing a generic Exception is a bad practice (except in tests)
• the Spring way is to re-throw an unchecked exception and handle it in ExceptionHandlers

[mcac0006]

the Spring way is to re-throw an unchecked exception and handle it in ExceptionHandlers

Wouldn't this require to add a try ... catch wrapper in every operation method?

[cbornet]

Not if the code call doesn't throw checked exceptions. If it does, it's probably because it wants you to do something with the exception.
Throwing a generic exception is a commonly known smell: https://rules.sonarsource.com/java/RSPEC-112
Throwing a non-generic checked exception results in: https://rules.sonarsource.com/java/RSPEC-1130

[mcac0006]

the Spring way is to re-throw an unchecked exception and handle it in ExceptionHandlers

Is there a reference to this you may recommend?

Let's put some context into the issue: let's say my controller implementation uses common-io's IOUtils.readLines(InputStream) which throws a checked IOException which I would like to leave it unhandled (because I really don't know what to do with it - it's a generic checked exception and because they really don't specify what kind of IO issues may occur).

• One might argue that IOException should really be an unchecked exception, but it isn't and the library is beyond my control,
• if it really throws, it is an ungraceful failure for my logic (and therefore HTTP 5xx),
• Spring allows you to declare your controller methods which checked exceptions and it takes care of them, without issues or warnings.

Therefore: why would I want to wrap it unto an unchecked exception only to handle it into an ExceptionHandler later on? How is that a better solution than just declaring it throws checked exceptions and handle them with a proper ExceptionHandler/ControllerAdvice?

Why would a code generator like OpenAPI dictate things which are beyond its true scope? If the generator supplies code for SPRING, why shouldn't it support everything that Spring supports?

[cbornet]

Well, doing that you're defeating the purpose of checked exceptions which are here to signal that an error that could be recovered from occurred and you're kinda transforming them into unchecked exceptions. In your particular case, you can't recover from your IOException but that doesn't mean you should swallow other checked exceptions that could be thrown from other parts of the code. That said, checked exceptions are less and less popular so maybe some people find it interesting to swallow them by default and since you're doing it as a non-default option, I will not oppose against merging. I'm just worried by the growing number of options we have...

[mcac0006]

Hi, any updates on the above please? I can't seem to have any other options. Thank you.

[wing328]

Thanks for the PR but your commit (as shown in the Commits tab) is not linked to your Github account, which means this PR won't count as your contribution in https://github.com/OpenAPITools/openapi-generator/graphs/contributors.

Let me know if you need help fixing it.

Ref: https://github.com/OpenAPITools/openapi-generator/wiki/FAQ#how-can-i-update-commits-that-are-not-linked-to-my-github-account

[wing328]

@mcac0006 can you please also resolve the merge conflicts?

I agree with @cbornet about too many options but for this one I am ok with it.

Once you've resolved the merge conflicts, please ping me via https://gitter.im (ID: wing32).

[ilyas-keser]

@mcac0006 @wing328 Waiting for this PR. Any idea which version will include it?

[wing328]

The CI failure (outdated doc) will be addressed after merging this PR into master.

[wing328]

@mcac0006 thanks for the PR.

@ilyas-keser PR has been merged into master. Please pull the latest to give it a try.