Skip to content

Commit

Permalink
fix(NODE-3546): revert findOne not found result type to null (#2945)
Browse files Browse the repository at this point in the history
  • Loading branch information
nbbeeken committed Aug 24, 2021
1 parent 74bd7bd commit 1c576e9
Show file tree
Hide file tree
Showing 6 changed files with 61 additions and 44 deletions.
57 changes: 27 additions & 30 deletions src/collection.ts
@@ -1,4 +1,4 @@
import { DEFAULT_PK_FACTORY, emitWarningOnce, maybePromise, resolveOptions } from './utils';
import { DEFAULT_PK_FACTORY, emitWarningOnce, resolveOptions } from './utils';
import { ReadPreference, ReadPreferenceLike } from './read_preference';
import {
normalizeHintField,
Expand Down Expand Up @@ -99,7 +99,7 @@ import type {

/** @public */
export interface ModifyResult<TSchema = Document> {
value?: TSchema;
value: TSchema | null;
lastErrorObject?: Document;
ok: 0 | 1;
}
Expand Down Expand Up @@ -676,51 +676,48 @@ export class Collection<TSchema extends Document = Document> {
* @param options - Optional settings for the command
* @param callback - An optional callback, a Promise will be returned if none is provided
*/
findOne(): Promise<TSchema | undefined>;
findOne(callback: Callback<TSchema | undefined>): void;
findOne(filter: Filter<TSchema>): Promise<TSchema | undefined>;
findOne(filter: Filter<TSchema>, callback: Callback<TSchema | undefined>): void;
findOne(filter: Filter<TSchema>, options: FindOptions): Promise<TSchema | undefined>;
findOne(
filter: Filter<TSchema>,
options: FindOptions,
callback: Callback<TSchema | undefined>
): void;
findOne(): Promise<TSchema | null>;
findOne(callback: Callback<TSchema | null>): void;
findOne(filter: Filter<TSchema>): Promise<TSchema | null>;
findOne(filter: Filter<TSchema>, callback: Callback<TSchema | null>): void;
findOne(filter: Filter<TSchema>, options: FindOptions): Promise<TSchema | null>;
findOne(filter: Filter<TSchema>, options: FindOptions, callback: Callback<TSchema | null>): void;

// allow an override of the schema.
findOne<T = TSchema>(): Promise<T | undefined>;
findOne<T = TSchema>(callback: Callback<T | undefined>): void;
findOne<T = TSchema>(filter: Filter<TSchema>): Promise<T | undefined>;
findOne<T = TSchema>(filter: Filter<TSchema>, options?: FindOptions): Promise<T | undefined>;
findOne<T = TSchema>(): Promise<T | null>;
findOne<T = TSchema>(callback: Callback<T | null>): void;
findOne<T = TSchema>(filter: Filter<TSchema>): Promise<T | null>;
findOne<T = TSchema>(filter: Filter<TSchema>, options?: FindOptions): Promise<T | null>;
findOne<T = TSchema>(
filter: Filter<TSchema>,
options?: FindOptions,
callback?: Callback<T | undefined>
callback?: Callback<T | null>
): void;

findOne(
filter?: Filter<TSchema> | Callback<TSchema | undefined>,
options?: FindOptions | Callback<TSchema | undefined>,
callback?: Callback<TSchema>
): Promise<TSchema | undefined> | void {
filter?: Filter<TSchema> | Callback<TSchema | null>,
options?: FindOptions | Callback<TSchema | null>,
callback?: Callback<TSchema | null>
): Promise<TSchema | null> | void {
if (callback != null && typeof callback !== 'function') {
throw new MongoInvalidArgumentError(
'Third parameter to `findOne()` must be a callback or undefined'
);
}

if (typeof filter === 'function')
(callback = filter as Callback<Document | undefined>), (filter = {}), (options = {});
if (typeof options === 'function') (callback = options), (options = {});
if (typeof filter === 'function') {
callback = filter as Callback<TSchema | null>;
filter = {};
options = {};
}
if (typeof options === 'function') {
callback = options;
options = {};
}

const finalFilter = filter ?? {};
const finalOptions = options ?? {};
return maybePromise(callback, callback =>
this.find(finalFilter, finalOptions)
.limit(-1)
.batchSize(1)
.next((error, result) => callback(error, result ?? undefined))
);
return this.find(finalFilter, finalOptions).limit(-1).batchSize(1).next(callback);
}

/**
Expand Down
1 change: 1 addition & 0 deletions src/cursor/abstract_cursor.ts
Expand Up @@ -297,6 +297,7 @@ export abstract class AbstractCursor<
/** Get the next available document from the cursor, returns null if no more documents are available. */
next(): Promise<TSchema | null>;
next(callback: Callback<TSchema | null>): void;
next(callback?: Callback<TSchema | null>): Promise<TSchema | null> | void;
next(callback?: Callback<TSchema | null>): Promise<TSchema | null> | void {
return maybePromise(callback, done => {
if (this[kId] === Long.ZERO) {
Expand Down
23 changes: 22 additions & 1 deletion test/functional/crud_api.test.js
@@ -1,7 +1,7 @@
'use strict';
const test = require('./shared').assert;
const { expect } = require('chai');
const { ReturnDocument } = require('../../src');
const { ReturnDocument, ObjectId } = require('../../src');
const setupDatabase = require('./shared').setupDatabase;

// instanceof cannot be use reliably to detect the new models in js due to scoping and new
Expand All @@ -14,6 +14,27 @@ describe('CRUD API', function () {
return setupDatabase(this.configuration);
});

it('should correctly execute findOne method using crud api', async function () {
const configuration = this.configuration;
const client = configuration.newClient();
await client.connect();
const db = client.db(configuration.db);
const collection = db.collection('t');

await collection.insertOne({ findOneTest: 1 });

const findOneResult = await collection.findOne({ findOneTest: 1 });

expect(findOneResult).to.have.property('findOneTest', 1);
expect(findOneResult).to.have.property('_id').that.is.instanceOf(ObjectId);

const findNoneResult = await collection.findOne({ findOneTest: 2 });
expect(findNoneResult).to.be.null;

await collection.drop();
await client.close();
});

it('should correctly execute find method using crud api', {
// Add a tag that our runner can trigger on
// in this case we are setting that node needs to be higher than 0.10.X to run
Expand Down
4 changes: 2 additions & 2 deletions test/functional/sessions.test.js
Expand Up @@ -301,8 +301,8 @@ describe('Sessions', function () {
controlSession = client.startSession();

// set up sessions with two sets of cluster times
expect(await collection.findOne({}, { session: controlSession })).to.be.undefined;
expect(await collection.findOne({}, { session: testSession })).to.be.undefined;
expect(await collection.findOne({}, { session: controlSession })).to.be.null;
expect(await collection.findOne({}, { session: testSession })).to.be.null;
await collection.insertOne({ apple: 'green' });
expect(await collection.findOne({}, { session: otherSession }))
.property('apple')
Expand Down
16 changes: 7 additions & 9 deletions test/types/community/collection/findX.test-d.ts
Expand Up @@ -87,9 +87,7 @@ cursor.forEach(
}
);

expectType<Bag | undefined>(
await collectionBag.findOne({ color: 'red' }, { projection: { cost: 1 } })
);
expectType<Bag | null>(await collectionBag.findOne({ color: 'red' }, { projection: { cost: 1 } }));

const overrideFind = await collectionBag.findOne<{ cost: number }>(
{ color: 'white' },
Expand All @@ -98,7 +96,7 @@ const overrideFind = await collectionBag.findOne<{ cost: number }>(
expectType<PropExists<typeof overrideFind, 'color'>>(false);

// Overriding findOne, makes the return that exact type
expectType<{ cost: number } | undefined>(
expectType<{ cost: number } | null>(
await collectionBag.findOne<{ cost: number }>({ color: 'red' }, { projection: { cost: 1 } })
);

Expand All @@ -121,13 +119,13 @@ interface House {

const car = db.collection<Car>('car');

expectNotType<House | undefined>(await car.findOne({}));
expectNotType<House | null>(await car.findOne({}));

interface Car {
make: string;
}

function printCar(car: Car | undefined) {
function printCar(car: Car | null) {
console.log(car ? `A car of ${car.make} make` : 'No car');
}

Expand Down Expand Up @@ -232,12 +230,12 @@ interface TypedDb extends Db {
const typedDb = client.db('test2') as TypedDb;

const person = typedDb.collection('people').findOne({});
expectType<Promise<Person | undefined>>(person);
expectType<Promise<Person | null>>(person);

typedDb.collection('people').findOne({}, function (_err, person) {
expectType<Person | undefined>(person);
expectType<Person | null | undefined>(person); // null is if nothing is found, undefined is when there is an error defined
});

typedDb.collection('things').findOne({}, function (_err, thing) {
expectType<Thing | undefined>(thing);
expectType<Thing | null | undefined>(thing);
});
4 changes: 2 additions & 2 deletions test/types/union_schema.test-d.ts
Expand Up @@ -31,9 +31,9 @@ expectAssignable<ShapeInsert>({ height: 4, width: 4 });
expectAssignable<ShapeInsert>({ radius: 4 });

const c: Collection<Shape> = null as never;
expectType<Promise<Shape | undefined>>(c.findOne({ height: 4, width: 4 }));
expectType<Promise<Shape | null>>(c.findOne({ height: 4, width: 4 }));
// collection API can only respect TSchema given, cannot pick a type inside a union
expectNotType<Promise<Rectangle | undefined>>(c.findOne({ height: 4, width: 4 }));
expectNotType<Promise<Rectangle | null>>(c.findOne({ height: 4, width: 4 }));

interface A {
_id: number;
Expand Down

0 comments on commit 1c576e9

Please sign in to comment.