Repository: uber-go/dig (go)

Issue:
Result of multiple decorations of the same type depends on invocation order
Consider: 

```go
type A struct {
	name string
}

c := dig.New()
child := c.Scope("child")
c.Provide(func() *A { return &A{name: "A"} })

c.Decorate(func(a *A) *A { return &A{name: a.name + "'"} })
child.Decorate(func(a *A) *A { return &A{name: a.name + "'"} })

c.Invoke(func(a *A) {
    fmt.Println(a.name)
})

child.Invoke(func(a *A) {
    fmt.Println(a.name)
})
```

This prints `A''` and `A'` respectively to the console.

Now let's say the invokes were reversed in order:
```go
child.Invoke(func(a *A) {
    fmt.Println(a.name)
})

c.Invoke(func(a *A) {
    fmt.Println(a.name)
})
```
That ends up printing `A'` and `A'` on the screen.

The order of invocation should not alter the committed value - this is likely a bug in the paramSingle.Build() logic where it's being too "lazy" with the lookups for decorated values.



Pull request description:
Fix Invoke order changing execution result in multiple decorations of same type
This fixes the multiple decoration logic for paramSingle where there
are more than one scopes that contain decorators for a type. The lookup
logic only looked up one-level for the decorator. For example, if Scope S
has a decorator and its parent Sp also has a decorator for the same type, it never
executed Sp's decorator.

This caused the order of the Scopes in which a function that used such type
was Invoked from to change the execution result.

This PR addresses the issue by more diligently looking up the types by
traversing for more decorators until there is no more decorators found
in the path to the root Scope.

Also added several tests that cover this case.

Fixes #321 and https://github.com/uber-go/fx/issues/841
Internal Ref: GO-1228

Patch:
diff --git a/decorate.go b/decorate.go
index 2d96b75..2a57b7d 100644
--- a/decorate.go
+++ b/decorate.go
@@ -98,7 +98,7 @@ func (n *decoratorNode) Call(s containerStore) error {
 		}
 	}
 
