Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

fix: order by nulls (first | last) for sqlserver issue #10860#10861

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
awaismehmood88 wants to merge2 commits intotypeorm:master
base:master
Choose a base branch
Loading
fromawaismehmood88:fix-order-by-nulls-first-last-for-sqlserver

Conversation

@awaismehmood88
Copy link

@awaismehmood88awaismehmood88 commentedApr 30, 2024
edited
Loading

Description of change

Fixes#10860
This change fix nulls sort for sqlserver

Pull-Request Checklist

  • Code is up-to-date with themaster branch
  • npm run format to apply prettier formatting
  • npm run test passes with this change
  • This pull request links relevant issues asFixes #10860
  • There are new or updated unit tests validating the change
  • Documentation has been updated to reflect this change
  • The new commits follow conventions explained inCONTRIBUTING.md

Copy link
Collaborator

@gioboagioboa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks@awaismehmood88 for you help
is it still a valid PR ?

@awaismehmood88
Copy link
Author

Thanks@awaismehmood88 for you help is it still a valid PR ?

Yes, it is valid

gioboa reacted with thumbs up emoji

Copy link
Collaborator

@gioboagioboa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Can you add a test for this specific fix please?
This will help a lot our review

Copy link
Collaborator

@gioboagioboa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Can you solve the conflicts please?

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

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                   Impact
Security
Escape column names in SQL

Escape thecolumnName increateOrderByNullsSQLServer to prevent SQL injection
vulnerabilities.

src/query-builder/SelectQueryBuilder.ts [3433-3454]

 private createOrderByNullsSQLServer(orderBys: any) {     let sqlServerOrder = undefined     if (this.connection.driver.options.type === "mssql")         sqlServerOrder = Object.keys(orderBys)             .filter(                 (columnName) => typeof orderBys[columnName] !== "string",             )             .map((columnName) => {                 const last =                     (orderBys[columnName] as any).nulls === "NULLS LAST"+                const escapedColumn = this.escape(columnName)                 return (                     `CASE WHEN ` +-                    columnName ++                    escapedColumn +                     ` IS NULL THEN ${last ? "1" : "0"} ELSE ${                         last ? "0" : "1"                     } END`                 )             })             .join(", ")      return sqlServerOrder ? sqlServerOrder + ", " : "" }
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a potential SQL injection vulnerability where a user-provided column name is used in a raw SQL fragment without escaping, which is a critical security issue.

High
Possible issue
Allow null in WHERE conditions

Remove the check fornull in thebuildWhere method to allow queries withIS NULL
conditions.

src/query-builder/SelectQueryBuilder.ts [4334]

-if (where[key] === undefined || where[key] === null) continue+if (where[key] === undefined) continue
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly points out that filteringnull values inWHERE conditions is a breaking change, asnull is a valid value forIS NULL checks, and this change would silently alter query behavior.

Medium
  • More

@awaismehmood88
Copy link
Author

Can you solve the conflicts please?

Sorry for the late reply, I have resolved the conflicts; please merge now.

@pkg-pr-new
Copy link

commit:063f959

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

Reviewers

@gioboagioboagioboa left review comments

@alumnialumniAwaiting requested review from alumni

@sgarnersgarnerAwaiting requested review from sgarner

@naorpelednaorpeledAwaiting requested review from naorpeled

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bug: ORDER BY NULLS (FIRST | LAST) for SQLServer

2 participants

@awaismehmood88@gioboa

[8]ページ先頭

©2009-2025 Movatter.jp