Add DB_URL support #240
Add DB_URL support #240
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.
| #[serde(rename = "DB_HOST", default)] | ||
| pub db_host: String, | ||
| #[serde(rename = "DB_PORT")] | ||
| #[serde(rename = "DB_PORT", default)] | ||
| pub db_port: u16, | ||
| #[serde(rename = "DB_USER")] | ||
| #[serde(rename = "DB_USER", default)] | ||
| pub db_user: String, |
There was a problem hiding this comment.
Using default attribute on non-Optional fields with primitive types may cause deserialization issues. When DB_URL is provided, these fields should be Option<String> and Option<u16> respectively, or the struct should validate that either DB_URL or all individual parameters are present.
| 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), |
No description provided.