fix: honor take(0) when joins are present - #12687

Description

executeEntitiesAndRawResults now routes joined queries through the pagination subquery when skip
is 0, which can generate an OFFSET without a LIMIT and throw OffsetWithoutLimitNotSupportedError
on MySQL-family/SAP/Spanner. This is triggered via FindOptions as well because applyFindOptions
calls .skip(0) when findOptions.skip is set to 0.

Code

src/query-builder/SelectQueryBuilder.ts[R3492-3496]

       if (
-            (this.expressionMap.skip || this.expressionMap.take) &&
+            (this.expressionMap.skip != null ||
+                this.expressionMap.take != null) &&
           this.expressionMap.joinAttributes.length > 0
       ) {

Evidence

The new condition treats skip=0 as enabled pagination for joined queries; the pagination subquery
uses .offset(skip) and .limit(take). MySQL-family/SAP/Spanner explicitly throw when OFFSET is
set without LIMIT, and FindOptions can set skip=0 via .skip(0) when findOptions.skip is
provided.

src/query-builder/SelectQueryBuilder.ts[3488-3554]
src/query-builder/SelectQueryBuilder.ts[2665-2722]
src/query-builder/SelectQueryBuilder.ts[3326-3342]

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 joined-pagination branch is entered when `skip` is `0` due to the new nullish check. In that branch, `paginationQueryBuilder.offset(this.expressionMap.skip)` is applied even when `take` is `undefined`, which can produce OFFSET-without-LIMIT and throw on MySQL-family/SAP/Spanner.
### Issue Context
The previous truthy check avoided entering the pagination path for `skip=0`, but the new `!= null` check makes `0` count as “set”.
### Fix Focus Areas
- src/query-builder/SelectQueryBuilder.ts[3492-3496]
- src/query-builder/SelectQueryBuilder.ts[3551-3554]
- src/query-builder/SelectQueryBuilder.ts[2665-2721]
- src/query-builder/SelectQueryBuilder.ts[3326-3342]
### Suggested fix
Change the guard to only treat `skip` as pagination when it is non-null and non-zero, while still treating `take=0` as pagination:
- e.g. `((this.expressionMap.skip != null && this.expressionMap.skip !== 0) || this.expressionMap.take != null) && joins>0`
Alternatively, keep the new guard but avoid calling `.offset()` when `skip` is `0` and `take` is `null/undefined`.

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

Description

The new MySQL-specific assertion expects table.indices[0].name to equal the renamed FK constraint
name, but the test’s category table defines questionId as isUnique: true, which creates a
unique index named via uniqueConstraintName (UQ_*), not the FK name. MysqlQueryRunner.renameTable
only renames indexes whose names match namingStrategy.indexName (IDX_*), so this assertion is
likely incorrect/flaky on MySQL-family.

Code

test/functional/query-runner/rename-table.test.ts[R285-287]

+                if (DriverUtils.isMySQLFamily(dataSource.driver)) {
+                    expect(table!.indices[0].name).to.equal(newForeignKeyName)
+                }

Evidence

The test creates a unique FK column, which MySQLQueryRunner models as a unique index (UQ_*).
MysqlQueryRunner.renameTable’s index renaming only applies to indexes whose names match indexName
(IDX_*), so the unique index will not be renamed to the FK name, making the new assertion
unreliable.

test/functional/query-runner/rename-table.test.ts[201-287]
src/driver/mysql/MysqlQueryRunner.ts[704-753]
src/driver/mysql/MysqlQueryRunner.ts[3192-3221]

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 test asserts `table!.indices[0].name === newForeignKeyName` for MySQL-family after renaming the `category` table, but `indices[0]` in this setup is likely the unique index created for `questionId` (UQ_*), not an FK supporting index, and MySQL renameTable only renames IDX_* indexes.
### Issue Context
In the test, the `category` table is created with `questionId` marked `isUnique: true`, which forces a unique index; the FK does not need an additional supporting index.
### Fix Focus Areas
- test/functional/query-runner/rename-table.test.ts[226-256]
- test/functional/query-runner/rename-table.test.ts[272-287]
- src/driver/mysql/MysqlQueryRunner.ts[704-753]
- src/driver/mysql/MysqlQueryRunner.ts[3192-3221]
### Suggested fix
Either:
1) Remove/adjust the MySQL-only index assertion to check the FK name only, or
2) If the intent is to test FK supporting-index renaming, create a table where the FK column is *not* unique and has no explicit index, then explicitly locate the FK-supporting index by column list (or query information_schema) rather than using `indices[0]`.

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

Description

The new “default invalidWhereValuesBehavior” tests expect EntityManager.update/delete to throw
when criteria contain null/undefined, but OrmUtils.normalizeWhereCriteria is a no-op when
invalidWhereValuesBehavior is undefined and DataSource does not set a default value. As written,
these tests will fail unless the external ormconfig.json used by the test harness sets
invalidWhereValuesBehavior for every connection.

Code

test/functional/null-undefined-handling/query-builders.test.ts[R12-81]

+function invalidWhere<T>(value: T): never {
+    return value as never
+}
+
+// Regression for #12578: default invalidWhereValuesBehavior should throw for invalid where values.
+describe("entity manager > invalidWhereValuesBehavior default behavior", () => {
+    let dataSources: DataSource[]
+
+    before(async () => {
+        dataSources = await createTestingConnections({
+            disabledDrivers: ["spanner"],
+            entities: [Post, Category],
+            schemaCreate: true,
+            dropSchema: true,
+        })
+    })
+    beforeEach(() => reloadTestingDatabases(dataSources))
+    after(() => closeTestingConnections(dataSources))
+
+    async function prepareData(connection: DataSource) {
+        const category = new Category()
+        category.name = "Test Category"
+        await connection.manager.save(category)
+
+        const post = new Post()
+        post.title = "Test Post"
+        post.text = "Some text"
+        post.category = category
+        await connection.manager.save(post)
+
+        return { category, post }
+    }
+
+    it("should throw error for undefined values in EntityManager.update() by default", async () => {
+        for (const connection of dataSources) {
+            await prepareData(connection)
+
+            try {
+                await connection.manager.update(
+                    Post,
+                    invalidWhere({ category: { name: undefined } }),
+                    { title: "Updated" },
+                )
+                expect.fail("Expected error")
+            } catch (error) {
+                expect(error).to.be.instanceOf(TypeORMError)
+                expect(error.message).to.include("Undefined value encountered")
+            }
+        }
+    })
+
+    it("should throw error for null values in EntityManager.delete() by default", async () => {
+        for (const connection of dataSources) {
+            await prepareData(connection)
+
+            try {
+                await connection.manager.delete(
+                    Post,
+                    invalidWhere({
+                        category: { name: null },
+                    }),
+                )
+                expect.fail("Expected error")
+            } catch (error) {
+                expect(error).to.be.instanceOf(TypeORMError)
+                expect(error.message).to.include("Null value encountered")
+            }
+        }
+    })
+})

Evidence

DataSource stores options verbatim, so invalidWhereValuesBehavior can be undefined. EntityManager
passes that value into OrmUtils.normalizeWhereCriteria, which immediately returns the criteria
unchanged when options are falsy, so no exception is thrown in the default configuration.

test/functional/null-undefined-handling/query-builders.test.ts[16-80]
src/data-source/DataSource.ts[135-153]
src/entity-manager/EntityManager.ts[835-883]
src/util/OrmUtils.ts[668-676]

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 tests assume that invalid where values throw by default in EntityManager operations, but the current code path only applies `invalidWhereValuesBehavior` when it is present in `DataSource.options`. Without a default, `OrmUtils.normalizeWhereCriteria` returns criteria unchanged and EntityManager operations will not throw.
### Issue Context
This suite uses `createTestingConnections` without passing `driverSpecific.invalidWhereValuesBehavior`, and `ormconfig.sample.json` does not include it, so the tests are not self-contained.
### Fix Focus Areas
- test/functional/null-undefined-handling/query-builders.test.ts[16-80]
- src/data-source/DataSource.ts[135-153]
- src/entity-manager/EntityManager.ts[835-883]
- src/util/OrmUtils.ts[668-676]
### Suggested fix
Pick one:
1) Implement a true default by setting `invalidWhereValuesBehavior` to `{ null: 'throw', undefined: 'throw' }` when omitted (e.g., in `DataSource` construction or in EntityManager before calling normalize).
2) Or, if the library default is intentionally “no behavior unless configured”, update these new tests to pass `driverSpecific.invalidWhereValuesBehavior` explicitly instead of calling it “default behavior”.

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

