Introduces unified validation result structure

[artem-zakharchenko]

BREAKING CHANGE: Gavel validation result structure includes the next changes.

GitHub

• Closes Define unified Gavel validation result shape #48
• Closes Consider requirement keywords from RFC2119 #47
• Closes Consider "actual" vs "real" terminology #45

Changelog

Features

• Validity of an HTTP message and HTTP message fields is stored in the valid property

- result.isValid
+result.valid
-result.fields.body.isValid
+result.fields.body.valid

• Each HTTP message field contains the kind enum property describing the end value type:

result.fields.body.kind // "json"
result.fields.statusCode.kind // "text"

• Each HTTP message field error now contains a location property that stores information about the occurred error, specific to the kind value:

interface Error {
  message: string
  location?: {
    pointer: string
    property: string[]
  }
}

Deprecations

• Each HTTP message field no longer contains the next properties:
  • validator
  • expectedType
  • realType
  • rawData

[artem-zakharchenko]

I would also take this opportunity to refine certain specification aspects:

• Use common requirements vocabulary (Consider requirement keywords from RFC2119 #47)
• Refine the way spec features are declared, make sure we test behavior, not implementation
  • Spec doesn't enlist behaviors for invalid fields, only validators deviations (validators are rather an implementation detail)

[honzajavorek]

• I'm really not entirely sure we should follow RFC2119 here. As I mentioned in Consider requirement keywords from RFC2119 #47 (comment), Gherkins are pretty much binary, it doesn't make much sense to allow for various levels of possibility. The behavior should be described strictly, without room for doubt. This reflects in the changes made in this PR as well, as essentially everything got transformed to MUST or MUST NOT. I'd personally opt for a more human-friendly language: "then field body is valid" reads easier to me than "then field body MUST be valid" as I can focus on the behavior described, not on why there are uppercase words and what does it exactly mean. The same goes for "then field body is not valid" instead of "then field body MUST NOT be valid"

• I suggest we use "HTTP message" instead of "Request or Response" where both are possible

• This is perhaps a nitpick, but when I read "you define following expected HTTP request object" I wonder whether the word "define" actually describes the user scenario well. The object can often come from other places and I as a user almost never define it myself. Perhaps "you have following expected HTTP request" would make more sense? The "Given" phase is about given state, so I think it's fine to use "have", which doesn't imply how you got to the state of being in possession of the object.

[artem-zakharchenko]

as essentially everything got transformed to MUST or MUST NOT

This is because currently we don't state optional behaviors. For example, when describing a result.fields[name].errors[n].location property we can use "MAY" as it's exactly an optional property. We cannot write a strict assertion because it would fail in cases where location is not set and it's perfectly valid that way. It brings me to the question of how we write a spec. At the moment I wouldn't say it's evolving around features, as neither "request" not "response" are features of Gavel. Perhaps, we should reconsider how we describe the features.

At the same time if features are example-based, then in an example's scope even the optional property would be MUST. Hm.

I imagine the features are:

• validate() - high-level definition of how validation behaves. Must include failing scenarios too (currently only positives).
• validating [fieldName] - detailed explanation how validation of individual fields is performed. Describes required and optional properties, and explains when those are expected.

I would like to think about feature descriptions like docs (which they are, in fact). So the docs must mention the returned structures, and explain when and why certain properties are present or not. For example, currently the errors[n] properties are described like a feature description:

gavel-spec/features/validators.feature

Lines 8 to 12 in 0cb8b32

	**realType** - required - Media Type of real data
	**expectedType** - optional - Media Type of expected (example) data used for validation
	**validator** - optional - Data validator used for real and expected data comparison
	**errors** - required - Validation result errors and warnings
	**rawData** - optional - Raw output from the data validator

I suggest we integrate them into actual feature branches to have more concrete examples when certain properties appear and what they contain.

I suggest we use "HTTP message" instead of "Request or Response" where both are possible

Absolutely and extremely agree.

Perhaps "you have following expected HTTP request" would make more sense?

I don't see a reason why not. It does move away from the emphasis on define, which is not often the case (as you've mentioned).

[artem-zakharchenko]

@honzajavorek, also shouldn't Gavel CLI start to return the validation result object instead of terminating/not terminating the process? Process termination alone is not a reliable truth to assume that given HTTP messages are invalid/valid, as it may happen for various reasons. Especially when we started to introduce exceptions in Gavel (i.e. on malformed JSON Schema), the nature of process termination reasons expanded.

I advocate Gavel CLI returns the validation result Object for two reasons:

1. To align CLI and NodeJS usage expectations
2. Because validation result is already known

[honzajavorek]

shouldn't Gavel CLI start to return the validation result object instead of terminating/not terminating the process

Yeah, that's a valid point. My concern is I don't know how many people actually use that CLI. I think it's too cute to be killed, but I'm not convinced it is cute enough to be worked on/extended. Can we have this as a GitHub issue for future generations and wait if it gets any traction?

