Validation on subschemas is not run unless parent schema is valid

[bengourley]

I want all of the validation errors for an object to presented as a summary to the user. I can't do this because schemata early-exists from the validation async flow at the existence of the first validation error.

https://github.com/serby/schemata/blob/8f88de59f38e24de350d5c006a742f78888e373b/lib/schemata.js#L276-L344

[serby]

I agree, but in what use-case is there validation on the parent when there is sub schema.

[bengourley]

var openingTimes =
  { monday: { type: Object, validators: { all: [ validateDay ] }, defaultValue: null }
  , tuesday: { type: Object, validators: { all: [ validateDay ] }, defaultValue: null }
  , wednesday: { type: Object, validators: { all: [ validateDay ] }, defaultValue: null }
  , thursday: { type: Object, validators: { all: [ validateDay ] }, defaultValue: null }
  , friday: { type: Object, validators: { all: [ validateDay ] }, defaultValue: null }
  , saturday: { type: Object, validators: { all: [ validateDay ] }, defaultValue: null }
  , sunday: { type: Object, validators: { all: [ validateDay ] }, defaultValue: null }
  , exceptions: { type: Array }
  }

var schema = schemata(
{ name: { type: String }
, openingTimes: { openingTimesSchema() }
}

[bengourley]

a valid object:

{ name: 'Uncle Ben\'s Widget Store'
, openingTimes:
  { monday: { open: '0900', close: '1700' }
  , tuesday: { open: '0900', close: '1700' }
  , wednesday: { open: '0900', close: '1700' }
  , thursday: { open: '0900', close: '1700' }
  , friday: { open: '0900', close: '1700' }
  , saturday: { open: '1000', close: '1600' }
  , sunday: null
  , exceptions: []
  }
}

[serby]

Sorry I was confused by your title: "Validation on subschemas is not run unless parent schema is valid"

Is it that Subschemas stop on the first validation error.

[bengourley]

No, worse than that, if the parent is invalid (in my example, eg. if name was required but missing) none of the subschemas even attempt to validate. It's because in the block of code I linked to, the error is passed to async's callback, and so it doesn't do any more of the functions in the series.

[kuba81]

I've been looking at this, but from what I see (https://github.com/serby/schemata/blob/8f88de59f38e24de350d5c006a742f78888e373b/test/schemata.test.js#L545-561) this is intended behavior. Would it be safe to change it?

The alternative is to introduce a boolean parameter to the validate() method, but that strikes me as bit messy. I was also considering adding another method like deepValidate(), but I didn't like that idea, either.

Or is there a more elegant way to solve this that I am missing?

[bengourley]

I think it would be safe to change, provided we do it with good reason. That's what semver is for after all.

The elegant solution for me is:

• default behaviour: descend the entire schema tree, running all top level and nested validatation
• optional behaviour: halt on first error. Incrementally run all validation and halt (and return) on the first error

[serby]

For the use case where a first level property validator checks that the value is not null. If this failed then it wouldn't make sense to descend into the subschema.

I thought the issue is that all sub schemas are not validated as soon as one first level property fails validation.

From Ben "No, worse than that, if the parent is invalid (in my example, eg. if name was required but missing) none of the subschemas even attempt to validate."

I'm pretty happy with that we stick with only descending when the parent property is valid.

[kuba81]

@bengourley, we looked into this with @serby yesterday, and we wrote a test that ensures top-level validation of one property does not prevent validation of other properties’ subschemas. Can you please have a look at it and confirm if this is what you needed? (https://github.com/serby/schemata/pull/14) The new test passed without making any changes to the existing code.

[bengourley]

No this is a false positive. I suspect it has to do with the order that the properties are validated with.

I've updated schemata on the application that is exhibiting the failure, and sure enough it still fails.

I believe the problem stems from here: https://github.com/serby/schemata/blob/master/lib/schemata.js#L306

If the 'errors' object has any properties at any time that line 306 is reached, the async.series will stop processing and call the error callback: https://github.com/serby/schemata/blob/master/lib/schemata.js#L349-L351

The properties are validated in an async.forEach which does things in parallel, so there's no guarantee about the validation order, which makes it hard.

[kuba81]

Hmm.. I tried running the validators in series by changing async.forEach to async.eachSeries just to see if it’s failing (https://github.com/serby/schemata/blob/master/lib/schemata.js#L278), but regardless of whether the failing top-level validator executed before or after the subschema validation, that validation still executed.

[bengourley]

Reduced test case: https://gist.github.com/bengourley/bd91686f8d8a767c3781

It looks like the combination of 'type' and 'validators' on properties causes the issue.

[serby]

I agree it, must be a problem.

We need to create a failing test case.

@kuba81 Can you create a fixture were all properties fail except that of the property with the subschema.

[bengourley]

Fixed.