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?