Conversation
WalkthroughAcross the codebase, hardcoded or module-specific binding constants have been replaced with centralized constants from a new contracts package. The changes update service provider files across multiple modules (auth, cache, config, console, crypt, event, filesystem, grpc, hash, http, log, mail, queue, route, schedule, session, testing, translation, and validation) by replacing their binding constants (e.g. "goravel.auth", "goravel.cache", etc.) with contracts.Binding* constants. Additionally, method receiver variables have been renamed to the uniform identifier “r”. Related database and foundation files have also been updated to reference the new binding constants, and a new contracts package was added to centralize these definitions. Changes
✨ 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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #868 +/- ##
=======================================
Coverage 67.54% 67.54%
=======================================
Files 151 151
Lines 10343 10343
=======================================
Hits 6986 6986
Misses 2990 2990
Partials 367 367 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
translation/service_provider.go (1)
11-11: Remove unused constant.The
Bindingconstant is no longer used since it's been replaced withcontracts.BindingTranslation.-const Binding = "goravel.translation"mail/service_provider.go (1)
13-13: Remove redundant Binding constant.The hardcoded Binding constant should be removed as it's been replaced by
contracts.BindingMail.-const Binding = "goravel.mail"
🧹 Nitpick comments (10)
config/service_provider.go (1)
12-12: Consider a more descriptive receiver name.While 'r' is consistent with other service providers, in this case it might be confusing as 'config' (the previous name) better describes the receiver's purpose. Consider using 'sp' (for ServiceProvider) or 'provider' for better clarity.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 12-13: config/service_provider.go#L12-L13
Added lines #L12 - L13 were not covered by testshash/service_provider.go (1)
4-4: LGTM! Changes align with optimization goals.The replacement of hardcoded binding strings with centralized constants from the contracts package is a good approach that will help reduce the final binary size by allowing better tree-shaking of unused packages.
Also applies to: 12-13
log/service_provider.go (1)
27-29: Consider removing empty Boot method.The Boot method is empty and could be removed to reduce code size, which aligns with the PR's objective of reducing output file size.
-func (r *ServiceProvider) Boot(app foundation.Application) { - -}crypt/service_provider.go (1)
28-30: Consider removing empty Boot method.The Boot method is empty and could be removed to reduce code size, which aligns with the PR's objective of reducing output file size.
-func (r *ServiceProvider) Boot(app foundation.Application) { - -}🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 28-28: crypt/service_provider.go#L28
Added line #L28 was not covered by testsfilesystem/service_provider.go (1)
11-14: Consider encapsulating global variables.The global variables
ConfigFacadeandStorageFacadecould lead to issues in concurrent scenarios. Consider encapsulating them within the ServiceProvider struct.type ServiceProvider struct { + ConfigFacade configcontract.Config + StorageFacade filesystemcontract.Storage }contracts/binding.go (1)
3-28: LGTM! Well-organized centralization of binding constants.The centralization of binding constants in a dedicated package improves maintainability and allows for selective inclusion of services, which aligns with the PR objective of reducing output file size.
This architectural change provides several benefits:
- Single source of truth for binding keys
- Consistent naming conventions
- Easier maintenance
- Enables tree-shaking optimization
session/service_provider.go (1)
35-38: Consider adding error handling in Boot method.The Boot method assigns global variables without checking if MakeSession or MakeConfig returned nil.
Consider adding error handling:
func (r *ServiceProvider) Boot(app foundation.Application) { - SessionFacade = app.MakeSession() - ConfigFacade = app.MakeConfig() + if session := app.MakeSession(); session != nil { + SessionFacade = session + } + if config := app.MakeConfig(); config != nil { + ConfigFacade = config + } }schedule/service_provider.go (1)
38-40: Document the empty Boot method.The Boot method is empty but retained. Consider adding a comment explaining why it's needed.
func (r *ServiceProvider) Boot(app foundation.Application) { + // No boot operations required for schedule service provider }queue/service_provider.go (1)
13-16: Consider refactoring global variables to dependency injection.Global variables
LogFacadeandOrmFacadecould be refactored to use dependency injection for better testability and maintainability.Consider moving these to struct fields:
type ServiceProvider struct { + logFacade log.Log + ormFacade orm.Orm }http/service_provider.go (1)
14-17: Consider refactoring global variables to dependency injection.Global variables
CacheFacadeandRateLimiterFacadecould be refactored to use dependency injection for better testability and maintainability.Consider moving these to struct fields:
type ServiceProvider struct { + cacheFacade cache.Cache + rateLimiterFacade http.RateLimiter }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
auth/service_provider.go(2 hunks)cache/service_provider.go(2 hunks)config/service_provider.go(1 hunks)console/service_provider.go(1 hunks)contracts/binding.go(1 hunks)crypt/service_provider.go(2 hunks)database/orm/orm.go(2 hunks)database/schema/schema.go(0 hunks)database/seeder/seeder.go(0 hunks)database/service_provider.go(4 hunks)event/service_provider.go(2 hunks)filesystem/service_provider.go(3 hunks)foundation/application_test.go(16 hunks)foundation/container.go(25 hunks)grpc/service_provider.go(2 hunks)hash/service_provider.go(2 hunks)http/service_provider.go(1 hunks)log/service_provider.go(2 hunks)mail/service_provider.go(3 hunks)queue/service_provider.go(3 hunks)route/service_provider.go(2 hunks)schedule/service_provider.go(2 hunks)session/service_provider.go(3 hunks)testing/service_provider.go(2 hunks)translation/service_provider.go(3 hunks)validation/service_provider.go(1 hunks)
💤 Files with no reviewable changes (2)
- database/seeder/seeder.go
- database/schema/schema.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
config/service_provider.go
[warning] 12-13: config/service_provider.go#L12-L13
Added lines #L12 - L13 were not covered by tests
[warning] 18-18: config/service_provider.go#L18
Added line #L18 was not covered by tests
filesystem/service_provider.go
[warning] 19-20: filesystem/service_provider.go#L19-L20
Added lines #L19 - L20 were not covered by tests
[warning] 30-30: filesystem/service_provider.go#L30
Added line #L30 was not covered by tests
console/service_provider.go
[warning] 14-15: console/service_provider.go#L14-L15
Added lines #L14 - L15 were not covered by tests
[warning] 23-24: console/service_provider.go#L23-L24
Added lines #L23 - L24 were not covered by tests
[warning] 27-27: console/service_provider.go#L27
Added line #L27 was not covered by tests
crypt/service_provider.go
[warning] 12-13: crypt/service_provider.go#L12-L13
Added lines #L12 - L13 were not covered by tests
[warning] 28-28: crypt/service_provider.go#L28
Added line #L28 was not covered by tests
hash/service_provider.go
[warning] 12-13: hash/service_provider.go#L12-L13
Added lines #L12 - L13 were not covered by tests
[warning] 23-23: hash/service_provider.go#L23
Added line #L23 was not covered by tests
event/service_provider.go
[warning] 14-15: event/service_provider.go#L14-L15
Added lines #L14 - L15 were not covered by tests
[warning] 25-26: event/service_provider.go#L25-L26
Added lines #L25 - L26 were not covered by tests
[warning] 29-29: event/service_provider.go#L29
Added line #L29 was not covered by tests
auth/service_provider.go
[warning] 18-19: auth/service_provider.go#L18-L19
Added lines #L18 - L19 were not covered by tests
[warning] 43-43: auth/service_provider.go#L43
Added line #L43 was not covered by tests
[warning] 48-49: auth/service_provider.go#L48-L49
Added lines #L48 - L49 were not covered by tests
[warning] 52-52: auth/service_provider.go#L52
Added line #L52 was not covered by tests
cache/service_provider.go
[warning] 14-15: cache/service_provider.go#L14-L15
Added lines #L14 - L15 were not covered by tests
[warning] 32-33: cache/service_provider.go#L32-L33
Added lines #L32 - L33 were not covered by tests
[warning] 36-36: cache/service_provider.go#L36
Added line #L36 was not covered by tests
grpc/service_provider.go
[warning] 12-13: grpc/service_provider.go#L12-L13
Added lines #L12 - L13 were not covered by tests
[warning] 23-23: grpc/service_provider.go#L23
Added line #L23 was not covered by tests
http/service_provider.go
[warning] 19-20: http/service_provider.go#L19-L20
Added lines #L19 - L20 were not covered by tests
[warning] 23-23: http/service_provider.go#L23
Added line #L23 was not covered by tests
[warning] 28-28: http/service_provider.go#L28
Added line #L28 was not covered by tests
foundation/container.go
[warning] 247-247: foundation/container.go#L247
Added line #L247 was not covered by tests
[warning] 280-280: foundation/container.go#L280
Added line #L280 was not covered by tests
[warning] 300-300: foundation/container.go#L300
Added line #L300 was not covered by tests
[warning] 310-310: foundation/container.go#L310
Added line #L310 was not covered by tests
🔇 Additional comments (29)
config/service_provider.go (1)
12-16: 🏁 Script executed:#!/bin/bash # Check if there are any existing tests for the Register method rg -l "TestServiceProvider.*Register" ./config/Length of output: 49
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 12-13: config/service_provider.go#L12-L13
Added lines #L12 - L13 were not covered by testsroute/service_provider.go (1)
12-21: 🏁 Script executed:#!/bin/bash # Check for existing test coverage rg -l "TestServiceProvider.*Register.*Config" ./route/Length of output: 56
grpc/service_provider.go (2)
12-12: Previous receiver name was incorrect.The previous receiver name 'route' in the gRPC service provider was incorrect. Good that it's been changed to 'r', though consider using 'sp' or 'provider' for even better clarity.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 12-13: grpc/service_provider.go#L12-L13
Added lines #L12 - L13 were not covered by tests
12-21: 🏁 Script executed:#!/bin/bash # Check for existing test coverage rg -l "TestServiceProvider.*Register.*Config" ./grpc/Length of output: 55
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 12-13: grpc/service_provider.go#L12-L13
Added lines #L12 - L13 were not covered by testshash/service_provider.go (1)
12-21: 🏁 Script executed:#!/bin/bash # Check for existing test coverage rg -l "TestServiceProvider.*Register.*Config" ./hash/Length of output: 55
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 12-13: hash/service_provider.go#L12-L13
Added lines #L12 - L13 were not covered by testsvalidation/service_provider.go (1)
4-4: LGTM! Changes align with binding centralization.The changes correctly move from package-specific binding to centralized contracts.BindingValidation while maintaining existing functionality.
Also applies to: 13-14
log/service_provider.go (1)
4-4: LGTM! Changes align with binding centralization.The changes correctly move from package-specific binding to centralized contracts.BindingLog while maintaining existing functionality and error handling.
Also applies to: 12-13
crypt/service_provider.go (1)
4-4: 🏁 Script executed:#!/bin/bash # Description: Check existing test coverage for crypt service provider rg -l "TestServiceProvider" -g "*_test.go"Length of output: 42
event/service_provider.go (3)
4-4: LGTM! Added import for centralized contracts package.The import of the contracts package aligns with the new binding constant usage.
14-22: Add test coverage for the Register method.The Register method lacks test coverage according to static analysis.
Please add tests to verify the registration behavior, especially:
- Successful registration with queue facade
- Error handling when queue facade is not set
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 14-15: event/service_provider.go#L14-L15
Added lines #L14 - L15 were not covered by tests
25-27: Add test coverage for Boot and registerCommands methods.These methods lack test coverage according to static analysis.
Please add tests to verify:
- Command registration in Boot
- Proper initialization of event commands
Also applies to: 29-34
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 25-26: event/service_provider.go#L25-L26
Added lines #L25 - L26 were not covered by testssession/service_provider.go (1)
19-33: LGTM! Well-structured dependency validation.The Register method properly validates all required dependencies (config and JSON parser) before initialization.
schedule/service_provider.go (1)
12-36: LGTM! Comprehensive dependency validation.The Register method thoroughly validates all required dependencies (config, artisan, log, and cache) before initialization.
cache/service_provider.go (2)
14-15: LGTM! Improved binding management and receiver naming.The changes correctly move the binding constant to the contracts package and fix the misleading receiver name.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 14-15: cache/service_provider.go#L14-L15
Added lines #L14 - L15 were not covered by tests
32-34: Add test coverage for modified methods.The modified methods lack test coverage according to the code coverage report.
Would you like me to help generate test cases for these methods?
Also applies to: 36-36
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 32-33: cache/service_provider.go#L32-L33
Added lines #L32 - L33 were not covered by teststranslation/service_provider.go (1)
16-17: LGTM! Improved binding management.The changes correctly move the binding constant to the contracts package.
queue/service_provider.go (1)
21-22: LGTM! Improved binding management.The changes correctly move the binding constant to the contracts package.
http/service_provider.go (1)
19-25: Add test coverage for modified methods.The modified methods lack test coverage according to the code coverage report.
Would you like me to help generate test cases for these methods?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 19-20: http/service_provider.go#L19-L20
Added lines #L19 - L20 were not covered by tests
[warning] 23-23: http/service_provider.go#L23
Added line #L23 was not covered by teststesting/service_provider.go (2)
4-4: LGTM! Importing contracts package.The change aligns with the PR objective of centralizing binding constants.
23-26: Add test coverage for the Register method.The Register method's binding implementation lacks test coverage. Consider adding tests to verify the singleton registration works correctly.
Would you like me to help generate test cases for this method?
console/service_provider.go (1)
14-20: Add test coverage for the Register method.The Register method's implementation of console binding lacks test coverage. Consider adding tests to verify:
- Correct binding name registration
- Application version passing
- Error handling scenarios
Would you like me to help generate test cases for this method?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 14-15: console/service_provider.go#L14-L15
Added lines #L14 - L15 were not covered by testsmail/service_provider.go (1)
18-31: LGTM! Clean binding implementation.The Register method correctly uses contracts.BindingMail and includes proper error handling for config and queue dependencies.
auth/service_provider.go (2)
18-46: Add test coverage for binding registrations.The Register method's implementations for auth and gate bindings lack test coverage. Consider adding tests to verify:
- Correct binding registration for both auth and gate
- Error handling for missing dependencies
- Context parameter validation
Would you like me to help generate test cases for these scenarios?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 18-19: auth/service_provider.go#L18-L19
Added lines #L18 - L19 were not covered by tests
[warning] 43-43: auth/service_provider.go#L43
Added line #L43 was not covered by tests
52-57: LGTM! Clean command registration.The command registration is well-structured and properly uses the contracts package.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 52-52: auth/service_provider.go#L52
Added line #L52 was not covered by testsdatabase/service_provider.go (1)
7-7: LGTM! The binding constants have been successfully migrated to the contracts package.The changes effectively centralize the binding constants while maintaining the existing functionality. This aligns with the PR's objective of optimizing the output file size.
Also applies to: 25-25, 51-51, 80-80
database/orm/orm.go (1)
8-8: LGTM! The ORM binding constant has been successfully migrated to the contracts package.The changes maintain the existing functionality while aligning with the centralized binding approach.
Also applies to: 137-137
foundation/container.go (2)
8-8: LGTM! All binding constants have been successfully migrated to the contracts package.The changes effectively centralize all binding constants while maintaining the existing dependency injection functionality.
Also applies to: 67-310
247-247: Add test coverage for Make methods.*The following methods lack test coverage:
MakeSchema(line 247)MakeTesting(line 280)MakeView(line 300)MakeSeeder(line 310)Consider adding test cases to ensure these methods work as expected.
Would you like me to help generate test cases for these methods?
Also applies to: 280-280, 300-300, 310-310
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 247-247: foundation/container.go#L247
Added line #L247 was not covered by testsfoundation/application_test.go (1)
15-15: LGTM! Test cases have been successfully updated to use the new binding constants.The changes maintain comprehensive test coverage while adapting to the centralized binding approach.
Also applies to: 87-371
| package filesystem | ||
|
|
||
| import ( | ||
| "github.com/goravel/framework/contracts" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM! Changes align with binding centralization.
The changes correctly move from package-specific binding to centralized contracts.BindingFilesystem while maintaining existing functionality.
Static analysis indicates missing test coverage. Consider adding tests for the Register method:
Also applies to: 19-20
🏁 Script executed:
#!/bin/bash
# Description: Check existing test coverage for filesystem service provider
rg -l "TestServiceProvider" -g "*_test.go"Length of output: 42
I'm going to run an additional shell script to search for test functions that exercise the Register method (which the review comment suggests is missing). This will help us reconfirm whether test coverage is indeed absent.
🏁 Script executed:
#!/bin/bash
# Searching for test functions in *_test.go files that mention "Register"
rg --ignore-case -l "func Test.*Register" -g "*_test.go"Length of output: 84
Missing Test Coverage for Filesystem Register Method
• The initial test search for “TestServiceProvider” returned no matching test files, indicating that there isn’t a dedicated test suite for the filesystem service provider.
• The only test function we found mentioning “Register” is in "event/application_test.go", which appears unrelated to the filesystem service provider.
It looks like the Register method in filesystem/service_provider.go (also regarding lines 19–20) does not have direct test coverage. Please add tests specifically for this functionality to ensure reliability.
📑 Description
Closes https://github.com/goravel/goravel/issues/?
Normal binding for internal packages will include the package even we never registered it in app because of Make func in container which make the output file larger, so we need a diff. way to bind these packages and keep Make functions in container
Summary by CodeRabbit
• Behind-the-scenes improvements have been made to standardize how core services are registered and managed.
• Internal naming conventions have been updated for consistency, enhancing maintainability without affecting user-facing behavior.
fresh goravl contains all service providers
➜ goravel git:(v1.15.x) ✗ go build -o goravel main.go ➜ goravel git:(v1.15.x) ✗ ll total 152304 -rw-r--r--@ 1 ahmed3mar staff 550B Feb 7 10:01 Dockerfile -rw-r--r--@ 1 ahmed3mar staff 1.0K Feb 7 10:01 LICENSE -rw-r--r--@ 1 ahmed3mar staff 9.2K Feb 7 10:01 README.md -rw-r--r--@ 1 ahmed3mar staff 9.0K Feb 7 10:01 README_zh.md drwxr-xr-x@ 7 ahmed3mar staff 224B Feb 7 10:01 app drwxr-xr-x@ 3 ahmed3mar staff 96B Feb 7 10:01 bootstrap drwxr-xr-x@ 16 ahmed3mar staff 512B Feb 7 10:01 config drwxr-xr-x@ 5 ahmed3mar staff 160B Feb 7 10:01 database -rw-r--r--@ 1 ahmed3mar staff 114B Feb 7 10:01 docker-compose.yml -rw-r--r--@ 1 ahmed3mar staff 9.9K Feb 8 02:57 go.mod -rw-r--r--@ 1 ahmed3mar staff 111K Feb 8 02:57 go.sum -rwxr-xr-x@ 1 ahmed3mar staff 74M Feb 8 12:35 goravel # <--- 74MB -rw-r--r--@ 1 ahmed3mar staff 707B Feb 7 10:01 main.go drwxr-xr-x 5 ahmed3mar staff 160B Feb 7 11:45 packages drwxr-xr-x@ 3 ahmed3mar staff 96B Feb 7 10:01 public drwxr-xr-x@ 3 ahmed3mar staff 96B Feb 7 10:01 resources drwxr-xr-x@ 5 ahmed3mar staff 160B Feb 7 10:01 routes drwxr-xr-x@ 4 ahmed3mar staff 128B Feb 7 10:01 storage drwxr-xr-x@ 4 ahmed3mar staff 128B Feb 7 10:01 testswith flags
-s -wbut if we need to use goravel for example only with log, console, http, route and gin only and our project doesn't need for database or queue system or others
normal
with move binding to external not inside same package
the size of output file is decreased from 39MB to 22MB
✅ Checks