feat: Dont make copies of structs for validation #665
Conversation
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #665 +/- ##
==========================================
- Coverage 77.61% 77.59% -0.03%
==========================================
Files 22 22
Lines 7953 7959 +6
==========================================
+ Hits 6173 6176 +3
- Misses 1362 1364 +2
- Partials 418 419 +1 |
|
I'm not sure, if the |
There was a problem hiding this comment.
Pull Request Overview
This PR modifies the validation implementation to avoid unnecessary struct copies by ensuring a pointer is used during validation.
- Updated the test validation helper and added tests to enforce pointer-based validation.
- Modified the decoding logic to conditionally pass the address of a struct for validation when possible.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| testdata/validate_test.go | Added a testValidator helper and tests for pointer-based validation |
| decode.go | Updated decodeStruct to use pointer when validating a struct to avoid unnecessary copying |
| @@ -1425,7 +1425,13 @@ func (d *Decoder) decodeStruct(ctx context.Context, dst reflect.Value, src ast.N | |||
| } | |||
|
|
|||
| if d.validator != nil { | |||
| if err := d.validator.Struct(dst.Interface()); err != nil { | |||
| var err error | |||
There was a problem hiding this comment.
As you already mentioned in your comment, dst is guaranteed to be addressable here, so there's no need to call CanAddr.
if err := d.validator.Struct(dst.Addr().Interface()); err != nil {
| return tv.v.Struct(v) | ||
| } | ||
|
|
||
| func TestStructValidatorWithAPointer(t *testing.T) { |
There was a problem hiding this comment.
Thank you for adding unit tests here! However, checking whether v is a pointer seems like an unnecessary implementation detail that we don't really care about. This code path is already well covered by the unit tests above, so I believe we don't really need these tests.
|
Closing in favour of #737 |
This PR fixes a small problem with validation implementation:
Current implementation uses
dst.Interface()which returns a value(T), not a pointer(*T) of a structT. This can be a performance hit on bigger structures as compiler would have to partially copy the structT. And since validation is performed on each structure, the whole thing would be copied at least N times (where N is the structure depth).Passing by value also does not play nicely with other (non-reflection based) validation libraries, such as
github.com/go-ozzo/ozzo-validation, since validating a structure there can require a method call, which is not possible. Converting aninterface{}with value is not an option also -reflect.Value.Addr()is prohibited forValues, created viareflect.ValueOf(v).