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

Error serialization logic is faulty #51

Closed
rekmarks opened this issue Mar 14, 2022 · 1 comment · Fixed by #61
Closed

Error serialization logic is faulty #51

rekmarks opened this issue Mar 14, 2022 · 1 comment · Fixed by #61
Assignees
Labels
bug Something isn't working

Comments

@rekmarks
Copy link
Member

rekmarks commented Mar 14, 2022

Errors are "serialized" using the serializeError function, and this function has considerable issues. We use the internal utility isValidCode which only returns true if this package has a message for that code in its enums. This sucks, because technically any valid integer code is valid. I don't know what I was thinking when I implemented this.

The error serialization should continue to optionally include the stack property, but it should permit any error object (and especially, error code) that is valid per the JSON-RPC 2.0 specification, i.e. any integer number. Only if the original error object does not conform to the following interface should we modify it and replace it with a -32603 error:

type JsonRpcError = {
  code: number, // specifically, any safe integer
  message: string,
  data?: Json,
  stack?: string, // non-standard, but on occasion useful
}

If a value passed to serializeError violates this interface in any way, we should create a new JSON-RPC error as follows, where data.originalError is the original error value if it's valid JSON:

{
  code: -32603,
  message: 'Invalid internal error. See "data.originalError" for original value. Please report this bug.',
  data: { originalError }
}

See #48 and the following screenshot for examples of how the current logic mangles errors:
image

@kumavis
Copy link
Member

kumavis commented Mar 8, 2023

need to rewrite enumerable to true

> e = new Error
Error
    at REPL1:1:5
    at Script.runInThisContext (node:vm:129:12)
    at REPLServer.defaultEval (node:repl:566:29)
    at bound (node:domain:421:15)
    at REPLServer.runBound [as eval] (node:domain:432:12)
    at REPLServer.onLine (node:repl:893:10)
    at REPLServer.emit (node:events:539:35)
    at REPLServer.emit (node:domain:475:12)
    at REPLServer.Interface._onLine (node:readline:487:10)
    at REPLServer.Interface._line (node:readline:864:8)
> JSON.stringify(e)
'{}'
> Object.getOwnPropertyDescriptors(e)
{
  stack: {
    value: 'Error\n' +
      '    at REPL1:1:5\n' +
      '    at Script.runInThisContext (node:vm:129:12)\n' +
      '    at REPLServer.defaultEval (node:repl:566:29)\n' +
      '    at bound (node:domain:421:15)\n' +
      '    at REPLServer.runBound [as eval] (node:domain:432:12)\n' +
      '    at REPLServer.onLine (node:repl:893:10)\n' +
      '    at REPLServer.emit (node:events:539:35)\n' +
      '    at REPLServer.emit (node:domain:475:12)\n' +
      '    at REPLServer.Interface._onLine (node:readline:487:10)\n' +
      '    at REPLServer.Interface._line (node:readline:864:8)',
    writable: true,
    enumerable: false,
    configurable: true
  }
}
> Reflect.defineProperty(e, 'stack', { enumerable: true })
true
> JSON.stringify(e)
'{"stack":"Error\\n    at REPL1:1:5\\n    at Script.runInThisContext (node:vm:129:12)\\n    at REPLServer.defaultEval (node:repl:566:29)\\n    at bound (node:domain:421:15)\\n    at REPLServer.runBound [as eval] (node:domain:432:12)\\n    at REPLServer.onLine (node:repl:893:10)\\n    at REPLServer.emit (node:events:539:35)\\n    at REPLServer.emit (node:domain:475:12)\\n    at REPLServer.Interface._onLine (node:readline:487:10)\\n    at REPLServer.Interface._line (node:readline:864:8)"}'
> 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants