Provide external jackson object mapper #395
Conversation
|
@SlyngDK Please review. |
| } | ||
|
|
||
| protected Config.FieldConfig fieldConfig(Optional<String> fieldName, Optional<Boolean> enabled) { | ||
| protected Config.FieldConfig fieldConfig(String fieldName, Boolean enabled) { |
There was a problem hiding this comment.
Using proper types instead of Optional.
| return mapper.readValue(getResult(jsonProvider, event), JsonNode.class); | ||
| } | ||
|
|
||
| protected Config.FieldConfig fieldConfig(Optional<String> fieldName, Optional<Boolean> enabled) { |
There was a problem hiding this comment.
Optionals should only be used as a return type.
| @Override | ||
| public Optional<String> fieldName() { | ||
| return fieldName; | ||
| return Optional.ofNullable(fieldName); |
There was a problem hiding this comment.
Will return empty optional if input is null.
| @Override | ||
| public Optional<Boolean> enabled() { | ||
| return enabled; | ||
| return Optional.ofNullable(enabled); |
There was a problem hiding this comment.
Will return empty optional if input is null.
# dependency-version: 0.8.14 # dependency-type: direct:development # update-type: version-update:semver-patch # dependency-version: 0.8.14 # dependency-type: direct:production # update-type: version-update:semver-patch # dependency-version: 3.29.0 # dependency-type: direct:production # update-type: version-update:semver-minor # dependency-version: 3.29.0 # dependency-type: direct:production # update-type: version-update:semver-minor # dependency-version: 3.29.0 # dependency-type: direct:production # update-type: version-update:semver-minor # dependency-version: 3.29.0 # dependency-type: direct:production # update-type: version-update:semver-minor # dependency-version: 3.29.0 # dependency-type: direct:production # update-type: version-update:semver-minor # dependency-version: 3.29.0 # dependency-type: direct:development # update-type: version-update:semver-minor #dependency-version: 3.29.0 # dependency-type: direct:development
IvanPuntev
force-pushed
the
provide-external-jackson-object-mapper
branch
from
099fc28 to
1f915cc
Compare
|
@SlyngDK Should we use a single json factory instance instead of creating new one for each build step? |
gsmet
left a comment
gsmet
left a comment
There was a problem hiding this comment.
Had a quick look and spotted a few things.
| <dependency> | ||
| <groupId>org.eclipse</groupId> | ||
| <artifactId>yasson</artifactId> | ||
| </dependency> |
There was a problem hiding this comment.
Why are you enforcing Yasson now? It doesn't look like an improvement?
There was a problem hiding this comment.
The jsonb factory initialization is now moved from the processor to the recorder and it is instantiated only on demand. The dependency is added because the native build starts to break without it.
Error: Discovered unresolved type during parsing: org.eclipse.yasson.internal.JsonBindingBuilder. This error is reported at image build time because class io.quarkiverse.loggingjson.jsonb.JsonbJsonFactory is registered for linking at image build time by command line and command line.
This solution appears to be working but if there is a better / correct way to solve this I will change it.
runtime/src/main/java/io/quarkiverse/loggingjson/jackson/JacksonJsonFactory.java
Outdated
Show resolved
Hide resolved
| @Singleton | ||
| public class ObjectMapperProvider { | ||
|
|
||
| @Inject | ||
| ObjectMapper mapper; | ||
|
|
||
| public ObjectMapper get() { | ||
| return this.mapper; | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
I doubt this will work well in practice as you want logging very early, even before ArC is set up.
If you require ArC to be set up to set up logging, it might work but you will collect lots of messages and buffer them in memory before the setup is fully done.
I think you probably need to use the ServiceLoader for this and my guess is that you won't be able to reuse the common ObjectMapper.
There was a problem hiding this comment.
The idea is to be able to get the object mapper provided from the project including the extension. Don't think the ServiceLoader will do...
| final Config.FieldConfig config = fieldConfig(Optional.empty(), Optional.empty()); | ||
| final Config.FieldConfig config = fieldConfig(null, null); |
There was a problem hiding this comment.
I think it's not a very good idea to mix changes that are not really related to the issue at hand. It makes the PR very verbose.
There was a problem hiding this comment.
I'm aware of that but I considered these changes harmless and since there is no frequent activity in this repo decided to include them.
3 participants
Fix #242
Fix #305
Obsolete #306