fix(mssql): place table lock hint inside nested join parentheses by lazerg · Pull Request #12765 · typeorm/typeorm

lazerg · 2026-08-07T18:49:12Z

Description of change

On SQL Server, a relation chain two levels deep combined with setLock() produced invalid T-SQL. The table hint landed after the closing parenthesis of the nested join, where SQL Server does not allow one, and the inner table was left without a hint at all:

LEFT JOIN ("profile" "p" LEFT JOIN "country" "c" WITH (NOLOCK) ON ...) WITH (NOLOCK) ON ...

The query then failed with Incorrect syntax near the keyword 'with'. Nesting the joins in parentheses came in with #11137, but buildJoinClause still appended the hint after postfix, so once a join had children the hint drifted outside the parentheses instead of staying on its own table reference. Moving the call in front of childJoins puts the hint straight after the alias it belongs to, which is where the flat (depth 1) case already put it:

LEFT JOIN ("profile" "p" WITH (NOLOCK) LEFT JOIN "country" "c" WITH (NOLOCK) ON ...) ON ...

Verified with a new test in test/functional/query-builder/locking, which fails on master and passes here. The full suite runs green against SQL Server 2025.

Pull-Request Checklist

Code is up-to-date with the master branch
This pull request links a relevant issue using a closing keyword:
Fixes #12764
There are new or updated tests validating the change (tests/**.test.ts)
Documentation has been updated to reflect this change (docs/docs/**.md) N/A, bug fix with no documented behavior change

qodo-free-for-open-source-projects · 2026-08-07T18:52:06Z

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📜 Skill insights (0)

1. Test returns inside loop 🐞 Bug ☼ Reliability

Description

The new nested-join lock test uses return inside a for (const dataSource of dataSources) loop,
so if the first DataSource isn’t MSSQL the test exits early and runs zero assertions. This makes the
new regression test order-dependent and can let the MSSQL bug slip through CI when multiple drivers
are enabled.

Code

test/functional/query-builder/locking/query-builder-locking.test.ts[R251-254]

+        for (const dataSource of dataSources) {
+            if (!(dataSource.driver.options.type === "mssql")) {
+                return
+            }

Evidence
dataSources is an array produced by createTestingConnections, so a return inside the loop
aborts the entire test and prevents reaching any later MSSQL DataSource(s). Other functional tests
in the repo use continue to skip unsupported drivers within such loops, demonstrating the intended
control flow.
test/functional/query-builder/locking/query-builder-locking.test.ts[22-30]
test/functional/query-builder/locking/query-builder-locking.test.ts[250-255]
test/functional/query-builder/select/query-builder-select.test.ts[612-616]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new test returns from the test callback when it encounters a non-MSSQL DataSource. In a multi-driver run, this can skip the MSSQL assertions entirely.
### Issue Context
`createTestingConnections` returns an array of `dataSources`, and this test iterates them. Skipping should be done per-iteration (`continue`) or by filtering to MSSQL sources first.
### Fix Focus Areas
- test/functional/query-builder/locking/query-builder-locking.test.ts[249-267]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. Nested join not fully asserted 🐞 Bug ⚙ Maintainability

Description

The added test claims to ensure the dirty-read lock hint is attached to every table in the nested
join, but it only asserts the hint on the first joined table and forbids the misplaced trailing hint
pattern. It does not assert that the deeper joined table alias ("images") also has WITH (NOLOCK),
so a partial regression could still pass.

Code

test/functional/query-builder/locking/query-builder-locking.test.ts[R263-266]

+            expect(sql).to.contain(
+                'INNER JOIN ("category" "categories" WITH (NOLOCK)',
+            )
+            expect(sql).to.not.contain(") WITH (NOLOCK)")

Evidence

The test builds a query that joins post.categories and then categories.images, but the
expectations only check the categories join fragment and the absence of ) WITH (NOLOCK). The
Image entity exists and (by default naming) corresponds to the "image" table name, so asserting
the images join lock hint is feasible and would match the test’s stated intent.

test/functional/query-builder/locking/query-builder-locking.test.ts[256-267]
test/functional/query-builder/locking/entity/Image.ts[1-15]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new test doesn’t verify that the nested joined table (alias `images`) also receives `WITH (NOLOCK)`, despite the test name stating "every table".
### Issue Context
The query includes `.innerJoinAndSelect("categories.images", "images")`, so the SQL should include a fragment like `"image" "images" WITH (NOLOCK)` for MSSQL dirty_read.
### Fix Focus Areas
- test/functional/query-builder/locking/query-builder-locking.test.ts[256-267]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Author self-review: I have reviewed the code review findings, and addressed the relevant ones.

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

Previous review results

Review updated until commit be4b77a ⚖️ Balanced

Results up to commit cd4e0bd

🐞 Bugs (2) 📘 Rule violations (0) 📜 Skill insights (0)

1. Test returns inside loop 🐞 Bug ☼ Reliability

Description

The new nested-join lock test uses return inside a for (const dataSource of dataSources) loop,
so if the first DataSource isn’t MSSQL the test exits early and runs zero assertions. This makes the
new regression test order-dependent and can let the MSSQL bug slip through CI when multiple drivers
are enabled.

Code

test/functional/query-builder/locking/query-builder-locking.test.ts[R251-254]

+        for (const dataSource of dataSources) {
+            if (!(dataSource.driver.options.type === "mssql")) {
+                return
+            }

Evidence
dataSources is an array produced by createTestingConnections, so a return inside the loop
aborts the entire test and prevents reaching any later MSSQL DataSource(s). Other functional tests
in the repo use continue to skip unsupported drivers within such loops, demonstrating the intended
control flow.
test/functional/query-builder/locking/query-builder-locking.test.ts[22-30]
test/functional/query-builder/locking/query-builder-locking.test.ts[250-255]
test/functional/query-builder/select/query-builder-select.test.ts[612-616]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new test returns from the test callback when it encounters a non-MSSQL DataSource. In a multi-driver run, this can skip the MSSQL assertions entirely.
### Issue Context
`createTestingConnections` returns an array of `dataSources`, and this test iterates them. Skipping should be done per-iteration (`continue`) or by filtering to MSSQL sources first.
### Fix Focus Areas
- test/functional/query-builder/locking/query-builder-locking.test.ts[249-267]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. Nested join not fully asserted 🐞 Bug ⚙ Maintainability

Description

The added test claims to ensure the dirty-read lock hint is attached to every table in the nested
join, but it only asserts the hint on the first joined table and forbids the misplaced trailing hint
pattern. It does not assert that the deeper joined table alias ("images") also has WITH (NOLOCK),
so a partial regression could still pass.

Code

test/functional/query-builder/locking/query-builder-locking.test.ts[R263-266]

+            expect(sql).to.contain(
+                'INNER JOIN ("category" "categories" WITH (NOLOCK)',
+            )
+            expect(sql).to.not.contain(") WITH (NOLOCK)")

Evidence

The test builds a query that joins post.categories and then categories.images, but the
expectations only check the categories join fragment and the absence of ) WITH (NOLOCK). The
Image entity exists and (by default naming) corresponds to the "image" table name, so asserting
the images join lock hint is feasible and would match the test’s stated intent.

test/functional/query-builder/locking/query-builder-locking.test.ts[256-267]
test/functional/query-builder/locking/entity/Image.ts[1-15]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new test doesn’t verify that the nested joined table (alias `images`) also receives `WITH (NOLOCK)`, despite the test name stating "every table".
### Issue Context
The query includes `.innerJoinAndSelect("categories.images", "images")`, so the SQL should include a fragment like `"image" "images" WITH (NOLOCK)` for MSSQL dirty_read.
### Fix Focus Areas
- test/functional/query-builder/locking/query-builder-locking.test.ts[256-267]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Results up to commit 01db026

🐞 Bugs (2) 📘 Rule violations (0) 📜 Skill insights (0)

1. Test returns inside loop 🐞 Bug ☼ Reliability

Description

The new nested-join lock test uses return inside a for (const dataSource of dataSources) loop,
so if the first DataSource isn’t MSSQL the test exits early and runs zero assertions. This makes the
new regression test order-dependent and can let the MSSQL bug slip through CI when multiple drivers
are enabled.

Code

test/functional/query-builder/locking/query-builder-locking.test.ts[R251-254]

+        for (const dataSource of dataSources) {
+            if (!(dataSource.driver.options.type === "mssql")) {
+                return
+            }

Evidence
dataSources is an array produced by createTestingConnections, so a return inside the loop
aborts the entire test and prevents reaching any later MSSQL DataSource(s). Other functional tests
in the repo use continue to skip unsupported drivers within such loops, demonstrating the intended
control flow.
test/functional/query-builder/locking/query-builder-locking.test.ts[22-30]
test/functional/query-builder/locking/query-builder-locking.test.ts[250-255]
test/functional/query-builder/select/query-builder-select.test.ts[612-616]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The new test returns from the test callback when it encounters a non-MSSQL DataSource. In a multi-driver run, this can skip the MSSQL assertions entirely.

### Issue Context
`createTestingConnections` returns an array of `dataSources`, and this test iterates them. Skipping should be done per-iteration (`continue`) or by filtering to MSSQL sources first.

### Fix Focus Areas
- test/functional/query-builder/locking/query-builder-locking.test.ts[249-267]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. Nested join not fully asserted 🐞 Bug ⚙ Maintainability

Description

The added test claims to ensure the dirty-read lock hint is attached to every table in the nested
join, but it only asserts the hint on the first joined table and forbids the misplaced trailing hint
pattern. It does not assert that the deeper joined table alias ("images") also has WITH (NOLOCK),
so a partial regression could still pass.

Code

test/functional/query-builder/locking/query-builder-locking.test.ts[R263-266]

+            expect(sql).to.contain(
+                'INNER JOIN ("category" "categories" WITH (NOLOCK)',
+            )
+            expect(sql).to.not.contain(") WITH (NOLOCK)")

Evidence

The test builds a query that joins post.categories and then categories.images, but the
expectations only check the categories join fragment and the absence of ) WITH (NOLOCK). The
Image entity exists and (by default naming) corresponds to the "image" table name, so asserting
the images join lock hint is feasible and would match the test’s stated intent.

test/functional/query-builder/locking/query-builder-locking.test.ts[256-267]
test/functional/query-builder/locking/entity/Image.ts[1-15]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The new test doesn’t verify that the nested joined table (alias `images`) also receives `WITH (NOLOCK)`, despite the test name stating "every table".

### Issue Context
The query includes `.innerJoinAndSelect("categories.images", "images")`, so the SQL should include a fragment like `"image" "images" WITH (NOLOCK)` for MSSQL dirty_read.

### Fix Focus Areas
- test/functional/query-builder/locking/query-builder-locking.test.ts[256-267]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

qodo-free-for-open-source-projects · 2026-08-07T19:04:22Z

Code Review by Qodo

New Review Started

This review has been superseded by a new analysis

…n in nested join lock test

pkg-pr-new · 2026-08-26T08:46:15Z

typeorm-sql-js-example
```
npm i https://pkg.pr.new/typeorm@12765
```

commit: be4b77a

gioboa

Thanks for your fix @lazerg

alumni · 2026-08-26T08:51:15Z

DO NOT MERGE (yet)

Need to check if it works for other databases. This might be an SqlServer syntax, but for other databases the place where the hint was added before this PR could have been correct.

lazerg · 2026-08-26T09:01:39Z

I checked this across all dialects.

That call site only emits createTableLockExpression(), and that method returns a non-empty string for mssql only (SelectQueryBuilder.ts L2755-L2767). It has been MSSQL-only since it was added in #8507. Every other dialect builds its lock in createLockExpression() (L2773), which is appended once at the end of the whole query: FOR UPDATE, FOR SHARE, LOCK IN SHARE MODE, FOR SHARE LOCK, FOR NO KEY UPDATE, FOR KEY SHARE. Join position has no effect there. useIndex is also unrelated, because it is written into the FROM clause and not into joins, and that code is untouched.

To make sure I did not miss a path, I generated the SQL for a two-level nested join with all five lock modes, on the old code and on the new code, for 10 driver types: mssql, mysql, mariadb, postgres, cockroachdb, oracle, sap, better-sqlite3, aurora-mysql, spanner. That is 50 queries per side. The two sets are identical except for the three MSSQL lock modes.

Samples that the patch does not change:

postgres, pessimistic_write:

... INNER JOIN ("category" "categories" INNER JOIN "category_images_image" "categories_images" ON ... INNER JOIN "image" "images" ON ...) ON ... FOR UPDATE

mysql, pessimistic_read:

... INNER JOIN (`category` `categories` INNER JOIN `category_images_image` `categories_images` ON ... INNER JOIN `image` `images` ON ...) ON ... LOCK IN SHARE MODE

oracle and sap put FOR UPDATE and FOR SHARE LOCK in that same trailing position. better-sqlite3 and spanner throw LockNotSupportedOnGivenDriverError before any SQL is built.

MSSQL, dirty_read, before:

... INNER JOIN ("category" "categories" INNER JOIN "category_images_image" "categories_images" WITH (NOLOCK) ON ... INNER JOIN "image" "images" WITH (NOLOCK) ON ...) WITH (NOLOCK) ON ...

after:

... INNER JOIN ("category" "categories" WITH (NOLOCK) INNER JOIN "category_images_image" "categories_images" WITH (NOLOCK) ON ... INNER JOIN "image" "images" WITH (NOLOCK) ON ...) ON ...

The old form drops the hint from category and puts it after the closing bracket, which SQL Server rejects.

The non-MSSQL results above come from generated SQL only, so no server ran them. The MSSQL case ran on a real SQL Server container: the new test fails on master and passes with this patch.

github-actions Bot added the linked-issue PR references an issue label Aug 7, 2026

lazerg force-pushed the fix/12764-mssql-nested-join-lock-hint branch from 01db026 to 5102231 Compare August 7, 2026 19:04

fix(mssql): place table lock hint inside nested join parentheses

cd4e0bd

lazerg force-pushed the fix/12764-mssql-nested-join-lock-hint branch from 5102231 to cd4e0bd Compare August 7, 2026 19:06

fix(mssql): skip non-mssql datasources with continue instead of retur…

be4b77a

…n in nested join lock test

alumni approved these changes Aug 26, 2026

View reviewed changes

gioboa approved these changes Aug 26, 2026

View reviewed changes

Navigation Menu

Uh oh!

Uh oh!

fix(mssql): place table lock hint inside nested join parentheses - #12765

fix(mssql): place table lock hint inside nested join parentheses#12765
lazerg wants to merge 2 commits into
typeorm:masterfrom
lazerg:fix/12764-mssql-nested-join-lock-hint

lazerg commented Aug 7, 2026 •

edited

Loading

Uh oh!

qodo-free-for-open-source-projects Bot commented Aug 7, 2026 •

edited

Loading

Uh oh!

qodo-free-for-open-source-projects Bot commented Aug 7, 2026 •

edited

Loading

Uh oh!

pkg-pr-new Bot commented Aug 26, 2026

Uh oh!

gioboa left a comment

Uh oh!

alumni commented Aug 26, 2026 •

edited

Loading

Uh oh!

lazerg commented Aug 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Uh oh!

Uh oh!

Conversation

lazerg commented Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of change

Pull-Request Checklist

Uh oh!

qodo-free-for-open-source-projects Bot commented Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Previous review results

Uh oh!

qodo-free-for-open-source-projects Bot commented Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

New Review Started

Uh oh!

pkg-pr-new Bot commented Aug 26, 2026

Uh oh!

gioboa left a comment

Choose a reason for hiding this comment

Uh oh!

alumni commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lazerg commented Aug 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

lazerg commented Aug 7, 2026 •

edited

Loading

qodo-free-for-open-source-projects Bot commented Aug 7, 2026 •

edited

Loading

qodo-free-for-open-source-projects Bot commented Aug 7, 2026 •

edited

Loading

alumni commented Aug 26, 2026 •

edited

Loading