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

feat(depinject): support ignored fields in input structs #22409

Closed
wants to merge 4 commits into from
Closed

feat(depinject): support ignored fields in input structs #22409

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e46696d
feat(depinject): support ignore tag in input structs
kocubinski Nov 1, 2024
cd90eea
clean up
kocubinski Nov 1, 2024
58698d5
moar cleanup
kocubinski Nov 1, 2024
64db97b
changelog entry
kocubinski Nov 1, 2024
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
2 changes: 2 additions & 0 deletions depinject/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ Each entry must include the Github issue reference in the following format:

## [Unreleased]

* [#22409](https://github.com/cosmos/cosmos-sdk/pull/22409) Add support for `ignored` field tag input structs.

## 1.0.0

* [#20540](https://github.com/cosmos/cosmos-sdk/pull/20540) Add support for defining `appconfig` module configuration types using `github.com/cosmos/gogoproto/proto` in addition to `google.golang.org/protobuf` so that users can use gogo proto across their stack.
Expand Down
14 changes: 14 additions & 0 deletions depinject/binding_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,3 +300,17 @@ func TestBindingInterfaceTwoModuleScopedAndGlobalBinding(t *testing.T) {
IsResolvedModuleScope(t, pond, moduleC, "Marbled")
IsResolvedInGlobalScope(t, pond, "Marbled")
}

func TestIgnoredField(t *testing.T) {
t.Parallel()
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)
Comment on lines +306 to +315
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance test coverage with additional assertions

While the test verifies successful injection, it should also validate:

  1. The ignored field remains unmodified
  2. 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.

Suggested change
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)

}
18 changes: 15 additions & 3 deletions depinject/struct_args.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,11 @@ import (
//
// Fields of the struct may support the following tags:
//
// 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
// 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
Comment on lines +15 to +19
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 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 TestIgnoredField only 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

type In struct{}

func (In) isIn() {}
Expand Down Expand Up @@ -113,6 +115,11 @@ func structArgsInTypes(typ reflect.Type) ([]providerInput, error) {
continue
}

_, found := f.Tag.Lookup("ignored")
if found {
continue
}

var optional bool
optTag, found := f.Tag.Lookup("optional")
if found {
Expand Down Expand Up @@ -170,6 +177,11 @@ func buildIn(typ reflect.Type, values []reflect.Value) (reflect.Value, int, erro
if f.Type.AssignableTo(isInType) {
continue
}
_, found := f.Tag.Lookup("ignored")
if found {
continue
}

Comment on lines +180 to +184
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ 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.

if !res.Elem().Field(i).CanSet() {
return reflect.Value{}, 0, fmt.Errorf("depinject.In struct %s on package %s can't have unexported field", res.Elem().String(), f.PkgPath)
}
Expand Down
Loading