Skip to content

RFC: Return type overlap validation #162

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

Merged
merged 1 commit into from
Apr 7, 2016
Merged

Conversation

leebyron
Copy link
Collaborator

@leebyron leebyron commented Apr 6, 2016

This alters the "Overlapping Fields Can Be Merged" validation rule to better express return type validation issues.

The existing rule has presented some false-positives in some schema where interfaces with co-variant types are commonly used. The "same return type" check doesn't remove any ambiguity.

Instead what that "same return type" check is attempting to prevent is spreading two fragments (or inline fragments) which have fields with return types where ambiguity would be introduced in the response.

In order to curb false-positives, we later changed this rule such that if two fields were known to never apply simultaneously, then we would skip the remainder of the rule.

{
  ... on Person {
    foo: fullName
  }
  ... on Pet {
    foo: petName
  }
}

However this can introduce false-negatives!

{
  ... on Person {
    foo: birthday {
      bar: year
    }
  }
  ... on Business {
    foo: location {
      bar: street
    }
  }
}

In the above example, data.foo.bar could be of type Int or type String, it's ambiguous!

This differing return type breaks some client model implementations (Fragment models) in addition to just being confusing.

For example this invalid query:

fragment A on Type {
  field: someIntField
}

fragment B on Type {
  ...A
  field: someStringField
}

Might produce the following illegal Java artifacts:

interface A {
  int getField()
}

interface B implements A {
  string getField()
}

This changes the "Overlapping Fields Can Be Merged" rule to avoid this ambiguity by comparing response shapes rather than return types.

leebyron added a commit to graphql/graphql-js that referenced this pull request Apr 6, 2016
Implements graphql/graphql-spec#162

This alters the "Overlapping Fields Can Be Merged" validation rule to better express return type validation issues.

The existing rule has presented some false-positives in some schema where interfaces with co-variant types are commonly used. The "same return type" check doesn't remove any ambiguity.

Instead what that "same return type" check is attempting to prevent is spreading two fragments (or inline fragments) which have fields with return types where ambiguity would be introduced in the response.

In order to curb false-positives, we later changed this rule such that if two fields were known to never apply simultaneously, then we would skip the remainder of the rule.

```
{
  ... on Person {
    foo: fullName
  }
  ... on Pet {
    foo: petName
  }
}
```

However this can introduce false-negatives!

```
{
  ... on Person {
    foo: birthday {
      bar: year
    }
  }
  ... on Business {
    foo: location {
      bar: street
    }
  }
}
```

In the above example, `data.foo.bar` could be of type `Int` or type `String`, it's ambiguous!

This differing return type breaks some client model implementations (Fragment models) in addition to just being confusing.
leebyron added a commit to graphql/graphql-js that referenced this pull request Apr 6, 2016
Implements graphql/graphql-spec#162

This alters the "Overlapping Fields Can Be Merged" validation rule to better express return type validation issues.

The existing rule has presented some false-positives in some schema where interfaces with co-variant types are commonly used. The "same return type" check doesn't remove any ambiguity.

Instead what that "same return type" check is attempting to prevent is spreading two fragments (or inline fragments) which have fields with return types where ambiguity would be introduced in the response.

In order to curb false-positives, we later changed this rule such that if two fields were known to never apply simultaneously, then we would skip the remainder of the rule.

```
{
  ... on Person {
    foo: fullName
  }
  ... on Pet {
    foo: petName
  }
}
```

However this can introduce false-negatives!

```
{
  ... on Person {
    foo: birthday {
      bar: year
    }
  }
  ... on Business {
    foo: location {
      bar: street
    }
  }
}
```

In the above example, `data.foo.bar` could be of type `Int` or type `String`, it's ambiguous!

This differing return type breaks some client model implementations (Fragment models) in addition to just being confusing.
@leebyron leebyron force-pushed the return-type-validation branch from c9bc141 to d5d3357 Compare April 6, 2016 21:23
@jjergus
Copy link
Contributor

jjergus commented Apr 6, 2016

lgtm, I like this approach

leebyron added a commit to graphql/graphql-js that referenced this pull request Apr 6, 2016
Implements graphql/graphql-spec#162

This alters the "Overlapping Fields Can Be Merged" validation rule to better express return type validation issues.

The existing rule has presented some false-positives in some schema where interfaces with co-variant types are commonly used. The "same return type" check doesn't remove any ambiguity.

Instead what that "same return type" check is attempting to prevent is spreading two fragments (or inline fragments) which have fields with return types where ambiguity would be introduced in the response.

In order to curb false-positives, we later changed this rule such that if two fields were known to never apply simultaneously, then we would skip the remainder of the rule.

```
{
  ... on Person {
    foo: fullName
  }
  ... on Pet {
    foo: petName
  }
}
```

However this can introduce false-negatives!

```
{
  ... on Person {
    foo: birthday {
      bar: year
    }
  }
  ... on Business {
    foo: location {
      bar: street
    }
  }
}
```

In the above example, `data.foo.bar` could be of type `Int` or type `String`, it's ambiguous!

This differing return type breaks some client model implementations (Fragment models) in addition to just being confusing.
leebyron added a commit to graphql/graphql-js that referenced this pull request Apr 6, 2016
Implements graphql/graphql-spec#162

This alters the "Overlapping Fields Can Be Merged" validation rule to better express return type validation issues.

The existing rule has presented some false-positives in some schema where interfaces with co-variant types are commonly used. The "same return type" check doesn't remove any ambiguity.

Instead what that "same return type" check is attempting to prevent is spreading two fragments (or inline fragments) which have fields with return types where ambiguity would be introduced in the response.

In order to curb false-positives, we later changed this rule such that if two fields were known to never apply simultaneously, then we would skip the remainder of the rule.

```
{
  ... on Person {
    foo: fullName
  }
  ... on Pet {
    foo: petName
  }
}
```

However this can introduce false-negatives!

```
{
  ... on Person {
    foo: birthday {
      bar: year
    }
  }
  ... on Business {
    foo: location {
      bar: street
    }
  }
}
```

In the above example, `data.foo.bar` could be of type `Int` or type `String`, it's ambiguous!

This differing return type breaks some client model implementations (Fragment models) in addition to just being confusing.
This alters the "Overlapping Fields Can Be Merged" validation rule to better express return type validation issues.

The existing rule has presented some false-positives in some schema where interfaces with co-variant types are commonly used. The "same return type" check doesn't remove any ambiguity.

Instead what that "same return type" check is attempting to prevent is spreading two fragments (or inline fragments) which have fields with return types where ambiguity would be introduced in the response.

In order to curb false-positives, we later changed this rule such that if two fields were known to never apply simultaneously, then we would skip the remainder of the rule.

```
{
  ... on Person {
    foo: fullName
  }
  ... on Pet {
    foo: petName
  }
}
```

However this can introduce false-negatives!

```
{
  ... on Person {
    foo: birthday {
      bar: year
    }
  }
  ... on Business {
    foo: location {
      bar: street
    }
  }
}
```

In the above example, `data.foo.bar` could be of type `Int` or type `String`, it's ambiguous!

This differing return type breaks some client model implementations (Fragment models) in addition to just being confusing.

For example this invalid query:

```
fragment A on Type {
  field: someIntField
}

fragment B on Type {
  ...A
  field: someStringField
}
```

Might produce the following illegal Java artifacts:

```
interface A {
  int getField()
}

interface B implements A {
  string getField()
}
```
@leebyron leebyron force-pushed the return-type-validation branch from d5d3357 to d481d17 Compare April 7, 2016 05:49
leebyron added a commit to graphql/graphql-js that referenced this pull request Apr 7, 2016
Implements graphql/graphql-spec#162



This alters the "Overlapping Fields Can Be Merged" validation rule to better express return type validation issues.



The existing rule has presented some false-positives in some schema where interfaces with co-variant types are commonly used. The "same return type" check doesn't remove any ambiguity.



Instead what that "same return type" check is attempting to prevent is spreading two fragments (or inline fragments) which have fields with return types where ambiguity would be introduced in the response.



In order to curb false-positives, we later changed this rule such that if two fields were known to never apply simultaneously, then we would skip the remainder of the rule.



```

{

  ... on Person {

    foo: fullName

  }

  ... on Pet {

    foo: petName

  }

}

```



However this can introduce false-negatives!



```

{

  ... on Person {

    foo: birthday {

      bar: year

    }

  }

  ... on Business {

    foo: location {

      bar: street

    }

  }

}

```



In the above example, `data.foo.bar` could be of type `Int` or type `String`, it's ambiguous!



