Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for a custom database connection URL (DB_URL) as an alternative to specifying individual connection parameters (host, port, user, etc.). When DB_URL is provided, individual connection parameters become optional and can fall back to default values.
Key changes:
- Added
db_urlfield across configuration structures (CLI args, dotenv, and config structs) - Modified connection parameter validation to allow defaults when
DB_URLis present - Updated credential string builders to prioritize
DB_URLwhen available
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/common/cli.rs | Added CLI argument for custom database URL |
| src/common/dotenv.rs | Added support for reading DB_URL from environment variables |
| src/common/config.rs | Modified configuration logic to make individual DB parameters optional when DB_URL is provided, and updated credential string builders |
| tests/sample/sample.queries.ts | Added TypeScript query type definitions (appears unrelated to DB_URL feature) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| db_host, | ||
| db_port, | ||
| db_user, |
There was a problem hiding this comment.
[nitpick] Removing .to_owned() calls here changes the ownership semantics. Since these are now owned values instead of references, this is correct, but the surrounding code at lines 328-329 still uses .to_owned() creating inconsistency. Consider applying the same pattern to db_pass and db_name for consistency.
| export interface ISampleSelectQueryQuery { | ||
| params: SampleSelectQueryParams; | ||
| result: ISampleSelectQueryResult; | ||
| } |
There was a problem hiding this comment.
[nitpick] The interface name ISampleSelectQueryQuery contains redundant 'Query' suffix. Consider renaming to ISampleSelectQuery for clarity.
JasonShin
force-pushed
the
support-db-url
branch
from
61f306e to
7022f96
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let db_host = match (db_url.is_some(), db_host_chain()) { | ||
| (true, Some(v)) => v, | ||
| (true, None) => String::new(), | ||
| (false, Some(v)) => v, | ||
| (false, None) => panic!( |
There was a problem hiding this comment.
The pattern (db_url.is_some(), db_host_chain()) is repeated three times for db_host, db_port, and db_user. Consider extracting this logic into a helper function to reduce code duplication and improve maintainability.
| (true, Some(v)) => v, | ||
| (true, None) => 0, | ||
| (false, Some(v)) => v, |
There was a problem hiding this comment.
Using 0 as a default port when db_url is provided but db_port is not may cause issues if the port value is used elsewhere in the code for validation or logging. Consider using a more explicit sentinel value or documenting this behavior, as port 0 is typically reserved and may not be a valid database port.
| (true, Some(v)) => v, | |
| (true, None) => 0, | |
| (false, Some(v)) => v, | |
| (true, Some(v)) => Some(v), | |
| (true, None) => None, | |
| (false, Some(v)) => Some(v), |
JasonShin
force-pushed
the
support-db-url
branch
from
7022f96 to
a729125
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let demo_dir = tempdir()?; | ||
| let demo_path = demo_dir.path(); | ||
| let sql_file_path = demo_path.join("test-query.sql"); | ||
| let sample_query_path = demo_path.join("test-query.queries.ts"); |
There was a problem hiding this comment.
The variable sample_query_path is declared but never used in this test. Consider removing it to avoid confusion.
| let sample_query_path = demo_path.join("test-query.queries.ts"); |
| let demo_dir = tempdir()?; | ||
| let demo_path = demo_dir.path(); | ||
| let sql_file_path = demo_path.join("test-query.sql"); | ||
| let sample_query_path = demo_path.join("test-query.queries.ts"); |
There was a problem hiding this comment.
The variable sample_query_path is declared but never used in this test. Consider removing it to avoid confusion.
| let sample_query_path = demo_path.join("test-query.queries.ts"); |
|
|
||
| let db_port = match (db_url.is_some(), db_port_chain()) { | ||
| (true, Some(v)) => v, | ||
| (true, None) => 0, |
There was a problem hiding this comment.
Setting db_port to 0 when db_url is provided but no port chain value exists could cause issues if the DB_URL is malformed or missing. Consider validating that db_url is actually valid before allowing zero as a fallback value.
|
|
||
| let db_user = match (db_url.is_some(), db_user_chain()) { | ||
| (true, Some(v)) => v, | ||
| (true, None) => String::new(), |
There was a problem hiding this comment.
Setting db_user to an empty string when db_url is provided but no user chain value exists could cause issues if the DB_URL is malformed or missing. Consider validating that db_url is actually valid before allowing empty fallback values.
No description provided.