Skip to content

Commit

Permalink
fix: await resolver function
Browse files Browse the repository at this point in the history
  • Loading branch information
@angeloashmore
angeloashmore committed Feb 8, 2020
1 parent a030b61 commit 5bf1d43
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 2 deletions.
2 changes: 1 addition & 1 deletion src/generator.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ function generateFieldMiddlewareFromRule(
const res = await rule.resolve(parent, args, ctx, info, options)

if (res === true) {
return resolve(parent, args, ctx, info)
return await resolve(parent, args, ctx, info)

This comment has been minimized.

Sign in to view

Copy link
@johnsjulander

johnsjulander Feb 12, 2020 •
edited
Loading

I don't think this is what you want to do here, but not completely sure.

I think by changing this, if the resolver throws an error, shield will now intercept it.

My code that I'm test looks like:

...

resolver.ts:

  Mutation: {
    login: async (_, { email }, ctx: IContext) => {
      const [user] = await ctx.prisma.user.findMany({ where: { email } })

      if (!user) {
        throw new UserInputError(`No user was found on email: ${email}`)
      }

      return {
        token: createUserToken(user.id),
        user
      }
    },
  },

permissions.ts:

import { and, shield } from 'graphql-shield'
import { isAuthenticated, isBankIdSessionNotComplete } from './rules'

export const permissions = shield({
  Query: {
    me: and(isAuthenticated)
  },

  Mutation: {
    me: and(isAuthenticated)
  },

  BankIdSessionMutation: and(isBankIdSessionNotComplete)
})

The code above was previously not intercepted by shield. Note that the login field is not even present in shields permissions.

My tests has now started to throw:

  1) Graphql Resolvers
       Login
         should throw when trying to login non-existing user:

      AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ 'Not Authorised!'
- 'No user was found on email: some-random@email'
     ^
      + expected - actual

      -Not Authorised!
      +No user was found on email: some-random@email
      
      at Suite.<anonymous> (graphql/resolvers.test.ts:1***5:14)
      at Generator.next (<anonymous>)
      at fulfilled (graphql/resolvers.test.ts:5:58)
      at processTicksAndRejections (internal/process/task_queues.js:94:5)

The test:

...
    it('should throw when trying to login non-existing user', async () => {
      const email = 'some-random@email'
      const [error] = (
        await mutate({ mutation: Login, variables: { email: 'some-random@email' } })
      ).errors

      assert.strictEqual(error.message, `No user was found on email: ${email}`)
      assert.strictEqual(error.extensions.code, 'BAD_USER_INPUT')
    })

From what I understand is "Not Authorised!" a shiled thing.

This comment has been minimized.

Sign in to view

Copy link
@johnsjulander

johnsjulander Feb 12, 2020

This test failed when we updated graphql-shield from 7.0.9 to 7.0.11

This comment has been minimized.

Sign in to view

Copy link
@angeloashmore

angeloashmore Feb 12, 2020

Author Contributor

When the resolver throws, graphql-shield will catch with the following logic:

      if (options.debug) {
        throw err
      } else if (options.allowExternalErrors) {
        return err
      } else {
        return options.fallbackError
      }

So if you don't have debug or allowExternalErrors enabled, it will return the fallback default error. Without awaiting, the thrown error was simply ignored since it would just return a Promise (i.e. always truthy).

This comment has been minimized.

Sign in to view

Copy link
@maticzav

maticzav Feb 13, 2020

Owner

Indeed! If you want to return a custom error you shouldn’t throw it but instead return it. That’s also a mechanism that allows shield to differentiate between unexpected and handled errors.

This comment has been minimized.

Sign in to view

Copy link
@johnsjulander

johnsjulander Mar 9, 2020

Ok! Nice, will change to instead return my custom error. Thx!
} else if (res === false) {
return options.fallbackError
} else {
Expand Down
2 changes: 1 addition & 1 deletion tests/fallback.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ describe('fallbackError correctly handles errors', () => {
`
const resolvers = {
Query: {
test: () => {
test: async () => {
throw new Error()
},
},
Expand Down

0 comments on commit 5bf1d43

Please sign in to comment.