Skip to content

Is the order of argument definitions significant? #577

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

Open
spawnia opened this issue Apr 13, 2019 · 8 comments · May be fixed by #673
Open

Is the order of argument definitions significant? #577

spawnia opened this issue Apr 13, 2019 · 8 comments · May be fixed by #673

Comments

@spawnia
Copy link
Member

spawnia commented Apr 13, 2019

I am aware the spec defines that the order in which the arguments are passed does not matter: https://graphql.github.io/graphql-spec/draft/#sec-Language.Arguments

However, We are currently dealing with an issue where the order in which those arguments are applied is significant, see nuwave/lighthouse#728

Let me give you a simple example. Suppose we defined a query that concats two strings:

type Query {
	concat(left: String!, right: String!): String
}

Given the rule that arguments are unordered and maintain identical semantical reason no matter in which order they are passed, the following calls must return the same result. Both fields should return foobar.

{
	concat(left: "foo", right: "bar")
    concat(right: "bar", left: "foo")
}

This indicates that on the server side, there must be an implicit ordering that determines how those arguments should be handled. In this particular case, the implementation will probably just do that by just doing something like this in my resolver.

return args.left + args.right

Things are working fine at this point.

Now, i might want to grow the number of allowed arguments to the concat function. As the spec declares, the order in which the user passes them must be normalized. Instead of spelling out each argument, i might want to do something like this in my resolver:

return reduce(args, (acc, item) => acc + item);

In that case, it would be practical for me if the arguments were always passed to the resolver in the order that i defined in the schema, instead of me having to take care of ensuring the correct order myself. This can be a subtle cause of bugs.

I am not quite sure if this issue falls within the scope of the spec or is just an implementation detail that might be handled by the various GraphQL libraries.
It would surely make it easier on implementors if they would not have to worry about
the order in which a user passes the arguments and are rather always passed them in
a previously defined order.

It would be worthwile to clarify if the schema definition itself can be used
to define the canonical order that arguments will be normalized to, or if the following two definitions are considered equivalent:

type Query {
	concat(left: String!, right: String!): String
    # Switch the argument order in the schema definition
	concat(right: String!, left: String!): String
}

A simple note like #470 could at least enable the implementations to build atop the ordering of argument definitions safely.

@nodkz
Copy link

nodkz commented Apr 14, 2019

For named parameters, the order is not significant in most programming languages.
Sorting arguments for every request at runtime are a costly task, and the majority of users does not require it. So for what purpose we should do a redundant job?

So if you need concat operation with ordered args, then you need

  • to use one arg with Array shape. Order in arrays is guaranteed.
  • or you may read fourth argument info in your resolve method, which will contain AST of the query from which you may read the order of provided args from the client.

I don't feel that reordering of arguments from the client according to the schema is a common task.

PS According to your issue nuwave/lighthouse#728 you need to use several mutations in your operation. Mutations calls are ordered and sync.

@megawubs
Copy link

If I understand correctly, the current implementation is correct and the bug is actually not a bug?

@jdehaan
Copy link

jdehaan commented Apr 14, 2019

There is a bug IMHO, but in the implementation that relies on the order of arguments not in the spec. The whole point to have named arguments is to not to have to care about that and be extensible (addition or later removal of obsolete things) without breaking.

In the example above rename a into leftPart and b into rightPart then things should be understandable to anyone... In the case of adding multiple parts, using an array rightParts has the proper semantics to handle the ordering.

@spawnia
Copy link
Member Author

spawnia commented Apr 14, 2019

For named parameters, the order is not significant in most programming languages.

That is true if you access them by name. However, if you iterate through them, the order might matter. The languages i know do actually preserve the order of maps/associate arrays.

I do agree that the concat example would be better of with an array of arguments. It is not actual code i would write, but it serves to illustrate the point i am making.

I don't feel that reordering of arguments from the client according to the schema is a common task.

It is probably not that common, but it might come up. Given it is probably pretty rare and has a runtime performance penalty, the spec should not force the implementations to do it.
Thanks for chiming in on this and working out this part.

However, it would be great if the spec could clarify if the definition order is significant. So what this issue basically boils down to whether the two following schema definitions should be considered equivalent.

type Query {
	concat(left: String!, right: String!): String
}
type Query {
	concat(right: String!, left: String!): String
}

If they are distinct, an implementation of an SDL parser would have to make sure to preserve the order when converting to an AST.

PS According to your issue nuwave/lighthouse#728 you need to use several mutations in your operation. Mutations calls are ordered and sync.

Actually, we are sending one big mutation which contains a large, nested input. What you are saying is true, but the sub-operations given by the input are not governed by the GraphQL spec in such a way.

@IvanGoncharov
Copy link
Member

IvanGoncharov commented Apr 15, 2019

@spawnia From a practical point of view some languages (e.g. Go) doesn't keep stable order of keys in maps. That means every time you ask for introspection you get a different order of stuff (fields, args, enum values, etc.). That's why I implemented lexicographicSortSchema:
https://github.com/graphql/graphql-js/blob/38b9935430c34d8c700a1cc463a4f039739ce71d/src/utilities/lexicographicSortSchema.js#L36-L39

Same goes true for every schema that generated from a source (other than SDL) that doesn't guarantee consistent order, e.g. JSON Schema/OpenAPI or any other JSON based format since JSON doesn't guarantee the order of keys.

Also, does it mean that any reordering of argument should be treated as breaking change for your API?

So it would be significant break-in change if we start enforcing an order of any elements in SDL except for directives.

Next question: Why directives are special in this case?

Directives exist only in the context of GraphQL Document (SDL or query) so we should just ensure they are stored as an array. Moreover, it's up to you to control what directives do if an order of particular directive mean something.

@spawnia
Copy link
Member Author

spawnia commented Apr 15, 2019

@IvanGoncharov thanks for your answer, it was really insightful.

I think it is worth clarifying this point in the spec. Maybe we can address this more generally by declaring that maps are never guaranteed to preserve order?

@leebyron
Copy link
Collaborator

leebyron commented Aug 7, 2019

Coming back to this issue, I feel pretty strongly that order should be maintained ("significant") between SDL and introspection.

The example in the original post here was about an implementation detail (reduce over the args) and I agree that is out of the scope of this spec.

However what is observable is the order fields, args, etc are produced from introspection.

I'm going to propose that fields, args, and other elements with lexical ordering be maintained in introspection.

So for the given SDL:

type MyType {
  field1(arg1: String, arg2: Int): Boolean
  field2(arg1: String, arg2: Int): Boolean
}

The introspection result MUST produce an ordered array of fields [field1, field2] for MyType and an ordered array of arguments [arg1, arg2].

From a change cost point of view, I think this is light.

  • Because of the executable rules https://graphql.github.io/graphql-spec/draft/#sec-Language.Arguments.Arguments-are-unordered queries should continue to produce identical results.
  • This is currently ambiguous where some implementations do maintain order while others do not. Because of this, tools cannot always rely on this order though sometimes make use of it. If some current implementation does not maintain order then introducing order is not breaking since existing expectations of lack of order would not be disturbed

@leebyron
Copy link
Collaborator

leebyron commented Aug 7, 2019

Also, this wouldn't be the only place we have ordered map behavior. https://graphql.github.io/graphql-spec/draft/#sel-FAHZNABAB6J8uC and https://graphql.github.io/graphql-spec/draft/#note-5efbc explain another example.

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 a pull request may close this issue.

6 participants