graphql-client icon indicating copy to clipboard operation
graphql-client copied to clipboard

Custom scalar deserialization error

Open jesskfullwood opened this issue 6 years ago • 6 comments

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.

jesskfullwood avatar Aug 15 '19 15:08 jesskfullwood

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 avatar Aug 15 '19 16:08 tomhoule

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

tomhoule avatar Aug 15 '19 16:08 tomhoule

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.

jesskfullwood avatar Aug 15 '19 16:08 jesskfullwood

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 avatar Aug 15 '19 16:08 tomhoule

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

tomhoule avatar Aug 15 '19 16:08 tomhoule

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.

jesskfullwood avatar Aug 15 '19 16:08 jesskfullwood