Skip to content
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

Variable values ordering not kept during execution #225

Closed
bellini666 opened this issue Aug 24, 2024 · 4 comments
Closed

Variable values ordering not kept during execution #225

bellini666 opened this issue Aug 24, 2024 · 4 comments

Comments

@bellini666
Copy link

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
Copy link
Member

Cito commented Aug 25, 2024

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
Copy link

j30ng commented Aug 28, 2024

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
Copy link
Member

Cito commented Sep 8, 2024

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.

@Cito Cito closed this as completed Sep 8, 2024
@bellini666
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants