-
Notifications
You must be signed in to change notification settings - Fork 32
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
SO-5868: Retrieve validation results via REST #1310
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 9.x #1310 +/- ##
============================================
+ Coverage 48.46% 48.57% +0.10%
- Complexity 14037 14071 +34
============================================
Files 1949 1949
Lines 95376 95379 +3
Branches 11024 11024
============================================
+ Hits 46228 46329 +101
+ Misses 46108 46018 -90
+ Partials 3040 3032 -8 ☔ View full report in Codecov by Sentry. |
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.
🥇
@@ -181,6 +181,16 @@ public SnowOwlOpenApiWebMvcResource openApiWebMvcResource( | |||
@Bean | |||
@Scope(scopeName = WebApplicationContext.SCOPE_REQUEST, proxyMode = ScopedProxyMode.TARGET_CLASS) | |||
public ObjectMapper objectMapper(@Autowired HttpServletRequest request) { | |||
return objectMapperFromRequest(request); |
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 think for consistency, this should be a provider as well.
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.
As I see it, we are going to need the two different methods:
This method with ObjectMapper return type is necessary for mappingJackson2HttpMessageConverter()
And the other one, with Provider is necessary for the RepositoryValidationRestService class
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.
IIRC we tried three approaches:
- Keep the initialization code in this method and refer to it in the provider lambda
- → the
Provider
producedObjectMapper
delegates (proxies) that expected a request scope to be present, triggering the exception in the validation REST controller (where it was originally reported)
- → the
- Move the initialization code to the lambda and create an instance here using the
Provider
- → the returned instance is an actual
ObjectMapper
but creating it requires a request scope which is not available at startup, triggering the exception right away
- → the returned instance is an actual
- Move the initialization code to a non-@
Bean
-annotated method and refer to it from both annotated methods- → the current state of the PR that worked without issues
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.
🥓
No description provided.