apiaryio/gavel-spec

Define unified Gavel validation result shape

artem-zakharchenko opened this issue · 5 comments

artem-zakharchenko commented

We must define a unified validation result Object structure for Gavel.

Motivation

  1. To become independent from the JSON validators we use or may use. Any validator's output is coerced to the unified validation result structure, preventing us from breaking changes.
  2. To contain common data that is currently missing (i.e. actual/expected values of individual fields, normalize pointers format, adjust fields results to be easily printed out).
artem-zakharchenko commented

I find current structure of the validation result being in a good shape, yet there are some points I'd like to address in the improvements phase.

Refine validator being optional/required/necessary

Nevertheless, validator field is optional per spec.

result.fields[name].validator is used primarily in gavel2html to determine proper converter class. However, I have a strong feeling we can derive the converter from result.fields[name].realType, as it's the value that defines validator. In other words:

realType -> validator -> converter

# is the same as
realType -> converter

# on the condition that "validator" is not used for anything else

Exposing which validator is used is a rather implementation detail and I can't see it useful for the end user, as the validators themselves are not a part of Gavel's public API.

Refine rawData

Being marked as optional in the documentation, rawData seems to be used during result reporting (gavel2html). It's accessed there with a safety check, yet it would be nice to define if and why it's necessary. Perhaps, the same logic can be achieved using existing fields and we can ditch the rawData property.

rawData MUST not be coming as-is from a validator used (Amanda, etc.). We need to assess its usage and design it to be abstract.

Include expected and real values on the field level

I suggest for a field validation result to include its expected and real values:

interface FieldResult {
  validator?: string
  realType: string
  expectedType?: string

  values: {
    real: string
    expected: string
  }
}

Motivation:

  1. To allow values references to build custom messages from the validation result. At the moment the end user is left with the computed errors[n].message and has no way to get the expected/real values to include in the UI or any custom logic.
  2. To expose the resolved values, meaning values after coercion and normalization. This can help the end user to understand what is actually compared, as well as report us an issue in case coercion/normalization behaves unexpectedly.
artem-zakharchenko commented

Findings from gavel2html

text-result-converter

https://github.com/apiaryio/gavel2html/blob/8443b2dc87d86849ddb88341c103277367fade2f/src/text-result-converter.coffee#L14-L16

Expects dataError and diffError to come from Gavel.js.

dataError looks like an input validation error to me. That indeed should be performed by Gavel, but most likely not critical at the current moment.

honzajavorek commented

My comments:

  • removal of validator in favor of realType makes sense to me, also simplifies some code in gavel itself
  • I agree about rawData, I also think it is the most problematic and least understandable part (by someone not deeply familiar with gavel) of the validation result
  • Regarding values, I think it is a good idea, but we should have a way to somehow cap it or omit it in case it is of monstrous dimensions (think of passing the Gavel results around Apiary SaaS infrastructure, JSON-parsing and JSON-serializing the structure multiple times on the way, or being saved to database, etc.)
artem-zakharchenko commented

Regarding omitting the values property, that should be the responsibility of the storing surface (Core app, for example). However, we need to bear in mind that currently the dataReal and dataExpected are stored in the database (I presume), because they are the part of the gavel2html output.

We should verify where those diff chunks come from, but I wouldn't expect anything else but the database. In case that moving data to a proper domain (gavel result) shouldn't be a problem.

  • 👍1
ApiaryBot commented

🎉 This issue has been resolved in version 4.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