From 4687be8b77b0f807b2fe4b1e2278e05d0dbd4431 Mon Sep 17 00:00:00 2001 From: Uros Smolnik Date: Tue, 12 Apr 2022 05:30:52 -0700 Subject: [PATCH] feat: add `for_key_share` ("FOR KEY SHARE") lock mode for postgres driver (#8879) Adds support for new lock mode `for_key_share` - generating FOR KEY SHARE lock. Postgres specific. Closes: #8878 --- docs/find-options.md | 19 ++-- src/find-options/FindOneOptions.ts | 1 + src/find-options/FindOptionsUtils.ts | 3 +- src/query-builder/QueryExpressionMap.ts | 1 + src/query-builder/SelectQueryBuilder.ts | 20 ++++- .../locking/query-builder-locking.ts | 88 +++++++++++++++++++ .../find-options-locking.ts | 37 ++++++++ 7 files changed, 155 insertions(+), 14 deletions(-) diff --git a/docs/find-options.md b/docs/find-options.md index 168b8fa18d..24dd68c2fa 100644 --- a/docs/find-options.md +++ b/docs/find-options.md @@ -221,7 +221,8 @@ or "dirty_read" | "pessimistic_partial_write" | "pessimistic_write_or_fail" | - "for_no_key_update" + "for_no_key_update" | + "for_key_share" } ``` @@ -239,14 +240,14 @@ userRepository.findOne({ Support of lock modes, and SQL statements they translate to, are listed in the table below (blank cell denotes unsupported). When specified lock mode is not supported, a `LockNotSupportedOnGivenDriverError` error will be thrown. ```text -| | pessimistic_read | pessimistic_write | dirty_read | pessimistic_partial_write | pessimistic_write_or_fail | for_no_key_update | -| --------------- | -------------------- | ----------------------- | ------------- | --------------------------- | --------------------------- | ------------------- | -| MySQL | LOCK IN SHARE MODE | FOR UPDATE | (nothing) | FOR UPDATE SKIP LOCKED | FOR UPDATE NOWAIT | | -| Postgres | FOR SHARE | FOR UPDATE | (nothing) | FOR UPDATE SKIP LOCKED | FOR UPDATE NOWAIT | FOR NO KEY UPDATE | -| Oracle | FOR UPDATE | FOR UPDATE | (nothing) | | | | -| SQL Server | WITH (HOLDLOCK, ROWLOCK) | WITH (UPDLOCK, ROWLOCK) | WITH (NOLOCK) | | | | -| AuroraDataApi | LOCK IN SHARE MODE | FOR UPDATE | (nothing) | | | | -| CockroachDB | | FOR UPDATE | (nothing) | | FOR UPDATE NOWAIT | FOR NO KEY UPDATE | +| | pessimistic_read | pessimistic_write | dirty_read | pessimistic_partial_write | pessimistic_write_or_fail | for_no_key_update | for_key_share | +| --------------- | -------------------- | ----------------------- | ------------- | --------------------------- | --------------------------- | ------------------- | ------------- | +| MySQL | LOCK IN SHARE MODE | FOR UPDATE | (nothing) | FOR UPDATE SKIP LOCKED | FOR UPDATE NOWAIT | | | +| Postgres | FOR SHARE | FOR UPDATE | (nothing) | FOR UPDATE SKIP LOCKED | FOR UPDATE NOWAIT | FOR NO KEY UPDATE | FOR KEY SHARE | +| Oracle | FOR UPDATE | FOR UPDATE | (nothing) | | | | | +| SQL Server | WITH (HOLDLOCK, ROWLOCK) | WITH (UPDLOCK, ROWLOCK) | WITH (NOLOCK) | | | | | +| AuroraDataApi | LOCK IN SHARE MODE | FOR UPDATE | (nothing) | | | | | +| CockroachDB | | FOR UPDATE | (nothing) | | FOR UPDATE NOWAIT | FOR NO KEY UPDATE | | ``` diff --git a/src/find-options/FindOneOptions.ts b/src/find-options/FindOneOptions.ts index 51f3409d1f..058206c8a1 100644 --- a/src/find-options/FindOneOptions.ts +++ b/src/find-options/FindOneOptions.ts @@ -77,6 +77,7 @@ export interface FindOneOptions { | "pessimistic_partial_write" | "pessimistic_write_or_fail" | "for_no_key_update" + | "for_key_share" tables?: string[] } diff --git a/src/find-options/FindOptionsUtils.ts b/src/find-options/FindOptionsUtils.ts index ca3cf25f74..ee5e967d83 100644 --- a/src/find-options/FindOptionsUtils.ts +++ b/src/find-options/FindOptionsUtils.ts @@ -181,7 +181,8 @@ export class FindOptionsUtils { options.lock.mode === "dirty_read" || options.lock.mode === "pessimistic_partial_write" || options.lock.mode === "pessimistic_write_or_fail" || - options.lock.mode === "for_no_key_update" + options.lock.mode === "for_no_key_update" || + options.lock.mode === "for_key_share" ) { const tableNames = options.lock.tables ? options.lock.tables.map((table) => { const tableAlias = qb.expressionMap.aliases.find((alias) => { diff --git a/src/query-builder/QueryExpressionMap.ts b/src/query-builder/QueryExpressionMap.ts index d944b6ab17..2b2e8627d9 100644 --- a/src/query-builder/QueryExpressionMap.ts +++ b/src/query-builder/QueryExpressionMap.ts @@ -189,6 +189,7 @@ export class QueryExpressionMap { | "pessimistic_partial_write" | "pessimistic_write_or_fail" | "for_no_key_update" + | "for_key_share" /** * Current version of the entity, used for locking. diff --git a/src/query-builder/SelectQueryBuilder.ts b/src/query-builder/SelectQueryBuilder.ts index 7eabeca0e3..a68385787b 100644 --- a/src/query-builder/SelectQueryBuilder.ts +++ b/src/query-builder/SelectQueryBuilder.ts @@ -1487,7 +1487,8 @@ export class SelectQueryBuilder | "dirty_read" | "pessimistic_partial_write" | "pessimistic_write_or_fail" - | "for_no_key_update", + | "for_no_key_update" + | "for_key_share", lockVersion?: undefined, lockTables?: string[], ): this @@ -1503,7 +1504,8 @@ export class SelectQueryBuilder | "dirty_read" | "pessimistic_partial_write" | "pessimistic_write_or_fail" - | "for_no_key_update", + | "for_no_key_update" + | "for_key_share", lockVersion?: number | Date, lockTables?: string[], ): this { @@ -2564,6 +2566,14 @@ export class SelectQueryBuilder } else { throw new LockNotSupportedOnGivenDriverError() } + + case "for_key_share": + if (driver.options.type === "postgres") { + return " FOR KEY SHARE" + lockTablesClause + } else { + throw new LockNotSupportedOnGivenDriverError() + } + default: return "" } @@ -3061,7 +3071,8 @@ export class SelectQueryBuilder "pessimistic_partial_write" || this.findOptions.lock.mode === "pessimistic_write_or_fail" || - this.findOptions.lock.mode === "for_no_key_update" + this.findOptions.lock.mode === "for_no_key_update" || + this.findOptions.lock.mode === "for_key_share" ) { const tableNames = this.findOptions.lock.tables ? this.findOptions.lock.tables.map((table) => { @@ -3140,7 +3151,8 @@ export class SelectQueryBuilder this.expressionMap.lockMode === "pessimistic_write" || this.expressionMap.lockMode === "pessimistic_partial_write" || this.expressionMap.lockMode === "pessimistic_write_or_fail" || - this.expressionMap.lockMode === "for_no_key_update") && + this.expressionMap.lockMode === "for_no_key_update" || + this.expressionMap.lockMode === "for_key_share") && !queryRunner.isTransactionActive ) throw new PessimisticLockTransactionRequiredError() diff --git a/test/functional/query-builder/locking/query-builder-locking.ts b/test/functional/query-builder/locking/query-builder-locking.ts index ed683aaaa2..0395c7f17b 100644 --- a/test/functional/query-builder/locking/query-builder-locking.ts +++ b/test/functional/query-builder/locking/query-builder-locking.ts @@ -162,6 +162,41 @@ describe("query builder > locking", () => { }), )) + it("should throw error if for key share lock used without transaction", () => + Promise.all( + connections.map(async (connection) => { + if (connection.driver.options.type === "postgres") { + return connection + .createQueryBuilder(PostWithVersion, "post") + .setLock("for_key_share") + .where("post.id = :id", { id: 1 }) + .getOne() + .should.be.rejectedWith( + PessimisticLockTransactionRequiredError, + ) + } + return + }), + )) + + it("should not throw error if for key share lock used with transaction", () => + Promise.all( + connections.map(async (connection) => { + if (connection.driver.options.type === "postgres") { + return connection.manager.transaction((entityManager) => { + return Promise.all([ + entityManager + .createQueryBuilder(PostWithVersion, "post") + .setLock("for_key_share") + .where("post.id = :id", { id: 1 }) + .getOne().should.not.be.rejected, + ]) + }) + } + return + }), + )) + it("should throw error if pessimistic_partial_write lock used without transaction", () => Promise.all( connections.map(async (connection) => { @@ -459,6 +494,37 @@ describe("query builder > locking", () => { }), )) + it("should not attach for key share lock statement on query if locking is not used", () => + Promise.all( + connections.map(async (connection) => { + if (connection.driver.options.type === "postgres") { + const sql = connection + .createQueryBuilder(PostWithVersion, "post") + .where("post.id = :id", { id: 1 }) + .getSql() + + expect(sql.indexOf("FOR KEY SHARE") === -1).to.be.true + } + return + }), + )) + + it("should attach for key share lock statement on query if locking enabled", () => + Promise.all( + connections.map(async (connection) => { + if (connection.driver.options.type === "postgres") { + const sql = connection + .createQueryBuilder(PostWithVersion, "post") + .setLock("for_key_share") + .where("post.id = :id", { id: 1 }) + .getSql() + + expect(sql.indexOf("FOR KEY SHARE") !== -1).to.be.true + } + return + }), + )) + it("should not attach pessimistic_partial_write lock statement on query if locking is not used", () => Promise.all( connections.map(async (connection) => { @@ -792,6 +858,28 @@ describe("query builder > locking", () => { }), )) + it("should throw error if for key share locking not supported by given driver", () => + Promise.all( + connections.map(async (connection) => { + if (!(connection.driver.options.type === "postgres")) { + return connection.manager.transaction((entityManager) => { + return Promise.all([ + entityManager + .createQueryBuilder(PostWithVersion, "post") + .setLock("for_key_share") + .where("post.id = :id", { id: 1 }) + .getOne() + .should.be.rejectedWith( + LockNotSupportedOnGivenDriverError, + ), + ]) + }) + } + + return + }), + )) + it("should only specify locked tables in FOR UPDATE OF clause if argument is given", () => Promise.all( connections.map(async (connection) => { diff --git a/test/functional/repository/find-options-locking/find-options-locking.ts b/test/functional/repository/find-options-locking/find-options-locking.ts index e8c1c6ac37..1148ab3ebc 100644 --- a/test/functional/repository/find-options-locking/find-options-locking.ts +++ b/test/functional/repository/find-options-locking/find-options-locking.ts @@ -190,6 +190,35 @@ describe("repository > find options > locking", () => { }), )) + it("should attach for key share lock statement on query if locking enabled", () => + Promise.all( + connections.map(async (connection) => { + if (!(connection.driver.options.type === "postgres")) return + + const executedSql: string[] = [] + + await connection.manager.transaction((entityManager) => { + const originalQuery = entityManager.queryRunner!.query.bind( + entityManager.queryRunner, + ) + entityManager.queryRunner!.query = (...args: any[]) => { + executedSql.push(args[0]) + return originalQuery(...args) + } + + return entityManager + .getRepository(PostWithVersion) + .findOne({ + where: { id: 1 }, + lock: { mode: "for_key_share" }, + }) + }) + + expect(executedSql.join(" ").includes("FOR KEY SHARE")).to.be + .true + }), + )) + it("should attach pessimistic write lock statement on query if locking enabled", () => Promise.all( connections.map(async (connection) => { @@ -606,6 +635,14 @@ describe("repository > find options > locking", () => { tables: ["post"], }, }), + entityManager.getRepository(Post).findOne({ + where: { id: 1 }, + relations: ["author"], + lock: { + mode: "for_key_share", + tables: ["post"], + }, + }), ]) }) }),