fix: compare entity ids of mixed string/number type as equal - #12780

Description

In OrmUtils.compareIds, the mixed string/number fast-path compares ids via String(...) for any
finite number; for numbers > Number.MAX_SAFE_INTEGER this can silently match the wrong database id
due to IEEE-754 rounding. Because compareIds is used to match loaded entities/relations (e.g.,
SubjectDatabaseEntityLoader and OneToManySubjectBuilder), an incorrect equality can associate
changes with the wrong row.

Code

src/util/OrmUtils.ts[R254-257]

+            const numericId =
+                typeof firstId.id === "number" ? firstId.id : secondId.id
+            if (!Number.isFinite(numericId)) return false
+            return String(firstId.id) === String(secondId.id)

Evidence

The new cross-type path only rejects non-finite numbers, then compares by stringification, which is
unsafe for integers beyond JS’s safe range. compareIds is used in persistence matching/diffing
paths, so a false positive equality can mis-associate entities/relations.

src/util/OrmUtils.ts[233-258]
src/persistence/SubjectDatabaseEntityLoader.ts[133-139]
src/persistence/subject-builder/OneToManySubjectBuilder.ts[137-146]

Agent prompt

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

### Issue description
`OrmUtils.compareIds` now bridges `string` vs `number` ids by comparing `String(firstId.id) === String(secondId.id)` after only `Number.isFinite(...)`. If the numeric id is outside JS safe-integer range, `String(number)` can reflect a *rounded* value, which can incorrectly match a different bigint id string.

### Issue Context
This function is used in persistence logic to decide whether two identifiers refer to the same row.

### Fix Focus Areas
- src/util/OrmUtils.ts[239-258]

### Suggested fix
- In the mixed-type branch, add a guard requiring `Number.isSafeInteger(numericId)` (and implicitly integer-ness) before doing `String(...)` equality. If it’s not a safe integer, return `false` (forcing callers to use consistent types such as strings/native bigint for large ids).
- Add a unit test to `test/unit/util/orm-utils.test.ts` asserting that an unsafe integer (e.g. `9007199254740993`) does **not** cross-type match a string id.

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

Description

OrmUtils.compareIds now stringifies mixed string/number ids, which makes { id: NaN } equal to `{
id: "NaN" } (and similarly for ±Infinity`). This can cause identifier collisions in
persistence-diffing paths that rely on compareIds to decide whether two relations/subjects refer
to the same row.

Code

src/util/OrmUtils.ts[R247-249]

+            return typeof firstId.id === typeof secondId.id
+                ? firstId.id === secondId.id
+                : String(firstId.id) === String(secondId.id)

Evidence

The new fast path explicitly string-compares mixed string/number ids, which makes String(NaN)
become "NaN" and String(Infinity) become "Infinity", enabling cross-type equality that
previously could not happen. deepCompare/compare2Objects only special-cases NaN when both sides
are NaN and otherwise rejects mixed primitive types, so this cross-type equality is new and can
affect persistence logic that uses compareIds for relation/id matching.

src/util/OrmUtils.ts[233-250]
src/util/OrmUtils.ts[454-491]
src/persistence/SubjectChangedColumnsComputer.ts[247-286]

Agent prompt

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

## Issue description
`OrmUtils.compareIds` treats mixed `{ id: number }` vs `{ id: string }` as equal by comparing their `String(...)` forms. This introduces a regression where invalid numeric ids (`NaN`, `Infinity`, `-Infinity`) become equal to the literal strings `"NaN"`, `"Infinity"`, and `"-Infinity"`.
### Issue Context
`compareIds` is used in persistence diffing (e.g., relation change detection). Allowing non-finite numbers to collide with string ids can make unrelated entities appear identical.
### Fix
Inside the single-`id` fast path, before doing the cross-type `String(...) === String(...)` comparison, guard against non-finite numbers:
- If either side is a `number` and `!Number.isFinite(value)`, return `false` (or skip the fast path and fall back).
Add unit tests asserting:
- `compareIds({id: NaN}, {id: "NaN"}) === false`
- `compareIds({id: Infinity}, {id: "Infinity"}) === false`
### Fix Focus Areas
- src/util/OrmUtils.ts[239-250]
- test/unit/util/orm-utils.test.ts[274-313]

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

Description

This PR adds regression coverage for #11773 only in test/unit, but the checklist expects issue-fix
coverage to live in test/functional to exercise real persistence behavior across drivers. Relying
solely on unit tests may miss integration regressions in relation handling (the original reported
failure mode).

Code

test/unit/util/orm-utils.test.ts[R284-288]

+        it("matches a string id against a numeric id when their values are equal", () => {
+            // MySQL and some other drivers return bigint columns as strings, so
+            // the DB-loaded id ("1") and the in-memory id (1) must still compare
+            // as equal — otherwise the relation looks removed and its FK is nulled
+            // (see #11773).

Evidence

PR Compliance ID 3 calls for issue fixes to be validated in the functional test suite. The added
regression coverage for compareIds (including a see #11773 note) is located under test/unit,
indicating the issue fix is not validated via a functional persistence scenario.

Rule 3: Prefer functional tests over per-issue tests
test/unit/util/orm-utils.test.ts[284-292]

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 regression for #11773 is currently covered only by a unit test (`describe("compareIds" ...)`) rather than a functional/integration test.
## Issue Context
PR Compliance ID 3 requires issue fixes to be validated in the functional suite (`test/functional`) so the behavior is exercised through real persistence flows and driver-specific type behavior (e.g., MySQL bigint ids as strings).
## Fix Focus Areas
- test/unit/util/orm-utils.test.ts[274-313]
- test/functional/relations/orphaned-row-action/orphaned-row-action.test.ts[1-120]

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

Description

The new compareIds test claims to cover null and undefined inputs, but it only asserts undefined
cases, so regressions in the explicit null handling path would not be caught.

Code

test/unit/util/orm-utils.test.ts[R299-303]

+        it("returns false when either id map is null or undefined", () => {
+            expect(OrmUtils.compareIds(undefined, { id: 1 })).to.equal(false)
+            expect(OrmUtils.compareIds({ id: 1 }, undefined)).to.equal(false)
+            expect(OrmUtils.compareIds(undefined, undefined)).to.equal(false)
+        })

Evidence

The test block title mentions null, but the assertions only pass undefined. The production code
explicitly checks for both undefined and null, so the null branch remains unexercised by this
new test.

test/unit/util/orm-utils.test.ts[274-313]
src/util/OrmUtils.ts[221-232]

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 `compareIds` test case says it covers "null or undefined" inputs but only tests `undefined`. This reduces regression coverage for the implementation’s explicit `null` checks.
### Issue Context
`OrmUtils.compareIds` returns `false` when either input is `null` or `undefined`.
### Fix Focus Areas
- test/unit/util/orm-utils.test.ts[299-303]
### Suggested change
Extend the test to include:
- `OrmUtils.compareIds(null as any, { id: 1 }) === false`
- `OrmUtils.compareIds({ id: 1 }, null as any) === false`
- `OrmUtils.compareIds(null as any, null as any) === false`
Or, if you prefer, rename the test title to mention only `undefined` if that’s the intended coverage.

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

💡 Did you know, you can enable the Remediation agent and Qodo fixes findings in a dedicated fix PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

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

Description

This PR adds regression coverage for #11773 only in test/unit, but the checklist expects issue-fix
coverage to live in test/functional to exercise real persistence behavior across drivers. Relying
solely on unit tests may miss integration regressions in relation handling (the original reported
failure mode).

Code

test/unit/util/orm-utils.test.ts[R284-288]

+        it("matches a string id against a numeric id when their values are equal", () => {
+            // MySQL and some other drivers return bigint columns as strings, so
+            // the DB-loaded id ("1") and the in-memory id (1) must still compare
+            // as equal — otherwise the relation looks removed and its FK is nulled
+            // (see #11773).

Evidence

PR Compliance ID 3 calls for issue fixes to be validated in the functional test suite. The added
regression coverage for compareIds (including a see #11773 note) is located under test/unit,
indicating the issue fix is not validated via a functional persistence scenario.

Rule 3: Prefer functional tests over per-issue tests
test/unit/util/orm-utils.test.ts[284-292]

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 regression for #11773 is currently covered only by a unit test (`describe("compareIds" ...)`) rather than a functional/integration test.

## Issue Context
PR Compliance ID 3 requires issue fixes to be validated in the functional suite (`test/functional`) so the behavior is exercised through real persistence flows and driver-specific type behavior (e.g., MySQL bigint ids as strings).

## Fix Focus Areas
- test/unit/util/orm-utils.test.ts[274-313]
- test/functional/relations/orphaned-row-action/orphaned-row-action.test.ts[1-120]

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

Description

OrmUtils.compareIds now stringifies mixed string/number ids, which makes { id: NaN } compare
equal to { id: "NaN" } (and similarly for Infinity), potentially treating invalid/uninitialized
identifiers as the same entity during persistence diffing.

Code

src/util/OrmUtils.ts[R247-250]

+            return typeof firstId.id === typeof secondId.id
+                ? firstId.id === secondId.id
+                : String(firstId.id) === String(secondId.id)
        }

Evidence

The new mixed-type branch compares ids via String(...), which makes NaN/Infinity comparable to
their string literals. compare2Objects already treats NaN as a special case, suggesting that NaN
semantics matter and should not be accidentally broadened for ids.

src/util/OrmUtils.ts[221-252]
src/util/OrmUtils.ts[446-489]

Agent prompt

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

### Issue description
`OrmUtils.compareIds` falls back to `String(firstId.id) === String(secondId.id)` when one side is a `number` and the other is a `string`. This unintentionally allows non-finite numbers (e.g. `NaN`, `Infinity`) to compare equal to their string spellings ("NaN", "Infinity"), which can cause incorrect id equality results.

### Issue Context
Elsewhere in `OrmUtils` there is explicit NaN-handling in `compare2Objects`, indicating that special numeric values need deliberate treatment rather than accidental equality via stringification.

### Fix Focus Areas
- src/util/OrmUtils.ts[233-252]

### Suggested change
Before applying the mixed-type string comparison, guard against non-finite numeric values:
- If `typeof firstId.id === "number"` and `!Number.isFinite(firstId.id)`, return `false`.
- If `typeof secondId.id === "number"` and `!Number.isFinite(secondId.id)`, return `false`.

(Optionally also consider requiring integers for the numeric side if ids are expected to be integral.)

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

Description

OrmUtils.compareIds now stringifies mixed string/number ids, which makes { id: NaN } equal to `{
id: "NaN" } (and similarly for ±Infinity`). This can cause identifier collisions in
persistence-diffing paths that rely on compareIds to decide whether two relations/subjects refer
to the same row.

Code

src/util/OrmUtils.ts[R247-249]

+            return typeof firstId.id === typeof secondId.id
+                ? firstId.id === secondId.id
+                : String(firstId.id) === String(secondId.id)

Evidence

The new fast path explicitly string-compares mixed string/number ids, which makes String(NaN)
become "NaN" and String(Infinity) become "Infinity", enabling cross-type equality that
previously could not happen. deepCompare/compare2Objects only special-cases NaN when both sides
are NaN and otherwise rejects mixed primitive types, so this cross-type equality is new and can
affect persistence logic that uses compareIds for relation/id matching.

src/util/OrmUtils.ts[233-250]
src/util/OrmUtils.ts[454-491]
src/persistence/SubjectChangedColumnsComputer.ts[247-286]

Agent prompt

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

## Issue description
`OrmUtils.compareIds` treats mixed `{ id: number }` vs `{ id: string }` as equal by comparing their `String(...)` forms. This introduces a regression where invalid numeric ids (`NaN`, `Infinity`, `-Infinity`) become equal to the literal strings `"NaN"`, `"Infinity"`, and `"-Infinity"`.
### Issue Context
`compareIds` is used in persistence diffing (e.g., relation change detection). Allowing non-finite numbers to collide with string ids can make unrelated entities appear identical.
### Fix
Inside the single-`id` fast path, before doing the cross-type `String(...) === String(...)` comparison, guard against non-finite numbers:
- If either side is a `number` and `!Number.isFinite(value)`, return `false` (or skip the fast path and fall back).
Add unit tests asserting:
- `compareIds({id: NaN}, {id: "NaN"}) === false`
- `compareIds({id: Infinity}, {id: "Infinity"}) === false`
### Fix Focus Areas
- src/util/OrmUtils.ts[239-250]
- test/unit/util/orm-utils.test.ts[274-313]

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

Description

The new compareIds test claims to cover null and undefined inputs, but it only asserts undefined
cases, so regressions in the explicit null handling path would not be caught.

Code

test/unit/util/orm-utils.test.ts[R299-303]

+        it("returns false when either id map is null or undefined", () => {
+            expect(OrmUtils.compareIds(undefined, { id: 1 })).to.equal(false)
+            expect(OrmUtils.compareIds({ id: 1 }, undefined)).to.equal(false)
+            expect(OrmUtils.compareIds(undefined, undefined)).to.equal(false)
+        })

Evidence

The test block title mentions null, but the assertions only pass undefined. The production code
explicitly checks for both undefined and null, so the null branch remains unexercised by this
new test.

test/unit/util/orm-utils.test.ts[274-313]
src/util/OrmUtils.ts[221-232]

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 `compareIds` test case says it covers "null or undefined" inputs but only tests `undefined`. This reduces regression coverage for the implementation’s explicit `null` checks.

### Issue Context
`OrmUtils.compareIds` returns `false` when either input is `null` or `undefined`.

### Fix Focus Areas
- test/unit/util/orm-utils.test.ts[299-303]

### Suggested change
Extend the test to include:
- `OrmUtils.compareIds(null as any, { id: 1 }) === false`
- `OrmUtils.compareIds({ id: 1 }, null as any) === false`
- `OrmUtils.compareIds(null as any, null as any) === false`

Or, if you prefer, rename the test title to mention only `undefined` if that’s the intended coverage.

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

💡 Did you know, you can enable the Remediation agent and Qodo fixes findings in a dedicated fix PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