-	args, err := n.params.BuildList(n.s, true /* decorating */)
+	args, err := n.params.BuildList(n.s, s == n.s /* decorating */)
 	if err != nil {
 		return errArgumentsFailed{
 			Func:   n.location,
diff --git a/param.go b/param.go
index 6327a62..a97eacc 100644
--- a/param.go
+++ b/param.go
@@ -221,12 +221,19 @@ func (ps paramSingle) getValue(c containerStore) (reflect.Value, bool) {
 // current scope, if there are any. If there are multiple Scopes that decorates
 // this parameter, the closest one to the Scope that invoked this will be used.
 // If there are no decorators associated with this parameter, _noValue is returned.
-func (ps paramSingle) buildWithDecorators(c containerStore) (v reflect.Value, found bool, err error) {
+func (ps paramSingle) buildWithDecorators(c containerStore, decorating bool) (v reflect.Value, found bool, err error) {
 	var (
 		decorators      []decorator
 		decoratingScope containerStore
 	)
-	for _, s := range c.storesToRoot() {
+	stores := c.storesToRoot()
+	// If we are already in a decorating stack,
+	// skip the current Scope to avoid infinite
+	// recursion.
+	if decorating {
+		stores = stores[1:]
+	}
+	for _, s := range stores {
 		if decorators = s.getValueDecorators(ps.Name, ps.Type); len(decorators) > 0 {
 			decoratingScope = s
 			break
@@ -237,7 +244,7 @@ func (ps paramSingle) buildWithDecorators(c containerStore) (v reflect.Value, fo
 	}
 	found = true
 	for _, d := range decorators {
-		err := d.Call(c)
+		err := d.Call(decoratingScope)
 		if err == nil {
 			continue
 		}
@@ -256,11 +263,9 @@ func (ps paramSingle) buildWithDecorators(c containerStore) (v reflect.Value, fo
 }
 
 func (ps paramSingle) Build(c containerStore, decorating bool) (reflect.Value, error) {
-	if !decorating {
-		v, found, err := ps.buildWithDecorators(c)
-		if found {
-			return v, err
-		}
+	v, found, err := ps.buildWithDecorators(c, decorating)
+	if found {
+		return v, err
 	}
 
 	// Check whether the value is a decorated value first.
@@ -314,7 +319,7 @@ func (ps paramSingle) Build(c containerStore, decorating bool) (reflect.Value, e
 
 	// If we get here, it's impossible for the value to be absent from the
 	// container.
-	v, _ := ps.getValue(c)
+	v, _ = ps.getValue(c)
 	return v, nil
 }
 


Test patch:
diff --git a/decorate_test.go b/decorate_test.go
index 8c0d5d9..b60ec99 100644
--- a/decorate_test.go
+++ b/decorate_test.go
@@ -153,7 +153,6 @@ func TestDecorateSuccess(t *testing.T) {
 
 		c.RequireDecorate(func(a *A) *A { return &A{name: a.name + "'"} })
 		child.RequireDecorate(func(a *A) *A { return &A{name: a.name + "'"} })
-
 		c.RequireInvoke(func(a *A) {
 			assert.Equal(t, "A'", a.name, "expected decorated name in parent")
 		})
@@ -586,6 +585,135 @@ func TestDecorateFailure(t *testing.T) {
 	})
 }
 
+func TestMultipleDecorates(t *testing.T) {
+	t.Run("decorate same type from parent and child, invoke child first", func(t *testing.T) {
+		t.Parallel()
+		type A struct{ value int }
+		root := digtest.New(t)
+		child := root.Scope("child")
+
+		decorator := func(a A) A {
+			return A{value: a.value + 1}
+		}
+		root.RequireProvide(func() A { return A{value: 0} })
+		root.RequireDecorate(decorator)
+		child.RequireDecorate(decorator)
+
+		child.RequireInvoke(func(a A) {
+			assert.Equal(t, 2, a.value)
+		})
+		root.RequireInvoke(func(a A) {
+			assert.Equal(t, 1, a.value)
+		})
+	})
+
+	t.Run("decorate same type from parent and child, invoke parent first", func(t *testing.T) {
+		t.Parallel()
+		type A struct{ value int }
+		root := digtest.New(t)
+		child := root.Scope("child")
+		decorator := func(a A) A {
+			return A{value: a.value + 1}
+		}
+		root.RequireProvide(func() A { return A{value: 0} })
+		root.RequireDecorate(decorator)
+		child.RequireDecorate(decorator)
+
+		root.RequireInvoke(func(a A) {
+			assert.Equal(t, 1, a.value)
+		})
+		child.RequireInvoke(func(a A) {
+			assert.Equal(t, 2, a.value)
+		})
+	})
+
+	t.Run("decorate same value group from parent and child, invoke child", func(t *testing.T) {
+		t.Parallel()
+		type A struct {
+			dig.In
+
+			Values []int `group:"val"`
+		}
+		type B struct {
+			dig.Out
+
+			Values []int `group:"val"`
+		}
+
+		root := digtest.New(t)
+		child := root.Scope("child")
+		decorator := func(a A) B {
+			var newV []int
+			for _, v := range a.Values {
+				newV = append(newV, v+1)
+			}
+			return B{Values: newV}
+		}
+
+		// provide {0, 1, 2}
+		root.Provide(func() int { return 0 }, dig.Group("val"))
+		root.Provide(func() int { return 1 }, dig.Group("val"))
+		root.Provide(func() int { return 2 }, dig.Group("val"))
+
+		// decorate +1 to each element in parent
+		root.RequireDecorate(decorator)
+
+		// decorate +1 to each element in child
+		child.RequireDecorate(decorator)
+
+		child.RequireInvoke(func(a A) {
+			assert.ElementsMatch(t, []int{2, 3, 4}, a.Values)
+		})
+
+		root.RequireInvoke(func(a A) {
+			assert.ElementsMatch(t, []int{1, 2, 3}, a.Values)
+		})
+	})
+
+	t.Run("decorate same value group from parent and child, invoke parent", func(t *testing.T) {
+		t.Parallel()
+		type A struct {
+			dig.In
+
+			Values []int `group:"val"`
+		}
+		type B struct {
+			dig.Out
+
+			Values []int `group:"val"`
+		}
+
+		root := digtest.New(t)
+		child := root.Scope("child")
+		decorator := func(a A) B {
+			var newV []int
+			for _, v := range a.Values {
+				newV = append(newV, v+1)
+			}
+			return B{Values: newV}
+		}
+
+		// provide {0, 1, 2}
+		root.Provide(func() int { return 0 }, dig.Group("val"))
+		root.Provide(func() int { return 1 }, dig.Group("val"))
+		root.Provide(func() int { return 2 }, dig.Group("val"))
+
+		// decorate +1 to each element in parent
+		root.RequireDecorate(decorator)
+
+		// decorate +1 to each element in child
+		child.RequireDecorate(decorator)
+
+		root.Invoke(func(a A) {
+			assert.ElementsMatch(t, []int{1, 2, 3}, a.Values)
+		})
+
+		child.Invoke(func(a A) {
+			assert.ElementsMatch(t, []int{2, 3, 4}, a.Values)
+		})
+	})
+}
+
 func TestFillDecorateInfoString(t *testing.T) {
 	t.Parallel()
 
@@ -602,5 +730,4 @@ func TestFillDecorateInfoString(t *testing.T) {
 		assert.NotEqual(t, fmt.Sprint(opt), "FillDecorateInfo(0x0)")
 		assert.Contains(t, fmt.Sprint(opt), "FillDecorateInfo(0x")
 	})
-
 }