Skip to content
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

Forbid to implement same same interface twice #262

Merged
merged 1 commit into from
Apr 27, 2017

Conversation

IvanGoncharov
Copy link
Member

Both spec and reference implementation allow you to implement the same interface twice:

type Cat implements Pet, Pet {
  name: String
  currentlyPurring: Boolean
}

Moreover, I've discovered this issue in production GitHub's GraphQL API
image

@wincent
Copy link
Contributor

wincent commented Feb 13, 2017

On the surface that does seem like something that shouldn't be allowed.

On your wording:

An object can not implement particular interface more than once.

I think there are two aspects to "implementing" an interface here that we should be explicit and tease apart here:

  1. Declaring that a type implements an interface.
  2. Actually implementing that interface, by implementing the fields declared in the interface.

The other rules in that section are enough to prevent the object type from doubly "implementing" an interface in the second sense already:

The fields of an Object type must have unique names within that Object type; no two fields may share the same name.

So in the interests of clarity here I'd prefer wording that explicitly referred to sense "1.". Something like:

An object type may declare that it implements 1 or more unique interfaces

Or, we could move that down to the "Interfaces" section where the implements keyword is first shown/demonstrated.

@IvanGoncharov
Copy link
Member Author

@wincent Agree. + I changed the order of rules.

Or, we could move that down to the "Interfaces" section where the implements keyword is first shown/demonstrated.

I think rules about validating 1 and 2 on your list should live together.

@wincent
Copy link
Contributor

wincent commented Feb 14, 2017

I like the new approach.

@leebyron
Copy link
Collaborator

Great! Do you also have a PR for applying this to the reference implementation?

@IvanGoncharov
Copy link
Member Author

@leebyron Not at the moment but I can start working on it.

IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Feb 26, 2017
IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Feb 26, 2017
IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Mar 6, 2017
IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Mar 6, 2017
IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Apr 17, 2017
@leebyron leebyron merged commit a05a6cf into graphql:master Apr 27, 2017
IvanGoncharov added a commit to IvanGoncharov/graphql that referenced this pull request Jun 17, 2017
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.

4 participants