validate.Struct() always return error

Question

validate.Struct() always return error

aliuygur opened this issue 10 years ago · 13 comments

hi, i am new in golang, maybe that problem about me.

when i use validator.Struct() in a function that return named error, the err var always not nil

func ValidateStruct(v interface{}) (err error) {
    err = validate.Struct(v)
    return
}

but when i use it like this, it works

func ValidateStruct(v interface{}) (err error) {
    errs := validate.Struct(v)
    if errs != nil {
        err = errs
        return
    }
    return nil
}

so what the problem ?

deankarn commented 10 years ago

Sure

Answer 1 · 2015-07-30T20:43:20.000Z

Hey @alioygur that's an interesting one, I do have a few questions:

what version of this library are you using v5 or v6?
how are you checking if the error is nil or not from the calling code? could you provide a sample?

This is just a guess before I can get how your checking the nil, but if your using v6 and you are printing the error and it outputs map[] this is a let's call it goism for lack of a better term. fmt.Println and log.Println don't always print the value but just call the String() function of the object, by default, and that function returns a string.

V6 returns a type ValidationErrors which is really a map[string]*FieldError and a map in go is a by-reference type, meaning it's always passed by reference. Because of this a maps default value is nil, but printing it out it's Internal String() function will print "map[]" which is it's type and not value.

Assuming this is all true if you do fmt.Println(err == nil) it will print "true", it is nil, just doesn't print that way by default in go unless other options with Printf are used, see fmt package documentation for those.

Either way let me know

Answer 2 · 2015-07-30T20:57:21.000Z

i am using v6, there is sample.

package main

import (
    "fmt"

    "gopkg.in/bluesuncorp/validator.v6"
)

// User contains user information
type User struct {
    FirstName string `validate:"required"`
    LastName  string `validate:"required"`
}

var validate *validator.Validate

func main() {

    config := validator.Config{
        TagName:         "validate",
        ValidationFuncs: validator.BakedInValidators,
    }

    validate = validator.New(config)

    validateStruct()
    validateStruct2()
}

// this not works
func validateStruct() (err error) {
    user := &User{
        FirstName: "Badger",
        LastName:  "Smith",
    }

    err = validate.Struct(user)

    if err != nil {
        fmt.Println("validation failed")
    }

    return
}

// this works
func validateStruct2() (err error) {
    user := &User{
        FirstName: "Badger",
        LastName:  "Smith",
    }

    errs := validate.Struct(user)

    if errs != nil {
        fmt.Println("validation failed 2")
        err = errs
    }

    return
}

Answer 3 · 2015-07-30T21:06:51.000Z

I will take a look at this asap, probably a little later tonight and get back to you.

Answer 4 · 2015-07-30T21:08:27.000Z

P.s. doe's it work without name error parameters?

Answer 5 · 2015-07-31T00:04:55.000Z

This is a little tricky to explain, but I'll try my best:

Understanding By-Reference for map

ok so like I have mentioned above map is one of go's by reference data types, so what that means is that the map is actually a pointer to location in memory. In this case the pointer can point to a valid ValidationErrors object or nil; but either way the pointer is always valid even if the value it points to isn't

in go you can access the values of reference types just like you would as if they were not ( except in special cases but we're not getting into that ) so that's why this works:

// declaring variables in long form for clarity
var err ValidationErrors = validate.Struct(user)

// when accessing err it will actually access the value in memory which may be nil or valid
if err != nil {
// ...
}

Situation

In validateStruct() your assigning

err = validate.Struct(user)

but the return value of validate.Struct() is of type ValidationErrors, you can do this because ValidationErrors implements the error interface; however it does not work quite how one would expect.

Expected

The value of err gets set to the value of the ValidationErrors, which in your example is nil.

What Actually Happens

The value of err gets set to the pointer of ValidationErrors which points to it's value
so errs value -->pointer and pointers value --> value
so that's why your error is always not nil, because it's value is actually the pointer, which is always not nil, even if it's underlying value is.

Why not pass back error from validate.Struct() instead of ValidationErrors

It's true that this would make your failing example work as expected. Doing it this way was a design decision so that some people may avoid unnecessary type assertions. Take the following example into consideration, many people, including myself, handle their errors right away, without passing the error around as error.

for example

// err will be ValidationErrors
err := validate.Struct(user)

if err != nil {
  // range over err, create error messages, based off of FieldError
  // information and return those errors, or a map[string]string
}

return formatted errors

but if I got type error returned

err = validate.Struct(user)

if err != nil {
  // now to range over my ValidationErrors I would need to assert 
  // that the err object is in fact ValidationErrors
  valErrs := err.(validator.ValidationErrors)
  // then you can range over valErrs or whatever, however you see 
  // that we had to assign another variable and do a type assertion, 
  // although this is pretty light weight it still has some overhead
}

so those that pass the ValidationErrors as error and want access to the FieldError data will need to do type assertions, but not everyone has to the way it works now.

Recommended Approach if you still wish to pass by error

func validateStruct() error {
    user := &User{
        FirstName: "Badger",
        LastName:  "Smith",
    }

    err := validate.Struct(user)

    if err != nil {
        return err
    }

    return nil
}

@alioygur sorry for being so long winded about this, but because you said you were new to go I just wanted the answer to be as thorough as possible. I hope I made sense, do you have any questions?

Final thoughts - also because you said you were new to go

Feel completely free to totally disregard this recommendation, it's just a suggestion, as I've played around with named return params myself and stopped completely.

I would recommend not using named return params they are a super micro micro optimization that tends, IMHO, to really hurt code maintainability; they seem like a good idea, but as your functions size grows and you have to constantly scroll back to see what you named the variable, and tracing back where a value was last set, or if a value was set really slow you down. On the flip side returning where and when you need to is easier to read, even for those that did not write the code, and is much clearer about what is happening.

Answer 6 · 2015-07-31T15:32:26.000Z

@joeybloggs so so thank you for your response. i will not use the named error.

Answer 7 · 2015-07-31T16:28:28.000Z

No problem @alioygur I'll leave this issue open for a few days before I close it.

If you have any more questions, issues or any recommendations don't hesitate to ask!

Answer 8 · 2015-07-31T16:37:19.000Z

@joeybloggs , can you review this code for me ?

Answer 9 · 2015-07-31T17:55:11.000Z

It seems like you have a pretty good grip on go, code looks very good. And FINALLY! someone using the "dive" tag I worked so hard to have that work like it does 😄

I would recommend a few things, but they are minor and some just pure preference.

1. DeletePost possible issue, see comment in code

// DeletePost delete post handler
func DeletePost(c *Context) error {
    post := new(Post)
    err := DB.Find(post, c.Param(":id")).Error
    if err != nil {
        return err
    }

        // does DB.Find return an error when post is not found?
        // if not the next line will probably error out

       // secondly is not finding a post for deletion an error, or is it ok?

    err = DB.Delete(post).Error
    if err != nil {
        return err
    }

    return c.NoContent(204)
}

2. Functions that don't return a new parameter

this ones totally a preference, but when a function doesn't return a new variable, save a reusable one like error, or I don't need the new variable below the current line, I tend to write it like this

var err error

if err = BindAndValidate(c.Request, req); err != nil
  return err
}

so your CreatePost could be rewritten in fewer lines, but just as readable

// CreatePost create post handler
func CreatePost(c *Context) error {

        var err error
    req := new(NewPostReq)

    if err = BindAndValidate(c.Request, req); err != nil {
        return err
    }

    post := new(Post)
    if err = CopyTo(req, post); err != nil {
        return err
    }
    post.UserID = 1
    if err = DB.Create(post).Error; err != nil {
        return err
    }

    pr := new(PostResource)
    if err = CopyTo(post, pr); err != nil {
        return err
    }

    return c.JSON(201, pr)
}

3. dynamic structs

This one may be because of how JSON is processed, so if so totally ignore this. I usually take the time to create the sub structs that could be dynamic, which allows for reusability later down the line; but there are caveats, see below:

// so instead of
type NewPostReq struct {
    Title       string `validate:"required"`
    Description string `validate:"required"`
    Body        string `validate:"required"`
    Tags        []struct {
        Tag string `validate:"required"`
    } `validate:"min=2,dive"`
}

// I would do 

type Tag struct {
  Name string `validate:"required"`
}

type NewPostReq struct {
    Title       string `validate:"required"`
    Description string `validate:"required"`
    Body        string `validate:"required"`
    Tags        []*Tag `validate:"min=2,dive"`
}

