Skip to content

Comments

fix: fix up orderBy on subquery aliases with pagination#12018

Open
gioboa wants to merge 3 commits intotypeorm:masterfrom
gioboa:fix/10814
Open

fix: fix up orderBy on subquery aliases with pagination#12018
gioboa wants to merge 3 commits intotypeorm:masterfrom
gioboa:fix/10814

Conversation

@gioboa
Copy link
Collaborator

@gioboa gioboa commented Feb 20, 2026

Close #10814

Description of change

Pull-Request Checklist

  • Code is up-to-date with the master branch
  • 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)

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

User description

Close #10814

Description of change

Pull-Request Checklist

  • Code is up-to-date with the master branch
  • 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
+37/-8   
Tests
Parent.ts
Add Parent test entity for pagination tests                           

test/functional/query-builder/subquery-join-pagination/entity/Parent.ts

  • Created new test entity representing a parent record
  • Defines one-to-many relationship with Child entity
  • Includes id and name columns for test data
+17/-0   
Child.ts
Add Child test entity for pagination tests                             

test/functional/query-builder/subquery-join-pagination/entity/Child.ts

  • Created new test entity representing a child record
  • Defines many-to-one relationship with Parent entity
  • Includes id, name columns and foreign key reference
+19/-0   
subquery-join-pagination.test.ts
Add subquery join pagination test suite                                   

test/functional/query-builder/subquery-join-pagination/subquery-join-pagination.test.ts

+143/-0 

@pkg-pr-new
Copy link

pkg-pr-new bot commented Feb 20, 2026

commit: 5f02557

@gioboa
Copy link
Collaborator Author

gioboa commented Feb 21, 2026

/review

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

PR Reviewer Guide 🔍

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

⚡ Recommended focus areas for review

Duplicate Selects

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.

for (const orderCriteria of Object.keys(
    this.expressionMap.allOrderBys,
)) {
    if (orderCriteria.indexOf(".") !== -1) {
        const criteriaParts = orderCriteria.split(".")
        const aliasName = criteriaParts[0]
        const propertyPath = criteriaParts.slice(1).join(".")
        const alias = this.expressionMap.findAliasByName(aliasName)
        if (!alias.hasMetadata) {
            const columnAlias = DriverUtils.buildAlias(
                this.connection.driver,
                undefined,
                aliasName,
                propertyPath,
            )
            originalQuery.expressionMap.selects.push({
                selection: `${aliasName}.${propertyPath}`,
                aliasName: columnAlias,
            })
        }
    }
}
Alias Handling

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.

if (orderCriteria.indexOf(".") !== -1) {
    const criteriaParts = orderCriteria.split(".")
    const aliasName = criteriaParts[0]
    const propertyPath = criteriaParts.slice(1).join(".")
    const alias = this.expressionMap.findAliasByName(aliasName)
    if (!alias.hasMetadata) {
        const columnAlias = DriverUtils.buildAlias(
            this.connection.driver,
            undefined,
            aliasName,
            propertyPath,
        )
        originalQuery.expressionMap.selects.push({
            selection: `${aliasName}.${propertyPath}`,
            aliasName: columnAlias,
        })
    }
Regex Scope

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.

protected replacePropertyNamesForTheWholeQuery(statement: string) {
    const replacements: { [key: string]: { [key: string]: string } } = {}
    const noMetadataAliasPrefixes = new Set<string>()

    for (const alias of this.expressionMap.aliases) {
        const replaceAliasNamePrefix =
            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 Path

        for (const relation of alias.metadata.relations) {
            if (relation.joinColumns.length > 0)
                replacements[replaceAliasNamePrefix][
                    relation.propertyPath
                ] = relation.joinColumns[0].databaseName
        }

        for (const relation of alias.metadata.relations) {
            const allColumns = [
                ...relation.joinColumns,
                ...relation.inverseJoinColumns,
            ]
            for (const joinColumn of allColumns) {
                const propertyKey = `${relation.propertyPath}.${
                    joinColumn.referencedColumn!.propertyPath
                }`
                replacements[replaceAliasNamePrefix][propertyKey] =
                    joinColumn.databaseName
            }
        }

        for (const column of alias.metadata.columns) {
            replacements[replaceAliasNamePrefix][column.databaseName] =
                column.databaseName
        }

        for (const column of alias.metadata.columns) {
            replacements[replaceAliasNamePrefix][column.propertyName] =
                column.databaseName
        }

        for (const column of alias.metadata.columns) {
            replacements[replaceAliasNamePrefix][column.propertyPath] =
                column.databaseName
        }
    }

    const replacementKeys = Object.keys(replacements)
    const allPrefixKeys = [...replacementKeys, ...noMetadataAliasPrefixes]
    const replaceAliasNamePrefixes = allPrefixKeys
        .map((key) => escapeRegExp(key))
        .join("|")

    if (allPrefixKeys.length > 0) {
        statement = statement.replace(
            new RegExp(
                // 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) => {
                let match: string, pre: string, p: string
                if (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-is
                    if (noMetadataAliasPrefixes.has(matches[2])) {
                        return `${pre}${this.escape(
                            matches[2].substring(0, matches[2].length - 1),
                        )}.${this.escape(p)}`
                    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SelectQueryBuilder error when using subqueries and pagination

1 participant