fix: Throw error if Context is http Context #866
Conversation
WalkthroughThis pull request introduces multiple modifications across the codebase. In auth tests, a fixed timestamp variable is used for JWT token expiration and issuance. The console application initialization now accepts a direct boolean flag for artisan mode. The Blueprint and Grammar interfaces, along with their implementations and tests, have been extended to support boolean and custom type columns. Additionally, ORM context handling is updated to unwrap HTTP contexts, logger behavior and GRPC logging messages have been adjusted, mock functions for schema methods have been added, support utilities have been modified, and several dependencies were updated. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant O as Orm.WithContext
participant H as HTTPContext
C->>O: Call WithContext(ctx)
O->>O: Check if ctx implements contractshttp.Context
alt ctx implements contractshttp.Context
O->>H: Retrieve underlying HTTP context via http.Context()
end
O->>C: Return new Orm instance with updated context
Possibly related PRs
Suggested reviewers
✨ 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: 1
🧹 Nitpick comments (8)
contracts/database/schema/blueprint.go (1)
18-19: Consider enhancing the Column method documentation.While the method is a good addition, consider adding examples of valid type strings in the documentation comment.
- // Column Create a new custom type column on the table. + // Column Create a new custom type column on the table. + // Example types: "jsonb", "uuid", "inet", etc.database/schema/blueprint_test.go (1)
118-125: Consider adding more test cases for the Boolean method.While the basic test case is good, consider adding tests for:
- Nullable boolean columns
- Boolean columns with default values
- Boolean columns with comments
func (s *BlueprintTestSuite) TestBoolean() { name := "name" s.blueprint.Boolean(name) s.Contains(s.blueprint.GetAddedColumns(), &ColumnDefinition{ name: &name, ttype: convert.Pointer("boolean"), }) + + // Test nullable boolean + s.blueprint.Boolean(name).Nullable() + s.Contains(s.blueprint.GetAddedColumns(), &ColumnDefinition{ + name: &name, + ttype: convert.Pointer("boolean"), + nullable: convert.Pointer(true), + }) + + // Test boolean with default + s.blueprint.Boolean(name).Default(true) + s.Contains(s.blueprint.GetAddedColumns(), &ColumnDefinition{ + name: &name, + ttype: convert.Pointer("boolean"), + default: convert.Pointer(true), + }) + + // Test boolean with comment + s.blueprint.Boolean(name).Comment("Is active") + s.Contains(s.blueprint.GetAddedColumns(), &ColumnDefinition{ + name: &name, + ttype: convert.Pointer("boolean"), + comment: convert.Pointer("Is active"), + }) }database/schema/grammars/mysql_test.go (1)
355-359: Consider adding more test cases.While the basic test is good, consider adding test cases for:
- Nullable boolean columns
- Boolean columns with default values
- Boolean columns with constraints
func (s *MysqlSuite) TestTypeBoolean() { mockColumn := mocksschema.NewColumnDefinition(s.T()) + // Test basic boolean s.Equal("tinyint(1)", s.grammar.TypeBoolean(mockColumn)) + + // Test nullable boolean + mockColumn.EXPECT().GetNullable().Return(true).Once() + s.Equal("tinyint(1)", s.grammar.TypeBoolean(mockColumn)) + + // Test boolean with default + mockColumn.EXPECT().GetDefault().Return(true).Once() + s.Equal("tinyint(1)", s.grammar.TypeBoolean(mockColumn)) }database/schema/grammars/postgres_test.go (1)
412-416: Consider enhancing test coverage.While the test verifies the basic functionality, consider adding assertions to verify that the column parameter is correctly passed to the mock, maintaining consistency with other type tests.
func (s *PostgresSuite) TestTypeBoolean() { mockColumn := mocksschema.NewColumnDefinition(s.T()) + mockColumn.EXPECT().GetType().Return("boolean").Once() s.Equal("boolean", s.grammar.TypeBoolean(mockColumn)) }database/schema/schema_test.go (1)
2397-2401: Consider using a map for database-specific type mappings.The current if-else logic for custom type mapping could be refactored to use a map for better maintainability, especially if more database-specific types are added in the future.
- if schema.GetConnection() != database.DriverPostgres.String() { - table.Column("custom_type", "geometry").Comment("This is a custom type column") - } else { - table.Column("custom_type", "macaddr").Comment("This is a custom type column") - } + customTypes := map[string]string{ + database.DriverPostgres.String(): "macaddr", + database.DriverMysql.String(): "geometry", + database.DriverSqlite.String(): "geometry", + database.DriverSqlserver.String(): "geometry", + } + table.Column("custom_type", customTypes[schema.GetConnection()]).Comment("This is a custom type column")mocks/database/schema/Blueprint.go (1)
1-3555: Consider splitting the file into smaller modules.While the implementation is correct, the file is quite large (3500+ lines). Consider:
- Splitting it into smaller files by grouping related methods (e.g., numeric types, text types, etc.)
- Adding package-level documentation to explain the purpose and usage of these mocks
console/service_provider.go (1)
20-20: Consider making the artisan mode configurable.The hardcoded
truevalue makes the code less maintainable. Consider extracting this into a configuration value or constant with a descriptive name to improve code clarity.- return NewApplication(name, usage, usageText, app.Version(), true), nil + const useArtisan = true // Enable artisan CLI mode + return NewApplication(name, usage, usageText, app.Version(), useArtisan), nilconsole/application.go (1)
84-118: Consider simplifying the artisanIndex logic.The current implementation can be simplified by moving the artisanIndex assignment inside the outer if block since it's only used there.
Consider this refactoring:
func (r *Application) Run(args []string, exitIfArtisan bool) error { - artisanIndex := -1 if r.useArtisan { + artisanIndex := -1 for i, arg := range args { if arg == "artisan" { artisanIndex = i break } } - } else { - artisanIndex = 0 - } - if artisanIndex != -1 { + if artisanIndex != -1 { // Add --help if no command argument is provided. if artisanIndex+1 == len(args) { args = append(args, "--help") } cliArgs := append([]string{args[0]}, args[artisanIndex+1:]...) if err := r.instance.Run(cliArgs); err != nil { if exitIfArtisan { panic(err.Error()) } return err } if exitIfArtisan { os.Exit(0) } + } + } else { + cliArgs := args + if err := r.instance.Run(cliArgs); err != nil { + if exitIfArtisan { + panic(err.Error()) + } + return err + } + if exitIfArtisan { + os.Exit(0) + } } return nil }
📜 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 (28)
auth/auth_test.go(2 hunks)console/application.go(5 hunks)console/application_test.go(1 hunks)console/cli_helper_test.go(1 hunks)console/service_provider.go(1 hunks)contracts/database/schema/blueprint.go(1 hunks)contracts/database/schema/grammar.go(1 hunks)database/gorm/logger.go(0 hunks)database/orm/orm.go(2 hunks)database/schema/blueprint.go(2 hunks)database/schema/blueprint_test.go(1 hunks)database/schema/grammars/mysql.go(1 hunks)database/schema/grammars/mysql_test.go(1 hunks)database/schema/grammars/postgres.go(1 hunks)database/schema/grammars/postgres_test.go(1 hunks)database/schema/grammars/sqlite.go(1 hunks)database/schema/grammars/sqlite_test.go(1 hunks)database/schema/grammars/sqlserver.go(1 hunks)database/schema/grammars/sqlserver_test.go(1 hunks)database/schema/grammars/utils.go(1 hunks)database/schema/grammars/utils_test.go(1 hunks)database/schema/schema_test.go(11 hunks)go.mod(4 hunks)grpc/application.go(1 hunks)mocks/database/schema/Blueprint.go(2 hunks)mocks/database/schema/Grammar.go(1 hunks)support/collect/collect.go(2 hunks)support/constant.go(1 hunks)
💤 Files with no reviewable changes (1)
- database/gorm/logger.go
✅ Files skipped from review due to trivial changes (1)
- support/constant.go
🔇 Additional comments (31)
database/schema/grammars/utils.go (1)
62-62: LGTM! Improved type handling fallback.The change to return
column.GetType()instead of an empty string is a better approach, as it allows fallback to the original type when a specific type handler method is not found.database/schema/grammars/utils_test.go (1)
58-62: LGTM! Test updated to match new behavior.The test changes correctly reflect the updated behavior of
getTypefunction, now expecting the original type to be returned when no specific type handler is found.contracts/database/schema/grammar.go (1)
64-65: LGTM! Well-documented boolean type support.The addition of
TypeBooleanmethod follows the consistent pattern of the interface and is properly documented.contracts/database/schema/blueprint.go (1)
12-13: LGTM! The Boolean method is a good addition.The method follows the established pattern and is well-documented.
database/schema/grammars/sqlite_test.go (1)
361-365: LGTM! The test case is well-structured.The test verifies the correct SQLite type mapping for boolean columns.
database/schema/grammars/sqlserver_test.go (1)
288-292: LGTM! The test case is well-structured.The test verifies the correct SQL Server type mapping for boolean columns.
database/schema/grammars/sqlite.go (1)
282-284: LGTM! Correct implementation of boolean type for SQLite.The implementation uses the standard SQLite convention of
tinyint(1)for boolean values, which is the correct approach since SQLite doesn't have a native boolean type.database/schema/grammars/mysql.go (1)
320-322: LGTM! Correct implementation of boolean type for MySQL.The implementation uses the standard MySQL convention of
tinyint(1)for boolean values, which is the correct approach.database/schema/grammars/sqlserver.go (1)
314-316: LGTM! Correct implementation of boolean type for SQL Server.The implementation uses SQL Server's native
bittype for boolean values, which is the correct approach.database/schema/grammars/postgres.go (1)
325-327: LGTM! Clean and correct implementation.The implementation correctly returns the PostgreSQL boolean type and follows the established pattern of other type methods.
database/schema/blueprint.go (2)
37-39: LGTM! Clean implementation of boolean column support.The Boolean method follows the established pattern and correctly delegates to createAndAddColumn.
63-65: LGTM! Good addition of generic column creation.The Column method provides flexibility for custom types while maintaining consistency with the existing pattern.
mocks/database/schema/Grammar.go (1)
1426-1470: LGTM! Correctly generated mock implementation.The mock implementation follows the standard mockery pattern and provides all necessary functionality for testing.
database/schema/schema_test.go (2)
82-82: LGTM: Boolean column type implementation looks correct.The change from Integer to Boolean type properly tests the boolean column creation with a default value.
195-203: LGTM: Comprehensive boolean type testing across databases.The test cases properly verify boolean column behavior for each supported database:
- Postgres: native boolean type
- SQLite: tinyint(1)
- MySQL: tinyint(1)
- SQLServer: bit
The assertions cover all important aspects:
- Autoincrement flag
- Collation settings
- Comment preservation
- Default value handling
- Nullability
- Type mapping
Also applies to: 533-540, 833-841, 1164-1172
mocks/database/schema/Blueprint.go (3)
1-14: LGTM! Clean imports and package declaration.The file header and imports are well-organized and include all necessary dependencies.
120-166: LGTM! Well-implemented Boolean method.The Boolean method implementation follows the established pattern and includes proper error handling and type assertions.
278-325: LGTM! Well-implemented Column method.The Column method implementation follows the established pattern and includes proper error handling and type assertions.
support/collect/collect.go (2)
66-69: Document mutable behavior change.Similar to
Reverse, this function now mutates the input slice directly. This change in behavior should be clearly documented.Please ensure that:
- The function's documentation clearly states it modifies the input slice.
- All callers are aware of this behavior change.
-func Shuffle[T any](collection []T) []T { +// Shuffle modifies the input slice in-place, randomly shuffling its elements. +// Warning: This function mutates the input slice directly. +func Shuffle[T any](collection []T) []T {
60-63: Document mutable behavior change.The function now mutates the input slice directly instead of returning a new copy. This change in behavior should be clearly documented to prevent unexpected side effects.
Please ensure that:
- The function's documentation clearly states it modifies the input slice.
- All callers are aware of this behavior change.
-func Reverse[T any](collection []T) []T { +// Reverse modifies the input slice in-place, reversing its elements. +// Warning: This function mutates the input slice directly. +func Reverse[T any](collection []T) []T {grpc/application.go (1)
83-83: LGTM!The updated logging format is more consistent and clearer.
console/cli_helper_test.go (1)
16-16: LGTM!The test has been correctly updated to match the new
NewApplicationfunction signature.console/application.go (4)
13-16: LGTM! Field rename improves clarity.The rename from
isArtisantouseArtisanbetter describes the field's purpose and follows Go naming conventions.
18-33: LGTM! Function signature change improves API clarity.Making
useArtisanan explicit parameter rather than deriving it from environment improves the API's clarity and testability.
61-63: LGTM! Field usage updated correctly.The method correctly uses the renamed field with no functional changes.
76-78: LGTM! Field usage updated correctly.The method correctly uses the renamed field with no functional changes.
console/application_test.go (1)
16-16: LGTM! Test updated to match new function signature.The test correctly uses the new
useArtisanparameter.auth/auth_test.go (2)
279-285: LGTM! Test reliability improved.Using a consistent timestamp for both expiration and issuance time calculations prevents potential flaky tests.
300-306: LGTM! Test reliability improved.Using a consistent timestamp for both expiration and issuance time calculations prevents potential flaky tests.
go.mod (2)
11-11: Verify stability of pre-release dependency.The
github.com/charmbracelet/huh/spinnerpackage is using a development version. Consider using a stable release if available.✅ Verification successful
🌐 Web query:
What is the latest stable version of github.com/charmbracelet/huh/spinner?💡 Result:
The latest stable version of `github.com/charmbracelet/huh/spinner` is **v0.6.0**, as indicated by its inclusion in the most recent Huh release on GitHub[1]. This version introduced automatic window focus event support for input components and requires explicit focus reporting when used within Bubble Tea programs[1]. Key details about this release: - Focus management improvements for `Input` and `Text` components - Requires `tea.WithReportFocus()` when integrating with Bubble Tea - Released through Huh's standard versioning system - Marked as stable in GitHub's release documentation[1] The spinner component itself remains part of Huh's core functionality across recent versions, with no separate versioning from the main package[3][9]. For integration examples using this spinner with context handling, see Huh's official documentation[9]. Citations: - 1: https://github.com/charmbracelet/huh/releases - 2: https://pkg.go.dev/github.com/charmbracelet/bubbletea - 3: https://github.com/charmbracelet/huh - 4: https://github.com/charmbracelet/huh/actions/runs/9265279334/job/25486993787 - 5: https://pkg.go.dev/github.com/willie-engelbrecht/huh - 6: https://github.com/charmbracelet/huh/runs/25558612530?check_suite_focus=true - 7: https://charm.sh - 8: https://gitnet.fr/deblan-mirror/gum/commits/commit/c11af42c1b438bac62c7ece0271f450770da6e07?page=3 - 9: https://pkg.go.dev/github.com/charmbracelet/huh/spinner/examples/context - 10: https://gitlab.com/gitlab-com/gl-infra/pmv/-/blob/renovate/github.com-charmbracelet-huh-spinner-digest/.golangci.yaml
Update dependency to stable v0.6.0
- The go.mod currently uses a pseudo-version (v0.0.0-20250109160224-6c6b31916f8e), which indicates a pre-release commit.
- According to the latest GitHub releases, the stable version of github.com/charmbracelet/huh/spinner is v0.6.0.
- Consider updating the go.mod dependency to v0.6.0 for better stability.
3-5: Verify Go toolchain version compatibility.The specified Go version and toolchain are very recent. Ensure these versions are stable and compatible with your deployment environment.
✅ Verification successful
🌐 Web query:
What is the latest stable version of Go?💡 Result:
As of February 2025, the latest stable version of Go (Golang) is **1.23**, with its most recent patch release being **1.23.5** (released January 16, 2025)[2][5]. This follows Go's release policy where each major version remains supported until two newer major versions are released[2][5]. Key details: - **Version support**: 1.x releases receive security and critical bug fixes until two subsequent major versions exist[2][5]. 1.22 (latest patch: **1.22.11**) and 1.23 remain actively supported[5]. - **Recent updates in Go tooling**: The `gopls` language server now requires Go toolchain versions from the last two major releases (minimum Go 1.22 for compatibility)[3]. - **Upcoming development**: An unstable **Go 1.24rc2** release candidate exists but isn't recommended for production use[6]. Developers should upgrade to **Go 1.23.x** for security patches and compiler/runtime improvements like enhanced memory management and package vulnerability scanning[2][7]. Citations: - 1: https://www.easiio.com/latest-golang-version/ - 2: https://go.dev/doc/devel/release - 3: https://github.com/golang/tools/releases - 4: https://tip.golang.org/doc/go1.21 - 5: https://endoflife.date/go - 6: https://go.dev/dl/ - 7: https://tip.golang.org/doc/go1.23 - 8: https://groups.google.com/g/golang-nuts/c/_ROx_UGCCCg
Review Findings: Go toolchain version compatibility confirmed.
- The go.mod file specifies Go 1.22.7, which is part of the 1.22 series that remains supported.
- The toolchain is set to go1.23.4—only one patch behind the latest stable 1.23.5 as of February 2025.
- Both versions are stable under Go’s release policy; they are acceptable for deployment, though consider updating to the newest patch releases if you require the latest security fixes.
| func (r *Orm) WithContext(ctx context.Context) contractsorm.Orm { | ||
| if http, ok := ctx.(contractshttp.Context); ok { | ||
| ctx = http.Context() | ||
| } |
There was a problem hiding this comment.
LGTM! Critical fix for HTTP context handling.
This change addresses the PR objective by safely unwrapping the HTTP context before using it in the ORM. This prevents the nil pointer dereference that was occurring during stress testing.
![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: 0479545 | Previous: 998e34f | Ratio |
|---|---|---|---|
Benchmark_DecryptString |
5988 ns/op 1592 B/op 17 allocs/op |
2125 ns/op 1592 B/op 17 allocs/op |
2.82 |
Benchmark_DecryptString - ns/op |
5988 ns/op |
2125 ns/op |
2.82 |
This comment was automatically generated by workflow using github-action-benchmark.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v1.15.x #866 +/- ##
==========================================
Coverage ? 69.79%
==========================================
Files ? 218
Lines ? 18890
Branches ? 0
==========================================
Hits ? 13184
Misses ? 4991
Partials ? 715 ☔ View full report in Codecov by Sentry. |
📑 Description
https://github.com/goravel/fiberrun stress test like wrk
wrk -t12 -c400 -d40s http://127.0.0.1:3000/usersSummary by CodeRabbit
New Features
Chores
✅ Checks