Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 39 commits into from
Aug 22, 2023
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
a8653ba
feat(NODE-5484)!: make MongoError constructors internal and remove No…
W-A-James Aug 7, 2023
d504317
fix(NODE-5484): only remove unnecessary options
W-A-James Aug 7, 2023
d2012c5
fix(NODE-5484): make constructors internal
W-A-James Aug 7, 2023
ea5ea72
test(NODE-5484): add unit tests to check that MongoCrypt Errors subcl…
W-A-James Aug 7, 2023
53491bd
docs(NODE-5484): start docs update
W-A-James Aug 7, 2023
55f487c
fix(NODE-5484): remove unneeded type
W-A-James Aug 7, 2023
ccb0866
docs(NODE-5484): update docs
W-A-James Aug 7, 2023
be7c2f9
docs(NODE-5484): fix diagram
W-A-James Aug 7, 2023
85b02a5
docs(NODE-5484): fix diagram
W-A-James Aug 7, 2023
4b1ad15
docs(NODE-5484): clean up mermaid code
W-A-James Aug 7, 2023
aac86bc
fix(NODE-5484): fix inheritance
W-A-James Aug 7, 2023
48f7e15
fix(NODE-5484): undo unneeded change
W-A-James Aug 7, 2023
6618575
fix(NODE-5484): override cause field
W-A-James Aug 8, 2023
ae76093
Merge branch 'main' into NODE-5484
W-A-James Aug 8, 2023
1d08c4b
test(NODE-5484): fix test
W-A-James Aug 8, 2023
2a58338
test(NODE-5484): update test
W-A-James Aug 8, 2023
12d8aa7
test(NODE-5484): remove duplicate describe block
W-A-James Aug 8, 2023
484519d
test(NODE-5484): fix unit test
W-A-James Aug 8, 2023
d8ee589
docs(NODE-5484): update MongoError API docs
W-A-James Aug 10, 2023
0838e97
docs(NODE-5484): Refine docs
W-A-James Aug 10, 2023
7120ef7
docs(NODE-5484): formatting and wording
W-A-James Aug 10, 2023
414d53a
Update etc/notes/errors.md
W-A-James Aug 15, 2023
b51ad79
docs(NODE-5484): update docs
W-A-James Aug 15, 2023
161d32b
Update etc/notes/errors.md
W-A-James Aug 16, 2023
564d117
docs(NODE-5484): update wording
W-A-James Aug 16, 2023
aaa2ebe
fix(NODE-5484): update constructors to not take error
W-A-James Aug 16, 2023
4ca5acd
test(NODE-5484): fix test assertions
W-A-James Aug 16, 2023
6511534
test(NODE-5484): start fixing csfle tests
W-A-James Aug 16, 2023
65c7767
fix(NODE-5484): Update error constructor calls and csfle prose tests
W-A-James Aug 18, 2023
4ec65c2
test(NODE-5484): update assertion
W-A-James Aug 21, 2023
56920da
test(NODE-5484): Fix test assertions
W-A-James Aug 21, 2023
9d01de6
test(NODE-5484): revert test changes and update errors tests
W-A-James Aug 21, 2023
25ad3a4
fix(NODE-5484): fix error message construction
W-A-James Aug 21, 2023
b0d687a
style(NODE-5484): remove comment
W-A-James Aug 21, 2023
2cfa31d
test(NODE-5484): revert unit test
W-A-James Aug 21, 2023
b818f3b
test(NODE-5484): revert test change
W-A-James Aug 21, 2023
d24285b
test(NODE-5484): update tests to accept options
W-A-James Aug 22, 2023
c942e9f
test(NODE-5484): migrate MongoError.buildErrorMessage tests to unit t…
W-A-James Aug 22, 2023
7553a6d
Merge branch 'main' into NODE-5484
baileympearson Aug 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/bulk/common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -598,7 +598,7 @@ export class MongoBulkWriteError extends MongoServerError {
writeErrors: OneOrMore<WriteError> = [];
err?: WriteConcernError;

/** Creates a new MongoBulkWriteError */
/** @internal Creates a new MongoBulkWriteError */
constructor(
error:
| { message: string; code: number; writeErrors?: WriteError[] }
Expand Down
3 changes: 2 additions & 1 deletion src/client-side-encryption/errors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import { type Document } from '../bson';
import { MongoError } from '../error';

/**
* @public
* An error indicating that something went wrong specifically with MongoDB Client Encryption
*/
export class MongoCryptError extends Error {
export class MongoCryptError extends MongoError {
/** @internal */
constructor(message: string, options: { cause?: Error } = {}) {
super(message, options);
Expand Down
4 changes: 4 additions & 0 deletions src/cmap/errors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ export class PoolClosedError extends MongoDriverError {
/** The address of the connection pool */
address: string;

/** @internal */
constructor(pool: ConnectionPool) {
super('Attempted to check out a connection from closed connection pool');
this.address = pool.address;
Expand All @@ -27,6 +28,7 @@ export class PoolClearedError extends MongoNetworkError {
/** The address of the connection pool */
address: string;

/** @internal */
constructor(pool: ConnectionPool, message?: string) {
const errorMessage = message
? message
Expand All @@ -47,6 +49,7 @@ export class PoolClearedError extends MongoNetworkError {
* @category Error
*/
export class PoolClearedOnNetworkError extends PoolClearedError {
/** @internal */
constructor(pool: ConnectionPool) {
super(pool, `Connection to ${pool.address} interrupted due to server monitor timeout`);
}
Expand All @@ -64,6 +67,7 @@ export class WaitQueueTimeoutError extends MongoDriverError {
/** The address of the connection pool */
address: string;

/** @internal */
constructor(message: string, address: string) {
super(message);
this.address = address;
Expand Down
56 changes: 42 additions & 14 deletions src/error.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,10 @@ export interface ErrorDescription extends Document {
errInfo?: Document;
}

interface OptionsWithCause {
cause?: Error;
}

function isAggregateError(e: Error): e is Error & { errors: Error[] } {
return 'errors' in e && Array.isArray(e.errors);
}
Expand All @@ -130,16 +134,10 @@ export class MongoError extends Error {
code?: number | string;
topologyVersion?: TopologyVersion;
connectionGeneration?: number;
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
cause?: Error; // depending on the node version, this may or may not exist on the base

constructor(message: string | Error) {
super(MongoError.buildErrorMessage(message));
if (message instanceof Error) {
this.cause = message;
}

/** @internal */
constructor(message: string | Error, options?: OptionsWithCause) {
super(MongoError.buildErrorMessage(message), options);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

this[kErrorLabels] = new Set();
}

Expand Down Expand Up @@ -198,6 +196,7 @@ export class MongoServerError extends MongoError {
ok?: number;
[key: string]: any;

/** @internal */
constructor(message: ErrorDescription) {
super(message.message || message.errmsg || message.$err || 'n/a');
if (message.errorLabels) {
Expand All @@ -222,6 +221,7 @@ export class MongoServerError extends MongoError {
* @category Error
*/
export class MongoDriverError extends MongoError {
/** @internal */
constructor(message: string) {
super(message);
}
Expand All @@ -242,6 +242,7 @@ export class MongoDriverError extends MongoError {
*/

export class MongoAPIError extends MongoDriverError {
/** @internal */
constructor(message: string) {
super(message);
}
Expand All @@ -262,6 +263,7 @@ export class MongoAPIError extends MongoDriverError {
* @category Error
*/
export class MongoRuntimeError extends MongoDriverError {
/** @internal */
constructor(message: string) {
super(message);
}
Expand All @@ -279,6 +281,7 @@ export class MongoRuntimeError extends MongoDriverError {
* @category Error
*/
export class MongoBatchReExecutionError extends MongoAPIError {
/** @internal */
constructor(message = 'This batch has already been executed, create new batch to execute') {
super(message);
}
Expand All @@ -296,6 +299,7 @@ export class MongoBatchReExecutionError extends MongoAPIError {
* @category Error
*/
export class MongoDecompressionError extends MongoRuntimeError {
/** @internal */
constructor(message: string) {
super(message);
}
Expand All @@ -313,6 +317,7 @@ export class MongoDecompressionError extends MongoRuntimeError {
* @category Error
*/
export class MongoNotConnectedError extends MongoAPIError {
/** @internal */
constructor(message: string) {
super(message);
}
Expand All @@ -330,6 +335,7 @@ export class MongoNotConnectedError extends MongoAPIError {
* @category Error
*/
export class MongoTransactionError extends MongoAPIError {
/** @internal */
constructor(message: string) {
super(message);
}
Expand All @@ -347,6 +353,7 @@ export class MongoTransactionError extends MongoAPIError {
* @category Error
*/
export class MongoExpiredSessionError extends MongoAPIError {
/** @internal */
constructor(message = 'Cannot use a session that has ended') {
super(message);
}
Expand All @@ -364,6 +371,7 @@ export class MongoExpiredSessionError extends MongoAPIError {
* @category Error
*/
export class MongoKerberosError extends MongoRuntimeError {
/** @internal */
constructor(message: string) {
super(message);
}
Expand All @@ -381,6 +389,7 @@ export class MongoKerberosError extends MongoRuntimeError {
* @category Error
*/
export class MongoAWSError extends MongoRuntimeError {
/** @internal */
constructor(message: string) {
super(message);
}
Expand All @@ -398,6 +407,7 @@ export class MongoAWSError extends MongoRuntimeError {
* @category Error
*/
export class MongoAzureError extends MongoRuntimeError {
/** @internal */
constructor(message: string) {
super(message);
}
Expand All @@ -414,6 +424,7 @@ export class MongoAzureError extends MongoRuntimeError {
* @category Error
*/
export class MongoChangeStreamError extends MongoRuntimeError {
/** @internal */
constructor(message: string) {
super(message);
}
Expand All @@ -430,6 +441,7 @@ export class MongoChangeStreamError extends MongoRuntimeError {
* @category Error
*/
export class MongoTailableCursorError extends MongoAPIError {
/** @internal */
constructor(message = 'Tailable cursor does not support this operation') {
super(message);
}
Expand All @@ -445,6 +457,7 @@ export class MongoTailableCursorError extends MongoAPIError {
* @category Error
*/
export class MongoGridFSStreamError extends MongoRuntimeError {
/** @internal */
constructor(message: string) {
super(message);
}
Expand All @@ -462,6 +475,7 @@ export class MongoGridFSStreamError extends MongoRuntimeError {
* @category Error
*/
export class MongoGridFSChunkError extends MongoRuntimeError {
/** @internal */
constructor(message: string) {
super(message);
}
Expand All @@ -488,6 +502,7 @@ export class MongoGridFSChunkError extends MongoRuntimeError {
* @category Error
*/
export class MongoUnexpectedServerResponseError extends MongoRuntimeError {
/** @internal */
constructor(message: string) {
super(message);
}
Expand All @@ -505,6 +520,7 @@ export class MongoUnexpectedServerResponseError extends MongoRuntimeError {
* @category Error
*/
export class MongoCursorInUseError extends MongoAPIError {
/** @internal */
constructor(message = 'Cursor is already initialized') {
super(message);
}
Expand All @@ -522,6 +538,7 @@ export class MongoCursorInUseError extends MongoAPIError {
* @category Error
*/
export class MongoServerClosedError extends MongoAPIError {
/** @internal */
constructor(message = 'Server is closed') {
super(message);
}
Expand All @@ -538,6 +555,7 @@ export class MongoServerClosedError extends MongoAPIError {
* @category Error
*/
export class MongoCursorExhaustedError extends MongoAPIError {
/** @internal */
constructor(message?: string) {
super(message || 'Cursor is exhausted');
}
Expand All @@ -555,6 +573,7 @@ export class MongoCursorExhaustedError extends MongoAPIError {
* @category Error
*/
export class MongoTopologyClosedError extends MongoAPIError {
/** @internal */
constructor(message = 'Topology is closed') {
super(message);
}
Expand Down Expand Up @@ -585,6 +604,7 @@ export class MongoNetworkError extends MongoError {
/** @internal */
[kBeforeHandshake]?: boolean;

/** @internal */
constructor(message: string | Error, options?: MongoNetworkErrorOptions) {
super(message);

Expand All @@ -607,6 +627,7 @@ export class MongoNetworkError extends MongoError {
* mongodb-client-encryption has a dependency on this error with an instanceof check
*/
export class MongoNetworkTimeoutError extends MongoNetworkError {
/** @internal */
constructor(message: string, options?: MongoNetworkErrorOptions) {
super(message, options);
}
Expand All @@ -622,6 +643,7 @@ export class MongoNetworkTimeoutError extends MongoNetworkError {
* @category Error
*/
export class MongoParseError extends MongoDriverError {
/** @internal */
constructor(message: string) {
super(message);
}
Expand All @@ -640,6 +662,7 @@ export class MongoParseError extends MongoDriverError {
* @category Error
*/
export class MongoInvalidArgumentError extends MongoAPIError {
/** @internal */
constructor(message: string) {
super(message);
}
Expand All @@ -658,6 +681,7 @@ export class MongoInvalidArgumentError extends MongoAPIError {
* @category Error
*/
export class MongoCompatibilityError extends MongoAPIError {
/** @internal */
constructor(message: string) {
super(message);
}
Expand All @@ -676,6 +700,7 @@ export class MongoCompatibilityError extends MongoAPIError {
* @category Error
*/
export class MongoMissingCredentialsError extends MongoAPIError {
/** @internal */
constructor(message: string) {
super(message);
}
Expand All @@ -692,9 +717,9 @@ export class MongoMissingCredentialsError extends MongoAPIError {
* @category Error
*/
export class MongoMissingDependencyError extends MongoAPIError {
constructor(message: string, { cause }: { cause?: Error } = {}) {
/** @internal */
constructor(message: string) {
super(message);
if (cause) this.cause = cause;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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); }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!


override get name(): string {
Expand All @@ -710,11 +735,12 @@ export class MongoSystemError extends MongoError {
/** An optional reason context, such as an error saved during flow of monitoring and selecting servers */
reason?: TopologyDescription;

constructor(message: string, reason: TopologyDescription) {
/** @internal */
constructor(message: string, reason: TopologyDescription, options?: OptionsWithCause) {
if (reason && reason.error) {
super(reason.error.message || reason.error);
super(reason.error.message || reason.error, options);
} else {
super(message);
super(message, options);
}

if (reason) {
Expand All @@ -735,6 +761,7 @@ export class MongoSystemError extends MongoError {
* @category Error
*/
export class MongoServerSelectionError extends MongoSystemError {
/** @internal */
constructor(message: string, reason: TopologyDescription) {
super(message, reason);
}
Expand Down Expand Up @@ -766,6 +793,7 @@ export class MongoWriteConcernError extends MongoServerError {
/** The result document (provided if ok: 1) */
result?: Document;

/** @internal */
constructor(message: ErrorDescription, result?: Document) {
if (result && Array.isArray(result.errorLabels)) {
message.errorLabels = result.errorLabels;
Expand Down
37 changes: 37 additions & 0 deletions test/unit/client-side-encryption/errors.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/* eslint-disable @typescript-eslint/no-restricted-imports */
import { expect } from 'chai';

import {
MongoCryptAzureKMSRequestError,
MongoCryptCreateDataKeyError,
MongoCryptCreateEncryptedCollectionError,
MongoCryptError,
MongoCryptInvalidArgumentError
} from '../../../src/client-side-encryption/errors';
import { MongoError } from '../../mongodb';

describe('ClientEncryption', function () {
describe('Errors', function () {
const errors = [
new MongoCryptAzureKMSRequestError(''),
new MongoCryptCreateDataKeyError({
encryptedFields: {},
cause: new Error()
}),
new MongoCryptCreateEncryptedCollectionError({
encryptedFields: {},
cause: new Error()
}),
new MongoCryptError(''),
new MongoCryptInvalidArgumentError('')
];

for (const err of errors) {
describe(err.name, function () {
it('is subclass of MongoError', function () {
expect(err).to.be.instanceOf(MongoError);
});
});
}
});
});