Skip to content
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

database configuration abstraction is very leaky #2945

Closed
CobaltCause opened this issue Jun 7, 2023 · 3 comments · Fixed by #2956
Closed

database configuration abstraction is very leaky #2945

CobaltCause opened this issue Jun 7, 2023 · 3 comments · Fixed by #2956
Labels
bug Something isn't working

Comments

@CobaltCause
Copy link
Contributor

CobaltCause commented Jun 7, 2023

pub fn get_database_url(&self) -> String {
let conf = &self.database;
format!(
"postgres://{}:{}@{}:{}/{}",
utf8_percent_encode(&conf.user, NON_ALPHANUMERIC),
utf8_percent_encode(&conf.password, NON_ALPHANUMERIC),
conf.host,
conf.port,
utf8_percent_encode(&conf.database, NON_ALPHANUMERIC),
)
}

This doesn't handle the use of unix domain sockets properly, for example. It looks like converting everything into a connection URI is forced upon you by the database connection management crate being used by this function signature. As such, it might be best/easiest to change

pub database: DatabaseConfig,

to pub database: String (which would then take a connection URI from the file without having to transform it) to reflect this, instead of trying to expose a structured interface but failing to handle the possible options.

I know the LEMMY_DATABASE_URL environment variable exists and can take a raw connection URI without transforming it, but I think the file configuration should be able to be just as powerful as the environment variable. Having to use the environment variable just to work around this issue feels pretty hacky.

@CobaltCause CobaltCause added the bug Something isn't working label Jun 7, 2023
CobaltCause added a commit to CobaltCause/nixpkgs that referenced this issue Jun 7, 2023
Lemmy checks the environment variable before the configuration file;
i.e. if the file is used to configure the database but the environment
varible is set to anything, the connection will fail because it'll
ignore the file. This was the previous behavior.

Now, the environment variable will be unset unless the user explicitly
chooses to set it, which makes the file-based configuration function
correctly. It's also possible to manually set the environment variable,
which has the major advantage of working around [this issue][0], which
prevents certain setups from working.

[0]: LemmyNet/lemmy#2945
CobaltCause added a commit to CobaltCause/nixpkgs that referenced this issue Jun 7, 2023
Lemmy checks the environment variable before the configuration file;
i.e. if the file is used to configure the database but the environment
variable is set to anything, the connection will fail because it'll
ignore the file. This was the previous behavior.

Now, the environment variable will be unset unless the user explicitly
chooses to set it, which makes the file-based configuration function
correctly. It's also possible to manually set the environment variable,
which has the major advantage of working around [this issue][0], which
prevents certain setups from working.

[0]: LemmyNet/lemmy#2945
@CobaltCause
Copy link
Contributor Author

To do this in a non-breaking way,

#[serde(with = "either::serde_untagged")]
pub database: Either<String, DatabaseConfig>

could be used.

@CobaltCause
Copy link
Contributor Author

If the maintainers are interested in a change to address this issue, I'd be happy to try to contribute it.

@Nutomic
Copy link
Member

Nutomic commented Jun 7, 2023

Sure, contributions are always welcome.

CobaltCause added a commit to CobaltCause/nixpkgs that referenced this issue Jun 12, 2023
Lemmy checks the environment variable before the configuration file;
i.e. if the file is used to configure the database but the environment
variable is set to anything, the connection will fail because it'll
ignore the file. This was the previous behavior.

Now, the environment variable will be unset unless the user explicitly
chooses to set it, which makes the file-based configuration function
correctly. It's also possible to manually set the environment variable,
which has the major advantage of working around [this issue][0], which
prevents certain setups from working.

[0]: LemmyNet/lemmy#2945
CobaltCause added a commit to CobaltCause/nixpkgs that referenced this issue Jun 12, 2023
Lemmy checks the environment variable before the configuration file;
i.e. if the file is used to configure the database but the environment
variable is set to anything, the connection will fail because it'll
ignore the file. This was the previous behavior.

Now, the environment variable will be unset unless the user explicitly
chooses to set it, which makes the file-based configuration function
correctly. It's also possible to manually set the environment variable,
which has the major advantage of working around [this issue][0], which
prevents certain setups from working.

[0]: LemmyNet/lemmy#2945
CobaltCause added a commit to CobaltCause/nixpkgs that referenced this issue Jun 12, 2023
Lemmy checks the environment variable before the configuration file;
i.e. if the file is used to configure the database but the environment
variable is set to anything, the connection will fail because it'll
ignore the file. This was the previous behavior.

