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 scalar deserialization error #247

Closed
jesskfullwood opened this issue Aug 15, 2019 · 7 comments
Closed

Custom scalar deserialization error #247

jesskfullwood opened this issue Aug 15, 2019 · 7 comments

Comments

@jesskfullwood
Copy link

jesskfullwood commented Aug 15, 2019

Not sure if I'm just misunderstanding how this is supposed to work. I have a somewhat complicated type that I would like to send over GraphQL. No problem, I used the juniper::graphql_scalar! macro to just dump it to and load from a JSON string, seem to compile and serializes fine.

But when I query the server with the client, the client doesn't use the from_str function, it just tries to call serde_json::from_str on the whole graphql blob here, which of course fails because my custom type is now just a string from serde's point of view. Is this intended behavior? I expected the client to use it's own deserialization rather than rely on serde.

@tomhoule
Copy link
Member

I'm not sure I understand the exact setup, correct me if I'm wrong. You have a custom scalar type that is serialized as a string in the JSON responses from your API. The right thing to do, on the client side, would be using the FromStr implementation on the JSON string to get your custom type, but it doesn't work because it's using the serde::Deserialize impl, which probably expects an object.

Ideally we would have a way to insert a #[deserialize_with = "<the custom type::from_str"] on the field, maybe that's what we should do (but it's also nice not to be tied to strings, as far as I know custom scalars can be serialized to something else).

One way to do it would be implement TryFrom<&str> for your type by using from_str, and then use the try_from annotation on your struct.

@tomhoule
Copy link
Member

It's a real problem so thanks for reporting, by the way :)

@jesskfullwood
Copy link
Author

jesskfullwood commented Aug 15, 2019

You are mostly right except from_str is a function I defined inside the juniper::graphql_scalar! macro. I basically have

juniper::graphql_scalar!(MyType {
    resolve(&self) -> Value {
        let s = serde_json::to_string(self).unwrap();
        juniper::Value::scalar(s)
    }
    from_input_value(v: &InputValue) -> Option<Self> {
        v.as_scalar_value::<String>().and_then(|s| {
            serde_json::from_str(s).ok()
        })
    }
    from_str<'a>(value: ScalarToken<'a>) -> juniper::ParseScalarResult<'a> {
        <String as juniper::ParseScalarValue>::from_str(value)
    }
});

But the from_str impl (EDIT - or perhaps I mean from_input_value) doesn't actually get used by the web client.

Incidentally, MyType isn't super complex, it is just an enum where some variants have fields

enum MyType {
    X,
    Y { a: i32, b: Vec<i32> },
    Z { c: String }
}

etc.

If there is a better way to do it, I'm all ears! I looked into using a union but would seem to require lots more boilerplate.

@tomhoule
Copy link
Member

tomhoule commented Aug 15, 2019

Ok I think I have an idea (may be wrong).

One way to "trick" serde into doing it would be, when defining your alias for the custom scalar in your client code, do something like:

#[derive(Deserialize)]
struct MyType(#[serde(deserialize_with="backend_types::MyType::from_json_str")] backend_types::MyType);

See https://serde.rs/field-attrs.html for the serde attribute. Since tuple structs (structs with unnamed fields) are "transparent" for serde-json, that's equivalent to redefining the deserialize implementation.

Then you can define from_json_str on your type in terms of serde_json::from_str(s).

@tomhoule
Copy link
Member

Even if it works it's ugly, so we should document it, and try to find a cleaner way to achieve this.

@jesskfullwood
Copy link
Author

jesskfullwood commented Aug 15, 2019

Yes that's a decent workaround, I'll give it a go.

At first I thought this PR would fix it but actually I think it would suffer the same problem.

The nicest thing for the user would be to allow opaque types to be used as scalars as long as they implement serde::Deserialize, and then output them as JSON like everything else (effectively this is what the client code is assuming is already happening). But the above PR mentions that this is not desirable for ... some reason.

@tomhoule
Copy link
Member

Closing this since the issue is 5 years old. Let's open a new one to discuss similar concerns if they pop up again.

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

No branches or pull requests

2 participants