You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change fixes an issue where leftJoin combined with skip/take pagination was not properly loading related entities due to missing primary keys in the pagination subquery.
The SelectQueryBuilder uses a two-query approach for pagination with joins to avoid N+1 problems. The first query selects primary keys with pagination, and the second query loads full data for those keys. The bug was in the first query not including joined entity primary keys, causing entity identification issues.
-// 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.
+// 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.
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.
// 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.
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().
+} 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.
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.
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.
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.
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.
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.
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.
+} 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.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
pkuczynski
left a comment
Description of change
This change fixes an issue where leftJoin combined with skip/take pagination was not properly loading related entities due to missing primary keys in the pagination subquery.
The SelectQueryBuilder uses a two-query approach for pagination with joins to avoid N+1 problems. The first query selects primary keys with pagination, and the second query loads full data for those keys. The bug was in the first query not including joined entity primary keys, causing entity identification issues.
Pull-Request Checklist
masterbranchskipandtakedon't work for join tables if primary key is not selected #11662