Description

The new take(0) + joins regression test is in test/functional, but it does not include an issue
reference comment tying it back to the fixed issue. This reduces traceability for future maintenance
and can cause the test intent to be lost over time.

Code

test/functional/query-builder/select/query-builder-select.test.ts[R835-836]

+        it("should return empty array when take(0) is used in actual query execution with joins", () =>
+            Promise.all(

Evidence

PR Compliance ID 3 requires issue fixes to live in the functional suite and include an issue
reference comment when applicable. The added functional test for take(0) with joins has no
#12666 (or similar) reference comment near the test definition.

Rule 3: Prefer functional tests over per-issue tests
test/functional/query-builder/select/query-builder-select.test.ts[835-836]

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 functional regression test was added for an issue fix, but it lacks an issue reference comment (e.g. `// Regression test for #12666`). The compliance checklist asks for an issue reference in the test comment when applicable.
## Issue Context
This PR is an issue fix (Fixes `#12666` per PR metadata). Adding a short comment above the new test preserves traceability.
## Fix Focus Areas
- test/functional/query-builder/select/query-builder-select.test.ts[835-836]

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

Description

MysqlQueryRunner.renameTable tries to find an index whose name equals the old FK constraint name
and renames it in-memory, but MySQL table loading explicitly filters out indexes whose name matches
a referential constraint name. This means the intended “FK-named supporting index” is unlikely to
exist in newTable.indices, so the new logic may never run and cannot fix schema diff issues as
intended.

Code

src/driver/mysql/MysqlQueryRunner.ts[R812-820]

+            // MySQL creates a supporting index for the FK columns when needed.
+            // If the index inherited the FK name, keep it in sync with the
+            // renamed constraint so schema diffs do not try to drop it later.
+            const supportingIndex = newTable.indices.find(
+                (index) => index.name === oldForeignKeyName,
+            )
+            if (supportingIndex) {
+                supportingIndex.name = newForeignKeyName
+            }

Evidence

The MySQL indices loading query excludes indexes that match FK constraint names (`rc.CONSTRAINT_NAME
IS NULL after joining on INDEX_NAME = CONSTRAINT_NAME`), which prevents FK-named supporting
indexes from appearing in table.indices. The new supporting-index rename code relies on that exact
name being present in newTable.indices.

src/driver/mysql/MysqlQueryRunner.ts[812-820]
src/driver/mysql/MysqlQueryRunner.ts[2736-2749]
src/driver/mysql/MysqlQueryRunner.ts[3141-3171]

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 supporting-index rename logic searches `newTable.indices` by `oldForeignKeyName`, but `MysqlQueryRunner`’s index introspection query filters out indexes whose `INDEX_NAME` equals a FK `CONSTRAINT_NAME`. This makes the lookup unlikely to ever succeed in the exact case described by the comment.
### Issue Context
Additionally, the new code only updates the cached `Table` model and does not emit SQL to rename the index, so even if it did find an index it might desync cache vs database.
### Fix Focus Areas
- src/driver/mysql/MysqlQueryRunner.ts[812-820]
- src/driver/mysql/MysqlQueryRunner.ts[2736-2749]
- src/driver/mysql/MysqlQueryRunner.ts[3141-3171]
### Suggested fix
Decide on one consistent approach:
- If FK-named supporting indexes should be tracked, stop filtering them out in the `indicesSql` query (or load them separately), and implement an actual index rename (e.g. `ALTER TABLE ... RENAME INDEX old TO new` or drop/add where supported).
- Or, if they should remain filtered out, remove this lookup/rename block and adjust any tests/expectations accordingly.

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

🐞 Bugs (4) 📘 Rule violations (1) 📜 Skill insights (0)