feat: [#358] Add facades.DB, to provider an original sql flow #869
Conversation
WalkthroughA new constant was introduced in the contracts package, and the database configuration was extended with additional fields and an interface for placeholder replacement. New interfaces for database operations (DB and Query) and a corresponding struct with query-building functions were added. The Driver interface now includes a method to retrieve a SQL database connection. Across the codebase, error constants and messages have been renamed for clarity. In addition, new mocks for various database components were generated, updated tests and test suites were added, and the Go module files were updated with additional dependencies. Changes
Suggested reviewers
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)Error: can't load config: the Go language version (go1.23) used to build golangci-lint is lower than the targeted Go version (1.24.0) 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 300000ms (1)
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #869 +/- ##
=======================================
Coverage 66.47% 66.47%
=======================================
Files 151 151
Lines 10387 10387
=======================================
Hits 6905 6905
Misses 3124 3124
Partials 358 358 ☔ View full report in Codecov by Sentry. |
| for driver, query := range s.queries { | ||
| s.Run(driver, func() { | ||
| var user []User | ||
| err := query.DB().Table("users").Where("name = ?", "count_user").Get(&user) |
There was a problem hiding this comment.
Will add more test cases in other PRs.
| CreatedAt carbon.DateTime `gorm:"autoCreateTime;column:created_at" json:"created_at"` | ||
| UpdatedAt carbon.DateTime `gorm:"autoUpdateTime;column:updated_at" json:"updated_at"` | ||
| CreatedAt carbon.DateTime `gorm:"autoCreateTime;column:created_at" json:"created_at" db:"created_at"` | ||
| UpdatedAt carbon.DateTime `gorm:"autoUpdateTime;column:updated_at" json:"updated_at" db:"updated_at"` |
There was a problem hiding this comment.
The DB facade needs to special the tag.
| Schema string | ||
| Username string | ||
| Version string | ||
| PlaceholderFormat PlaceholderFormat |
There was a problem hiding this comment.
Different drivers have different placeholders.
|
PTAL @goravel/core-developers |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 3c21735 | Previous: a113be2 | Ratio |
|---|---|---|---|
Benchmark_Fatal |
0.0000132 ns/op 0 B/op 0 allocs/op |
2e-7 ns/op 0 B/op 0 allocs/op |
66 |
Benchmark_Fatal - ns/op |
0.0000132 ns/op |
2e-7 ns/op |
66 |
This comment was automatically generated by workflow using github-action-benchmark.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (16)
database/db/conditions.go (1)
3-6: Add documentation for the Conditions struct.Please add documentation to explain the purpose and usage of the
Conditionsstruct, as it's an exported type.+// Conditions represents a set of database query conditions for a specific table. type Conditions struct { table string where []Where }contracts/database/db/db.go (2)
3-5: Add documentation for the DB interface.Please add documentation to explain the purpose and usage of the
DBinterface.+// DB represents a database connection that provides access to tables. type DB interface { + // Table returns a Query instance for the specified table. Table(name string) Query }
7-10: Add documentation for the Query interface.Please add documentation to explain the purpose and usage of the
Queryinterface and its methods.+// Query represents a database query builder for constructing and executing queries. type Query interface { + // Where adds a WHERE condition to the query. + // query: The condition expression (e.g., "column = ?", "column IN (?)") + // args: The arguments for the condition Where(query any, args ...any) Query + // Get executes the query and scans the result into dest. + // dest: A pointer to the destination where the result will be stored Get(dest any) error }contracts/database/config.go (2)
3-16: Add documentation for the Config struct.Please add documentation to explain the purpose and usage of the
Configstruct and its fields.+// Config represents database configuration settings. type Config struct { Connection string - DNS string + DSN string // Data Source Name for database connection Database string Driver string Host string Password string Port int Prefix string Schema string Username string Version string PlaceholderFormat PlaceholderFormat }
18-20: Add documentation for the PlaceholderFormat interface.Please add documentation to explain the purpose and usage of the
PlaceholderFormatinterface.+// PlaceholderFormat defines methods for handling SQL placeholder formatting. +// Different database drivers may use different placeholder formats (e.g., ? for MySQL, $1 for PostgreSQL). type PlaceholderFormat interface { + // ReplacePlaceholders replaces the placeholders in the SQL query with the appropriate format for the current driver. ReplacePlaceholders(sql string) (string, error) }contracts/database/driver/driver.go (1)
16-16: Add documentation for the DB method.Please add documentation to explain the purpose and usage of the
DBmethod.+ // DB returns the underlying *sql.DB instance for raw SQL operations. DB() (*sql.DB, error)contracts/binding.go (1)
24-24: LGTM with a minor suggestion.The new
BindingDBconstant follows the established naming pattern and aligns with the PR objectives.Consider maintaining alphabetical order by moving
BindingDBbetweenBindingCryptandBindingEvent:- BindingDB = "goravel.db" + BindingDB = "goravel.db"database/db/db.go (1)
24-41: Consider wrapping errors for better context.The error handling is good, but consider wrapping errors with additional context to help with debugging.
driverCallback, exist := config.Get(fmt.Sprintf("database.connections.%s.via", connection)).(func() (contractsdriver.Driver, error)) if !exist { - return nil, errors.DatabaseConfigNotFound + return nil, fmt.Errorf("database config not found for connection %s: %w", connection, errors.DatabaseConfigNotFound) } driver, err := driverCallback() if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get driver for connection %s: %w", connection, err) } instance, err := driver.DB() if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get DB instance for connection %s: %w", connection, err) }database/db/query.go (2)
14-28: Add input parameter validation in NewQuery constructor.The constructor should validate that the required dependencies (config and instance) are not nil to prevent potential nil pointer dereferences.
func NewQuery(config database.Config, instance *sqlx.DB, table string) *Query { + if instance == nil { + panic("database instance is required") + } return &Query{ conditions: Conditions{ table: table, }, config: config, instance: instance, } }
30-37: Add input validation in Where method.The method should validate that the query parameter is not nil to prevent potential runtime errors.
func (r *Query) Where(query any, args ...any) db.Query { + if query == nil { + panic("query condition cannot be nil") + } r.conditions.where = append(r.conditions.where, Where{ query: query, args: args, }) return r }mocks/database/db/Query.go (2)
70-90: Consider using more specific types for query parameters.The
Wheremethod usesinterface{}for both query and args parameters. Consider using more specific types (e.g., string for query, specific types for args) to prevent potential runtime type errors.-func (_m *Query) Where(query interface{}, args ...interface{}) db.Query { +func (_m *Query) Where(query string, args ...any) db.Query {
24-39: Consider using a more specific type for the destination parameter.The
Getmethod usesinterface{}for the dest parameter. Consider using a more specific type or adding runtime type checks to prevent potential type assertion errors.-func (_m *Query) Get(dest interface{}) error { +func (_m *Query) Get(dest any) error {database/service_provider.go (2)
53-65: Consider wrapping errors with context.The error handling could be improved by wrapping errors from
BuildDBwith additional context about the connection being used.- return db.BuildDB(config, connection) + db, err := db.BuildDB(config, connection) + if err != nil { + return nil, fmt.Errorf("failed to build DB for connection %s: %w", connection, err) + } + return db, nil
86-86: Ensure consistent error module naming.The error
DatabaseConfigNotFoundshould include the module name for consistency with other errors in the file.- return nil, errors.DatabaseConfigNotFound + return nil, errors.DatabaseConfigNotFound.SetModule(errors.ModuleDB)database/console/seed_command_test.go (1)
35-102: Consider adding more edge cases to the test suite.The test coverage could be improved by adding the following test cases:
- Empty seeder slice ([]string{})
- Nil config
- Invalid seeder name format
Example test case:
{ name: "Empty seeder slice", setup: func() { s.mockContext.EXPECT().OptionBool("force").Return(false).Once() s.mockConfig.EXPECT().GetString("app.env").Return("development").Once() s.mockContext.EXPECT().OptionSlice("seeder").Return([]string{}).Once() s.mockContext.EXPECT().Success("no seeders found").Once() }, },tests/query.go (1)
39-54: Consider combining error checks for better readability.The sequential error checks for
driver.Gorm()anddriver.DB()could be combined for better readability.Consider this refactoring:
- query, gormQuery, err := driver.Gorm() - if err != nil { - return nil, err - } - - db, err := driver.DB() - if err != nil { - return nil, err - } + query, gormQuery, err := driver.Gorm() + if err != nil { + return nil, fmt.Errorf("failed to get gorm: %w", err) + } + + db, err := driver.DB() + if err != nil { + return nil, fmt.Errorf("failed to get db: %w", err) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
contracts/binding.go(1 hunks)contracts/database/config.go(1 hunks)contracts/database/db/db.go(1 hunks)contracts/database/driver/driver.go(2 hunks)database/console/seed_command.go(3 hunks)database/console/seed_command_test.go(4 hunks)database/db/conditions.go(1 hunks)database/db/db.go(1 hunks)database/db/query.go(1 hunks)database/gorm/query.go(1 hunks)database/service_provider.go(3 hunks)errors/list.go(1 hunks)errors/modules.go(1 hunks)go.mod(3 hunks)mocks/database/PlaceholderFormat.go(1 hunks)mocks/database/db/DB.go(1 hunks)mocks/database/db/Query.go(1 hunks)mocks/database/driver/Driver.go(2 hunks)testing/docker/database.go(1 hunks)testing/docker/database_test.go(1 hunks)tests/db_test.go(1 hunks)tests/go.mod(3 hunks)tests/models.go(1 hunks)tests/query.go(3 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
database/db/conditions.go
11-11: field or is unused
(unused)
🪛 GitHub Check: lint / lint
database/db/conditions.go
[failure] 11-11:
field or is unused (unused)
🪛 GitHub Actions: Lint
database/db/conditions.go
[warning] 11-11: field or is unused
🔇 Additional comments (19)
database/db/conditions.go (1)
8-12: Document the Where struct and verify the unused 'or' field.The
orfield is currently unused according to static analysis. If this field is intended for supporting OR conditions in WHERE clauses, please implement the functionality or remove the field.Additionally, please add documentation to explain the purpose and usage of the
Wherestruct.+// Where represents a single condition in a database query. +// query: The condition expression (e.g., "column = ?", "column IN (?)") +// args: The arguments for the condition +// or: Indicates if this condition should be combined with OR instead of AND type Where struct { query any args []any or bool }🧰 Tools
🪛 golangci-lint (1.62.2)
11-11: field
oris unused(unused)
🪛 GitHub Check: lint / lint
[failure] 11-11:
fieldoris unused (unused)🪛 GitHub Actions: Lint
[warning] 11-11: field
oris unusederrors/modules.go (1)
8-8: LGTM!The new
ModuleDBconstant follows the established naming pattern and aligns with the PR objectives to add facades.DB.tests/db_test.go (2)
15-20: LGTM!Good practice to run tests in parallel using
t.Parallel()for better test performance.
37-45: Add more test cases for comprehensive coverage.The current test suite only includes a basic test for the
Whereclause. Consider adding test cases for:
- Different query conditions (AND, OR, etc.)
- Various data types in WHERE clauses
- Edge cases and error scenarios
Based on the past review comment, you've acknowledged that more test cases will be added in other PRs.
database/db/db.go (1)
15-18: LGTM!Good encapsulation of the database configuration and instance with unexported fields.
database/db/query.go (1)
50-67: Add destination type validation in buildSelect.The method should validate that the destination is a pointer to a slice or struct to prevent runtime errors.
❌ Incorrect review comment
🏁 Script executed:
#!/bin/bash # Check if similar validation exists in other query builders ast-grep --pattern 'func $_(dest any) error { $$$ if reflect.TypeOf(dest).Kind() != reflect.Ptr { $$$ } $$$ }'Length of output: 124
Clarification on Destination Validation in buildSelect
The
buildSelectmethod only constructs the SQL query and arguments without accepting any destination parameter. Since it doesn’t interact with any destination (pointer to a slice or struct) for scanning results, adding destination type validation here isn’t applicable. If destination type validation is required, it should be implemented in the functions that handle scanning of query results.Likely an incorrect or invalid review comment.
mocks/database/db/DB.go (1)
1-84: LGTM! Well-structured mock implementation.The mock implementation is clean, follows best practices, and includes proper cleanup handling.
testing/docker/database.go (1)
41-41: LGTM! Error naming standardization.The error type change aligns with the standardized naming convention across the codebase.
database/console/seed_command.go (1)
72-72: LGTM! Consistent error naming updates.The error type changes maintain consistency with the new naming convention across the codebase.
Also applies to: 86-86, 98-98
mocks/database/PlaceholderFormat.go (1)
1-89: LGTM! Well-structured mock implementation.The mock implementation follows best practices with proper error handling, type-safe expectations, and test cleanup registration.
testing/docker/database_test.go (1)
106-106: LGTM! Error constant renamed for consistency.The change from
OrmDatabaseConfigNotFoundtoDatabaseConfigNotFoundaligns with the standardization of error naming conventions across the codebase.errors/list.go (1)
48-52: LGTM! Error constants reorganized for better clarity.The changes improve consistency by:
- Using full words instead of abbreviations (e.g., "Database" instead of "DB")
- Grouping related database errors together
- Using more descriptive and standardized error messages
mocks/database/driver/Driver.go (2)
78-106: LGTM! New DB() method properly implemented in mock.The implementation follows the established mock patterns and includes proper error handling and type assertions.
108-133: LGTM! Helper methods for DB() mock properly implemented.The implementation includes all necessary helper methods (Run, Return, RunAndReturn) following the established patterns.
tests/query.go (1)
33-33: LGTM! Clean addition of DB field and getter.The new DB field and its getter method provide clean access to raw SQL operations while maintaining encapsulation.
Also applies to: 79-81
tests/models.go (1)
24-26: LGTM! Thedbstruct tags are correctly added.The addition of
dbstruct tags toDeletedAt,CreatedAt, andUpdatedAtfields is correct and aligns with the new facades.DB functionality for SQL column mapping.Also applies to: 28-31
database/gorm/query.go (1)
69-69: LGTM! Error type renamed for consistency.The error type has been updated from
errors.OrmDatabaseConfigNotFoundtoerrors.DatabaseConfigNotFound, which aligns with the standardized error naming convention across the codebase.tests/go.mod (1)
25-25: Verify the compatibility and security of new dependencies.Several new dependencies have been added to support the facades.DB functionality:
squirrelfor SQL query buildingsqlxfor enhanced SQL operationslib/pqfor PostgreSQL support- OpenTelemetry packages for metrics and tracing
Run the following script to check for any known vulnerabilities or compatibility issues:
Also applies to: 47-50, 65-66, 68-68
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for vulnerabilities in new dependencies # Check for known vulnerabilities gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: GO, package: "github.com/Masterminds/squirrel") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } } ' # Check for latest versions go list -m -versions github.com/Masterminds/squirrel go list -m -versions github.com/jmoiron/sqlx go list -m -versions github.com/lib/pq go list -m -versions go.opentelemetry.io/otel/metric go list -m -versions go.opentelemetry.io/otel/traceLength of output: 1949
Dependency Security & Compatibility: Verified
- The GitHub vulnerability query for
github.com/Masterminds/squirrelreturned no known issues.- The version check shows that the pinned version (
v1.5.4) is among a series of valid releases.- The other new dependencies (
sqlx,lib/pq, and the OpenTelemetry packages) display healthy version histories.Based on these checks, the new dependencies appear secure and compatible.
go.mod (1)
8-8: Verify the compatibility and security of new core dependencies.Core dependencies have been added to support the facades.DB functionality:
squirrelfor SQL query buildingsqlxfor SQL operationsRun the following script to check for any known vulnerabilities or compatibility issues:
Also applies to: 22-22, 66-67
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for vulnerabilities in new core dependencies # Check for known vulnerabilities gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: GO, package: "github.com/jmoiron/sqlx") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } } ' # Check for latest versions and compatibility go list -m -versions github.com/Masterminds/squirrel go list -m -versions github.com/jmoiron/sqlx # Check if these versions are compatible with the Go version go mod verifyLength of output: 742
Dependency Compatibility and Security Verification
- The vulnerability check for
github.com/jmoiron/sqlxreturned no advisories.- The version check shows that
github.com/Masterminds/squirrelis used at v1.5.4, which is within the available versions.- The
go mod verifystep failed because it was executed outside the repository root—running it from the correct directory should validate module integrity.Overall, the new core dependencies appear secure and compatible.
|
@almas1992 Thanks. FYI, you can approve this if it looks good to you. |
📑 Description
goravel/goravel#358
Summary by CodeRabbit
New Features
DBinterface andQueryinterface for flexible database interactions.BindingDBfor improved binding options.PlaceholderFormatto facilitate testing.Driverinterface for direct SQL database access.Improvements
SoftDeletesandTimestampsto improve ORM interactions.Tests
✅ Checks