Home Original page

fix: include joined entity primary keys in pagination subquery by mag123c · Pull Request #11669 · typeorm/typeorm

PR Code Suggestions ✨

Latest suggestions up to 9064fa0

CategorySuggestion                                                                                                                                    Impact
Possible issue
Verify all composite primary keys

To ensure correct grouping for composite primary keys, change the check from
some to every, requiring all primary key columns to be selected.

src/query-builder/transformer/RawSqlResultsToEntityTransformer.ts [117-120]

-// Check if primary key columns are actually selected in the raw results
-const primaryKeysSelected = keys.some(
+// Check if all primary key columns are actually selected in the raw results
+const primaryKeysSelected = keys.every(
     (key) => key in (rawResults[0] ?? {}),
 )
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: Using some is incorrect for composite primary keys, as it can lead to incorrect entity grouping if only a partial key is selected. Changing to every ensures that grouping by primary key only occurs when all parts of the key are present, which is critical for data integrity.

High
General
Handle empty results array

Add a check to handle an empty rawResults array at the beginning of the function
to avoid potential errors and improve code clarity.

src/query-builder/transformer/RawSqlResultsToEntityTransformer.ts [117-122]

+// Handle empty results
+if (rawResults.length === 0) {
+    return map
+}
+
 // Check if primary key columns are actually selected in the raw results
 const primaryKeysSelected = keys.some(
-    (key) => key in (rawResults[0] ?? {}),
+    (key) => key in rawResults[0],
 )
 
 for (const rawResult of rawResults) {
  • Apply / Chat
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly identifies a potential issue with an empty rawResults array, but the current code handles this case without error due to optional chaining (?? {}). The proposed change improves explicitness and slightly simplifies the check by removing the need for optional chaining.

Low
  • More

Previous suggestions

Suggestions up to commit 5429b85
CategorySuggestion                                                                                                                                    Impact
Possible issue
Validate all primary keys exist

To ensure correct entity grouping with composite primary keys, change the check
from keys.some() to keys.every(). This validates that all primary key columns
are present in the query result, not just some of them.

src/query-builder/transformer/RawSqlResultsToEntityTransformer.ts [117-120]

 // Check if primary key columns are actually selected in the raw results
-const primaryKeysSelected = keys.some(
-    (key) => key in (rawResults[0] ?? {}),
-)
+const primaryKeysSelected = rawResults.length > 0 && 
+    keys.every((key) => key in rawResults[0])
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that using keys.some() is insufficient for composite primary keys, as all parts of the key are required for unique identification. Using keys.every() as proposed would make the check more robust and prevent incorrect grouping when only a partial composite primary key is selected.

Medium
Suggestions up to commit dae49c7
CategorySuggestion                                                                                                                                    Impact
General
Optimize row index lookup performance

Replace the inefficient rawResults.indexOf(rawResult) call inside the loop with
a more performant method of getting the row index, such as using the index from
a for...of loop with entries().

src/query-builder/transformer/RawSqlResultsToEntityTransformer.ts [142-147]

+} else {
+    // Fallback: use row index when primary keys are not available
+    // This ensures each row gets its own group for proper entity mapping
+    const rowIndex = rawResults.indexOf(rawResult)
+    id = `row_${rowIndex}`
+}
 
-
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies a performance issue where using indexOf inside a loop leads to O(n^2) complexity. Replacing it with a loop index is a valid optimization that improves efficiency for large result sets.

Low
Suggestions up to commit f94b754
CategorySuggestion                                                                                                                                    Impact
Possible issue
Validate all composite primary keys

Replace keys.some() with keys.every() in the hasValidKeys check to ensure all
parts of a composite primary key are present before attempting to group by key.

src/query-builder/transformer/RawSqlResultsToEntityTransformer.ts [118-125]

 const hasValidKeys =
     keys.length > 0 &&
     rawResults.length > 0 &&
-    keys.some(
+    keys.every(
         (key) =>
             rawResults[0].hasOwnProperty(key) &&
             rawResults[0][key] !== undefined,
     )
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a bug in the new hasValidKeys logic where using some instead of every can lead to incorrect entity grouping for entities with composite primary keys when only a partial key is selected.

Medium
Suggestions up to commit 9ddcf86
CategorySuggestion                                                                                                                                    Impact
Possible issue
Validate all composite primary keys

Replace keys.some() with keys.every() to ensure all parts of a composite primary
key are present before attempting to group entities by key. This prevents
incorrect grouping when only a partial key is available.

src/query-builder/transformer/RawSqlResultsToEntityTransformer.ts [118-125]

 const hasValidKeys =
     keys.length > 0 &&
     rawResults.length > 0 &&
-    keys.some(
+    keys.every(
         (key) =>
             rawResults[0].hasOwnProperty(key) &&
             rawResults[0][key] !== undefined,
     )
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that using some() can lead to incorrect entity grouping with composite primary keys when only a partial key is selected. Changing to every() ensures that grouping by primary key only occurs when the full key is present, making the logic more robust and preventing a potential bug.

Medium
Suggestions up to commit 17cab60
CategorySuggestion                                                                                                                                    Impact
Possible issue
Validate all composite primary keys

Change keys.some() to keys.every() in the hasValidKeys check. This ensures that
grouping by primary key only occurs when all columns of a composite primary key
are present, preventing incorrect data mapping.

src/query-builder/transformer/RawSqlResultsToEntityTransformer.ts [118-125]

 const hasValidKeys =
     keys.length > 0 &&
     rawResults.length > 0 &&
-    keys.some(
+    keys.every(
         (key) =>
             rawResults[0].hasOwnProperty(key) &&
             rawResults[0][key] !== undefined,
     )
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that using keys.some() can lead to incorrect entity grouping when only a partial composite primary key is selected, which is a subtle but important bug. Changing to keys.every() makes the grouping logic more robust and correct.

Medium
General
Optimize row index lookup performance

Optimize the row index lookup by replacing the for...of loop and indexOf call
with a loop that provides an index, such as forEach or a standard for loop, to
avoid O(n²) complexity.

src/query-builder/transformer/RawSqlResultsToEntityTransformer.ts [147-152]

+} else {
+    // Fallback: use row index when primary keys are not available
+    // This ensures each row gets its own group for proper entity mapping
+    const rowIndex = rawResults.indexOf(rawResult)
+    id = `row_${rowIndex}`
+}
 
-
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly points out a performance issue with O(n²) complexity by using indexOf inside a for...of loop. While this is a valid optimization, the improved_code is identical to the existing_code and does not provide an actionable fix.

Low