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

Native scalar support for json (w/ feature = "json") #280

Open
kestred opened this issue Nov 23, 2018 · 9 comments · May be fixed by #975
Open

Native scalar support for json (w/ feature = "json") #280

kestred opened this issue Nov 23, 2018 · 9 comments · May be fixed by #975

Comments

@kestred
Copy link
Contributor

kestred commented Nov 23, 2018

I would like to be able to use a serde_json::Value as an Input or Output value in the graph.
This can work fine for output values, but doesn't work for Input values.

Simple-ish work arounds include newtype-ing json::Value and having it serialize as some sort of string or blob--- but that causes easily avoidable and unnecessary pain for both a Javascript Client and for the server itself (which might, for example use the same struct with both diesel and juniper).

This doesn't work (I think) because using an arbitrary JSON value as an input (or output) would require unusual handling during validation that is not described by the GraphQL spec. We could, as an extension have special handling for a serde_json::Value that does not attempt to further validate the input after identifying that the input variable / input-object field is expecting arbitrary JSON.

I believe an extension like this must be implemented within juniper itself (although if I'm wrong, I'd love to know). This should be fairly low risk to add to core, as serde_json is an incredibly widely used and well supported rust crate. Presumably it would be a off-by-default feature flag since it is an extension to the GraphQL spec.

@LegNeato
Copy link
Member

Did you see 2e5df9f on master?

@kestred
Copy link
Contributor Author

kestred commented Nov 25, 2018

@LegNeato --- that doesn't work for actually allowing a json::Value to serialize as you'd expect (e.g. as an Object rather than a blob); so it seems, at least in part because the ScalarValue trait doesn't have a way to represent complex objects.

There are no ScalarValue::as_array or ScalarValue::as_object equivalent; ParseScalarValue can't parse from an object or array (there is no ScalarToken for arrays or objects).

Presumably an alternative to providing json_scalar as a feature would be to provide a complex_scalars feature that adds arrays and objects to the scalar parsing infrastructure. While not everyone will want raw JSON scalars, the serde_json crate is more widely depended-on than chrono, uuid, and url.

Side Note: AFAIK the problem can't be fully solved by implementing GraphQLType normally for serde_json::Value because there isn't a way to allow such a type to be used as both an input and output value.

@piperRyan
Copy link
Contributor

Related discussion on this issue: graphql/graphql-spec#101

@mwilliammyers
Copy link

Is there a workaround for this or could someone point me in the right direction in the codebase so I could submit a PR? The only place I can see is that ScalarValue trait...

@LegNeato
Copy link
Member

LegNeato commented Mar 2, 2019

I'm not sure we want to do this? I don't see a massive benefit over just creating custom scalars and de/serializing Value to a string on both ends. Sure, in the schema it will ultimately be a String once you follow the scalar type def chain and clients will have to have logic, but I am not sure this is worth deviating from the spec. The comments in the above graphql issue basically point to it not being a clear win in general.

I am sure I am missing something though...

@LegNeato
Copy link
Member

LegNeato commented Mar 2, 2019

In any case, I put up a PR for the current-style integration. It still serializes and unserializes to a GraphQL String though, which from the comments above sounds like is not sufficient.

@mwilliammyers
Copy link

I guess it doesn't conform to the spec, but in another Node.js (Apollo) server I have worked with, I used something like https://github.com/taion/graphql-type-json so that you could specify JSON a lot more ergonomically via dev tools like graphiql. It is not a huge deal though, I went ahead and made my own scalar that wraps a Value via a newtype struct.

Being able to directly use Value would be a lot better though, so thanks for the PR!

@kestred
Copy link
Contributor Author

kestred commented Mar 3, 2019

@LegNeato - yes, for me the benefit would be if serialization did not serialize to string; while having a standard serialized-to-string Json is useful in terms of "batteries included" for library users, I think the benefit is to minimize complexity of deserialization/serialization in clients by having it be "full" JSON (instead of serialized json within json).

@SamKomesarook
Copy link

Any update on this?

@tyranron tyranron linked a pull request Aug 15, 2021 that will close this issue
11 tasks
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 a pull request may close this issue.

5 participants