Schema composition with directives that use complex argument types

[azaxarov]

Consider the following type definitions that will be used to build an Apollo Federation schema:

directive @something(rules: [Rule!]!) on OBJECT | FIELD_DEFINITION

enum Number {
  one
  two
}

input Rule {
  numbers: [Number!]!
}

type Something {
  id: ID!
}

extend type Query {
  getSomething(id: ID!): Something!
    @something(rules: [{ numbers: [one, two] }])
}

A single directive called @something is declared, that takes in a non-nullable list of GraphQL input object types. This directive can then be used on objects and field definitions to perform some task. An example of a similar directive can be found here: https://docs.amplify.aws/cli/graphql-transformer/auth#auth.

The problem arises during composition of the schema, namely that input type Rule is being registered twice, leading to this error:

graphql-js/src/type/schema.js

Line 223 in 00eab30

`Schema must contain uniquely named types but contains multiple types named "${typeName}".`,

.

From what it seems, type property name is used to uniquely identify any given type in the schema, as evidenced by:

graphql-js/src/type/schema.js

Line 216 in 00eab30

const typeName = namedType.name;

graphql-js/src/type/schema.js

Line 221 in 00eab30

if (this._typeMap[typeName] !== undefined) {

During schema composition, all declared types are collected in a set using the following helper function:

graphql-js/src/type/schema.js

Line 403 in 00eab30

function collectReferencedTypes(

Inside that function, there is a check that should prevent duplicate types from being added to the set of all referenced types:

graphql-js/src/type/schema.js

Line 409 in 00eab30

if (!typeSet.has(namedType)) {

Given this information and understanding the mechanics of Set class in JS, there is a chance that !typeSet.has(namedType)will result in a false positive if two objects have different references. As a consequence, some types may be registered twice, as in the example above with Rule type.

This problem appears to be quite simple to fix. There are two approaches:

• Use Set class from immutable-js, which I would not recommend (see: https://stackoverflow.com/a/56353815)

• Change the following check !typeSet.has(namedType) to something like ![...typeSet].find(({ name }) => name === namedType.name) (see the following line:

  graphql-js/src/type/schema.js

  Line 409 in 00eab30

  if (!typeSet.has(namedType)) {

  )

I published an example repository to illustrate the problem: https://github.com/azaxarov/graphql-service-with-complex-directive-parameter

So far, I see no obvious side-effects of implementing a fix using approach nr. 2.

[yaacovCR]

We get around this when schema stitching by rewiring all types, which is not possible with all schemas. (See possibly related comment apollographql/apollo-server#4237 (comment))

I believe the rewiring would still be necessary even if this duplicate check would be changed to make sure all fields reference types actually in the type map.

But I am not sure...

[azaxarov]

@yaacovCR
As far as I can see, the two issues appear unrelated to each other. The issue I described happens in a federated service, thus long before Apollo Gateway merges federated service schemas.

[yaacovCR]

Why in the federated service are the object types with the same name not actually identical?

Sorry, just curious...

[yaacovCR]

Depending on what's going on exactly, you may be able to use the mapSchema or the underlying rewire types functions from graphql-tools to rewire the types so that they are identical.

I may be completely misunderstanding the issue, of course.

[azaxarov]

@yaacovCR That's a good question. I'll try my best to answer it, but don't take my word for it.

From what it seems, when federated service creates its schema, it makes several trips (6) into GraphQLSchema constructor, with each pass adding more types to the set of all referenced types. During the initial passes, input object type from my example Rule gets declared. However, when the directive is being parsed during one of the later passes, it also generates an internal representation of that same Rule type. They have the same structure, but different object reference as far as JS is concerned. This is the reason .has check fails.

I should not need to use mapScema in the service, as it should be handled correctly in buildFederatedSchema from @apollo/federation.

[yaacovCR]

So, as reasoned above, this is a bug in Apollo federated service building?

Again, I'm not confident enough in my knowledge of the upstream GraphQL Js source code, but are you sure that if you only merge one type everything will still work out? Will you be using the correct one type everywhere? Or whatever still be hidden references to the shadow type?

[azaxarov]

From what I can tell this is a bug in graphql (this repository), that takes place when Apollo service constructs its schema using buildFederatedSchema, which in turn uses graphql.

I've tested this locally by implementing the fix I suggested in the node_modules of my project and the service starts up just fine after the fix.

This is not a problem with just this type I have in the example, but it applies to all types in the schema. Using Set.has is not the best way to filter out already defined types. I am unsure, but my guess is that shadow types should not be a problem, because they will never be added to the schema.

[yaacovCR]

Not sure I agree where the bug is. Seems like similar types with identical names are being created unnecessarily/inappropriately.

[benjie]

I agree with @yaacovCR; this doesn't seem to be a bug in GraphQL.js to me. It seems like you're saying buildFederatedSchema is creating duplicate types which is the problem - duplicate types should definitely throw an error in GraphQL.js - I suggest you propose a fix to buildFederatedSchema.

[yaacovCR]

You may still be able to utilize workaround of type rewiring provided by graphql-tools, but I can't guarantee that won't further impinge upon whatever stored types that buildFederatedSchema requires as exact references....

[azaxarov]

@benjie @yaacovCR

Thanks guys for the feedback. I went back to further investigate the issue and I believe I was able to identify it. I opened the following issue: apollographql/apollo-tooling#2162

I hope Apollo team will be able to shed more light on the issue.

That being said, I am still convinced that using JS object references to check for equality is not the best approach. Firstly, there is no way to customise equals method is JS as in many other programming languages. And secondly, if name property is used to uniquely identify a type, then it should be used to perform equality checks.

[IvanGoncharov]

@azaxarov every time this issue brought up is due to bugs in libraries using graphql-js.
The problem here is that we can't deeply compare non-trivial objects two types can have differences hidden in unexpected places (e.g. extensions or custom class inherited from GraphQLInputObjectType).
So our position is to be safe-side instead of silently ignoring such bugs.

That said if you think such checks restricting valid usage scenarios please feel free to open an issue and describe a such scenario.