ardanlabs / service

Starter-kit for writing services in Go using Kubernetes.
https://www.ardanlabs.com
Apache License 2.0
3.4k stars 613 forks source link

validation error is giving internal error instead #335

Closed khatibomar closed 5 months ago

khatibomar commented 5 months ago

When having validation error, the middle ware is unable to catch it.

I figured out why

https://github.com/ardanlabs/service/blob/c2335ad03241a8fded921cfa5f0efbf5cc5eb224/business/web/v1/mid/errors.go#L31C26-L31C26

func IsError(err error) bool {
    var re *Error
    return errors.As(err, &re)
}

here it's doing an validation with Error which is of type

type Error struct {
    Err    error
    Status int
}

while the Field error is

// FieldError is used to indicate an error with a specific request field.
type FieldError struct {
    Field string `json:"field"`
    Err   string `json:"error"`
}

so it doesn't enter the case, I've fixed it in my code, but not sure how to implemented in such good reusable way.

I am reporting the error now, when I have time I will take a look more into it.

ardan-bkennedy commented 5 months ago

Start with this code that is inside the middleware for error handling.

switch {
    case response.IsError(err):
    reqErr := response.GetError(err)

    if validate.IsFieldErrors(reqErr.Err) {
        fieldErrors := validate.GetFieldErrors(reqErr.Err)
        er = response.ErrorDocument{
            Error:  "data validation error",
            Fields: fieldErrors.Fields(),
        }
        status = reqErr.Status
        break
    }

You can see a validate.FieldError is expected to be inside an response.Error. That happens in the app layer.

    var app AppNewProduct
    if err := web.Decode(r, &app); err != nil {
        return response.NewError(err, http.StatusBadRequest)
    }

The web.Decode function is calling validate as you will see in a second. That return is wrapping the validate.FieldError inside of a response.Error. We want this so we can control the Status to be used on the response.

    if v, ok := val.(validator); ok {
        if err := v.Validate(); err != nil {
            return fmt.Errorf("unable to validate payload: %w", err)
        }
    }

Here is code inside the web.Decode call. That is an interface call, so we need to find a concrete implementation.

    func (app AppNewProduct) Validate() error {
        if err := validate.Check(app); err != nil {
             return err
        }

        return nil
    }

That is calling validate.Check. Let's look at that.

    func Check(val any) error {
    if err := validate.Struct(val); err != nil {

        // Use a type assertion to get the real error value.
        verrors, ok := err.(validator.ValidationErrors)
        if !ok {
            return err
        }

        var fields FieldErrors
        for _, verror := range verrors {
            field := FieldError{
                Field: verror.Field(),
                Err:   verror.Translate(translator),
            }
            fields = append(fields, field)
        }

        return fields
    }

    return nil
}

There you have it. This function returns a FieldError, which is then passed up to the app layer through the Validate function. Once the app layer gets it, it is passed to the middleware like this.

    return response.NewError(err, http.StatusBadRequest)

So the error is embedded in the wrapping. This is done so the app layer can control the status code.

NOW, if you don't want to hide the error in a response.Error, you can add a case to the middleare for this specific error type.

khatibomar commented 5 months ago

Thank you, now it's clear for me.