Repository navigation
fix: prevent composite primary key collisions during hydration - #12905
Conversation
commit: |
Code Review by Qodo
1.
|
|
Addressed the non-finite key finding in ebec2f7. Confirmed against PostgreSQL 15.6: three composite keys containing NaN, Infinity, and -Infinity hydrated as one entity before this follow-up. Non-finite numbers now use nested tuple values, distinguishing them from each other, null, and literal strings during JSON encoding. Single-column string coercion is preserved. Added a PostgreSQL regression that verifies distinct root entities, their own descendants, and all three joined children of a common parent. It failed before the change and passes afterward. Also added the issue reference to the original functional suite. Validation: TypeScript compilation passed; focused hydration suites passed (3 tests, including PostgreSQL); broader query-builder tests passed with SQL.js/better-sqlite3 (434 passing, 6 pending). ESLint reported zero errors and 38 existing warnings. Formatting and whitespace checks passed. |
Cprakhar
left a comment
Cprakhar
left a comment
There was a problem hiding this comment.
@IbrahimHafez1 Your solution looks solid to me.
|
@alumni, would you have time to review this when convenient? The current head has green CI, including the PostgreSQL regression for non-finite composite keys, and Cprakhar has approved it. GitHub reports no merge conflicts, though the branch is behind master. The remaining tradeoff is the composite-key encoding cost documented in the description. It prevents distinct entities from being grouped together when their key components contain underscores. Feedback on whether this is the preferred approach would be helpful. |
Description of change
Fixes #12902.
Composite primary keys such as
("a_b", "c")and("a", "b_c")both becomea_b_cin the hydration grouping map. This drops a root entity and can combine children belonging to different entities.Encode normalized composite values as a JSON tuple so component boundaries remain distinct. Preserve the single-column grouping behavior and existing binary/object normalization; convert native bigint values to strings before JSON encoding.
Two regression tests cover distinct roots with their own children and colliding joined child keys, including repeated underscores. Both failed before the change. The linked issue includes a standalone reproduction.
Validation on Windows / Node 22.17.1:
Performance: grouping remains linear, but collision-safe composite encoding costs more than delimiter joining. A local microbenchmark of the actual grouping method over 100,000 rows / 20,000 entities (15 alternating measured iterations after warmup) measured composite medians of 14.8 ms before and 21.3 ms after. Single-column medians were 7.0 ms and 7.8 ms. These are local grouping-only measurements, excluding SQL and entity construction; this is a correctness fix, not a speedup.
Pull-Request Checklist
f279fd1367f24ad108a1b11cf833f1620274088d.Review follow-up
Addressed the non-finite key finding in ebec2f7.
Confirmed against PostgreSQL 15.6: three composite keys containing NaN, Infinity, and -Infinity hydrated as one entity before this follow-up. Non-finite numbers now use nested tuple values, distinguishing them from each other, null, and literal strings during JSON encoding. Single-column string coercion is preserved.
Added a PostgreSQL regression that verifies distinct root entities, their own descendants, and all three joined children of a common parent. It failed before the change and passes afterward. Also added the issue reference to the original functional suite.
Validation: TypeScript compilation passed; focused hydration suites passed (3 tests, including PostgreSQL); broader query-builder tests passed with SQL.js/better-sqlite3 (434 passing, 6 pending). ESLint reported zero errors and 38 existing warnings. Formatting and whitespace checks passed.