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

Cache hint not applied to findOne resolver, affecting nested findMany and other resolvers #9434

Open
simonfan opened this issue Dec 15, 2024 · 0 comments

Comments

@simonfan
Copy link
Contributor

The problem happens when using list-level cacheHint with findOne queries with nested findMany queries. Cache hint does not apply correctly due to missing cache hint application on findOne query.

Say we have the following example:

const COMMON_CACHE_HINT = {
  maxAge: 60,
  scope: 'PUBLIC',
}

const lists = lists({
  User: list({
    fields: {
      name: text(),
      posts: relationship({
        ref: 'Post.user',
        many: true,
      }),
    },
    graphql: {
      cacheHint: COMMON_CACHE_HINT,
    },
  }),
  Post: list({
    fields: {
      user: relationship({
        ref: 'User.posts',
        many: false,
      }),
      category: text(),
      title: text(),
      body: json(),
    },
    graphql: {
      cacheHint: COMMON_CACHE_HINT,
    },
  }),
})

The cache hint works perfectly when running a query for a list of posts:

query {
  posts(where: { category: { equals: "Category_1" } }) {
    title
    body
  }
}

# Returns maxAge=60 and caches correctly

But if the same query is executed as a nested query for a single user, say, a user's posts, the cache headers are set to "no-cache", even if the cacheHint for the User list is set to { maxAge: 60, scope: "PUBLIC" } as well.

query {
  user(where: { id: "22fc8814-3845-4287-bbbe-34cb65ecebb6" }) {
    name
    posts(where: { category: { equals: "Category_1" } }) {
      title
      body
    }
  }
}

# Returns no-cache, even though both lists are set to have
# cache maxAge=60 and scope=PUBLIC

That happens because Apollo caching calculation strategy is to respect the most restrictive cache policy from all fields of a query, and the findOne resolver seems not to be setting the correct cache hinting.

After fiddling with the code, I managed to identify two places that need to be slightly modified:

1 - At core/queries/resolvers.ts, around line 105, right before the result for the findOne resolver is returned, it should check if a cache hint was defined for the list and let apollo know of the cache settings. Just as is done on the findMany and count resolvers.

export async function findOne (
args: { where: UniqueInputFilter },
list: InitialisedList,
context: KeystoneContext
) {
// check operation permission to pass into single operation
const operationAccess = await getOperationAccess(list, context, 'query')
if (!operationAccess) return null
const accessFilters = await getAccessFilters(list, context, 'query')
if (accessFilters === false) return null
// validate and resolve the input filter
const uniqueWhere = await resolveUniqueWhereInput(args.where, list, context)
const resolvedWhere = mapUniqueWhereToWhere(uniqueWhere)
// findOne requires at least one filter
if (Object.keys(resolvedWhere).length === 0) return null
// check filter access
for (const fieldKey in resolvedWhere) {
await checkFilterOrderAccess([{ fieldKey, list }], context, 'filter')
}
// apply access control
const filter = await accessControlledFilter(list, context, resolvedWhere, accessFilters)
return await context.prisma[list.listKey].findFirst({ where: filter })
}

Should become:

export async function findOne (
  args: { where: UniqueInputFilter },
  list: InitialisedList,
  context: KeystoneContext
) {
  // ...

  const result = await context.prisma[list.listKey].findFirst({ where: filter })

  if (list.cacheHint) {
    maybeCacheControlFromInfo(info)
      ?.setCacheHint(list.cacheHint({
        result,
        operationName: info.operation.name?.value,
        meta: false
      }))
  }

  return result
}

Just like the resolver for findMany:

export async function findMany (
{ where, take, skip, orderBy: rawOrderBy, cursor }: FindManyArgsValue,
list: InitialisedList,
context: KeystoneContext,
info: GraphQLResolveInfo,
extraFilter?: PrismaFilter
): Promise<BaseItem[]> {
const maxTake = (list.graphql.types.findManyArgs.take.defaultValue ?? Infinity) as number
if ((take ?? Infinity) > maxTake) {
throw limitsExceededError({ list: list.listKey, type: 'maxTake', limit: maxTake })
}
// TODO: rewrite, this actually checks access
const orderBy = await resolveOrderBy(rawOrderBy, list, context)
const operationAccess = await getOperationAccess(list, context, 'query')
if (!operationAccess) return []
const accessFilters = await getAccessFilters(list, context, 'query')
if (accessFilters === false) return []
const resolvedWhere = await resolveWhereInput(where, list, context)
// check filter access (TODO: why isn't this using resolvedWhere)
await checkFilterOrderAccess([...traverse(list, where)], context, 'filter')
const filter = await accessControlledFilter(list, context, resolvedWhere, accessFilters)
const results = await context.prisma[list.listKey].findMany({
where: extraFilter === undefined ? filter : { AND: [filter, extraFilter] },
orderBy,
take: take ?? undefined,
skip,
cursor: cursor ?? undefined,
})
if (list.cacheHint) {
maybeCacheControlFromInfo(info)
?.setCacheHint(list.cacheHint({
results,
operationName: info.operation.name?.value,
meta: false
}))
}
return results
}

2 - At core/queries/index.ts, around line 16, where the resolver is defined and the resolver info needs to be forwarded to the findOne resolver so that it may get cache info.

const findOne = graphql.field({
type: list.graphql.types.output,
args: {
where: graphql.arg({
type: graphql.nonNull(list.graphql.types.uniqueWhere),
defaultValue: list.isSingleton ? { id: '1' } : undefined,
}),
},
async resolve (_rootVal, args, context) {
return queries.findOne(args, list, context)
},
})

Should become:

export function getQueriesForList (list: InitialisedList) {
  // ...
  const findOne = graphql.field({
    // ...
    async resolve (_rootVal, args, context, info) {
      return queries.findOne(args, list, context, info)
    },
  })
  //
}

Just like the resolver for findMany:

const findMany = graphql.field({
type: graphql.list(graphql.nonNull(list.graphql.types.output)),
args: list.graphql.types.findManyArgs,
async resolve (_rootVal, args, context, info) {
return queries.findMany(args, list, context, info)
},
})

In the coming days I'll take a deeper look and would be totally willing to submit a PR if that helps.

Thanks!

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

No branches or pull requests

1 participant