Skip to content

chore(NODE-4321): add typescript to eslint rules #3304

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

Merged
merged 3 commits into from
Jun 30, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
23 changes: 23 additions & 0 deletions .eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,29 @@
"typescript-eslint/ban-ts-comment": "off"
}
},
{
// Settings for typescript test files
"files": [
"src/**/*.ts"
],
"parser": "@typescript-eslint/parser",
"parserOptions": {
"project": ["./tsconfig.json"]
},
"extends": [
"plugin:@typescript-eslint/recommended-requiring-type-checking"
],
"rules": {
"@typescript-eslint/no-unsafe-member-access": "off",
"@typescript-eslint/no-unsafe-argument": "off",
"@typescript-eslint/no-unsafe-assignment": "off",
"@typescript-eslint/no-unsafe-return": "off",
"@typescript-eslint/no-unsafe-call": "off",

"@typescript-eslint/restrict-plus-operands": "off",
"@typescript-eslint/restrict-template-expressions": "off"
}
},
{
// Settings for typescript type test files
"files": [
Expand Down
5 changes: 3 additions & 2 deletions src/change_stream.ts
Original file line number Diff line number Diff line change
Expand Up @@ -894,7 +894,7 @@ export class ChangeStream<

if (isResumableError(changeStreamError, this.cursor.maxWireVersion)) {
this._endStream();
this.cursor.close();
this.cursor.close().catch(() => null);

const topology = getTopology(this.parent);
topology.selectServer(this.cursor.readPreference, {}, serverSelectionError => {
Expand All @@ -913,6 +913,7 @@ export class ChangeStream<
*
* we promisify _processErrorIteratorModeCallback until we have a promisifed version of selectServer.
*/
// eslint-disable-next-line @typescript-eslint/unbound-method
private _processErrorIteratorMode = promisify(this._processErrorIteratorModeCallback);

/** @internal */
Expand All @@ -923,7 +924,7 @@ export class ChangeStream<
}

if (isResumableError(changeStreamError, this.cursor.maxWireVersion)) {
this.cursor.close();
this.cursor.close().catch(() => null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Btw, the docs for this rule state regarding prefixing the expression with void:

This can be a good way to explicitly mark a promise as intentionally not awaited.

That's also something we have standardized on in our team's JS code, i.e. that would be:

Suggested change
this.cursor.close().catch(() => null);
void this.cursor.close();

This is purely stylistic, of course, so feel free to ignore this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Appreciate the suggestion! IIUC wouldn't leaving the promise without a rejection handler risk exiting the process if it did error? it's spec-ed behavior to ignore errors from close (@baileympearson do you have the reference?)

Copy link
Contributor

@baileympearson baileympearson Jun 30, 2022

Choose a reason for hiding this comment

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

I don't know if it spec-ed. We had a discussion about this on the PR already but I'll post this again with more detail.

Relevant ticket: jira.mongodb.org/browse/NODE-4217
Related PR: #3226

I encountered this issue for cursor.close and filed the aforementioned ticket and left the TODOs that you see in the above PR. When we triaged/refined it, we decided that the current behavior was correct and closed the ticket as won't do (at least, I thought the jira status should be won't do but I guess we kinda ruined that flow by adding AC to the ticket related to removing TODOs. Maybe that's a process improvement we could make for the future). You can see Marin's AC on the ticket is to remove the TODOs I added, which I did in the PR. We don't have any real record of this but I do remember Jeff mentioning that this would be the intended behavior. My understanding was that the justification was two-fold:

  1. What exactly would the user do in the case that cursor.close failed? Try again? If killCursors fails, there isn't much they can do because it probably means something larger is going wrong.
  2. Cursors timeout on the server anyways, so if the cursor fails to close eventually it will be GC'd.

I think we could revisit this though because I think an argument could be made for cursor.close() propagating errors.


Now this particular instance is a separate issue, irrespective whether or not cursor.close() swallows errors. This call to cursor.close() is inside the error handler for ChangeStreams. This code is run if there is an error with the change stream. The desired (but not explicitly spec'd behavior that I know of) would be

  1. Error occurs in the CS (resumable or not)
  2. We attempt to resume. This involves cleaning up the previous cursor and restarting the stream
  3. Resuming, or failing to resume and propagating the original error to the user

What we don't want is the error from cursor.close() either crashing the user's process (unhandled process rejection) or overridding the original change stream error. The fact that our CS uses cursors internally and that they're closed during the resume process is an implementation detail and not something that a user cares about. So in this scenario, irrespective of whether or not cursor.close() swallows errors, we should swallow the error.

Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC wouldn't leaving the promise without a rejection handler risk exiting the process if it did error?

Right, it does make sense to leave a .catch(() => {}) in places where you intentionally ignore errors 👍


const topology = getTopology(this.parent);
topology.selectServer(this.cursor.readPreference, {}, serverSelectionError => {
Expand Down
5 changes: 2 additions & 3 deletions src/cmap/command_monitoring_events.ts
Original file line number Diff line number Diff line change
Expand Up @@ -248,9 +248,8 @@ function extractCommand(command: WriteProtocolMessageType): Document {
});

OP_QUERY_KEYS.forEach(key => {
const opKey = key as typeof OP_QUERY_KEYS[number];
if (command[opKey]) {
result[opKey] = command[opKey];
if (command[key]) {
result[key] = command[key];
}
});

Expand Down
45 changes: 20 additions & 25 deletions src/cmap/connect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import type { Document } from '../bson';
import { Int32 } from '../bson';
import { LEGACY_HELLO_COMMAND } from '../constants';
import {
AnyError,
MongoCompatibilityError,
MongoError,
MongoErrorLabel,
Expand Down Expand Up @@ -462,40 +461,36 @@ function makeSocks5Connection(options: MakeConnectionOptions, callback: Callback
}

// Then, establish the Socks5 proxy connection:
SocksClient.createConnection(
{
existing_socket: rawSocket,
timeout: options.connectTimeoutMS,
command: 'connect',
destination: {
host: destination.host,
port: destination.port
},
proxy: {
// host and port are ignored because we pass existing_socket
host: 'iLoveJavaScript',
port: 0,
type: 5,
userId: options.proxyUsername || undefined,
password: options.proxyPassword || undefined
}
SocksClient.createConnection({
existing_socket: rawSocket,
timeout: options.connectTimeoutMS,
command: 'connect',
destination: {
host: destination.host,
port: destination.port
},
(err: AnyError, info: { socket: Stream }) => {
if (err) {
return callback(connectionFailureError('error', err));
}

proxy: {
// host and port are ignored because we pass existing_socket
host: 'iLoveJavaScript',
port: 0,
type: 5,
userId: options.proxyUsername || undefined,
password: options.proxyPassword || undefined
}
}).then(
({ socket }) => {
// Finally, now treat the resulting duplex stream as the
// socket over which we send and receive wire protocol messages:
makeConnection(
{
...options,
existingSocket: info.socket,
existingSocket: socket,
proxyHost: undefined
},
callback
);
}
},
error => callback(connectionFailureError('error', error))
);
}
);
Expand Down
2 changes: 1 addition & 1 deletion src/cmap/connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -533,7 +533,7 @@ export class Connection extends TypedEventEmitter<ConnectionEvents> {
clusterTime = session.clusterTime;
}

const err = applySession(session, finalCmd, options as CommandOptions);
const err = applySession(session, finalCmd, options);
if (err) {
return callback(err);
}
Expand Down
14 changes: 8 additions & 6 deletions src/cmap/wire_protocol/compression.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,10 @@ export function compress(
if (Snappy[PKG_VERSION].major <= 6) {
Snappy.compress(dataToBeCompressed, callback);
} else {
Snappy.compress(dataToBeCompressed)
.then(buffer => callback(undefined, buffer))
.catch(error => callback(error));
Snappy.compress(dataToBeCompressed).then(
buffer => callback(undefined, buffer),
error => callback(error)
);
}
break;
}
Expand Down Expand Up @@ -102,9 +103,10 @@ export function decompress(
if (Snappy[PKG_VERSION].major <= 6) {
Snappy.uncompress(compressedData, { asBuffer: true }, callback);
} else {
Snappy.uncompress(compressedData, { asBuffer: true })
.then(buffer => callback(undefined, buffer))
.catch(error => callback(error));
Snappy.uncompress(compressedData, { asBuffer: true }).then(
buffer => callback(undefined, buffer),
error => callback(error)
);
}
break;
}
Expand Down
8 changes: 4 additions & 4 deletions src/collection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -725,7 +725,7 @@ export class Collection<TSchema extends Document = Document> {
}

if (typeof filter === 'function') {
callback = filter as Callback<WithId<TSchema> | null>;
callback = filter;
filter = {};
options = {};
}
Expand Down Expand Up @@ -1128,7 +1128,7 @@ export class Collection<TSchema extends Document = Document> {
this.s.db.s.client,
new CountDocumentsOperation(
this as TODO_NODE_3286,
filter as Document,
filter,
resolveOptions(this, options as CountDocumentsOptions)
),
callback
Expand Down Expand Up @@ -1191,7 +1191,7 @@ export class Collection<TSchema extends Document = Document> {
callback?: Callback<any[]>
): Promise<any[]> | void {
if (typeof filter === 'function') {
(callback = filter as Callback<any[]>), (filter = {}), (options = {});
(callback = filter), (filter = {}), (options = {});
} else {
if (arguments.length === 3 && typeof options === 'function') {
(callback = options), (options = {});
Expand Down Expand Up @@ -1667,7 +1667,7 @@ export class Collection<TSchema extends Document = Document> {
callback?: Callback<number>
): Promise<number> | void {
if (typeof filter === 'function') {
(callback = filter as Callback<number>), (filter = {}), (options = {});
(callback = filter), (filter = {}), (options = {});
} else {
if (typeof options === 'function') (callback = options), (options = {});
}
Expand Down
8 changes: 4 additions & 4 deletions src/cursor/abstract_cursor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ export interface CursorCloseOptions {
/** @public */
export interface CursorStreamOptions {
/** A transformation method applied to each document emitted by the stream */
transform?(doc: Document): Document;
transform?(this: void, doc: Document): Document;
}

/** @public */
Expand Down Expand Up @@ -597,7 +597,7 @@ export abstract class AbstractCursor<
// We only want to end this session if we created it, and it hasn't ended yet
if (session.explicit === false) {
if (!session.hasEnded) {
session.endSession(() => null);
session.endSession().catch(() => null);
}
this[kSession] = this.client.startSession({ owner: this, explicit: false });
}
Expand Down Expand Up @@ -883,7 +883,7 @@ class ReadableCursorStream extends Readable {
// a client during iteration. Alternatively, we could do the "right" thing and
// propagate the error message by removing this special case.
if (err.message.match(/server is closed/)) {
this._cursor.close();
this._cursor.close().catch(() => null);
return this.push(null);
}

Expand All @@ -902,7 +902,7 @@ class ReadableCursorStream extends Readable {
if (result == null) {
this.push(null);
} else if (this.destroyed) {
this._cursor.close();
this._cursor.close().catch(() => null);
} else {
if (this.push(result)) {
return this._readNext();
Expand Down
27 changes: 9 additions & 18 deletions src/deps.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,12 @@ try {
} catch {} // eslint-disable-line

export interface KerberosClient {
step: (challenge: string, callback?: Callback<string>) => Promise<string> | void;
wrap: (
challenge: string,
options?: { user: string },
callback?: Callback<string>
) => Promise<string> | void;
unwrap: (challenge: string, callback?: Callback<string>) => Promise<string> | void;
step(challenge: string): Promise<string>;
step(challenge: string, callback: Callback<string>): void;
wrap(challenge: string, options: { user: string }): Promise<string>;
wrap(challenge: string, options: { user: string }, callback: Callback<string>): void;
unwrap(challenge: string): Promise<string>;
unwrap(challenge: string, callback: Callback<string>): void;
}

type ZStandardLib = {
Expand Down Expand Up @@ -80,11 +79,7 @@ type SnappyLib = {
* @param callback - ONLY USED IN SNAPPY 6.x
*/
compress(buf: Buffer): Promise<Buffer>;
compress(buf: Buffer, callback: (error?: Error, buffer?: Buffer) => void): Promise<Buffer> | void;
compress(
buf: Buffer,
callback?: (error?: Error, buffer?: Buffer) => void
): Promise<Buffer> | void;
compress(buf: Buffer, callback: (error?: Error, buffer?: Buffer) => void): void;

/**
* - Snappy 6.x takes a callback and returns void
Expand All @@ -99,12 +94,7 @@ type SnappyLib = {
buf: Buffer,
opt: { asBuffer: true },
callback: (error?: Error, buffer?: Buffer) => void
): Promise<Buffer> | void;
uncompress(
buf: Buffer,
opt: { asBuffer: true },
callback?: (error?: Error, buffer?: Buffer) => void
): Promise<Buffer> | void;
): void;
};

export let Snappy: SnappyLib | { kModuleError: MongoMissingDependencyError } = makeErrorModule(
Expand Down Expand Up @@ -141,6 +131,7 @@ interface AWS4 {
* @param credentials - AWS credential details, sessionToken should be omitted entirely if its false-y
*/
sign(
this: void,
options: {
path: '/';
body: string;
Expand Down
5 changes: 3 additions & 2 deletions src/mongo_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -493,7 +493,6 @@ export class MongoClient extends TypedEventEmitter<MongoClientEvents> {
{ endSessions },
{ readPreference: ReadPreference.primaryPreferred, noResponse: true }
)
.then(() => null) // outcome does not matter
.catch(() => null); // outcome does not matter
})
.then(() => {
Expand Down Expand Up @@ -638,7 +637,9 @@ export class MongoClient extends TypedEventEmitter<MongoClientEvents> {
.then(() => {
// Do not return the result of callback
})
.finally(() => session.endSession());
.finally(() => {
session.endSession().catch(() => null);
});
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/operations/common_functions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export function indexInformation(
let options = _optionsOrCallback as IndexInformationOptions;
let callback = _callback as Callback;
if ('function' === typeof _optionsOrCallback) {
callback = _optionsOrCallback as Callback;
callback = _optionsOrCallback;
options = {};
}
// If we specified full information
Expand Down
3 changes: 2 additions & 1 deletion src/operations/execute_operation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -144,8 +144,9 @@ export function executeOperation<
});
} catch (error) {
if (session?.owner != null && session.owner === owner) {
session.endSession();
session.endSession().catch(() => null);
}

throw error;
}
});
Expand Down
14 changes: 5 additions & 9 deletions src/read_preference.ts
Original file line number Diff line number Diff line change
Expand Up @@ -163,14 +163,10 @@ export class ReadPreference {
} else if (!(readPreference instanceof ReadPreference) && typeof readPreference === 'object') {
const mode = readPreference.mode || readPreference.preference;
if (mode && typeof mode === 'string') {
return new ReadPreference(
mode as ReadPreferenceMode,
readPreference.tags ?? readPreferenceTags,
{
maxStalenessSeconds: readPreference.maxStalenessSeconds,
hedge: options.hedge
}
);
return new ReadPreference(mode, readPreference.tags ?? readPreferenceTags, {
maxStalenessSeconds: readPreference.maxStalenessSeconds,
hedge: options.hedge
});
}
}

Expand All @@ -193,7 +189,7 @@ export class ReadPreference {
} else if (r && !(r instanceof ReadPreference) && typeof r === 'object') {
const mode = r.mode || r.preference;
if (mode && typeof mode === 'string') {
options.readPreference = new ReadPreference(mode as ReadPreferenceMode, r.tags, {
options.readPreference = new ReadPreference(mode, r.tags, {
maxStalenessSeconds: r.maxStalenessSeconds
});
}
Expand Down
2 changes: 1 addition & 1 deletion src/sessions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -616,7 +616,7 @@ function attemptTransaction<TSchema>(
}

if (!isPromiseLike(promise)) {
session.abortTransaction();
session.abortTransaction().catch(() => null);
throw new MongoInvalidArgumentError(
'Function provided to `withTransaction` must return a Promise'
);
Expand Down
Loading