do
do copied to clipboard
refactor: including package path in implicit service name
Thanks for this great work! I've used it in my open source project.
During use, I found that registering services with the same name will panic, even if they came from different packages. Here's a minimal repro:
// a/c/a-c.go
package c
import ( /* ... */ )
type MyStruct struct { /* ... */ }
func NewMyStruct(i *do.Injector) (*MyStruct, error) { /* ... */ }
// b/c/b-c.go
package c
import ( /* ... */ )
type MyStruct struct { /* ... */ }
func NewMyStruct(i *do.Injector) (*MyStruct, error) { /* ... */ }
// main.go
package main
import (
ac "test/a/c"
bc "test/b/c"
"github.com/samber/do"
)
func main() {
i := do.New()
do.Provide(i, ac.NewMyStruct)
do.Provide(i, bc.NewMyStruct)
}
// panic: DI: service `*c.MyStruct` has already been declared
The problem occurs because generateServiceName()
didn't create a "global-unique" qualifier for type. This PR refactors generateServiceName()
to create unique qualifiers by prepending package path.
Implementation
This implementation uses reflect.TypeOf()
, which is the same as fmt.Sprintf("%T")
. It basically the same as typeOfT.PkgPath() + "." + typeOfT.Name()
.
For the following code it generates such qualifiers:
// service_test.go
type testStruct struct{}
type testInterface interface{}
type | qualifier |
---|---|
testStruct |
github.com/samber/do.testStruct |
*testStruct |
github.com/samber/do.*testStruct |
testInterface |
github.com/samber/do.*testInterface |
int |
int |
*int |
*int |
Alternatives
typeOfT.Name()
vs. typeOfT.String()
fmt.Sprintf("%T")
uses typeOfT.String()
. But typeOfT.String()
says "The string representation may use shortened package names and is not guaranteed to be unique among types." Therefore it cannot be guaranteed for usage as a type qualifier.
Processing method of pointer indirect star
The current do.*test
looks strange but I haven't thought of a better one yet.
Testing
Tests have all been updated. Maybe it's necessary to create two test packages to test bugs mentioned above, but adding two new files for this is somewhat weird so I didn't do that. Is it necessary?
Compatibility
This doesn't introduce API changes and therefore it should be possible to add into v1. But it introduces internal behavior changes so please consider it carefully.
Downgrading PR to draft due to design defects.
I did some research and found some other problems of this PR.
typeOfT.String()
Do you know in what case
typeOfT.String()
might break?
Currently typeOfT.String()
works well on all types using formats like package.TypeName
(eg. do.Test
). I didn't choose typeOfT.String()
because the documentation clearly states that it is not guaranteed as type qualifers and may change at any time in the future. And there's another point that makes me drop typeOfT.String()
:
typeOfT.Path() + typeOfT.String()
Another consideration is of the format of typeOfT.String()
. Consider *[]do.Test
: it is actually a combination of extra indicator *[]
, package name do
and type name Test
. That means that if we use typeOfT.Path() + typeOfT.String()
, the final result will be something like this:
github.com/samber/do. *[] do .Test
| | | |
| Why indicator here? | Type name
| Package name again?
The package path (including package name)
To solve this problem and resort the final string, we have to add another piece of logic to split typeOfT.String()
into parts. That of course hurts the robustness again.
Unnamed Types
Then I found another serious problem browsing the documentation: the typeOfT.PkgPath()
and typeOfT.Name()
return empty string for unnamed types. *do.Test
and []do.Test
are of course unnamed types. typeOfT.String()
is not affected because it actually uses magic to get qualifiers "implemented in the runtime package", not info in reflect.Type
.
This means that, if we don't handle all reflect.Kind
(Chan
, Func
, Map
, Pointer
, etc.) manually, we won't able to use typeOfT.PkgPath()
and obviously this PR will not achieve the goal.
For example a type like *map[string]*do.Test
, we need to 1. extract typeOfT.Elem()
to indirect pointer; 2. extract typeOfT.Key()
and typeOfT.Elem()
to get type of K/V; 3. extract typeOfT.Elem()
again to finally get its base struct, and then get the package path to qualify it in the global. This is far more than such a lightweight DI can do and should do.
Conclusion
As a temporary solution, we can extract base struct only for maps (uses package path of value type), slices and pointers, then use typeOfT.Path() + typeOfT.String()
. It may produce results like github.com/samber/do.*[]do.Test
but will still have many edge cases, any of which can lead to bugs. I don't know if it's worth doing this any longer...
Codecov Report
Merging #13 (c5d56ed) into master (7123d38) will increase coverage by
0.44%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## master #13 +/- ##
==========================================
+ Coverage 91.16% 91.60% +0.44%
==========================================
Files 6 6
Lines 396 405 +9
==========================================
+ Hits 361 371 +10
+ Misses 26 25 -1
Partials 9 9
Flag | Coverage Δ | |
---|---|---|
unittests | 91.60% <100.00%> (+0.44%) |
:arrow_up: |
Flags with carried forward coverage won't be shown. Click here to find out more.
Impacted Files | Coverage Δ | |
---|---|---|
service.go | 100.00% <100.00%> (+11.11%) |
:arrow_up: |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
Hi @ilharp 👋
I added a commit for supporting array and slice. Map, Func and Chan are still missing.
name = generateServiceName[*[]*[]**int]()
is.Equal("*[]*[]**int", name)
I will try to push it to do@v2 (with this code moved to a dedicated pkg eventually)
I made a dedicated pkg with 95% type coverage -> https://github.com/samber/go-type-to-string