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

Add integration for serde_json::Value #325

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

LegNeato
Copy link
Member

@LegNeato LegNeato commented Mar 2, 2019

No description provided.

@mwilliammyers
Copy link

For some reason I am getting an:

Invalid value for argument ..., expected type ...

error when I try to use an object (e.g.: "{\"foo\": 1}") whereas other JSON strings work like "[]" and "1".

.and_then(|s| serde_json::from_str(s).ok())
}

from_str<'a>(value: ScalarToken<'a>) -> ParseScalarResult<'a, S> {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My custom scalar implementation that worked with JSON objects looked like this:

from_str<'a>(value: ScalarToken<'a>) -> ParseScalarResult<'a, S> {
    <String as ParseScalarValue<S>>::from_str(value)
}

Otherwise, it looks pretty much exactly the same. I don't understand the intricacies of graphql_scalar! so I am not sure that is the correct way to do it...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tested this change and it is working for me.

@theduke
Copy link
Member

theduke commented Mar 5, 2019

Mhm, I know some other libraries that have explicitly rejected this functionality as a built-in, since it goes a bit against the purpose of GraphQL and having this built in could be seen like recommending this pattern.

I realize that it's sometimes necessary and this adds a bit of convenience. But one could argue that this transform would be better as an explicit, manual step.

I'm not too sure though, happy to hear other viewpoints.

@mwilliammyers
Copy link

You changed my mind. There are times when it is 100% necessary, but those are rare. We shouldn't cater to the minority at the potential expense of the majority, especially when it's not that bad to create your own scalar; although it would be much much easier with #303!

@LegNeato LegNeato added the enhancement Improvement of existing features or bugfix label Mar 9, 2019
@theduke
Copy link
Member

theduke commented Mar 9, 2019

@LegNeato do you know of Graphql.js supports this?

@theduke
Copy link
Member

theduke commented May 15, 2019

@LegNeato I'm still rather negative on this due to the reasons stated above. We could put it into the book instead.

Do you feel otherwise?

@mwilliammyers
Copy link

After thinking about this more, I think it would be a good idea to provide this functionality behind a feature flag (that defaults to off). I think that sort of qualifies as an explicit manual step. Maybe the only reason that other libraries do not have this feature is that the language does not have an equivalent to compilation time feature flags?

The desired behavior is simply not possible via newtype structs.

Additionally, using newtype structs are a pain in IDE-like tools like GraphQL playground and graphiql because everything is serialized as a string. It is also really confusing - it is really weird to see a number look like "7"). It also makes using them everywhere in the code a pain.

Maybe these pain points will be alleviated if/when input unions are supported and we wouldn't need this?

@UkonnRa
Copy link

UkonnRa commented Jan 26, 2020

Hi, any update? This feature is necessary in my project

@tyranron
Copy link
Member

tyranron commented Oct 7, 2020

@LegNeato what about finishing this feature? I've recently received a lot of feedback from different people looking for such feature. It seems that passing schemaless opaque JSON is quite widely used.

@LegNeato
Copy link
Member Author

LegNeato commented Oct 7, 2020

Yeah I'll clean it up and land

Copy link

@nlinker nlinker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me add my 2 cents.

What is bothers me the most is that serde_json::Value is mapped to the String type. This makes the corresponding APIs to return escaped strings.
I'd say, this is suboptimal, since we can achieve the same effect by just serializing the serde_json::Value on the dispatch side. I'd however try to make this:
image

This can be achieved by implementing resolve as

impl ScalarValue ... {
    fn resolve(&self) -> juniper::Value {
        convert_to_juniper_value(&self)
    }
    ...
}

pub fn convert_to_juniper_value<S>(json: &serde_json::Value) -> juniper::Value<S>
where
    S: juniper::ScalarValue,
{
    match json {
        serde_json::Value::Null => juniper::Value::null(),
        serde_json::Value::Bool(b) => juniper::Value::scalar(*b),
        serde_json::Value::Number(n) => {
            if let Some(n) = n.as_u64() {
                juniper::Value::scalar(n as i32)
            } else if let Some(n) = n.as_i64() {
                juniper::Value::scalar(n as i32)
            } else if let Some(n) = n.as_f64() {
                juniper::Value::scalar(n)
            } else {
                unreachable!("serde_json::Number has only 3 number variants")
            }
        }
        serde_json::Value::String(s) => juniper::Value::scalar(s.clone()),
        serde_json::Value::Array(a) => {
            let arr = a
                .iter()
                .map(|v| convert_to_juniper_value(v))
                .collect::<Vec<_>>();
            juniper::Value::list(arr)
        }
        serde_json::Value::Object(o) => {
            let obj: juniper::Object<S> = o
                .iter()
                .map(|(k, v)| (k, convert_to_juniper_value(v)))
                .collect();
            juniper::Value::object(obj)
        }
    }
}

The code to check that the function works (one can use the assertion to make the actual test out of it):

    #[test]
    pub fn test_conv() {
        let j = serde_json::json!({
          "key 1": {},
          "key 2": [],
          "key 3": [{}],
          "key 3": [1, "a", true, null],
          "key 5": {
            "2018-10-26": { "x": "", "y": [ "106.9600" ] },
            "2018-10-25": { "x": 2.1, "y": { "arg": 106.9600 } }
          }
        });

        let o = convert_to_juniper_value::<DefaultScalarValue>(&j);
        println!("{}", o);
    }

Comment on lines +7 to +8
graphql_scalar!(JsonValue as "JsonString" where Scalar = <S> {
description: "JSON serialized as a string"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
graphql_scalar!(JsonValue as "JsonString" where Scalar = <S> {
description: "JSON serialized as a string"
#[juniper::graphql_scalar(name = "JsonString", description = "JSON serialized as a string")]
impl<S> GraphQLScalar for GraphQLMap
where
S: juniper::ScalarValue,
{

It seems the macro graphql_scalar! is deprecated, now the #[graphql_scalar] should be used

@nlinker
Copy link

nlinker commented Oct 8, 2020

A small example with opaque json is here, it uses the approach for the resolving that I talking about:
https://github.com/nlinker/rust-graphql-json

@tyranron
Copy link
Member

@nlinker OK, that's easy with output, but I'm unsure about input. Whether the complex JSON object will be parsed OK as field input argument.

Would you be so kind to add an example to you project, where you're accepting something like {"int": 11, "arr": [1.1, null, {}, "c"]} as input argument of computable field?

@nlinker
Copy link

nlinker commented Oct 21, 2020

@nlinker OK, that's easy with output, but I'm unsure about input. Whether the complex JSON object will be parsed OK as field input argument.

Would you be so kind to add an example to you project, where you're accepting something like {"int": 11, "arr": [1.1, null, {}, "c"]} as input argument of computable field?

Ok, will do.

@saskenuba
Copy link

Any updates on this?

@chirino
Copy link

chirino commented Aug 5, 2021

I like where @nlinker was going with this. In cases where you know that the json value is going to have a fixed schema it would be nice if you could parse a graphql SDL doc and pick a type from that to provide the type info. That way you can also use graphql to select into fields of the json value.

@chirino
Copy link

chirino commented Aug 14, 2021

Here's another approach to the idea: #975

@tyranron tyranron mentioned this pull request Aug 15, 2021
11 tasks
@tyranron tyranron added this to the 0.16.0 milestone Apr 13, 2022
@tyranron tyranron modified the milestones: 0.16.0, 0.17.0 Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement of existing features or bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants