Parser too strict on ordering after frozen lists

[chadwhitely]

When parsing a query, the parser adheres to the original order, even in places where order shouldn't matter (arguments). This causes the two below queries to be marked as inequal by the comparator. Recommendation would be to sort any nodes for places where order doesn't matter, as part of optional_many.

We previously got around this issue by sorting the AST ourselves, but with the introduction of FrozenList, this now results in errors.

E       AssertionError: queryA =
E       {
E         vehicle(id: 3, otherId: 2) {
E           version
E           cancel
E           appointment {
E             when
E             afterDate
E           }
E           trigger {
E             how
E             Phase
E             PhaseOption
E             PhaseDelaySecs
E           }
E         }
E       }
E       
E        queryB =
E       {
E         vehicle(otherId: 2, id: 3) {
E           version
E           cancel
E           appointment {
E             when
E             afterDate
E           }
E           trigger {
E             how
E             Phase
E             PhaseOption
E             PhaseDelaySecs
E           }
E         }
E       }

[Cito]

Can you post the exact code that raised this AssertionError? Also, I'm not sure where you think a change is needed: In the node comparison method or in the parser?

[chadwhitely]

Let me try to get you a cleaned up version of that code. The TL;DR is that we have a static query that is the expected query output of our component. We send a REST call into the component, capture the actual output towards our DB API, and compare the two objects. Previously, we had been sorting the AST according to node type.

As for implementation, my suggestion was to sort it in the parser when in a section that order doesn't matter. However, that may end up being misleading if you're not expecting the behavior, as the AST could be different from your given query. Changing the comparison function would be ideal, as it would allow for two functionally identical objects to be marked as equivalent.

[chadwhitely]

Click to expand archived comment. This comment is invalid. I used `no_location=True` on parse and was able to have a successful comparison.

Ok, so this just became an issue with the comparison function. I went ahead and ordered my test query to match the application query as a workaround. However, it is now becoming even more problematic. Even identical queries are being marked as inequal for whatever reason: ``` def assert_graphql_queries_equal(query_block_a, query_block_b): queryA = parse(query_block_a["query"]) queryB = parse(query_block_b["query"])

    assert queryA == queryB, \

      f"queryA=\n{print_ast(queryA)} \nqueryB=\n{print_ast(queryB)}"

E AssertionError: queryA=
E {
E vehicle(id: 2, otherId: 3) {
E version
E cancel
E appointment {
E when
E afterDate
E }
E trigger {
E how
E Phase
E PhaseOption
E PhaseDelaySecs
E }
E }
E }
E
E queryB=
E {
E vehicle(id: 2, otherId: 3) {
E version
E cancel
E appointment {
E when
E afterDate
E }
E trigger {
E how
E Phase
E PhaseOption
E PhaseDelaySecs
E }
E }
E }

Version is `graphql-core       3.0.3 `.

Test data generated using this function:

@staticmethod
def query_vehicle(otherId, id):
    query_string = f"""
        query {{
        vehicle(id: {id}, otherId: {otherId}) {{
            version,
            cancel,
            appointment {{
            when,
            afterDate
            }},
            trigger {{
            how,
            Phase,
            PhaseOption,
            PhaseDelaySecs
        }}
        }}
        }}
        """
    return {"query": query_string}

</details>

[Cito]

@chadwhitely Thanks for the example code, but is is still not so complete that I can reproduce it. Particularly, it's unclear to me whether query_block_a["query"] == query_block_b["query"] before the parse step. When I take the same query as you, parse it two times, and compare, the results are equal:

>>> query = query_vehicle(2, 3)
>>> query_a = parse(query)
>>> query_b = parse(query)
>>> print(query_a == query_b)
True

The no_location determines whether locations are stored. So if you make changes like adding an empty line somewhere in only one of the sources, and you parse with no_location=False (the default), then they will not be equal.

[chadwhitely]

Ok, this should be what you need. Disclaimer: this was hacked up just for this debugging purpose. The spacing within the strings is indicative of the spacing within our tests and application.

from graphql.language import parse
from graphql.language.printer import print_ast


queryStringA="""
            query {
            vehicle(otherId: 3, id: 2) {
                Version,
                cancel,
                appointment {
                when,
                afterDate
                },
                trigger {
                how,
                Phase,
                PhaseOption,
                PhaseDelaySecs
            }
            }
            }
            """

queryStringB="""query {
vehicle(id: 2, otherId: 3)
  {
    Version,
    cancel,
    appointment
    {
      when,
      afterDate
    },
    trigger
    {
      how,
      Phase,
      PhaseOption,
      PhaseDelaySecs
    }
  }
}"""

queryA = parse(queryStringA, no_location=True)
queryB = parse(queryStringB, no_location=True)

assert queryA == queryB, \
assert queryA == queryB, \
f"""
queryStringA=
{queryStringA}
queryA=
{print_ast(queryA)}
queryStringB=
{queryStringB}
queryB=
{print_ast(queryB)}"""

Edit: And of course, by making the arguments of vehicle match their order, these two parse as identical.

[Cito]

Ok, now your issue is clear. Sorry for not reading your original question carefully enough.

Possible solutions: 1) We could change the parser to always return sorted arguments as you suggested. However, this would deviate too much from the behavior of GraphQL.js which we want to emulate, and it could lead to surprises when arguments are expected to appear in the order of the source for one reason or the other. Sorting arguments will also add a small performance penalty whenever we parse a query. 2) We could change the comparison operator. However, it would become very complicated and slow. We would need to convert every argument list to a set before comparing them. 3) We could add a utility function to normalize AST documents by lexicographically sorting them, like you used to do. Looks like the best option to me. Btw, the FrozenLists should not be a big issue here, you just have to write node.args = FrozenList(sorted(node.args)) instead of sort(node.args).

Actually, there is already such a utility function for schemas called lexicographic_sort_schema.

But I think it is not so straight forward to implement a similar thing for GraphQL queries. For instance, the question is, would we want to sort the fields as well (e.g. switch when and afterDate)? I think you can argue that this would be still the "same" query since JSON is not ordered. But it gets complicated here - actually GraphQL tries to maintain the order if you query locally, so it may or may not be relevant. So we need to think about and define which Node properties should be sorted by that utility function. Also, we must decide whether this should become a Node method that works in place (probably recursively), or a function that returns a normalized copy of the node, like lexicographic_sort_schema.

[Cito]

I noticed that GraphQL.js already has a related issue.

[Cito]

In the meantime, you can use something like this for normalizing argument lists in your queries:

from operator import attrgetter
from graphql.language import Node
from graphql.pyutils import FrozenList

def normalize_node(node):
    for key in node.keys:
        value = getattr(node, key)
        if not value:
            continue
        if isinstance(value, Node):
            normalize_node(value)
        elif isinstance(value, FrozenList):
            for v in value:
                normalize_node(v)
            if key == 'arguments':
                node.arguments = FrozenList(sorted(
                    value, key=attrgetter("name.value")))

[chadwhitely]

Many thanks. It's at this point that I confess that I am working with secondhand GraphQL knowledge. I cannot contribute to your decision, but I appreciate you humoring me. For the time being, we have just modified the test data to have the arguments in the correct order, as matches the application for now. Should the app order change, we will review and utilize your normalize_node function. Thank you!