Error handling override in DefaultResponseErrorHandler ignored after upgrade to 6.2.0

[blopatka]

Change introduced in spring 6.2.0 changes the behavior of DefaultResponseErrorHandler.

Until now

spring-framework/spring-web/src/main/java/org/springframework/web/client/DefaultResponseErrorHandler.java

Line 161 in 5024bb7

public void handleError(URI url, HttpMethod method, ClientHttpResponse response) throws IOException {

method didn't exist and the default implementation from interface was calling handleError(ClientHttpResponse response) method, right now the behavior is broken and the said method is not being called.

In classess derived from the DefaultResponseErrorHandler when overriding only handleError(ClientHttpResponse response), the error handling stops working properly, as now the call to the method is completely ommited.

This happened without any notice in release notes, my custom error handler stopped working, errors were only default processed.

[rstoyanchev]

Thanks for the report.

Technically not a regression since the goal in #28958 was switch to the new method, but we didn't document it in the migration notes, and I think we can make it backwards compatible by making handleError(response) no-op, and calling it first. If it doesn't raise an exception, we proceed in the new 6.2 method.

We'll also deprecate the handleError(response) as it is no longer called directly.

[blopatka]

Switching to the new method is fine, but right now the 1-argument 'handleError method is totally omitted in RestTemplate lifecycle. It's fine to remove the method, but as long as it's not removed it should be included in the lifecycle.
Right now I have a dozen of not working ErrorHandlers. For some i can perform migration to the new method right away, others are in compiled libs so migration is not straightforward.
Keep in mind that this change is not causing any compile time errors. Just on the runtime my clients would get responses that in case of errors would be not handled properly.

[rstoyanchev]

We're not removing the method yet, only deprecating it, and during the deprecation phase it will continue to be called. This is why I put the "regression" label.

[rstoyanchev]

The change is now available in 6.2.1-SNAPSHOT if you want to give it a try.

[blopatka]

Without checking the snapshot in apps that i'm supporting (i don't have direct Access to spring snapshot versions in my workplace) i can tell that i see a problem with this solution.
Right now both handleError methods are being called in Default... It will work for implementations of handleError(response) that always end throwing Exception.
But for custom ErrorHandlers that can act as noop, still the second handleError(response, status, URL, method) will be called and the error will be processed with default implementation.

I know that is maybe unusual situation that i have custom noop error handler but IT is like that. So in that case it's not solving all of the issues (but at least custom error handling will work) as i would need to change all of the custom noop error handlers to NoopResponseErrorHandler provided by spring. And it would still be failing silently.
If I have overriden the handleError(response) i don't want the default errorhandling to be executed just after my implementation.

For me the proposed solution doesn't fully solve the bug introduced in 6.2.0. I understand that the handleError(response) will be removed, but for now bumping spring dependency should not break valid flow. In 6.2.0 it wasn't working at all, with this snapshot i can end with processing both handleError methods.
I Hope that there is solution that will work as in spring 6.1.x and still be going toward deprecating old method.

[rstoyanchev]

I'm re-opening to address the remaining concerns.

Fair point that an ResponseErrorHandler could choose to suppress an error status, and not to throw an exception. We would need some actual side effect in handleError(response) to tell if it was overridden.

[mcordeiro73]

@rstoyanchev We saw a similar issue and got around it by also implementing the handleError(url, method, response) method in our implementation.

So while this wouldn't be an issue in our scenario, wasn't sure if there was a concern about custom code that uses a DefaultResponseErrorHandler and calls handleError(response) directly, which would now no-op. Might be very edge case as I doubt many call methods on these implementations from custom code but thought it might be worth at least mentioning.

[blopatka]

@rstoyanchev Can we reopen this ticket and merge the commit to 6.2.2 release?
The change was not released with 6.2.1 milestone (you can check 6.2.1 tag).

[bclozel]

@blopatka thanks for the heads up and sorry we missed that. I've created #34231 to apply this change for the next 6.2.x release.

[blopatka]

@blopatka thanks for the heads up and sorry we missed that. I've created #34231 to apply this change for the next 6.2.x release.

Thanks!
Looking forward for the next release!

[snicoll]

This issue is now superseded by #34231