feat(NODE-5484)!: mark MongoError for internal use and remove Node14 cause assignment logic

[W-A-James]

Description

What is changing?

• MongoError and all MongoError subclasses now have documentation stating that they are no longer covered under semver
• MongoError constructor now defers to Node16+'s Error constructor to set the cause field
• MongoCryptError is now a subclass of MongoError
• Add unit tests checking that all subclasses of MongoCryptError are subclasses of MongoError
• Update etc/notes/error.md to add MongoCrypt to error hierarchy

Is there new documentation needed for these changes?

No

What is the motivation for this change?

NODE-5484

Release Highlight

MongoError and its subclasses now have internal constructors

MongoError and its subclasses are not meant to be constructed by users as they are thrown within the driver on specific error conditions to allow users to react to these conditions in ways which match their use cases. The constructors for these types are now subject to change outside of major versions and their documentation updated to reflect this.

MongoCryptError is now a subclass of MongoError

Since MongoCryptError made use of NodeJS 16's Error API, it has long supported setting the Error.cause field using options passed in via the constructor. Now that Node 16 is our minimum supported version, MongoError has been modified to make use of this API as well, allowing us to let MongoCryptError subclass from it directly.

Double check the following

• Ran npm run check:lint script
• Self-review completed using the steps outlined here
• PR title follows the correct format: type(NODE-xxxx)[!]: description
  • Example: feat(NODE-1234)!: rewriting everything in coffeescript
• Changes are covered by tests
• New TODOs have a related JIRA ticket

[addaleax]

What exactly is it that adding @internal is supposed to achieve? It won’t prevent users from creating class instances, it’s only going to make the class look like it doesn’t have its own constructor, i.e. uses the constructor(...args) { super(...args); } one.

If you want to prevent users from creating instances of these classes, you may need to do something like

export class MongoSomethingError extends MongoError  {
  private constructor(...) {}

  /** @internal */
  static create(...args) { return new this(...args); }
}

[W-A-James]

We're doing this in an attempt to communicate that these classes shouldn't be instantiated by users. It's a pattern we have a lot elsewhere in the driver that probably could use some work with regards to getting that message across.

Wouldn't this solution also have the same effect? The user might not have documentation for the static create method, but they would still be able to call it on their end, right?

[addaleax]

Yeah, I get the intention here, and I agree that it's a goal that's worth putting effort into. :)

The problem is specifically with marking constructors as @internal, which is different from all other methods, because that it does not make it appear to consumers of your .d.ts file that there is no publicly accessible constructor; instead, the TS definition effectively says that "the constructor of this class has the same signature as the constructor of the parent class".

That's different with a static method; yeah, of course users could still call it by casting the class to as any or just not using TS validation at all, but at least it would not show up in the generated TS definitions.

[W-A-James]

Okay, I see what you're saying now. I'll check with the team to see if we want to start with that new pattern here or deal with it in a separate ticket that takes care of this across the driver. Thanks for the clarification!

[baileympearson]

Rather than linking the package, maybe we could just say that these errors are thrown from the driver's CSFLE logic?

[baileympearson]

I think the error descriptions were a little clunky - can we reword them?

[nbbeeken]

Why did this change? IIUC we still want errors passed as the first argument to become the cause. Unless we want to go through and make sure we stop passing errors as the first argument and move them to the options.cause?

[baileympearson]

I say no, and that we should fix all places that rely on the message being assigned to the cause now. There are only a few errors that provide constructors that take errors as the first property.

Relying solely on the cause is simpler and more aligned with how errors work in JS generally. Also, it avoid the ambiguous situation where both an error message is provided and a cause is as well (

new MongoError(
    new Error('ahh'),
    { cause: new Error('other error') }
) // what is `cause` assigned to? 

the ticket does note that the signature of MongoError should be constructor(message: string, { cause?: AnyError }) and it currently doesn't match this, good catch

[W-A-James]

Updated errors. Going through the tests to update their assertions now.

[nbbeeken]

buildErrorMessage relied on being passed the potential AggregateError, this change should result in some test failures.

[baileympearson]

Is this comment suggesting we fix the tests or that we revert the change?

[nbbeeken]

Either, I don't feel strongly. I don't see test failures which is intriguing, maybe because the tests are still just passing in an error as the first argument?

[nbbeeken]

I was going to suggest stealing the message from the error here but I think this is our aggregate error case so the message will be an empty string.

Maybe we allow the string argument to be nullish, then buildMessage's job will be to obtain it from the cause, or we make this say "error during connection establishment"

[baileympearson]

I'm a fan of the "make all consumers specify the error message". It's more explicit and it makes it easier to reason about what error messages are for given errors.

If we want to handle aggregate errors, there's nothing stopping us from using buildErrorMessage wherever we want:

      return new MongoNetworkError(MongoError.buildErrorMessage(err), { cause: err });