Skip to content

Fixing arguments order when getting value from AST #533

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

Conversation

ArnoudThibaut
Copy link

Hello,
Given the following InputObjectType :

new InputObjectType([
    'name'   => 'order',
    'fields' => [
        'foo'  => ['type' => Type::string()],
        'bar'  => ['type' => Type::string()]
    ],
]);

and something like order: {bar: "ASC", foo: "ASC"} as value, the return of GraphQL\Utils\AST::valueFromAST will be ['foo' => 'ASC', 'bar' = 'ASC'].

However when the order of arguments has importance, like constructing an ORDER SQL clause, this can pose a problem and the return should be ['bar' => 'ASC', 'foo' = 'ASC'].

The aim of this PR is to fix this by using the order given by the user instead of the order defining during the field configuration.

I used directly assertSame() in the test case instead of the helper method runTestCaseWithVars because this method use assertEquals and this doesn't test the order of the keys.

Tell me if I should replace assertEquals by assertSame in the helper method, I was not sure about that.

fix : api-platform/core#2808

@vladar
Copy link
Member

vladar commented Aug 12, 2019

This is not a bug. The specification clearly states this:

Input object fields are unordered

Input object fields may be provided in any syntactic order and maintain identical semantic meaning.

These two queries are semantically identical:

Example № 30

{
  nearestThing(location: { lon: 12.43, lat: -53.211 })
}

Example № 31

{
  nearestThing(location: { lat: -53.211, lon: 12.43 })
}

While I understand what you are trying to achieve, relying on fields / arguments order is not spec-compliant. If we merge this it will make client apps fragile because they will rely on the implementation-specific feature.

See also recent discussion about related topic in the graphql-spec repo.

So for your use-case of order clause the correct model would be a single field of List type. Lists do preserve ordering of elements.

P.S. Edited this comment to address input object fields (initially posted about arguments, but spec addresses both input object fields and arguments ordering)

@vladar vladar closed this Aug 12, 2019
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

Successfully merging this pull request may close these issues.

[GraphQL] OrderFilter does not consider the order of the properties to filter on
2 participants