[BUG] [Java] retry logic no longer throws HttpServerErrorException

[mvannes]

Bug Report Checklist

Description

When upgrading from 7.1.0 to 7.2.0 we are encountering that the change made in #17375 has ensured that the HttpServerException is no longer thrown in generated api clients.

This was used in our code to wrap the error with a better message for the client, while also logging it.
This change has caused that exception to not be thrown, but a RestClientException is thrown instead, which is now unhandled.

was this an intentional change that should be reflected in our codebase, or unintended behaviour?

openapi-generator version

7.2.0, regression from 7.1.0

Generation Details

generator version 7.2.0, installed using the gradle plguin

org.openapitools:openapi-generator-gradle-plugin:7.2.0

generation is done with library = resttemplate.

Steps to reproduce

Using generator version 7.1.0, generate a client using the resttemplate library.
Make an unsuccessful call (500 response) using htis client, while having it wrapped in a try-catch clause for org.springframework.web.client.HttpServerErrorException

This should work, and the catch part should be hit.

Now, upgrade to 7.2.0 and it should not work.

Related issues/PRs

#17375

Suggest a fix

Continue throwing the HttpServerException, or confirm that our use case was incorrect all along.

[ilam-natarajan]

I think this is bug.

https://github.com/OpenAPITools/openapi-generator/pull/17381/files#diff-1bf26e25dd0a8458d52cb2165c03871b9fcbb28d111bdcb6132af867d69502e7

here instead of throwing RestClientException, the original exception should be thrown.

[wing328]

PR merged. Please give it a try with the latest master or snapshot version when you've time.

[GregDThomas]

I think your MR to fix this introduced another bug.

The code is now (~line 776)

 if (ex instanceof HttpServerErrorException
                        || ((HttpClientErrorException) ex)
                        .getStatusCode()
                        .equals(HttpStatus.TOO_MANY_REQUESTS)) {
... then retry the request

i.e. if the response is an HttpClientErrorException and it's HttpStatus.TOO_MANY_REQUESTS you retry.

I think you are missing a "not" in there - i.e. you don't want to retry TOO_MANY_REQUESTS.

(Given this is a SpringBoot template, I probably would have used a @Retryable but I guess that's just personal preference)

[wing328]

@GregDThomas do you mind filing a PR with the suggested fix (missing a not)?

[ilam-natarajan]

@GregDThomas It was my intention to retry when the status is TOO_MANY_REQUESTS .
I can't think of any other status code in 4xx series when one would like to retry. So I think the question is one want's to retry on 429 or not. Ideally it will be good to retry with Exponential Backoff , but I think this retry implementation was a good start, we just need to keep building on top of it.
cc @wing328

[GregDThomas]

So you're currently retry every server error, and the TOO_MANY_REQUESTS client error? If that's your intent, that's what the code does AFAICT. It just that the behaviour surprised me. Hence my comment. So there's no need for an imminent PR, though I may make some improvements if I ever get around to it in the not near-future. (e.g. using the Retry-After header if present, avoiding a Thread.sleep, and making the codes to retry configurable).