Code review comment for lp:~mwhudson/lava-dispatcher/more-validation

> On Thu, 08 Mar 2012 09:23:19 -0000, Zygmunt Krynicki
> <email address hidden> wrote:
> > Review: Approve
> >
> > +1
> >
> > We should think how to validate commands better.
>
> Yes, good idea.
>
> > Perhaps we could attempt constructing them and calling per-command
> > validate()
>
> Have a look at what I just pushed. I worry a bit that some of the
> errors will be a bit opaque.

I will (just started reading now). I was thinking that we could expand the schema with custom error traps. This would allow us to keep validation to the schema _and_ provide good user experience. I have not thought much about implementation but I generally think that without general-purpose expressions (turing complete) - something I _don't_ want to offer we need to focus on node-level error traps and violation-specific error messages:

Imagine the following schema:

I want to validate a "cmd_lava_test_install" action. Here's the schema for that action. Let's start with the basic schema. No error handling (apart from validation)

{
    "type": "object",
    "additionalProperties": false,
    "properties": {
        "command": {
            "type": "string",
            "enum": "lava_test_install"
        },
        "tests": {
            "type": "array",
            "items": "string",
            "uniqueItems": true,
            "minItems": 1,
        },
        "install_python": {
            "optional": true,
            "type": "array",
            "items": "string",
            "uniqueItems": true,
            "minItems": 1,
        },
        "register": {
            "optional": true,
            "type": "array",
            "items": "string",
            "uniqueItems": true,
            "minItems": 1,
        }
    }
}

Now let's suppose we want to provide nice error message to all kinds of errors related to the "tests" property:

"lava-errors": {
    "optional": "lava_test_install action must define which tests to install using thest 'tests' property",
    "type": "lava_test_install 'tests' property must be an array of tests to install",
    "items": "lava_test_install 'tests' property must be an array of strings (where each string is a name of test to install)",
    "uniqueItems": "lava_test_install 'tests' property cannot contain duplicates",
    "minItems": "lava_test_install 'tests' property cannot be empty. If you don't want to install any tests then simply omit this action",
}

Each of those items would be used when an error would have been reported by the validator. Notice how this also shows the logical bug in the definition of the schema above. If I just want to install python/json tests then I cannot do it with tests.minItems = 1. Perhaps tests should be optional as well.

Anyway I think this can be very powerful. Remember that we can construct type unions (type may be an alternative of types) and that "dependencies" (http://tools.ietf.org/html/draft-zyp-json-schema-03#section-5.8) and "disallow" (http://tools.ietf.org/html/draft-zyp-json-schema-03#section-5.25) are super-powerful to fine tune everything. We could use it to provide conditional validation.

> One thing that is interesting here is that my immediate reason for doing
> this is that I want to do validation in the scheduler but the scheduler
> actually accepts a slightly different schema to the dispatcher -- for
> example, 'target' is optional for the scheduler but not at all for the

We can do that with "extends" (schema inheritance) but I'd have to check if it can loosen the specification. In any way I'm sure we can support that with a non-standard extension (like extends + loosen)

> dispatcher, and I'd like to make the 'server' parameter optional for the
> submit_results command. Not sure what to do about that though...

I think server should really go away. It should be just the pathname and we should really really really really merge the two.

I'll post the actual code review separately