Core: AbstractItemNormalizer does not map basic non-string types correctly in XML and CSV

[warslett]

API Platform version(s) affected: Observed in 1.2.0

Description
When using XML or CSV encoder types, values are always decoded as strings. This results in errors at denormalisation "The type of the \"myFieldName\" attribute must be \"bool\", \"string\" given." Same issue for float and integer values.

How to reproduce

1. Create a resource with a boolean value
2. Enable XML encoder
3. Post XML to resource endpoint setting boolean value
4. Observe error

Possible Solution
I have fixed the issue locally by adding some logic to AbstractItemNormalizer::createAttributeValue to infer valid values depending on type:

        if (is_string($value) && 'string' !== $type->getBuiltinType()) {

            if (ctype_digit($value)) {
                if ('float' === $type->getBuiltinType()) {
                    $value = floatval($value);
                }
                if ('int' === $type->getBuiltinType()) {
                    $value = intval($value);
                }
            }

            if ('bool' === $type->getBuiltinType()) {
                if (in_array($value, ['true', 'True', 'T', 't', "1"], true)) {
                    $value = true;
                }
                if (in_array($value, ['false', 'False', 'F', 'f', "0"], true)) {
                    $value = false;
                }
            }
        }

Additional Context
This issue is also present in the Serializer project even though it uses a different implementation: https://github.com/symfony/symfony/issues/33849

[soyuka]

This needs to be fixed upstream as the issue comes from the xml encoder IIUC.

[warslett]

@soyuka I dissagree. XML encoder cannot know what type the value is because there is no way of telling the type of an XML value without context. ie.

<Resource>
  <MyField>true</MyField>
</Resource>

In this example, MyField could either be a string that we are trying to set to "true" OR a boolean, in either situation the XML would be the same. This is not the case in JSON where the string "true" and the boolean true are not written the same way (the string is in quotes). Therefor the encoder can figure out whether or not it is a string or a boolean. The encoder is not aware of what class the normalized data will be mapped to so does not know what type it needs to decode the value to.

If we are using XML there are only two ways we can figure out if the value is a string or boolean. Either we have to allow the consumer to be explicit and allow them to do something like this:

<Resource>
  <MyField type="bool">true</MyField>
</Resource>

This will allow the encoder to decode the correct type but doesn't really make any sense from the consumers perspective because why would they need to provide the type for a field that can only ever be bool? Alternatively we cast the value to a boolean at denormalisation stage where we know what type we need.

[warslett]

It might be possible to fix this issue for both Serializer component and APIPlatform by putting the logic in \Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer::validateAndDenormalize I think

[soyuka]

I understand. Through the normalizer we indeed know of the property types via PropertyInfo. This could then be fixed in symfony's ObjectNormalizer that we extend here?

[warslett]

I had a look to see if we can put this logic in AbstractObjectNormalizer::validateAndDenormalize but it looks like this function isn't doing anything when used with APIPlatform because it calls the private function getTypes which returns null if propertyTypeExtractor is null and then validateAndDenormalize just returns the data (essentially it does nothing if $propertyTypeExtractor is null). There are no comments explaining this but I presume it is designed so that propertyTypeExtractor can optionally be injected to do some validation and if it isn't injected it does nothing. AbstractItemNormalizer overrides the constructor for AbstractObjectNormalizer so propertyTypeExtractor can never be injected. AbstractItemNormalizer seems to have it's own separate implementation for type resolution which is using PropertyMetadataFactory.

I'm rambling but essentially the crux of it is, it looks like this issue needs to be solved separately in AbstractItemNormalizer and AbstractObjectNormalizer because each implementation detects types differently.

[warslett]

PR open @soyuka would appreciate some eyes on it https://github.com/api-platform/core/pull/3191

[soyuka]

We do need to fix this upstream (in symfony) and then see what we can do about it in api platform as mentioned by @teohhanhui in https://github.com/api-platform/core/pull/3191#issuecomment-544474697.

[warslett]

@soyuka @teohhanhui yes I understand. Just for visibility of where it's are up to... There is a pull request open in Symfony Serializer to fix this issue in AbstractObjectNormalizer which has been quiet for 2 weeks. https://github.com/symfony/symfony/pull/33850

However as I've previously explained, the fix in the Symfony Serializer component will not fix the problem in the API Platform because the fix depends on a dependency which is not injected when used with API Platform. Therefor I have asked the contributor to break his fix out into a protected method so that we can call it in ObjectItemNormalizer so that we can solve the problem with less duplication. If the PR in Symfony Serializer doesn't move in the next few days I will pick it up.

I think this is what you call inheritance hell :laughing:. It looks to me like there is a case in the long term for decoupling AbstractItemNormalizer from AbstractObjectNormalizer as it's own independent implementation of NormalizerInterface and dealing with duplication by composition over inheritance.

[soyuka]

If you have time to improve the PR in question you could always open a new one there reusing the commits of the original author, I'm sure he won't see any issue in that.

[teohhanhui]

I'd recommend some patience when contributing to open source projects. It's unreasonable to expect something to move forward quickly, especially for a large project like Symfony. :smile:

[warslett]

no problem @teohhanhui just dumping all my thoughts while it's fresh in the mind. I'll have forgotten about it in 2 weeks. :smile:

[lyrixx]

Since the upstream PR has been merged, I think we can close this issue, isn't?

[warslett]

@lyrixx I'll double check this evening but I think when I first opened this issue the fix in the PR in symfony serializer was not going to fix the problem in ApiPlatform because the fix depends on a dependency which is not injected when used with API Platform. However that was a while ago now so I'll take another look it might have all changed