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

Added validate function to PgConfig and invoked it in finalize and pool initialization #1546

Merged
merged 10 commits into from
Oct 23, 2024

Conversation

Saru2003
Copy link
Contributor

This PR addresses issue #1542, which states that the PostgreSQL pool size should not be allowed to be set to less than 1.

Changes:

  1. Introduced a validate method in the PgConfig struct to check the following:
  • The pool_size must be greater than or equal to 1.
  • A connection string must be provided.
  1. Updated the finalize and resolve methods to call the validate method. This ensures that the configuration is validated before proceeding with further processing.

  2. Updated pool.rs, ensures the validation process is integrated when creating a new pool by calling the validate method from the PgConfig instance.

Please review my implementation and provide feedback.

@Saru2003
Copy link
Contributor Author

@nyurik Kindly refer to the latest commit I have made.

Copy link
Member

@nyurik nyurik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few minor things, and this will be ready to go! :)

Ok(res)
}

pub async fn resolve(&mut self, id_resolver: IdResolver) -> MartinResult<TileInfoSources> {
self.validate()?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to validate it in so many places - here, in the pool, and in finalize? I don't remember for sure which codepath always executes, but we should probably limit it to just one. Probably finalize? Please check. Thx!

@@ -69,4 +69,7 @@ pub enum PgError {

#[error(r#"Unable to get tile {2:#} with {:?} params from {1}: {0}"#, query_to_json(.3.as_ref()))]
GetTileWithQueryError(#[source] TokioPgError, String, TileCoord, Option<UrlQuery>),

#[error("Validation error: {0}")]
ValidationError(String),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets call this ConfigError. Also, at least for now, all errors are &'static str - so we don't need to call .into() on them to convert str into an allocated String.

Copy link
Member

@nyurik nyurik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!!

@nyurik nyurik merged commit 1326157 into maplibre:main Oct 23, 2024
18 of 19 checks passed
@Saru2003
Copy link
Contributor Author

@nyurik Thanks for merging! Could you please add the "hacktoberfest-accepted" label? Thanks for considering!

@CommanderStorm
Copy link
Collaborator

@Saru2003 adding labels is only nessesary if a PR is not accepted via an approving review or merged.

@nyurik
Copy link
Member

nyurik commented Oct 23, 2024

i'm a bit confused - what labeling are you talking about? We only add labels relevant to the Martin project, e.g. which subsystem the PR applies to, or things like "help needed", etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants