Gatsby: runQuery filter in custom resolver breaks other queries (race condition?)

[aaronadamsCA]

I define a custom relationship from neighbourhoods to stores using createSchemaCustomization:

gatsby-node.js

exports.createSchemaCustomization = ({ actions, schema }) => {
  const { createTypes } = actions
  const { buildObjectType } = schema

  createTypes([
    buildObjectType({
      name: "SanityNeighbourhood",
      interfaces: ["Node"],
      fields: {
        stores: {
          type: "[SanityStore]",
          resolve: async ({ id }, args, { nodeModel }) =>
            await nodeModel.runQuery({
              query: {
                filter: { neighbourhoods: { elemMatch: { id: { eq: id } } } },
              },
              type: "SanityStore",
              firstOnly: false,
            }),
        },
      },
    }),
  ])
}

Then I create a page for each neighbourhood:

gatsby-node.js

exports.createPages = async ({ graphql, actions }) => {
  const { createPage } = actions

  const { data, errors } = await graphql(`
    query CreatePages {
      neighbourhoods: allSanityNeighbourhood {
        nodes {
          slug {
            current
          }
        }
      }
    }
  `)

  if (errors) {
    throw errors
  }

  data.neighbourhoods.nodes.forEach(({ slug: { current: slug } }) => {
    createPage({
      path: slug,
      component: require.resolve(`./src/templates/neighbourhood.js`),
      context: { slug },
    })
  })
}

My neighbourhood template calls my custom resolver to retrieve a list of stores for each neighbourhood:

src/templates/neighbourhood.js

export const query = graphql`
  query NeighbourhoodTemplate($slug: String!) {
    sanityNeighbourhood(slug: { current: { eq: $slug } }) {
      stores {
        id
      }
    }
  }
`

So far, everything works.

Now, on my home page, if I try to grab a list of stores in a particular neighbourhood:

src/pages/index.js

export const query = graphql`
  query IndexPage {
    allSanityStore(
      filter: {
        neighbourhoods: {
          elemMatch: { slug: { current: { eq: "harbourfront" } } }
        }
      }
    ) {
      nodes {
        id
      }
    }
  }
`

allSanityStore.nodes comes back empty. If I make the exact same query at runtime using GraphiQL, I get the expected list of nodes.

For some reason, my custom resolver has affected the result of a seemingly unrelated query.

This is just one example. It seems that any custom resolver that calls nodeModel.runQuery with a filter seems to have unpredictable, far-reaching impacts on query results across the entire site.

Steps to reproduce

See https://github.com/gatsbyjs/gatsby/issues/26056#issuecomment-675508616

Expected result

Defining a custom resolver should not affect the results of unrelated queries.

Actual result

My custom resolver affects the results of unrelated queries.

Environment

  System:
    OS: Linux 4.19 Debian GNU/Linux 9 (stretch) 9 (stretch)
    CPU: (12) x64 Intel(R) Core(TM) i7-8850H CPU @ 2.60GHz
    Shell: 4.4.12 - /bin/bash
  Binaries:
    Node: 12.18.3 - /usr/local/bin/node
    Yarn: 1.22.4 - /usr/bin/yarn
    npm: 6.14.6 - /usr/local/bin/npm
  Languages:
    Python: 2.7.13 - /usr/bin/python
  npmPackages:
    gatsby: ^2.24.47 => 2.24.47
    gatsby-source-sanity: ^6.0.3 => 6.0.3

[aaronadamsCA]

Note that I've tried converting this static query to a page query for now, and all I managed to do was reverse the effect of the apparent race condition. Now my new page query receives an empty list of neighbourhoods, while the existing page query continues to receive a correct list of neighbourhoods. It's as if the first hit on the resolver doesn't actually resolve.

My gatsby-node.js only contains two nodeModel references, one to runQuery and the other to getNodeById. Retrieving a neighbourhood first does 1 or 2 getNodeById calls, and then 1 runQuery call. To my knowledge both of these should handle dependency tracking on their own; is that still correct?

(The good news is I may be able to trim down the original report now and get to a repro easier.)

