Spec Bug...? ProvidedRequiredArgumentsRule fails to evaluate all potential runtime types

[yaacovCR]

Consider the following schema:

type Query {
  someInterface: SomeInterface
}

interface SomeInterface {
  echo(value: String! = "default"): String
}

type SomeType implements SomeInterface {
  echo(value: String!): String
}

As mentioned by @vepanimas at graphql/graphql-js#3214 => types implementing interfaces have to conform to their types, but not to their default values, and so the above is now considered valid.

Consider the following operation:

{
  someInterface {
    echo
  }
}

The ProvidedRequiredArgumentsRule as specified and implemented within the reference evaluation states that an argument for echo on SomeInterface is not required, because a default value exists, but at runtime, the execution portion of the spec and the implementation take default values from the concrete runtime type, ignoring the interface. The operation will validate successfully, but throw a runtime error.

Thoughts:

1. We either should (A) not allow default values on interface fields (because they are currently apparently never used) or (B) require that implementing types also have default values.
2. We can solve the validation/execution discrepancy either via option (B), or by (Z) changing the Validation Rule to inspect all the possible runtime types.

I think option (B) is a breaking change in terms of 1, because some schemas will break.
I think fixing the validation rule via option (B) or (Z) may also be a breaking change in terms of 2, because some operations will break => but does it count if it is a bug fix? Seems like it does, similar to #1059

[yaacovCR]

See graphql/graphql-js#4272 for a test that demonstrates the current reference implementation behavior.

[martinbonnin]

We either should (A) not allow default values on interface fields (because they are currently apparently never used)

TIL! So given this schema:

interface Displayable {
  thumbnailUrl(size: Size! = MEDIUM): String!
}

type Album implements Displayable {
  thumbnailUrl(size: Size! = LARGE): String!
}

type Query {
  displayable: Displayable
}

and this query:

{
  displayable {
    thumbnailUrl
  }
}

I will get the LARGE thumbnail always? Because there is only 1 concrete type and the interface default value is never used?

I find this surprising. The fact that the actual argument and the default value displayed by tooling such as graphiql, etc.. don't match feels pretty weird.

[yaacovCR]

The fact that the actual argument and the default value displayed by tooling such as graphiql, etc.. don't match feels pretty weird.

If we go with (B) for (1), the existence of a default value on an interface would be meaningful, because it would result in enforcement that implementing types also have default values, but the actual value of that default value would still not be useful, still leading to the weirdness you are noting ==> assuming that tools such as graphiql are not updated. We could extend (B) even further and require all the default values of the implementing types to be identical to the default value on the interface. This would also appear to be a breaking change for some schemas => and potentially not desirable.

[benjie]

Recreation using the reference implementation below; note that the schema and operation are both valid but that the resolver itself is never executed due to the behavior of CoerceArgumentValues() - i.e. this isn't a critical bug, but is something where stronger validation may be helpful. In particular, it feels like this schema should raise an error that the implementations should have a default since the interface does. I don't think the defaults have to necessarily align, though having them not align is potentially confusing.

import { buildSchema, graphqlSync, parse, print, printSchema, validate, validateSchema } from "graphql";

const schema = buildSchema(/* GraphQL */ `
  enum Size { MEDIUM LARGE }
  interface Displayable {
    thumbnailUrl(size: Size! = MEDIUM): String!
  }
  type Album implements Displayable {
    thumbnailUrl(size: Size!): String!
  }
  type Query {
    displayable: Displayable
  }
`);

{
  console.log();
  const errors = validateSchema(schema);
  if (errors.length > 0) {
    console.log("SCHEMA IS INVALID");
    console.dir(errors);
    process.exit(1);
  }
  console.log("SCHEMA IS VALID");
  console.log(printSchema(schema));
}

const source = /* GraphQL */ `
  {
    displayable {
      thumbnailUrl
    }
  }
`;

{
  console.log();
  const ast = parse(source);
  const errors = validate(schema, ast);
  if (errors.length > 0) {
    console.log("OPERATION IS INVALID");
    console.dir(errors);
    process.exit(1);
  }
  console.log("OPERATION IS VALID");
  console.log(print(ast));
}

const result = graphqlSync({
  schema,
  source,
  rootValue: {
    displayable() {
      return {
        __typename: "Album",
        thumbnailUrl(_, { size }) {
          return size;
        },
      };
    },
  },
});

console.log();
console.log("OPERATION RESULT");
console.log(result);

Output:

SCHEMA IS VALID
enum Size {
  MEDIUM
  LARGE
}

interface Displayable {
  thumbnailUrl(size: Size! = MEDIUM): String!
}

type Album implements Displayable {
  thumbnailUrl(size: Size!): String!
}

type Query {
  displayable: Displayable
}

OPERATION IS VALID
{
  displayable {
    thumbnailUrl
  }
}

OPERATION RESULT
{
  errors: [
    GraphQLError: Argument "size" of required type "Size!" was not provided.
        at getArgumentValues (node_modules/graphql/execution/values.js:192:15)
        at executeField (node_modules/graphql/execution/execute.js:483:48)
        at executeFields (node_modules/graphql/execution/execute.js:414:22)
        at completeObjectValue (node_modules/graphql/execution/execute.js:925:10)
        at completeAbstractValue (node_modules/graphql/execution/execute.js:806:10)
        at completeValue (node_modules/graphql/execution/execute.js:635:12)
        at executeField (node_modules/graphql/execution/execute.js:500:19)
        at executeFields (node_modules/graphql/execution/execute.js:414:22)
        at executeOperation (node_modules/graphql/execution/execute.js:344:14)
        at execute (node_modules/graphql/execution/execute.js:136:20) {
      path: [Array],
      locations: [Array],
      extensions: [Object: null prototype] {}
    }
  ],
  data: [Object: null prototype] { displayable: null }
}

[benjie]

This isn't quite an RFC yet, but it should be.

[Keweiqu]

@jjergus this might be interesting to you.

[benjie]

Here are the options I see:

1. Forbid interface fields from having default values on their arguments.
2. If an interface field argument has a default value, each implementation must have the exact same default value.
3. If an interface field argument has a default value, each implementation must have a default value, but it need not be the same.

(Note: additional thought may need to be put into nullable arguments with a default = null).

I'm currently in favour of option (3) since it's non-breaking. The fact that the specific default value is not enforced to be identical across implementations is distasteful, but we can encourage people to ensure they're the same with a SHOULD (rather than MUST) rule. This can be enforced through tools like graphql-eslint.

(1) is a breaking change because querying the field on an interface would now always require the argument to be specified, which would break existing queries that do not specify the argument here.

(2) is arguably breaking because the default value used in these positions may have to change, which could change the meaning of existing queries (though they would remain valid). This is relatively minor, but it's just enough for me to favour option (3) over option (2) which was my initial favourite.

(3) is non-breaking from a client perspective (the only perspective that the spec generally cares about):

• With this new schema validation rule, a schema that has a default on an interface field argument but no default on an implementation would become invalid (and thus could not be deployed).
• Schema designers would need to fix the schema by applying default values to each affected implementation's field's argument.
• From a client perspective, non-nullable arguments (and nullable arguments for that matter) gaining a default is a non-breaking change - no existing queries would be invalidated or have their meaning impacted by this change.

I propose we move ahead with option 3.

[yaacovCR]

Option 1 may not be a breaking change from a client perspective => because we would in tandem change the validation rule to the correct form, which would make sure that an argument was included unless every possible runtime type has a default.

A query which was not specifying an argument would have failed previously at runtime if defaults were not specified for all runtime types, but would validate and succeed at runtime if that indeed was the case.

I think!

[yaacovCR]

Adding a new object type that implements the interface without a default would be possible and a breaking change which would be undesirable, but that’s currently the case. Options 2 and 3 fix that, which is a real advantage.

