Support for aggregate errors

[sameer-coder]

Example

const log = require('pino')()
const foo = new Error('foo')
const bar = new Error('bar')
const aggregate = new AggregateError([foo, bar], 'aggregated message')
log.error(aggregate)

Produces the following serialized output:

{
  "level": 50,
  "time": 1641313649352,
  "pid": 1788,
  "hostname": "SAMEER",
  "err": {
    "type": "AggregateError",
    "message": "aggregated message",
    "stack": "AggregateError: aggregated message\n at Object.<anonymous> (pino-std-serializers\\example.js:12:19)\n at Module._compile (node:internal/modules/cjs/loader:1108:14)\n at Object.Module._extensions..js (node:internal/modules/cjs/loader:1137:10)\n at Module.load (node:internal/modules/cjs/loader:988:32)\n at Function.Module._load (node:internal/modules/cjs/loader:828:14)\n at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:76:12)\n at node:internal/main/run_main_module:17:47",
    "aggregateErrors": [
      {
        "type": "Error",
        "message": "foo",
        "stack": "Error: foo\n at Object.<anonymous> (pino-std-serializers\\example.js:10:15)\n at Module._compile (node:internal/modules/cjs/loader:1108:14)\n at Object.Module._extensions..js (node:internal/modules/cjs/loader:1137:10)\n at Module.load (node:internal/modules/cjs/loader:988:32)\n at Function.Module._load (node:internal/modules/cjs/loader:828:14)\n at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:76:12)\n at node:internal/main/run_main_module:17:47"
      },
      {
        "type": "Error",
        "message": "bar",
        "stack": "Error: bar\n at Object.<anonymous> (pino-std-serializers\\example.js:11:15)\n at Module._compile (node:internal/modules/cjs/loader:1108:14)\n at Object.Module._extensions..js (node:internal/modules/cjs/loader:1137:10)\n at Module.load (node:internal/modules/cjs/loader:988:32)\n at Function.Module._load (node:internal/modules/cjs/loader:828:14)\n at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:76:12)\n at node:internal/main/run_main_module:17:47"
      }
    ]
  },
  "msg": "aggregated message"
}

Note: AggregateError is only supported by node 15+

Closes #86

[mcollina]

lgtm

[voxpelli]

Some comments, thoughts, questions – but most importantly: A note that this is susceptible to infinite recursion. Only the latter is why I set "Request changes" :)

Do note though: I'm not a maintainer of this module

[voxpelli]

This bypasses the if (!val.hasOwnProperty(seen)) { check and thus is susceptible to infinite recursion if an aggregate error includes itself in the list of errors.

We should add a prevents infinite aggregate recursion test + either add a check right here or in the beginning of errSerializer to ensure that infinite is aborted.

[sameer-coder]

@voxpelli I agree that infinite recursion is possible if the aggregate error includes itself in the list of errors but can you provide a scenario where this could happen? I think creating a new aggregate error requires you to provide an iterable of already existing errors so the list of errors cannot contain a reference to the aggregate error itself.

[mcollina]

@sameer-coder I'm kind of lost on why it's impossible to write a unit test that cover this case. Can you add one anyway?

[sameer-coder]

Not really sure if this test is written correctly

[jsumners]

Wouldn't this be agg1.errors.push(agg1)? Although, I agree with you. I'd like to have a clear explanation of when we think this would actually happen, @voxpelli. An AggregateError must be initialized with an array of existing errors. Thus, it cannot be initialized with itself in the aggregate. Anyone appending to the aggregate to its errors list seems a Bad Idea, and a scenario we should just classify as unsupported.

http://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/AggregateError

Also, the spec requires at least the array of errors be present when creating a new aggregate -- https://tc39.es/ecma262/multipage/fundamental-objects.html#sec-aggregate-error-constructor

[sameer-coder]

@voxpelli Did you get a chance to look at this again?

[voxpelli]

Anyone appending to the aggregate to its errors list seems a Bad Idea, and a scenario we should just classify as unsupported.

I agree, but since the consequences are catastrophic, I think we should still prevent it.

Same would apply for Error Causes.

[jsumners]

@mcollina do you have any thoughts on this? @davidmarkclements ?

[mcollina]

I don't think cycles are a problem. It will be catastrophic but also very easy to debug and fix.
They will need to modify the .errors property manually.

To prevernt this, I would just wrap the function in a try/catch

[jsumners]

So, to be clear, we are agreeing to remove the edge case detection in this code?

[voxpelli]

And if someone jumps on eg. huntr.dev and reports this as a potential DDoS vector, then we'll simply say that its not valid?

I'm okay with that 👍

[jsumners]

Yes. As stated, it's unsupported.

[sameer-coder]

@jsumners @voxpelli I have updated the code

[jsumners]

Looks good to me.