-
Notifications
You must be signed in to change notification settings - Fork 0
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
AJ-1157: Spring Boot 3.2.1 #440
Conversation
prometheus: | ||
metrics: | ||
export: | ||
enabled: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-3.0-Migration-Guide#actuator-metrics-export-properties discusses changes to prometheus config.
implementation 'javax.cache:cache-api' | ||
implementation 'org.ehcache:ehcache:3.10.8' | ||
implementation 'org.ehcache:ehcache:3.10.8:jakarta' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the ehcache version is managed by Spring Dependency Management, so ideally we'd leave the version number off here. However, it requires the jakarta
classifier, and we can't specify a classifier without also specifying a version using this syntax, so I left the version in.
implementation group: 'org.ehcache', name: 'ehcache', classifier: 'jakarta'
would work, but results in IntelliJ complaining about the missing version.
} | ||
|
||
return ResponseEntity.badRequest().contentType(MediaType.APPLICATION_JSON).body(errorBody); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In these methods, I wanted to use the ErrorResponse
class instead of a raw Map<String, Object>
. However, ErrorResponse
does not serialize to json consistent with other classes. We could use its generated .toJson()
method, but that uses gson instead of Jackson. Or, we could serialize it with Jackson …. but then, the timestamp
field is serialized as a string instead of a Date, and we'd have to do our own custom Date formatting inside the exception handler. So, I'm sticking with the raw Map. Hopefully we upgrade to Problem Details soon and this code can mostly go away.
Quality Gate passedThe SonarCloud Quality Gate passed, but some issues were introduced. 19 New issues |
import org.springframework.web.servlet.mvc.method.annotation.ResponseEntityExceptionHandler; | ||
|
||
@ControllerAdvice | ||
public class GlobalExceptionHandler extends ResponseEntityExceptionHandler { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a lot of new/novel code -- was this logic handled somehow for us automatically by something that got removed in the upgrade to SpringBoot 3 or did I miss a move in the red parts of the diff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also confused what this is all about. Did you hand write this code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apologies, I meant to document this in the PR!
Spring Boot 3 / Spring Framework 6 enables, by default, the RFC 7807 "Problem Details for HTTP APIs" standard for structuring its error responses. In WDS today on Spring Boot 2.7, Spring uses the ErrorResponse
structure instead.
If we blindly accepted the change to use Problem Details responses, we'd break anyone using the Java or Python clients to handle error messages, and potentially (probably?) break Terra UI when it tries to parse error messages from WDS.
So, in this PR, I have set mvc.problemdetails.enabled: false
in application.yml to maintain the previous behavior. But that's not sufficient, because in multiple places WDS's exception classes extend ResponseStatusException
(example here). When those exceptions are serialized to the response, they serialize using the Problem Details structure. Therefore, I've also added this global exception handler to intercept any Problem Details being sent to the response and transform them to ErrorResponse instead.
You can see this in practice if you run this branch locally by turning off the global exception handler (comment out the @ControllerAdvice
annotation) and doing something like sending an invalid UUID or an invalid version string in various APIs. You will also see a unit test fail because it asserts on the old error messages.
@calypsomatic yes, this is hand-written code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks great to me! Nice work.
I don't think I understand what precipitated the introduciton of GlobalExceptionHandler
but it looks fine to me either way.
LMK if I can help with Prometheus being busted! |
@jladieu I believe it is working, if you see anything wrong with it let's tackle it! |
@davidangb I tried locally and prometheus LGTM too...I just noticed that you had left it in your TODOs and thought there might still be something up |
@jladieu ah yeah - I have that still listed under TODO, but I ticked the box next to it to show it's complete. This a note-to-self technique I use when iterating on a PR, I realize in hindsight it is not obvious to others! |
Updates WDS to Spring Boot 3.2.1. Supersedes #210.
Highlights of this PR:
Recommended Reading:
Still TODO:
For future PRs: