feat: support standard postgres env vars for db connection#674
feat: support standard postgres env vars for db connection#674
Conversation
psteinroe
force-pushed
the
feat/db-connection-string
branch
2 times, most recently
from
ede9f8d to
3f539f1
Compare
psteinroe
changed the title
psteinroe
force-pushed
the
feat/db-connection-string
branch
2 times, most recently
from
404a683 to
704f5e5
Compare
|
|
||
| /// The password to connect to the database. | ||
| #[partial(bpaf(long("password")))] | ||
| #[partial(bpaf(env("PGPASSWORD"), long("password")))] |
There was a problem hiding this comment.
This might have an unexpected side effect of leaking password in the --help message. If that's not acceptable - I'd implement a separate function for obtaining the password and call it here with bpaf(external(get_password)).
This function would have a parser for password and a parser for env, hidden. Something like this:
fn get_password() -> impl Parser<String> {
let pass = long("password")
.help("The password to connect to the database")
.argument("PASSWORD");
let epass = env("PGPASSOWRD").argument("PASSWORD").hide();
construct!([pass, epass])
}There was a problem hiding this comment.
thanks for noting this! super helpful, did not know that it leaks in --help.
There was a problem hiding this comment.
Hmm... I'm wondering. Are there any scenarios where you want to see the value of the variable in the --help message? I could make it to tell the variable name and indicate if it is set or not. Or even just the name...
There was a problem hiding this comment.
Mhh I think with just showing that it is set right now will cover the same use case that showing the actual value does today. Users could just echo it to reveal the value.
psteinroe
force-pushed
the
feat/db-connection-string
branch
from
99cd97e to
2b49a04
Compare
| &self.pghost, | ||
| &self.pgport, | ||
| &self.pguser, | ||
| &self.pgpassword, |
There was a problem hiding this comment.
not sure if we want to log the password somewhere?
There was a problem hiding this comment.
we could alternatively wrap it in a secret-string
There was a problem hiding this comment.
i opted for the easy way now because using any wrapper type would mean adding biomes merge partial to our repo to support the type... but it might make sense to bring all the biome deps in-house anyways. will note it as a follow up.
There was a problem hiding this comment.
create a note to bring biome_deserialise in-house and replace all other biome_ dependencies with ocx - their lib apis are much better and well maintained.
| { | ||
| let env_config = PartialConfiguration { | ||
| db: Some(env_db), | ||
| ..Default::default() |
There was a problem hiding this comment.
i'm a little bit confused about the ..default::default().. shouldnt we marge the configuration here? or are the defaults all "None"?
There was a problem hiding this comment.
i double checked and it does not overwrite
| username: pguser, | ||
| password: pgpassword, | ||
| database: pgdatabase, | ||
| ..Default::default() |
There was a problem hiding this comment.
..Default::default() returns all None?
just wondering if we need to be careful about not overwriting everything with default values?
Support DATABASE_URL, PGHOST, PGPORT, PGUSER, PGPASSWORD, PGDATABASE environment variables for configuring the database connection. Env vars take highest priority, overriding CLI args, LSP settings, and config file values. Closes #302
Support DATABASE_URL, PGHOST, PGPORT, PGUSER, PGPASSWORD, PGDATABASE environment variables for configuring the database connection. Env vars take highest priority, overriding CLI args, LSP settings, and config file values. Closes #302
Env config is computed once at ServerFactory creation and threaded through to Session, which merges it as the highest-priority layer during config loading. Tests use ServerFactory::default() which provides None, avoiding DATABASE_URL interference from sqlx::test.
bpaf's env() renders the current env var value in --help, e.g. [env:PGPASSWORD = "secret"]. Remove env() from sensitive fields (password, connection_string) and use from_env() merge in the CLI path instead, matching the LSP approach.
DATABASE_URL and PGPASSWORD now display "set" instead of their actual values in the environment variable overview.
Custom Debug impls for DatabaseConfiguration and DatabaseSettings mask password and connection_string fields to prevent accidental credential exposure in logs or debug output.
Merge order: config file → env vars → CLI args. This ensures explicit --connection-string flags take precedence over DATABASE_URL.
psteinroe
force-pushed
the
feat/db-connection-string
branch
from
44b1bb1 to
42c1b77
Compare
3 participants
Support
DATABASE_URL,PGHOST,PGPORT,PGUSER,PGPASSWORD,PGDATABASEenv vars for configuring the database connection with highest priority.we use bpaf(env()) for the cli, and support loading the db config from_env for the lsp.
These env var names follow the standard libpq environment variables (
PG*) and the widely adoptedDATABASE_URLconvention used by tools likesqlx,diesel,prisma, anddotenv.Example
DATABASE_URL=postgres://user:pass@localhost:5432/mydb postgres-language-server check file.sql # or individual vars PGHOST=localhost PGPORT=5432 PGUSER=user PGPASSWORD=pass PGDATABASE=mydb postgres-language-server check file.sqlPrecedence (highest to lowest)
Closes #302