graphql-parser
graphql-parser copied to clipboard
Avoid using BTreeMap in `Value::Object` and use `Vec<(String, Value::Object)` instead?
GraphQL-JS (the reference implementation) implements the values as an array of key->value.
This is done in order to allow parsing things like:
field(arg: { f: true, f: false })
And be able to have in the result [ {key: "f", value: true}, {key: "f", value: false }]
. Today it's just { f: false}
in graphql-parser
.
Where a field can be specified multiple times. In terms of parsing, it should be better to use the array.
The spec/graphql-js also implements a validation rule (UniqueInputFieldNamesRule
) to strictly enforce the existence of only one field.
Currently, this creates an ambiguity issue, since the consumer of the parsed Value
is getting the "latest" value specific.
The spec/graphql-js also implements a validation rule (
UniqueInputFieldNamesRule
) to strictly enforce the existence of only one field.Currently, this creates an ambiguity issue, since the consumer of the parsed
Value
is getting the "latest" value specific.
This sounds like specifying key twice is useless, just we don't check for that speficically, right?
Can we just add a check to the parser and keep BTreeMap
?
This sounds like specifying key twice is useless, just we don't check for that speficically, right?
Yeah, but according to the spec, this is not a concern for the GraphQL parser - the parser needs to just parse both.
If a user executes the following query: field(arg: { f: true, f: false })
, then the GraphQL response should be a validation error.
Can we just add a check to the parser and keep
BTreeMap
?
With the current behavior, based on BTreeMap
, we can't fail it since we don't know if the user-specified more than one. this means that running field(arg: { f: true, f: false })
becomes a valid query, while it isn't.
I know this might be a breaking change to the API of the existing struct
s, but it's important since it affects the GraphQL execution and determinism.
@tailhook I can open a PR if that helps :)
I'm not sure yet. I wonder what others think on this. /cc @graphql-rust/graphql-parser-maintainers, @graphql-rust/graphql-client-maintainers
I'm not sure yet. I wonder what others think on this. /cc @graphql-rust/graphql-parser-maintainers, @graphql-rust/graphql-client-maintainers
Hi @tailhook , any update on that? thanks!
Is this something where the server wants the Vec
to be able to tell the difference, but a client wants BTreeMap
to avoid having invalid states it sends?
Is this something where the server wants the
Vec
to be able to tell the difference, but a client wantsBTreeMap
to avoid having invalid states it sends?
I think even from the point of view of a client, it's not a good practice to override keys? It might lead to unexpected behavior.
Also, this crate is not "taking a side" (client/server), so I believe it's better to just stick to the GraphQL spec/reference implementation when it comes to parsing and handling of those?
@tailhook how should we proceed with this issue?