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

[dotansimha]

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]

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]

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]

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

[dotansimha]

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]

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]

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]

@tailhook how should we proceed with this issue?