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 pull request links relevant issues as Fixes #00000
There are new or updated tests validating the change (tests/**.test.ts)
Documentation has been updated to reflect this change (docs/docs/**.md)
PR Type
Bug fix, Tests
Description
Fix orderBy on subquery aliases when pagination is applied
Handle aliases without metadata in pagination distinct queries
Add null-safety checks for alias metadata lookups
Add comprehensive test suite for subquery join pagination scenarios
Diagram Walkthrough
flowchart LR
A["Subquery with<br/>orderBy alias"] -->|pagination applied| B["Distinct query<br/>wrapping"]
B -->|check alias<br/>metadata| C["Add missing<br/>columns to select"]
C -->|null-safe<br/>lookups| D["Proper column<br/>resolution"]
E["Test scenarios"] -->|validate| D
Loading
File Walkthrough
Relevant files
Bug fix
SelectQueryBuilder.ts
Handle subquery aliases in pagination distinct queries
src/query-builder/SelectQueryBuilder.ts
Added logic to detect and handle subquery aliases without metadata when pagination is applied
Automatically adds missing columns from subquery aliases to the select clause
Added null-safety checks using alias.hasMetadata before accessing metadata properties
Prevents errors when resolving column names for aliases without entity metadata
Here are some key observations to aid the review process:
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 Security concerns
SQL injection surface: In src/query-builder/SelectQueryBuilder.ts, the added select uses string interpolation selection: \${aliasName}.${propertyPath}`for non-metadata aliases. IforderCriteria/propertyPathcan be influenced by untrusted input (e.g., passed directly from an API parameter intoorderBy()), this could allow crafted values to end up in SQL. Consider ensuring these tokens are safely escaped/validated at the point they are injected into expressionMap.selects(or document/guard thatorderBy` criteria must be trusted).
When pagination + DISTINCT is used, the new loop adds extra selects for non-metadata aliases referenced in ORDER BY. This may introduce duplicate select entries (same selection/aliasName) if multiple order criteria repeat or if the select already exists, potentially affecting generated SQL or performance. Consider de-duplicating against existing expressionMap.selects.
The new code assumes findAliasByName(aliasName) always returns a valid alias and immediately reads alias.hasMetadata. If an ORDER BY references an unknown alias (or if behavior differs across code paths), this could throw unexpectedly. Consider ensuring a clearer error path or guarding before accessing alias properties.
The replacement logic now includes non-metadata alias prefixes in the regex matching set. This broadens what gets rewritten/escaped and may affect edge cases (e.g., alias-like substrings in complex expressions). It’s worth validating that only intended alias/property tokens are matched and that escaping behavior remains correct across drivers.
protectedreplacePropertyNamesForTheWholeQuery(statement: string){constreplacements: {[key: string]: {[key: string]: string}}={}constnoMetadataAliasPrefixes=newSet<string>()for(constaliasofthis.expressionMap.aliases){constreplaceAliasNamePrefix=this.expressionMap.aliasNamePrefixingEnabled&&alias.name
? `${alias.name}.`
: ""if(!alias.hasMetadata){if(replaceAliasNamePrefix){noMetadataAliasPrefixes.add(replaceAliasNamePrefix)}continue}if(!replacements[replaceAliasNamePrefix]){replacements[replaceAliasNamePrefix]={}}// Insert & overwrite the replacements from least to most relevant in our replacements object.// To do this we iterate and overwrite in the order of relevance.// Least to Most Relevant:// * Relation Property Path to first join column key// * Relation Property Path + Column Path// * Column Database Name// * Column Property Name// * Column Property Pathfor(constrelationofalias.metadata.relations){if(relation.joinColumns.length>0)replacements[replaceAliasNamePrefix][relation.propertyPath]=relation.joinColumns[0].databaseName}for(constrelationofalias.metadata.relations){constallColumns=[
...relation.joinColumns,
...relation.inverseJoinColumns,]for(constjoinColumnofallColumns){constpropertyKey=`${relation.propertyPath}.${joinColumn.referencedColumn!.propertyPath}`replacements[replaceAliasNamePrefix][propertyKey]=joinColumn.databaseName}}for(constcolumnofalias.metadata.columns){replacements[replaceAliasNamePrefix][column.databaseName]=column.databaseName}for(constcolumnofalias.metadata.columns){replacements[replaceAliasNamePrefix][column.propertyName]=column.databaseName}for(constcolumnofalias.metadata.columns){replacements[replaceAliasNamePrefix][column.propertyPath]=column.databaseName}}constreplacementKeys=Object.keys(replacements)constallPrefixKeys=[...replacementKeys, ...noMetadataAliasPrefixes]constreplaceAliasNamePrefixes=allPrefixKeys.map((key)=>escapeRegExp(key)).join("|")if(allPrefixKeys.length>0){statement=statement.replace(newRegExp(// Avoid a lookbehind here since it's not well supported`([ =(]|^.{0})`+// any of ' =(' or start of line// followed by our prefix, e.g. 'tablename.' or ''`${replaceAliasNamePrefixes ? "("+replaceAliasNamePrefixes+")" : ""}([^ =(),]+)`+// a possible property name: sequence of anything but ' =(),'// terminated by ' =),' or end of line`(?=[ =),]|.{0}$)`,"gm",),(...matches)=>{letmatch: string,pre: string,p: stringif(replaceAliasNamePrefixes){match=matches[0]pre=matches[1]p=matches[3]if(replacements[matches[2]]&&replacements[matches[2]][p]){return`${pre}${this.escape(matches[2].substring(0,matches[2].length-1),)}.${this.escape(replacements[matches[2]][p])}`}// For non-metadata aliases (e.g. subquery joins),// escape both the alias name and property path as-isif(noMetadataAliasPrefixes.has(matches[2])){return`${pre}${this.escape(matches[2].substring(0,matches[2].length-1),)}.${this.escape(p)}`}
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
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.
Close #10814
Description of change
Pull-Request Checklist
masterbranchFixes #00000tests/**.test.ts)docs/docs/**.md)