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

Fix regression in authenticatedItem query #8278

Merged
merged 2 commits into from
Jan 30, 2023

Conversation

marekryb
Copy link
Contributor

PR #8182 introduced regression that causes authenticatedItem query to return error when current user is unauthenticated (session is undefined).

Example query:

query authenticate {
      authenticatedItem {
        __typename
        ... on User {
          id
          name
        }
      }
    }

Results in

{
  "errors": [
    {
      "message": "Unexpected error.",
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],
      "path": [
        "authenticatedItem"
      ],
      "extensions": {
        "originalError": {
          "message": "Cannot read properties of undefined (reading 'itemId')",
          "stack": "TypeError: Cannot read properties of undefined (reading 'itemId')\n    at resolve (webpack-internal:///(api)/../../packages/auth/src/gql/getBaseAuthSchema.ts:56:80)\n    at executeField (file:///home/exar/os/keystone/node_modules/@graphql-tools/executor/esm/execution/execute.js:294:24)\n    at executeFields (file:///home/exar/os/keystone/node_modules/@graphql-tools/executor/esm/execution/execute.js:241:28)\n    at executeOperation (file:///home/exar/os/keystone/node_modules/@graphql-tools/executor/esm/execution/execute.js:205:18)\n    at file:///home/exar/os/keystone/node_modules/@graphql-tools/executor/esm/execution/execute.js:62:37\n    at new ValueOrPromise (/home/exar/os/keystone/node_modules/value-or-promise/build/main/ValueOrPromise.js:14:21)\n    at executeImpl (file:///home/exar/os/keystone/node_modules/@graphql-tools/executor/esm/execution/execute.js:62:12)\n    at execute (file:///home/exar/os/keystone/node_modules/@graphql-tools/executor/esm/execution/execute.js:48:12)\n    at file:///home/exar/os/keystone/node_modules/@graphql-tools/executor/esm/execution/normalizedExecutor.js:12:37\n    at new ValueOrPromise (/home/exar/os/keystone/node_modules/value-or-promise/build/main/ValueOrPromise.js:14:21)\n    at normalizedExecutor (file:///home/exar/os/keystone/node_modules/@graphql-tools/executor/esm/execution/normalizedExecutor.js:12:12)\n    at file:///home/exar/os/keystone/node_modules/@envelop/core/esm/orchestrator.js:373:33\n    at async YogaServer.getResultForParams (file:///home/exar/os/keystone/node_modules/graphql-yoga/esm/server.js:262:26)\n    at async YogaServer.getResponse (file:///home/exar/os/keystone/node_modules/graphql-yoga/esm/server.js:328:23)\n    at async YogaServer.handle (file:///home/exar/os/keystone/node_modules/graphql-yoga/esm/server.js:38:34)\n    at async requestListener (file:///home/exar/os/keystone/node_modules/@whatwg-node/server/index.mjs:214:26)\n    at async Object.apiResolver (/home/exar/os/keystone/node_modules/next/dist/server/api-utils/node.js:363:9)\n    at async DevServer.runApi (/home/exar/os/keystone/node_modules/next/dist/server/next-server.js:487:9)\n    at async Object.fn (/home/exar/os/keystone/node_modules/next/dist/server/next-server.js:749:37)\n    at async Router.execute (/home/exar/os/keystone/node_modules/next/dist/server/router.js:253:36)\n    at async DevServer.run (/home/exar/os/keystone/node_modules/next/dist/server/base-server.js:384:29)\n    at async DevServer.run (/home/exar/os/keystone/node_modules/next/dist/server/dev/next-dev-server.js:741:20)\n    at async DevServer.handleRequest (/home/exar/os/keystone/node_modules/next/dist/server/base-server.js:322:20)"
        }
      }
    }
  ],
  "data": {
    "authenticatedItem": null
  }
}

@vercel
Copy link

vercel bot commented Jan 28, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
keystone-next-docs ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Jan 28, 2023 at 0:33AM (UTC)

@changeset-bot

This comment was marked as resolved.

@vercel vercel bot temporarily deployed to Preview January 28, 2023 12:33 Inactive
@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 969cdfd:

Sandbox Source
@keystone-6/sandbox Configuration

@@ -55,7 +55,7 @@ export function getBaseAuthSchema<I extends string, S extends string>({
}),
resolve(root, args, { session, db }) {
if (
(typeof session?.itemId === 'string' || typeof session.itemId === 'number') &&
(typeof session?.itemId === 'string' || typeof session?.itemId === 'number') &&
typeof session.listKey === 'string'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be session?.listKey?

Line 54 uses context.session?.listKey.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checks go from left to right, so in case of no session, session.listKey is not executed.
This condition was not changed.

Copy link
Member

@dcousens dcousens Jan 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it would short circuit, but I think for readability I might prefer for it to be ?. until the type is refined within the next block...

I don't think I'll ask for that change in this pull request though, the actual problem and reason for this regression is that session is effectively any.

Copy link
Member

@dcousens dcousens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, @borisno2

@borisno2
Copy link
Member

borisno2 commented Jan 29, 2023

LGTM, @borisno2

LGTM too! Thanks @marekryb

@dcousens dcousens merged commit 48ff6f1 into keystonejs:main Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Unresolved bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants