BREAKING/BUGFIX: Strict coercion of scalar types (#278)

webonyx · Aug 29, 2019 · 975c9fe · 975c9fe
1 parent ef8e3b3
commit 975c9fe
Show file tree

Hide file tree

Showing 13 changed files with 252 additions and 343 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,6 +1,7 @@
 # Changelog
 
 ## Unreleased
+- **BREAKING/BUGFIX:** Strict coercion of scalar types (#278)
 - **BREAKING:** Removed deprecated directive introspection fields (onOperation, onFragment, onField)
 - **BREAKING:** Removal of `VariablesDefaultValueAllowed` validation rule. All variables may now specify a default value.
 - **BREAKING:** renamed `ProvidedNonNullArguments` to `ProvidedRequiredArguments` (no longer require values to be provided to non-null arguments which provide a default value).

diff --git a/src/Type/Definition/BooleanType.php b/src/Type/Definition/BooleanType.php
@@ -21,23 +21,15 @@ class BooleanType extends ScalarType
     public $description = 'The `Boolean` scalar type represents `true` or `false`.';
 
     /**
-     * Coerce the given value to a boolean.
+     * Serialize the given value to a boolean.
      *
      * The GraphQL spec leaves this up to the implementations, so we just do what
      * PHP does natively to make this intuitive for developers.
      *
      * @param mixed $value
-     *
-     * @throws Error
      */
     public function serialize($value) : bool
     {
-        if (is_array($value)) {
-            throw new Error(
-                'Boolean cannot represent an array value: ' . Utils::printSafe($value)
-            );
-        }
-
         return (bool) $value;
     }
 
@@ -54,7 +46,7 @@ public function parseValue($value)
             return $value;
         }
 
-        throw new Error('Cannot represent value as boolean: ' . Utils::printSafe($value));
+        throw new Error('Boolean cannot represent a non boolean value: ' . Utils::printSafe($value));
     }
 
     /**

diff --git a/src/Type/Definition/FloatType.php b/src/Type/Definition/FloatType.php
@@ -10,9 +10,12 @@
 use GraphQL\Language\AST\IntValueNode;
 use GraphQL\Language\AST\Node;
 use GraphQL\Utils\Utils;
+use function floatval;
 use function is_array;
 use function is_bool;
 use function is_finite;
+use function is_float;
+use function is_int;
 use function is_nan;
 use function is_numeric;
 use function sprintf;
@@ -37,26 +40,9 @@ class FloatType extends ScalarType
      */
     public function serialize($value)
     {
-        return $this->coerceFloat($value);
-    }
-
-    private function coerceFloat($value)
-    {
-        if (is_array($value)) {
-            throw new Error(
-                sprintf('Float cannot represent an array value: %s', Utils::printSafe($value))
-            );
-        }
+        $float = is_numeric($value) || is_bool($value) ? floatval($value) : null;
 
-        if ($value === '') {
-            throw new Error(
-                'Float cannot represent non numeric value: (empty string)'
-            );
-        }
-
-        $float = is_numeric($value) || is_bool($value) ? (float) $value : null;
-
-        if ($float === null || ! is_finite($float) || is_nan($float)) {
+        if ($float === null || ! is_finite($float)) {
             throw new Error(
                 'Float cannot represent non numeric value: ' .
                 Utils::printSafe($value)
@@ -75,7 +61,16 @@ private function coerceFloat($value)
      */
     public function parseValue($value)
     {
-        return $this->coerceFloat($value);
+        $float = is_float($value) || is_int($value) ? floatval($value) : null;
+
+        if ($float === null || ! is_finite($float)) {
+            throw new Error(
+                'Float cannot represent non numeric value: ' .
+                Utils::printSafe($value)
+            );
+        }
+
+        return $float;
     }
 
     /**

diff --git a/src/Type/Definition/IDType.php b/src/Type/Definition/IDType.php
@@ -39,22 +39,12 @@ class IDType extends ScalarType
      */
     public function serialize($value)
     {
-        if ($value === true) {
-            return 'true';
-        }
-        if ($value === false) {
-            return 'false';
-        }
-        if ($value === null) {
-            return 'null';
-        }
-        if (is_array($value)) {
-            throw new Error(
-                'ID cannot represent an array value: ' . Utils::printSafe($value)
-            );
-        }
-        if (! is_scalar($value) && (! is_object($value) || ! method_exists($value, '__toString'))) {
-            throw new Error('ID cannot represent non scalar value: ' . Utils::printSafe($value));
+        $canCast = is_string($value)
+            || is_int($value)
+            || (is_object($value) && method_exists($value, '__toString'));
+
+        if (! $canCast) {
+            throw new Error('ID cannot represent value: ' . Utils::printSafe($value));
         }
 
         return (string) $value;
@@ -72,13 +62,7 @@ public function parseValue($value)
         if (is_string($value) || is_int($value)) {
             return (string) $value;
         }
-        if (is_array($value)) {
-            throw new Error(
-                'ID cannot represent an array value: ' . Utils::printSafe($value)
-            );
-        }
-
-        throw new Error('Cannot represent value as ID: ' . Utils::printSafe($value));
+        throw new Error('ID cannot represent value: ' . Utils::printSafe($value));
     }
 
     /**

diff --git a/src/Type/Definition/IntType.php b/src/Type/Definition/IntType.php
@@ -10,9 +10,12 @@
 use GraphQL\Language\AST\Node;
 use GraphQL\Utils\Utils;
 use function floatval;
+use function floor;
 use function intval;
 use function is_array;
 use function is_bool;
+use function is_float;
+use function is_int;
 use function is_numeric;
 use function sprintf;
 
@@ -43,53 +46,28 @@ class IntType extends ScalarType
      */
     public function serialize($value)
     {
-        return $this->coerceInt($value);
-    }
-
-    /**
-     * @param mixed $value
-     *
-     * @return int
-     */
-    private function coerceInt($value)
-    {
-        if (is_array($value)) {
-            throw new Error(
-                sprintf('Int cannot represent an array value: %s', Utils::printSafe($value))
-            );
+        // Fast path for 90+% of cases:
+        if (is_int($value) && $value <= self::MAX_INT && $value >= self::MIN_INT) {
+            return $value;
         }
 
-        if ($value === '') {
-            throw new Error(
-                'Int cannot represent non-integer value: (empty string)'
-            );
-        }
+        $float = is_numeric($value) || is_bool($value) ? floatval($value) : null;
 
-        if (! is_numeric($value) && ! is_bool($value)) {
+        if ($float === null || floor($float) !== $float) {
             throw new Error(
                 'Int cannot represent non-integer value: ' .
                 Utils::printSafe($value)
             );
         }
 
-        $num = floatval($value);
-        if ($num > self::MAX_INT || $num < self::MIN_INT) {
+        if ($float > self::MAX_INT || $float < self::MIN_INT) {
             throw new Error(
                 'Int cannot represent non 32-bit signed integer value: ' .
                 Utils::printSafe($value)
             );
         }
-        $int = intval($num);
-        // int cast with == used for performance reasons
-        // phpcs:ignore
-        if ($int != $num) {
-            throw new Error(
-                'Int cannot represent non-integer value: ' .
-                Utils::printSafe($value)
-            );
-        }
 
-        return $int;
+        return intval($float);
     }
 
     /**
@@ -101,7 +79,23 @@ private function coerceInt($value)
      */
     public function parseValue($value)
     {
-        return $this->coerceInt($value);
+        $isInt = is_int($value) || (is_float($value) && floor($value) === $value);
+
+        if (! $isInt) {
+            throw new Error(
+                'Int cannot represent non-integer value: ' .
+                Utils::printSafe($value)
+            );
+        }
+
+        if ($value > self::MAX_INT || $value < self::MIN_INT) {
+            throw new Error(
+                'Int cannot represent non 32-bit signed integer value: ' .
+                Utils::printSafe($value)
+            );
+        }
+
+        return intval($value);
     }
 
     /**

diff --git a/src/Type/Definition/StringType.php b/src/Type/Definition/StringType.php
@@ -12,6 +12,7 @@
 use function is_array;
 use function is_object;
 use function is_scalar;
+use function is_string;
 use function method_exists;
 
 class StringType extends ScalarType
@@ -34,31 +35,13 @@ class StringType extends ScalarType
      */
     public function serialize($value)
     {
-        return $this->coerceString($value);
-    }
+        $canCast = is_scalar($value)
+            || (is_object($value) && method_exists($value, '__toString'))
+            || $value === null;
 
-    private function coerceString($value)
-    {
-        if ($value === true) {
-            return 'true';
-        }
-        if ($value === false) {
-            return 'false';
-        }
-        if ($value === null) {
-            return 'null';
-        }
-        if (is_array($value)) {
+        if (! $canCast) {
             throw new Error(
-                'String cannot represent an array value: ' . Utils::printSafe($value)
-            );
-        }
-        if (is_object($value) && method_exists($value, '__toString')) {
-            return (string) $value;
-        }
-        if (! is_scalar($value)) {
-            throw new Error(
-                'String cannot represent non scalar value: ' . Utils::printSafe($value)
+                'String cannot represent value: ' . Utils::printSafe($value)
             );
         }
 
@@ -74,7 +57,13 @@ private function coerceString($value)
      */
     public function parseValue($value)
     {
-        return $this->coerceString($value);
+        if (! is_string($value)) {
+            throw new Error(
+                'String cannot represent a non string value: ' . Utils::printSafe($value)
+            );
+        }
+
+        return $value;
     }
 
     /**

diff --git a/tests/Executor/ExecutorSchemaTest.php b/tests/Executor/ExecutorSchemaTest.php
@@ -198,7 +198,7 @@ public function testExecutesUsingASchema() : void
                             'isPublished' => true,
                             'title'       => 'My Article 1',
                             'body'        => 'This is a post',
-                            'keywords'    => ['foo', 'bar', '1', 'true', null],
+                            'keywords'    => ['foo', 'bar', '1', '1', null],
                         ],
                     ],
                     'meta' => [ 'title' => 'My Article 1 | My Blog' ],

diff --git a/tests/Executor/VariablesTest.php b/tests/Executor/VariablesTest.php
@@ -705,7 +705,7 @@ public function testReportsErrorForArrayPassedIntoStringInput() : void
             'errors' => [[
                 'message'   =>
                     'Variable "$value" got invalid value [1,2,3]; Expected type ' .
-                    'String; String cannot represent an array value: [1,2,3]',
+                    'String; String cannot represent a non string value: [1,2,3]',
                 'locations' => [
                     ['line' => 2, 'column' => 31],
                 ],