|
| 1 | +# Decoration Test Gaps for Map Value Groups |
| 2 | + |
| 3 | +## Critical Gap: Your TODO Comment |
| 4 | + |
| 5 | +**Location**: `param.go:638` |
| 6 | +```go |
| 7 | +// Check if we have decorated values |
| 8 | +// qjeremy(how to handle this with maps?) |
| 9 | +if decoratedItems, ok := pt.getDecoratedValues(c); ok { |
| 10 | + return decoratedItems, nil |
| 11 | +} |
| 12 | +``` |
| 13 | + |
| 14 | +## The Problem |
| 15 | + |
| 16 | +The decoration system was originally designed for slice value groups. When map value groups were added, the decoration interaction may not have been fully considered. |
| 17 | + |
| 18 | +### Key Concerns: |
| 19 | + |
| 20 | +1. **Type Mismatch**: |
| 21 | + - `getDecoratedValues()` calls `c.getDecoratedValueGroup(pt.Group, pt.Type)` |
| 22 | + - `pt.Type` could be `[]T` (slice) or `map[string]T` (map) |
| 23 | + - Decorators might return slice types but consumers expect map types |
| 24 | + |
| 25 | +2. **Inconsistent Return Types**: |
| 26 | + ```go |
| 27 | + // Scenario that may be broken: |
| 28 | + c.Provide(func() int{return 1}, dig.Name("a"), dig.Group("nums")) |
| 29 | + c.Provide(func() int{return 2}, dig.Name("b"), dig.Group("nums")) |
| 30 | + |
| 31 | + // Decorator expects and returns slice |
| 32 | + c.Decorate(func(nums []int) []int { |
| 33 | + return append(nums, 999) |
| 34 | + }) |
| 35 | + |
| 36 | + // Consumer expects map - what happens? |
| 37 | + type In struct { |
| 38 | + dig.In |
| 39 | + NumMap map[string]int `group:"nums"` // May get slice instead? |
| 40 | + } |
| 41 | + ``` |
| 42 | + |
| 43 | +3. **Missing Test Coverage**: |
| 44 | + - No tests combining decoration + map consumption |
| 45 | + - Unclear behavior when decorator returns slice but consumer wants map |
| 46 | + - No validation that map structure is preserved through decoration |
| 47 | + |
| 48 | +## Potential Issues |
| 49 | + |
| 50 | +### Issue 1: Wrong Return Type |
| 51 | +Decoration may return `[]int{1, 2, 999}` when consumer expects `map[string]int{"a":1, "b":2}`. |
| 52 | + |
| 53 | +### Issue 2: Lost Key Information |
| 54 | +When decorating, the map keys (names) might be lost since decorators work with slices. |
| 55 | + |
| 56 | +### Issue 3: Silent Failures |
| 57 | +The system might fail silently or return unexpected data structures. |
| 58 | + |
| 59 | +## Test Scenarios Needed |
| 60 | + |
| 61 | +1. **Basic decoration + map consumption**: |
| 62 | + ```go |
| 63 | + // Provide with names |
| 64 | + c.Provide(..., dig.Name("key1"), dig.Group("test")) |
| 65 | + |
| 66 | + // Decorate (expects slice) |
| 67 | + c.Decorate(func(items []T) []T { return modified }) |
| 68 | + |
| 69 | + // Consume as map |
| 70 | + map[string]T `group:"test"` |
| 71 | + ``` |
| 72 | + |
| 73 | +2. **Map-aware decoration**: |
| 74 | + ```go |
| 75 | + // Decorator that works with maps |
| 76 | + c.Decorate(func(items map[string]T) map[string]T { return modified }) |
| 77 | + ``` |
| 78 | + |
| 79 | +3. **Mixed consumption**: |
| 80 | + ```go |
| 81 | + // One consumer wants slice, another wants map |
| 82 | + []T `group:"test"` |
| 83 | + map[string]T `group:"test"` |
| 84 | + ``` |
| 85 | + |
| 86 | +## Existing Map Decoration Test |
| 87 | + |
| 88 | +There IS one existing test in `decorate_test.go:455` - "decorate with map value groups". However, this test: |
| 89 | +- Uses `map[string]string` for both input AND output of decorator |
| 90 | +- May not test the problematic slice→map conversion path |
| 91 | +- Doesn't test mixed slice/map consumption scenarios |
| 92 | + |
| 93 | +## CRITICAL DISCOVERY ⚠️ |
| 94 | + |
| 95 | +**ROOT CAUSE IDENTIFIED**: Slice decorators are fundamentally incompatible with named value groups. |
| 96 | + |
| 97 | +### What Works ✅ |
| 98 | +- **Unnamed groups** + **slice decorators** + **slice consumers** |
| 99 | +- **Named groups** + **map decorators** + **map consumers** (existing test: `decorate_test.go:455`) |
| 100 | + |
| 101 | +### What's Broken ❌ |
| 102 | +- **Named groups** + **slice decorators** + **any consumers** |
| 103 | + |
| 104 | +### The Problem |
| 105 | +When you provide values with names (`dig.Name()`) and then use a slice decorator (`func([]T) []T`), the decorator strips away the key information needed to reconstruct maps. The slice decorator only sees `[value1, value2]` without knowing which value corresponds to which name. |
| 106 | + |
| 107 | +### Evidence |
| 108 | +```go |
| 109 | +// This pattern is BROKEN: |
| 110 | +c.Provide(func() int{return 1}, dig.Name("a"), dig.Group("nums")) |
| 111 | +c.Provide(func() int{return 2}, dig.Name("b"), dig.Group("nums")) |
| 112 | + |
| 113 | +c.Decorate(func(nums []int) []int { |
| 114 | + // This sees [1, 2] but has NO KNOWLEDGE of names "a", "b" |
| 115 | + return modified_slice |
| 116 | +}) |
| 117 | + |
| 118 | +// Consumers get UNDECORATED values: |
| 119 | +map[string]int `group:"nums"` // Gets original: {"a":1, "b":2} |
| 120 | +[]int `group:"nums"` // Gets original: [1, 2] |
| 121 | +``` |
| 122 | + |
| 123 | +### Why This Happens |
| 124 | +1. **Storage**: Decorated values stored under slice type `[]T` |
| 125 | +2. **Lookup**: Map consumers look for type `map[string]T` |
| 126 | +3. **Mismatch**: No decorated values found, falls back to original providers |
| 127 | +4. **Result**: Both slice AND map consumers get undecorated values |
| 128 | + |
| 129 | +## Resolution Priority |
| 130 | + |
| 131 | +**CRITICAL** - This represents a fundamental design limitation where two major features (named value groups + slice decorators) are mutually incompatible. |
| 132 | + |
| 133 | +## Recommended Actions |
| 134 | + |
| 135 | +1. **Document the limitation**: Slice decorators don't work with named value groups |
| 136 | +2. **Update TODO comment**: Explain the fundamental incompatibility |
| 137 | +3. **Consider design options**: |
| 138 | + - Forbid slice decorators when names are present (breaking change) |
| 139 | + - Support map decorators as the primary pattern for named groups |
| 140 | + - Implement key-preserving slice decoration (complex) |
| 141 | +4. **Update tests**: Add tests that demonstrate the limitation and expected behavior |
0 commit comments