[vladar]

Are you querying a field with a custom resolver in the runQuery? Can you share your runQuery call?

[aaronadamsCA]

Sorry for the delayed response, I really need to de-abstract my types/resolvers to get to the bottom of this (and to make sure my premature abstractions aren't part of the problem). I'm working on that now. I'm also starting to eye gatsby-source-sanity with some suspicion.

A question that may help me close my own ticket here, and open a new more specific one later: is it "legal" to define new resolvers using the createTypes action in the createSchemaCustomization API, or is this to be avoided?

I'm using createSchemaCustomization because of this createResolvers limitation:

New fields will not be available on filter and sort input types. Extend types defined with createTypes if you need this.

I really want to be able to filter/sort using my custom resolvers. My approach, so far, was to extend types with createTypes in createSchemaCustomization and add my custom resolvers there. It works, but sometimes the race conditions get in the way. Was this the wrong way to go about this? Should I maybe be using createTypes in sourceNodes instead? Or something else altogether?

If this was a total violation of the intended usage of the APIs, maybe the real improvement here is just to the documentation and/or to detect/prevent these problems before they bite people.

[vladar]

Some context on what's going on under the hood: when Gatsby sees filter or sort on fields with custom resolvers - it executes those resolvers _before_ running a query. Then it puts resolved values to the node object itself (to special __gatsby_resolved property).

It works fine until you use runQuery inside your custom resolvers that affects the same fields.

My hypothesis is that this is what happens:

1. Gatsby runs resolvers when starting GraphQL query and puts results into __gatsby_resolved property on the node object itself

2. One of your custom resolvers runs runQuery and the result is replacing __gatsby_resolved property

That's why we see conflicts. In other words, the problem is that the query mutates the node itself (to enable search/sorting on those "dynamic" properties). And then the next query sees values that were set by the previous query.

That's a hypothesis as it is indeed hard to catch because when you debug it queries may be executed in a different order. It's only visible when a certain race condition occurs.

To confirm - try replacing runQuery call with custom filtering on all nodes. We really want to catch this issue (as I've seen similar reports in the past but also couldn't reproduce reliably).

[aaronadamsCA]

Your theory sounds about right. I'm finally able to dig into this issue starting this weekend. I can start as you suggested and confirm custom filtering resolves the immediate issue. If so, I may just need to leave it there until a bit later so I can move on to other things, but at least that hopefully gives you some good information!

I'll definitely want/need to move back to GraphQL filtering eventually, so I may try playing around with sourceNodes or other APIs to figure out if there's a non-dangerous combination.

[aaronadamsCA]

@vladar I found the easiest way to test your theory; just don't pass any filters to runQuery:

    const nodes = await nodeModel.runQuery({
      // query: { filter },
      query: {},
      type,
      firstOnly: false,
    });

Aside from the obvious effect of my custom resolvers now relating every node to every other node (making them decidedly less useful!), this also appears to resolve the race condition. Where I was previously seeing no data or incorrect data, I now see consistently correct data returned by both page queries and static queries. In particular, the one consistent problem spot I had is now working correctly.

So it seems your theory is correct; it's okay to use runQuery, but if you pass any filter it causes lasting effects.

The biggest problem I see in implementing filtering myself is that I don't think I'd get the benefit of runQuery's dependency tracking anymore. I assume Gatsby thinking every node is related to every other node is going to cause some pain.

Is there a "right way" to define custom resolvers that can leverage runQuery and dependency tracking without these side effects? This makes me think I should try sourceNodes:

If you define (sourceNodes) in gatsby-node.js it will be called exactly once after all of your source plugins have finished creating nodes.

[vladar]

@aaronadamsCA This is definitely a bug in Gatsby core. Now I am sure about this. If you could extract your findings into a minimal reproduction I will fix this for good.

But yeah, sourceNodes is probably an even better option - prepare your data beforehand and don't use custom resolvers.

Also, see https://github.com/gatsbyjs/gatsby/issues/23549#issuecomment-621363404 as I now think the underlying issue there was the same - I just couldn't debug it reliably as the behavior was changing when attaching a debugger (race condition didn't occur)

[aaronadamsCA]

@vladar, you should be able to clone this to reproduce:

https://github.com/shopnishe/gatsby-issue-26056

It's using a public Sanity dataset. The index page should be displaying a list of store IDs, but for me it's blank. If you can populate it with the same set of IDs as the other two pages I linked (a template page and a GraphiQL query), I believe you've fixed the bug.

I've also updated the ticket description with a straightforward explanation of the use case described in the reproduction repository.

Let me know if you need anything more. Thanks!

[aaronadamsCA]

Also, I tried this modification without success, so it seems my concern about which API I'm using is unnecessary; even when I declare my custom resolver the "right" way, the bug still stands.

gatsby-node.js

exports.createSchemaCustomization = ({ actions }) => {
  const { createTypes } = actions
  createTypes(`
    type SanityNeighbourhood implements Node @infer {
      stores: [SanityStore]
    }
  `)
}

exports.createResolvers = ({ createResolvers }) => {
  createResolvers({
    SanityNeighbourhood: {
      stores: {
        resolve: async ({ id }, args, { nodeModel }) =>
          await nodeModel.runQuery({
            type: "SanityStore",
            query: {
              filter: { neighbourhoods: { elemMatch: { id: { eq: id } } } },
            },
            firstOnly: false,
          }),
      },
    },
  })
}

[vladar]

I think I've traced the root of the issue. Haven't decided what is the right fix yet, so just sharing what I've found so far:

1. Template queries from templates/neighbourhood.js are executed. This query selects field stores here.

2. Custom resolver for this field calls runQuery which filters on the neighborhoods field by id here.

3. Under the hood, Gatsby detects which fields of SanityStore node must be resolved before actually running filters here.

4. It detects that neighborhoods field with nested id field must be resolved and resolves it. So for each node of type SanityStore we get an object like this:

{
  neighbourhoods: [
    { id: '-b4bfb35e-3ab8-55e4-a766-79324fa5f76c' },
    { id: '-67437add-1fa5-5aaf-9a2b-4b024f1f9bb9' }
  ]
}

1. The queries from steps 2 and 1 are executed correctly. So far so good.

2. The query for index page is executed here which also filters SanityStore by neighborhoods
   The important difference is that it filters it not by id but by slug.

3. Gatsby does the same thing as in step 3 and discovers that field neighborhoods must be resolved before we can run the query.

4. [Here things break]: Gatsby has an optimization that allows it to skip previously resolved fields here:

https://github.com/gatsbyjs/gatsby/blob/677a5651245137165841cd42ff7f46abf2a69a74/packages/gatsby/src/schema/node-model.js#L369-L372

The problem though is that after it skips neighbourhoods field, it uses the cached value from step 4 which only has an id and doesn't have slug:

{
  neighbourhoods: [
    { id: '-b4bfb35e-3ab8-55e4-a766-79324fa5f76c' },
    { id: '-67437add-1fa5-5aaf-9a2b-4b024f1f9bb9' }
  ]
}

If I remove this optimization it works as it should:

 const actualFieldsToResolve = fieldsToResolve

But it will deoptimize many of the existing sites and hurt performance, so this is not a viable fix (CC @pvdz).

Ideally fieldsToResolve should contain a full path to all fields we are going to resolve, i.e. { neighbourhoods: { id: true } } not just { neighbourhoods: true } (as it happens now)

Then the optimization should still work properly. But this is not a trivial change and I need to find time to work on it first.

P.S. Thanks for the repro, it is very helpful! 👍

P.P.S. Looks like you've updated the repro, so this explanation links to a specific revision I was testing against.

[vladar]

Here is my attempt to fix it: https://github.com/gatsbyjs/gatsby/pull/26644

Works for your reproduction and also preserves all the optimizations in place.

[vladar]

Published in [email protected]

[aaronadamsCA]

@vladar I haven't had any unexpected/unusual query errors since updating to this version. Thank you for the quick resolution!