[benjie]

Consider this schema:

interface I {
  field(int: Int! = 1): Int
}

type T implements I {
  field(int: Int! = 1): Int
}

type Query {
  i: I
}

and this query:

{
  i {
    field
  }
}

I believe both of these are currently valid?

If we adopt option 1, the interface would lose the default value and become:

interface I {
  field(int: Int!): Int
}

Without your extra "make sure that an argument was included unless every possible runtime type has a default" rule, the query would now be invalid => breaking change. (I think we agree on this.)

However, your proposed rule is also not sufficient because there is nothing within the schema that would enforce this to continue to hold for types newly added to the interface. Consider that we add a new type, U:

type U implements I {
  field(int: Int!): Int
}

U is a valid implementation of I, and thus this remains a valid schema. However, our query is now invalid, since it's no longer the case that "every possible runtime type has a default".

To make your suggested additional rule work, we'd have to make an additional schema validation rule "if any implementation of an interface has a default value for one of the interface field's arguments, then all implementations of the interface must have a default value for that field's argument"; and that's a really awkward rule - the kind of thing the interface itself should enforce, by expressing the presence of the default value there, which then moves from being option (1) to being option (3).

[jjergus]

It's sometimes useful to have an argument that's non-nullable but at the same time optional (clients can omit it). Option (1) would make this impossible.

It is a bit weird that the only way to declare such argument on an interface is to give it a meaningless default value (it doesn't matter what the default value is, just that it's there), but I don't know any better option.

[yaacovCR]

Could we add an option 4:

Interfaces would not be allowed to specify default values for field arguments, but could be annotated with a directive that would enforce that all implementing types provide default values.

This would avoid having the actual value of the default value for field arguments for interfaces not being meaningful…

[benjie]

I think option three with the strong recommendation that they pretend it’s option 2 is fine; that way tooling can more easily enforce it. It’s also the least breaking for schema authors: people already following the option 2 pattern (which I imagine would be most people who have an interface field like this, otherwise we would have heard about it sooner?) wouldn’t need to do anything. I think most people using SDL copy/paste the interface fields into the implementation; and code first people probably already have this enforced via their programming language.

[martinbonnin]

My vote goes to option 2., anything else would be very suprising IMO:

• I have an interface
• I'm looking at the documentation of a field
• The default value is fine by me so I do not pass an argument
• The actual runtime value is different from the one in the doc <-- surprise!

It's breaking but as @benjie said, the fact that we're only hearing about this now is signal that the blast radius of the breaking change should be relatively small IMO.

most people using SDL copy/paste the interface fields into the implementation

Linking #533 about allowing to omit inherited fields from implementations. That would alleviate some of the possible pain in having to copy/paste fields. But also having to keep default values in sync.

[benjie]

Just to be clear: option 2 could change the meaning of existing queries, option 3 would not. I think option 3 with a "SHOULD" for having the values match is sufficient to make it non-breaking whilst also encouraging pretty much everyone to experience option 2 from now on.

[martinbonnin]

@benjie fair enough 👍 . Always the usual compatibility vs simplicity tradeoff.

Did you have anything in mind already how we can encourage the users? Warnings while assembling the schema maybe? Or linters?

[benjie]

Honestly it depends how much we want to encourage it; but right now I'm thinking we forbid it with a schema lint rule in GraphQL.js and then let you pass a flag to disable that rule (something along the lines of allowInterfaceFieldArgumentDefaultValuesToDiffer). This could be factored into error messages:

The default value for `Type.field(arg:)`, `VALUE_1` differs to that of the interface `Interface.field(arg:)`, `VALUE_2`. This is not recommended, you should change the argument so that its default matches that of the interface. If this was deliberate (or a legacy schema that you cannot fix), you can disable this recommendation by configuring your GraphQL schema with `allowInterfaceFieldArgumentDefaultValuesToDiffer: true`.

[martinbonnin]

Oh so it'd be opt-out, I like that 👍