Repository navigation
fix(sqlite): preserve leading slash in database URLs #897
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -71,7 +71,13 @@ impl ConnectionInfo { | |||||||||||||||||||||||
| Some(url.username().to_string()) | ||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||
| password: url.password().map(|s| s.to_string()), | ||||||||||||||||||||||||
| database: if url.path().len() > 1 { | ||||||||||||||||||||||||
| database: if url.scheme() == "sqlite" { | ||||||||||||||||||||||||
| match url.host_str() { | ||||||||||||||||||||||||
| Some(host) if !host.is_empty() => Some(format!("{}{}", host, url.path())), | ||||||||||||||||||||||||
| _ if url.path().len() > 1 => Some(url.path().to_string()), | ||||||||||||||||||||||||
| _ => None, | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } else if url.path().len() > 1 { | ||||||||||||||||||||||||
| Some(url.path().trim_start_matches('/').to_string()) | ||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||
| None | ||||||||||||||||||||||||
|
|
@@ -233,6 +239,7 @@ mod tests { | |||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| assert_eq!(info.scheme, "sqlite"); | ||||||||||||||||||||||||
| assert_eq!(info.database_type(), "sqlite"); | ||||||||||||||||||||||||
| assert_eq!(info.database, Some("/path/to/db.sqlite".to_string())); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| #[test] | ||||||||||||||||||||||||
|
|
@@ -244,4 +251,11 @@ mod tests { | |||||||||||||||||||||||
| assert_eq!(info.path, Some("/path/to/database.db".to_string())); | ||||||||||||||||||||||||
| assert_eq!(info.database_type(), "sqlite"); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| #[test] | ||||||||||||||||||||||||
| fn test_parse_sqlite_relative_url() { | ||||||||||||||||||||||||
| let info = ConnectionInfo::parse("sqlite://rel.sqlite").unwrap(); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| assert_eq!(info.scheme, "sqlite"); | ||||||||||||||||||||||||
| assert_eq!(info.database, Some("rel.sqlite".to_string())); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
Comment on lines
+260
to
261
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A guard for it (fails today with
Suggested change
|
||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -42,6 +42,16 @@ impl SqliteConnector { | |||||||||||||||
| }); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| if connection_info.scheme == "sqlite" | ||||||||||||||||
| && (connection_info.username.is_some() | ||||||||||||||||
| || connection_info.password.is_some() | ||||||||||||||||
| || connection_info.port.is_some()) | ||||||||||||||||
| { | ||||||||||||||||
| return Err(DataProfilerError::DatabaseConfigError { | ||||||||||||||||
| message: "SQLite URLs do not support credentials or ports".to_string(), | ||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: git show e6e3cee4e1fc1ce35a260bbae5d4fadf22ab22b5:crates/dataprof-db/src/connectors/sqlite.rs | nl -ba | sed -n '1,125p'
rg -n 'DatabaseConfigError|SQLite URLs do not support|to_string\\(\\)|format!.*error' crates/dataprof-db/srcRepository: AndreaBozzo/dataprof Length of output: 5917 Name the rejected form without exposing credentials. This error must identify the rejected SQLite URL form and provide a supported alternative. Do not include the raw URL, because it can contain credentials. Use a redacted form or the parsed components, followed by an example such as 🐛 Suggested fix--- "a/crates/dataprof-db/src/connectors/sqlite.rs"
+++ "b/crates/dataprof-db/src/connectors/sqlite.rs"
@@ -48,7 +48,10 @@
|| connection_info.port.is_some())
{
return Err(DataProfilerError::DatabaseConfigError {
- message: "SQLite URLs do not support credentials or ports".to_string(),
+ message: format!(
+ "Unsupported SQLite URL form: credentials or ports are not supported; \
+ use sqlite:///absolute/path or sqlite://relative.db"
+ ),
});
}
📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #839 asks that a rejected form names the one to use. Keep the raw URL out, since it may carry a password:
Suggested change
|
||||||||||||||||
| }); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| Ok(Self { | ||||||||||||||||
| config, | ||||||||||||||||
| connection_info, | ||||||||||||||||
|
|
||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -106,6 +106,7 @@ pub fn create_connector( | |
| || connection_str.ends_with(".db") | ||
| || connection_str.ends_with(".sqlite") | ||
| || connection_str == ":memory:" | ||
| || connection_str == "sqlite::memory:" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
# Inspect path construction and the in-memory regression test.
rg -n -C 10 'get_db_path|SqlitePoolOptions|sqlite::memory:|database_list' \
crates/dataprof-db/src/connectors/sqlite.rs \
crates/dataprof-db/tests/column_order.rsRepository: AndreaBozzo/dataprof Length of output: 11472 🏁 Script executed: sed -n '1,180p' crates/dataprof-db/src/connection.rs
printf '\n--- sqlite connector ---\n'
sed -n '35,110p' crates/dataprof-db/src/connectors/sqlite.rs
printf '\n--- parser callers ---\n'
rg -n -C 5 -F -- 'ConnectionInfo::parse' crates/dataprof-db/src crates/dataprof-db/testsRepository: AndreaBozzo/dataprof Length of output: 21035 Preserve the in-memory SQLite URL.
Handle Suggested connector fix- let connection_string = if db_path == ":memory:" {
+ let connection_string = if db_path == ":memory:" || db_path == "sqlite::memory:" {🤖 Prompt for AI Agents |
||
| { | ||
| Ok(Box::new(connectors::sqlite::SqliteConnector::new(config)?)) | ||
| } else { | ||
|
|
@@ -150,7 +151,7 @@ fn apply_environment_configuration( | |
| .apply_to_connection_string(config.connection_string.clone(), database_type); | ||
| } | ||
|
|
||
| if config.load_credentials_from_env { | ||
| if config.load_credentials_from_env && database_type != "sqlite" { | ||
| let credentials = DatabaseCredentials::from_environment(database_type); | ||
| config.connection_string = | ||
| credentials.apply_to_connection_string(&config.connection_string); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -202,3 +202,113 @@ async fn the_order_is_the_same_on_every_run() { | |
|
|
||
| connector.disconnect().await.unwrap(); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn sqlite_absolute_url_opens_existing_database() { | ||
| let (_dir, db_path) = fixture(1).await; | ||
|
|
||
| let path = db_path.replace('\\', "/"); | ||
| let url = if cfg!(windows) { | ||
| format!("sqlite:///{}", path) | ||
| } else { | ||
| format!("sqlite://{}", path) | ||
| }; | ||
| let mut connector = connect(&url).await; | ||
|
|
||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| let columns = connector.profile_query("SELECT * FROM t").await.unwrap(); | ||
| assert_eq!(names(&columns), DECLARED_ORDER); | ||
|
|
||
| connector.disconnect().await.unwrap(); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn sqlite_absolute_url_with_default_config_opens_existing_database() { | ||
| let (_dir, db_path) = fixture(1).await; | ||
|
|
||
| let path = db_path.replace('\\', "/"); | ||
| let url = if cfg!(windows) { | ||
| format!("sqlite:///{}", path) | ||
| } else { | ||
| format!("sqlite://{}", path) | ||
| }; | ||
|
|
||
| let mut connector = create_connector(DatabaseConfig { | ||
| connection_string: url, | ||
| ..Default::default() | ||
| }) | ||
| .unwrap(); | ||
|
|
||
| connector.connect().await.unwrap(); | ||
|
|
||
| let columns = connector.profile_query("SELECT * FROM t").await.unwrap(); | ||
| assert_eq!(names(&columns), DECLARED_ORDER); | ||
| assert_eq!(columns.row_count(), 1); | ||
|
|
||
| connector.disconnect().await.unwrap(); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn sqlite_relative_url_opens_existing_database() { | ||
| let cwd = std::env::current_dir().unwrap(); | ||
| let dir = tempfile::tempdir_in(&cwd).unwrap(); | ||
| let db_path = dir.path().join("relative.db"); | ||
| std::fs::File::create(&db_path).unwrap(); | ||
|
|
||
| let pool = SqlitePoolOptions::new() | ||
| .max_connections(1) | ||
| .connect(&format!("sqlite://{}", db_path.display())) | ||
| .await | ||
| .unwrap(); | ||
| sqlx::query("CREATE TABLE relative_marker (id INTEGER)") | ||
| .execute(&pool) | ||
| .await | ||
| .unwrap(); | ||
| sqlx::query("INSERT INTO relative_marker (id) VALUES (1)") | ||
| .execute(&pool) | ||
| .await | ||
| .unwrap(); | ||
| pool.close().await; | ||
|
|
||
| let relative_path = db_path.strip_prefix(&cwd).unwrap(); | ||
| let relative_path = relative_path.to_string_lossy().replace('\\', "/"); | ||
|
|
||
| let mut connector = create_connector(DatabaseConfig { | ||
| connection_string: format!("sqlite://{relative_path}"), | ||
| ..Default::default() | ||
| }) | ||
| .unwrap(); | ||
|
|
||
| connector.connect().await.unwrap(); | ||
|
|
||
| let columns = connector | ||
| .profile_query("SELECT * FROM relative_marker") | ||
| .await | ||
| .unwrap(); | ||
| assert_eq!(names(&columns), ["id"]); | ||
|
|
||
| connector.disconnect().await.unwrap(); | ||
| drop(connector); | ||
| std::fs::remove_file(&db_path).unwrap(); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn sqlite_explicit_memory_url_connects() { | ||
| let mut connector = create_connector(DatabaseConfig { | ||
| connection_string: "sqlite::memory:".to_string(), | ||
| ..Default::default() | ||
| }) | ||
| .unwrap(); | ||
|
|
||
| connector.connect().await.unwrap(); | ||
| connector.disconnect().await.unwrap(); | ||
| } | ||
|
|
||
| #[test] | ||
| fn sqlite_url_with_credentials_is_rejected() { | ||
| let result = create_connector(DatabaseConfig { | ||
| connection_string: "sqlite://user:password@server/database.db".to_string(), | ||
| ..Default::default() | ||
| }); | ||
|
|
||
| assert!(result.is_err(), "SQLite URLs must not accept credentials"); | ||
| } | ||
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.
sqlite://C:/pathstill becomesC/path:urlreadsCas the host. sqlx reads this form asC:/path, so matching it keeps both sides in agreement: