Skip to content

fix: read closure-table primary values through column metadata - #12907

Merged
alumni merged 5 commits into
typeorm:masterfrom
IbrahimHafez1:fix/closure-renamed-primary-reparent
Oct 7, 2026
Merged

fix: read closure-table primary values through column metadata#12907
alumni merged 5 commits into
typeorm:masterfrom
IbrahimHafez1:fix/closure-renamed-primary-reparent

Conversation

@IbrahimHafez1

@IbrahimHafez1 IbrahimHafez1 commented Sep 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description of change

Fixes #12904.

When a primary property maps to a differently named database column, closure-table reparenting reads the DELETE parameter using the database column name on the entity object. That returns undefined, so old ancestor links remain. Cross-root moves return stale ancestors; same-tree moves can fail with a unique-constraint error.

Read the primary value through column.getEntityValue(entity), matching the existing INSERT phase. The fix changes neither SQL shape nor query count.

Three regression cases cover moving a branch with a child across roots, within the same tree, and to a new root. They check exact ancestors and closure-record counts. The linked issue includes a standalone reproduction.

Validation on Windows / Node 22.17.1:

  • TypeScript compilation passed.
  • Combined run with the three independent tree/hydration fixes: 3,081 passing, 179 pending, using SQL.js and better-sqlite3. Other database engines were not run.
  • New regression cases failed before the fix and passed after it.
  • Changed-file formatting and lint checks passed (lint reported existing warnings).

Pull-Request Checklist

  • Based on current master at f279fd1367f24ad108a1b11cf833f1620274088d.
  • Linked the relevant issue and checked related issues/PRs.
  • Added regression tests.
  • Documentation: N/A; restores existing behavior without a public API change.

Review follow-up

Addressed in ed33893. The review is correct: repository.count() checks entity rows, not closure-junction rows. The original PR description overstated what that assertion verified.

Kept the four-entity assertion and added a direct count of repository.metadata.closureJunctionTable.tablePath. Expected junction totals are 7 for moving across roots, 10 within the same tree, and 5 when detaching as a root. These complement the exact leaf-ancestor assertions. Added the #12904 issue reference as well.

All four tests in the custom-primary-column suite passed with SQL.js/better-sqlite3. TypeScript compilation, ESLint, formatting, and whitespace checks passed. No runtime-code change was needed for this finding.

@github-actions github-actions Bot added the linked-issue PR references an issue label Sep 26, 2026
@pkg-pr-new

pkg-pr-new Bot commented Sep 26, 2026 •
edited
Loading

Copy link
Copy Markdown

  • typeorm-sql-js-example

    npm i https://pkg.pr.new/typeorm@12907

commit: 169cf37

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Sep 26, 2026 •
edited
Loading

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Broken closure rows can pass the test ✓ Resolved
Description 
repository.count() counts RenamedCategory entities rather than records in its closure junction
table. A reparenting defect that leaves an extra link for the moved branch while returning the
expected leaf ancestors can therefore satisfy both assertions without preserving the complete
closure table.
Code

packages/typeorm/test/functional/tree-tables/closure-table/custom-primary-column/custom-primary-column.test.ts[59]

+                    expect(await repository.count()).to.equal(4)
Evidence 
TreeRepository inherits the standard repository implementation, whose count() delegates to the
manager using the entity target. Closure ancestry is handled through a separate junction-table
query, so the added count assertion cannot detect closure-row discrepancies.

packages/typeorm/test/functional/tree-tables/closure-table/custom-primary-column/custom-primary-column.test.ts[47-59]
packages/typeorm/src/repository/TreeRepository.ts[8-16]
packages/typeorm/src/repository/Repository.ts[553-561]
packages/typeorm/src/repository/TreeRepository.ts[259-269]

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 test calls `repository.count()`, which counts `RenamedCategory` rows rather than closure-junction rows and therefore does not validate complete closure-table integrity after reparenting.
## Fix Focus Areas
- packages/typeorm/test/functional/tree-tables/closure-table/custom-primary-column/custom-primary-column.test.ts[47-59]
## Recommended Fix
Obtain the junction table from `dataSource.getMetadata(RenamedCategory).closureJunctionTable`, count its rows directly, and assert the destination-specific totals: 7 for `other root`, 10 for `same tree`, and 5 for `root`.

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

Grey Divider

Tip of the day 
💡 Did you know, you can route each severity your way: inline, summary, both, or drop

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@IbrahimHafez1

IbrahimHafez1 commented Sep 26, 2026

Copy link
Copy Markdown
Contributor Author

Addressed in ed33893. The review is correct: repository.count() checks entity rows, not closure-junction rows. The original PR description overstated what that assertion verified.

Kept the four-entity assertion and added a direct count of repository.metadata.closureJunctionTable.tablePath. Expected junction totals are 7 for moving across roots, 10 within the same tree, and 5 when detaching as a root. These complement the exact leaf-ancestor assertions. Added the #12904 issue reference as well.

All four tests in the custom-primary-column suite passed with SQL.js/better-sqlite3. TypeScript compilation, ESLint, formatting, and whitespace checks passed. No runtime-code change was needed for this finding.

@IbrahimHafez1

IbrahimHafez1 commented Sep 26, 2026

Copy link
Copy Markdown
Contributor Author

The Oracle CI failure occurs in the reparenting INSERT ... SELECT: Oracle rejects AS before table aliases. Removed AS from the two table aliases, preserving the query and parameters. The regression remains enabled for Oracle.

All four custom-primary-column cases pass locally with SQL.js/better-sqlite3; TypeScript compilation and lint/format/whitespace checks pass (existing lint warnings remain). Oracle verification is pending the new CI run.

@alumni
alumni enabled auto-merge (squash) October 7, 2026 19:43
@alumni
alumni merged commit 876eb04 into typeorm:master Oct 7, 2026
46 checks passed
@github-actions github-actions Bot added this to the 2.0.next milestone Oct 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alumni alumni alumni approved these changes
@qodo-free-for-open-source-projects qodo-free-for-open-source-projects[bot] qodo-free-for-open-source-projects[bot] approved these changes

Assignees

No one assigned

Labels

linked-issue PR references an issue

Milestone

2.0.next

Development

Successfully merging this pull request may close these issues.

Closure-table reparenting leaves stale ancestors when a primary column is renamed

3 participants

@IbrahimHafez1 @alumni @Cprakhar