Decoding fails for non-string scalars

[xtian]

In our application we have scalars such as Watts and WattHours where the back-end serializes the value to a JSON float. However, the Fields that Graphqelm generates expect all scalars to be serialized as strings:

power : Field (Maybe Api.Scalar.Watts) Api.Object.SomeRecord
power =
    Object.fieldDecoder "power" [] (Decode.string |> Decode.map Api.Scalar.Watts |> Decode.maybe)

This decoder causes the whole decode for this record to fail and return a Nothing

This is also problematic on the encoding side since the back-end will expect these values to be serialized as JSON floats there as well.

[xtian]

On the decoder side, perhaps what we need is something like this:

Decode.oneOf
    [ Decode.float |> Decode.map toString
    , Decode.int |> Decode.map toString
    , Decode.string
    ]

(Although ideally the definition of Api.Scalar.Watts would be type Watts = Watts Float, there's no way to generate that via GraphQL introspection as far as I know)

[dillonkearns]

Hey @xtian, thanks for filing the issue! I thought this day might come, I saw this oddity in the GraphQL spec and had a card for it on my Trello board, but I thought I'd see if this was actually ever a problem in practice to be sure. It is unfortunate that GraphQL is perfectly typed with this one exception, it would be pretty easy to add it to the spec that scalars can specify a type and are strings by default for example.

The fix is exactly as you say, using Decode.oneOf, and since Graphqelm can't know what the scalar's primitive type is, the user will then have to use Field.mapOrFail to parse it from there. I'll push a fix for this shortly!

[xtian]

@dillonkearns Thanks for the quick response! I agree that this seems like an oversight in the GraphQL spec. Like you say, it seems to me that the spec should require custom scalars to inherit from one of the built-in scalars.

Do you have any thoughts on how to handle this on the encoder side?

For example, we have generated encoder functions that look like this:

encodeCreateSomeRecordInput : CreateSomeRecordInput -> Value
encodeCreateSomeRecordInput input =
    Encode.maybeObject
        [ ( "power", (\(Api.Scalar.Watts raw) -> Encode.string raw) |> Encode.optional input.power ) ]

but the back-end is expecting power to be a float in the JSON payload.

EDIT: I was thinking we could just send everything up as strings and coerce them on the back-end, but the spec actually dictates that string values should not be accepted.

From the section on float scalars:

When expected as an input type, both integer and float input values are accepted. Integer input values are coerced to Float by adding an empty fractional part, for example 1.0 for the integer input value 1. All other input values, including strings with numeric content, must raise a query error indicating an incorrect type.

[dillonkearns]

Yeah this is another unfortunate result of that part of the GraphQL spec. When I looked at this originally my thought was that since servers can be flexible about accepting a string and serializing it into a float this would solve the problem.

The approaches that I can think of are:

1. Fix Scalars the GraphQL spec 😄
2. Add some functions for transforming a Scalar value into something with a different primitive. This seems like a really messy API from what I can imagine off the top of my head (since it's just a wrapped String, so access would get really messy if we turned it into a wrapped value which could be any primitive).
3. Use a magic syntax in the description field of the Scalar to indicate the type to GraphQL (something like [type=Float], for example
4. Create some sort of registry, like a json file with

   {
     "Watts": "Float"
   }

   Then Graphqelm can use this to handle these as the correct type in the generated code.
5. Just let the server-side deal with it by accepting strings (I believe with any GraphQL server DSL this should be pretty straightforward).

It definitely seems worth filling an issue on the GraphQL spec repo for approach 1, but obviously this could take quite a while to get through and get into server implementations if it ever does!

I really don't like approach 2, I think that would make users' code very bug prone and would make the API more confusing. 3 and 4 could be interesting to explore. 3 is a bit hackier, and if you don't control the server you aren't able to deal with this. But on the other hand it is part of the schema, which is interesting.

I won't have access to a computer for the next couple of weeks so it will have to be approach 5 for now. But I think that it's worth exploring approach 4 to see if this might be an elegant solution. What do you think?

[dillonkearns]

npm package version 3.1.3 fixes the decode side!

Let me know your thoughts on the different strategies.

[xtian]

@dillonkearns Yeah I think those are all interesting ideas.

1. Fix the GraphQL spec 😄

There is actually some movement on this! RFC: Scalar serialize as built-in scalar type

It seems like this RFC has received a favorable response from the GraphQL WG and they are considering a syntax such as scalar SomeScalarType as String. This would be awesome!

2. Add some functions for transforming a Scalar value into something with a different primitive

My other thought here was possibly defining scalars like this: type SomeScalar a = SomeScalar a

We could then remove the Decode.map toString bit from the decoder and require the user to provide the appropriate encoder. I'm not sure what the all the ramifications of this approach are, though.

3. Use a magic syntax in the description field of the Scalar to indicate the type to GraphQL

I hadn't considered this, but I think it's a pretty interesting idea. Seems like it would be pretty straightforward to implement until there is a change in the spec. There is also a proposal for formalizing access to this sort of metadata in graphql/graphql-spec#300

4. Create some sort of registry

I would prefer 3. to this option since the metadata would be maintained in the same location as the scalar definition. Seems like with a separate file things could accidentally get out of sync. But like you say, it makes sense if you don't control the server implementation. That's not my use-case so I often forget about it 😆

5. Just let the server-side deal with it by accepting strings

In the near-term we can make this change in our back-end but I would be interested in working on an implementation of the RFC I linked for point 1 (I would have to see what it would take to implement this as an extension to Absinthe) or one of other approaches if you think one of those would be better. What do you think?

EDIT: Also thanks again for the quick response and release :)

[dillonkearns]

Nice @xtian, that's great news that the RFC is moving along! I searched for it but somehow missed that. That would of course be ideal and I think it would be really great to get this into Absinthe! I'd be happy to help push this along however I can, and I will of course jump on parsing that "serializes as" data from the Graphqelm side as soon as it's available!

Your idea for 2 is interesting, I hadn't considered that. Requiring the user to provide an encoder seems like a bit of a hurdle though which would be unfortunate, especially considering that most cases of scalars seem to be wrappers for strings (like dates, times, etc., for example). I think you'd actually have to supply a decoder as well since you couldn't define something like

scalarDecoder : Decoder anything
scalarDecoder =
    Decode.oneOf [ Decode.string, Decode.float, Decode.int ]

since Decode.string is a Decoder String and Decoder.float is a Decoder Float, so they can't be in oneOf together. Hence the user would have to provide the decoder, which seems error prone and confusing.

I would prefer 3. to this option since the metadata would be maintained in the same location as the scalar definition. Seems like with a separate file things could accidentally get out of sync.

I definitely share your sentiments there, this solution seems suboptimal as well for this reason, and in its own way kind of hacky.

Given the progress on 1, I'd say let's push for this and go with approach 5 in the meantime. It's exciting that approach 1 is making progress, and if we can help get this into Absinthe that would be fantastic!

[dillonkearns]

@xtian thanks for the excellent discussion! Let's close this issue so that people browsing open issues can see that the decode failures are fixed. I'd love to track progress on server implementations and hear if you do anything on this with Absinthe, so I've opened up #39.