-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
fix(mysql): support AnalyticDB returning version() column name in getVersion() #11555
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
""" WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant MysqlQueryRunner
participant MySQL/ADS DB
App->>MysqlQueryRunner: call getVersion()
MysqlQueryRunner->>MySQL/ADS DB: SELECT VERSION()
MySQL/ADS DB-->>MysqlQueryRunner: returns [{version: "x.y.z"}] or [{"version()": "x.y.z"}]
MysqlQueryRunner->>MysqlQueryRunner: Extract version from 'version' or 'version()'
MysqlQueryRunner-->>App: Return normalized version string
Assessment against linked issues
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/driver/mysql/MysqlQueryRunner.ts (1)
3373-3391: Excellent implementation with minor error handling consideration.The changes correctly address the AnalyticDB compatibility issue by supporting both column name variants. The use of nullish coalescing operator and proper TypeScript typing is well done.
However, consider adding error handling for the edge case where both
versionandversion()properties are undefined:const versionString = row.version ?? row["version()"]; + +if (!versionString) { + throw new TypeORMError("Unable to determine database version: neither 'version' nor 'version()' property found in query result"); +} return versionString.replace(/^([\d.]+).*$/, "$1")This would prevent a potential runtime error if the query result doesn't contain either expected property.
|
Hi @alumni , could you please help review this PR when you have time? Thanks! |
|
@alumni |
|
@Minglu-Huo the format check will fail if I will start the workflow :) Could you fix the formatting? Thanks :)
|
|
@alumni |
commit: |
|
Thanks @alumni for the feedback! I've updated the query to use lowercase |
|
Thanks for the fix 😀 |
gioboa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your help @Minglu-Huo 👍
|
@alumni |
…e in getVersion() (typeorm#11555)
Description of change
When using Alibaba Cloud AnalyticDB for MySQL (ADS), running
SELECT VERSION() AS versionreturns a column namedversion()instead ofversion, which causes TypeORM to throw a TypeError inMysqlQueryRunner.getVersion().This change updates
getVersion()to support bothversionandversion()column keys, making it compatible with ADS while keeping compatibility with standard MySQL and MariaDB.Verified locally using ADS instance, and also tested on standard MySQL.
Related issue: #11553
Pull-Request Checklist
masterbranchFixes #11553🎉 Thank you for reviewing this PR!
Summary by CodeRabbit