split a simpler json-schema validator from the route-based-validator #46

abacaphiliac · 2016-11-07T15:22:13Z

split a simpler json-schema validator from the multi-validator. the simple validator handles a single schema and there is no route-based logic. the file-per-route implementation consumes the simple validator.

abacaphiliac · 2016-11-07T15:22:34Z

this is preliminary work. i need to update documentation.

oscarotero

Hi. I gave you some feedback :)

oscarotero · 2016-11-07T19:22:45Z

src/Middleware/JsonSchema.php

@@ -30,31 +29,15 @@ public function __construct(array $schemas)
     *
     * @return ResponseInterface
     */
-    public function __invoke(ServerRequestInterface $request, ResponseInterface $response, callable $next)
+    public function __invoke(ServerRequestInterface $request, ResponseInterface $response, callable $next = null)


$next cannot be null, this argument is mandatory.

oscarotero · 2016-11-07T19:23:14Z

src/Middleware/JsonValidator.php

+     * @throws \InvalidArgumentException
+     * @throws \JsonSchema\Exception\ExceptionInterface
+     */
+    public function __invoke(ServerRequestInterface $request, ResponseInterface $response, callable $next = null)


As said before, $next is required

oscarotero · 2016-11-07T19:27:55Z

src/Middleware/JsonSchema.php

-                    json_encode($validator->getErrors(), JSON_UNESCAPED_SLASHES)
-                );
+            $validator = new JsonValidator($schema);
+            $response = $validator($request, $response);


If this middleware is the same than JsonValidator but handle multiple schemes, once the scheme has been choosen, why not simply execute the following?

return $validator($request, $response, $next);

abacaphiliac · 2016-11-08T00:35:04Z

@oscarotero thanks for the review! i've added a new commit to require $next. this enforcement allowed me to remove two branches in the validator middlewares.

i still need to add documentation, and when this is all done i will squash the commits. how does the latest change set look?

…imple validator handles a single schema and there is no route-based logic. the file-per-route implementation consumes the simple validator. corrected some false-positives in the json-schema tests.

abacaphiliac · 2016-11-09T00:00:15Z

@oscarotero updated docs. i think my change set is stable, unless you have some other feedback.

let me know if the changes to the json-schema test make you nervous. i can split this request. i noticed that there were some false positives, and i had forgotten to update the test's assoc config after we renamed it : /

sander-bol · 2016-11-10T10:14:04Z

README.md

+    ]),
+
+    // Specify a JSON file (publicly-accessible in this example), or a JSON string decoded into object-notation.
+    Middleware::jsonValidator((object) [


I think you're pushing quite a bit of implementation detail to the middleware definition here... I'd prefer to just push a reference to a file (either SplFileObject or a string to the path), rather than deal with the specifics of how the chosen Schema Validator needs to receive the schema.

great point. the implementation uses justinrainbow/json-schema, which can accept a file ref or a decoded schema. so your validator factory could create a schema inline, without using any files or file ops. do we really want to enforce file only?

oh, i could create a couple of named factories, one for a decoded object, and another that accepts SplFileObject (great suggestion).

sander-bol · 2016-11-10T10:15:44Z

README.md

+        'forceArray' => false,
+    ]),
+
+    // Specify a JSON file (publicly-accessible in this example), or a JSON string decoded into object-notation.


Comment does not match the method signature... the case of specifying a file is not supported.

the underlying lib accepts the ref as an object, e.g.

(object) [ "$ref" => "path/to/schema.json", ]

sander-bol · 2016-11-10T10:20:32Z

src/Middleware/JsonValidator.php

+                [
+                    'Content-Type' => 'application/json',
+                ],
+                json_encode($validator->getErrors(), JSON_UNESCAPED_SLASHES)


You're assuming the API will be providing a JSON response here, which is not necessarily true for all cases, even if we're in the business of receiving JSON. I'd be very surprised as a developer if a middleware layer suddenly decided to switch the content-type around. Think for example about people using JSON-API, which specifies a content type of Content-Type: application/vnd.api+json.

@oscarotero Does the project have a "best practice" way of dealing with generating responses to match the Accept headers provided by the user?

Maybe the response generation can be configured in a similar way than errorHandler or shutdown, providing a callable handler:

$errorResponse = function ($request, $response) { $validator = JsonValidator::getValidator($request); $response = $response->withHeader('Content-Type', 'application/json'); $response->getBody()->write(json_encode($validator->getErrors(), JSON_UNESCAPED_SLASHES)); }; $middlewares[] = Middlewares::JsonValidator()->errorHandler($errorResponse);

And if no custom errorHandler is provided, use the default.

ok thanks, i'll try this out. great suggestions @sbol-coolblue and @oscarotero .

…stronger API. creation of the validator by file or json string (for example) is more obvious. updated docs.

…w clone. see https://travis-ci.org/oscarotero/psr7-middlewares/builds/175723305.

sander-bol · 2016-11-14T14:59:26Z

Thanks for the improvements - really appreciate them.

abacaphiliac · 2016-11-14T15:01:19Z

@sbol-coolblue no problem! i really appreciate the feedback. my builds are failing right now because of some issue with the composer dependency versions. i'll get it working and then we can all discuss. also, i still need to update the error handler. i haven't forgotten.

abacaphiliac · 2016-11-14T15:11:37Z

@sbol-coolblue @oscarotero i made some changes based on your feedback last week. the build failed because a composer dependency was not able to be installed. see the build output:
https://travis-ci.org/abacaphiliac/psr7-middlewares/builds/175723278

i was able to resolve by changing "nikic/fast-route": "0.*" to "nikic/fast-route": "^0.7", so i think something happened recently with nikic/fast-route or with league/route. if you're interested, i will submit the version change in a separate pull.

i'll also update the error handler this week : )

sander-bol · 2016-11-15T09:28:01Z

Running into the same issue. I'll create a PR for the fast-route change.

oscarotero · 2016-11-15T19:48:58Z

Oops!! I've merged this by mistake.
Can you create a new pull request for the remaining things? (the error handler and that stuff)
Sorry :(

abacaphiliac · 2016-11-15T20:12:01Z

@oscarotero you got click-happy! yes it's not a problem at all. i think the error-handler changes can be done in a compatible fashion. do you mind holding off on the tag until i can complete this work? would take the pressure off of me haha.

oscarotero · 2016-11-15T20:18:34Z

Don't worry. I have not planned a new release in this week, there are still some things to do.
Thanks

abacaphiliac · 2016-11-15T20:25:57Z

no problem. i'll start working on it tonight. do you have a documented release schedule?

oscarotero · 2016-11-15T20:52:38Z

No really, but I'd like to close other issues like #50 before tagging

abacaphiliac mentioned this pull request Nov 7, 2016

add middleware to validate JSON request body with schema #44

Merged

oscarotero reviewed Nov 7, 2016

View reviewed changes

split a simpler json-schema validator from the multi-validator. the s…

388071a

…imple validator handles a single schema and there is no route-based logic. the file-per-route implementation consumes the simple validator. corrected some false-positives in the json-schema tests.

sander-bol reviewed Nov 10, 2016

View reviewed changes

abacaphiliac added 2 commits November 14, 2016 07:45

create a set of static JsonValidator factories with a consistent and …

4358d38

…stronger API. creation of the validator by file or json string (for example) is more obvious. updated docs.

nikic/fast-route:0.* prevents composer update from succeeding on a ne…

5b09668

…w clone. see https://travis-ci.org/oscarotero/psr7-middlewares/builds/175723305.

sander-bol mentioned this pull request Nov 15, 2016

Update fast-route dependency #53

Merged

oscarotero merged commit 04484f5 into oscarotero:master Nov 15, 2016

abacaphiliac mentioned this pull request Nov 17, 2016

allow JSON-validator default error-handler to be overridden. #58

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

split a simpler json-schema validator from the route-based-validator #46

split a simpler json-schema validator from the route-based-validator #46

abacaphiliac commented Nov 7, 2016

abacaphiliac commented Nov 7, 2016

oscarotero left a comment

oscarotero Nov 7, 2016

oscarotero Nov 7, 2016

oscarotero Nov 7, 2016

abacaphiliac commented Nov 8, 2016

abacaphiliac commented Nov 9, 2016

sander-bol Nov 10, 2016

abacaphiliac Nov 10, 2016

abacaphiliac Nov 10, 2016

sander-bol Nov 10, 2016

abacaphiliac Nov 10, 2016

sander-bol Nov 10, 2016 •

edited

Loading

oscarotero Nov 10, 2016

abacaphiliac Nov 10, 2016

sander-bol commented Nov 14, 2016

abacaphiliac commented Nov 14, 2016

abacaphiliac commented Nov 14, 2016

sander-bol commented Nov 15, 2016

oscarotero commented Nov 15, 2016

abacaphiliac commented Nov 15, 2016

oscarotero commented Nov 15, 2016

abacaphiliac commented Nov 15, 2016

oscarotero commented Nov 15, 2016

split a simpler json-schema validator from the route-based-validator #46

split a simpler json-schema validator from the route-based-validator #46

Conversation

abacaphiliac commented Nov 7, 2016

abacaphiliac commented Nov 7, 2016

oscarotero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abacaphiliac commented Nov 8, 2016

abacaphiliac commented Nov 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sander-bol Nov 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sander-bol commented Nov 14, 2016

abacaphiliac commented Nov 14, 2016

abacaphiliac commented Nov 14, 2016

sander-bol commented Nov 15, 2016

oscarotero commented Nov 15, 2016

abacaphiliac commented Nov 15, 2016

oscarotero commented Nov 15, 2016

abacaphiliac commented Nov 15, 2016

oscarotero commented Nov 15, 2016

sander-bol Nov 10, 2016 •

edited

Loading