fix(GraphQL): This PR allows repetition of fields inside implementing type which are present in interface and also allow to inherit field of same name of type ID from multiple interfaces.

[JatinDev543]

Fixes GRAPHQL-672
Currently, we don't allow the repetition of fields in implementing types that are in the interface. But according to GraphQl specs, we should allow them. Not allowing them can also result in unexpected behavior in some scenarios.
For example, in the below example Banana type doesn't have any fields apart from that are in the Fruit interface. And we don't allow empty types. So allowing repetitive fields solve this problem.

interface Fruit {
    id: ID!
    price: Int!
}

type Apple implements Fruit {
    id: ID!
    price: Int!
    color: String!
}

type Banana implements Fruit {
    id: ID!
    price: Int!
}

Also, currently, if we have two interfaces and a type that implements both then we don't allow a field of the same name in both of them. We are allowing it for ID type fields because it's required in some use cases and there is no problem with it as ID type field is not actually a predicate.

interface I1 {
  f1: ID!
}
interface I2 {
 f1: ID!
}
type T implements I1 & I2 {
 f2: Int!
}

---

This change is 

[netlify]

Deploy preview for dgraph-docs ready!

Built with commit 4284d8d

https://deploy-preview-7053--dgraph-docs.netlify.app

[abhimanyusinghgaur]

Got some nit-picks, overall

Reviewable status: 0 of 5 files reviewed, 7 unresolved discussions (waiting on @jatindevdg, @MichaelJCompton, and @pawanrawal)

---

graphql/schema/gqlschema.go, line 654 at r2 (raw file):

initialDefFields

---

graphql/schema/gqlschema.go, line 664 at r2 (raw file):

fieldSeen := make(map[string]string)

please add a comment that:

fieldSeen a map from field name to interface name in which the field was seen.

---

graphql/schema/gqlschema.go, line 665 at r2 (raw file):

defFields := make(map[string]int64)

add a comment about it that:

defFields is used to keep track of fields in the defn before any inherited fields are added to it.

also, it could just be map[string]bool, we don't necessarily need to keep track of the count. All we need to care about is whether the field already existed in the definition or not, if yes then only we override it with the field from the interface. If there were more than one field with the same name in the defn already, it would anyways throw an error later.

Even better if we make it a map[string]*ast.FieldDefinition, then we won't need initialDefFields.

---

graphql/schema/gqlschema.go, line 686 at r2 (raw file):

assignAstFieldDef

overrideAstFieldDef
actually, instead of this function, this one-liner should work as well:

*defn.Field.ForName(field.Name) = *field

---

graphql/schema/gqlschema.go, line 694 at r2 (raw file):

through

throw

---

graphql/schema/gqlschema_test.yml, line 2694 at r2 (raw file):

    input: |
      interface Y {
        id: ID

let's add one non-ID field as well in interface, and repeat that in type too.

---

graphql/schema/schemagen.go, line 198 at r2 (raw file):

Quoted 4 lines of code…

	gqlErr = expandSchema(doc)
	if gqlErr != nil {
		return nil, gqlerror.List{gqlErr}
	}

if gqlErr := expandSchema(doc); gqlErr != nil {
return nil, ...
}

[JatinDev543]

Reviewable status: 0 of 5 files reviewed, 7 unresolved discussions (waiting on @abhimanyusinghgaur, @jatindevdg, @MichaelJCompton, and @pawanrawal)

---

graphql/schema/gqlschema.go, line 664 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

fieldSeen := make(map[string]string)

please add a comment that:

fieldSeen a map from field name to interface name in which the field was seen.

done.

---

graphql/schema/gqlschema.go, line 665 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

defFields := make(map[string]int64)

add a comment about it that:

defFields is used to keep track of fields in the defn before any inherited fields are added to it.

also, it could just be map[string]bool, we don't necessarily need to keep track of the count. All we need to care about is whether the field already existed in the definition or not, if yes then only we override it with the field from the interface. If there were more than one field with the same name in the defn already, it would anyways throw an error later.

Even better if we make it a map[string]*ast.FieldDefinition, then we won't need initialDefFields.

yeah, but that will result in different errors when we have the same fields in a type but the order is different. Because we overwrite only the first occurrence of the field. So, better to bypass this check for duplicate fields and let them result in error later validation.

---

graphql/schema/gqlschema.go, line 686 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

assignAstFieldDef

overrideAstFieldDef
actually, instead of this function, this one-liner should work as well:

*defn.Field.ForName(field.Name) = *field

changed.

---

graphql/schema/gqlschema.go, line 694 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

through

throw

changed.

---

graphql/schema/gqlschema_test.yml, line 2694 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

let's add one non-ID field as well in interface, and repeat that in type too.

done.

---

graphql/schema/schemagen.go, line 198 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

	gqlErr = expandSchema(doc)
	if gqlErr != nil {
		return nil, gqlerror.List{gqlErr}
	}

if gqlErr := expandSchema(doc); gqlErr != nil {
return nil, ...
}

changed.