This is because currently we don't state optional behaviors. For example, when describing a result.fields[name].errors[n].location property we can use "MAY" as it's exactly an optional property. We cannot write a strict assertion because it would fail in cases where location is not set and it's perfectly valid that way. It brings me to the question of how we write a spec. At the moment I wouldn't say it's evolving around features, as neither "request" not "response" are features of Gavel. Perhaps, we should reconsider how we describe the features.

If location is optional property and we're about to assert it, then the feature should have two scenarios, one which illustrates on examples under which circumstances the property is there, and another one, which illustrates on examples under which circumstances the property is missing.

I would like to think about feature descriptions like docs...

So the docs must mention the returned structures, and explain when and why certain properties are present or not...

TL;DR I think we should have features per field. I'm not so sure we need to be specific about the properties in Gherkins (in Gavel.js reference docs, sure). It might make sense to be specific if we want to make sure any implementation of the spec produces the same result structure, but since it's just about Gavel.js now, we're good covering specific properties in the unit/integration tests and be more vague here. But I don't have very strong opinions on this, hence the "mmmmmh-there's-no-rule-of-thumb-I-don't-know-wall-of-text" below (sorry for that).

---

I understand features as docs (and tests) of how the thing is expected to behave. That means, I think "body validation" is a feature of Gavel, and the Gherkin file could describe in scenarios (illustrate on examples) how the validation is done - positive scenario, negative scenario, and explain corner cases of the behavior.

I don't think it necessarily needs to describe the input or output in details, unless it's useful (the explicitness of the input/output examples makes it more readable) or unless it describes public interface users directly interact with. But still, even in that case I'm more keen to describe things in abstract way, closer to the behavior itself than to implementation - e.g. buttons and form controls are a public interface user directly interacts with, but I prefer "when I send an email" rather than "when I click the Send button". I think all properties etc. definitely need to be in reference docs, but don't necessarily need to be directly a visible part of the features.

But in the end it really depends on what the "feature" in hand is. If the "feature" is more abstract, like sending an email, the scenario steps probably shouldn't be very specific regarding what is being clicked on or which property is where. If the "feature" is closer to implementation, like testing the button itself, then it's okay to have a scenario about whether it's red or green and how it changes its text.

In the particular case of Gavel validation result properties I think we should have features at least per one field (i.e. body validation, status code validation, etc.) with scenarios describing the possible situations (especially the corner ones, like validating text vs. json). Whether to directly mention the properties on output, I don't know. I'm fine having the steps saying just "isn't valid" or "it gets validated as (text|json)" where the underlying code inspects the kind property.

Thought exercise: If Gavel had more implementations then just Gavel.js, it might make sense to get specific about the public interface, but since the spec doesn't give you steps pre-implemented, like dredd-hooks-template does, it actually doesn't really matter. Implementation authors provide both implementation code and the steps code. As far as they comply with the behavior described, it is fine, regardless what properties they use to represent the result. Depends whether the spec wants to make sure the output is uniform for all implementations.

[artem-zakharchenko]

I've went through all the points and our design session, and the current state should reflect it.

@honzajavorek, could you please review it once more? Thank you.

Gavel's pull request is also ready according to this spec.

[honzajavorek]

I need to run away in the middle of the review, but will post what I have already. It all looks good so far, I have just one major suggestion regarding wording so the feature files read more nicely from the user perspective.

---

Note for myself: I've finished in the middle of features/javascript/fields/body.feature

[honzajavorek]

The subsequent steps mention HTTP message and "fields", but when I'm reading the file top down, it's not entirely clear where they come from.

Perhaps it could be more along the lines of the following?

Given I expect the HTTP message "body" to equal: ...  # background
And the actual "body" equals: ...
When Gavel validates the HTTP message
Then the HTTP message is NOT valid
And the "body" is NOT valid

With this you can just change "body" for what you need (headers, status code, ...) and it should still work. The fact that it is a field somewhere can be left out as an implementation detail.

[artem-zakharchenko]

I like the Given phrasing, but regarding omitting fields: I've tried to have a lego vocabulary so different assertions kinds do not need to introduce new steps. That way I've managed to delete the entire step_defs folder and stick up with just 3 small modules to describe steps without losing value from the tests themselves.

This means that the result field "name" is NOT valid and result field "name" equals: operate on the same vocabulary. If I omit the field or result part then assertions of the result will be unclear:

Then the HTTP message is NOT valid
And the "body" equals:
"""
# Assertion here
"""

I am specifically confused by the "body" equals part. It reads like it relates to the "body" I have declared in the scenario expectations, but it's actually a result body.

As a middle ground we could separate these two steps into:

- the "fieldName" is valid # simple field assertion
- the validation result for "fieldName" is: # asserts the result block

Do you find this solving the problem of reading in reverse?

[honzajavorek]

I've made a few comments or suggestions, but I found nothing so flawed that I'd hold the PR from being merged now. Good job!

[artem-zakharchenko]

Thanks for the review! All the comments addressed.

[honzajavorek]

[ApiaryBot]

🎉 This PR is included in version 4.0.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