Now, the environment variable will be unset unless the user explicitly
chooses to set it, which makes the file-based configuration function
correctly. It's also possible to manually set the environment variable,
which has the major advantage of working around [this issue][0], which
prevents certain setups from working.

[0]: LemmyNet/lemmy#2945
CobaltCause added a commit to CobaltCause/nixpkgs that referenced this issue Jun 12, 2023
Lemmy checks the environment variable before the configuration file;
i.e. if the file is used to configure the database but the environment
variable is set to anything, the connection will fail because it'll
ignore the file. This was the previous behavior.

Now, the environment variable will be unset unless the user explicitly
chooses to set it, which makes the file-based configuration function
correctly. It's also possible to manually set the environment variable,
which has the major advantage of working around [this issue][0], which
prevents certain setups from working.

[0]: LemmyNet/lemmy#2945
CobaltCause added a commit to CobaltCause/nixpkgs that referenced this issue Jun 13, 2023
Lemmy checks the environment variable before the configuration file;
i.e. if the file is used to configure the database but the environment
variable is set to anything, the connection will fail because it'll
ignore the file. This was the previous behavior.

Now, the environment variable will be unset unless the user explicitly
chooses to set it, which makes the file-based configuration function
correctly. It's also possible to manually set the environment variable,
which has the major advantage of working around [this issue][0], which
prevents certain setups from working.

[0]: LemmyNet/lemmy#2945
CobaltCause added a commit to CobaltCause/nixpkgs that referenced this issue Jun 13, 2023
Lemmy checks the environment variable before the configuration file;
i.e. if the file is used to configure the database but the environment
variable is set to anything, the connection will fail because it'll
ignore the file. This was the previous behavior.

Now, the environment variable will be unset unless the user explicitly
chooses to set it, which makes the file-based configuration function
correctly. It's also possible to manually set the environment variable,
which has the major advantage of working around [this issue][0], which
prevents certain setups from working.

[0]: LemmyNet/lemmy#2945
CobaltCause added a commit to CobaltCause/nixpkgs that referenced this issue Jun 14, 2023
Lemmy checks the environment variable before the configuration file;
i.e. if the file is used to configure the database but the environment
variable is set to anything, the connection will fail because it'll
ignore the file. This was the previous behavior.

Now, the environment variable will be unset unless the user explicitly
chooses to set it, which makes the file-based configuration function
correctly. It's also possible to manually set the environment variable,
which has the major advantage of working around [this issue][0], which
prevents certain setups from working.

[0]: LemmyNet/lemmy#2945
happysalada pushed a commit to NixOS/nixpkgs that referenced this issue Jun 15, 2023
Lemmy checks the environment variable before the configuration file;
i.e. if the file is used to configure the database but the environment
variable is set to anything, the connection will fail because it'll
ignore the file. This was the previous behavior.

Now, the environment variable will be unset unless the user explicitly
chooses to set it, which makes the file-based configuration function
correctly. It's also possible to manually set the environment variable,
which has the major advantage of working around [this issue][0], which
prevents certain setups from working.

[0]: LemmyNet/lemmy#2945
github-actions bot pushed a commit to NixOS/nixpkgs that referenced this issue Jun 15, 2023
Lemmy checks the environment variable before the configuration file;
i.e. if the file is used to configure the database but the environment
variable is set to anything, the connection will fail because it'll
ignore the file. This was the previous behavior.

Now, the environment variable will be unset unless the user explicitly
chooses to set it, which makes the file-based configuration function
correctly. It's also possible to manually set the environment variable,
which has the major advantage of working around [this issue][0], which
prevents certain setups from working.

[0]: LemmyNet/lemmy#2945

(cherry picked from commit 7621077)
happysalada pushed a commit to NixOS/nixpkgs that referenced this issue Jun 16, 2023
Lemmy checks the environment variable before the configuration file;
i.e. if the file is used to configure the database but the environment
variable is set to anything, the connection will fail because it'll
ignore the file. This was the previous behavior.

Now, the environment variable will be unset unless the user explicitly
chooses to set it, which makes the file-based configuration function
correctly. It's also possible to manually set the environment variable,
which has the major advantage of working around [this issue][0], which
prevents certain setups from working.

[0]: LemmyNet/lemmy#2945

(cherry picked from commit 7621077)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants