search

graphql-rust / graphql-parser

A graphql query language and schema definition language parser and formatter for rust
Apache License 2.0
355 stars 77 forks source link

Avoid using BTreeMap in `Value::Object` and use `Vec<(String, Value::Object)` instead? #59

Open dotansimha opened 2 years ago

dotansimha commented 2 years ago

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.

tailhook commented 2 years ago

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?

dotansimha commented 2 years ago

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 structs, but it's important since it affects the GraphQL execution and determinism.

@tailhook I can open a PR if that helps :)

tailhook commented 2 years ago

I'm not sure yet. I wonder what others think on this. /cc @graphql-rust/graphql-parser-maintainers, @graphql-rust/graphql-client-maintainers

dotansimha commented 2 years ago

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!

mathstuf commented 2 years ago

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?

dotansimha commented 2 years ago

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?

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?

dotansimha commented 2 years ago

@tailhook how should we proceed with this issue?