feat(depinject): support ignored fields in input structs #22409
Conversation
kocubinski
requested review from
aaronc,
julienrbrt and
a team
as code owners
|
Warning Rate limit exceeded@kocubinski has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 37 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a new test function, Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant Depinject
participant StructArgs
Test->>Depinject: Call Inject with cfg
Depinject->>StructArgs: Check fields in cfg
StructArgs->>StructArgs: Skip IgnoredField (tag: ignored)
StructArgs-->>Depinject: Return processed fields
Depinject-->>Test: Return success (no error)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
depinject/struct_args.go (1)
15-19: Documentation could be more descriptive for theignoredtag.While the documentation is well-formatted, consider expanding the description of the
ignoredtag to explain its purpose and use cases.optional if set to true, the dependency is optional and will be set to its default value if not found, rather than causing an error - ignored if set to any value the dependency will be ignored + ignored if set to any value, the field will be completely skipped + during dependency injection, useful for fields that should + not participate in the injection processdepinject/binding_test.go (1)
304-316: Add test cases for edge cases and error scenariosThe current test only covers the basic scenario. Consider adding test cases for:
- Multiple ignored fields in the same struct
- Invalid
ignoredtag values (e.g.,ignored:"invalid")- Interaction with other dependency injection features (e.g., optional fields)
Example additional test case:
func TestIgnoredFieldEdgeCases(t *testing.T) { t.Parallel() t.Run("multiple ignored fields", func(t *testing.T) { cfg := struct { depinject.In Field1 Duck `ignored:"true"` Field2 Duck `ignored:"true"` Field3 Duck }{} err := depinject.Inject(depinject.Provide(ProvideMallard), &cfg) require.NoError(t, err) require.Nil(t, cfg.Field1) require.Nil(t, cfg.Field2) require.NotNil(t, cfg.Field3) }) t.Run("invalid ignored tag", func(t *testing.T) { cfg := struct { depinject.In Field Duck `ignored:"invalid"` }{} err := depinject.Inject(depinject.Provide(ProvideMallard), &cfg) require.Error(t, err) }) }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
depinject/binding_test.go(1 hunks)depinject/struct_args.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
depinject/binding_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
depinject/struct_args.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (2)
depinject/struct_args.go (1)
118-122: Implementation looks good!
The ignored field handling is implemented correctly and follows the established patterns in the codebase.
depinject/binding_test.go (1)
304-305: LGTM: Test function follows best practices
The test function follows Go testing best practices with proper naming and parallel execution.
| _, found := f.Tag.Lookup("ignored") | ||
| if found { | ||
| continue | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting the ignored tag check into a helper function.
The ignored tag check is duplicated between structArgsInTypes and buildIn. Consider extracting it into a helper function to follow the DRY principle.
+func isIgnoredField(f reflect.StructField) bool {
+ _, found := f.Tag.Lookup("ignored")
+ return found
+}
func buildIn(typ reflect.Type, values []reflect.Value) (reflect.Value, int, error) {
// ...
- _, found := f.Tag.Lookup("ignored")
- if found {
+ if isIgnoredField(f) {
continue
}
// ...
}
func structArgsInTypes(typ reflect.Type) ([]providerInput, error) {
// ...
- _, found := f.Tag.Lookup("ignored")
- if found {
+ if isIgnoredField(f) {
continue
}
// ...
}Committable suggestion skipped: line range outside the PR's diff.
| // optional if set to true, the dependency is optional and will | ||
| // be set to its default value if not found, rather than causing | ||
| // an error | ||
| // | ||
| // ignored if set to any value the dependency will be ignored |
There was a problem hiding this comment.
💡 Codebase verification
Test coverage exists but could be more comprehensive
The ignored tag functionality is implemented and has basic test coverage in binding_test.go, but could benefit from additional test cases:
- Current test in
TestIgnoredFieldonly verifies basic functionality with a single ignored field - Missing test cases for:
- Multiple ignored fields
- Interaction between ignored and optional fields
- Edge cases with unexported ignored fields
🔗 Analysis chain
Verify test coverage for ignored fields.
Let's verify that there are adequate test cases for the new ignored field functionality.
Also applies to: 118-122, 180-184
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases related to ignored fields
echo "Searching for test cases covering ignored fields..."
rg -l "ignored.*tag" --type go "depinject/*_test.go"
Length of output: 227
Script:
#!/bin/bash
# Let's first find the test files in the repository
fd "_test.go$" --type f
# Then search for any mentions of "ignored" in test files
rg "ignored" --type go -C 3 ".*_test\.go$"
# Also search for struct tag related tests
rg "struct.*tag" --type go -C 3 ".*_test\.go$"
# Let's also check the implementation of ignored field handling
ast-grep --pattern 'if $_.Tag.Get("ignored") $$$'
Length of output: 27881
Script:
#!/bin/bash
# Let's search for the implementation of ignored tag handling in struct_args.go
rg -C 5 "ignored.*tag" depinject/struct_args.go
# Let's also check for any tests in the depinject directory
rg -C 5 "ignored" "depinject/*_test.go"
# Let's look for the struct field processing implementation
ast-grep --pattern 'func $_(field reflect.StructField, $$$) {
$$$
}'
Length of output: 222
Script:
#!/bin/bash
# Let's check the actual implementation in struct_args.go
cat depinject/struct_args.go
# Let's find test files in depinject directory with correct path
fd "_test.go$" depinject/
# Let's also check for any struct tag processing in the codebase
ast-grep --pattern 'type $_ struct {
$$$
`$$$"ignored"$$$`
$$$
}'
Length of output: 5560
Script:
#!/bin/bash
# Let's check the test files for any test cases involving ignored tag
rg -C 5 "ignored" depinject/container_test.go depinject/binding_test.go
# Also check for any struct definitions with tags in tests
ast-grep --pattern 'type $_ struct {
$_ $_ `$_`
}' depinject/container_test.go depinject/binding_test.go
Length of output: 728
| cfg := struct { | ||
| depinject.In | ||
| TheDuck Duck | ||
| IgnoredField bool `ignored:"true"` | ||
| DuckAgain Duck | ||
| DuckAgainAgain Duck | ||
| }{} | ||
|
|
||
| err := depinject.Inject(depinject.Provide(ProvideMallard), &cfg) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage with additional assertions
While the test verifies successful injection, it should also validate:
- The ignored field remains unmodified
- The Duck fields are properly injected with Mallard instances
Consider enhancing the test with these assertions:
func TestIgnoredField(t *testing.T) {
t.Parallel()
cfg := struct {
depinject.In
TheDuck Duck
IgnoredField bool `ignored:"true"`
DuckAgain Duck
DuckAgainAgain Duck
- }{}
+ }{
+ IgnoredField: true, // Set initial value
+ }
err := depinject.Inject(depinject.Provide(ProvideMallard), &cfg)
require.NoError(t, err)
+
+ // Verify ignored field remains unchanged
+ require.True(t, cfg.IgnoredField)
+
+ // Verify all Duck fields are properly injected
+ require.IsType(t, Mallard{}, cfg.TheDuck)
+ require.IsType(t, Mallard{}, cfg.DuckAgain)
+ require.IsType(t, Mallard{}, cfg.DuckAgainAgain)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cfg := struct { | |
| depinject.In | |
| TheDuck Duck | |
| IgnoredField bool `ignored:"true"` | |
| DuckAgain Duck | |
| DuckAgainAgain Duck | |
| }{} | |
| err := depinject.Inject(depinject.Provide(ProvideMallard), &cfg) | |
| require.NoError(t, err) | |
| cfg := struct { | |
| depinject.In | |
| TheDuck Duck | |
| IgnoredField bool `ignored:"true"` | |
| DuckAgain Duck | |
| DuckAgainAgain Duck | |
| }{ | |
| IgnoredField: true, // Set initial value | |
| } | |
| err := depinject.Inject(depinject.Provide(ProvideMallard), &cfg) | |
| require.NoError(t, err) | |
| // Verify ignored field remains unchanged | |
| require.True(t, cfg.IgnoredField) | |
| // Verify all Duck fields are properly injected | |
| require.IsType(t, Mallard{}, cfg.TheDuck) | |
| require.IsType(t, Mallard{}, cfg.DuckAgain) | |
| require.IsType(t, Mallard{}, cfg.DuckAgainAgain) |
private fields on input structs are not supported. trying to use one will result in an error from depinject with where would you like the documentation? right now it's on the godoc for the |
|
for my use case I'd be fine silently ignoring private fields, but that could cause some confusion if someone's attempting to use one. |
|
Can you explain what the purpose of these ignored fields is? |
A note in the README would be great yeah |
|
EDIT: as discussed on the call, maybe refactoring inputs of autocli is better than adding this. |
Description
Support ignore flags in input structs. This will clean up an incoming PR around AutoCLI. Seems like a reasonable feature to me.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
ignoredtag.Tests
Documentation