feat: [#540] Move the Postgres driver to a single package#1
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package : could not load export data: no export data for "github.com/goravel/framework/contracts/config"" WalkthroughThe pull request introduces a comprehensive PostgreSQL driver package for the Goravel framework. It creates a standalone package Changes
Assessment against linked issues
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (9)
docker.go (2)
50-51: Correct the error message grammarThe error message in line 51 should be corrected for proper grammar and clarity.
Apply this diff to fix the error message:
- return fmt.Errorf("no container id return when creating Postgres docker") + return fmt.Errorf("no container ID returned when creating Postgres Docker container")
144-155: Implement context with timeout in connection retriesThe
connectmethod attempts to connect up to 60 times with a 2-second interval, resulting in a potential 2-minute wait time if the database is unreachable. This could block the application unnecessarily.Consider using
context.Contextwith a timeout to manage retries and allow for cancellation if the connection cannot be established within a reasonable time frame.service_provider.go (1)
25-27: Document empty Boot methodAdd a comment explaining why the Boot method is empty or remove it if not needed.
+// Boot performs no operation as no boot-time initialization is required func (receiver *ServiceProvider) Boot(app foundation.Application) { }contracts/config.go (2)
7-11: Improve ConfigBuilder interface documentationAdd detailed documentation for the interface methods to explain their purpose and return values.
type ConfigBuilder interface { + // Config returns the current configuration Config() contractsconfig.Config + // Reads returns a list of read-only database configurations Reads() []FullConfig + // Writes returns a list of write-enabled database configurations Writes() []FullConfig }
29-42: Document FullConfig fieldsAdd field documentation to explain the purpose of each configuration option.
type FullConfig struct { Config + // Driver specifies the database driver name Driver string + // Connection specifies the connection name in database config Connection string + // Prefix is added to all table names Prefix string + // Singular determines if table names should be singular Singular bool + // Charset specifies the character set for MySQL and SQLServer Charset string + // Loc specifies the time zone location for MySQL Loc string + // Sslmode specifies the SSL mode for PostgreSQL Sslmode string + // Timezone specifies the timezone for PostgreSQL Timezone string + // NoLowerCase prevents automatic lowercase conversion of table names NoLowerCase bool + // NameReplacer provides custom table name transformation NameReplacer Replacer }docker_test.go (2)
14-18: Consider using environment variables for test credentials.While hardcoded credentials are acceptable for tests, using environment variables would allow for easier configuration changes and prevent accidental commits of sensitive values.
-const ( - testDatabase = "goravel" - testUsername = "goravel" - testPassword = "Framework!123" -) +var ( + testDatabase = getEnvOrDefault("TEST_DB", "goravel") + testUsername = getEnvOrDefault("TEST_USER", "goravel") + testPassword = getEnvOrDefault("TEST_PASS", "Framework!123") +) + +func getEnvOrDefault(key, defaultValue string) string { + if value := os.Getenv(key); value != "" { + return value + } + return defaultValue +}
51-56: Use parameterized queries to prevent SQL injection.Even in tests, it's good practice to use parameterized queries to maintain consistency with production code patterns.
- res := instance.Exec(` - CREATE TABLE users ( - id SERIAL PRIMARY KEY NOT NULL, - name varchar(255) NOT NULL - ); - `) + res := instance.Exec(`CREATE TABLE users (id SERIAL PRIMARY KEY NOT NULL, name varchar(255) NOT NULL)`) - res = instance.Exec(` - INSERT INTO users (name) VALUES ('goravel'); - `) + res = instance.Exec(`INSERT INTO users (name) VALUES ($1)`, "goravel")Also applies to: 59-61
config.go (1)
51-100: Consider refactoring the long method.The
fillDefaultmethod is quite long with repeated config.GetString calls. Consider extracting the configuration retrieval logic into a separate method.+func (c *ConfigBuilder) getConfigValue(key, defaultValue string) string { + return c.config.GetString(fmt.Sprintf("database.connections.%s.%s", c.connection, key), defaultValue) +} func (c *ConfigBuilder) fillDefault(configs []contracts.Config) []contracts.FullConfig { // ... existing checks ... for _, config := range configs { fullConfig := contracts.FullConfig{ Config: config, Connection: c.connection, Driver: driver, - Prefix: c.config.GetString(fmt.Sprintf("database.connections.%s.prefix", c.connection)), + Prefix: c.getConfigValue("prefix", ""), // ... apply similar changes to other fields } // ... rest of the method }config_test.go (1)
21-25: Consider parameterizing the connection type.The connection type is hardcoded as "mysql" in the test suite, but this is a Postgres package. Consider using a more appropriate default or making it configurable.
func TestConfigTestSuite(t *testing.T) { suite.Run(t, &ConfigTestSuite{ - connection: "mysql", + connection: "postgres", }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
config.go(1 hunks)config_test.go(1 hunks)contracts/config.go(1 hunks)docker.go(1 hunks)docker_test.go(1 hunks)errors.go(1 hunks)facades/postgres.go(1 hunks)go.mod(1 hunks)postgres.go(1 hunks)postgres_test.go(1 hunks)service_provider.go(1 hunks)utils.go(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- errors.go
- go.mod
🔇 Additional comments (5)
postgres.go (1)
146-148:⚠️ Potential issueAvoid including sensitive information in error logs
The DSN constructed in the
connectmethod includes the username and password in plain text. Ensure that this information is not exposed in error messages or logs to prevent sensitive information leakage.Review the code to confirm that the DSN or any error containing it is not logged. If any logging occurs, consider masking sensitive information before logging.
postgres_test.go (1)
10-14: Validate driver name consistencyThe test function
TestDriverNamecorrectly ensures that the driver name has not been modified unexpectedly, which is important for maintaining compatibility.docker_test.go (1)
87-97: LGTM! Well-structured test methods.The test methods are focused and verify essential functionality.
config.go (1)
12-22: LGTM! Well-designed configuration builder.The struct is well-encapsulated and follows good design practices.
config_test.go (1)
114-229: LGTM! Well-structured table-driven tests.The test cases are comprehensive and well-organized, covering empty configs, default values, and custom configurations.
| func (r *Docker) Database(name string) (testing.DatabaseDriver, error) { | ||
| go func() { | ||
| gormDB, err := r.connect() | ||
| if err != nil { | ||
| color.Errorf("connect Postgres error: %v", err) | ||
| return | ||
| } | ||
|
|
||
| res := gormDB.Exec(fmt.Sprintf(`CREATE DATABASE "%s";`, name)) | ||
| if res.Error != nil { | ||
| color.Errorf("create Postgres database error: %v", res.Error) | ||
| } | ||
|
|
||
| if err := r.close(gormDB); err != nil { | ||
| color.Errorf("close Postgres connection error: %v", err) | ||
| } | ||
| }() | ||
|
|
||
| postgresImpl := NewDocker(name, r.username, r.password) | ||
| postgresImpl.containerID = r.containerID | ||
| postgresImpl.port = r.port | ||
|
|
||
| return postgresImpl, nil | ||
| } |
There was a problem hiding this comment.
Handle errors synchronously in the Database method
In the Database method, the database creation is performed asynchronously in a goroutine, and any errors are only logged but not returned. This can lead to silent failures and potential race conditions if the database is accessed before it's ready.
Consider modifying the code to perform database creation synchronously and properly handle errors by returning them to the caller.
Apply this diff to refactor the method:
-func (r *Docker) Database(name string) (testing.DatabaseDriver, error) {
- go func() {
- gormDB, err := r.connect()
- if err != nil {
- color.Errorf("connect Postgres error: %v", err)
- return
- }
-
- res := gormDB.Exec(fmt.Sprintf(`CREATE DATABASE "%s";`, name))
- if res.Error != nil {
- color.Errorf("create Postgres database error: %v", res.Error)
- }
-
- if err := r.close(gormDB); err != nil {
- color.Errorf("close Postgres connection error: %v", err)
- }
- }()
-
- postgresImpl := NewDocker(name, r.username, r.password)
+ gormDB, err := r.connect()
+ if err != nil {
+ return nil, fmt.Errorf("connect Postgres error: %v", err)
+ }
+
+ res := gormDB.Exec(fmt.Sprintf(`CREATE DATABASE "%s";`, name))
+ if res.Error != nil {
+ return nil, fmt.Errorf("create Postgres database error: %v", res.Error)
+ }
+
+ if err := r.close(gormDB); err != nil {
+ return nil, fmt.Errorf("close Postgres connection error: %v", err)
+ }
+
+ postgresImpl := NewDocker(name, r.username, r.password)
postgresImpl.containerID = r.containerID
postgresImpl.port = r.port
return postgresImpl, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (r *Docker) Database(name string) (testing.DatabaseDriver, error) { | |
| go func() { | |
| gormDB, err := r.connect() | |
| if err != nil { | |
| color.Errorf("connect Postgres error: %v", err) | |
| return | |
| } | |
| res := gormDB.Exec(fmt.Sprintf(`CREATE DATABASE "%s";`, name)) | |
| if res.Error != nil { | |
| color.Errorf("create Postgres database error: %v", res.Error) | |
| } | |
| if err := r.close(gormDB); err != nil { | |
| color.Errorf("close Postgres connection error: %v", err) | |
| } | |
| }() | |
| postgresImpl := NewDocker(name, r.username, r.password) | |
| postgresImpl.containerID = r.containerID | |
| postgresImpl.port = r.port | |
| return postgresImpl, nil | |
| } | |
| func (r *Docker) Database(name string) (testing.DatabaseDriver, error) { | |
| gormDB, err := r.connect() | |
| if err != nil { | |
| return nil, fmt.Errorf("connect Postgres error: %v", err) | |
| } | |
| res := gormDB.Exec(fmt.Sprintf(`CREATE DATABASE "%s";`, name)) | |
| if res.Error != nil { | |
| return nil, fmt.Errorf("create Postgres database error: %v", res.Error) | |
| } | |
| if err := r.close(gormDB); err != nil { | |
| return nil, fmt.Errorf("close Postgres connection error: %v", err) | |
| } | |
| postgresImpl := NewDocker(name, r.username, r.password) | |
| postgresImpl.containerID = r.containerID | |
| postgresImpl.port = r.port | |
| return postgresImpl, nil | |
| } |
| func (r *Postgres) Config() database.Config1 { | ||
| writers := r.configBuilder.Writes() | ||
| if len(writers) == 0 { | ||
| return database.Config1{} | ||
| } |
There was a problem hiding this comment.
Correct the return type of the Config method
The return type database.Config1 does not exist and should be corrected to database.Config.
Apply this diff to fix the return type:
-func (r *Postgres) Config() database.Config1 {
+func (r *Postgres) Config() database.Config {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (r *Postgres) Config() database.Config1 { | |
| writers := r.configBuilder.Writes() | |
| if len(writers) == 0 { | |
| return database.Config1{} | |
| } | |
| func (r *Postgres) Config() database.Config { | |
| writers := r.configBuilder.Writes() | |
| if len(writers) == 0 { | |
| return database.Config1{} | |
| } |
| func (r *Postgres) dns(config contracts.FullConfig) string { | ||
| if config.Dsn != "" { | ||
| return config.Dsn | ||
| } | ||
| if config.Host == "" { | ||
| return "" | ||
| } | ||
|
|
||
| return fmt.Sprintf("postgres://%s:%s@%s:%d/%s?sslmode=%s&timezone=%s&search_path=%s", | ||
| config.Username, config.Password, config.Host, config.Port, config.Database, config.Sslmode, config.Timezone, config.Schema) | ||
| } |
There was a problem hiding this comment.
Rename method from dns to dsn
The method name dns is likely a typo and should be dsn, which stands for Data Source Name.
Apply this diff to rename the method and its usages:
-func (r *Postgres) dns(config contracts.FullConfig) string {
+func (r *Postgres) dsn(config contracts.FullConfig) string {
// ...
}
-func (r *Postgres) configsToDialectors(configs []contracts.FullConfig) ([]gorm.Dialector, error) {
// ...
- dsn := r.dns(config)
+ dsn := r.dsn(config)
// ...
}Committable suggestion skipped: line range outside the PR's diff.
| return nil | ||
| } | ||
|
|
||
| return instance.(*postgres.Postgres) |
There was a problem hiding this comment.
Add type assertion safety check
The unsafe type assertion could panic if the instance is not of type *postgres.Postgres.
- return instance.(*postgres.Postgres)
+ driver, ok := instance.(*postgres.Postgres)
+ if !ok {
+ return nil, fmt.Errorf("invalid postgres driver instance type: %T", instance)
+ }
+ return driver, nilCommittable suggestion skipped: line range outside the PR's diff.
| if postgres.App == nil { | ||
| log.Fatalln("please register postgres service provider") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Replace log.Fatalln with error return
Using log.Fatalln in a library is not recommended as it forcefully terminates the application. Instead, return an error to let the caller decide how to handle it.
- if postgres.App == nil {
- log.Fatalln("please register postgres service provider")
- return nil
- }
+ if postgres.App == nil {
+ return nil, fmt.Errorf("postgres service provider not registered")
+ }Committable suggestion skipped: line range outside the PR's diff.
| DriverName = "postgres" | ||
| ) | ||
|
|
||
| var App foundation.Application |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid global state
Using a global App variable makes the code harder to test and reason about. Consider making it a field of ServiceProvider.
-var App foundation.Application
+type ServiceProvider struct {
+ app foundation.Application
+}Committable suggestion skipped: line range outside the PR's diff.
| // Used by TestContainer, to simulate the port is using. | ||
| var testPortUsing = false |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace global test variable
Using a global variable for testing makes the code harder to maintain and test. Consider using dependency injection or interface-based testing.
-var testPortUsing = false
+type portChecker interface {
+ isPortUsing(port int) bool
+}Committable suggestion skipped: line range outside the PR's diff.
| func getValidPort() int { | ||
| for i := 0; i < 60; i++ { | ||
| random := rand.Intn(10000) + 10000 | ||
| l, err := net.Listen("tcp", fmt.Sprintf(":%d", random)) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| defer l.Close() | ||
|
|
||
| return random | ||
| } | ||
|
|
||
| return 0 | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve port allocation reliability
The current implementation has several issues:
- Magic numbers (10000, 60) should be constants
- Limited retries might fail in high-contention scenarios
- No context for timeout/cancellation
+const (
+ minPort = 10000
+ maxPort = 20000
+ maxRetries = 60
+)
+
-func getValidPort() int {
+func getValidPort(ctx context.Context) (int, error) {
- for i := 0; i < 60; i++ {
- random := rand.Intn(10000) + 10000
+ for i := 0; i < maxRetries; i++ {
+ select {
+ case <-ctx.Done():
+ return 0, ctx.Err()
+ default:
+ random := rand.Intn(maxPort-minPort) + minPort
+ l, err := net.Listen("tcp", fmt.Sprintf(":%d", random))
+ if err != nil {
+ continue
+ }
+ l.Close()
+ return random, nil
+ }
+ }
- l, err := net.Listen("tcp", fmt.Sprintf(":%d", random))
- if err != nil {
- continue
- }
- defer l.Close()
-
- return random
- }
-
- return 0
+ return 0, fmt.Errorf("failed to find available port after %d attempts", maxRetries)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func getValidPort() int { | |
| for i := 0; i < 60; i++ { | |
| random := rand.Intn(10000) + 10000 | |
| l, err := net.Listen("tcp", fmt.Sprintf(":%d", random)) | |
| if err != nil { | |
| continue | |
| } | |
| defer l.Close() | |
| return random | |
| } | |
| return 0 | |
| } | |
| const ( | |
| minPort = 10000 | |
| maxPort = 20000 | |
| maxRetries = 60 | |
| ) | |
| func getValidPort(ctx context.Context) (int, error) { | |
| for i := 0; i < maxRetries; i++ { | |
| select { | |
| case <-ctx.Done(): | |
| return 0, ctx.Err() | |
| default: | |
| random := rand.Intn(maxPort-minPort) + minPort | |
| l, err := net.Listen("tcp", fmt.Sprintf(":%d", random)) | |
| if err != nil { | |
| continue | |
| } | |
| l.Close() | |
| return random, nil | |
| } | |
| } | |
| return 0, fmt.Errorf("failed to find available port after %d attempts", maxRetries) | |
| } |
| func run(command string) (string, error) { | ||
| cmd := exec.Command("/bin/sh", "-c", command) | ||
|
|
||
| var out bytes.Buffer | ||
| var stderr bytes.Buffer | ||
|
|
||
| cmd.Stdout = &out | ||
| cmd.Stderr = &stderr | ||
|
|
||
| if err := cmd.Run(); err != nil { | ||
| return "", fmt.Errorf("%s: %s", err, stderr.String()) | ||
| } | ||
|
|
||
| return str.Of(out.String()).Squish().String(), nil | ||
| } |
There was a problem hiding this comment.
Secure command execution
The current implementation might be vulnerable to command injection. Consider:
- Validating command input
- Using proper argument escaping
- Adding timeout context
-func run(command string) (string, error) {
+func run(ctx context.Context, command string) (string, error) {
+ if err := validateCommand(command); err != nil {
+ return "", fmt.Errorf("invalid command: %w", err)
+ }
+
cmd := exec.Command("/bin/sh", "-c", command)
+ cmd.Cancel = ctx.Done()
var out bytes.Buffer
var stderr bytes.Buffer
cmd.Stdout = &out
cmd.Stderr = &stderr
if err := cmd.Run(); err != nil {
+ if ctx.Err() != nil {
+ return "", ctx.Err()
+ }
return "", fmt.Errorf("%s: %s", err, stderr.String())
}
return str.Of(out.String()).Squish().String(), nil
}Committable suggestion skipped: line range outside the PR's diff.
| func (c *ConfigBuilder) Reads() []contracts.FullConfig { | ||
| configs := c.config.Get(fmt.Sprintf("database.connections.%s.read", c.connection)) | ||
| if readConfigs, ok := configs.([]contracts.Config); ok { | ||
| return c.fillDefault(readConfigs) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for type assertions.
The type assertion could panic if the configuration value is of an unexpected type.
func (c *ConfigBuilder) Reads() []contracts.FullConfig {
configs := c.config.Get(fmt.Sprintf("database.connections.%s.read", c.connection))
- if readConfigs, ok := configs.([]contracts.Config); ok {
+ readConfigs, ok := configs.([]contracts.Config)
+ if configs != nil && !ok {
+ // Log or handle the type mismatch
+ return nil
+ }
+ if ok {
return c.fillDefault(readConfigs)
}
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (c *ConfigBuilder) Reads() []contracts.FullConfig { | |
| configs := c.config.Get(fmt.Sprintf("database.connections.%s.read", c.connection)) | |
| if readConfigs, ok := configs.([]contracts.Config); ok { | |
| return c.fillDefault(readConfigs) | |
| } | |
| return nil | |
| } | |
| func (c *ConfigBuilder) Reads() []contracts.FullConfig { | |
| configs := c.config.Get(fmt.Sprintf("database.connections.%s.read", c.connection)) | |
| readConfigs, ok := configs.([]contracts.Config) | |
| if configs != nil && !ok { | |
| // Log or handle the type mismatch | |
| return nil | |
| } | |
| if ok { | |
| return c.fillDefault(readConfigs) | |
| } | |
| return nil | |
| } |
1 participant
📑 Description
Closes goravel/goravel#540
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
✅ Checks