Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support multiple ResultTags annotations #1225

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Support multiple ResultTags annotations #1225

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 91 additions & 22 deletions annotated.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,15 +377,12 @@
// If the tag is invalid and has mismatched quotation for example,
// (`tag_name:"tag_value') , this will return an error.
func (rt resultTagsAnnotation) apply(ann *annotated) error {
if len(ann.ResultTags) > 0 {
return errors.New("cannot apply more than one line of ResultTags")
}
for _, tag := range rt.tags {
if err := verifyAnnotateTag(tag); err != nil {
return err
}
}
ann.ResultTags = rt.tags
ann.ResultTags = append(ann.ResultTags, rt.tags)
return nil
}

Expand Down Expand Up @@ -426,12 +423,16 @@
// if there's no Out struct among the return types, there was no As annotation applied
// just replace original result types with an Out struct and apply tags
var (
newOut outStructInfo
existingOuts []reflect.Type
newOut outStructInfo
existingOuts []reflect.Type
existingOutsMapping = make(map[int][]reflect.Type)
)

newOut.Fields = []reflect.StructField{_outAnnotationField}
newOut.Offsets = []int{}
// to prevent duplicate applying of the same tag to the same type, it is kept in the following format
// {"foo": {"fmt.Stringer": struct{}{}, "myStringer": struct{}{}}, "bar": {"fmt.Stringer": struct{}{}}}
retaggedTypeMap := map[string]map[string]struct{}{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we validate whether or not a given set of ResultTags annotations will provide conflicting types to the container before we try to build out an actual constructor? (in apply() method) If we validate it beforehand, we won't have to worry about this at all here.


for i, t := range types {
if !isOut(t) {
Expand All @@ -448,26 +449,92 @@
newOut.Fields = append(newOut.Fields, field)
continue
}
// this must be from an As annotation
// apply the tags to the existing type
// this must be from an As annotation or a ResultTags annotation
// apply the tags to the existing type if it comes from an As annotation
taggedFields := make([]reflect.StructField, t.NumField())
taggedFields[0] = _outAnnotationField
// apply the tags to the existing type after replication the existing type if it comes from a ResultTags annotation
originalTaggedFields := make([]reflect.StructField, t.NumField())
originalTaggedFields[0] = _outAnnotationField

var newlyTagged bool
var hasBeenTagged bool
for j, tag := range rt.tags {
if j+1 < t.NumField() {
field := t.Field(j + 1)
// if the field has already been tagged by ResultTags annotation, avoid overwriting it
_, taggedName := field.Tag.Lookup("name")
_, taggedGroup := field.Tag.Lookup("group")
if taggedName || taggedGroup {
originalTaggedFields[j+1] = reflect.StructField{
Name: field.Name,
Type: field.Type,
Tag: field.Tag,
}
hasBeenTagged = true
}

structTag := reflect.StructTag(tag)
if typeNames, ok := retaggedTypeMap[structTag.Get("name")]; ok {
if _, ok := typeNames[field.Type.String()]; ok {
continue
}
}
if typeNames, ok := retaggedTypeMap[structTag.Get("group")]; ok {
if _, ok := typeNames[field.Type.String()]; ok {
continue

Check warning on line 485 in annotated.go

View check run for this annotation

Codecov / codecov/patch

annotated.go#L484-L485 

Added lines #L484 - L485 were not covered by tests
}
}
if n, ok := structTag.Lookup("name"); ok {
typeNames, ok := retaggedTypeMap[n]
if !ok {
typeNames = make(map[string]struct{})
retaggedTypeMap[n] = typeNames
}
typeNames[field.Type.String()] = struct{}{}
}
if g, ok := structTag.Lookup("group"); ok {
typeNames, ok := retaggedTypeMap[g]
if !ok {
typeNames = make(map[string]struct{})
retaggedTypeMap[g] = typeNames

Check warning on line 500 in annotated.go

View check run for this annotation

Codecov / codecov/patch

annotated.go#L497-L500 

Added lines #L497 - L500 were not covered by tests
}
typeNames[field.Type.String()] = struct{}{}

Check warning on line 502 in annotated.go

View check run for this annotation

Codecov / codecov/patch

annotated.go#L502 

Added line #L502 was not covered by tests
}

if hasBeenTagged && !taggedName && !taggedGroup {
// if other fields are already tagged and this field is untagged, apply the new tag
originalTaggedFields[j+1] = reflect.StructField{
Name: field.Name,
Type: field.Type,
Tag: structTag,

Check warning on line 510 in annotated.go

View check run for this annotation

Codecov / codecov/patch

annotated.go#L507-L510 

Added lines #L507 - L510 were not covered by tests
}
continue

Check warning on line 512 in annotated.go

View check run for this annotation

Codecov / codecov/patch

annotated.go#L512 

Added line #L512 was not covered by tests
}
taggedFields[j+1] = reflect.StructField{
Name: field.Name,
Type: field.Type,
Tag: reflect.StructTag(tag),
Tag: structTag,
}
newlyTagged = true
}
}
existingOuts = append(existingOuts, reflect.StructOf(taggedFields))
currentTypeExistingOuts := make([]reflect.Type, 0, 2)
if hasBeenTagged {
currentTypeExistingOuts = append(currentTypeExistingOuts, reflect.StructOf(originalTaggedFields))
}
if newlyTagged {
currentTypeExistingOuts = append(currentTypeExistingOuts, reflect.StructOf(taggedFields))
}
existingOutsMapping[i] = currentTypeExistingOuts
existingOuts = append(existingOuts, currentTypeExistingOuts...)
}

resType := reflect.StructOf(newOut.Fields)

outTypes := []reflect.Type{resType}
var outTypes []reflect.Type
if len(newOut.Fields) > 1 {
outTypes = append(outTypes, resType)
}
// append existing outs back to outTypes
outTypes = append(outTypes, existingOuts...)
if hasError {
Expand All @@ -479,9 +546,10 @@
outErr error
outResults []reflect.Value
)
outResults = append(outResults, reflect.New(resType).Elem())

tIdx := 0
if len(newOut.Fields) > 1 {
outResults = append(outResults, reflect.New(resType).Elem())
}
existingOutResults := make([]reflect.Value, 0, len(existingOuts))
for i, r := range results {
if i == len(results)-1 && hasError {
// If hasError and this is the last item,
Expand All @@ -501,21 +569,22 @@
// to prevent panic from setting fx.Out to
// a value.
outResults[0].Field(fieldIdx).Set(r)
continue
}
continue
}
if isOut(r.Type()) {
tIdx++
if tIdx < len(outTypes) {
newResult := reflect.New(outTypes[tIdx]).Elem()
for j := 1; j < outTypes[tIdx].NumField(); j++ {
for _, existingOuts := range existingOutsMapping[i] {
newResult := reflect.New(existingOuts).Elem()
for j := 1; j < existingOuts.NumField(); j++ {
newResult.Field(j).Set(r.Field(j))
}
outResults = append(outResults, newResult)
existingOutResults = append(existingOutResults, newResult)
}
}
}

outResults = append(outResults, existingOutResults...)

if hasError {
if outErr != nil {
outResults = append(outResults, reflect.ValueOf(outErr))
Expand Down Expand Up @@ -1528,7 +1597,7 @@
Target interface{}
Annotations []Annotation
ParamTags []string
ResultTags []string
ResultTags [][]string
As [][]asType
From []reflect.Type
FuncPtr uintptr
Expand Down
58 changes: 54 additions & 4 deletions annotated_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1431,7 +1431,7 @@ func TestAnnotate(t *testing.T) {
t.Run("specify two ResultTags", func(t *testing.T) {
t.Parallel()

app := NewForTest(t,
app := fxtest.New(t,
fx.Provide(
// This should just leave newA as it is.
fx.Annotate(
Expand All @@ -1440,12 +1440,62 @@ func TestAnnotate(t *testing.T) {
fx.ResultTags(`name:"AA"`),
),
),
fx.Invoke(newB),
fx.Invoke(
fx.Annotate(func(a, aa *a) (*b, *b) {
return newB(a), newB(aa)
}, fx.ParamTags(`name:"A"`, `name:"AA"`))),
)

err := app.Err()
require.Error(t, err)
assert.Contains(t, err.Error(), "encountered error while applying annotation using fx.Annotate to go.uber.org/fx_test.TestAnnotate.func1(): cannot apply more than one line of ResultTags")
require.NoError(t, err)
defer app.RequireStart().RequireStop()
})

t.Run("specify two ResultTags containing multiple tags", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a test case for using multiple As annotations and ResultTags annotations. For example, something like the following:

fx.Provide(
    fx.Annotate(
        func() *bytes.Buffer { return bytes.NewBuffer() },
        fx.As(new(io.Reader)),
        fx.As(new(io.Writer)),
        fx.ResultTags(`name:"foo"`),
        fx.ResultTags(`name:"bar"`),
    )
)

In fact, what's the expected behavior in this case?
I think the current implementation will produce 4 results from the above code, right? But does that make sense? Does "using multiple ResultTags annotation" only make sense when there is 0 or 1 As annotation? If so, we should probably add additional logic to validate that to apply() method of resultTagsAnnotation.

t.Parallel()

app := fxtest.New(t,
fx.Provide(
fx.Annotate(
func() (*a, *b) {
return newA(), newB(&a{})
},
fx.ResultTags(`name:"A"`, `name:"B"`),
fx.ResultTags(`name:"AA"`, `name:"BB"`),
),
),
fx.Invoke(
fx.Annotate(func(a, aa *a, b, bb *b) (*b, *b, *c, *c) {
return newB(a), newB(aa), newC(b), newC(b)
}, fx.ParamTags(`name:"A"`, `name:"AA"`, `name:"B"`, `name:"BB"`))),
)

err := app.Err()
require.NoError(t, err)
defer app.RequireStart().RequireStop()
})

t.Run("specify Three ResultTags", func(t *testing.T) {
t.Parallel()

app := fxtest.New(t,
fx.Provide(
fx.Annotate(
newA,
fx.ResultTags(`name:"A"`),
fx.ResultTags(`name:"AA"`),
fx.ResultTags(`name:"AAA"`),
),
),
fx.Invoke(
fx.Annotate(func(a, aa, aaa *a) (*b, *b, *b) {
return newB(a), newB(aa), newB(aaa)
}, fx.ParamTags(`name:"A"`, `name:"AA"`, `name:"AAA"`))),
)

err := app.Err()
require.NoError(t, err)
defer app.RequireStart().RequireStop()
})

t.Run("annotate with a non-nil error", func(t *testing.T) {
Expand Down
2 changes: 1 addition & 1 deletion app_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ func TestNewApp(t *testing.T) {
// cannot provide fx_test.t1[name="foo"] from [0].Field0:
// already provided by "reflect".makeFuncStub (/.../reflect/asm_amd64.s:30)
assert.Contains(t, err.Error(), `fx.Provide(fx.Annotate(`)
assert.Contains(t, err.Error(), `fx.ResultTags(["name:\"foo\""])`)
assert.Contains(t, err.Error(), `fx.ResultTags([["name:\"foo\""]])`)
assert.Contains(t, err.Error(), "already provided")
})

Expand Down