From 295b52799e14aee4103fc18ba006203f08a2aa13 Mon Sep 17 00:00:00 2001 From: Victor Date: Sat, 16 May 2020 02:38:51 +0300 Subject: [PATCH] fix: findOne(undefined|null|false) returns undefined (#5905) Co-authored-by: Victor Daev --- src/entity-manager/LiteralEntityManager.ts | 21 +- .../LiteralMongoEntityManager.ts | 22 +- src/repository/BaseEntity.ts | 6 +- src/repository/LiteralMongoRepository.ts | 8 +- src/repository/LiteralRepository.ts | 8 +- test/github-issues/2500/entity/Post.mongo.ts | 16 ++ test/github-issues/2500/entity/Post.sql.ts | 16 ++ test/github-issues/2500/issue-2500.ts | 208 ++++++++++++++++++ 8 files changed, 294 insertions(+), 11 deletions(-) create mode 100644 test/github-issues/2500/entity/Post.mongo.ts create mode 100644 test/github-issues/2500/entity/Post.sql.ts create mode 100644 test/github-issues/2500/issue-2500.ts diff --git a/src/entity-manager/LiteralEntityManager.ts b/src/entity-manager/LiteralEntityManager.ts index c2322c24c3..9671c193ae 100644 --- a/src/entity-manager/LiteralEntityManager.ts +++ b/src/entity-manager/LiteralEntityManager.ts @@ -472,12 +472,27 @@ export function createLiteralEntityManager({ connection, queryRunner }: // console.log("results", results); // return results; }, - + /** + * @param entityClass + * @param {string | string[] | number | number[] | Date | Date[] | ObjectID | ObjectID[] | FindOptions | any} [idOrOptionsOrConditions] + * @param {FindOptions} [maybeOptions] + */ findOne( entityClass: EntityTarget, - idOrOptionsOrConditions?: string | string[] | number | number[] | Date | Date[] | ObjectID | ObjectID[] | FindOptions | any, - maybeOptions?: FindOptions + ...args: (string | string[] | number | number[] | Date | Date[] | ObjectID | ObjectID[] | FindOptions | any)[] ): Promise { + if (args.length > 2) { + throw new Error("Too many arguments."); + } + + const idOrOptionsOrConditions = args[0]; + const maybeOptions = args[1]; + + if (args.length >= 1) { + if (idOrOptionsOrConditions === undefined || idOrOptionsOrConditions === null || idOrOptionsOrConditions === false) { + return Promise.resolve(undefined); + } + } let findOptions: FindOptions | undefined = undefined; if (FindOptionsUtils.isFindOptions(idOrOptionsOrConditions)) { diff --git a/src/entity-manager/LiteralMongoEntityManager.ts b/src/entity-manager/LiteralMongoEntityManager.ts index 98508a49f7..ff33c87a4e 100644 --- a/src/entity-manager/LiteralMongoEntityManager.ts +++ b/src/entity-manager/LiteralMongoEntityManager.ts @@ -300,9 +300,27 @@ export function createLiteralMongoEntityManager({ connection }: { return await cursor.toArray(); }, + /** + * @param entityClassOrName + * @param {string | string[] | number | number[] | Date | Date[] | ObjectID | ObjectID[] | FindOptions | FindOptionsWhere} [optionsOrConditions] + * @param {FindOptions} [maybeOptions] + */ async findOne(entityClassOrName: EntityTarget, - optionsOrConditions?: string | string[] | number | number[] | Date | Date[] | ObjectID | ObjectID[] | FindOptions | FindOptionsWhere, - maybeOptions?: FindOptions): Promise { + ...args: (string | string[] | number | number[] | Date | Date[] | ObjectID | ObjectID[] | FindOptions | FindOptionsWhere | undefined)[] + ): Promise { + if (args.length > 2) { + throw new Error("Too many arguments."); + } + + const optionsOrConditions = args[0]; + const maybeOptions = args[1]; + + if (args.length >= 1) { + if (optionsOrConditions === undefined || optionsOrConditions === null || optionsOrConditions === false) { + return Promise.resolve(undefined); + } + } + const objectIdInstance = PlatformTools.load("mongodb").ObjectID; const id = (optionsOrConditions instanceof objectIdInstance) || typeof optionsOrConditions === "string" ? optionsOrConditions : undefined; const findOneOptionsOrConditions = (id ? maybeOptions : optionsOrConditions) as any; diff --git a/src/repository/BaseEntity.ts b/src/repository/BaseEntity.ts index a2b9bc7059..38ab40ff1e 100644 --- a/src/repository/BaseEntity.ts +++ b/src/repository/BaseEntity.ts @@ -320,9 +320,11 @@ export class BaseEntity { /** * Finds first entity that matches given conditions. + * @param {string|number|Date|ObjectID|FindOptions|FindOptionsWhere} [optionsOrConditions] + * @param {FindOptions} [maybeOptions] */ - static findOne(this: ObjectType, optionsOrConditions?: string|number|Date|ObjectID|FindOptions|FindOptionsWhere, maybeOptions?: FindOptions): Promise { - return (this as any).getRepository().findOne(optionsOrConditions as any, maybeOptions); + static findOne(this: ObjectType, ...args: (string|number|Date|ObjectID|FindOptions|FindOptionsWhere|undefined)[]): Promise { + return (this as any).getRepository().findOne(...args); } /** diff --git a/src/repository/LiteralMongoRepository.ts b/src/repository/LiteralMongoRepository.ts index b95bc143f0..744e1fa24f 100644 --- a/src/repository/LiteralMongoRepository.ts +++ b/src/repository/LiteralMongoRepository.ts @@ -70,8 +70,12 @@ export function createLiteralMongoRepository({ manager, target, queryRun return this.manager.findByIds(this.getMetadata().target, ids, optionsOrConditions); }, - findOne(optionsOrConditions?: string | number | Date | ObjectID | FindOptions | FindOptionsWhere, maybeOptions?: FindOptions): Promise { - return this.manager.findOne(this.getMetadata().target, optionsOrConditions as any, maybeOptions as any); + /** + * @param {string | number | Date | ObjectID | FindOptions | FindOptionsWhere} [optionsOrConditions] + * @param {FindOptions} [maybeOptions] + */ + findOne(...args: (string | number | Date | ObjectID | FindOptions | FindOptionsWhere | undefined)[]): Promise { + return this.manager.findOne(this.getMetadata().target, ...args); }, createCursor(query?: ObjectLiteral): Cursor { diff --git a/src/repository/LiteralRepository.ts b/src/repository/LiteralRepository.ts index 564e10e6e9..c985f5165f 100644 --- a/src/repository/LiteralRepository.ts +++ b/src/repository/LiteralRepository.ts @@ -123,8 +123,12 @@ export function createLiteralRepository({ manager, target, queryRunner } return this.manager.findByIds(this.getMetadata().target as any, ids, optionsOrConditions as any); }, - findOne(optionsOrConditions?: string | number | Date | ObjectID | FindOptions | FindOptionsWhere, maybeOptions?: FindOptions): Promise { - return this.manager.findOne(this.getMetadata().target as any, optionsOrConditions as any, maybeOptions); + /** + * @param {string | number | Date | ObjectID | FindOptions | FindOptionsWhere} [optionsOrConditions] + * @param {FindOptions} [maybeOptions] + */ + findOne(...args: (string | number | Date | ObjectID | FindOptions | FindOptionsWhere | undefined)[]): Promise { + return this.manager.findOne(this.getMetadata().target as any, ...args); }, findOneOrFail(optionsOrConditions?: string | number | Date | ObjectID | FindOptions | FindOptionsWhere, maybeOptions?: FindOptions): Promise { diff --git a/test/github-issues/2500/entity/Post.mongo.ts b/test/github-issues/2500/entity/Post.mongo.ts new file mode 100644 index 0000000000..cee864fa22 --- /dev/null +++ b/test/github-issues/2500/entity/Post.mongo.ts @@ -0,0 +1,16 @@ +import {Entity} from "../../../../src/decorator/entity/Entity"; +import {Column} from "../../../../src/decorator/columns/Column"; +import {BaseEntity} from "../../../../src/repository/BaseEntity"; +import {ObjectID, ObjectIdColumn} from "../../../../src"; + +@Entity() +export class MongoPost extends BaseEntity { + @ObjectIdColumn() + id: ObjectID; + + @Column() + title: string; + + @Column() + author: string; +} diff --git a/test/github-issues/2500/entity/Post.sql.ts b/test/github-issues/2500/entity/Post.sql.ts new file mode 100644 index 0000000000..7727425d99 --- /dev/null +++ b/test/github-issues/2500/entity/Post.sql.ts @@ -0,0 +1,16 @@ +import {Entity} from "../../../../src/decorator/entity/Entity"; +import {Column} from "../../../../src/decorator/columns/Column"; +import {BaseEntity} from "../../../../src/repository/BaseEntity"; +import {PrimaryGeneratedColumn} from "../../../../src/decorator/columns/PrimaryGeneratedColumn"; + +@Entity() +export class SqlPost extends BaseEntity { + @PrimaryGeneratedColumn() + id: number; + + @Column() + title: string; + + @Column() + author: string; +} diff --git a/test/github-issues/2500/issue-2500.ts b/test/github-issues/2500/issue-2500.ts new file mode 100644 index 0000000000..b986ed2f57 --- /dev/null +++ b/test/github-issues/2500/issue-2500.ts @@ -0,0 +1,208 @@ +import "reflect-metadata"; +import {expect} from "chai"; +import {createTestingConnections, closeTestingConnections, reloadTestingDatabases, TestingOptions} from "../../utils/test-utils"; +import {Connection} from "../../../src/connection/Connection"; +import {BaseEntity} from "../../../src/repository/BaseEntity"; +import {SqlPost} from "./entity/Post.sql"; +import {MongoPost} from "./entity/Post.mongo"; + +const testParams: {dbType: string, connectionsParams: TestingOptions, PostConstructor: typeof SqlPost | typeof MongoPost}[] = [ + { + dbType: "sql", + connectionsParams: { + entities: [__dirname + "/entity/*.sql{.js,.ts}"], + schemaCreate: true, + dropSchema: true, + }, + PostConstructor: SqlPost + }, + { + dbType: "mongo", + connectionsParams: { + entities: [__dirname + "/entity/*.mongo{.js,.ts}"], + enabledDrivers: ["mongodb"], + schemaCreate: true, + dropSchema: true, + }, + PostConstructor: MongoPost + } +]; + +describe("github issues > #2500 .findOne(undefined) returns first item in the database instead of undefined", () => { + testParams.forEach(({dbType, connectionsParams, PostConstructor}) => describe(dbType, () => { + let connections: Connection[]; + before(async () => connections = await createTestingConnections(connectionsParams)); + beforeEach(() => reloadTestingDatabases(connections)); + after(() => closeTestingConnections(connections)); + + const Post = PostConstructor; + let firstPost: SqlPost | MongoPost; + let secondPost: SqlPost | MongoPost; + + beforeEach(() => Promise.all(connections.map(async connection => { + firstPost = new Post(); + firstPost.title = "How to buy a cat"; + firstPost.author = "John Doe"; + await connection.manager.save(firstPost); + + secondPost = new Post(); + secondPost.title = "How to buy a dog"; + secondPost.author = "Jane Doe"; + await connection.manager.save(secondPost); + }))); + + describe("EntityManager.findOne", () => { + it("should find one record when no arguments given", () => Promise.all(connections.map(async connection => { + const post = await connection.manager.findOne(Post); + + expect(post).not.to.be.undefined; + expect(post!.id).not.to.be.undefined; + expect(post!.title).to.be.a("string"); + expect(post!.author).to.be.a("string"); + }))); + + it("should find one record for given criteria", () => Promise.all(connections.map(async connection => { + const post1 = await connection.manager.findOne(Post, firstPost.id); + expect(post1).not.to.be.undefined; + expect(post1!.id).to.be.eql(firstPost.id); + expect(post1!.title).to.be.equal("How to buy a cat"); + expect(post1!.author).to.be.equal("John Doe"); + + const post2 = await connection.manager.findOne(Post, { title: "How to buy a dog" }); + expect(post2).not.to.be.undefined; + expect(post2!.id).to.be.eql(secondPost.id); + expect(post2!.title).to.be.equal("How to buy a dog"); + expect(post2!.author).to.be.equal("Jane Doe"); + + const post3 = await connection.manager.findOne(Post, firstPost.id, { where: { title: "How to buy a cat" } }); + expect(post3).not.to.be.undefined; + expect(post3!.id).to.be.eql(firstPost.id); + expect(post3!.title).to.be.equal("How to buy a cat"); + expect(post3!.author).to.be.equal("John Doe"); + }))); + + it("should find no record for wrong criteria", () => Promise.all(connections.map(async connection => { + expect(await connection.manager.findOne(Post, { title: "How to buy a pig" })).to.be.undefined; + expect(await connection.manager.findOne(Post, firstPost.id, { where: { title: "How to buy a dog" } })).to.be.undefined; + }))); + + it("should find no record for findOne(undefined)", () => Promise.all(connections.map(async connection => { + expect(await connection.manager.findOne(Post, undefined)).to.be.undefined; + }))); + + it("should throw an error for findOne(null)", () => Promise.all(connections.map(async connection => { + expect(await connection.manager.findOne(Post, null as any)).to.be.undefined; + }))); + + it("should throw an error for findOne(false)", () => Promise.all(connections.map(async connection => { + expect(await connection.manager.findOne(Post, false as any)).to.be.undefined; + }))); + }); + + describe("Repository.findOne", () => { + it("should find one record when no arguments given", () => Promise.all(connections.map(async connection => { + const post = await connection.getRepository(Post).findOne(); + + expect(post).not.to.be.undefined; + expect(post!.id).not.to.be.undefined; + expect(post!.title).to.be.a("string"); + expect(post!.author).to.be.a("string"); + }))); + + it("should find one record for given criteria", () => Promise.all(connections.map(async connection => { + const post1 = await connection.getRepository(Post).findOne(firstPost.id); + expect(post1).not.to.be.undefined; + expect(post1!.id).to.be.eql(firstPost.id); + expect(post1!.title).to.be.equal("How to buy a cat"); + expect(post1!.author).to.be.equal("John Doe"); + + const post2 = await connection.getRepository(Post).findOne({ title: "How to buy a dog" }); + expect(post2).not.to.be.undefined; + expect(post2!.id).to.be.eql(secondPost.id); + expect(post2!.title).to.be.equal("How to buy a dog"); + expect(post2!.author).to.be.equal("Jane Doe"); + + const post3 = await connection.getRepository(Post).findOne(firstPost.id, { where: { title: "How to buy a cat" } }); + expect(post3).not.to.be.undefined; + expect(post3!.id).to.be.eql(firstPost.id); + expect(post3!.title).to.be.equal("How to buy a cat"); + expect(post3!.author).to.be.equal("John Doe"); + }))); + + it("should find no record for wrong criteria", () => Promise.all(connections.map(async connection => { + expect(await connection.getRepository(Post).findOne({ title: "How to buy a pig" })).to.be.undefined; + expect(await connection.getRepository(Post).findOne(firstPost.id, { where: { title: "How to buy a dog" } })).to.be.undefined; + }))); + + it("should find no record for findOne(undefined)", () => Promise.all(connections.map(async connection => { + expect(await connection.getRepository(Post).findOne(undefined)).to.be.undefined; + }))); + + it("should throw an error for findOne(null)", () => Promise.all(connections.map(async connection => { + expect(await connection.getRepository(Post).findOne(null as any)).to.be.undefined; + }))); + + it("should throw an error for findOne(false)", () => Promise.all(connections.map(async connection => { + expect(await connection.getRepository(Post).findOne(false as any)).to.be.undefined; + }))); + }); + + describe("BaseEntity.findOne", () => { + it("should find one record when no arguments given", () => Promise.all(connections.map(async connection => { + BaseEntity.useConnection(connection); + const post = await Post.findOne(); + + expect(post).not.to.be.undefined; + expect(post!.id).not.to.be.undefined; + expect(post!.title).to.be.a("string"); + expect(post!.author).to.be.a("string"); + }))); + + it("should find one record for given criteria", () => Promise.all(connections.map(async connection => { + BaseEntity.useConnection(connection); + const post1 = await Post.findOne(firstPost.id); + expect(post1).not.to.be.undefined; + expect(post1!.id).to.be.eql(firstPost.id); + expect(post1!.title).to.be.equal("How to buy a cat"); + expect(post1!.author).to.be.equal("John Doe"); + + BaseEntity.useConnection(connection); + const post2 = await Post.findOne({ title: "How to buy a dog" }); + expect(post2).not.to.be.undefined; + expect(post2!.id).to.be.eql(secondPost.id); + expect(post2!.title).to.be.equal("How to buy a dog"); + expect(post2!.author).to.be.equal("Jane Doe"); + + BaseEntity.useConnection(connection); + const post3 = await Post.findOne(firstPost.id, { where: { title: "How to buy a cat" } }); + expect(post3).not.to.be.undefined; + expect(post3!.id).to.be.eql(firstPost.id); + expect(post3!.title).to.be.equal("How to buy a cat"); + expect(post3!.author).to.be.equal("John Doe"); + }))); + + it("should find no record for wrong criteria", () => Promise.all(connections.map(async connection => { + BaseEntity.useConnection(connection); + expect(await Post.findOne({ title: "How to buy a pig" })).to.be.undefined; + + BaseEntity.useConnection(connection); + expect(await Post.findOne(firstPost.id, { where: { title: "How to buy a dog" } })).to.be.undefined; + }))); + + it("should find no record for findOne(undefined)", () => Promise.all(connections.map(async connection => { + BaseEntity.useConnection(connection); + expect(await Post.findOne(undefined)).to.be.undefined; + }))); + + it("should throw an error for findOne(null)", () => Promise.all(connections.map(async connection => { + BaseEntity.useConnection(connection); + expect(await Post.findOne(null as any)).to.be.undefined; + }))); + + it("should throw an error for findOne(false)", () => Promise.all(connections.map(async connection => { + BaseEntity.useConnection(connection); + expect(await Post.findOne(false as any)).to.be.undefined; + }))); + }); + })); +});