From f569c6de2d445b8948ce008ff3c3e8998fe4cebb Mon Sep 17 00:00:00 2001 From: Vladimir Razuvaev Date: Tue, 4 Jul 2017 18:27:20 +0700 Subject: [PATCH] Spec compliance: coercion of Int values --- src/Executor/Values.php | 2 +- src/Type/Definition/IntType.php | 34 +++++++--- tests/Type/ScalarSerializationTest.php | 47 ++++++++++---- tests/Utils/AstFromValueTest.php | 12 +++- tests/Utils/IsValidPHPValueTest.php | 90 ++++++++++++++++++++++++++ 5 files changed, 164 insertions(+), 21 deletions(-) create mode 100644 tests/Utils/IsValidPHPValueTest.php diff --git a/src/Executor/Values.php b/src/Executor/Values.php index e3f6023..1e9c69d 100644 --- a/src/Executor/Values.php +++ b/src/Executor/Values.php @@ -183,7 +183,7 @@ class Values * @param InputType $type * @return array */ - private static function isValidPHPValue($value, InputType $type) + public static function isValidPHPValue($value, InputType $type) { // A value must be provided if the type is non-null. if ($type instanceof NonNull) { diff --git a/src/Type/Definition/IntType.php b/src/Type/Definition/IntType.php index ff0dd49..e80c668 100644 --- a/src/Type/Definition/IntType.php +++ b/src/Type/Definition/IntType.php @@ -1,9 +1,9 @@ coerceInt($value); + try { + return $this->coerceInt($value); + } catch (InvariantViolation $e) { + throw new UserError($e->getMessage(), $e->getCode(), $e); + } } /** @@ -57,19 +61,33 @@ values. Int can represent values between -(2^31) and 2^31 - 1. '; private function coerceInt($value) { if ($value === '') { - throw new UserError( + throw new InvariantViolation( 'Int cannot represent non 32-bit signed integer value: (empty string)' ); } if (false === $value || true === $value) { return (int) $value; } - if (is_numeric($value) && $value <= self::MAX_INT && $value >= self::MIN_INT) { - return (int) $value; + if (!is_numeric($value) || $value > self::MAX_INT || $value < self::MIN_INT) { + throw new InvariantViolation( + sprintf('Int cannot represent non 32-bit signed integer value: %s', Utils::printSafe($value)) + ); } - throw new UserError( - sprintf('Int cannot represent non 32-bit signed integer value: %s', Utils::printSafe($value)) - ); + $num = (float) $value; + + // The GraphQL specification does not allow serializing non-integer values + // as Int to avoid accidental data loss. + // Examples: 1.0 == 1; 1.1 != 1, etc + if ($num != (int)$value) { + // Additionally account for scientific notation (i.e. 1e3), because (float)'1e3' is 1000, but (int)'1e3' is 1 + $trimmed = floor($num); + if ($trimmed !== $num) { + throw new InvariantViolation( + 'Int cannot represent non-integer value: ' . Utils::printSafe($value) + ); + } + } + return (int) $value; } /** diff --git a/tests/Type/ScalarSerializationTest.php b/tests/Type/ScalarSerializationTest.php index 23165d7..9320da5 100644 --- a/tests/Type/ScalarSerializationTest.php +++ b/tests/Type/ScalarSerializationTest.php @@ -1,6 +1,7 @@ assertSame(123, $intType->serialize('123')); $this->assertSame(0, $intType->serialize(0)); $this->assertSame(-1, $intType->serialize(-1)); - $this->assertSame(0, $intType->serialize(0.1)); - $this->assertSame(1, $intType->serialize(1.1)); - $this->assertSame(-1, $intType->serialize(-1.1)); $this->assertSame(100000, $intType->serialize(1e5)); + $this->assertSame(0, $intType->serialize(0e5)); + + // The GraphQL specification does not allow serializing non-integer values + // as Int to avoid accidental data loss. + try { + $intType->serialize(0.1); + $this->fail('Expected exception not thrown'); + } catch (InvariantViolation $e) { + $this->assertEquals('Int cannot represent non-integer value: 0.1', $e->getMessage()); + } + try { + $intType->serialize(1.1); + $this->fail('Expected exception not thrown'); + } catch (InvariantViolation $e) { + $this->assertEquals('Int cannot represent non-integer value: 1.1', $e->getMessage()); + } + try { + $intType->serialize(-1.1); + $this->fail('Expected exception not thrown'); + } catch (InvariantViolation $e) { + $this->assertEquals('Int cannot represent non-integer value: -1.1', $e->getMessage()); + } + try { + $intType->serialize('-1.1'); + $this->fail('Expected exception not thrown'); + } catch (InvariantViolation $e) { + $this->assertEquals('Int cannot represent non-integer value: "-1.1"', $e->getMessage()); + } + // Maybe a safe PHP int, but bigger than 2^32, so not // representable as a GraphQL Int try { $intType->serialize(9876504321); $this->fail('Expected exception was not thrown'); - } catch (UserError $e) { + } catch (InvariantViolation $e) { $this->assertEquals('Int cannot represent non 32-bit signed integer value: 9876504321', $e->getMessage()); } try { $intType->serialize(-9876504321); $this->fail('Expected exception was not thrown'); - } catch (UserError $e) { + } catch (InvariantViolation $e) { $this->assertEquals('Int cannot represent non 32-bit signed integer value: -9876504321', $e->getMessage()); } try { $intType->serialize(1e100); $this->fail('Expected exception was not thrown'); - } catch (UserError $e) { + } catch (InvariantViolation $e) { $this->assertEquals('Int cannot represent non 32-bit signed integer value: 1.0E+100', $e->getMessage()); } try { $intType->serialize(-1e100); $this->fail('Expected exception was not thrown'); - } catch (UserError $e) { + } catch (InvariantViolation $e) { $this->assertEquals('Int cannot represent non 32-bit signed integer value: -1.0E+100', $e->getMessage()); } - $this->assertSame(-1, $intType->serialize('-1.1')); - try { $intType->serialize('one'); $this->fail('Expected exception was not thrown'); - } catch (UserError $e) { + } catch (InvariantViolation $e) { $this->assertEquals('Int cannot represent non 32-bit signed integer value: "one"', $e->getMessage()); } try { $intType->serialize(''); $this->fail('Expected exception was not thrown'); - } catch (UserError $e) { + } catch (InvariantViolation $e) { $this->assertEquals('Int cannot represent non 32-bit signed integer value: (empty string)', $e->getMessage()); } diff --git a/tests/Utils/AstFromValueTest.php b/tests/Utils/AstFromValueTest.php index 9cf983c..e6af51a 100644 --- a/tests/Utils/AstFromValueTest.php +++ b/tests/Utils/AstFromValueTest.php @@ -40,8 +40,17 @@ class ASTFromValueTest extends \PHPUnit_Framework_TestCase public function testConvertsIntValuesToASTs() { $this->assertEquals(new IntValueNode(['value' => '123']), AST::astFromValue(123.0, Type::int())); - $this->assertEquals(new IntValueNode(['value' => '123']), AST::astFromValue(123.5, Type::int())); $this->assertEquals(new IntValueNode(['value' => '10000']), AST::astFromValue(1e4, Type::int())); + $this->assertEquals(new IntValueNode(['value' => '0']), AST::astFromValue(0e4, Type::int())); + + // GraphQL spec does not allow coercing non-integer values to Int to avoid + // accidental data loss. + try { + AST::astFromValue(123.5, Type::int()); + $this->fail('Expected exception not thrown'); + } catch (\Exception $e) { + $this->assertEquals('Int cannot represent non-integer value: 123.5', $e->getMessage()); + } try { AST::astFromValue(1e40, Type::int()); // Note: js version will produce 1e+40, both values are valid GraphQL floats @@ -61,6 +70,7 @@ class ASTFromValueTest extends \PHPUnit_Framework_TestCase $this->assertEquals(new FloatValueNode(['value' => '123.5']), AST::astFromValue(123.5, Type::float())); $this->assertEquals(new IntValueNode(['value' => '10000']), AST::astFromValue(1e4, Type::float())); $this->assertEquals(new FloatValueNode(['value' => '1e+40']), AST::astFromValue(1e40, Type::float())); + $this->assertEquals(new IntValueNode(['value' => '0']), AST::astFromValue(0e40, Type::float())); } /** diff --git a/tests/Utils/IsValidPHPValueTest.php b/tests/Utils/IsValidPHPValueTest.php new file mode 100644 index 0000000..051f704 --- /dev/null +++ b/tests/Utils/IsValidPHPValueTest.php @@ -0,0 +1,90 @@ +expectNoErrors($result); + + // returns no error for negative int input: + $result = Values::isValidPHPValue('-1', Type::int()); + $this->expectNoErrors($result); + + // returns no error for exponent input: + $result = Values::isValidPHPValue('1e3', Type::int()); + $this->expectNoErrors($result); + $result = Values::isValidPHPValue('0e3', Type::int()); + $this->expectNoErrors($result); + + // returns no error for null value: + $result = Values::isValidPHPValue(null, Type::int()); + $this->expectNoErrors($result); + + // returns a single error for empty value + $result = Values::isValidPHPValue('', Type::int()); + $this->expectErrorResult($result, 1); + + // returns error for float input as int + $result = Values::isValidPHPValue('1.5', Type::int()); + $this->expectErrorResult($result, 1); + + // returns a single error for char input + $result = Values::isValidPHPValue('a', Type::int()); + $this->expectErrorResult($result, 1); + + // returns a single error for char input + $result = Values::isValidPHPValue('meow', Type::int()); + $this->expectErrorResult($result, 1); + } + + public function testValidFloatValue() + { + // returns no error for int input + $result = Values::isValidPHPValue('1', Type::float()); + $this->expectNoErrors($result); + + // returns no error for exponent input + $result = Values::isValidPHPValue('1e3', Type::float()); + $this->expectNoErrors($result); + $result = Values::isValidPHPValue('0e3', Type::float()); + $this->expectNoErrors($result); + + // returns no error for float input + $result = Values::isValidPHPValue('1.5', Type::float()); + $this->expectNoErrors($result); + + // returns no error for null value: + $result = Values::isValidPHPValue(null, Type::float()); + $this->expectNoErrors($result); + + // returns a single error for empty value + $result = Values::isValidPHPValue('', Type::float()); + $this->expectErrorResult($result, 1); + + // returns a single error for char input + $result = Values::isValidPHPValue('a', Type::float()); + $this->expectErrorResult($result, 1); + + // returns a single error for char input + $result = Values::isValidPHPValue('meow', Type::float()); + $this->expectErrorResult($result, 1); + } + + private function expectNoErrors($result) + { + $this->assertInternalType('array', $result); + $this->assertEquals([], $result); + } + + private function expectErrorResult($result, $size) { + $this->assertInternalType('array', $result); + $this->assertEquals($size, count($result)); + } +}