add middleware to validate JSON request body with schema

[abacaphiliac]

add JsonSchema middleware to validate request body using route-matched JSON schema files.

uses justinrainbow/json-schema:>=3.0. earlier versions resolve schema files differently and i chose not support it. i'd be happy to add support if requested.

route-matching is performed by prefix, e.g.:

Middleware::JsonSchema([
    '/en/v1/users' => WEB_ROOT . '/json-schema/en.v1.users.json',
    '/en/v1/posts' => WEB_ROOT . '/json-schema/en.v1.posts.json',
    '/en/v2/posts' => WEB_ROOT . '/json-schema/en.v2.posts.json',
]);

failed validation and malformed JSON will result in a short-circuited 422 response.

should the status code, message, and/or short-circuit behavior be configurable?

[abacaphiliac]

@oscarotero what do you think?

• bonus BodyParser tests : )

[oscarotero]

Mmm, I think I have not explained well. I mean to keep the body parsing process and the body validation process in two different components, so you have to do the following:

$middeware = [
    Middleware::payLoad(), //parse the body here
    Middleware::jsonSchema($config), //validate the json here
];

Currently, Payload has not options, and by default the parses the body with associative = true. So we need to add a new argument to pass options:

$middeware = [
    Middleware::payLoad(['assoc' => false]), //parse the body here passing options
    Middleware::jsonSchema($config), //validate the json here
];

[abacaphiliac]

@oscarotero thanks for the feedback. i have implemented it per your suggestion. i have some followup questions based on my implementation:

• do you think Payload should accept a concrete options object instead of a loose array?
• Payload passes the assoc option to the BodyParser transformer. should BodyParser also accept options (object or array) instead of a single value?
• BodyParser only uses assoc for JSON, but i feel that either the option should be renamed to jsonAssoc, or assoc should affect csv and form encoding as well. thoughts?

[oscarotero]

do you think Payload should accept a concrete options object instead of a loose array?

Because they are settings passed to BodyParser (do not affect directly to jsonSchema), I'd use an array.

Payload passes the assoc option to the BodyParser transformer. should BodyParser also accept options (object or array) instead of a single value?

I'd change the transformer interface to the following signature:

public function resolve($id, array $options = null);

So, it's easy to update the other transformers to match this interface with no breaking changes.

BodyParser only uses assoc for JSON, but i feel that either the option should be renamed to jsonAssoc, or assoc should affect csv and form encoding as well. thoughts?

I'm not sure. In theory, assoc is only for json bodies, but it is not about the input format but the output (stdObject instead array), so maybe could be useful for csv or urlencoded too.

[abacaphiliac]

i can update the signature of \Psr7Middlewares\Transformers\ResolverInterface::resolve to be public function resolve($id, array $options = null);, as you suggested, but i think it will still be a compatibility break.

the change will require an update to \Psr7Middlewares\Transformers\Resolver::resolve and \Psr7Middlewares\Transformers\BodyParser::resolve which i would take care of in this patch, but if someone else has created their own transformer then they will get a fatal error after upgrading the package version without updating their source code: e.g. PHP Fatal error: Declaration of MyMiddlewareApplication\Transformers\MyResolver::resolve() must be compatible with Psr7Middlewares\Transformers\ResolverInterface::resolve($id, array $options = NULL).

the assoc option feels like an implementation detail of the BodyParser transformer which is why i think it is better suited as a constructor param rather than an interface param. i think we should be able to avoid the BC break.

[oscarotero]

Ok, fair enought. +1 to add this to the constructor.

[abacaphiliac]

thanks! then i will add $options to the BodyParams constructor, instead of the scalar value defined currently. should i implement some form of !$assoc for csv and form transformations? or shall we defer that until we have the use case?

[oscarotero]

Right now, BodyParser always returns an array, so maybe a more json_decode independent option is better, so it can be supported also by csv and urlencode parsers. For example:

public function __construct($forceArray = true)

[abacaphiliac]

ah, i do like that name better. i have also updated the BodyParams transformer to accept an extensible array of options, just like the Payload middleware which simply passes the options through.

would you prefer to have BodyParams accept a single param?

[oscarotero]

no, an array is good, to make it extensible. 👍

[abacaphiliac]

anything else you'd like me to do here?

[oscarotero]

Sorry for the delay. Good job, thank you! 👍

[abacaphiliac]

no problem. i thought maybe i had forgotten something.

thank you for letting me contribute to such a helpful library.

[abacaphiliac]

hi @oscarotero , i had a followup conversation with another dev that i greatly respect and trust. he pointed something out in this merge request that i feel compelled to bring to your attention.

the combination of json schema validation with routing encourages duplicate route specification in applications. i think i could easily split a simple single-schema validator from the existing implementation. this new validator would accept a schema as a constructor param. the existing validator-per-route implementation could be safely refactored to delegate to the simple validator. these two independent validators will give developers greater control.

if any of that sounds good to you, or gives you some other ideas, i'll get right to work. i'll do my best to maintain compatibility. the name of the component i've outlined is the most challenging piece. i almost wish i had named the existing component JsonSchemaMulti. ah well. what do you think?

[sander-bol]

@abacaphiliac As a user of this library, your last comment makes a lot of sense to me... having to split the route definition across multiple places (the routes list, and then duplicate it into the middleware definition) feels like definitions-out-of-sync related bugs waiting to happen. Thanks for taking the alternative into consideration!

[oscarotero]

I'm fully agree with this but I think we should create a new different middleware to keep backward compatibility, let's say, for example, JsonValidator. @abacaphiliac if you want to work on this, this contribution will be greatly appreciated 😃

[abacaphiliac]

@sbol-coolblue @oscarotero thank you for the feedback. here is my preliminary work on the solution: https://github.com/oscarotero/psr7-middlewares/pull/46

i need some more time to review myself, and to update docs, but wanted to get early feedback from you. let me know what you think.