Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JsonSchemaExceptionHandlerInterface doesn't catch JsonException in requests #199

Open
sosuke-ito opened this issue Oct 4, 2019 · 2 comments

Comments

@sosuke-ito
Copy link
Contributor

JsonSchemaExceptionHandlerInterface handles JsonExceptions on ill-formed responses but not on ill-formed requests.

Is this behavior intentional?

Example

For example, in this test case

public function testParameterException()
{
$caughtException = null;
$ro = $this->getRo(FakeUser::class);
try {
$ro->onGet(30, 'invalid gender');
} catch (JsonSchemaException $e) {
$caughtException = $e;
}
$this->assertEmpty($ro->body);
$this->assertInstanceOf(JsonSchemaException::class, $caughtException);
}

we get JsonException when we provide $gender = 'invalid gender' even if we bind a handler to JsonSchemaExceptionHandlerInterface.

I guess this is intentional, because ill-formed requests normally cannot continue and end up with abort. We might need handlers for this if we want to return some error-reason-responses or specify errors in response headers.

Proposal

If this is NOT intentional, we may want to handle JsonExceptions thrown in the request validation here:

public function invoke(MethodInvocation $invocation)
{
/** @var ReflectionMethod $method */
$method = $invocation->getMethod();
/** @var JsonSchema $jsonSchema */
$jsonSchema = $method->getAnnotation(JsonSchema::class);
if ($jsonSchema->params) {
$arguments = $this->getNamedArguments($invocation);
$this->validateRequest($jsonSchema, $arguments);
}
/** @var ResourceObject $ro */
$ro = $invocation->proceed();
if ($ro->code === 200 || $ro->code == 201) {
$this->validateResponse($ro, $jsonSchema);
}
return $ro;
}
private function validateRequest(JsonSchema $jsonSchema, array $arguments)
{
$schemaFile = $this->validateDir . '/' . $jsonSchema->params;
$this->validateFileExists($schemaFile);
$this->validate($arguments, $schemaFile);
}

Although I've got no good idea. Like this? (maybe BC-break):

diff --git a/src/JsonSchema/Interceptor/JsonSchemaInterceptor.php b/src/JsonSchema/Interceptor/JsonSchemaInterceptor.php
index 024a9e4..d3d2113 100644
--- a/src/JsonSchema/Interceptor/JsonSchemaInterceptor.php
+++ b/src/JsonSchema/Interceptor/JsonSchemaInterceptor.php
@@ -63,11 +63,11 @@ final class JsonSchemaInterceptor implements MethodInterceptor
         $method = $invocation->getMethod();
         /** @var JsonSchema $jsonSchema */
         $jsonSchema = $method->getAnnotation(JsonSchema::class);
-        if ($jsonSchema->params) {
-            $arguments = $this->getNamedArguments($invocation);
-            $this->validateRequest($jsonSchema, $arguments);
-        }
         /** @var ResourceObject $ro */
+        $ro = $this->validateRequest($jsonSchema, $invocation);
+        if ($ro->code === 400) {
+            return $ro;
+        }
         $ro = $invocation->proceed();
         if ($ro->code === 200 || $ro->code == 201) {
             $this->validateResponse($ro, $jsonSchema);
@@ -76,11 +76,23 @@ final class JsonSchemaInterceptor implements MethodInterceptor
         return $ro;
     }

-    private function validateRequest(JsonSchema $jsonSchema, array $arguments)
+    private function validateRequest(JsonSchema $jsonSchema, MethodInvocation $invocation)
     {
+        $ro = $invocation->getThis();
+        if (! $jsonSchema->params) {
+            return $ro;
+        }
         $schemaFile = $this->validateDir . '/' . $jsonSchema->params;
         $this->validateFileExists($schemaFile);
-        $this->validate($arguments, $schemaFile);
+        $arguments = $this->getNamedArguments($invocation);
+        try {
+            $this->validate($arguments, $schemaFile);
+        } catch (JsonSchemaException $e) {
+            $ro->code = 400;
+            $this->handler->handle($ro, $e, $schemaFile);
+        }
+
+        return $ro;
     }

     private function validateResponse(ResourceObject $ro, JsonSchema $jsonSchema)
@koriym
Copy link
Member

koriym commented Oct 21, 2019

@sosuke-ito Sorry for the late response.
What's your use case in practice ? I have no objection to handle bad request with JsonSchemaExceptionHandlerInterface. But I would like to dig deeper beforehand.

@koriym
Copy link
Member

koriym commented Oct 25, 2019

@sosuke-ito ping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants