Modularize validation

[artem-zakharchenko]

These changes are meant to modularize the logic in Gavel and replace classes architecture by functions and functional composition.

GitHub

• Relates to Refactor Gavel validators (non-breaking) #150
• Fixes Charset header comparisons should be case insensitive. #131

Roadmap

• Validation units
  • validateStatusCode
  • validateHeaders
  • validateBody
• Public API
  • validate
    • validateElement
  • isValid (out of scope)
  • isValidatable (out of scope)

Discoveries

• Make the same call signature of validator units (right now some accept relevant data, but some the entire real/expected transaction elements)
• Consider using TextDiff as a fallback body validator in case no matching validator is found
• Consider exporting and testing getBodyType and similar utils to prevent deep testing hierarchy. Assert validity of the validation response on validateBody, assert validity of real/expected body types in the respective utility unit tests. (8630403)
• Account for validation units like validateBody that need access to other fields of req/res (i.e. headers). It appears that normalization of transaction elements must happen before each individual validation unit is executed, meaning before invoking validateReq/validateRes.
• Handle headers normalization. Ensure it's an Object, fallback to empty Object if it's not set, and throw exception if it's rubbish (aef5079)
• Align export of validation units (some have default exports, some named, it's shuffled) (72354d4)
• Propagate the error arguments to the callback of validate (8ae92b4)
• Adjust getBodyType error on non-parsable JSON to be different for real/expected content types (e8a6406)
• Integrate new implementation into old unit tests (334f9ad)
• Move normalization of real/expected away from validate (dc8a743)
• Clean up old implementation and tests

[artem-zakharchenko]

Updated validateBody unit. I will highlight main changes below.

Tuples

During various validation phases we often push parsing/other errors to the results (errors field of the validation result). To preserve that logic while not dragging error handling into independent utils I have introduces tuples of [error, data] shape. For example:

gavel.js/lib/api/units/validateBody.js

Line 45 in d7495f0

const [error, mediaType] = parseContentType(contentType);

parseContentType may fail trying to parse a given content type, but the responsibility of handling that error is delegated to the parental scope. Based on the notion, we can handle thrown errors of different nature as intended. For example, push them to the public results:

gavel.js/lib/api/units/validateBody.js

Lines 186 to 197 in d7495f0

	const [realTypeError, realType] = getBodyType(real.body, real.headers);
	const [expectedTypeError, expectedType] = expected.bodySchema
	? getBodySchemaType(expected.bodySchema)
	: getBodyType(expected.body, expected.headers);
	if (realTypeError) {
	results.push(realTypeError);
	}
	if (expectedTypeError) {
	results.push(expectedTypeError);
	}

Parsed body media type

The main algorithmic difference introduced is the storage of body type as a parsed media type:

gavel.js/lib/api/units/validateBody.js

Lines 64 to 73 in d7495f0

	jph.parse(body);
	const [mediaTypeError, bodyMediaType] = parseContentType(
	hasJsonContentType ? contentType : 'application/json'
	);
	if (mediaTypeError) {
	throw new Error(mediaTypeError);
	}
	return [null, bodyMediaType];

This way we can get a relevant body validator based on already parsed body media type (previously we would have to parse body content type again):

gavel.js/lib/api/units/validateBody.js

Lines 138 to 154 in d7495f0

	const validators = [
	[TextDiff, both(isPlainText)],
	// List JsonSchema first, because weak predicate of JsonExample
	// would resolve on "application/schema+json" media type too.
	[
	JsonSchema,
	(real, expected) => {
	return (
	isJson(real) &&
	expected.type === 'application' &&
	expected.subtype === 'schema' &&
	expected.suffix === 'json'
	);
	}
	],
	[JsonExample, both(isJsonWeak)]
	];

Please, @honzajavorek, I would like to ask you to review getBodyValidator with extra cautious, since I've rewritten the logic, so I might have missed something. Let me know your opinion on the points above as well. Thanks!

[honzajavorek]

My main concern would be I didn't find enough evidence to support the case for tuples instead of exceptions.

Also we should go by trial & error and find out what the current behavior is regarding content types. I assembled a table according to my own assumptions, but it is incorrect according to how Gavel currently works. We should find out and test it and document it, perhaps even before reimplementing it.

[honzajavorek]

I see the PR description mentions it fixes an issue, but none of the 30 commits has a fix: classification. I guess the one which implements the lowercasing should have it.

[honzajavorek]

Why is this added? I don't see anything with leading underscores.

[artem-zakharchenko]

It's used here currently:

gavel.js/lib/api/units/validateBody.js

Line 140 in e260a4c

const validator = validators.find(([_name, predicate]) => {

We can always swap the order of arguments, but it feels as a wrong compromise imho. The order of arguments if a functional design decision, and alerts on unused variables is a developer-friendly reminder. I would prioritize the first one, but am always open to discussion :)

[honzajavorek]

I'm fine with it, but perhaps it doesn't deserve a global rule if it's at a single place :)

[honzajavorek]

Nitpick: res can sometimes mean response (often within express.js), here it's result. May be worth having the full word here, but it's a very minor thing, leaving up to you.

[ApiaryBot]

🎉 This PR is included in version 3.1.0 🎉

The release is available on:

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

Your semantic-release bot 📦🚀