Improve handling of non-standard status codes in WebMvcTags

[dreis2211]

Hi,

funny enough #17991 reminded me of a similar branch I was working on, but forgot. The changes in this PR align the handling of non-standard status codes in WebMvcTags with the latest changes to WebClientExchangeTags and now RestTemplateExchangeTags. More specifically the more lenient handling of responses with status codes like 490 inside their respective outcome() methods.

I wanted to change WebFluxTags accordingly, but since ServerHttpResponse doesn't expose an API to access the raw status code I didn't see a good way of doing so. WebFluxTags additionally returns HttpStatus.OK in case of unknown codes inside WebFluxTags#status(), which seems questionable. Usually, an unknown status code indicates situations where OK isn't appropriate.

With this PR we end up with the following handling at least.

Class/Method	Status 490	Status 701
RestTemplateExchangeTags#status()	490	701
WebClientExchangeTags#status()	490	701
WebMvcTags#status()	490	701
WebFluxTags#status()	200	200
RestTemplateExchangeTags#outcome()	CLIENT_ERROR	UNKNOWN
WebClientExchangeTags#outcome()	CLIENT_ERROR	UNKNOWN
WebMvcTags#outcome()	CLIENT_ERROR (previously UNKNOWN)	UNKNOWN
WebFluxTags#outcome()	UNKNOWN	UNKNOWN

Let me know what you think.
Cheers,
Christoph

[dreis2211]

Build failed, but I think it's unrelated and seems to be a problem with Concourse at the moment. (Given that it says there is no space left on the device).

[wilkinsona]

Thank you, @dreis2211.

I've opened spring-projects/spring-framework#23547 to see if access to the raw status code can be provided on ServerHttpResponse.

[wilkinsona]

@dreis2211 I'm wondering if you considered and rejected the map-based approach for mapping a series to an outcome? If you did, can you share your reasoning please? It'd be nice to keep things consistent if we can, either by updating this proposal or by changing the existing code to use a switch.

[dreis2211]

@wilkinsona I had a reason indeed: performance. Without benchmarking it (shame on me), I think the vanilla switch-case approach should be slightly faster than the map-based approach. The switch-case is probably translated to a tableswitch expression under the hood, which should beat the map access (although both should have O(1) complexity). Moreover, I like the "locality" of my approach. I see directly that input X produces output Y without scrolling/jumping to the top (but I'd describe that as personal taste and it might be just me with that taste 😄 ). I honestly don't have strong feelings for my approach and I'm happy to change it to the map based one. (If we stick to the Map approach, I'd suggest to switch from a HashMap to EnumMap at least - should save a couple of cycles).

Having said the above: I'm now super interested to write a small JMH benchmark that compares HashMap vs. EnumMap vs. switch-case. After all, EnumMap.get() is just a simple array access as well and should be fast. Give me a minute

[dreis2211]

Here we go. Testing the approaches in isolation delivers the following results for status code 200 (given that this should be the most common one):

Benchmark                  Mode  Cnt         Score         Error   Units
MyBenchmark.testEnumMap   thrpt   10  74383795,366 ± 11900523,397   ops/s
MyBenchmark.testHashMap   thrpt   10  67004451,224 ±  4387559,934   ops/s
MyBenchmark.testSwitch    thrpt   10  87443228,709 ±  1913140,913   ops/s

For codes outside of the range (let's say 700) we get the following:

Benchmark                  Mode  Cnt         Score         Error   Units
MyBenchmark.testEnumMap   thrpt   10  78674193,884 ± 3031404,207   ops/s
MyBenchmark.testHashMap   thrpt   10  78308259,066 ± 2982167,493   ops/s
MyBenchmark.testSwitch    thrpt   10  79193285,975 ± 1730592,024   ops/s

Again, I don't have strong feelings for my solution even if my educated guess was correct from a performance perspective. I'm more than happy to change to any approach. As you said: it should be probably consistent and I should have explained my reasoning earlier. Sorry for that.

Cheers,
Christoph

[philwebb]

It looks like the other series based changes are only on 2.2.x. Although #17950 was fixed in 2.1.x, the series work was handled in the merge commit (b54ff7c).

[wilkinsona]

Thanks once again, @dreis2211.

Inspired by this and the desire to make all the implementations consistent, I also pushed e8de5a6. It moved to an if-else based approach rather than switching on Series to avoid a dependency on spring-web (in case we want to use it with Jersey or the like in the future).

[dreis2211]

Already saw it. Looks good! Thanks for approving this :)