validating icon indicating copy to clipboard operation
validating copied to clipboard

Enrich error messages

Open francesconi opened this issue 2 years ago • 15 comments

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.

francesconi avatar Oct 28 '22 09:10 francesconi

@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?

RussellLuo avatar Oct 29 '22 07:10 RussellLuo

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.

francesconi avatar Nov 01 '22 10:11 francesconi

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?

RussellLuo avatar Nov 02 '22 00:11 RussellLuo

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))
}

francesconi avatar Nov 02 '22 21:11 francesconi

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"}}
}

RussellLuo avatar Nov 03 '22 01:11 RussellLuo

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?

francesconi avatar Nov 03 '22 06:11 francesconi

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.)

RussellLuo avatar Nov 03 '22 10:11 RussellLuo

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 :thumbsup:

francesconi avatar Nov 03 '22 12:11 francesconi

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?

RussellLuo avatar Nov 04 '22 00:11 RussellLuo

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.

francesconi avatar Nov 04 '22 09:11 francesconi

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?

RussellLuo avatar Nov 04 '22 09:11 RussellLuo

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)

francesconi avatar Nov 04 '22 11:11 francesconi

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 https://github.com/RussellLuo/validating/issues/10#issuecomment-1301543424 help in this scenario?

RussellLuo avatar Nov 04 '22 14:11 RussellLuo

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.

francesconi avatar Nov 07 '22 19:11 francesconi

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 https://github.com/RussellLuo/validating/issues/10#issuecomment-1301543424. 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 :)

RussellLuo avatar Nov 08 '22 00:11 RussellLuo