Disallow non-breakable chains of circular references in Input Objects

[spawnia]

This was already discussed in Issue #189

Input Objects are allowed to reference other Input Objects. A circular reference occurs
when an Input Object references itself either directly or through subordinated Input Objects.

Circular references are generally allowed, however they may not be defined as an
unbroken chain of Non-Null fields. Such Input Objects are invalid, because there
is no way to provide a legal value for them.

The following examples are allowed:

input Example {
  self: Example
  value: String
}

This is fine because a value for self may simply be omitted from the arguments.

input Example {
  self: [Example!]!
  value: String
}

This also works as self can just contain an empty list.

The following examples are invalid:

input Example {
  value: String
  self: Example!
}

input First {
  second: Second!
  value: String
}

input Second {
  first: First!
  value: String
}

The following example shows why no possible value can be provided:

{
  someField(input: {
    value: "val"
    # self is required
    self: {
      value: "nextval"
      # self is still required
      self: {
        # We would have to recurse down infinitely
        ...
      }
    }
  })
}

[spawnia]

graphql/graphql-js#1352 contains tests that specify how the reference implementation should behave.

[IvanGoncharov]

This Input definition does not really make much sense, as the only use might be somehow conveying a certain nesting depth?

It's hard to write something like this by accident so I don't think it's worth to complicate spec and all implementations to prevent such constructs.
Plus technically this construct is valid it just happens to be useless in 99.999% of cases.
So I think it may be worth to add this check in 3rd-party GraphQL linters but not in the spec itself.

For me it's simular to how a = a is valid in JS, Flow and TS but you can configure ESLint to reject it:
https://eslint.org/docs/rules/no-self-assign

[spawnia]

Good point. I also just thought of a use case where something like this might occur. When evolving a schema, there may be a period of time inbetween changes where one field is removed but later added.

Having such a rule in place would require major structural changes, so while it does not make sense often - it definitely should be allowed. Thank you for helping me to figure this out.

What is your opinion on the PR? Anything you would change?

[IvanGoncharov]

What is your opinion on the PR? Anything you would change?

I think you need to split your text into two parts, one explaining "self-referencing" and second is an additional step in Type Validation section.
For example here is how it's done for field types:

• Explanation: https://github.com/facebook/graphql/blame/master/spec/Section%203%20--%20Type%20System.md#L1274-L1291
• Validation step: https://github.com/facebook/graphql/blame/master/spec/Section%203%20--%20Type%20System.md#L1366

Given the number of examples maybe it worth to have separate 'self-referencing' section.

[leebyron]

Just to make sure I understand correctly, the example you posted should be possible:

input Hello {
  world: Hello
}

Since by default values can be null. But this would become invalid:

input Hello {
  world: Hello!
}

Since there cannot exist a value which satisfies it?

I think this is an interesting and potentially useful validation rule, but I sort of agree with Ivan that part of what we should weigh here is if this solves enough of a salient pain point for developers that it is worth the additional complexity in the spec.

Perhaps a good way to understand this question is to gauge how difficult actually implementing this validation rule in graphql-js would be. Is that something you're willing to try out, @spawnia ?

[spawnia]

I think we all agree the first example is valid, which is why it does not need to be mentioned in the spec. Linters may give hints but it should not throw a validation error.
As a result of the discussion here, i did not put it in the proposed changes and i am not planning to consider it for validation in the reference implementation.

What i do actually propose to change is some clear rules and examples that define and show how the second example is invalid. Since there is no way to provide a legal value for it, it definitely is an invalid definition.

In my opinion, the specification should be as clear as possible about such corner cases and specify exactly what is and is not a valid schema definition. It is then up to the implementations to follow suit.

Leaving the specification incomplete or ambigious because some of the rules in it may be hard to implement seems dangerous to me. It may lead to a drift in the implementations, where some of them validate for this and some do not. Which one is the correct behaviour then if the specification does not mention it?

I did already submit a PR to graphql-js with test cases covering this rule. However, i am not that familiar with the inner workings of the implementation and do not feel quite confident in implementing a solution.

[IvanGoncharov]

Perhaps a good way to understand this question is to gauge how difficult actually implementing this validation rule in graphql-js would be. Is that something you're willing to try out, @spawnia ?

Based on @spawnia original PR I implemented proposed validation rule here:
graphql/graphql-js#1359

[IvanGoncharov]

Merged in graphql-js: graphql/graphql-js#1359 and planned to be released in 14.4.0.

[spawnia]

PR for the PHP implementation is on the way webonyx/graphql-php#492.

[spawnia]

Both major JS and PHP implementations have implemented this. @leebyron is there anything left for me to do?

[IvanGoncharov]

@spawnia I think the last required step would be to discuss it on the WG.
We will have it tomorrow on WG: https://github.com/graphql/graphql-wg/blob/master/agendas/2019-10-10.md#agenda
Would you be able to join?

[jbaxleyiii]

@IvanGoncharov @leebyron from what I can tell there is nothing technically blocking circular input types from being possible? Circular references are easily acomplished in many languages and may be the best representation of an actual input. Even serilization and transport is possible using different methods or transports (i.e. https://www.npmjs.com/package/arson)

To be concrete, there is no reason why this shouldn't be valid (in my mind)

input User {
  id: ID
  friend: User!
}

If I had a user who's only friend was themselves

const user = { id: "1" }
user.friend = user

I should be able to send this as a valid input object

[spawnia]

@jbaxleyiii i can understand the logic behind your use case, it makes sense.

As of now, there is no way to provide a legal value for the input User you defined, using just a literal GraphQL query. The GraphQL query language lacks support for reference types.

It is certainly possible to represent such a data structure in most programming languages and possible some transport formats. That would force you to use variables whenever using that query. That might become problematic when, for example, you want to write a unit test or test the query through GraphiQL.

We could discuss adding reference types to GraphQL in a seperate RFC. This RFC merely clarifies what is possible with GraphQL as it is now, without actually changing the language.

[jbaxleyiii]

As of now, there is no way to provide a legal value for the input User you defined, using just a literal GraphQL query.

This is also true for

input User {
  id: ID
  friend: User
}

GraphQL can't model recursive types (required or possible) in its current form. Due to that, I think this change is half in the door of making recursion forbidden and half in the door of making it possible but actually solving neither of the two.

[spawnia]

If the type is nullable, you can do this:

{
  user(input: {
    id: "1"
    friend: {
      id: "2"
    }
  }) {
    id
  }
}

[jbaxleyiii]

Right, but you can't model all possible values there?

[IvanGoncharov]

@spawnia Based on discussion during last WG the last point we need to address is the complexity of implementation: https://github.com/graphql/graphql-wg/blob/master/notes/2019-10-10.md#disallow-non-breakable-chains-of-circular-references-in-input-objects-benedikt-5m
Would you be able to make a presentation of JS or PHP implementation on WG tomorrow?

[spawnia]

Hey @IvanGoncharov thanks for reminding me. I plan to participate, yes.

Here is a quick objective analysis in terms of metrics:

LOC

Raw lines of code, including comments and whitespace

-	PHP	JS
Implementation	104	50
Tests	138	114

Cyclomatic Complexity

In the JS implementation, the validation function has a cyclomatic complexity of 6 and is called in exactly one place.

[spawnia]

I want to add a few thoughts i developed during the last month.

Implementors are not forced to add this validation

Just because it is in the spec does not mean that a validation that enforces this rule must be implemented. Implementors can choose to ignore it, if they don't want the extra complexity or spend time on implementing the check.

Assumptions

There are a few assumptions underlying this rule, which naturally lead to it being a requirement.

Schemas must be queryable

GraphQL schemas are meant to be used for querying and must be functional for such. GraphQL is not meant solely to express a type system and offer introspection capabilities.

The sole purpose of a non-breakable self-referencing input object would be to be introspected upon. It could not ever be used in an actual query.

GraphQL literals are first class

All GraphQL operations must be able to be expressed by using literal GraphQL query language. Features that work only when variables are involved are not valid.

While there may be ways to express non-breakable self-referencing input object in most programming languages and some transport formats, it is not possible to do so using only the GraphQL query language itself and the literals it provides.

[spawnia]

The pull bot nuked my master branch, this continues in #701