fix(quaint): add url decode for MSSQL database name#5786
fix(quaint): add url decode for MSSQL database name#5786dimsssss wants to merge 4 commits intoprisma:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
WalkthroughMSSQL URL database names are now percent-decoded on access. Changes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
quaint/src/connector/connection_info.rsquaint/src/connector/mssql/native/mod.rsquaint/src/connector/mssql/url.rsschema-engine/connectors/sql-schema-connector/src/flavour/mssql.rs
| let (db_name, master_uri) = Self::master_url(¶ms.connector_params.connection_string)?; | ||
| assert!(db_name != "master", "Cannot drop the `master` database."); | ||
| let mut conn = Connection::new(&master_uri.to_string()).await?; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider returning an error instead of panicking.
Using assert! in production code will cause a panic if the user attempts to drop the master database. While this is a clear programming error, returning a ConnectorError would provide a more graceful failure mode and better error messages to end users.
♻️ Proposed refactor to return an error instead of panicking
let params = self.state.get_unwrapped_params();
let (db_name, master_uri) = Self::master_url(¶ms.connector_params.connection_string)?;
- assert!(db_name != "master", "Cannot drop the `master` database.");
+ if db_name == "master" {
+ return Err(ConnectorError::from_msg("Cannot drop the `master` database.".to_owned()));
+ }
let mut conn = Connection::new(&master_uri.to_string()).await?;
Merging this PR will not alter performance
Comparing 
Footnotes
|
|
Thanks for the contribution! |
Thank you, I applied it. |
| /// Decoded database name. Defaults to `master`. | ||
| pub fn dbname(&self) -> Cow<'_, str> { | ||
| let db = self.query_params.database(); | ||
| match percent_decode(db.as_bytes()).decode_utf8() { | ||
| Ok(decoded) => decoded, | ||
| Err(_) => { | ||
| tracing::warn!("Couldn't decode dbname to UTF-8, using the non-decoded version."); | ||
| Cow::Borrowed(db) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider logging the error details for easier debugging.
The warning message could include the actual Utf8Error to help diagnose issues with malformed database names. This would provide more context about where the invalid UTF-8 sequence was encountered.
💡 Suggested improvement
match percent_decode(db.as_bytes()).decode_utf8() {
Ok(decoded) => decoded,
- Err(_) => {
- tracing::warn!("Couldn't decode dbname to UTF-8, using the non-decoded version.");
+ Err(e) => {
+ tracing::warn!(
+ "Couldn't decode dbname '{}' to UTF-8: {}. Using the non-decoded version.",
+ db,
+ e
+ );
Cow::Borrowed(db)
}
}Replace async integration test that requires a live MSSQL connection with a unit test that only verifies URL parsing of percent-encoded Chinese database names.
| #[test] | ||
| fn should_decode_percent_encoded_dbname() { | ||
| // Chinese characters: 测试库 (test database) | ||
| let url = MssqlUrl::new("sqlserver://localhost:1433;database=%E6%B5%8B%E8%AF%95%E5%BA%93;user=SA;password=pass;trustServerCertificate=true").unwrap(); | ||
| assert_eq!("测试库", url.dbname()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn should_decode_dbname_with_spaces() { | ||
| let url = MssqlUrl::new( | ||
| "sqlserver://localhost:1433;database=my%20database;user=SA;password=pass;trustServerCertificate=true", | ||
| ) | ||
| .unwrap(); | ||
| assert_eq!("my database", url.dbname()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn should_decode_dbname_with_special_characters() { | ||
| // test-db_name | ||
| let url = MssqlUrl::new( | ||
| "sqlserver://localhost:1433;database=test%2Ddb%5Fname;user=SA;password=pass;trustServerCertificate=true", | ||
| ) | ||
| .unwrap(); | ||
| assert_eq!("test-db_name", url.dbname()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn should_return_master_as_default_dbname() { | ||
| let url = | ||
| MssqlUrl::new("sqlserver://localhost:1433;user=SA;password=pass;trustServerCertificate=true").unwrap(); | ||
| assert_eq!("master", url.dbname()); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a test for the UTF-8 decode failure fallback path.
The test coverage is good for successful decoding scenarios. However, there's no test verifying the fallback behavior when percent_decode(...).decode_utf8() fails (returning Cow::Borrowed(db) with a warning). Adding such a test would ensure the error handling path is exercised.
💡 Suggested test for fallback behavior
#[test]
fn should_fallback_to_original_on_invalid_utf8() {
// %FF is not valid UTF-8
let url = MssqlUrl::new(
"sqlserver://localhost:1433;database=test%FF;user=SA;password=pass;trustServerCertificate=true",
)
.unwrap();
// Should return the original percent-encoded string since it can't decode to valid UTF-8
assert_eq!("test%FF", url.dbname());
}The same assertion was already covered by should_decode_percent_encoded_dbname.
2 participants
Title: fix(quaint): add url decode for MSSQL database name
Summary
%E6%B5%8B%E8%AF%95%E5%BA%93→测试库) were passed to the serverwithout decoding, causing connection failures
fix(quaint): add url decode for non-ascii db names) to MSSQLChanges
quaint/src/connector/mssql/url.rs: Applypercent_decodeinMssqlUrl::dbname()to return the decoded database name (&str→Cow<'_, str>)quaint/src/connector/mssql/native/mod.rs: Override the database name on the tiberiusConfigwithconfig.database(url.dbname())so thedecoded name is used for the actual connection
schema-engine/.../flavour/mssql.rs: Delegate percent-decoding inmaster_url()toMssqlUrl::dbname()instead of callingpercent_decodedirectly, eliminating duplication. Also replace redundant JDBC parsing in
drop_database()by reusingmaster_url(). This removes the need to addpercent-encodingas a dependency tosql-schema-connectorquaint/src/connector/connection_info.rs: Adapt call site to the changeddbname()return typeTest plan
测试库) and verify withSELECT DB_NAME()sql-schema-connectorMSSQL tests passSummary by CodeRabbit
Bug Fixes
Tests