8000 [Serializer] int is valid when float is expected when deserializing JSON by dunglas · Pull Request #21165 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Serializer] int is valid when float is expected when deserializing JSON #21165

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

[Serializer] int is valid when float is expected when deserializing JSON #21165

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fe7228b
[Serializer] int is valid when float is expected when deserializing JSON
dunglas Jan 4, 2017
d5177f1
Optimize test
dunglas Jan 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
[Serializer] int is valid when float is expected when deserializing JSON
  • Loading branch information
@dunglas
dunglas committed Jan 4, 2017
commit fe7228b05a374ba25237c531a7151b552843f32d
11 changes: 11 additions & 0 deletions src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
namespace Symfony\Component\Serializer\Normalizer;

use Symfony\Component\PropertyAccess\Exception\InvalidArgumentException;
use Symfony\Component\Serializer\Encoder\JsonEncoder;
use Symfony\Component\Serializer\Exception\CircularReferenceException;
use Symfony\Component\Serializer\Exception\LogicException;
use Symfony\Component\Serializer\Exception\UnexpectedValueException;
Expand Down Expand Up @@ -260,6 +261,16 @@ private function validateAndDenormalize($currentClass, $attribute, $data, $forma
}
}

// JSON only has a Number type corresponding to both int and float PHP types.
// PHP's json_encode, JavaScript's JSON.stringify, Go's json.Marshal as well as most other JSON encoders convert
// floating-point numbers like 12.0 to 12 (the decimal part is dropped when possible).
// PHP's json_decode automatically converts Numbers without a decimal part to integers.
// To circumvent this behavior, integers are converted to floats when denormalizing JSON based formats and when
// a float is expected.
if (Type::BUILTIN_TYPE_FLOAT === $builtinType && false !== strpos($format, JsonEncoder::FORMAT) && is_int($data)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the strpos($format, JsonEncoder::FORMAT) part really necessary? (Why not all formats?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is because some (most) formats make a difference between int and float.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, ok, but would it be harmful to always cast an int it to float if that's what is expected?
Just asking, I'm fine with it it, despite the format check looking kind of weird here. 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would swap is_int and the string search. Checking a variable type is cheaper (yeah, this is a micro-optimization, but it does not hurt readability at all, and it may be called often when deserializing an object graph

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stof done

@ogizanagi I'm not sure to understand :) JSON has only a "Number" type, it's a limitation of the format, but for a format having a float type (like YAML for instance), it's not a good security practice to allow a int where a float is expected.

return (float) $data;
}

if (call_user_func('is_'.$builtinType, $data)) {
return $data;
}
Expand Down
28 changes: 23 additions & 5 deletions src/Symfony/Component/Serializer/Tests/Normalizer/ObjectNormalizerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,11 +537,21 @@ public function testDenomalizeRecursive()
'inners' => array(array('foo' => 1), array('foo' => 2)),
), ObjectOuter::class);

$this->assertEquals('foo', $obj->getInner()->foo);
$this->assertEquals('bar', $obj->getInner()->bar);
$this->assertEquals('1988-01-21', $obj->getDate()->format('Y-m-d'));
$this->assertEquals(1, $obj->getInners()[0]->foo);
$this->assertEquals(2, $obj->getInners()[1]->foo);
$this->assertSame('foo', $obj->getInner()->foo);
$this->assertSame('bar', $obj->getInner()->bar);
$this->assertSame('1988-01-21', $obj->getDate()->format('Y-m-d'));
$this->assertSame(1, $obj->getInners()[0]->foo);
$this->assertSame(2, $obj->getInners()[1]->foo);
}

public function testAcceptJsonNumber()
{
$extractor = new PropertyInfoExtractor(array(), array(new PhpDocExtractor(), new ReflectionExtractor()));
$normalizer = new ObjectNormalizer(null, null, null, $extractor);
$serializer = new Serializer(array(new ArrayDenormalizer(), new DateTimeNormalizer(), $normalizer));

$this->assertSame(10.0, $serializer->denormalize(array('number' => 10), JsonNumber::class, 'json')->number);
$this->assertSame(10.0, $serializer->denormalize(array('number' => 10), JsonNumber::class, 'jsonld')->number);
}

/**
Expand Down Expand Up @@ -820,3 +830,11 @@ protected function isAllowedAttribute($classOrObject, $attribute, $format = null
return false;
}
}

class JsonNumber
{
/**
* @var float
*/
public $number;
}
0