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

Extensions in ApolloError can only contain code #2917

Closed
joepuzzo opened this issue Jun 25, 2019 · 12 comments
Closed

Extensions in ApolloError can only contain code #2917

joepuzzo opened this issue Jun 25, 2019 · 12 comments
Labels
🍐 error-handling Pertaining to error handling (or lack thereof), not just for just general errors/bugs.

Comments

@joepuzzo
Copy link

According to the graphql spec.

"GraphQL services may provide an additional entry to errors with key extensions. This entry, if set, must have a map as its value. This entry is reserved for implementors to add additional information to errors however they see fit, and there are no additional restrictions on its contents."

{
  "errors": [
    {
      "message": "Name for character with ID 1002 could not be fetched.",
      "locations": [ { "line": 6, "column": 7 } ],
      "path": [ "hero", "heroFriends", 1, "name" ],
      "extensions": {
        "code": "CAN_NOT_FETCH_BY_ID",
        "timestamp": "Fri Feb 9 14:33:09 UTC 2018"
      }
    }
  ]
}

"GraphQL services should NOT provide any additional entries to the error format since they could conflict with additional entries that may be added in future versions of this specification."

SPEC >>> https://github.com/graphql/graphql-spec/blob/master/spec/Section%207%20--%20Response.md

But it looks like when constructing an Apollo Error it will only allow you to shove the code into extensions. Why don't additional properties end up on extensions OR will the constructor allow you to pass extensions.

this.extensions = { code };

@joepuzzo
Copy link
Author

joepuzzo commented Jun 25, 2019

Ok so i have determined this is due to the enrichError function found here. This is really bad because if you call new ApolloError('The message i want shown', 'SOME_CODE', { some_exception_data: 'fasdfadf', message: 'This message will override the constructors message :('} ) Then the message in the exception ( additional properties ) will take over.

@SekibOmazic
Copy link

SekibOmazic commented Jul 22, 2019

@joepuzzo I experience the same issue in my project. My hacky solution was to overwrite the extensions attribute after creating an ApolloError (or even to define MyCustomError extends ApolloError)

@joepuzzo
Copy link
Author

@SekibOmazic Yeah i ended up Extending ApolloError

@Benrozenberg
Copy link

Hi @joepuzzo, Can you show an example of how did you extend ApollloError?
Here is what we did which is not working:

export class UserNotFoundError extends ApolloError {
  constructor(message) {
    super(message, 'NOT_AUTHORIZED_USER', {
      code: 245,
      clientMessage: 'user is not found',
    });
    Object.defineProperty(this, 'name', { value: 'UserNotFountError' });
  }
}

@joepuzzo
Copy link
Author

class ValidationError extends ApolloError {
  constructor(message) {
    const extensions = {
      type: 'api_error',
      name: 'ValidationError'
    };
    super(message, 'VALIDATION_ERROR', extensions);
  }
}

@joepuzzo
Copy link
Author

joepuzzo commented Jul 30, 2019

@Benrozenberg Remember all the stuff in extensions will end up in error.extensions.exception
So the above would look like this

{
  message: "The username 'Joe' is already taken"
  extensions: {
    code: "VALIDATION_ERROR"
    exception: {
      type: "api_error",
      name: "ValidationError",
    }
  }
}

@Benrozenberg
Copy link

@joepuzzo thanks, it seems like we are doing the same thing. Unfortunately even with your code I still cannot get the exception in the client.

@abernix abernix added the 🍐 error-handling Pertaining to error handling (or lack thereof), not just for just general errors/bugs. label Sep 3, 2019
@yuchristina
Copy link

yuchristina commented Sep 13, 2019

Hi @Benrozenberg! I was able to get around this problem this way

class GqlError extends ApolloError { 	
     constructor(message, code = ‘INTERNAL_SERVER_ERROR’, extensions = {}) { 		 
             super(message) 		
             this.extensions = { ...extensions, code } 		
             if (!this.name) { 			
                    Object.defineProperty(this, ‘name’, { value: ‘GqlError’ }) 		
             } 	
    } 
}

However, I am wondering whether Apollo tooling (e.g. Engine, Apollo-client) would be sensitive to code being a direct property in extensions, or is it perfectly fine for me to nest code in another property in extensions?

@Benrozenberg
Copy link

Hi @yuchristina, I think your solution should not cause any problem and you can do both but if you think it might why don't you create a new property (myCode) instead of overriding code property?

anyways if it helps i ended up doing as follow :

export class AuthenticationError extends ApolloError {
  constructor(message, clientMessage = 'user is not authenticated - 401') {
    super(message, 'UNAUTHENTICATED', {
      codeNumber: 401,
      clientMessage,
    });
    Object.defineProperty(this, 'name', { value: 'NotAuthorizedError' });
  }
}

After formating error this allows me to send different messages to the server and to the client

@trevor-scheer
Copy link
Member

Hey @joepuzzo, @yuchristina, @SekibOmazic, @Benrozenberg, and other interested parties 👋
Thank you all for opening this issue and providing context and workarounds!

I've opened a PR to make sure I'm understanding the issue(s) at hand correctly. It seems that we have multiple issues to address, so I've started with a couple test cases for validation from you all. These tests are currently failing as expected. Please feel free to @ me here or comment on the PR with anything that's incorrect or overlooked.

Confirmed ✅

  1. Extensions can't be provided via the ApolloError constructor (they are overwritten)
  2. The message property on the 3rd constructor argument overwrites the original message

Uncertain / needs clarification:
I see some discussion about name, though I'm unsure how it pertains. I see it being used in enrichError, but I'm not sure how it relates to the original intent of this issue. Possibly a separate issue?

@joepuzzo
Copy link
Author

@Benrozenberg Remember all the stuff in extensions will end up in error.extensions.exception
So the above would look like this

{
  message: "The username 'Joe' is already taken"
  extensions: {
    code: "VALIDATION_ERROR"
    exception: {
      type: "api_error",
      name: "ValidationError",
    }
  }
}

So I had made this comment a while ago and now noticed that apollo seems to correctly set the additional fields in extensions.

@trevor-scheer
Copy link
Member

@joepuzzo I just want to make sure I understand, are things as expected for you now? If not, please feel free to open a separate issue - I really should've closed this one when I landed #3408

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🍐 error-handling Pertaining to error handling (or lack thereof), not just for just general errors/bugs.
Projects
None yet
Development

No branches or pull requests

6 participants