Reworked Required and OmitEmpty #8

Merged
lunny merged 4 commits from KN4CK3R/binding:feature-ignore-empty into master 2 years ago
Collaborator

The old behaviour is inconsistent because this fails:

type TestForm struct {
	Valid string `binding:"Url"`
	Fails string `binding:"Email"`
}

form := &TestForm{
	Valid: "",
	Fails: "",
}
_ := RawValidate(form)

Here you need OmitEmpty even if the field is not required.

The new logic:

Empty Required Valid
Yes Yes No
Yes No Yes, all other rules are skipped
No / Maybe, other rules decide

In short: Rules are skipped if a field is empty and not required. Thats the normal behaviour in other validation libraries like Yup too.

After this change there are some places in Gitea where we can add form validation to optional fields which is not possible at the moment. Possible changes in:
21465a2ce3/modules/structs/admin_user.go (L31)
21465a2ce3/modules/structs/admin_user.go (L35)
21465a2ce3/modules/structs/org_team.go (L38)
21465a2ce3/modules/validation/binding.go (L90)
21465a2ce3/modules/validation/binding.go (L108)

The old behaviour is inconsistent because this fails: ```go type TestForm struct { Valid string `binding:"Url"` Fails string `binding:"Email"` } form := &TestForm{ Valid: "", Fails: "", } _ := RawValidate(form) ``` Here you need `OmitEmpty` even if the field is not required. The new logic: | Empty | Required | Valid | | -------- | -------- | -------- | | Yes | Yes | No | | Yes | No | Yes, all other rules are skipped | | No | / | Maybe, other rules decide | In short: Rules are skipped if a field is empty and not required. Thats the normal behaviour in other validation libraries like Yup too. After this change there are some places in Gitea where we can add form validation to optional fields which is not possible at the moment. Possible changes in: https://github.com/go-gitea/gitea/blob/21465a2ce361eeb2bff171a0b962dac771ab18d9/modules/structs/admin_user.go#L31 https://github.com/go-gitea/gitea/blob/21465a2ce361eeb2bff171a0b962dac771ab18d9/modules/structs/admin_user.go#L35 https://github.com/go-gitea/gitea/blob/21465a2ce361eeb2bff171a0b962dac771ab18d9/modules/structs/org_team.go#L38 https://github.com/go-gitea/gitea/blob/21465a2ce361eeb2bff171a0b962dac771ab18d9/modules/validation/binding.go#L90 https://github.com/go-gitea/gitea/blob/21465a2ce361eeb2bff171a0b962dac771ab18d9/modules/validation/binding.go#L108
KN4CK3R added 2 commits 2 years ago
continuous-integration/drone/pr Build was killed Details
e519a9d825
Added tests.
KN4CK3R added 1 commit 2 years ago
lunny commented 2 years ago
Owner

Please resolve the conflicts.

Please resolve the conflicts.
Poster
Collaborator

I see no conflicts? There was a conflict which should be resolved with the merge of the master branch some hours ago.

I see no conflicts? There was a conflict which should be resolved with the merge of the master branch some hours ago.
lunny commented 2 years ago
Owner

It maybe a bug of Gitea, when update, it displays below error.

Auto-merging validate_test.go
CONFLICT (content): Merge conflict in validate_test.go
Auto-merging binding.go
Automatic merge failed; fix conflicts and then commit the result.
It maybe a bug of Gitea, when update, it displays below error. ``` Auto-merging validate_test.go CONFLICT (content): Merge conflict in validate_test.go Auto-merging binding.go Automatic merge failed; fix conflicts and then commit the result. ```
6543 commented 2 years ago
Owner

@KN4CK3R can you rebase or update this pull please?

@KN4CK3R can you rebase or update this pull please?
KN4CK3R added 1 commit 2 years ago
6543 approved these changes 2 years ago
lunny approved these changes 2 years ago
lunny merged commit b29e041704 into master 2 years ago
lunny referenced this issue from a commit 2 years ago

Reviewers

6543 approved these changes 2 years ago
lunny approved these changes 2 years ago
continuous-integration/drone/pr Build is passing
The pull request has been merged as b29e041704.
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: go-chi/binding#8
Loading…
There is no content yet.