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

Enrich error messages #10

Open
francesconi opened this issue Oct 28, 2022 · 15 comments
Open

Enrich error messages #10

francesconi opened this issue Oct 28, 2022 · 15 comments

Comments

@francesconi
Copy link

francesconi commented Oct 28, 2022

Any chance you could extend MessageValidator with a function where I can set a custom error instead of just a string? So in addition to .Msg(s string) something like .Err(err error)? My errors contain additional information which would allow me further processing after validation.

@RussellLuo
Copy link
Owner

RussellLuo commented Oct 29, 2022

@francesconi Thanks for your attention!

validating concentrates on data validation, so the corresponding errors are also designed around data validation. For now, an error of validating consists of three parts:

  • error kind (i.e. UNSUPPORTED and INVALID)
  • field name
  • error message (customizable)

I'm wondering when a custom error (except the message itself), in practice, is needed while doing validation? Could you give me more details?

@francesconi
Copy link
Author

Let's say you have a custom error with a dynamic string

type NotFoundError struct {
    File string
}

func (e *NotFoundError) Error() string {
    return fmt.Sprintf("file %q not found", e.File)
}

you would be able to match it with errors.As.

for _, err := v.Validate(...) {
    var notFound *NotFoundError
    if errors.As(err, &notFound) {
        // handle the error
    } else {
        panic("unknown error")
    }
}

And you could still use errors.Is to match and handle errors with a static string.

How would you achieve this with the current implementation? I know this is a bit more than just validating but IMHO this approach would open the door to a lot more usecases.

@RussellLuo
Copy link
Owner

Thanks for the details! How will .Err(err error), if supported, be used along with NotFoundError in the schema definition? What does the code look like in your example?

@francesconi
Copy link
Author

Based on the previous example something like this

(untested)

type Foo struct {
    File string
}

func (f *Foo) Schema(whitelist []string) v.Schema {
    return v.Schema{
        v.F("file", p.File): v.In(whitelist).Err(&NotFoundError{File: p.File}),
    }
}

func main() {
    whitelist := []string{"a", "b", "c"}

    f := Foo{}
    errs := v.Validate(f.Schema(whitelist))
}

@RussellLuo
Copy link
Owner

If it's just a matter of dynamic message strings, I guess maybe we can do something like this:

[play]

package main

import (
	"fmt"

	v "github.com/RussellLuo/validating/v3"
)

type Foo struct {
	File string
}

func (f *Foo) Schema(whitelist []string) v.Schema {
	return v.Schema{
		v.F("file", f.File): v.In(whitelist...).Msg(fmt.Sprintf("file %q not found", f.File)),
	}
}

func main() {
	whitelist := []string{"a", "b", "c"}

	f := Foo{}
	errs := v.Validate(f.Schema(whitelist))
	fmt.Printf("errs: %#v\n", errs)

	// Output:
	// errs: validating.Errors{validating.errorImpl{field:"file", kind:"INVALID", message:"file \"\" not found"}}
}

@francesconi
Copy link
Author

The whole point about this is matching an error with a dynamic string

type Foo struct {
    File string
}

func (f *Foo) Schema(whitelist []string) v.Schema {
    return v.Schema{
        v.F("file", p.File): v.In(whitelist).Err(&NotFoundError{File: p.File}),
    }
}

func main() {
    whitelist := []string{"a", "b", "c"}

    f := Foo{}
    for _, err := v.Validate(f.Schema(whitelist)) {
        var notFound *NotFoundError
        if errors.As(err, &notFound) {
            // handle the error
        }
    }
}

Again, how would you achieve this with just a dynamic message string?

@RussellLuo
Copy link
Owner

RussellLuo commented Nov 3, 2022

Actually, what I'm trying to understand is why distinguishing different errors matters, in the scenario of data validation, if all of errors essentially are of the same kind (e.g. INVLIAD means the field is invalid).

In other words, does NotFoundError, OutOfRangeError or XxxError carry any extra information that's useful for further processing? If this is the case, I think maybe adding "error details" (similar to Google API's Error Design) is an option? (Arbitrarily custom error is very flexible, but at the cost of being non-canonical and too granular.)

@francesconi
Copy link
Author

I've just had a look into Google API's Error Design about error details and I must admit that they would work fine for me 👍

@RussellLuo
Copy link
Owner

Thanks for your feedback! Adding error details seems good, but will also introduce complexity.

For a better understanding of your problem, could you please expand the part // handle the error? That is, in your concrete example, how will the handling of NotFoundError be different from other built-in INVALID errors?

@francesconi
Copy link
Author

A client sends us availabilities for it's hotel rooms. When a certain room does not exists on our side RoomNotFound I can request the client to resend me all rooms it has on record so that the availabilities request will go through the next time.

type RoomNotFoundError struct {
    RoomCode string
}

func (e *RoomNotFoundError) Error() string {
    return fmt.Sprintf("room %s not found", e.RoomCode)
}

We do communicate with the OTA standard and my errors must have a code and a message accordingly ERR.

@RussellLuo
Copy link
Owner

Let me try to construct the validation flow at the server side:

  1. Handle a request containing a room code from client
  2. Validate the room code (using validating's schema)
  3. If the room code is invalid (non-existent), request the client to resend all rooms
    • In this step, is the INVALID information enough for you to make the request? Do you need the value of room code to achieve this?

@francesconi
Copy link
Author

Would be enough if one room is missing but if more rooms are missing i'll get duplicate error messages

// Output:
// err[0]: INVALID(room not found)
// err[1]: INVALID(room not found)

@RussellLuo
Copy link
Owner

RussellLuo commented Nov 4, 2022

If the reaction to any error is to request the client to resend all rooms, I guess the whole operation will be idempotent, i.e., the result will be the same no matter how many rooms are missing, right?

As for the duplicate error messages, what's the corresponding schema definition? Does the method #10 (comment) help in this scenario?

@francesconi
Copy link
Author

The only solution I see for now is using a static error for RoomNotFound in order to request the client to resend all rooms but our support team won't be happy at all having to read these vague error messages.

@RussellLuo
Copy link
Owner

RussellLuo commented Nov 8, 2022

The only solution I see for now is using a static error for RoomNotFound ... but our support team won't be happy to read these vague error messages.

Per the literal description, the problem seems to be the duplicate static messages, which I think should be resolved by the method mentioned in #10 (comment). If it's not the case, I think I still didn't catch the point.

Is there any minimal (and runnable) code that can reproduce the crux of the problem? I think it will be easier to find out the problem with the code, and to figure out the solution :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants