refactor that decouples opa-evaluator from Jackson - #21
Conversation
johanfylling
left a comment
johanfylling
left a comment
There was a problem hiding this comment.
Not as horrible to review as the line-count would suggest 😅
| } | ||
|
|
||
| @Override | ||
| public boolean isJsonCreator(Constructor<?> ctor) { |
There was a problem hiding this comment.
I wonder if not some of these methods are very jackson-specific. We should add a opa-gson module to see if the interface needs generalization.
There was a problem hiding this comment.
Yes it does seem very Jackson specific... could make it more abstract
There was a problem hiding this comment.
At least we got the dependency tree restructured. We can follow this up in a later PR. Don't need to be precious about breaking changes this early on.
| implementation("com.fasterxml.jackson.datatype:jackson-datatype-jsr310") | ||
| // RegoValueModule (auto-registered via Jackson SPI) provides (de)serializers for the AST | ||
| // types so they don't need to carry annotations. | ||
| runtimeOnly(project(":opa-jackson")) |
| JSON_MAPPER = new ObjectMapper(); | ||
| // findAndRegisterModules picks up RegoValueModule (from opa-jackson) via SPI so | ||
| // RegoString/RegoArray/RegoObject etc. (de)serialize without annotations on the AST types. | ||
| JSON_MAPPER = new ObjectMapper().findAndRegisterModules(); |
There was a problem hiding this comment.
I wonder if it'd be possible to add a simple serialize/deserialize SPI to cover all of the explicit jackson dependencies, or if there are things in here that need that strict coupling?
There was a problem hiding this comment.
There does seem to be some strict coupling, so this won't be straight forward change. Maybe a separate PR?
| private static final ObjectMapper JSON_MAPPER = new ObjectMapper(); | ||
| // Auto-register RegoValueModule (and any other Jackson modules on the classpath) via SPI so | ||
| // RegoObject (de)serialization works without the AST types carrying Jackson annotations. | ||
| private static final ObjectMapper JSON_MAPPER = new ObjectMapper().findAndRegisterModules(); |
There was a problem hiding this comment.
Would be nice if we didn't have this explicit Jackson dependency here, and could just SPI-ify it.
Signed-off-by: Sebastian Spaink <sebastianspaink@gmail.com>
Signed-off-by: Sebastian Spaink <sebastianspaink@gmail.com>
2 participants
resolve: #11