-
Notifications
You must be signed in to change notification settings - Fork 222
Add map value groups support to dig #381
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.
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
base: master
Are you sure you want to change the base?
Conversation
660e806 to
4541e57
Compare
09b8db4 to
0dfb5f1
Compare
5a24c26 to
23338b1
Compare
|
Any update on this? @JacobOaks could we make this go forward? |
As per Dig issue: uber-go#380 In order to support Fx feature requests uber-go/fx#998 uber-go/fx#1036 We need to be able to drop the restriction, both in terms of options dig.Name and dig.Group and dig.Out struct annotations on `name` and `group` being mutually exclusive. In a future PR, this can then be exploited to populate value group maps where the 'name' tag becomes the key of a map[string][T]
As part of uber-go#380 we allowed names and groups tags/options to co-exist to ultimately support Fx feature request uber-go/fx#998. We now intend to support Map value groups as per uber-go/fx#1036. We will do this in 2 steps. 1. This PR will begin tracking any names passed into value groups with out changing any external facing functionality. 2. a subsequent PR will exploit this structure to support Map value groups.
This revision allows dig to specify value groups of map type.
For example:
```
type Params struct {
dig.In
Things []int `group:"foogroup"`
MapOfThings map[string]int `group:"foogroup"`
}
type Result struct {
dig.Out
Int1 int `name:"foo1" group:"foogroup"`
Int2 int `name:"foo2" group:"foogroup"`
Int3 int `name:"foo3" group:"foogroup"`
}
c.Provide(func() Result {
return Result{Int1: 1, Int2: 2, Int3: 3}
})
c.Invoke(func(p Params) {
})
```
p.Things will be a value group slice as per usual, containing the
elements {1,2,3} in an arbitrary order.
p.MapOfThings will be a key-value pairing of
{"foo1":1, "foo2":2, "foo3":3}.
- Add tests for interface types with map value groups - Add tests for pointer types with map value groups - Add tests for dig.As integration with map value groups - Add CLAUDE.md for development context and future sessions - Add DECORATION_TEST_GAPS.md documenting critical decoration system issues Key findings: - Interface/pointer types work correctly with map value groups - dig.As integration works seamlessly with maps - CRITICAL: Slice decorators are incompatible with named value groups - Identified fundamental design limitation in decoration system The TODO comment at param.go:638 was correct - decoration has serious issues with map value groups due to type mismatch and key information loss. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…alue groups * Problem: Slice decorators fundamentally incompatible with named value groups - Slice decorators strip away key information needed for map reconstruction - Results in silent failures where decorators don't affect consumers * Solution: Add execution-time validation in paramGroupedCollection.Build() - Block slice consumption when slice decorators exist with named values - Provide clear error message directing users to map[string]T decorators - Preserve all existing functionality for valid use cases * Implementation details: - Added groupHasNamedValues() helper to detect named values in groups - Added hasSliceDecorator() helper to detect slice-type decorators - Validation triggers only when both conditions exist (no false positives) - Removed TODO comment at param.go:665 - issue resolved * Comprehensive edge case testing: - Multiple decorators (correctly forbidden) - Map decorators with unnamed values (fails correctly) - Mixed named/unnamed values with slice decorators (blocked) - Empty group decoration (works correctly) - Cross-scope decoration behavior (documented baseline) - Decorator chaining and value addition scenarios * Cross-scope behavior clarification: - Added baseline tests for existing slice decoration behavior - Confirmed map and slice decorators have identical cross-scope semantics - Decorators only see values in their scope context - Child-provided values not visible to parent decorators Key finding: This resolves the critical TODO comment by preventing the problematic pattern rather than trying to handle the impossible case of converting slice decorator output to map format while preserving keys. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
🚀 Ready for Review (Updated)This PR is now complete and ready for review! ✅ What's Included:Core Feature: Map value groups - consume value groups as Development Enhancement: Added robust validation to ensure decoration patterns work correctly with the new map functionality Comprehensive Testing: 527 lines of new tests covering edge cases, cross-scope behavior, and error conditions 🎯 Key Highlights:
🔍 Feature Implementation:The PR successfully implements map value groups as requested in issue #380, with comprehensive validation to ensure slice and map decoration patterns work correctly together. During development, we ensured that decoration behavior remains consistent and provides clear guidance when incompatible patterns are attempted. 📋 Testing Status:
@JacobOaks This addresses issue #380 and is ready for your review! Let me know if you have any questions or need clarification on any aspect of the implementation. The PR implements map value groups as discussed, with robust validation and comprehensive testing to ensure production quality. |
The tests previously suggested that slice decorators should not execute when used with named value groups. However, this is not accurate given the current architecture. **Actual Behavior:** - Slice decorators DO execute when building parameters - Their results are blocked during consumption validation - Proper error is returned preventing invalid usage **Why This is Correct:** - Decorators execute during parameter building phase - Validation happens during consumption phase - Preventing execution would require major architectural changes - Current behavior provides clear error messages to users **Changes:** - Updated test comments to reflect actual behavior - Changed log messages to clarify that results are blocked - Removed incorrect assertions that decorators shouldn't run This clarifies the intended behavior: decorators run but invalid consumption patterns are properly blocked with clear error messages. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
This test verifies that map value group decoration chains properly across module boundaries, demonstrating the correct behavior for named value groups as implemented in uber-go/dig#381.
Soft value groups only contain values from already-executed constructors, without triggering additional constructor execution. This commit adds comprehensive testing to ensure this behavior works correctly with the new map value group consumption feature. **Tests Added:** - Soft map providers not called when only consuming soft groups - Soft maps correctly contain values from executed constructors - Soft maps exclude values from non-executed constructors - Soft map consumption works alongside regular slice consumption - Soft map and regular map consumption produce equivalent results **Key Findings:** - Soft value groups work correctly with map consumption - Same execution semantics as slice consumption (only executed providers) - Map and slice soft consumption behavior is consistent - No additional implementation needed - existing soft group logic handles maps **Coverage:** This completes testing of soft groups with map value groups, ensuring the feature works reliably across all consumption patterns. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Add comprehensive changelog entry for map value groups functionality - Remove CLAUDE.md and DECORATION_TEST_GAPS.md development files 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Add Map Value Groups section to doc.go with complete usage examples - Document simultaneous name+group support and map consumption patterns - Include decorator compatibility section with slice decorator limitations - Explain error messages and provide correct map decorator examples - Clarify this is not a breaking change since name+group was previously impossible 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Summary
This PR implements map value groups for the dig dependency injection framework, allowing value groups to be consumed as
map[string]Tin addition to the existing[]Tslice format. This addresses issue #380 and related Fx dependent feature uber-go/fx#998, uber-go/fx#1036 which is co-implemented uber-go/fx#1279.Key Features
1. Map Value Group Consumption
2. Simultaneous Name + Group Support
dig.Name()anddig.Group()map[string]T3. Robust Decoration Validation
Implementation Details
Core Changes
Validation & Error Handling
map[string]Tenforced (validated at param creation)Comprehensive Test Coverage
Breaking Changes
None - This is a purely additive feature that maintains full backward compatibility.
Decoration System Design Decision
Pattern Compatibility & Non-Breaking Nature
During development, we identified that slice decorators (
func([]T) []T) are fundamentally incompatible with named value groups because:map[string]Twithout keysImportant: This validation is not a breaking change because:
dig.Name()anddig.Group()were mutually exclusiveDesign Rationale
We chose to block incompatible patterns rather than allow silent failures because:
Decorator Execution Clarification
Current Behavior: Slice decorators do execute when used with named groups, but their results are blocked during validation. This is the correct behavior given the current architecture:
The tests document this behavior:
Solution: Clear Validation
Added execution-time validation that prevents this pattern and guides users:
Correct Pattern:
Existing Patterns Unchanged:
Soft Value Groups Integration
New Test Coverage
Added comprehensive testing for soft value groups with map consumption:
Key Findings
Edge Cases Handled
Decoration System
Type System
Soft Groups
Testing Strategy
Functional Testing
Compatibility Testing
Performance Impact
Minimal - Map construction adds negligible overhead only when consuming groups as maps.
Documentation
Related Issues
🤖 Generated with Claude Code