Skip to content
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

Custom implementation of convertValue for input types #549

Merged
merged 3 commits into from Aug 20, 2021

Conversation

paulbakker
Copy link
Collaborator

@paulbakker paulbakker commented Aug 19, 2021

We use Jackson's convertValue to convert the Map of input values created by graphql-java to a Java class for ease-of-use in data fetchers.
Using Jackson's convertValue has always been problematic. It serializes the input Map and then deserializes it again into the target Class. In this process, it doesn't know that graphql-java already took care of deserializing scalar values.

This means the following happened:
Query input (as String) -> deserialized (using scalars) to Map by graphql-java -> Jackson serializes the value again, without using scalars, and deserializes to a Class, again without using scalars.

In most cases, this happened to work, because Jackson could "accidentally" successfully serialize/deserialize to the same value. In some cases, such as more complex scalars (e.g. data/time), this didn't result in the correct value.
Even though it worked "most of the time", it has always been fundamentally incorrect.

Changes in this PR

In this PR a custom "converter" is added. Note that the converter only maps a Map to a Java type, it doesn't do any deserialization. The deserialization is done by graphql-java.
It accounts for both Java and Kotlin classes because they are instantiated differently through reflection.
We already had good test coverage for @InputArgument and all tests are passing, making this mostly a change that users won't notice, except in the special cases that previously didn't work.

Fixes: #70

@paulbakker paulbakker requested a review from berngp August 19, 2021 19:58
Copy link
Contributor

@berngp berngp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me. I'll do another pass but I think we should merge and have an RC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom scalars not usable as mutation input
2 participants