Fix the schema validation API

[vbatts]

From #491 (comment)

an API like this:

if err := validate(content); err != nil {
  for _, warning := range Warnings(err) {
    // do something with warnings
  }
}

We could define interfaces like this:

type Warnings {
  Warnings() []error
}

type Errors {
  Errors() []error
}

Either of these could be asserted on the error returned from the validate function. We can define helpers for common cases. For example, the below would not take the error path when there are just warnings:

if err := validate(content); err != nil && IgnoreWarnings(err) {
   // handle hard errors
}

[wking]

On Thu, May 25, 2017 at 01:25:00PM -0700, v1.0.0.batts wrote: type Warnings { Warnings() []error } type Errors { Errors() []error }

I don't think this is the best approach because distinguishing errors vs. warnings depends on a number of parameters (opencontainers/image-spec#66). I think the API in flight with opencontainers/runtime-tools#354 is better, because you can extend ComplianceLevel to provide additional granularity if you decide you want a way to error out on “can image-tools unpack this content?” or some such condition that is not a spec-related issue. Of course, image-tools could wrap the Warnings/Errors approach with something like opencontainers/runtime-tools#354, but there would be no structured way to extract the compliance level (short of parsing the string and assuming that image-spec always set it?) or the reference URL for the violated condition.

[stevvooe]

@wking That PR is way too literal and the API is a complete mess for users to consume. opencontaienrs/runtime-tools#354 also ignores better advice on go error handling.

The approach proposed above is easy to consume and ensures correctness. There is no reason it can't be adapted to handle multiple levels, but for the most part, errors can be divided into terminal and non-terminal (warnings). Making it any more complicated serves no one.

[stevvooe]

Looking at the code, we are still extremely broken for 1.0. Here, we are printing to stdout, so we can't even use this in any of our tooling:

https://github.com/opencontainers/image-spec/blob/master/schema/validator.go#L114

We should clarify that 1.0 does not guarantee that Go API won't change. It only covers the specification.

[vbatts]

should that be clarified in the ./schema/README.md? or where?

…

On Tue, Jul 18, 2017 at 4:04 PM, Stephen Day ***@***.***> wrote: Looking at the code, we are still extremely broken for 1.0. Here, we are printing to stdout, so we can't even use this in any of our tooling: https://github.com/opencontainers/image-spec/blob/master/schema/validator. go#L114 We should clarify that 1.0 does not guarantee that Go API won't change. It only covers the specification. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#686 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEF6U99k-iCLifPxRGLCy2yDUqZA2IHks5sPQ_QgaJpZM4Nm2OJ> .

[stevvooe]

@vbatts Somewhere where when we break, it is clear. Perhaps, the godoc and have a table on the README that indicates which parts are currently strongly versioned.

[xiekeyang]

I would like to clear some questions about the design idea #686 (comment) .
As comment on #712 (comment), I have some question about the design :

Question 1

if err := validate(content); err != nil {
  for _, warning := range Warnings(err) {
    // do something with warnings
  }
}

if err := validate(content); err != nil && IgnoreWarnings(err) {
   // handle hard errors
}

The API function validate returns only one common error. It looks next handlers will be done by consumer, and are decoupled from validation API. So the validation API is a little weakly (at least customized method should likely run inside validate):

func validate(content) error{
  err:=validMediatypeMap(content);
  err=gojsonschema.Validate()
  ...
  return err
}

Question 2

And to this code line: for _, warning := range Warnings(err): how Warnings(I think it is customized function) get error list from one error?

Question 3

type Warnings {
  Warnings() []error
}

type Errors {
  Errors() []error
}

Related to Q1, if validation API just need validate problems and return 1 error, why it need define interface?

---

So, please consider the design framework idea:

Point 1:

validMediatypeMap returns []error (for all problems it encounter), instead error.

func validateDescriptor(r io.Reader) []error

func validateConfig(r io.Reader) []error

func validateManifest(r io.Reader) []error

The returned error list can be used as argument of Warnings interface and Errors interface. That allows consumer handle these errors at his will.

Point 2:

Define Warnings and Errors interface like:

type Warnings interface{
  Warnings([]error) error
}

type Errors interface{
  Errors([]error) error
}

Consumer implement the real methods at his side and at his will, and it can be run inside validation API:

func (v Validator) Validate(src io.Reader, warns Warnings, errors Errors) error{
  ...
  errs := validateMediatypeMap(src)
  err := warns.Warnings(errs)
  err = errors.Errors(errs)
  ...
} 

Above design brings benefit:

1. validation media-typed map returns all problems as common error, be compatible to spec;
2. Consumer must use uniform interface, with customized methods;
3. Validate API input freely(interface), and return simply(error);
   This design left freely handle to consumer, and make sure internal validation strictly.

@stevvooe Excuse me my comment is a little verbose, if you are unsatisfied, please just detail your idea, and I will figure the better implementation.

[stevvooe]

@xiekeyang The biggest thing to remember is that error is a value and that value can be made up of a slice. Take this simple example:

type Errors []error

func (e Errors) Error() string { ... }

This means that Errors can be both a []error and an error, such that a Validate function can return error which can be unpacked to a list of errors.

[wking]

On Thu, May 25, 2017 at 02:04:28PM -0700, W. Trevor King wrote:

I think the API in flight with opencontainers/runtime-tools#354 is better…

@stevvooe didn't like that API the last time he looked at it, but opencontainers/runtime-tools#354 landed last night. I've just filed opencontainers/runtime-tools#437 to pull it out into its own generic package, so you could import it from runtime-tools without adding other dependencies outside the stdlib.

[zhouhao3]

@vbatts @wking @stevvooe @xiekeyang How about using logrus.warnf to handle warnings?

[xiekeyang]

@q384566678
For this issue we should make consensus the Errors and Warns type with required methods firstly.

In specific API verifications, we should determine which errors should be ignored, and which ones should be returned.

Then we should implement the logging system, and specify the output dev(as stderr or stdout) in it.

logrus seems not to be what we care about now.

[dontlaugh]

What kind of API do we want here? Has your thinking changed at all since this issue was opened?

Maximum Backwards Compatibility: A new interface?

What follows are some sketches. The names are intentionally bad.

To keep everything the same for current users - both errors and println side effects - it seems like we need a new interface.

type Validator2 interface {
    Validate2(io.Reader) (*Result, error)
}

type Result struct {
    Warnings []Warning
    Errors []Error
    // add fields here until the end of time...
}

A big old Result struct could give you a future proof-ish place to hang any additional info you might want to surface.

In this case, you have to decide whether you want validation errors to be returned in the error variant at all. I can see it both ways:

• If validation errors return an error - like they do now - that's a less surprising API
• If they do not return an error, and instead the error variant is reserved for low-level weirdness like, say, failing to initialize the virtual filesystem or something, the caller needs to know that this is the case. It becomes more like http responses, where a nil error still requires the caller to check the response code.

You could mitigate this confusion with an additional return param.

type Validator3 interface {
    Validate3(io.Reader) (*Result, bool, error)
}

The boolean here could be an indication that some field of Result demands inspection, whether Warning or Error. For Go programmer, 3 return params is a pretty nice incentive to go read the docs ("Why is this 3 params?" etc).

Okay, that's a little complicated and kludgy! Is there another interface that can surface warnings?

Perhaps the caller can pass options to control validation behavior.

type Validator4 interface {
    Validate4(io.Reader, ...Option) error
}

type ValidationError struct {
    Errors []Error
    Warnings []Warning
}

If I care about warnings, I ask for the validator to treat them as errors. Kind of like -Werror for gcc.

if err := v.Validate4(buf, WarningsAreErrors()); err != nil {
    // err is not nil even if there are only warnings
}

While it's possible to default to always returning a non-nil error, even for a warning, this seems like it will be too common for an error variant, especially one that might get populated to non-validation errors.

[sudo-bmitch]

I'm tempted to make a ValidateWithOpts and pass an options struct:

if err := v.ValidateWithOpts(reader, Opts{Strict: true}); err != nil { ... }

That lets us remove any writes to stdout/stderr, the existing Validate becomes a wrapper with default opts, and consumers can decide whether they want to treat these other scenarios as errors. We could also add an error type to use with errors.Is, and join multiple errors with errors.Join, but the latter would trigger a bump of our minimum Go version.