Return list of validation errors instead of first failed

[baranov1ch]

Given that one wants to present validation errors to some client, it would be more useful to list all validation errors

[kyessenov]

Is there a reason why errors are not aggregated in the current design?

[rodaine]

The idea was to fail fast on the first invalid field (fast and closed), mostly because clients can't typically take an action based off the error without developer intervention. That said, I definitely see the value in collecting them to make said developer's debugging easier. Do you see this being a configuration option or always on?

[kyessenov]

I think "always on" makes sense since validation is fast, and the client can pick whether to use the first error or all errors. We can use https://github.com/hashicorp/go-multierror or just write a new one for error aggregation and error metadata for go.

[rodaine]

Don't want to take on dependencies we don't need. We can generate it as we currently are with the multiple errors.

[CAFxX]

Another argument in favor of supporting this is that in the ecosystem there is already standardized support for reporting validation errors in multiple fields:

https://github.com/googleapis/googleapis/blob/5691fcb7c1a926b52577aa1834f31d9c50efda54/google/rpc/error_details.proto#L132 https://github.com/grpc-ecosystem/grpc-gateway/blob/da7a886035e25b2f274f89b6f3c64bf70a9f6780/third_party/googleapis/google/rpc/error_details.proto#L133

If you’re using protocol buffers as your data format, however, you may wish to consider using the the richer error model developed and used by Google as described here. This model enables servers to return and clients to consume additional error details expressed as one or more protobuf messages. It further specifies a standard set of error message types to cover the most common needs (such as invalid parameters, quota violations, and stack traces). [...] This richer error model is already supported in the C++, Go, Java, Python, and Ruby libraries [...]

https://grpc.io/docs/guides/error/

[CAFxX]

Do you see this being a configuration option or always on?

I don't think this tradeoff is necessary (assuming that the speed penalty of always returning all of them is a concern). You could return just the first one in the ValidationError as today, but add a method to it (e.g. AllErrors() []ValidationError) that would resume validation after the first error and continue until it can return all validation errors.

Furthermore, it would be ideal if the field name returned was compatible with that expected by field_violation.field: https://github.com/googleapis/googleapis/blob/5691fcb7c1a926b52577aa1834f31d9c50efda54/google/rpc/error_details.proto#L122-L125

[ido-namely]

Hi all, I have a draft pr opened to address this issue, the CI/bazel build is still failing but I was hoping I get can you opinion on my implementation. Thanks!

[akonradi]

@ido-namely thanks for kicking this off for Go. There's been some discussion on that PR that I'd like to continue here around how to present the option of one or all errors to clients. I'd like to steer further brainstorming with the following requirements

1. clients should be able to request fail-fast or thorough error checking at runtime
2. the public API doesn't need to be consistent across languages, and should follow language-specific norms and idioms
3. API stability is good but this is a new feature so don't be afraid to break compatibility

[akonradi]

Inspired by @CAFxX's https://github.com/envoyproxy/protoc-gen-validate/pull/416#issuecomment-756491146, I was wondering whether it would make sense to have (in Go) something like the following:

type ValidationError interface {
    error
}

type SingleValidationError interface {
    ValidationError
    AllErrors() []ValidationError
}

func (m *XxxMessage) Validate() SingleValidationError {
  // ...
}

Clients that want the first error can simply call m.Validate(). Clients that want all errors can call m.Validate().AllErrors() (assuming the actual struct implementing SingleValidationError can be called with nil).

[akavel]

I'm confused how the approach with "SingleValidationError" is intended to work; i.e., what would the m.Validate() be expected to actually calculate? All errors? One error? I don't currently understand how it could achieve the goal of being able to choose fail-fast vs. thorough.

After reading the linked proposal, I think I understand its idea. That the basic error would be the "fail-fast" one, but calling AllErrors() would underneath re-validate "everything" (though, presumably just at single level) with a true flag on the internal validate method. Calling AllErrors on nil should not be actually necessary I believe: if Validate() returns nil, this means there was not even one error, so certainly there are no multiple errors either.

This looks and feels weird, but I think it could work. Though it would then actually seem to put extra performance penalty on the "all errors" case, as IIUC it would have to revalidate potentially nearly all fields (assuming the error was e.g. on the last one). Which would become even bigger (exponential? :/) if one wanted to then call AllErrors() recursively, no?

Also, defining the ValidationError as you propose above is AFAIU unnecessary, should be completely equivalent to just writing error, and I find it confusing as to what is its purpose of existence (and whether it does or does not introduce an extra risk of the nil interface pitfall). I don't think there would be a need to define SingleValidationError either; it's enough to just define a type implementing an interface (think Python/duck-typing, or C++ concepts/templates) in Go, there's no need to define an interface in advance (as in Java interfaces/C++ abstract classes).

Please also note, that the signature of the Validate method in the Go code currently generated by protoc-gen-validate has a "loosely typed" return value already: Validate() error; and that in generated concrete error types, both the generated cause error field and generated Cause() error methods also use the error interface. That's normal and expected in Go.

Based on all of the above, and trying to stay as much as I can in spirit of what you propose above, I would currently propose an API like below:

type XxxMultiError []error

func (m XxxMultiError) Error() string {
    // here, either somehow concat all messages of all sub-errors, or just return the first one
}

func (m XxxMultiError) AllErrors() []error { return m }

func (m *XxxMessage) Validate(all bool) error {
    var errors []error
    // ...
    if /* field check failed */ {  // for message fields, call: err := m.Field.Validate(all)
        verr := XxxValidationError{ ... }
        if !all { return verr }
        errors = append(errors, verr)
    }
    // ...
    if len(errors) > 0 {
        return XxxMultiError(errors)
    }
    return nil
}

This would seem to me mostly in spirit of what you proposed above (e.g. having an AllErrors() method returning a list of errors), with no unnecessary interface declarations, and with performance optimization in a place I find easy to understand, i.e. at the argument to Validate. Do you have some concerns about any aspect of this proposal?

[kindermoumoute]

What prevent us to evolve the interface?

type Validator interface {
    Validate() error
    ValidateAll() []error
}

It could rely on the same private function validate() []error as described above.

Whatever is the implementation, can't wait for this feature to land 😄

[akavel]

@akonradi I've opened a PR; I'd be super grateful if there was some chance you might be able to find some time to take a look ❤️ I tried to split the PR in commits to make it as easy to read as I could think of — among others, by isolating the unavoidable many-file changes to separate commits.

I'm open to doing changes to this to make it in shape you'd like, as long as I can understand the idea, and don't see major blockers. For this PR I tried to pick an approach that seemed a good compromise to me, between the code required, performance, and readability. Still, I'm open to change to something else. I'd be really super grateful if you could take a look and help me find a solution that could be accepted into the project!

Notably, I considered other solutions (including interface { Validate() error; ValidateAll() []error }, the idea with Validate().AllErrors(), and a few more), but all of them seemed to lead to some unexpected trouble further down the road. I can try to remember them and elaborate if you would find that important. Still, given what you wrote that this is still alpha and everything (including the interfaces) might change in the future, I'd love if we could go with some solution now, and make it at least usable until a better idea/approach is fleshed out enough, so as to not let perfect be the enemy of good. Yet as I say, I'd love to change it if you prefer it some other way that I can do (as long as it would be still usable to me in our project).

Thanks!

[kindermoumoute]

Looks like this feature was rollbacked, as mentionned in the PR the following code will silently break :

type validator interface {
  func Validate() error
}

func check(m proto.Message) error {
  if v, ok := m.(validator); ok {
    return m.Validate()
  }
  return nil
}

One way to implement this feature could be to add a new method but keep the current one in place. Inspired by @danielhochman suggestion it could be something like:

type Validator interface {
    Validate() error
    ValidateWith(opts ...ValidateOption)
}

Any thought @akavel ?

[akavel]

Honestly, I'm confused by this rollback. If the library is assumed to be alpha-level and (as explicitly mentioned) open to bravely changing its API, it would make perfect sense to me if downstream users were expected to modify their validator interface definitions to take bool; this should normally happen in just a single place in every downstream codebase.

As to a two-method interface (Validate + ValidateWith/ValidateAll/... whatever other name of the 2nd method), the reason why I was concerned of taking this approach, is that as far as I can see, using it makes the implementation much more complex/involved. It would require implementing 2 methods for each type now, each of them with a different body. Then, for each of them, it must be decided if it should only handle the matching interface when recursing into fields, or the "cross-call" too? (i.e., would Validate only call Validate recursively, or ValidateWith too when Validate is not found? conversely, would ValidateWith only call ValidateWith on fields, or if not found, should it call Validate? If the answer to any of those is "yes", there's a difference in semantics, and it's questionable if it makes much sense then really.) I believe the templates would have to become quite more complex.

The above is already a concern with just Validate + ValidateAll. When using ValidateWith, as much as it may look smart on the surface, there are even more challenges: what opts are you actually interested in supporting? how do you want to implement them? An even more tricky aspect: up till now, the generated code carefully avoids importing anything outside of the Go standard library, and it even advertises that as a benefit IIRC. If you want to introduce ValidateOption, you must define it somewhere. Where do you want to do that? In a new non-stdlib package that you want to start importing now? I would assume the answer would probably be "yes" (I don't see it done any other way), however that's a point where you need to make this decision that this is a tradeoff you now want to take, and explicitly drop the (arguably not especially valuable) characteristic of "not importing anything outside stdlib". If you change that, you might as well do some other sweeping changes in how the validator types are being generated, given that they're really jumping through some hoops to avoid doing the imports (in particular by defining custom error type for each validated type). Do you want to make those changes in the same PR?

I wish people throwing around various interfaces in this thread tried to actually walk the walk and implement them, and see how quickly they lead to various nontrivial challenges. Many of them IMO are not worth the cost, that's why I chose Validate(bool). But if the maintainers decide on something, I can try going with a different design, for the good of the project and community. But I'd really like to talk this through enough then, that I don't end up putting a lot of effort into writing a dozen various implementations, that are promptly rejected one by one due to complexity or other issues, to eventually end up back at Validate(bool). Though what I fear most now, is that the maintainers will now fear that whatever they propose will not satisfy community, so this feature will just die in a corner, with everybody afraid to even talk about it anymore, and soon to be dealt a parting headshot by the stale-bot.

Personally, I highly respect the philosophy expressed in Google Code Review Guidelines, that perfect should not be the enemy of good. I see that as the only viable approach for community involvement in open-source. That's why I went with Validate(bool), which seemed to me a good incremental step for a project consciously advertising its alpha-stage API policy. I wish the people having issues with this feature tried to improve on it further to fix it for them, bravely going forward, not shying away from a change. Is there something we can do to try and take this forward?

edit: In other words, whatever improvement is done to a piece of software, there will always be someone complaining. If every such complaint is treated seriously, by definition a software would have to freeze and never change anything anymore. I hope that's not the strategy for this project.

[akonradi]

I'm happy to explain why I performed the rollback. As I've mentioned before, I'm not that familiar with Go or what would seem normal or preferable from an API standpoint regarding having a single or multiple methods. I also agree that PGV explicitly declares itself to be an alpha-level project that doesn't guarantee API compatibility, and that means we should feel free to break the API where it makes sense.

What doesn't make sense is changing the API in a way that can introduce security bugs for applications that don't follow "good Go practice". This kind of silent breakage is the worst kind, because it's so easy to introduce (simply auto-upgrading dependencies). I'm more than happy to create incompatibilities that require changes elsewhere, but helping users to unknowingly introduce security vulnerabilities is not good.

@akavel your work on advancing this is appreciated. I fully expect that the bulk of the implementation and test cases from #455 will not be wasted, and look forward to reviewing that PR.

[akavel]

@akonradi Thanks for the kind response, and sorry for my outburst. I'm really hopeful @danielhochman will try and open a new PR exploring an alternative implementation, as he seems to have hinted at in the discussion in #455. I will admit I do appreciate that a two-method interface has its set of benefits, though I personally wasn't able to invent a way of reconciling the challenges it comes with on the other hand, in a shape that would seem sensible to me. I'd love to be shown some approach I totally might have missed! and anyway I personally really need this feature in some working shape, so I'm kinda desperate for just something here (just hopefully not O(n^2) or worse).

[danielhochman]

I have a proof-of-concept, built on top of the existing work: https://github.com/danielhochman/protoc-gen-validate/commit/ca6f8851740f8d6e45e60a3aafa9a0936f01612a

Have not had a chance to test it just yet other than that the generated code compiles in my repo and successfully validates a message.

Now I need to decide whether to pursue the variadic option interface or simply introduce a ValidateAll() call. To add ValidateAll(), I would just make an unexported validateWith(all bool) method and call it with false and true respectively from Validate() and ValidateAll(). The big change here if we pursue the variadic option interface, that was previously called out by @akavel, is that the generated code will now need to import protoc-gen-validate in order to use the configuration option types. Based on my experience with other modules, this is not a problem, and I think it's the best way forward if we want to have an extensible interface rather than introducing additional methods each time. Of course we could delay that until next time we need to extend the interface. It would also open the door to simplifying the generated code in other ways that @akavel mentioned, such as not generating errors for each individual type, but we don't need to do that now.

// protoc-gen-validate
type ValidationOption func(*Options)

type Options struct {
    all bool
}

func All(o *Options) {
    o.all = true
}

// Generated code.
func (m *SomeMessage) ValidateWith(opts ...ValidationOption) {
    cfg := &validate.Options{}
    for _, opt := range opts {
        opt(cfg)
    }

    if cfg.all {
      ...
    } else {
      ...
    }
}

// Example call.
SomeMessage{}.ValidateWith(validate.All)

Based on https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis. I think it should also work for mismatched versions among multiple modules as long an option is never deleted from the struct.

I'll try to spend more time on this throughout the week. Open to any feedback on the direction I'm going though.

[akavel]

@danielhochman As to the ValidationOption approach, I'm especially interested what's your thought as to recursive calls on sub-messages: would each of them be passed opts, and repeat the for ... range opts preamble?

As to next steps, personally I'm heavily in favor of small incremental steps, especially in open-source projects. From my experience, I believe they tend to be easier to manage to get to a successful merge:

• less chance that a contributor or a maintainer gets distracted by something else,
• by virtue of being small, they're easier to review for maintainer(s) and to even find time to review (multiplied by the number of subsequent back & forth rounds of review),
• less surface to debate and bikeshed and come to a standstill over,

and maybe even some more reasons as well. As such, your suggestion of Validate + ValidateAll + the private method sounds like an attractive idea here to me; also, for my taste I find it more clear what it means than ValidateWith(bool), and I think it might be beneficial that it would leave the ValidateWith name unused, keeping it available for a potential future expansion towards the opts... pattern.

I'm eagerly waiting to hopefully see a PR as soon as possible 🙂 ❤️

[brandoncole]

@danielhochman I like where you're going with the functional options. Although the recursive calls may have a few repetitions of the preamble as @akavel points out, I feel like the overhead of that would be fairly minor. But if ValidationOption and Options and All are redefined in each package, that may be a bit redundant unless they can be defined centrally somehow.

Another option might be a new method as also discussed that just lets the caller get all the errors but possibly clamp the maximum number of errors returned which many other frameworks accomodate too:

func (m *SomeMessage) Validate() error {
    errors := m.ValidateWithErrorLimit(1)
    if len(errors) > 0 { return errors[0]; }
    return nil
}

func (m *SomeMessage) ValidateAll() []error {
    return m.ValidateWithErrorLimit(0)
}

// ValidateWithErrorLimit validates all fields and sub-messages until the specified limit of errors is encountered.
// If the limit is 0 then all errors are returned.
func (m *SomeMessage) ValidateWithErrorLimit(limit int) []error {
   // Validation Code
}

[danielhochman]

To add that feature I would definitely urge the move to functional options, pending benchmarking of the overhead of repeated option struct creation for embedded messages. Also, yes they would need to be centrally defined since users often validate arbitrary protos, but this shouldn't be a problem, most other proto plugins generate code with module imports. The end result would be message.ValidateWith(validate.Limit(10)).

func Limit(limit int) func(o *Options) {
    return func(o *Options) {
        o.limit = limit
    }
}

Anyways, let's not add that to the existing scope. I would vote that we open a separate issue to track limits once this one is closed.

[kindermoumoute]

This is implemented in Go but it could be extended to other languages as well.

[elliotmjackson]

Hey there, thanks for sharing your thoughts on improving validation error handling in protoc-gen-validate. Great news – we've gone ahead and implemented your suggestion in protovalidate.

Experience the enhanced user interface this change brings. While the v1 release of protoc-gen-validate moves it into maintenance mode, protovalidate offers even more. We're excited about the possibilities!

Closing this issue now. Feel free to reach out if you have any quick questions.