nautilus/gateway

When merging type with different set of interfaces, distinct interfaces are not always added.

berend opened this issue · 3 comments

Given the two schema A and B, where B has more interfaces on a type, the order of merging changes the outcome.

Example:

A

schema {
    query: Query
}

type Query {
    node(id: ID!): Node
    Thing(id: ID!): Thing
}


interface Node {
    id: ID!
}


type Thing implements Node {
    id: ID!
    foo: String!
}

B

schema {
    query: Query
}

type Query {
    node(id: ID!): Node
    Thing(id: ID!): Thing
}


interface Node {
    id: ID!
}

type Thing implements Meta & Node {
    id: ID!
    bar: String!
    created_at: String!
}

interface Meta {
    created_at: String!
}

If we now merge this with A first like so:

	combined, err = New([]*graphql.RemoteSchema{
		{Schema: A, URL: "url1"},
		{Schema: B, URL: "url2"},
	})

then combined.schema.Types.[Thing].Interfaces is ["Node", "Meta"]. If we merge with B first

	combined, err = New([]*graphql.RemoteSchema{
		{Schema: B, URL: "url2"},
		{Schema: A, URL: "url1"},
	})

then combined.schema.Types.[Thing].Interfaces is ["Node"].

This may cause issues, when doing fragments on the resulting type. It looks like, Schema A is overwriting Schema B here, instead I would expect a merge here, also order is not relevant.

I have to investigate a bit more, but overwrites should never occur, because:

  • they have the same amount of interfaces (or fields,...) -> nothing to do
  • they have additional ones -> should be added
  • they have conflicting ones -> should return an error

I will try to fix that with another PR, but I am also wondering on your opinion on this one here, @AlecAivazis

Hey @berend! Thanks for bringing this up. I agree that we should never be overwriting interfaces and i think i agree with the three cases you outlined.

If you have the time to throw together a PR, id love to review it

Hi @AlecAivazis, just to let you know: I still plan to fix this, but christmas is the busiest time here, I have not yet found the time.

Thanks for opening @berend! This is fixed in v0.2.8