graphql-python / graphql-core

A Python 3.6+ port of the GraphQL.js reference implementation of GraphQL.
MIT License
512 stars 136 forks source link

Variable values ordering not kept during execution #225

Closed bellini666 closed 2 months ago

bellini666 commented 2 months ago

While trying to solve this issue on strawberry-django, I just noticed that variable values are not kept in the order they were given, unlike the fields themselves.

Was even checking the spec and found this (https://spec.graphql.org/draft/#sec-Serialized-Map-Ordering), which for sure does apply to fields as per the example in there, but I'm wondering if the variables ordering should be preserved as well?

Cito commented 2 months ago

Can you provide some GraphQL-core only example code that shows exactly what you expect?

I'd like to test this with the lastest version of GraphQL-core and with GraphQL.js for comparison. If there is a discrepancy, I would fix it in GraphQL-core, otherwise this needs to be solved in GraphQL.js (which is considered a reference implementation of the spec) and from there ported to GraphQL-core.

j30ng commented 2 months ago

Hi all. Here's a simple test I set up to illustrate the issue, @Cito. I used graphql-core version 3.3.0a6.

Schema:


query_type = GraphQLObjectType(
    "Query",
    lambda: {
        "echo": GraphQLField(
            GraphQLString,
            args={
                "arg": GraphQLArgument(
                    GraphQLInputObjectType(
                        "ArgInput",
                        lambda: {
                            "id": GraphQLInputField(GraphQLString),
                            "name": GraphQLInputField(GraphQLString),
                        }
                    ),
                )
            },
            resolve=lambda _source, _info, arg: ", ".join(arg.keys()),
        ),
        # ...
    },
)

Consider two test functions:

source = """
query Echo($arg: ArgInput) {
  echo(arg: $arg)
}
"""

def describe_ordering_tests():
    @pytest.mark.asyncio()
    async def test_correctly_ordered():
        variable_values = {"arg": {"id": "ABC", "name": "DEF"}}
        result = await graphql(
            schema=schema, source=source, variable_values=variable_values
        )
        assert result == ({"echo": "id, name"}, None)

    @pytest.mark.asyncio()
    async def test_correctly_ordered_2():
        variable_values = {"arg": {"name": "DEF", "id": "ABC"}}
        result = await graphql(
            schema=schema, source=source, variable_values=variable_values
        )
        assert result == ({"echo": "name, id"}, None)

The test result is as follows:

tests/test_order.py::describe_ordering_tests::test_correctly_ordered PASSED                                                                                                                                                                                                                                                                                                                                                        [ 50%]
tests/test_order.py::describe_ordering_tests::test_correctly_ordered_2 FAILED                                                                                                                                                                                                                                                                                                                                                      [100%]

===================== FAILURES =====================
_____________ test_correctly_ordered_2 _____________

    @pytest.mark.asyncio()
    async def test_correctly_ordered_2():
        variable_values = {"arg": {"name": "DEF", "id": "ABC"}}
        result = await graphql(
            schema=schema, source=source, variable_values=variable_values
        )
>       assert result == ({"echo": "name, id"}, None)
E       AssertionError: assert ExecutionResu..., errors=None) == ({'echo': 'name, id'}, None)
E
E         Full diff:
E         + ExecutionResult(data={'echo': 'id, name'}, errors=None)
E         - (
E         -     {
E         -         'echo': 'name, id',
E         -     },
E         -     None,
E         - )

tests/test_order.py:29: AssertionError

It seems after being parsed, the key order of the variable arg is id, name in both cases. I expected however when the properties of arg for the query is given in the order of name, id, the GraphQL server engine parses it as { "name" : ..., "id" : ... }, preserving the original order. I'm not quite sure how Graphql.js behaves though.

Cito commented 2 months ago

Hi @bellini666. I have tested this with GraphQL.js v16 and the latest v17 alpha, and the behavior is the same:

import {
  graphql,
  GraphQLSchema,
  GraphQLString,
  GraphQLInputObjectType,
  GraphQLObjectType,
} from "graphql";

const QueryType = new GraphQLObjectType({
  name: "Query",
  fields: () => ({
    echo: {
      type: GraphQLString,
      args: {
        arg: {
          type: new GraphQLInputObjectType({
            name: "ArgInput",
            fields: () => ({
              id: { type: GraphQLString },
              name: { type: GraphQLString },
            }),
          }),
        },
      },
      resolve: (_source, { arg }) => Object.keys(arg).join(", "),
    },
  }),
});

const schema = new GraphQLSchema({
  query: QueryType,
});

const source = `
query Echo($arg: ArgInput) {
  echo(arg: $arg)
}`;

let variableValues = { arg: { id: "ABC", name: "DEF" } };

let result = await graphql({ schema, source, variableValues });

console.log(result);  // { echo: 'id, name' }

variableValues = { arg: { name: "DEF",  id: "ABC" } };

result = await graphql({ schema, source, variableValues });

console.log(result);  // { echo: 'id, name' }

If you think you can argue that the behavior should be changed, please report this upstream.

You can paste the above example there.

bellini666 commented 2 months ago

Oh, that's a shame. Thanks for taking a look at the @Cito :)

I'll try to open an issue there to talk about this