graphql / graphql-spec

GraphQL is a query language and execution engine tied to any backend service.
https://spec.graphql.org
14.3k stars 1.12k forks source link

Is the order of argument definitions significant? #577

Open spawnia opened 5 years ago

spawnia commented 5 years ago

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 https://github.com/nuwave/lighthouse/issues/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 https://github.com/graphql/graphql-spec/pull/470 could at least enable the implementations to build atop the ordering of argument definitions safely.

nodkz commented 5 years ago

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

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

PS According to your issue https://github.com/nuwave/lighthouse/issues/728 you need to use several mutations in your operation. Mutations calls are ordered and sync.

megawubs commented 5 years ago

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

jdehaan commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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.

leebyron commented 5 years ago

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.