Conversation
WalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on the Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (3)log/formatter/general.go (1)
The removal of tab characters from the stack trace output improves readability while maintaining the logical structure of the output. This change aligns with the established formatting preferences. Also applies to: 142-142 log/formatter/general_test.go (2)
The string concatenation approach is clean and maintains readability while properly testing the stack trace formatting.
The test cases thoroughly cover various scenarios:
All expected outputs correctly reflect the removal of tab characters while maintaining proper formatting. Also applies to: 255-255, 260-260, 265-265, 270-270, 275-275, 280-280, 285-285, 290-290 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 (
|
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: 2ab20b3 | Previous: 62b24a9 | Ratio |
|---|---|---|---|
BenchmarkFile_ReadWrite |
298635 ns/op 2072 B/op 27 allocs/op |
197338 ns/op 2072 B/op 27 allocs/op |
1.51 |
BenchmarkFile_ReadWrite - ns/op |
298635 ns/op |
197338 ns/op |
1.51 |
This comment was automatically generated by workflow using github-action-benchmark.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #748 +/- ##
==========================================
+ Coverage 69.64% 69.80% +0.16%
==========================================
Files 210 210
Lines 17820 17839 +19
==========================================
+ Hits 12411 12453 +42
+ Misses 4728 4706 -22
+ Partials 681 680 -1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (14)
event/application.go (1)
Line range hint
22-37: Consider using a map for better performanceWhile the current implementation correctly prevents duplicate jobs using
slices.Contains, using a map would provide O(1) lookup time instead of O(n).Consider this optimization:
func (app *Application) Register(events map[event.Event][]event.Listener) { var ( jobs []queue.Job - jobNames []string + jobNames = make(map[string]struct{}) ) if app.events == nil { app.events = map[event.Event][]event.Listener{} } for e, listeners := range events { app.events[e] = listeners for _, listener := range listeners { - if !slices.Contains(jobNames, listener.Signature()) { + if _, exists := jobNames[listener.Signature()]; !exists { jobs = append(jobs, listener) - jobNames = append(jobNames, listener.Signature()) + jobNames[listener.Signature()] = struct{}{} } } }event/application_test.go (2)
20-60: Consider adding more edge casesWhile the current test cases cover basic scenarios, consider adding tests for:
- Listeners with identical signatures but different implementations
- Empty listener arrays
- Error cases from queue.Register
Example structure:
{ name: "DuplicateSignatures", events: func() map[event.Event][]event.Listener { listener1 := mocksevent.NewListener(t) listener2 := mocksevent.NewListener(t) // Both return same signature but are different listeners listener1.EXPECT().Signature().Return("same").Once() listener2.EXPECT().Signature().Return("same").Once() // Expect only one to be registered mockQueue.EXPECT().Register(mock.MatchedBy(func(listeners []queue.Job) bool { return len(listeners) == 1 })).Once() // ... rest of the test setup }, },
70-73: Enhance assertion error messagesConsider adding more descriptive error messages to assertions to make test failures more debuggable.
-assert.Equal(t, len(events), len(app.GetEvents())) +assert.Equal(t, len(events), len(app.GetEvents()), "number of registered events should match input events") -assert.ElementsMatch(t, listeners, app.GetEvents()[e]) +assert.ElementsMatch(t, listeners, app.GetEvents()[e], "registered listeners should match input listeners for event %v", e)database/schema/schema_test.go (4)
272-289: Consider Using Integer Data Types for Integer EnumsThe
enum_intcolumns are currently defined with string types likecharacter varying(255)andvarchar, which might not be the most efficient way to store integer values. Consider using integer data types withCHECKconstraints to enforce allowed values. This can improve data integrity and performance.
573-579: Consistent Handling of Integer Enums in SQLiteIn the SQLite tests, the
enum_intcolumn is stored asvarchar. As with other databases, consider whether storing integer enums as strings is appropriate. Using an integer type withCHECKconstraints might provide better data validation and storage efficiency.
1358-1496: Reduce Code Duplication in Enum TestsThe test functions
TestEnum_Postgres,TestEnum_Sqlite,TestEnum_Mysql, andTestEnum_Sqlserverhave similar implementations. Refactoring these tests to use a common helper function or test suite can improve maintainability and reduce redundancy.
2204-2218: Maintain Consistent Naming for IndexesIn the
TestUniquetest, a unique index named"name_age_unique"is created. Consistency in naming conventions for indexes helps with clarity and maintenance. Ensure that index names follow a consistent pattern across the codebase.auth/errors.go (1)
1-16: Add Documentation for Exported Error VariablesThe exported error variables are beneficial for users handling authentication errors. Adding documentation comments for each variable can improve code readability and help developers understand how to use them effectively.
database/migration/default_creator_test.go (1)
80-87: Consider adding test case for existing table scenarioThe table existence check is a good addition, but consider enhancing test coverage by adding a test case where the table already exists.
Example test case to add:
{ name: "Create stub with existing table", stub: Stubs{}.Create(), signature: "202410131203_create_users_table", table: "users", // Mock Schema().HasTable() to return true expected: // Expected output with just return nil },database/schema/grammars/sqlite.go (1)
307-307: LGTM! Safe type conversion implementation.The use of
cast.ToStringSliceensures safe conversion of the[]anyvalues to strings for SQL generation. This maintains compatibility while supporting the new interface.Consider adding a test case with non-string enum values to verify the conversion behavior:
mockColumn.EXPECT().GetAllowed().Return([]any{1, "two", true}).Once()auth/auth_test.go (4)
349-350: Check error return from ID() methodThe test should verify both the ID and error values to ensure complete validation of the behavior.
- id, _ := s.auth.ID() + id, err := s.auth.ID() + s.ErrorIs(err, errors.AuthParseTokenFirst) s.Empty(id)
369-370: Check error return from ID() methodThe test should verify both the ID and error values to ensure complete validation of the behavior.
- id, _ := s.auth.ID() + id, err := s.auth.ID() + s.Nil(err) s.Equal("1", id)
391-393: Check error return from ID() methodThe test should verify both the ID and error values to ensure complete validation of the behavior.
- id, _ := s.auth.ID() + id, err := s.auth.ID() + s.ErrorIs(err, errors.AuthTokenExpired) s.Empty(id)
407-408: Check error return from ID() methodThe test should verify both the ID and error values to ensure complete validation of the behavior.
- id, _ := s.auth.ID() + id, err := s.auth.ID() + s.ErrorIs(err, errors.AuthInvalidToken) s.Empty(id)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (4)
mocks/auth/Auth.gois excluded by!mocks/**mocks/database/schema/Blueprint.gois excluded by!mocks/**mocks/database/schema/ColumnDefinition.gois excluded by!mocks/**mocks/testing/Database.gois excluded by!mocks/**
📒 Files selected for processing (26)
auth/auth.go(1 hunks)auth/auth_test.go(4 hunks)auth/errors.go(1 hunks)contracts/auth/auth.go(1 hunks)contracts/database/schema/blueprint.go(1 hunks)contracts/database/schema/column.go(2 hunks)contracts/testing/testing.go(1 hunks)database/migration/default_creator_test.go(1 hunks)database/migration/default_migrator.go(1 hunks)database/migration/stubs.go(1 hunks)database/schema/blueprint.go(1 hunks)database/schema/column.go(3 hunks)database/schema/grammars/mysql.go(4 hunks)database/schema/grammars/mysql_test.go(6 hunks)database/schema/grammars/postgres.go(2 hunks)database/schema/grammars/postgres_test.go(1 hunks)database/schema/grammars/sqlite.go(2 hunks)database/schema/grammars/sqlite_test.go(2 hunks)database/schema/grammars/sqlserver.go(2 hunks)database/schema/grammars/sqlserver_test.go(1 hunks)database/schema/schema_test.go(18 hunks)database/service_provider.go(3 hunks)event/application.go(2 hunks)event/application_test.go(1 hunks)testing/docker/database.go(1 hunks)testing/docker/database_test.go(0 hunks)
💤 Files with no reviewable changes (1)
- testing/docker/database_test.go
🔇 Additional comments (34)
event/application.go (2)
Line range hint 43-61: LGTM!
The unchanged methods maintain their correct functionality.
4-12: Verify Go version compatibility
The use of slices.Contains requires Go 1.21+. Ensure this version requirement is documented in go.mod and project requirements.
✅ Verification successful
Go version requirement is properly documented
The codebase already specifies Go 1.22.7 in go.mod, which satisfies the version requirement for using slices.Contains (Go 1.21+).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Go version requirement in go.mod
grep -r "go 1." . --include="go.mod"
Length of output: 55
database/schema/schema_test.go (2)
843-851: Verify Storage Type of Integer Enums in MySQL
The enum_int column in MySQL is defined as enum('1','2','3'), treating integers as strings. This could lead to unintended type conversions and potential bugs. Ensure that handling integers as strings aligns with your application's logic, or consider using an integer type with constraints.
Line range hint 1849-1890: Update Expected Types in MySQL ID Tests
The TestID_Mysql test cases now expect types like "bigint unsigned". Ensure that this change reflects the actual database schema and that all related code (e.g., foreign keys, queries) accommodates the unsigned types to prevent type mismatches.
contracts/database/schema/column.go (2)
34-35: LGTM: GetUnsigned() addition
The new method complements the existing Unsigned() method and follows the established getter pattern in the interface.
11-11: Verify impact of GetAllowed() return type change
The change from []string to []any makes the interface more flexible but could break existing implementations.
✅ Verification successful
The previous pattern was too specific. Let's broaden our search to:
- Find all GetAllowed implementations with any return type
- Find all usages of GetAllowed to understand the impact
- Look for test files that might be affected
Change to GetAllowed() return type is safe to proceed
The change from []string to []any is well-handled across the codebase:
- The actual implementation in
database/schema/column.goalready uses[]any - All grammar implementations (MySQL, PostgreSQL, SQLite, SQLServer) properly handle the type using
cast.ToStringSlice()for string conversion - Test files have been updated to use
[]anyin their expectations - Mock implementations reflect the new type signature
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all implementations of GetAllowed() to verify compatibility
ast-grep --pattern 'func $_($$$) GetAllowed() []string {
$$$
}'
Length of output: 67
Script:
#!/bin/bash
# Let's try a broader search for GetAllowed implementations and usages
ast-grep --pattern 'GetAllowed() []$_'
# Also search for direct usages of GetAllowed
rg "GetAllowed\(\)" -A 2
# And check for any test files that might be using this method
fd ".*_test\.go$" --exec grep -l "GetAllowed"
Length of output: 3217
testing/docker/database.go (1)
73-75: LGTM: Clean separation of migration logic
Good refactoring to separate migration concerns from Build(). This improves testability and follows the single responsibility principle.
database/schema/column.go (3)
128-133: New method GetUnsigned follows consistent pattern
The implementation of GetUnsigned follows the established pattern of other getter methods in the file.
44-53:
Return type change requires verification of all consumers
The GetAllowed method now returns []any instead of []string. This is a breaking change that requires updates in all consuming code.
Ensure all consumers of this method are updated to handle the new return type correctly. Consider adding a helper method that returns []string for backward compatibility.
9-9: Type change enhances flexibility but requires careful handling
The change from []string to []any for allowed field provides more flexibility but requires careful type assertion in consuming code.
Consider adding validation in setter methods to ensure only compatible types are stored, preventing runtime type assertion errors.
✅ Verification successful
Let me analyze the usage of GetAllowed() further to understand how the values are being set.
Let me try a different approach to find how the allowed values are set.
Based on the evidence gathered, I can now provide a final response:
Type change from []string to []any is safe and properly handled
The change to []any for the allowed field is well-justified and safely implemented:
- The field is set through the
Enum()method which explicitly accepts[]any - All database drivers properly handle the type conversion using
cast.ToStringSlice()when generating SQL - Test cases demonstrate proper validation across different database types (MySQL, PostgreSQL, SQLite, SQLServer)
- The implementation includes proper constraint checks to ensure data integrity
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the type change impacts any existing usage
rg -A 3 "GetAllowed\(\)" | grep -v "test"
Length of output: 2540
Script:
#!/bin/bash
# Check for any SetAllowed or similar methods that set the allowed values
ast-grep --pattern 'func $_($_ *ColumnDefinition) $_ {
$$.allowed = $_
$$$
}'
# Also check for direct initialization
rg "allowed:\s*\[" --type go
Length of output: 114
Script:
#!/bin/bash
# Look for methods that might set the allowed values
ast-grep --pattern 'func ($_ *ColumnDefinition) SetAllowed($_) $_'
# Check for enum type methods that might use allowed values
rg "Enum|ENUM" --type go -A 3
Length of output: 16114
database/service_provider.go (2)
12-14: Package renaming improves clarity
The package aliases better reflect their purpose and avoid potential naming conflicts.
Line range hint 22-40: Consistent application of renamed packages
The changes correctly update all references to use the new package names.
Also applies to: 42-59, 60-61
✅ Verification successful
Package references have been correctly updated throughout the codebase
The verification confirms that:
- The file
database/service_provider.gouses the correct package aliases:databaseorm,databaseschema, anddatabaseseeder - All imports in the file are using the correct package paths from
github.com/goravel/framework/database - No old/incorrect package references remain in the codebase that would conflict with these changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify no old package references remain
rg -l "\"github.com/goravel/framework/database/(orm|schema|seeder)\"[^/]"
Length of output: 73
Script:
#!/bin/bash
# Let's try a broader search for any imports related to database packages
rg -l "\"github.com/goravel/framework/database"
# Also search for any remaining references to orm, schema, or seeder
rg -l "database/(orm|schema|seeder)"
# Let's also check the current imports in the file
cat database/service_provider.go
Length of output: 7752
auth/auth.go (1)
Line range hint 79-89: LGTM! Method renamed to follow Go conventions.
The renaming from Id() to ID() aligns with Go's standard naming conventions. The implementation remains correct.
Let's verify all usages of this method have been updated:
✅ Verification successful
Method renaming from Id() to ID() is correctly implemented
The search results confirm that:
- There are no remaining occurrences of the old
Id()method in the auth package - All usages in
auth_test.goare using the newID()method - The other occurrences of
ID()in the codebase are unrelated - they belong to the database schema functionality whereID()is used for defining table columns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining usages of the old method name
# Test: Search for potential missed updates. Expect: No occurrences of `.Id()`
rg -g '*.go' '\.Id\(\)'
Length of output: 25
Script:
#!/bin/bash
# Let's try a more comprehensive search to verify the method renaming
# 1. Search for any occurrence of 'Id()' in method declarations
ast-grep --pattern 'func $_($_) Id() $_'
# 2. Search for any occurrence of '.Id()' in method calls with more context
rg -g '*.go' '\.Id\(' -A 2 -B 2
# 3. Search for the new method name 'ID()' to confirm its usage
rg -g '*.go' '\.ID\(' -A 2 -B 2
Length of output: 7256
contracts/database/schema/blueprint.go (2)
58-59: LGTM! New method adds flexibility for constraint management.
The addition of DropUniqueByName complements the existing DropUnique method, providing more precise control over unique constraints.
61-61: Consider type safety implications of using []any.
While changing Enum's parameter from []string to []any provides more flexibility, it might lead to runtime errors if non-string values are passed. Consider:
- Adding runtime type checking
- Using a more specific type like
interface{ String() string }
Let's check current enum usage patterns:
database/migration/default_migrator.go (1)
312-325: LGTM! Improved transaction and connection management.
The changes enhance the robustness of the migration process by:
- Properly scoping the transaction
- Adding clear documentation about connection conflicts
- Maintaining correct connection state
Let's verify the transaction handling patterns across migrations:
✅ Verification successful
The previous ast-grep pattern might have been too strict. Let's try using ripgrep to find transaction-related code blocks and connection management patterns to verify the changes.
Transaction and connection management patterns are consistent
The code review is accurate. The search results confirm proper transaction and connection management patterns across the codebase:
- Transactions are properly scoped with clear boundaries using
Transaction(func(tx orm.Query) error). - Connection state is consistently managed with
SetConnectionandSetQuerypairs. - The pattern of resetting to default connection after transaction is consistently applied.
- The comment about connection conflicts is valid and the solution (logging outside transaction) is correctly implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check transaction handling patterns in migrations
# Test: Look for potential transaction-related issues
ast-grep --pattern 'Transaction(func($tx $_) error {
$$$
})'
Length of output: 64
Script:
#!/bin/bash
# Let's try a different pattern to match transaction handling
rg -U "Transaction.*\{[\s\S]+?\}" --type go
# Also search for specific connection management patterns
rg "SetQuery|SetConnection" --type go -A 2 -B 2
Length of output: 22478
database/schema/grammars/sqlite_test.go (1)
364-364: Verify the impact of GetAllowed interface change across the codebase.
The change from []string to []any for GetAllowed() return type is reflected here. Let's verify all implementations and usages are updated consistently.
✅ Verification successful
Interface change from []string to []any for GetAllowed() is consistently implemented
The verification shows that the interface change is properly reflected across the codebase:
- Interface definition in
contracts/database/schema/column.godeclaresGetAllowed() []any - Implementation in
database/schema/column.goreturns[]any - All test expectations in different database drivers (sqlite, postgres, mysql, sqlserver) consistently use
[]any{"a", "b"} - All usages in grammar implementations properly handle the type using
cast.ToStringSlice()for string conversion
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementations and usages of GetAllowed to ensure consistent updates
# Find interface definition
echo "=== GetAllowed interface definition ==="
rg --type go "GetAllowed\(\)" -A 2 -B 2
# Find implementations
echo "=== GetAllowed implementations ==="
ast-grep --pattern 'func $_($_) GetAllowed() $_ {
$$$
}'
# Find test expectations
echo "=== GetAllowed test expectations ==="
rg --type go "GetAllowed\(\).Return\(" -A 1
Length of output: 5715
database/schema/grammars/sqlserver_test.go (1)
299-299: LGTM! Consistent with interface changes.
The update to use []any for GetAllowed() is consistent with the interface change and properly validates SQL Server's enum handling with Unicode string literals.
database/schema/grammars/sqlite.go (1)
8-9: LGTM! Appropriate use of cast package.
The addition of github.com/spf13/cast is a good choice for safe type conversions, especially when dealing with interface{} values.
database/schema/grammars/mysql.go (3)
28-29: Review the order of modifiers carefully.
The order of modifiers affects the final SQL output. The current order places ModifyUnsigned before other modifiers, which is correct as it's a type modifier that should be applied immediately after the type definition.
Also applies to: 31-34
308-314: LGTM: Clean implementation of unsigned column support.
The ModifyUnsigned method follows a clear and consistent pattern with other modifier methods in the codebase.
361-361: Enhance type safety with cast.ToStringSlice.
Good use of cast.ToStringSlice for converting allowed values to strings, improving type safety and flexibility.
database/schema/grammars/mysql_test.go (4)
37-37: Test coverage properly validates unsigned column behavior.
The test case correctly verifies that the unsigned attribute is not added when GetUnsigned returns false.
Also applies to: 43-43
80-80: Comprehensive test coverage for SQL generation.
The test properly validates the SQL output with unsigned columns, ensuring the attribute is correctly positioned in the column definition.
Also applies to: 96-96, 98-98
261-261: Test coverage for column definition generation is complete.
The test properly validates the generation of column definitions with the unsigned attribute.
Also applies to: 270-270, 272-272
381-381: Updated test to reflect new type handling in enum values.
The test now correctly expects []any instead of []string for allowed values, aligning with the implementation changes.
database/schema/grammars/sqlserver.go (2)
8-9: LGTM: Consistent import usage across grammar implementations.
The addition of the cast package maintains consistency with other database grammar implementations.
339-339: LGTM: Consistent type handling in enum implementation.
The use of cast.ToStringSlice aligns with other grammar implementations, ensuring consistent type handling across different databases.
database/schema/blueprint.go (2)
182-186: LGTM: New method follows established patterns
The DropUniqueByName implementation is consistent with other similar methods in the file and properly uses the indexCommand helper.
188-188: Verify the impact of the Enum signature change
The parameter type change from []string to []any provides more flexibility but might affect existing code.
✅ Verification successful
Let me analyze the findings. I can see that:
- The
Enummethod is defined indatabase/schema/blueprint.gowith the new[]anysignature - The interface in
contracts/database/schema/blueprint.goalready defines it with[]any - All database drivers (mysql, postgres, sqlite, sqlserver) use
cast.ToStringSlice()to handle the values - The mock implementations also use
[]any
Let me verify if there are any direct usages of this method in the codebase:
The Enum signature change is safe and properly implemented
The change from []string to []any is well-supported throughout the codebase:
- The interface in
contracts/database/schema/blueprint.goalready defines it with[]any - All database drivers (MySQL, PostgreSQL, SQLite, SQLServer) handle the conversion using
cast.ToStringSlice() - Existing usage in the codebase already utilizes both string and integer enum values
- Mock implementations are aligned with the
[]anysignature
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of the Enum method to verify the impact
# Look for direct calls to Enum method
rg -l 'Enum\(' | grep -v '_test.go' | while read -r file; do
echo "=== $file ==="
rg -A 2 'Enum\(' "$file"
done
Length of output: 3374
Script:
#!/bin/bash
# Find all direct calls to Enum method in non-test files
rg -B 2 -A 2 '\.Enum\([^)]+\)' --type go --no-filename | grep -v '_test.go'
Length of output: 1183
database/schema/grammars/postgres.go (2)
8-8: LGTM: Added appropriate type conversion library
The cast package is a good choice for safe type conversions.
355-355: LGTM: Enhanced type safety in enum handling
The use of cast.ToStringSlice ensures safe conversion of enum values while maintaining SQL injection protection through proper quoting.
database/schema/grammars/postgres_test.go (1)
423-423: LGTM: Test updated to match new interface
The test expectation has been properly updated to reflect the new []any return type while maintaining the same functional validation.
auth/auth_test.go (1)
Line range hint 1-724: Overall test coverage is comprehensive
The test suite thoroughly covers various authentication scenarios including token parsing, validation, expiration, and refresh flows. The test cases are well-structured and follow good testing practices with proper setup and assertions.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
validation/validation_test.go (2)
198-228: Consider adding more test casesWhile the test effectively verifies basic nested struct binding, consider adding:
- Negative test cases to verify validation failures
- More complex nested structures with multiple levels
- Edge cases with empty/null nested maps
198-199: Enhance test documentationWhile the GitHub issue reference is helpful, consider adding:
- A brief description of what aspect of nested struct binding this test verifies
- Examples of the validation behavior being tested
// Fix: https://github.com/goravel/goravel/issues/533 +// TestBindWithNestedStruct verifies that the validator correctly binds nested map structures +// to struct fields, including proper type conversion of array values from interface{} to string.log/logrus_writer.go (1)
334-348: Reduce code duplication in driver configurationThe output and formatter configuration is duplicated between SingleDriver and DailyDriver cases.
Consider extracting to a helper function:
+func configureLoggerOutput(config config.Config, json foundation.Json, instance *logrus.Logger, channelPath string) { + if config.GetBool(channelPath + ".print") { + instance.SetOutput(os.Stdout) + instance.SetFormatter(formatter.NewGeneral(config, json)) + } +} case log.SingleDriver: logLogger := logger.NewSingle(config, json) hook, err = logLogger.Handle(channelPath) if err != nil { return err } - if config.GetBool(channelPath + ".print") { - instance.SetOutput(os.Stdout) - instance.SetFormatter(formatter.NewGeneral(config, json)) - } + configureLoggerOutput(config, json, instance, channelPath) case log.DailyDriver: logLogger := logger.NewDaily(config, json) hook, err = logLogger.Handle(channelPath) if err != nil { return err } - if config.GetBool(channelPath + ".print") { - instance.SetOutput(os.Stdout) - instance.SetFormatter(formatter.NewGeneral(config, json)) - } + configureLoggerOutput(config, json, instance, channelPath)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
mocks/log/Entry.gois excluded by!mocks/**
📒 Files selected for processing (7)
contracts/log/log.go(2 hunks)log/application.go(3 hunks)log/entry.go(2 hunks)log/logrus_writer.go(5 hunks)log/logrus_writer_test.go(0 hunks)support/constant.go(1 hunks)validation/validation_test.go(2 hunks)
💤 Files with no reviewable changes (1)
- log/logrus_writer_test.go
✅ Files skipped from review due to trivial changes (1)
- support/constant.go
🔇 Additional comments (7)
validation/validation_test.go (1)
9-9: LGTM: Appropriate import for test assertions
The addition of the require package from testify is appropriate for stricter test assertions.
log/entry.go (2)
12-12: LGTM: New data field addition
The addition of the data log.Data field aligns with the interface changes in contracts/log/log.go and enhances the logging capabilities.
34-35: LGTM: Method implementations are consistent
The implementations of all methods (Data, Code, Owner, Response, Time, Trace, User, With) are straightforward and correctly match their interface definitions. The reordering improves code organization while maintaining functionality.
Also applies to: 26-28, 46-47, 54-55, 62-63, 66-67, 70-71, 74-75
contracts/log/log.go (2)
26-27: LGTM: New Data type improves type safety
The introduction of the Data type alias provides better type safety and clarity for logging data structures.
101-102: LGTM: Interface enhancements are well-structured
The Entry interface updates are comprehensive and well-organized:
- New methods (Code, Data, Time) added
- Method signatures updated consistently
- Documentation is clear and descriptive
Also applies to: 105-106, 117-120, 123-126
log/logrus_writer.go (2)
306-312: LGTM! Clean variable organization
Good refactoring to group related variables in a single var block, improving code readability.
383-383: LGTM! Safe type conversion
The type conversion from entry.Data to map[string]any is safe and aligns with the Entry struct's data field type.
| fileLine := stackStr[secondLastColon+1:] | ||
| method := stackStr[:secondLastColon] | ||
| return fmt.Sprintf("\t%s [%s]\n", fileLine, method) | ||
| return fmt.Sprintf("%s [%s]\n", fileLine, method) |
There was a problem hiding this comment.
Removing \t looks better:
📑 Description
A few optimization before upgrading.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor