dillonkearns / elm-graphql

Autogenerate type-safe GraphQL queries in Elm.
https://package.elm-lang.org/packages/dillonkearns/elm-graphql/latest
BSD 3-Clause "New" or "Revised" License
778 stars 107 forks source link

Idea: serialize invalid JSON Input Values as arguments #606

Open dillonkearns opened 2 years ago

dillonkearns commented 2 years ago

Motivation

605 addresses #570, but also makes me think of one longer term thing for a future change. This PR causes errors in fewer cases and doesn't introduce any new error cases so I think it's a great starting point. Though it does give something that people could rely on that could still cause errors, so it's worth considering those things for the future.

The GraphQL spec allows for Input Values that are exactly the same as what JSON allows, except

1) Object keys aren't quoted (which #605 fixes)

2) Therefore, object keys must be GraphQL Names

The longer-term issue is that we're allowing arbitrary JSON values to be sent, but the spec doesn't actually allow that. If people start depending on this functionality then they can hit up against cases where there's no way to send certain values because they aren't valid GraphQL Names.

Summary

I think we could solve this problem by serializing these invalid

mutation Mutation($jsonABC: MyJsonScalar!) {
  saveJsonBlob(input: $json1) {
      # ...
  }
}

Implementation Ideas

This type could change to better capture the full valid GraphQL Input Values, and try to turn values into non-JSON values whenever possible:

https://github.com/dillonkearns/elm-graphql/blob/0d9f732e0160df59222ba4a7e3954c988ca855f5/src/Graphql/Internal/Encode.elm#L23-L29

Then for each Input Value that can't be serialized, the Encode.serialize function could hash the unserializable JSON and use that hash to refer to a variable name (similar to how field aliases are hashed). And it could change its type signature to return a (possibly empty) Set (String, String), where each tuple is a variable name that needs to be declared and its type name, like ( "jsonABC", "MyJsonScalar!" ). Then the recursive calls to serialize would need to propagate those up and declare them each as variables.

https://github.com/dillonkearns/elm-graphql/blob/0d9f732e0160df59222ba4a7e3954c988ca855f5/src/Graphql/Internal/Encode.elm#L142-L145