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.

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

fix: Validator Bind can't cast value #336

Merged
merged 23 commits into from
Nov 10, 2023
Merged

fix: Validator Bind can't cast value #336

merged 23 commits into from
Nov 10, 2023

Conversation

devhaozi
Copy link
Member

@devhaozi devhaozi commented Nov 5, 2023

Closes goravel/goravel#302

πŸ“‘ Description

βœ… Checks

  • My pull request adheres to the code style of this project
  • My code requires changes to the documentation
  • I have updated the documentation as required
  • All the tests have passed

β„Ή Additional Information

Copy link

codecov bot commented Nov 5, 2023

Codecov Report

Attention: 19 lines in your changes are missing coverage. Please review.

Files Coverage Ξ”
validation/validation.go 96.61% <71.42%> (-0.45%) ⬇️
validation/validator.go 76.66% <79.16%> (-7.55%) ⬇️

... and 2 files with indirect coverage changes

πŸ“’ Thoughts on this report? Let us know!

@devhaozi
Copy link
Member Author

devhaozi commented Nov 5, 2023

issue link to goravel/fiber/pull/36

@devhaozi devhaozi requested a review from hwbrzzl November 5, 2023 14:33
validation/validator.go Outdated Show resolved Hide resolved
validation/validator.go Outdated Show resolved Hide resolved
validation/validator.go Outdated Show resolved Hide resolved
validation/validator.go Outdated Show resolved Hide resolved
@devhaozi
Copy link
Member Author

devhaozi commented Nov 5, 2023

I found the mapstructure package that does the trick so we no longer need to implement the parsing step ourselves (it's complex and easy to make error).

@devhaozi devhaozi requested review from kkumar-gcc and a team November 7, 2023 11:47
@devhaozi
Copy link
Member Author

devhaozi commented Nov 7, 2023

After more testing, https://www.goravel.dev/the-basics/validation.html#int is not resolved because the json.Unmarshal used internally by gookit/validate does not handle this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

This test file has some error rules, I fixed it.

Comment on lines -32 to +45
var dataType reflect.Kind
switch data := data.(type) {
var dataFace validate.DataFace
var err error
switch td := data.(type) {
case validate.DataFace:
dataFace = td
case map[string]any:
if len(data) == 0 {
if len(td) == 0 {
return nil, errors.New("data can't be empty")
}
dataType = reflect.Map
}

val := reflect.ValueOf(data)
indirectVal := reflect.Indirect(val)
typ := indirectVal.Type()
if indirectVal.Kind() == reflect.Struct && typ != reflect.TypeOf(time.Time{}) {
dataType = reflect.Struct
}

var dataFace validate.DataFace
switch dataType {
case reflect.Map:
dataFace = validate.FromMap(data.(map[string]any))
case reflect.Struct:
var err error
dataFace = validate.FromMap(td)
case url.Values:
dataFace = validate.FromURLValues(td)
case map[string][]string:
dataFace = validate.FromURLValues(td)
default:
Copy link
Member Author

Choose a reason for hiding this comment

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

I refactored it by referring to the New method of github.com/gookit/validate

expectData: Data{A: "c"},
expectData: Data{A: ""},
Copy link
Member Author

Choose a reason for hiding this comment

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

Now validator only Bind safe data, it meens validation must passed, so I change some test data.

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to release this fix to v1.13.x, right? because it's a breaking change.

Copy link
Member Author

Choose a reason for hiding this comment

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

You don't need to release this fix to v1.13.x, right? because it's a breaking change.

Yes

@devhaozi
Copy link
Member Author

devhaozi commented Nov 7, 2023

After more testing, https://www.goravel.dev/the-basics/validation.html#int is not resolved because the json.Unmarshal used internally by gookit/validate does not handle this case.

This now can be solve use filter tag in Request struct.

See: goravel/fiber#38

validation/validation.go Show resolved Hide resolved
validation/validation.go Show resolved Hide resolved
validation/validator.go Show resolved Hide resolved
Copy link
Contributor

@hwbrzzl hwbrzzl left a comment

Choose a reason for hiding this comment

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

Perfect!

@devhaozi devhaozi enabled auto-merge (squash) November 10, 2023 07:44
@devhaozi devhaozi merged commit 376f1d9 into master Nov 10, 2023
8 checks passed
@devhaozi devhaozi deleted the haozi/validator branch November 10, 2023 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

πŸ› [Bug] Validator Bind can't cast value
2 participants