Address initial code review feedback

GoogleCloudPlatform · May 17, 2023 · f6a001e · f6a001e
1 parent d54252b
commit f6a001e
Show file tree

Hide file tree

Showing 9 changed files with 153 additions and 145 deletions.
diff --git a/src/CloudEventFunctionWrapper.php b/src/CloudEventFunctionWrapper.php
@@ -20,11 +20,15 @@
 
 use CloudEvents\V1\CloudEventInterface;
 use GuzzleHttp\Psr7\Response;
+use LogicException;
 use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\ServerRequestInterface;
+use ReflectionParameter;
 
-class CloudEventFunctionWrapper extends ValidatingFunctionWrapper
+class CloudEventFunctionWrapper extends FunctionWrapper
 {
+    use FunctionValidationTrait;
+
     private const TYPE_LEGACY = 1;
     private const TYPE_BINARY = 2;
     private const TYPE_STRUCTURED = 3;
@@ -50,6 +54,33 @@ public function __construct(callable $function, bool $marshalToCloudEventInterfa
     {
         $this->marshalToCloudEventInterface = $marshalToCloudEventInterface;
         parent::__construct($function);
+
+        $this->validateFunctionSignature(
+            $this->getFunctionReflection($function)
+        );
+    }
+
+    private function throwInvalidFunctionException(): void
+    {
+        $class = $this->getFunctionParameterClassName();
+        throw new LogicException(sprintf(
+            'Your function must have "%s" as the typehint for the first argument',
+            $class
+        ));
+    }
+
+    private function validateFirstParameter(ReflectionParameter $param): void
+    {
+        $type = $param->getType();
+        $class = $this->getFunctionParameterClassName();
+        if (!$type || $type->getName() !== $class) {
+            $this->throwInvalidFunctionException();
+        }
+    }
+
+    private function getFunctionParameterClassName(): string
+    {
+        return $this->marshalToCloudEventInterface ? CloudEventInterface::class : CloudEvent::class;
     }
 
     public function execute(ServerRequestInterface $request): ResponseInterface
@@ -145,9 +176,4 @@ private function fromBinaryRequest(
 
         return CloudEvent::fromArray($content);
     }
-
-    protected function getFunctionParameterClassName(): string
-    {
-        return $this->marshalToCloudEventInterface ? CloudEventInterface::class : CloudEvent::class;
-    }
 }
diff --git a/src/FunctionValidationTrait.php b/src/FunctionValidationTrait.php
@@ -0,0 +1,51 @@
+<?php
+/**
+ * Copyright 2019 Google LLC.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+namespace Google\CloudFunctions;
+
+use LogicException;
+use ReflectionFunctionAbstract;
+use ReflectionParameter;
+
+trait FunctionValidationTrait
+{
+    abstract function validateFirstParameter(ReflectionParameter $param): void;
+
+    abstract function throwInvalidFunctionException(): void;
+
+    private function validateFunctionSignature(
+        ReflectionFunctionAbstract $reflection
+    ) {
+        $parameters = $reflection->getParameters();
+        $parametersCount = count($parameters);
+
+        if ($parametersCount === 0) {
+            $this->throwInvalidFunctionException();
+        }
+
+        $this->validateFirstParameter($parameters[0]);
+
+        for ($i = 1; $i < $parametersCount; $i++) {
+            if (!$parameters[$i]->isOptional()) {
+                throw new LogicException(
+                    'If your function accepts more than one parameter the '
+                    . 'additional parameters must be optional'
+                );
+            }
+        }
+    }
+}
diff --git a/src/HttpFunctionWrapper.php b/src/HttpFunctionWrapper.php
@@ -21,9 +21,37 @@
 use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\ServerRequestInterface;
 use GuzzleHttp\Psr7\Response;
+use ReflectionParameter;
 
-class HttpFunctionWrapper extends ValidatingFunctionWrapper
+class HttpFunctionWrapper extends FunctionWrapper
 {
+    use FunctionValidationTrait;
+
+    public function __construct(callable $function)
+    {
+        parent::__construct($function);
+
+        $this->validateFunctionSignature(
+            $this->getFunctionReflection($function)
+        );
+    }
+
+    private function throwInvalidFunctionException(): void
+    {
+        throw new LogicException(sprintf(
+            'Your function must have "%s" as the typehint for the first argument',
+            ServerRequestInterface::class
+        ));
+    }
+
+    private function validateFirstParameter(ReflectionParameter $param): void
+    {
+        $type = $param->getType();
+        if (!$type || $type->getName() !== ServerRequestInterface::class) {
+            $this->throwInvalidFunctionException();
+        }
+    }
+
     public function execute(ServerRequestInterface $request): ResponseInterface
     {
         $path = $request->getUri()->getPath();
@@ -43,9 +71,4 @@ public function execute(ServerRequestInterface $request): ResponseInterface
             'Function response must be string or ' . ResponseInterface::class
         );
     }
-
-    protected function getFunctionParameterClassName(): string
-    {
-        return ServerRequestInterface::class;
-    }
 }
diff --git a/src/Invoker.php b/src/Invoker.php
@@ -75,6 +75,11 @@ public function __construct($target, ?string $signatureType = null)
         };
     }
 
+    public function setErrorLogger(callable $logFunc)
+    {
+        $this->errorLogFunc = $logFunc;
+    }
+
     public function handle(
         ServerRequestInterface $request = null
     ): ResponseInterface {
@@ -84,7 +89,7 @@ public function handle(
 
         try {
             return $this->function->execute($request);
-        } catch (ParseError $e) {
+        } catch (BadRequestError $e) {
             // Log the full error and stack trace
             ($this->errorLogFunc)((string) $e);
             return new Response(400, [], 'Bad Request');

diff --git a/src/TypedFunctionWrapper.php b/src/TypedFunctionWrapper.php
@@ -26,12 +26,13 @@
 use LogicException;
 use ReflectionClass;
 use ReflectionException;
-use ReflectionFunction;
 use ReflectionFunctionAbstract;
-use ReflectionMethod;
+use ReflectionParameter;
 
 class TypedFunctionWrapper extends FunctionWrapper
 {
+    use FunctionValidationTrait;
+
     public const FUNCTION_STATUS_HEADER = 'X-Google-Status';
 
     /** @var callable */
@@ -48,6 +49,29 @@ public function __construct(callable $function)
         );
     }
 
+    private function validateFirstParameter(ReflectionParameter $param): void
+    {
+        $type = $param->getType();
+        if ($type == null) {
+            $this->throwInvalidFunctionException();
+        }
+
+        try {
+            $this->functionArgClass = new ReflectionClass($type->getName());
+        } catch (ReflectionException $e) {
+            $name = $type->getName();
+            $message = $e->getMessage();
+            throw new LogicException("Could not find function parameter type $name, error: $message");
+        }
+    }
+
+    private function throwInvalidFunctionException(): void
+    {
+        throw new LogicException(
+            'Your function must declare exactly one required parameter that has a valid type hint'
+        );
+    }
+
     public function execute(ServerRequestInterface $request): ResponseInterface
     {
         try {
@@ -60,7 +84,7 @@ public function execute(ServerRequestInterface $request): ResponseInterface
         try {
             $argInst->mergeFromJsonString($body);
         } catch (Exception $e) {
-            throw new ParseError($e->getMessage(), 0, $e);
+            throw new BadRequestError($e->getMessage(), 0, $e);
         }
 
         $funcResult = call_user_func($this->function, $argInst);
@@ -72,50 +96,8 @@ public function execute(ServerRequestInterface $request): ResponseInterface
 
         return new Response(200, ['content-type' => 'application/cloudevents+json'], $resultJson);
     }
-
-    private function validateFunctionSignature(
-        ReflectionFunctionAbstract $reflection
-    ) {
-        $parameters = $reflection->getParameters();
-        $parametersCount = count($parameters);
-
-        // Check there is at least one parameter
-        if ($parametersCount === 0) {
-            $this->throwInvalidFunctionException();
-        }
-
-        // Check the first parameter has the proper typehint
-        $type = $parameters[0]->getType();
-        if ($type == null) {
-            $this->throwInvalidFunctionException();
-        }
-
-        for ($i = 1; $i < $parametersCount; $i++) {
-            if (!$parameters[$i]->isOptional()) {
-                throw new LogicException(
-                    'If your function accepts more than one parameter the '
-                        . 'additional parameters must be optional'
-                );
-            }
-        }
-
-        try {
-            $this->functionArgClass = new ReflectionClass($type->getName());
-        } catch (ReflectionException $e) {
-            $name = $type->getName();
-            $message = $e->getMessage();
-            throw new LogicException("Could not find function parameter type $name, error: $message");
-        }
-    }
-
-    private function throwInvalidFunctionException()
-    {
-        throw new LogicException(
-            'Your function must declare exactly one required parameter that has a valid type hint'
-        );
-    }
 }
 
-class ParseError extends Exception
+class BadRequestError extends Exception
 {
 }
diff --git a/src/ValidatingFunctionWrapper.php b/src/ValidatingFunctionWrapper.php
diff --git a/tests/FunctionsFrameworkTest.php b/tests/FunctionsFrameworkTest.php
@@ -61,7 +61,7 @@ public function testRegisterAndRetrieveCloudEventFunction(): void
 
     public function testRegisterAndRetrieveTypedFunction(): void
     {
-        $fn = function (IntValue $event) {
+        $fn = function (IntValue $event): IntValue {
             return $event;
         };