feat: [#667] Registering service providers don't dependent order#1097
feat: [#667] Registering service providers don't dependent order#1097
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors service provider handling to support explicit dependencies and ordering, improves error clarity by replacing ArtisanFacadeNotSet with ConsoleFacadeNotSet, and adds a topological sort (with cycle detection) for configured service providers.
- Added
Bindings,Dependencies, andProvideFormethods to key service providers and asortConfiguredServiceProvidersfunction with cycle detection. - Replaced all uses of
ArtisanFacadeNotSetwithConsoleFacadeNotSetand introducedConsoleProvidersNotArray. - Updated application initialization to cache and sort configured providers and enhanced related tests.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| testing/test_case.go | Panic now uses ConsoleFacadeNotSet instead of ArtisanFacadeNotSet |
| testing/service_provider.go | Updated error logging to use ConsoleFacadeNotSet |
| testing/docker/database_test.go | Test expectations updated to expect ConsoleFacadeNotSet |
| testing/docker/database.go | NewDatabase returns ConsoleFacadeNotSet instead of old error |
| schedule/service_provider.go | Replaced ArtisanFacadeNotSet with ConsoleFacadeNotSet |
| foundation/console/package_make_command_stubs.go | Added Bindings, Dependencies, ProvideFor stubs |
| foundation/application_test.go | Added tests for sortConfiguredServiceProviders and detectCycle |
| foundation/application.go | Implemented caching, topological sort of providers, and error updates |
| errors/list.go | Added ConsoleFacadeNotSet, ConsoleProvidersNotArray, and ServiceProviderCycle |
| database/service_provider.go | Added provider dependency metadata methods |
| cache/service_provider.go | Added provider dependency metadata methods |
Comments suppressed due to low confidence (2)
errors/list.go:7
- [nitpick] The error message includes context-specific instructions ("skipping artisan command execution"), which may not apply in all initialization paths. Consider simplifying to a generic message (e.g., "console facade is not initialized") and handle action-specific logging where it occurs.
ConsoleFacadeNotSet = New("console facade is not initialized, skipping artisan command execution")
errors/list.go:40
- [nitpick] This error message is grammatically awkward. Consider rephrasing to improve clarity, for example: "app.providers configuration is not a []foundation.ServiceProvider; service provider registration skipped."
ConsoleProvidersNotArray = New("the app.providers configuration is not of type []foundation.ServiceProvider, skipping registering service providers")
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1097 +/- ##
==========================================
+ Coverage 71.16% 71.45% +0.29%
==========================================
Files 183 183
Lines 12826 13057 +231
==========================================
+ Hits 9127 9330 +203
- Misses 3330 3358 +28
Partials 369 369 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| return envFilePath | ||
| } | ||
|
|
||
| func sortConfiguredServiceProviders(providers []foundation.ServiceProvider) []foundation.ServiceProvider { |
There was a problem hiding this comment.
We can create a separate package for the DAG (Directed Acyclic Graph) instead of including it in application.go. This package will handle all the heavy lifting, like topological sorting, cycle detection, and other related operations.
There was a problem hiding this comment.
Sorry, not very clear, welcome to create a related issue and discuss it first.
| type ServiceProvider struct { | ||
| } | ||
|
|
||
| func (r *ServiceProvider) Bindings() []string { |
There was a problem hiding this comment.
Is this used to bind the same service to multiple keys?
There was a problem hiding this comment.
For the case below: multiple bindings are in the same service provider.
|
|
||
| func (r *ServiceProvider) Bindings() []string { | ||
| return []string{ | ||
| contracts.BindingOrm, |
There was a problem hiding this comment.
We can move this to a different package, bindings. I'm not sure contracts conveys the right meaning here. Then we can use bindings.Orm.
foundation/application.go
Outdated
| } | ||
|
|
||
| // Create a map for quick lookup of providers by their binding names | ||
| providerMap := make(map[string]foundation.ServiceProvider) |
There was a problem hiding this comment.
We may not need providerMap, since both providerMap and bindingToProvider serve the same purpose.
foundation/application.go
Outdated
| for _, provider := range providers { | ||
| for _, binding := range getBindings(provider) { | ||
| providerMap[binding] = provider | ||
| bindingToProvider[binding] = provider | ||
| } | ||
| } | ||
|
|
||
| // Create adjacency list for dependency graph | ||
| graph := make(map[string][]string) | ||
| inDegree := make(map[string]int) | ||
|
|
||
| // Initialize inDegree for all providers | ||
| for _, provider := range providers { | ||
| for _, binding := range getBindings(provider) { | ||
| inDegree[binding] = 0 | ||
| } | ||
| } |
There was a problem hiding this comment.
| for _, provider := range providers { | |
| for _, binding := range getBindings(provider) { | |
| providerMap[binding] = provider | |
| bindingToProvider[binding] = provider | |
| } | |
| } | |
| // Create adjacency list for dependency graph | |
| graph := make(map[string][]string) | |
| inDegree := make(map[string]int) | |
| // Initialize inDegree for all providers | |
| for _, provider := range providers { | |
| for _, binding := range getBindings(provider) { | |
| inDegree[binding] = 0 | |
| } | |
| } | |
| // Create adjacency list for dependency graph | |
| graph := make(map[string][]string) | |
| inDegree := make(map[string]int) | |
| for _, provider := range providers { | |
| for _, binding := range getBindings(provider) { | |
| providerMap[binding] = provider | |
| bindingToProvider[binding] = provider | |
| inDegree[binding] = 0 | |
| } | |
| } |
📑 Description
Closes goravel/goravel#667
I'll create another PR for all service provider files and packages.
This pull request introduces several changes to improve service provider handling, error messaging, and dependency management in the application. The most notable updates include adding dependency and binding methods to service providers, implementing a topological sort for service provider dependencies, and replacing the
ArtisanFacadeNotSeterror withConsoleFacadeNotSetfor better clarity. Below are the key changes grouped by theme:Service Provider Enhancements:
Added
Bindings,Dependencies, andProvideFormethods to multiple service providers (cache/service_provider.go,database/service_provider.go,foundation/console/package_make_command_stubs.go). These methods define bindings, dependencies, and provide-for relationships to facilitate dependency management. [1] [2] [3]Implemented a
sortConfiguredServiceProvidersfunction infoundation/application.goto perform a topological sort of service providers based on their dependencies and provide-for relationships. This ensures proper initialization order and detects circular dependencies.Error Handling Improvements:
Replaced the
ArtisanFacadeNotSeterror with a more descriptiveConsoleFacadeNotSeterror across multiple files, improving clarity in error messages related to the artisan facade. [1] [2] [3] [4] [5] [6] [7]Added new error types:
ServiceProviderCyclefor circular dependency detection andConsoleProvidersNotArrayfor invalidapp.providersconfiguration. These errors enhance robustness in service provider management. [1] [2]Dependency Management:
Modified
getConfiguredServiceProvidersinfoundation/application.goto cache sorted service providers and handle invalid configurations gracefully.Enhanced the
bootArtisanandsetTimezonemethods to use the new error types for better error reporting when facades are not initialized. [1] [2]✅ Checks