Skip to content

[Serializer] Added functionality to parse string boolean values into object boolean fields on denormalization #42716

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

Closed
wants to merge 2 commits into from

Conversation

aurimasniekis
Copy link
Contributor

Q A
Branch? 5.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #42715
License MIT
Doc PR Will try to make soon

There are some cases where denormalizing array into an object there are situations where values are string, and it is not possible to convert them into different types using custom denormalizer without reimplementing whole object denormalization.

This pull request adds new attribute ENABLE_STRING_BOOL_VALUE into ObjectNormalizer which allows to parse string values:

["true", "yes", "on", "1"]  // => true
["false", "no", "off", "0", ""] // => false

Into representative boolean values using filter_var(..., FILTER_VALIDATE_BOOL, FILTER_NULL_ON_FAILURE) function if object field type is boolean and data is string. If the string is not one of the valid values it will throw an exception as not valid value provided.

I have chosen to use the flag FILTER_NULL_ON_FAILURE to return null if an invalid falsy value is provided instead of handling all non-truthy values as false value and throw an exception as the invalid value provided.

…ct Normalizer to try parse string boolean values if object attribute type is bool and data value is string using filter_var FILTER_VALIDATE_BOOL function
@aurimasniekis
Copy link
Contributor Author

aurimasniekis commented Aug 25, 2021

I am not sure why PHPUnit / Tests (7.2) (pull_request) broke because the broken test is from Cache component

@OskarStark OskarStark added this to the 5.4 milestone Aug 25, 2021
return $booleanValue;
}

throw new NotNormalizableValueException(sprintf('The value of the "%s" attribute for class "%s" must be one of "true", "yes", "on", "1", "false", "no", "off", "0", "" ("%s" given).', $attribute, $currentClass, $data));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
throw new NotNormalizableValueException(sprintf('The value of the "%s" attribute for class "%s" must be one of "true", "yes", "on", "1", "false", "no", "off", "0", "" ("%s" given).', $attribute, $currentClass, $data));
throw new NotNormalizableValueException(sprintf('The value of the "%s" attribute for class "%s" must be one of "true", "yes", "on", "1", "false", "no", "off", "0", ""(empty string) ("%s" given).', $attribute, $currentClass, $data));

Not sure if this could help and if it makes sense

Copy link
Contributor Author

@aurimasniekis aurimasniekis Aug 25, 2021

Choose a reason for hiding this comment

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

Maybe, but I was trying to follow Symfony style from exceptions around.

@@ -362,6 +362,83 @@ private function getDenormalizerForObjectWithBasicProperties()
return $denormalizer;
}

public function testDenormalizeBasicTypePropertiesWithStringBoolValues()
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better robuste a dataProvider here? The getDenormalizerForObject...() is tight coupled to the number of values provided by the on consecutive calls, while it's not supposed to be extended in the future I guess, it feels a bit weird.

While using a dataProvider it would be less complex and you don't the onConscutiveCalls anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this was my own library I would have written tests differently, but I tried to keep with the same style the all other tests are done in the file.

public function testDenormalizeBasicTypePropertiesFromXml()
{
$denormalizer = $this->getDenormalizerForObjectWithBasicProperties();
// bool
$objectWithBooleanProperties = $denormalizer->denormalize(
[
'boolTrue1' => 'true',
'boolFalse1' => 'false',
'boolTrue2' => '1',
'boolFalse2' => '0',
'int1' => '4711',
'int2' => '-4711',
'float1' => '123.456',
'float2' => '-1.2344e56',
'float3' => '45E-6',
'floatNaN' => 'NaN',
'floatInf' => 'INF',
'floatNegInf' => '-INF',
],
ObjectWithBasicProperties::class,
'xml'
);
$this->assertInstanceOf(ObjectWithBasicProperties::class, $objectWithBooleanProperties);
// Bool Properties
$this->assertTrue($objectWithBooleanProperties->boolTrue1);
$this->assertFalse($objectWithBooleanProperties->boolFalse1);
$this->assertTrue($objectWithBooleanProperties->boolTrue2);
$this->assertFalse($objectWithBooleanProperties->boolFalse2);
// Integer Properties
$this->assertEquals(4711, $objectWithBooleanProperties->int1);
$this->assertEquals(-4711, $objectWithBooleanProperties->int2);
// Float Properties
$this->assertEqualsWithDelta(123.456, $objectWithBooleanProperties->float1, 0.01);
$this->assertEqualsWithDelta(-1.2344e56, $objectWithBooleanProperties->float2, 1);
$this->assertEqualsWithDelta(45E-6, $objectWithBooleanProperties->float3, 1);
$this->assertNan($objectWithBooleanProperties->floatNaN);
$this->assertInfinite($objectWithBooleanProperties->floatInf);
$this->assertEquals(-\INF, $objectWithBooleanProperties->floatNegInf);
}
private function getDenormalizerForObjectWithBasicProperties()
{
$extractor = $this->createMock(PhpDocExtractor::class);
$extractor->method('getTypes')
->will($this->onConsecutiveCalls(
[new Type('bool')],
[new Type('bool')],
[new Type('bool')],
[new Type('bool')],
[new Type('int')],
[new Type('int')],
[new Type('float')],
[new Type('float')],
[new Type('float')],
[new Type('float')],
[new Type('float')],
[new Type('float')]
));
$denormalizer = new AbstractObjectNormalizerCollectionDummy(null, null, $extractor);
$arrayDenormalizer = new ArrayDenormalizerDummy();
$serializer = new SerializerCollectionDummy([$arrayDenormalizer, $denormalizer]);
$arrayDenormalizer->setSerializer($serializer);
$denormalizer->setSerializer($serializer);
return $denormalizer;
}
public function testDenormalizeBasicTypePropertiesWithStringBoolValues()
{
$denormalizer = $this->getDenormalizerForObjectWithStringBoolValues();
// bool
$objectWithBooleanProperties = $denormalizer->denormalize(
[
'true' => 'true',
'yes' => 'yEs',
'on' => 'ON',
'one' => '1',
'false' => 'falsE',
'no' => 'No',
'off' => 'OfF',
'zero' => '0',
'empty' => '',
],
ObjectWithStringBoolean::class,
null,
[AbstractObjectNormalizer::ENABLE_STRING_BOOL_VALUE => true]
);
$this->assertInstanceOf(ObjectWithStringBoolean::class, $objectWithBooleanProperties);
$this->assertTrue($objectWithBooleanProperties->true);
$this->assertTrue($objectWithBooleanProperties->yes);
$this->assertTrue($objectWithBooleanProperties->on);
$this->assertTrue($objectWithBooleanProperties->one);
$this->assertFalse($objectWithBooleanProperties->false);
$this->assertFalse($objectWithBooleanProperties->no);
$this->assertFalse($objectWithBooleanProperties->off);
$this->assertFalse($objectWithBooleanProperties->zero);
$this->assertFalse($objectWithBooleanProperties->empty);
}
public function testDenormalizeBasicTypePropertiesWithBadStringBoolValues()
{
$denormalizer = $this->getDenormalizerForObjectWithStringBoolValues();
$this->expectException(NotNormalizableValueException::class);
$this->expectExceptionMessage('The value of the "yes" attribute for class "Symfony\Component\Serializer\Tests\Normalizer\ObjectWithStringBoolean" must be one of "true", "yes", "on", "1", "false", "no", "off", "0", "" ("foo" given).');
$objectWithBooleanProperties = $denormalizer->denormalize(
[
'yes' => 'foo',
],
ObjectWithStringBoolean::class,
null,
[AbstractObjectNormalizer::ENABLE_STRING_BOOL_VALUE => true]
);
}
private function getDenormalizerForObjectWithStringBoolValues()
{
$extractor = $this->createMock(PhpDocExtractor::class);
$extractor->method('getTypes')
->will($this->onConsecutiveCalls(
[new Type('bool')],
[new Type('bool')],
[new Type('bool')],
[new Type('bool')],
[new Type('bool')],
[new Type('bool')],
[new Type('bool')],
[new Type('bool')],
[new Type('bool')]
));
$denormalizer = new AbstractObjectNormalizerCollectionDummy(null, null, $extractor);
$arrayDenormalizer = new ArrayDenormalizerDummy();
$serializer = new SerializerCollectionDummy([$arrayDenormalizer, $denormalizer]);
$arrayDenormalizer->setSerializer($serializer);
$denormalizer->setSerializer($serializer);
return $denormalizer;
}

@OskarStark
Copy link
Contributor

You can ignore the failing tests from the cache component

@aurimasniekis
Copy link
Contributor Author

Any updates on this?

@dunglas
Copy link
Member

dunglas commented Sep 3, 2021

In my opinion this should be done in the decoder of the specific format having such requirements, as we already do in the YAML decoder.

Converting special strings to bool is not the responsibility of the normalizer.

@aurimasniekis
Copy link
Contributor Author

What if it's not serialized format, but array input. For e.g. trying to denormalize ParameterBag values (which does have the getBoolen method which does exactly this)

@dunglas
Copy link
Member

dunglas commented Sep 3, 2021

Still, it's a niche and non-standard use case that is better handled in another class than the built-in normalizer.

@aurimasniekis
Copy link
Contributor Author

Still, it's a niche and non-standard use case that is better handled in another class than the built-in normalizer.

The issue is to create another normalizer or something I need to clone a whole normalizer because all methods required to tap in are private.

This would be required to make protected, so it could be extended in another normalizer.

private function validateAndDenormalize(array $types, string $currentClass, string $attribute, $data, ?string $format, array $context)

Otherwise, I would have to maintain an exact copy of AbstractObjectNormalizer which is pointless...

@derrabus
Copy link
Member

derrabus commented Sep 3, 2021

ParameterBag is a class that holds values coming from an HTTP request. I don't think that it's designed to be constructed by anyone but the HttpFoundation component itself. Why is that a denormalization target for you?

@aurimasniekis
Copy link
Contributor Author

ParameterBag is a class that holds values coming from an HTTP request. I don't think that it's designed to be constructed by anyone but the HttpFoundation component itself. Why is that a denormalization target for you?

I have really big and complicated query strings I need to parse and validate, so I passing the query string into the normalizer which fills the object, and then I am using a validator to validate it. This way I have parsed and validated query string params. Sadly I do not have control over the use of query string.

@aurimasniekis
Copy link
Contributor Author

By query string I meant $request->query->all()

@aurimasniekis
Copy link
Contributor Author

One of the reasons why I created this PR as I have mentioned in issue #42715 it is not possible to extend object denormalizer with any custom logic for property values. For which I was able to get around on some other occasions, but on this occasion, I was not able to get around it because of the need to get target property types.

I had made a denormalizer by basically copying AbstractObjectNormalizer but I don't feel it as a good way to make it as it means now I am required to maintain consistency. So I thought it's a good feature and made this PR.

@derrabus
Copy link
Member

derrabus commented Sep 3, 2021

If it's just about validation, you don't need a denormalizer. You could validate the return value of $request->query->all() right away.

The denormalizer is not designed to be used standalone, but as the second step of a two-step deserialization. It operates under the assumption that the input array is created by a decoder that has already taken care of parsing primitive types. What you are basically suggesting is correcting mistakes of the decoding step within the denormalization step.

In your case, the missing piece is something like a QueryStringDecoder that parses the query string and returns the structure expected by the denormalizers (don't take this as a suggestion to implement such a decoder, though).

@aurimasniekis
Copy link
Contributor Author

aurimasniekis commented Sep 3, 2021

If it's just about validation, you don't need a denormalizer. You could validate the return value of $request->query->all() right away.

The denormalizer is not designed to be used standalone, but as the second step of a two-step deserialization. It operates under the assumption that the input array is created by a decoder that has already taken care of parsing primitive types. What you are basically suggesting is correcting mistakes of the decoding step within the denormalization step.

In your case, the missing piece is something like a QueryStringDecoder that parses the query string and returns the structure expected by the denormalizers (don't take this as a suggestion to implement such a decoder, though).

I perfectly understand your suggestion, but the problem for this case is not validation, but working with that structure it is nested array type query string, anyway, my main issue here is not about query string, but the inability to extend AbstractObjectAnalyzer with additional denormalization logic for properties.

There is a piece of logic that allows u to handle objects denormalization inside it, and an edge case for handling JSON float values, but no logic to extend for other scalar types.

if (Type::BUILTIN_TYPE_OBJECT === $builtinType) {
if (!$this->serializer instanceof DenormalizerInterface) {
throw new LogicException(sprintf('Cannot denormalize attribute "%s" for class "%s" because injected serializer is not a denormalizer.', $attribute, $class));
}
$childContext = $this->createChildContext($context, $attribute, $format);
if ($this->serializer->supportsDenormalization($data, $class, $format, $childContext)) {
return $this->serializer->denormalize($data, $class, $format, $childContext);
}
}

The easiest way would be to allow extending that method to be extended, or provide adding additional logic for handling scalar types. I had numerous cases where a simple custom denormalizer would have solved a lot of issues even when data is coming from encoders.

@OskarStark
Copy link
Contributor

To me this sounds like a dedicated response value object which validates the structure and values of the query string. Afterwards you can work on this kind of object, or am I missing sth. I would not use any de/normalize here

@aurimasniekis
Copy link
Contributor Author

To me this sounds like a dedicated response value object which validates the structure and values of the query string. Afterwards you can work on this kind of object, or am I missing sth. I would not use any de/normalize here

Okay, can we just focus on the extendability of object denormalizer?

@dunglas
Copy link
Member

dunglas commented Sep 4, 2021

@aurimasniekis yes! This is a known issue and there is an ongoing work to refactor this class and make it using composition instead of inheritance. See #30818

@fabpot
Copy link
Member

fabpot commented Oct 29, 2021

Can we take a decision here? @dunglas?

@fabpot fabpot modified the milestones: 5.4, 6.1 Nov 16, 2021
@Lappihuan
Copy link

i recently stumbled over the same issue and i feel like the biggest issue is, that the AbstractObjectNormalizer is handling scalars and does not delegate to scalar denormalizer/normalizers.
That way a custom denormalizer could be registered to handle such cases.

But regardless, having AbstractObjectNormalizer handle scalars even tho these are handled in their own denormalizer.
#35235

@dunglas
Copy link
Member

dunglas commented Mar 18, 2022

Sorry but I'm in favor of not merging it. The more complexity we add to AbstractObjectNormalizer, the harder it will be to refactor it (which is our main long term goal).

@fabpot
Copy link
Member

fabpot commented Mar 18, 2022

Thank you @dunglas for chiming in. Let's close then.

@fabpot fabpot closed this Mar 18, 2022
@TerjeBr
Copy link

TerjeBr commented Sep 14, 2023

Could you please consider to open this PR again?

The problem is that we want a deserialiser that can handle the arrays that have string values instead of boolean values in some places. This is very useful for data coming form query strings, but also for other sources of data. It is too late to deal with this after deserialisation, because then for example the string "false" has already become a true value. This is exactly the improvement of AbstractObjectNormalizer that we need. Please, please, could you please accept this PR?

@skobkin
Copy link

skobkin commented Jun 7, 2024

Same problem here. Remote system uses query or form data in callbacks. I'm trying to implement a decent wrapper using serializer. It works for outgoing requests when I can use JSON, but doesn't for incoming callbacks where I just can't customize denormalization of bool values.

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