-
Notifications
You must be signed in to change notification settings - Fork 427
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
base: master
Are you sure you want to change the base?
Conversation
For some reason I am getting an:
error when I try to use an object (e.g.: |
.and_then(|s| serde_json::from_str(s).ok()) | ||
} | ||
|
||
from_str<'a>(value: ScalarToken<'a>) -> ParseScalarResult<'a, S> { |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
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. |
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 do you know of Graphql.js supports this? |
@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? |
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 Maybe these pain points will be alleviated if/when input unions are supported and we wouldn't need this? |
Hi, any update? This feature is necessary in my project |
@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. |
Yeah I'll clean it up and land |
There was a problem hiding this 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:
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);
}
graphql_scalar!(JsonValue as "JsonString" where Scalar = <S> { | ||
description: "JSON serialized as a string" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
A small example with opaque json is here, it uses the approach for the resolving that I talking about: |
@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 |
Ok, will do. |
Any updates on this? |
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. |
Here's another approach to the idea: #975 |
No description provided.