Should the Errors struct provide getters and setters?

[mholt]

As I re-write the tests, I'm finding a few more edge cases and issues. For instance, if you have a simple struct:

type Post struct {
    Title   string `form:"title" binding:"required"`
    Content string `form:"content"`
}

Notice the Title field is required. However, suppose your struct is also a Validator, and its Validate() method looked like:

func (p Post) Validate(errs *Errors, req *http.Request) {
    if len(p.Title) < 10 {
        errs.Fields["title"] = "Title is too short"
    }
}

Now suppose a request comes in and the Title field is left blank. Well, the obvious error to get back is simply "Required" -- who cares that it's too short, it didn't even get submitted!

Turns out that, right now, this implementation will overwrite the "Required" error which is added for you.

So should the Fields and Overall fields of the Errors struct be made un-exported, and used with getters and setters instead? (The setter could prevent overwriting an existing error for that field.)

And should we allow multiple errors to the same key (i.e. instead of map[string]string, we have map[string][]string)? That would allow us to show both errors for the field... or however many... the downside is that it's slightly more complex.

See issue #3 for a similar discussion (though I'm not sure I want to include original input value, as that is even more complex).

Thoughts?

[mholt]

Okay, after some discussion with my coworker, I'm already pretty convinced that something like this will be much more flexible and useful:

var errors []Error
// ...
type Error struct {
    FieldNames     []string     // name(s) of the fields involved, if any
    Classification string       // error type or category
    Message        string       // human-readable or detailed message
}

There would probably be getters/setters as a little sugar. The reason FieldNames is plural and []string instead of just string is because there might be multiple fields together that cause a validation error. For example, a CC expiration date often spans two fields.

For "Overall" errors (problems validating the request as a whole, for example, deserialization errors or header problems), just leave the FieldNames empty.

Yes, I think I will proceed with this unless there's some backlash. I know it will break existing implementations but this is to prepare for a stable 1.0.

[attilaolah]

I think getters and setters are might be a good idea. Then you don't have to break the interface in case you want to add other functionality.

[mholt]

I just finished experimenting with getters/setters this weekend and it made the code more cumbersome; in reality, they were a nuisance more than a convenience. It turned out just to be easier to manually initialize the struct. The nice thing is that there's nothing stopping us from adding methods to the Errors or Error types if we deem them necessary later.

[attilaolah]

Yeah, I realised now that you can just append to errors, which does seem better. Although in your example here, that would report two errors for the title field, one from the required tag, and one for the too short validation.

[mholt]

Yup, that's the idea... granted, the example here is a bit contrived, since one error is kind of a subset of the other. Imagine a different case, maybe where a field is supposed to match a certain format and check out OK in your application (like meeting some criteria) -- this way your app can append to the errors such that the user knows "Hey, it's not in the right format, and also, this won't work because ..."

At work we came up with real scenarios where more than one thing was wrong with a field and also where one thing was wrong with more than one field.

Fortunately, if you want to limit to one error per field, you can either use your own error handler to strip out extra errors or just check for existing errors before adding new ones.

Maybe a method could be added, kind of like the new Has, to see if there's an error on a particular field name, like HasField?

[attilaolah]

Hmm… Maybe. Personally, I don't need it right now… But without such a method, you'd have to loop through the errors which could clutter up the code for someone. Then again, they could just implement it themselves.

Well, maybe adding it wouldn't hurt.

[mholt]

This topic is conveniently merging with the project ongoing in issue #9 related to validation features. The martini-contrib/binding package will have the Errors type which will implement an interface specified by this validation package, which settles the questions of having getters/setters.