Skip to content

fix: Improve friendly validation error messages #3390

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
Merged
merged 2 commits into from
Aug 9, 2022
Merged

fix: Improve friendly validation error messages #3390

merged 2 commits into from
Aug 9, 2022

Conversation

mafredri
Copy link
Member

@mafredri mafredri commented Aug 5, 2022 •
edited
Loading

This PR adds the validation errors to the friendly error message.

I found it confusing when working on #3000 and all I got was Validation failed..

Before:

❯ go run ./cmd/coder rename testy user/tester
Validation failed.
Run 'coder rename --help' for usage.

After:

❯ go run ./cmd/coder rename testy user/tester
Validation failed.
- name: Validation failed for tag "workspace_name" with value: "user/tester"
Run 'coder rename --help' for usage.

The original Validation failed for tag "username" was also confusing, so new validators were registered.

  • fix: Add validations to (*codersdk.Error).Friendly
  • fix: Add named validators for template and workspace name

Fixes #1673

@mafredri mafredri self-assigned this Aug 5, 2022
@mafredri mafredri requested a review from a team August 5, 2022 12:03
johnstcn
johnstcn reviewed Aug 5, 2022
View reviewed changes
coderd/httpapi/httpapi.go
@@ -31,16 +29,19 @@ func init() {
}
return name
})
err := validate.RegisterValidation("username", func(fl validator.FieldLevel) bool {
nameValidator := func(fl validator.FieldLevel) bool {
f := fl.Field().Interface()
str, ok := f.(string)
if !ok {
return false
}
return UsernameValid(str)
Copy link
Member

Choose a reason for hiding this comment

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

The semantics for username validation may be different to those of workspace and template names.
We should register different validators for each of these.

Copy link
Member Author

Choose a reason for hiding this comment

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

Previously these fields used the username validation tag, so behavior hasn't changed and I wasn't looking to change the behavior in this PR. If the validation is incorrect then we should definitely fix that.

Copy link
Member

Choose a reason for hiding this comment

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

Related #3321

@AbhineetJain
Copy link
Contributor

This PR seems related to #1943 and should be impacting the error messages that we show on frontend, so a verification of the frontend behavior and a review from @presleyp should be helpful.

@mafredri
Copy link
Member Author

mafredri commented Aug 8, 2022

@AbhineetJain thanks for keeping an eye out. I don't think this affects the frontend though, except for the validation error message when creating a workspace, if the workspace name is invalid. Then the error message will say that the validator that failed is workspace_name instead of username.

@mafredri mafredri mentioned this pull request Aug 8, 2022
Closed
bpmct
bpmct reviewed Aug 8, 2022
View reviewed changes
Copy link
Member

@bpmct bpmct left a comment

Choose a reason for hiding this comment

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

Are we able to surface the regex and/or what characters violated it? From looking at the code, it is clear you cannot use spaces and/or underscores but this may not be immediately clear to the user.

@mafredri
Copy link
Member Author

mafredri commented Aug 8, 2022

Are we able to surface the regex and/or what characters violated it? From looking at the code, it is clear you cannot use spaces and/or underscores but this may not be immediately clear to the user.

AFAICT, not very straight-forward with the validator library we use. I looked a bit through the API and validators are mostly booleans, so we'd need to utilize multiple validators for one field to be able to narrow down the error case? Perhaps we should look into replacing the library, or just implement custom types with their own JSON marshal/unmarshal where we can control the behavior.

@bpmct
Copy link
Member

bpmct commented Aug 8, 2022

AFAICT, not very straight-forward with the validator library we use. I looked a bit through the API and validators are mostly booleans, so we'd need to utilize multiple validators for one field to be able to narrow down the error case? Perhaps we should look into replacing the library, or just implement custom types with their own JSON marshal/unmarshal where we can control the behavior.

Oh, that makes sense. Perhaps something we can look into later if it comes up again.

@Emyrk
Copy link
Member

Emyrk commented Aug 8, 2022

Are we able to surface the regex and/or what characters violated it? From looking at the code, it is clear you cannot use spaces and/or underscores but this may not be immediately clear to the user.

AFAICT, not very straight-forward with the validator library we use. I looked a bit through the API and validators are mostly booleans, so we'd need to utilize multiple validators for one field to be able to narrow down the error case? Perhaps we should look into replacing the library, or just implement custom types with their own JSON marshal/unmarshal where we can control the behavior.

I noticed this too with the validator lib before. There is something you can do with their "translator" api,but it seems kinda jank. I wish the validator custom functions could return custom errors.

@mafredri
Copy link
Member Author

mafredri commented Aug 9, 2022

I noticed this too with the validator lib before. There is something you can do with their "translator" api,but it seems kinda jank. I wish the validator custom functions could return custom errors.

Agreed, using the translations API seems very jank. I found one project utilizing it, but that's a lot of code for working around the fact that the validator API has shortcomings: https://github.com/Aptomi/k8s-app-engine/blob/11b495fc520680093a990fee0eaaf598507596b5/pkg/lang/validation.go

There are a few other libs that seem to have a better API:

But I have no experience with these.

@mafredri mafredri merged commit c0cc8b9 into main Aug 9, 2022
@mafredri mafredri deleted the mafredri/improve-cli-error-message-on-validation-failed branch August 9, 2022 11:25
@mafredri mafredri mentioned this pull request Aug 9, 2022
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bpmct bpmct bpmct left review comments

@johnstcn johnstcn johnstcn approved these changes

Assignees

@mafredri mafredri

Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@mafredri @AbhineetJain @bpmct @Emyrk @johnstcn