// NOTE: you will notice that I used []*Tag, note the * 
// although there is debate on this I tend to pass almost 
// all struct by reference internally within my code.
// but there are times to lock it down, just take a look
// at Config within my validator, it is passed by value
// so that it gets copied and you can't manipulate it after 
initialization.

the caveats are

the JSON would probably have to be formatted differently
my validators error key in the map, if it failed, would be "NewPostReq.Tags[0].Tag.Name" instead of "NewPostReq.Tags[0].Tag" and the field would be "Name" instead of "Tag"

anyways @alioygur I thinks that's it, and again, code looks great, hopefully you can star my repo if this package/library works out for you.

P.S. if the code your working on becomes a project, and it's ok with you I would love to start listing some projects on my README that use my library. and be sure to check out some of my other projects here https://github.com/bluesuncorp I'm going to be adding more as time goes on.

Answer 10 · 2015-07-31T19:04:53.000Z

Tank you so much dear. I am not on computer now. I will reply to you
later.
On Jul 31, 2015 8:55 PM, "Dean Karn" notifications@github.com wrote:

It seems like you have a pretty good grip on go, code looks very good. And
FINALLY! someone using the "dive" tag I worked so hard to have that work
like it does [image: 😄]

I would recommend a few things, but they are minor and some just pure
preference.

DeletePost possible issue, see comment in code

// DeletePost delete post handlerfunc DeletePost(c *Context) error {
post := new(Post)
err := DB.Find(post, c.Param(":id")).Error
if err != nil {
return err
}
    // does DB.Find return an error when post is not found?
    // if not the next line will probably error out

   // secondly is not finding a post for deletion an error, or is it ok?

err = DB.Delete(post).Error
if err != nil {
    return err
}

return c.NoContent(204)
}

Functions that don't return a new parameter

this ones totally a preference, but when a function doesn't return a new
variable, save a reusable one like error, or I don't need the new variable
below the current line, I tend to write it like this

var err error
if err = BindAndValidate(c.Request, req); err != nil
return err
}

so your CreatePost could be rewritten in fewer lines, but just as readable

// CreatePost create post handlerfunc CreatePost(c *Context) error {
    var err error
req := new(NewPostReq)

if err = BindAndValidate(c.Request, req); err != nil {
    return err
}

post := new(Post)
if err = CopyTo(req, post); err != nil {
    return err
}
post.UserID = 1
if err = DB.Create(post).Error; err != nil {
    return err
}

pr := new(PostResource)
if err = CopyTo(post, pr); err != nil {
    return err
}

return c.JSON(201, pr)
}

dynamic structs

This one may be because of how JSON is processed, so if so totally ignore
this. I usually take the time to create the sub structs that could be
dynamic, which allows for reusability later down the line; but there are
caveats, see below:

// so instead oftype NewPostReq struct {
Title string validate:"required"
Description string validate:"required"
Body string validate:"required"
Tags []struct {
Tag string validate:"required"
} validate:"min=2,dive"
}
// I would do
type Tag struct {
Name string validate:"required"
}
type NewPostReq struct {
Title string validate:"required"
Description string validate:"required"
Body string validate:"required"
Tags []_Tag validate:"min=2,dive"
}
// NOTE: you will notice that I used []_Tag, note the * // although there is debate on this I tend to pass almost // all struct by reference internally within my code.// but there are times to lock it down, just take a look// at Config within my validator, it is passed by value// so that it gets copied and you can't manipulate it after
initialization.

the caveats are

the JSON would probably have to be formatted differently

my validators error key in the map, if it failed, would be
"NewPostReq.Tags[0].Tag.Name" instead of "NewPostReq.Tags[0].Tag" and the
field would be "Name" instead of "Tag"

anyways @alioygur https://github.com/alioygur I thinks that's it, and
again, code looks great, hopefully you can star my repo if this
package/library works out for you.

P.S. if the code your working on becomes a project, and it's ok with you I
would love to start listing some projects on my README that use my library.
and be sure to check out some of my other projects here
https://github.com/bluesuncorp I'm going to be adding more as time goes
on.

—
Reply to this email directly or view it on GitHub
#134 (comment)
.

Answer 11 · 2015-08-03T11:48:30.000Z

@joeybloggs i agree with your all caveats. i will refactor my code by your caveats.

thank you.

Answer 12 · 2015-08-03T12:28:51.000Z

No problems @alioygur, going to close this issue now and if you have anymore questions, just shoot me an email.