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

Different reference types at the same path are not correctly handled by the gateway #751

Closed
jonnydgreen opened this issue Mar 9, 2022 · 2 comments · Fixed by #754
Closed

Comments

@jonnydgreen
Copy link
Contributor

Howdy! I found an issue with the gateway resolver cache resolution where different reference types at the same path are not correctly handled by the gateway. This happens when we define a union list response where each union type contains different reference types at the same field path. Consider the following example as an illustration:

Problem

Consider the following message service schema:

Message service

type Query @extends {
  getMessages: [Message!]!
}

union Message = EmailMessage | TextMessage

type TextMessage {
  id: ID!
  message: String!
  recipients: [TextMessageUser!]!
}

type EmailMessage {
  id: ID!
  message: String!
  header: String!
  recipients: [EmailMessageUser!]!
}

type TextMessageUser @extends @key(fields: "id") {
  id: ID! @external
}

type EmailMessageUser @extends @key(fields: "id") {
  id: ID! @external
}

With the resolvers:

const resolvers = {
  Query: {
    getMessages: () => {
      return [
        {
          id: '1',
          message: 'Text Message.',
          recipients: [{ id: 'u1' }]
        },
        {
          id: '2',
          header: 'Email Header',
          message: 'Email Message.',
          recipients: [{ id: 'u2' }]
        }
      ]
    }
  },
  Message: {
    resolveType: (message) => {
      if ('header' in message) {
        return 'EmailMessage'
      }
      return 'TextMessage'
    }
  }
}

Consider the following user service schema:

User service

type TextMessageUser @key(fields: "id") {
  id: ID!
  name: String!
  fields: [String!]!
}

type EmailMessageUser @key(fields: "id") {
  id: ID!
  name: String!
  fields: [String!]!
}

With the resolvers:

const resolvers = {
  TextMessageUser: {
    __resolveReference: (user) => {
      return users[user.id]
    }
  },
  EmailMessageUser: {
    __resolveReference: (user) => {
      return users[user.id]
    }
  }
}

Replication

If we setup the gateway to federate the above service and we make the following query to the gateway:

query {
  getMessages {
    ... on TextMessage {
      id
      message
      recipients {
        name
        fields
      }
    }
    ... on EmailMessage {
      id
      header
      message
      recipients {
        name
        fields
      }
    }
  }
}

The system should use the above resolvers to return data such that each union type recipients list is correctly populated. However, we get the following error:

{
  "errors": [
    {
      "message": "Cannot return null for non-nullable field EmailMessageUser.name.",
      "locations": [
         {
          "line": 17,
          "column": 13
        }
      ],
      "path":  [
        "getMessages",
        1,
        "recipients",
        0,
        "name"
      ]
    }
  ],
  "data": null
}

I had a look at this and found the line causing the issue. Essentially the internal Mercurius resolver key for the LRU gateway resolvers cache is not unique enough if just using the normalised path to distinguish between different reference types at the same path. In this scenario, if we are returning different types, Mercurius still considers the resolution of the two different types to be the same and marks the second reference type as already cached when it hasn't been resolved yet. This is fine for most situations because in the vast majority of use cases, there is usually only one type per path; but here we have 2 for the same schema path.

I confirmed that this was definitely the cause by defining a test with the above scenario and found that it passes if I manually turn off the resolver cache (i.e. setting cached to undefined for all requests for illustration purposes only).

Solution

I think the solution is fairly simple and should have no impact: improve the uniqueness of the resolver key by adding the named type as a suffix.

I.e. instead of: queryId.replace(/\d/g, '_IDX_'), we define:

const resolverKey = `${queryId.replace(/\d/g, '_IDX_')}.${type.toString()}`

This should fix the problem and have no impact of existing functionality as far as I can tell. If you agree, I'll get started on this ASAP! :)

Caveat

There is a small caveat in that I was wondering if we could release this fix for both v8 and v9 versions of Mercurius? We are currently using both versions and it would be great to get this fixed in both if that's possible! On this, my plan would be to:

  • Submit a PR to the default branch as normal for v9
  • Branch off the v8.12.0 tag (with a branch called v8.x) and submit a PR with the change for this branch

What do you think?

@mcollina
Copy link
Collaborator

mcollina commented Mar 9, 2022

Go for it!

In the meanwhile, would you like to set up the backport action? https://github.com/fastify/fastify/blob/main/.github/workflows/backport.yml

So that we can automate the backport.

@jonnydgreen
Copy link
Contributor Author

Go for it!

Cool - will get something together today!

In the meanwhile, would you like to set up the backport action?

Absolutely!

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 a pull request may close this issue.

2 participants