fix(query-builder): return an empty result set for take(0) by Develop-KIM · Pull Request #12763 · typeorm/typeorm

Develop-KIM · 2026-08-07T05:49:37Z

Replaces #12685. I closed that one on Aug 6 while cutting down the number of repos I had open PRs in, then settled on a shorter list that TypeORM is on. I'd deleted my fork in the same pass, so GitHub wouldn't let me reopen it.

Description of change

take(0) combined with a join (e.g. leftJoinAndSelect) returns every row instead of an empty result, and getManyAndCount reports all of them as the page.

The join pagination branch in SelectQueryBuilder is guarded by (this.expressionMap.skip || this.expressionMap.take). With take(0), take is 0 (falsy), so the branch is skipped and execution falls through to the plain path — which only copies skip/take into limit/offset when there are no joins. With a join present no row limit is emitted at all, so everything comes back.

executeEntitiesAndRawResults now returns an empty result straight away when take is 0, before the query is built: a take of 0 asks for no rows, so there is nothing to ask the database for.

I first fixed this by making the pagination guard treat 0 as a set value, but that pushes a zero row count down to the driver, and both SQL Server (Msg 10744) and Oracle reject that in FETCH NEXT ... ROWS ONLY. The no-join path already has the same problem today — take(0) without a join emits exactly that — which has gone unnoticed because the existing take(0) tests in query-builder-select.test.ts are better-sqlite3-only. Short-circuiting fixes both paths on every driver instead of extending the breakage to joins.

An explicit limit() keeps its precedence over take(), the way createLimitOffsetExpression() has always applied it, so .limit(5).take(0) still returns up to five rows and only take(0) on its own short-circuits. .offset(10).take(0) does change: it now returns nothing where it used to return every row past the offset, since offset is not a competing row count and that read as the same bug rather than a precedence rule.

getManyAndCount still reports the true total: lazyCount falls back to a real count query when the result set is as large as the requested page, which 0 === 0 satisfies.

How I verified

Added test/github-issues/12666, running on every driver except spanner. On master the take(0) assertions fail (3 rows instead of 0) and so does the limit() precedence one (0 rows instead of 2); with the fix all four pass, and skip(0) with a join still returns every row. Full sqlite suite: 3028 passing, 0 failing — one Redis query-cache test errors locally because I have no working Docker here. I have no SQL Server or Oracle instance available, so the zero-row-count behaviour of those two is from their docs, not from a run.

Fixes #12666

AI-assisted: drafted with Claude, reviewed and verified by me.

Pull-Request Checklist

Code is up-to-date with the master branch
This pull request links a relevant issue using a closing keyword: Fixes #12666
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

github-actions · 2026-08-07T05:49:53Z

Other open PRs also reference #12666: #12687,#12722. Maintainers may want to coordinate.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2f566813a2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-07T05:53:01Z

+            ((this.expressionMap.take !== undefined &&
+                this.expressionMap.take !== null) ||


Avoid issuing FETCH NEXT 0 on SQL Server

When take(0) is used with a join on MSSQL, this newly entered branch passes 0 to the pagination builder's .limit(). The MSSQL renderer emits OFFSET 0 ROWS FETCH NEXT 0 ROWS ONLY, but SQL Server rejects a FETCH row count of zero with error 10744, so the query throws instead of returning an empty array. The added all-driver suite runs in the inspected MSSQL workflow because it disables only Spanner, so the MSSQL CI job and production MSSQL usage remain broken; handle take === 0 without sending an invalid OFFSET/FETCH query.

Useful? React with 👍 / 👎.

qodo-free-for-open-source-projects · 2026-08-07T05:53:07Z

Code Review by Qodo

🐞 Bugs (7) 📘 Rule violations (3) 📜 Skill insights (0)

1. take(0) behavior undocumented 📘 Rule violation ⚙ Maintainability

Description

The PR changes public QueryBuilder pagination semantics by making take(0) return no entities while
preserving explicit limit() precedence, but the pagination documentation is not updated. Users
cannot discover these new edge-case semantics from the documented API behavior.

Code

src/query-builder/SelectQueryBuilder.ts[R1924-1927]

+    private takesNoRows(): boolean {
+        return (
+            this.expressionMap.take === 0 &&
+            this.expressionMap.limit === undefined

Evidence
Compliance rule 2 requires documentation or samples for user-facing changes. The new takesNoRows()
logic defines externally observable take(0) and limit() precedence, while the existing
pagination documentation discusses take() and joined queries without documenting these semantics.
Rule 2: Docs updated for user-facing changes
src/query-builder/SelectQueryBuilder.ts[1920-1928]
docs/docs/query-builder/1-select-query-builder.md[870-912]

Agent prompt

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

## Issue description
Document the new public behavior of `take(0)` in QueryBuilder pagination.
## Issue Context
Explain that `take(0)` returns an empty entity result, including for joined queries, and clarify the existing precedence of an explicitly configured `limit()`.
## Fix Focus Areas
- docs/docs/query-builder/1-select-query-builder.md[870-912]

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

2. Writable CTE executed twice 🐞 Bug ≡ Correctness

Description

For take(0) with a writable CTE, executeEntitiesAndRawResults now explicitly executes the statement
via clone().limit(0).loadRawResults for side effects, but getManyAndCount will still run
executeCountQuery (because lazyCount treats 0===0 as needing a real count). Since executeCountQuery
clones the original query without clearing commonTableExpressions, the writable CTE can execute a
second time, duplicating side effects like INSERTs.

Code

src/query-builder/SelectQueryBuilder.ts[R3491-3494]

+                    entities: [],
+                    raw: await this.clone()
+                        .limit(0)
+                        .loadRawResults(queryRunner),

Evidence
The new take(0) branch runs clone().limit(0).loadRawResults for writable-CTE drivers.
getManyAndCount then calls lazyCount, which returns undefined when entities length equals maxResults
(0===0), forcing executeCountQuery. executeCountQuery uses clone().loadRawResults without clearing
commonTableExpressions; QueryExpressionMap.clone copies CTEs, so the writable CTE can be executed
again.
src/query-builder/SelectQueryBuilder.ts[3474-3495]
src/query-builder/SelectQueryBuilder.ts[1862-1893]
src/query-builder/SelectQueryBuilder.ts[1931-1944]
src/query-builder/SelectQueryBuilder.ts[3163-3178]
src/query-builder/QueryExpressionMap.ts[538-547]

Agent prompt

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

## Issue description
With `take(0)` + writable CTE + `getManyAndCount()`, the writable CTE can execute twice:
1) once in the new `take===0` CTE exception path (`clone().limit(0).loadRawResults()`), and
2) again in the subsequent count query (`executeCountQuery()`), because the count query clone retains the same CTEs.
This can duplicate data modifications (e.g., insert the same row twice).
### Issue Context
`lazyCount()` forces a real count when `entities.length === maxResults`; for `take(0)` this becomes `0 === 0`, so `getManyAndCount()` predictably executes a count query.
### Fix Focus Areas
- src/query-builder/SelectQueryBuilder.ts[3474-3495]
- src/query-builder/SelectQueryBuilder.ts[1862-1893]
- src/query-builder/SelectQueryBuilder.ts[1931-1944]
- src/query-builder/SelectQueryBuilder.ts[3163-3178]
- src/query-builder/QueryExpressionMap.ts[538-547]
### Proposed fix
Implement a `getManyAndCount()`-specific short-circuit for `take(0)` to ensure only **one** SQL execution occurs when writable CTEs are present.
One workable approach:
- In `getManyAndCount()`, after transaction setup, add:
- If `this.expressionMap.take === 0 && this.expressionMap.limit === undefined`:
 - Set `this.expressionMap.queryEntity = false`.
 - Execute `count = await this.executeCountQuery(queryRunner)`.
 - Return `[[], count]` **without calling** `executeEntitiesAndRawResults()`.
This makes the writable CTE run once (as part of the count query) instead of potentially twice.
Add a regression test:
- Use a writable CTE that inserts a row, then call `.take(0).getManyAndCount()` and assert the inserted row count is exactly 1 (not 2).

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

3. Raw take(0) still returns rows 🐞 Bug ≡ Correctness

Description

take(0) now short-circuits only executeEntitiesAndRawResults, so
getRawMany/getRawOne/stream still execute SQL; when joins are present, SQL generation does not
apply take as a LIMIT/OFFSET, so these raw APIs can still return all rows. This introduces an
inconsistency where getMany() returns [] for the same query while getRawMany() returns data.

Code

src/query-builder/SelectQueryBuilder.ts[R3474-3477]

+        // a take of 0 asks for no rows at all, so there is nothing to query;
+        // going to the database instead would emit a zero row limit, which
+        // SQL Server and Oracle reject, and which joins silently ignore
+        if (this.expressionMap.take === 0) return { entities: [], raw: [] }

Evidence
The PR adds an early return only inside executeEntitiesAndRawResults(). Raw APIs call
loadRawResults()/stream() directly, and getQuery() always appends
createLimitOffsetExpression(), which only maps skip/take into limit/offset when there are no
joins—so joined raw queries can ignore take(0) and return rows.
src/query-builder/SelectQueryBuilder.ts[3474-3477]
src/query-builder/SelectQueryBuilder.ts[1615-1633]
src/query-builder/SelectQueryBuilder.ts[1974-1999]
src/query-builder/SelectQueryBuilder.ts[84-95]
src/query-builder/SelectQueryBuilder.ts[2665-2677]
src/query-builder/SelectQueryBuilder.ts[3855-3903]

Agent prompt

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

## Issue description
`take(0)` is now handled only in `executeEntitiesAndRawResults()`, which affects entity-returning APIs (`getMany`, `getManyAndCount`, etc.). Raw-returning APIs (`getRawMany`/`getRawOne`) and `stream()` bypass that method and still execute SQL.
When joins are present, `createLimitOffsetExpression()` only maps `skip/take` into `offset/limit` when `joinAttributes.length === 0`, so raw SQL for joined queries can be unbounded even with `take(0)`. This means `take(0)` can still return rows via raw APIs, while entity APIs now return `[]`.
## Issue Context
- Entity path now returns early for `take === 0`.
- Raw path uses `loadRawResults()` / `stream()` which call `getQueryAndParameters()` and execute SQL.
- SQL LIMIT/OFFSET generation ignores `skip/take` when joins exist.
## Fix Focus Areas
- src/query-builder/SelectQueryBuilder.ts[3474-3477]
- src/query-builder/SelectQueryBuilder.ts[1615-1633]
- src/query-builder/SelectQueryBuilder.ts[1974-1999]
- src/query-builder/SelectQueryBuilder.ts[2665-2677]
- src/query-builder/SelectQueryBuilder.ts[3855-3903]
## Proposed fix
1. Add a consistent short-circuit for `this.expressionMap.take === 0` in the raw execution path:
- Preferably at the top of `loadRawResults(queryRunner)` so it covers `getRawMany()` and all internal raw executions.
- Also handle `stream()` explicitly (since it does not go through `loadRawResults`).
2. Ensure `getRawOne()` behavior follows from `getRawMany()` (it will then return `undefined`).
3. Add/extend tests for `take(0)` with joins using `getRawMany()` (and optionally `stream()` if there is an existing pattern for stream tests) to prevent future regressions.
## Acceptance criteria
- For the same query with joins + `take(0)`, `getMany()` and `getRawMany()` both return empty results.
- `getRawOne()` returns `undefined` for `take(0)`.
- `stream()` does not stream any rows for `take(0)`.

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

4. Alias validation is bypassed 🐞 Bug ≡ Correctness ⭐ New

Description

For .take(0).getManyAndCount(), the new shortcut bypasses executeEntitiesAndRawResults() without
performing its mainAlias check; executeCountQuery() then dereferences mainAlias! and throws a
generic TypeError instead of the established TypeORMError. This makes the same malformed builder
fail differently solely because take(0) was added.

Code

src/query-builder/SelectQueryBuilder.ts[1880]
+            if (this.takesNoRows()) {

Evidence
The shortcut returns empty entity/raw arrays without entering the normal executor. That executor
explicitly rejects a missing alias, whereas the subsequent count path calls code that directly
accesses mainAlias.name and mainAlias.metadata, proving that an alias-less take(0) query now
reaches an unsafe dereference.
src/query-builder/SelectQueryBuilder.ts[1878-1901]
src/query-builder/SelectQueryBuilder.ts[3491-3499]
src/query-builder/SelectQueryBuilder.ts[3087-3091]
src/query-builder/SelectQueryBuilder.ts[3203-3217]

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 `take(0)` shortcut in `getManyAndCount()` skips the normal main-alias validation. A builder without `from()` reaches count generation and crashes while dereferencing an undefined alias rather than throwing the established `TypeORMError`.

## Issue Context
`executeEntitiesAndRawResults()` validates `expressionMap.mainAlias` before all other entity-query work, while `computeCountExpression()` assumes it exists via non-null assertions. Apply equivalent validation before bypassing entity execution, preferably through a shared helper to avoid validation drift.

## Fix Focus Areas
- src/query-builder/SelectQueryBuilder.ts[1880-1886]
- src/query-builder/SelectQueryBuilder.ts[3494-3497]
- src/query-builder/SelectQueryBuilder.ts[3087-3091]

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

5. Lock validation bypassed 🐞 Bug ≡ Correctness

Description

getManyAndCount() bypasses executeEntitiesAndRawResults() for take(0), skipping the
active-transaction validation required for pessimistic locks. The query proceeds to count execution
instead of consistently throwing PessimisticLockTransactionRequiredError.

Code

src/query-builder/SelectQueryBuilder.ts[R1882-1884]

+            const entitiesAndRaw = this.takesNoRows()
+                ? { entities: [], raw: [] }
+                : await this.executeEntitiesAndRawResults(queryRunner)

Evidence
The added conditional replaces entity execution with an in-memory empty result. The required
pessimistic-lock transaction check exists inside the skipped method, while executeCountQuery
directly clones and loads raw results without performing that validation.
src/query-builder/SelectQueryBuilder.ts[1878-1895]
src/query-builder/SelectQueryBuilder.ts[3474-3481]
src/query-builder/SelectQueryBuilder.ts[3178-3192]

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 `getManyAndCount()` take-zero shortcut skips the pessimistic-lock transaction validation performed by `executeEntitiesAndRawResults()`. Ensure take-zero queries still apply the same lock validation as other entity queries before bypassing database execution.
## Issue Context
The normal execution path rejects pessimistic locks outside an active transaction, but the newly added shortcut avoids that path and proceeds directly to counting.
## Fix Focus Areas
- src/query-builder/SelectQueryBuilder.ts[1878-1885]
- src/query-builder/SelectQueryBuilder.ts[3474-3481]

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

6. Null limit defeats take 🐞 Bug ≡ Correctness

Description

takesNoRows() treats limit(null) as an explicit competing limit even though the numeric setter
accepts null and the surrounding pagination logic treats null as no effective limit. Consequently,
.limit(null).take(0) bypasses the shortcut and can return every matching row instead of none.

Code

src/query-builder/SelectQueryBuilder.ts[R1925-1928]

+        return (
+            this.expressionMap.take === 0 &&
+            this.expressionMap.limit === undefined
+        )

Evidence
normalizeNumber() preserves null and validation returns it, making null an accepted runtime value.
lazyCount() explicitly excludes both null and undefined from effective limits, but takesNoRows()
excludes only undefined; SQL generation then emits neither the null limit nor take(0), allowing rows
through.
src/query-builder/QueryBuilder.ts[1760-1783]
src/query-builder/SelectQueryBuilder.ts[1924-1951]
src/query-builder/SelectQueryBuilder.ts[2683-2696]
src/query-builder/SelectQueryBuilder.ts[3525-3531]

Agent prompt

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

## Issue description
Update the take-zero predicate so a null limit is treated consistently with the rest of pagination logic. Add coverage for `.limit(null).take(0)` through the runtime JavaScript-compatible API behavior.
## Issue Context
Numeric input normalization explicitly accepts null, and both `lazyCount()` and SQL limit emission regard null as no effective limit. The new predicate checks only for undefined.
## Fix Focus Areas
- src/query-builder/SelectQueryBuilder.ts[1924-1928]
- src/query-builder/SelectQueryBuilder.ts[1935-1951]
- src/query-builder/QueryBuilder.ts[1760-1783]
- test/github-issues/12666/issue-12666.test.ts[85-97]

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

View review recommended (3)

7. Verbose comments in take(0) 📘 Rule violation ⚙ Maintainability

Description

The new take(0) short-circuit adds a large, narrative comment block that reads like AI-generated
prose and is atypically verbose for production code. This increases maintenance overhead and risks
drifting from actual behavior over time.

Code

src/query-builder/SelectQueryBuilder.ts[R3482-3485]

+            // a data-modifying CTE runs for its side effects even when the outer
+            // query returns nothing, so a query carrying one still has to be sent.
+            // a zero limit is enough to send it and keep no rows, and the only
+            // drivers that accept a writable CTE also accept that limit

Evidence

PR Compliance ID 4 requires avoiding AI-generated noise, including extra comments and style
inconsistent with the file. The added block contains several long, explanatory comment paragraphs in
the new take(0) logic, which is not necessary to understand the code at a glance and is likely to
become stale.

Rule 4: Remove AI-generated noise
src/query-builder/SelectQueryBuilder.ts[3482-3485]

Agent prompt

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

## Issue description
A new `take(0)` early-return block introduces a long multi-line narrative comment that appears AI-generated and is unusually verbose for this codebase.
## Issue Context
PR Compliance requires avoiding AI-generated noise (extra comments / abnormal defensive additions). The implementation can remain, but the commentary should be tightened and/or moved to a more appropriate place (e.g., a short summary comment with a link to the issue).
## Fix Focus Areas
- src/query-builder/SelectQueryBuilder.ts[3474-3496]

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

8. Read-only CTE still queried 🐞 Bug ➹ Performance

Description

In executeEntitiesAndRawResults, take(0) will still send a LIMIT 0 query for any CTE as long as the
driver supports writable CTEs, even if all CTEs are read-only. This defeats the take(0)
short-circuit and can unnecessarily execute/validate complex CTE SQL (and potentially fail) despite
requesting zero rows.

Code

src/query-builder/SelectQueryBuilder.ts[R3486-3489]

+            if (
+                this.hasCommonTableExpressions() &&
+                this.dataSource.driver.cteCapabilities.writable
+            ) {

Evidence

The new take(0) block executes a query whenever there are any CTEs and the driver advertises
writable CTE capability, without checking whether any registered CTE is actually data-modifying.
QueryBuilder’s CTE building logic shows that writable-capable drivers may still have select-only
CTEs, so capability alone is not enough to justify executing SQL for take(0).

src/query-builder/SelectQueryBuilder.ts[3474-3495]
src/query-builder/QueryBuilder.ts[1193-1221]

Agent prompt

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

## Issue description
`take(0)` is intended to short-circuit and avoid DB access, but the current exception path runs a DB query whenever *any* CTE exists on a driver that *supports* writable CTEs. This should only happen when there is actually a **data-modifying** CTE (or an uninspectable raw-string CTE that could be modifying).
### Issue Context
The current condition checks `hasCommonTableExpressions()` plus `driver.cteCapabilities.writable`, which indicates capability, not whether the registered CTEs are insert/update/delete.
### Fix Focus Areas
- src/query-builder/SelectQueryBuilder.ts[3474-3495]
- src/query-builder/QueryBuilder.ts[1193-1221]
### Proposed fix
- Inspect `this.expressionMap.commonTableExpressions` and only take the “send LIMIT 0 query” path when at least one CTE is data-modifying (e.g., `queryBuilder` is a `QueryBuilder` but **not** a `SelectQueryBuilder`).
- If any CTE is provided as a raw `string`, consider conservatively treating it as potentially data-modifying (keep the current behavior for those), since it can’t be type-checked.
- Otherwise (all CTEs are select CTEs), return `{ entities: [], raw: [] }` without hitting the DB.
- Add a regression test: `take(0)` + a **select-only** CTE on a writable-CTE driver should not execute a query (can be asserted via query runner mocks or by verifying no CTE side effects/counters if such harness exists).

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

9. Issue test added under github-issues 📘 Rule violation ⚙ Maintainability

Description

The fix for #12666 is validated only via a new per-issue test under test/github-issues/12666,
rather than being added to the functional test suite. This increases long-term test suite
fragmentation and violates the preference to keep issue fixes in test/functional.

Code

test/github-issues/12666/issue-12666.test.ts[R1-4]
+import "reflect-metadata"
+import {
+    createTestingConnections,
+    closeTestingConnections,

Evidence

PR Compliance ID 3 requires issue-fix tests to be added/updated in test/functional, and flags
adding tests only under test/github-issues without a clear reason. This PR introduces a new test
file located at test/github-issues/12666/issue-12666.test.ts, indicating the fix is covered
exclusively via a per-issue test location.

Rule 3: Prefer functional tests over per-issue tests
test/github-issues/12666/issue-12666.test.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 PR adds coverage for the join + `take(0)` bug only under `test/github-issues/12666`, but the compliance checklist requires issue fixes to live in the functional test suite where feasible.
## Issue Context
This is a query-builder behavior regression test that appears broadly applicable and should be maintained alongside other functional query-builder tests.
## Fix Focus Areas
- test/github-issues/12666/issue-12666.test.ts[1-98]
- test/github-issues/12666/entity/Post.ts[1-21]
- test/github-issues/12666/entity/Category.ts[1-10]

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

10. Negative skip ignored joins 🐞 Bug ≡ Correctness

Description

When joins are present and take is unset, the new pagination guard only treats skip as set when
skip > 0, so skip(-1) bypasses the join-pagination path and emits no OFFSET/LIMIT, returning
unpaginated results. Since skip() accepts negative numbers (it only rejects NaN), this is an
inconsistent behavior change vs no-join queries (which still render OFFSET -1) and can mask invalid
caller input by returning all rows.

Code

src/query-builder/SelectQueryBuilder.ts[R3495-3499]

+            ((this.expressionMap.take !== undefined &&
+                this.expressionMap.take !== null) ||
+                (this.expressionMap.skip !== undefined &&
+                    this.expressionMap.skip !== null &&
+                    this.expressionMap.skip > 0)) &&

Evidence
The new guard requires skip > 0 (unless take is set), and if it does not pass, joined queries
fall back to loadRawResults() where skip/take are not translated into offset/limit when joins
exist. Since skip() uses validateNumericInput() which only rejects NaN (not negatives), a
negative skip remains possible and is now ignored for join queries.
src/query-builder/SelectQueryBuilder.ts[3491-3501]
src/query-builder/SelectQueryBuilder.ts[3625-3627]
src/query-builder/SelectQueryBuilder.ts[2665-2677]
src/query-builder/SelectQueryBuilder.ts[1516-1528]
src/query-builder/QueryBuilder.ts[1760-1784]

Agent prompt

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

## Issue description
With joins present, the join-pagination branch is now gated by `skip > 0` (unless `take` is set), which means `skip(<0)` is treated as “unset” and ignored entirely for joined queries. Because `skip()` currently allows negative numbers, this can cause `skip(-1)` to return all rows instead of failing fast.
### Issue Context
This PR intentionally changed the guard to treat `take(0)` as a meaningful limit and `skip(0)` as a no-op. The remaining inconsistency is specifically around **negative** pagination inputs.
### Fix Focus Areas
- src/query-builder/SelectQueryBuilder.ts[3488-3501]
- src/query-builder/SelectQueryBuilder.ts[1516-1528]
- src/query-builder/SelectQueryBuilder.ts[2665-2677]
- src/query-builder/QueryBuilder.ts[1760-1784]
### Suggested fix approach
- Add validation to reject negative values for pagination parameters (at least `skip`/`take`, potentially also `offset`/`limit`), e.g. throw a `TypeORMError` when the normalized value is `< 0`.
- Alternatively (less strict), adjust the join-pagination guard to treat any non-zero `skip` (including negative) as “set” to preserve previous behavior; but rejecting negatives is safer and avoids silently returning unpaginated results.

ⓘ 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.

Tip of the day

💡 Did you know, you can hide the parts of a finding you never read, like the evidence or the agent prompt

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Develop-KIM · 2026-08-10T05:16:41Z

The Codex note about SQL Server was right, so I've reworked the fix.

My first version made the join pagination branch treat take(0) as a set limit, which ends up as OFFSET 0 ROWS FETCH NEXT 0 ROWS ONLY on mssql — SQL Server rejects a zero row count there (Msg 10744), and Oracle rejects it too. Digging into it, the no-join path already emits exactly that for take(0) today, so take(0) is broken on both databases with or without a join. It's gone unnoticed because the existing take(0) tests in query-builder-select.test.ts are enabledDrivers: ["better-sqlite3"], and my new test would have been the first one to run it against mssql in CI.

So rather than pushing a zero limit down to the driver, executeEntitiesAndRawResults now returns an empty result before the query is built — a take of 0 asks for no rows, so there's nothing to ask the database for. The pagination guard goes back to what it is on master, the diff is smaller, and take(0) is fixed on mssql and oracle as well. getManyAndCount is unaffected: lazyCount still falls back to a real count query when the page is full, which 0 === 0 satisfies, so the count is the full row count.

Full sqlite suite is green (3027 passing). I don't have SQL Server or Oracle here, so that part is reasoned from their docs rather than run — if you'd rather I keep the driver list narrower on the new test until CI confirms it, happy to do that.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: eb213c0022

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-10T05:18:56Z

+        // a take of 0 asks for no rows at all, so there is nothing to query;
+        // going to the database instead would emit a zero row limit, which
+        // SQL Server and Oracle reject, and which joins silently ignore
+        if (this.expressionMap.take === 0) return { entities: [], raw: [] }


Honor explicit limit before short-circuiting take(0)

When a no-join builder combines an explicit limit with take(0), such as .limit(5).take(0), createLimitOffsetExpression() intentionally uses the explicit limit and ignores take; lazyCount() likewise documents that precedence. This unconditional return instead changes the query to produce no rows, and getManyAndCount() can now return [[], 0] even when matching data exists. Only short-circuit when take(0) is the effective limit rather than merely present.

Useful? React with 👍 / 👎.

Right again — thanks. .limit(5).take(0) was losing its five rows, since the short-circuit fired on take being present rather than on it being the row limit that would actually be used.

The guard now also requires limit to be unset, so an explicit limit() keeps the precedence it has had in createLimitOffsetExpression() all along, and a plain take(0) still short-circuits. One deliberate difference: .offset(10).take(0) now returns nothing where it used to return every row past the offset. offset is not a competing row count, so that looked like the same bug as #12666 rather than a precedence rule worth keeping.

Added a test for the limit() case; the full suite is green.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5a7a8556f9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-14T04:58:44Z

+            this.expressionMap.take === 0 &&
+            this.expressionMap.limit === undefined
+        )
+            return { entities: [], raw: [] }


Execute writable CTEs before short-circuiting take(0)

When a PostgreSQL or CockroachDB entity query includes an INSERT/UPDATE/DELETE through addCommonTableExpression(...), take(0) previously executed the statement and merely limited the outer SELECT; writable CTEs still run to completion even when the outer query returns no rows. This return occurs before loadRawResults, so the supported writable CTE is now silently skipped. Restrict the shortcut when writable CTEs are present or otherwise execute the statement while discarding its result rows.

Useful? React with 👍 / 👎.

Third one that was worth listening to — thanks.

A data-modifying CTE runs to completion whether or not the outer query reads it, so returning early was quietly dropping the write. Those queries now go out with an explicit limit(0) instead of short-circuiting, so the CTE still executes and the caller still gets nothing back. I gated it on cteCapabilities.writable, since Postgres and CockroachDB are the only drivers that accept a writable CTE in the first place, and both are happy with LIMIT 0 — the drivers that reject a zero row count never reach that branch. Going through limit rather than take also matters here: the join path ignores a bare take of 0, which is the original bug.

Added a test that inserts through addCommonTableExpression alongside take(0) and then counts the row; it fails on the previous commit and passes now. It only runs where writable CTEs exist. Verified against a local Postgres 17 — full suite is green apart from the postgis/pgvector/redis cases my machine can't run.

Also rebased on master while I was here.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9513a83281

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-18T05:32:14Z

+                    raw: await this.clone()
+                        .limit(0)
+                        .loadRawResults(queryRunner),


Avoid executing writable CTEs twice in getManyAndCount

When a joined take(0) query with a data-modifying CTE is executed through getManyAndCount(), this call runs the CTE once and returns zero entities; lazyCount() then returns undefined because the entity count equals the requested take (0 === 0), causing executeCountQuery() to clone the original expression map—including the same CTE—and run it again. An INSERT CTE can therefore create duplicate rows or fail on a uniqueness constraint, while an UPDATE can apply its mutation twice; the count query needs to exclude the already-executed writable CTE.

Useful? React with 👍 / 👎.

Fourth one worth listening to.

The count query does re-run the CTE, and I couldn't see a way to strip it there — an outer query is allowed to select from its own CTE, so dropping it from the count would break the count. So I went at it from the other end: getManyAndCount no longer runs the entity query at all when take is 0. There is nothing for it to fetch, and the count query it already runs carries the same CTE, so the write happens exactly once.

Measured on Postgres — three posts, a CTE that inserts one category, then counting the rows it inserted:

getManyAndCount() main previous commit now

take(0), no join 2 2 1

take(0) + leftJoinAndSelect 1, but all 3 rows came back (the bug) 2 1

So the double execution was already there on main for take(0) without a join: that path emits LIMIT 0, gets nothing back, and lazyCount then has to count for real. The join path never reached it because the take was being ignored. Both paths now send one statement instead of two.

Added a test that inserts through addCommonTableExpression alongside take(0), calls getManyAndCount, and asserts one inserted row and a count of 3; it fails on the previous commit with two rows. Full suite is green apart from the seven pgvector/postgis/redis failures my machine has without those extensions.

One thing I ran into while measuring that I have not touched: a writable CTE combined with a join and a non-zero take (take(2)) fails with syntax error at or near ":" on main as well — the join pagination sub-query doesn't seem to carry the CTE parameters. It reproduces without this PR, so I left it alone.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 85cee460a0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-20T05:42:48Z

+            const entitiesAndRaw = this.takesNoRows()
+                ? { entities: [], raw: [] }
+                : await this.executeEntitiesAndRawResults(queryRunner)


Run lock validation before the zero-take shortcut

When getManyAndCount() is called with a pessimistic lock outside an active transaction and take(0), this shortcut bypasses the PessimisticLockTransactionRequiredError check in executeEntitiesAndRawResults(). Execution then proceeds to the count query, which can either issue an invalid lock-bearing aggregate or succeed under an autocommit lock depending on the driver, rather than consistently enforcing TypeORM's transaction requirement; validate the lock before returning the synthetic empty entity result.

Useful? React with 👍 / 👎.

Fifth one, and right again.

getManyAndCount is the only caller that skips executeEntitiesAndRawResults for take(0), and the pessimistic lock guard lived inside that method — so a locked query outside a transaction stopped raising PessimisticLockTransactionRequiredError and went on to the count query instead. On Postgres that comes back as FOR UPDATE is not allowed with aggregate functions, which is a far worse way to find out.

I pulled the guard out into assertPessimisticLockIsInTransaction and call it from both places, so the short-circuit rejects exactly where the full path does.

Inside a transaction nothing changes: take(0) asks for no rows, and the zero-limit query the no-join path used to send locked nothing either.

Added a test that runs setLock("pessimistic_write").take(0).getManyAndCount() outside a transaction. It fails on the previous commit with the Postgres error above and passes now; it skips sqlite and spanner, the same way the existing locking tests do. Full suite is green here apart from the pgvector/postgis/Docker failures I get on main too.

`take(0)` combined with a join (e.g. `leftJoinAndSelect`) returned every row. The join pagination branch is guarded by `(skip || take)`, so a take of `0` is falsy and the branch is skipped; execution then falls through to the plain path, which only copies skip/take into limit/offset when there are no joins, so no row limit is emitted at all. Rather than teaching that guard about `0`, short-circuit before the query is built: a take of 0 asks for no rows, so there is nothing to ask the database for. That also keeps SQL Server and Oracle out of trouble - both reject a zero row count in `FETCH NEXT ... ROWS ONLY`, which is what the no-join path emits for `take(0)` today. An explicit `limit()` keeps its precedence over `take()`, as it has in `createLimitOffsetExpression()` all along, so `.limit(5).take(0)` still returns up to five rows and only `take(0)` on its own short-circuits. A query that carries a writable CTE is the one case that still has to be sent: a data-modifying CTE runs to completion even when the outer query returns no rows, so short-circuiting would silently swallow the write. Those queries go out with an explicit `limit(0)` instead, which every driver that accepts a writable CTE (Postgres, CockroachDB) also accepts, and which the join path honours where a bare `take` of 0 is ignored. `getManyAndCount` skips the entity query outright for a take of 0. The count still reflects every row, because `lazyCount` falls back to a real count query whenever the result set is as large as the requested page, and since that count query carries the same CTE, a writable one runs exactly once. It ran twice before this change on the no-join path, which already emitted a zero row limit and then counted on top of it. The short-circuit skips `executeEntitiesAndRawResults`, which is where the pessimistic lock guard lives, so that guard moved out into `assertPessimisticLockIsInTransaction` and both paths call it. Without it a locked query outside a transaction stopped raising `PessimisticLockTransactionRequiredError` and reached the count query instead, where Postgres rejects `FOR UPDATE` on an aggregate. Fixes typeorm#12666 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

github-actions Bot added linked-issue PR references an issue possible-duplicate PR may duplicate an existing open PR labels Aug 7, 2026

Develop-KIM mentioned this pull request Aug 7, 2026

fix(query-builder): treat take(0) as a limit when joins are present #12685

Closed

4 tasks

chatgpt-codex-connector Bot reviewed Aug 7, 2026

View reviewed changes

qodo-free-for-open-source-projects Bot added the Compliance violation label Aug 7, 2026

Develop-KIM force-pushed the fix/take-zero-with-join branch from 2f56681 to eb213c0 Compare August 10, 2026 05:15

Develop-KIM changed the title ~~fix(query-builder): treat take(0) as a limit when joins are present~~ fix(query-builder): return an empty result set for take(0) Aug 10, 2026

chatgpt-codex-connector Bot reviewed Aug 10, 2026

View reviewed changes

qodo-free-for-open-source-projects Bot removed the Compliance violation label Aug 10, 2026

Develop-KIM force-pushed the fix/take-zero-with-join branch from eb213c0 to 5a7a855 Compare August 14, 2026 04:56

chatgpt-codex-connector Bot reviewed Aug 14, 2026

View reviewed changes

Develop-KIM force-pushed the fix/take-zero-with-join branch from 5a7a855 to 9513a83 Compare August 18, 2026 05:30

chatgpt-codex-connector Bot reviewed Aug 18, 2026

View reviewed changes

qodo-free-for-open-source-projects Bot added the Compliance violation label Aug 18, 2026

Develop-KIM force-pushed the fix/take-zero-with-join branch from 9513a83 to 85cee46 Compare August 20, 2026 05:39

chatgpt-codex-connector Bot reviewed Aug 20, 2026

View reviewed changes

Develop-KIM force-pushed the fix/take-zero-with-join branch from 85cee46 to 5d596c8 Compare August 25, 2026 06:56

qodo-free-for-open-source-projects Bot removed the Compliance violation label Aug 25, 2026

		((this.expressionMap.take !== undefined &&
		this.expressionMap.take !== null) \|\|

`getManyAndCount()`	main	previous commit	now
`take(0)`, no join	2	2	1
`take(0)` + `leftJoinAndSelect`	1, but all 3 rows came back (the bug)	2	1

Uh oh!

Uh oh!

Conversation

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

Uh oh!

Description of change

How I verified

Pull-Request Checklist

Uh oh!

github-actions Bot commented Aug 7, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 7, 2026

Choose a reason for hiding this comment

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

Uh oh!

Develop-KIM commented Aug 10, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

Develop-KIM Aug 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

Develop-KIM Aug 18, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 18, 2026

Choose a reason for hiding this comment

Uh oh!

Develop-KIM Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

Develop-KIM Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

1 participant

Develop-KIM commented Aug 7, 2026 •

edited

Loading

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

edited

Loading

Develop-KIM Aug 14, 2026 •

edited

Loading