This differing return type breaks some client model implementations (Fragment models) in addition to just being confusing.
@leebyron leebyron merged commit 6c21794 into master Apr 7, 2016
@leebyron leebyron deleted the return-type-validation branch April 7, 2016 06:00
sogko added a commit to sogko/graphql that referenced this pull request May 31, 2016
Implements graphql/graphql-spec#162

This alters the "Overlapping Fields Can Be Merged" validation rule to better express return type validation issues.

The existing rule has presented some false-positives in some schema where interfaces with co-variant types are commonly used. The "same return type" check doesn't remove any ambiguity.

Instead what that "same return type" check is attempting to prevent is spreading two fragments (or inline fragments) which have fields with return types where ambiguity would be introduced in the response.

In order to curb false-positives, we later changed this rule such that if two fields were known to never apply simultaneously, then we would skip the remainder of the rule.

```
{
  ... on Person {
    foo: fullName
  }

  ... on Pet {
    foo: petName
  }
}

```

However this can introduce false-negatives!

```
{
  ... on Person {
    foo: birthday {
      bar: year
    }
  }

  ... on Business {
    foo: location {
      bar: street
    }
  }
}

```

In the above example, `data.foo.bar` could be of type `Int` or type `String`, it's ambiguous!

This differing return type breaks some client model implementations (Fragment models) in addition to just being confusing.

Commit:
c034de91acce10d5c06d03bd332c6ebd45e2213c [c034de9]
Parents:
ffe76c51c4
Author:
Lee Byron <[email protected]>
Date:
7 April 2016 at 2:00:31 PM SGT
Labels:
HEAD
samuel pushed a commit to sprucehealth/graphql that referenced this pull request Mar 28, 2017
Implements graphql/graphql-spec#162

This alters the "Overlapping Fields Can Be Merged" validation rule to better express return type validation issues.

The existing rule has presented some false-positives in some schema where interfaces with co-variant types are commonly used. The "same return type" check doesn't remove any ambiguity.

Instead what that "same return type" check is attempting to prevent is spreading two fragments (or inline fragments) which have fields with return types where ambiguity would be introduced in the response.

In order to curb false-positives, we later changed this rule such that if two fields were known to never apply simultaneously, then we would skip the remainder of the rule.

```
{
  ... on Person {
    foo: fullName
  }

  ... on Pet {
    foo: petName
  }
}

```

However this can introduce false-negatives!

```
{
  ... on Person {
    foo: birthday {
      bar: year
    }
  }

  ... on Business {
    foo: location {
      bar: street
    }
  }
}

```

In the above example, `data.foo.bar` could be of type `Int` or type `String`, it's ambiguous!

This differing return type breaks some client model implementations (Fragment models) in addition to just being confusing.

Commit:
c034de91acce10d5c06d03bd332c6ebd45e2213c [c034de9]
Parents:
ffe76c51c4
Author:
Lee Byron <[email protected]>
Date:
7 April 2016 at 2:00:31 PM SGT
Labels:
HEAD
mattstern31 pushed a commit to mattstern31/graphql-gqllero-repository that referenced this pull request Nov 10, 2022
Implements graphql/graphql-spec#162

This alters the "Overlapping Fields Can Be Merged" validation rule to better express return type validation issues.

The existing rule has presented some false-positives in some schema where interfaces with co-variant types are commonly used. The "same return type" check doesn't remove any ambiguity.

Instead what that "same return type" check is attempting to prevent is spreading two fragments (or inline fragments) which have fields with return types where ambiguity would be introduced in the response.

In order to curb false-positives, we later changed this rule such that if two fields were known to never apply simultaneously, then we would skip the remainder of the rule.

```
{
  ... on Person {
    foo: fullName
  }

  ... on Pet {
    foo: petName
  }
}

```

However this can introduce false-negatives!

```
{
  ... on Person {
    foo: birthday {
      bar: year
    }
  }

  ... on Business {
    foo: location {
      bar: street
    }
  }
}

```

In the above example, `data.foo.bar` could be of type `Int` or type `String`, it's ambiguous!

This differing return type breaks some client model implementations (Fragment models) in addition to just being confusing.

Commit:
c034de91acce10d5c06d03bd332c6ebd45e2213c [c034de9]
Parents:
ffe76c51c4
Author:
Lee Byron <[email protected]>
Date:
7 April 2016 at 2:00:31 PM SGT
Labels:
HEAD
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.

3 participants