Add support for custom object mapper in GraphQLResponse #1737
Conversation
The default object mapper is not enough to satisfy all deserialization scenarios. One such scenario is where we need object mapper with custom deserializer for graphql.relay.PageInfo.
|
Hi Team! I want to help add this generic functionallity inside the GraphQLResponse. This is rough draft of just adding the dependency to GraphQLResponse. I will need to investigate how I can inject the objectmapper directly from the spring context. I would like to discuss what you think of such support and can provide use-cases for it! Any feedback is welcome :) |
|
Thanks for the PR. The changes look good, there is a linter error that is failing the build btw. Once that is fixed, we should be good to merge, |
| @@ -53,6 +60,14 @@ data class GraphQLResponse(@Language("json") val json: String, val headers: Map< | |||
| val errors: List<GraphQLError> = parsed.read("errors", jsonTypeRef<List<GraphQLError>>()) ?: emptyList() | |||
|
|
|||
| constructor(@Language("json") json: String) : this(json, emptyMap()) | |||
| constructor(@Language("json") json: String, headers: Map<String, List<String>>) : this(json, | |||
There was a problem hiding this comment.
This creates a new mapper every time a GraphQLResponse is instantiated. I think we should leave the default mapper construction in the companion object.
There was a problem hiding this comment.
If we leave the default mapper in the companion object, how can we achieve the dependency injection? I'm not aware of the kotlin semantics, but I suspect that this companion object is some sort of a static instance.
There was a problem hiding this comment.
I was suggesting leaving the default ObjectMapper in the companion object so that it's only instantiated once, but can be used as an argument to the constructor. e.g.:
@@ -38,7 +38,9 @@ import org.slf4j.LoggerFactory
* Representation of a GraphQL response, which may contain GraphQL errors.
* This class gives convenient JSON parsing methods to get data out of the response.
*/
-data class GraphQLResponse(@Language("json") val json: String, val headers: Map<String, List<String>>) {
+data class GraphQLResponse(@Language("json") val json: String,
+ val headers: Map<String, List<String>>,
+ private val mapper: ObjectMapper = DEFAULT_MAPPER) {
/**
* A JsonPath DocumentContext. Typically, only used internally.
@@ -120,17 +122,12 @@ data class GraphQLResponse(@Language("json") val json: String, val headers: Map<
companion object {
private val logger: Logger = LoggerFactory.getLogger(GraphQLResponse::class.java)
- private val mapper: ObjectMapper = jacksonObjectMapper()
+ private val DEFAULT_MAPPER: ObjectMapper = jacksonObjectMapper()
.registerModule(JavaTimeModule())
.registerModule(ParameterNamesModule())
.registerModule(Jdk8Module())
.enable(DeserializationFeature.READ_UNKNOWN_ENUM_VALUES_USING_DEFAULT_VALUE)There was a problem hiding this comment.
This definitely looks better!
| data class GraphQLResponse( | ||
| @Language("json") val json: String, | ||
| val headers: Map<String, List<String>>, | ||
| val mapper: ObjectMapper |
There was a problem hiding this comment.
Maybe we should make the property private?
* Keep DEFAULT_OBJECTMAPPER in companion object * Make DI objectmapper private
|
Hi guys, could I get another review/+2? Thank you :) |
| @@ -118,19 +133,14 @@ data class GraphQLResponse(@Language("json") val json: String, val headers: Map< | |||
| fun hasErrors(): Boolean = errors.isNotEmpty() | |||
|
|
|||
| companion object { | |||
| private val logger: Logger = LoggerFactory.getLogger(GraphQLResponse::class.java) | |||
| val logger: Logger = LoggerFactory.getLogger(GraphQLResponse::class.java) | |||
There was a problem hiding this comment.
Why did this change? Seems unrelated?
* Keep DEFAULT_OBJECTMAPPER in companion object * Make DI objectmapper private
|
@gabbigum I think you introduced a compilation issue while rebasing (default mapper is there twice now). Could you fix that and squash the commits into a single commit? Changes look good, so I'll merge when that's done! |
* Keep DEFAULT_OBJECTMAPPER in companion object * Make DI objectmapper private
|
Hi Paul, I'm having some issues squashing the commits, as I've already rebased from master and if I squash them I'll override some of the commits I've rebased. Could you assist me with this? |
|
Thanks @gabbigum! |
The default object mapper is not enough to satisfy all deserialization scenarios.
One such scenario is where we need object mapper with custom deserializer for graphql.relay.PageInfo.
Pull request checklist
first
Pull Request type
Changes in this PR
Describe the new behavior from this PR, and why it's needed
Issue #
Alternatives considered
Describe alternative implementation you have considered