fix: Problem with submodels id string #903
Conversation
WalkthroughThis pull request introduces broad refactorings and enhancements across multiple components. Key changes include updating service provider registrations by removing local binding constants in favor of standardized ones from the central contracts package, renaming receiver variables for clarity, and updating test logic (e.g. using a shared timestamp in auth tests and a revised artisan flag in console applications). Schema capabilities have been extended with new methods for boolean and custom columns, and dependency versions in the module file have been updated. Additional updates include improvements to logging, ORM context handling, and mock functionalities. Changes
Sequence Diagram(s)Console Application Artisan Flag FlowsequenceDiagram
participant User as User
participant App as Console.Application
User->>App: NewApplication(name, usage, usageText, version, useArtisan)
Note right of App: Initializes with useArtisan flag
App->>App: Adjust command arguments based on flag
Service Provider Binding Registration FlowsequenceDiagram
participant Container as Application Container
participant SP as ServiceProvider (e.g., Auth, Cache, etc.)
participant Contracts as Contracts Package
Container->>SP: Call Register()
SP->>Container: app.Singleton(contracts.BindingX, factoryFunc)
Container-->>SP: Service bound using contract key
Possibly related PRs
✨ 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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 2
🧹 Nitpick comments (2)
translation/service_provider.go (1)
30-30: Consider improving context handling.The current approach of extracting context from parameters map using type assertion (
parameters["ctx"].(context.Context)) is not type-safe and could lead to runtime panics if the context is missing or of wrong type.Consider creating a dedicated constructor or configuration method that explicitly accepts the context:
-func (translation *ServiceProvider) Register(app foundation.Application) { +func (translation *ServiceProvider) Register(app foundation.Application) { app.BindWith(contracts.BindingTranslation, func(app foundation.Application, parameters map[string]any) (any, error) { + ctx, ok := parameters["ctx"].(context.Context) + if !ok { + return nil, errors.New("context not provided or invalid") + } // ... other initializations ... - trans := NewTranslator(parameters["ctx"].(context.Context), loader, locale, fallback, logger) + trans := NewTranslator(ctx, loader, locale, fallback, logger) return trans, nil }) }database/schema/grammars/sqlite_test.go (1)
361-365: Consider enhancing the boolean type test coverage.While the basic test is good, consider adding test cases for:
- Column with default values (true/false)
- Column with nullable constraint
This would align with the test coverage of other data types in the suite.func (s *SqliteSuite) TestTypeBoolean() { - mockColumn := mocksschema.NewColumnDefinition(s.T()) + tests := []struct { + name string + setup func(column *mocksschema.ColumnDefinition) + expected string + }{ + { + name: "basic boolean", + setup: func(column *mocksschema.ColumnDefinition) {}, + expected: "tinyint(1)", + }, + { + name: "boolean with default true", + setup: func(column *mocksschema.ColumnDefinition) { + column.EXPECT().GetDefault().Return(true).Once() + }, + expected: "tinyint(1) default true", + }, + { + name: "nullable boolean", + setup: func(column *mocksschema.ColumnDefinition) { + column.EXPECT().GetNullable().Return(true).Once() + }, + expected: "tinyint(1) null", + }, + } - s.Equal("tinyint(1)", s.grammar.TypeBoolean(mockColumn)) + for _, test := range tests { + s.Run(test.name, func() { + mockColumn := mocksschema.NewColumnDefinition(s.T()) + test.setup(mockColumn) + s.Equal(test.expected, s.grammar.TypeBoolean(mockColumn)) + }) + } }
🛑 Comments failed to post (2)
console/service_provider.go (1)
19-19: 💡 Verification agent
🧩 Analysis chain
Verify the impact of the new boolean parameter.
The addition of
trueas the last argument toNewApplicationis a breaking change. Please ensure:
- All existing calls to
NewApplicationare updated.- The purpose of this boolean flag is documented.
🏁 Script executed:
#!/bin/bash # Search for other calls to NewApplication that might need updating rg "NewApplication\(" --type goLength of output: 6927
Impact of the
useArtisanParameter in the Console Module
- Verified that in the console package—including in
console/service_provider.goandconsole/application_test.go—all calls toNewApplicationhave been updated to include the new boolean parameter.- The breaking change is isolated to the console-specific version of
NewApplication(which now accepts(name, usage, usageText, version, useArtisan)); other packages use different overloads and remain unaffected.- Action Required: Please add documentation in
console/application.go(and any relevant docs) to clearly explain the purpose and expected behavior of the newuseArtisanflag.support/collect/collect.go (1)
61-62:
⚠️ Potential issueBreaking Change: Mutable operations in Reverse and Shuffle.
The switch from
lotolo/mutablechanges the behavior of these functions:
- They now modify the input slice in-place instead of creating a new slice
- This is more memory efficient but could break existing code expecting immutable behavior
Consider these options:
- Document the mutable behavior clearly in the function comments
- Create new functions for mutable operations and maintain immutable versions
Apply this diff to add documentation:
// Reverse reverses array so that the first element becomes the last, the second element becomes the second to last, and so on. +// Note: This function modifies the input slice in-place. func Reverse[T any](collection []T) []T { // Shuffle returns an array of shuffled values. Uses the Fisher-Yates shuffle algorithm. +// Note: This function modifies the input slice in-place. func Shuffle[T any](collection []T) []T {Also applies to: 67-68
![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: c374a38 | Previous: 3cde68c | Ratio |
|---|---|---|---|
Benchmark_EncryptString |
6656 ns/op 2160 B/op 15 allocs/op |
2225 ns/op 2160 B/op 15 allocs/op |
2.99 |
Benchmark_EncryptString - ns/op |
6656 ns/op |
2225 ns/op |
2.99 |
Benchmark_DecryptString |
7517 ns/op 2040 B/op 17 allocs/op |
2045 ns/op 2040 B/op 17 allocs/op |
3.68 |
Benchmark_DecryptString - ns/op |
7517 ns/op |
2045 ns/op |
3.68 |
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 #903 +/- ##
==========================================
Coverage ? 68.62%
==========================================
Files ? 218
Lines ? 18882
Branches ? 0
==========================================
Hits ? 12957
Misses ? 5256
Partials ? 669 ☔ View full report in Codecov by Sentry. |
📑 Description
Summary by CodeRabbit
New Features
Refactor
Tests
Chores
✅ Checks