Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to spring boot 3 #47

Merged
merged 10 commits into from
Sep 8, 2023
Merged

Conversation

ckumpe
Copy link
Contributor

@ckumpe ckumpe commented Sep 6, 2023

This PR updates the rhyme spring integration to Spring Boot 3

@ckumpe ckumpe force-pushed the feature/upgrade_to_spring_boot_3 branch from 2ff6c45 to f274bbf Compare September 6, 2023 13:44
@ckumpe ckumpe force-pushed the feature/upgrade_to_spring_boot_3 branch from f274bbf to 2f76b9b Compare September 6, 2023 13:59
@ckumpe ckumpe marked this pull request as ready for review September 6, 2023 14:19
@ckumpe ckumpe requested a review from feffef September 6, 2023 14:19
@@ -149,10 +149,12 @@ void should_extract_status_code_from_NoHandlerFoundException() {

String nonExistingUrl = baseUri + "/foo/bar";

HalResponse errorResponse = getResponseFromCaughtClientException(
(er) -> client.getRemoteResource(nonExistingUrl, ErrorThrowingResource.class));
ErrorThrowingResource errorResource = client.getRemoteResource(nonExistingUrl, ErrorThrowingResource.class);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why can't this test use getResponseFromCaughtClientException any more? I didn't spot any difference in the assertions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the 404 is returned with another content-type "application/json" and not "application/vnd.error".
I decided that this is acceptable for simple unmapped URLs. Or do you think It's a regression?

Perhaps I'll refactor the code a little bit to spot this difference more clearly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, okay I missed that.

Then this is indeed a minor regression. The goal for the spring integration is that for as many errors as possible a full vnd.error response is being returned.

This is what the VndErrorHandlingControllerAdvice should take care of.

The previous solution was already a bit fragile, as it required some additional configuration in the application properties:

This did already change with some minor spring 2.x. update a while ago.

I guess if you start one of the spring examples now, and call a non-existant URL like /foo/bar, you'll probably see a default 404 JSON response from the spring framework now, instead of the custom vnd.error responses you get for any other exceptions.

This is something we would want to avoid, and make sure these HandlerNotFoundExceptions also are treated as other framework exceptions (i.e. caught and rendered by the VndErrorHandlingControllerAdvice )

If it's too much work to get this working again however, it's not essential

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I even managed to restore the expected error handling with new Spring version.
That obsoletes this question.

Thanks @feffef to spot this issue again.

@sonarcloud
Copy link

sonarcloud bot commented Sep 8, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@ckumpe ckumpe merged commit 1d539a6 into develop Sep 8, 2023
6 checks passed
@ckumpe ckumpe deleted the feature/upgrade_to_spring_boot_3 branch September 8, 2023 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants