search

brandur / json_schema

A JSON Schema V4 and Hyperschema V4 parser and validator.
MIT License
230 stars 45 forks source link

Configuration all_of_sub_errors makes allOf include sub-errors #84

Closed katsuya94 closed 7 years ago

katsuya94 commented 7 years ago

In our team's use of JsonSchema we found it useful to include the validation errors for allOf under sub_errors. As not to change the default behavior, which includes errors on the same level and stops after encountering the first error, this change is available via a configuration attribute that is false by default. Please take a look! Thank you.

brandur commented 7 years ago

Hi @katsuya94, this looks great! Thank you so much for sending in the patch, and sorry about the delay in getting around to it. I've also run into issues before where a failing allOf can be incredibly frustrating to debug.

I'm just thinking about the new configuration setting: it seems a little weird to me that anyOf get suberrors without configuration while allOf need for it to be enabled. I don't think adding suberrors for allOf would be a backward compatible change in any way, so what do you think about just enabling it by default? I'm wondering if there may be a good reason to disable it that I'm not considering.

katsuya94 commented 7 years ago

Hi @brandur thanks for your response. The only reason I thought to enable this as a configuration setting is for backwards compatibility, in case someone was relying on allOf not having sub-errors. If you feel comfortable moving forward with this as the default behavior I'd be happy to make the change.

The only other consideration is that there's a functional difference in that previously, if allOf encounters a failing subschema, it won't evaluate the remaining subschemas. This change will evaluate all subschemas even if it encounters a failure.

brandur commented 7 years ago

The only other consideration is that there's a functional difference in that previously, if allOf encounters a failing subschema, it won't evaluate the remaining subschemas. This change will evaluate all subschemas even if it encounters a failure.

Ahhh, very good point. I hadn't thought of the performance implications.

Alright, I'm comfortable merging this as in then. We can always make it default in the future and just leave the configuration option as a no-op.

Thanks!

brandur commented 7 years ago

Pushed as 0.15.0.