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

federation gateway should throw on type's conflicts #826

Open
Eomm opened this issue Jul 13, 2022 · 6 comments · May be fixed by #830
Open

federation gateway should throw on type's conflicts #826

Eomm opened this issue Jul 13, 2022 · 6 comments · May be fixed by #830
Labels
enhancement New feature or request

Comments

@Eomm
Copy link
Contributor

Eomm commented Jul 13, 2022

Given the following schemas:

Node 1

    extend type Query {
      getOrder: Order
    }

    type Order @key(fields: "id") {
      id: ID!
      name: String
    }

Node 2

  type Order @key(fields: "id") {
    id: ID!
    surname: String
  }

The gateway starts correctly instead of throwing.

This is a clear developer's error, as the Node 2's schema should use the extends keyword:

  extends type Order {
    surname: String
  }

In this case, Mercurius should throw an error when:

  • there is a naming conflict without the extends keyword
  • the extends entity must not use the ID! property value

In both the above cases, Apollo server trigger this error:

This data graph is missing a valid configuration. Field "Order.id" can only be defined once.

There can be only one type named "Order".

This would help developers to spot these schema conflicts at first gateway run.

@mcollina
Copy link
Collaborator

Thanks for reporting! Would you like to send a Pull Request to address this issue? Remember to add unit tests.

@mcollina mcollina added the enhancement New feature or request label Jul 14, 2022
@JamesLMilner
Copy link

JamesLMilner commented Jul 25, 2022

I have been looking at this today and have some code that works for the case above. However I'm trying to get a better understanding of what the defined behaviour should be for when federated service schemas come together more broadly (for example if it is not an entity type as above). Is there any specification or guidance on how federated service schemas should behave when they come together? I have been looking at the Apollo v1 documentation - is this what we want to follow? It would be great to have some more test cases to help write this feature.

@jonnydgreen
Copy link
Contributor

Is there any specification or guidance on how federated service schemas should behave when they come together?

Good question! This is actually being fleshed out as we speak in the GraphQL Composite Schemas Working Group: https://github.com/graphql/composite-schemas-wg . Unfortunately, there is not much behavioural stuff yet so that will be available down the line - although I don't know how long this will be or what it will look like. E.g. it was discussed in the inaugural WG meet the other week about the fact that there are lots of valid ways to combine these schemas and how one would resolve a conflict such as this.

In the meantime, following what Apollo has done is not a bad shout as I understand that Mercurius Federation was originally based on Apollo Federation?

@mcollina
Copy link
Collaborator

Indeed

@mcollina
Copy link
Collaborator

note that Mercurius fully support @external:

.

@JamesLMilner
Copy link

JamesLMilner commented Jul 26, 2022

Just to give an example of some ambiguity here, the example given in the opening post for the first service schemas uses an entity type like so:

   type Order @key(fields: "id") {
      id: ID!
      name: String
    }

For extending entities in Apollo v1, the canonical way is as so:

extend type Order @key(fields: "id") {
  id: String! @external
  surname: String
}

I believe the following attempt to extend (in the initial example) should probably throw an error because it's not the defined way (at least as defined by Apollo v1) to extend an entity (here the key directive is not used so it is not even an entity definition):

extend type Order {
    surname: String
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
4 participants