Skip to content
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

Introduce strict deserializer visitor #1401

Merged
merged 1 commit into from
Aug 6, 2022
Merged

Conversation

simPod
Copy link
Contributor

@simPod simPod commented Mar 18, 2022

Q A
Bug fix? no
New feature? yes
Doc updated no
BC breaks? yes
Deprecations? no
Tests pass? yes
Fixed tickets #...
License MIT

Recently, I've found that

  • false is cast to 0 for int properties
  • null is cast to "" for string properties
  • etc. etc.

This greatly modifies the input data that is not desired.

Consider a case when I want to deserialize json [1,2,3] and validate that all values are of int type. However, json ["1", "2", "3"] is cast to [1,2,3] so I cannot validate it after deserialization and such input is forwarded further into application.

Therefore, I've created strict visitor that does not cast values to desire types and invalid value conversion does not happen anymore. Also, nulls are handled properly. But - I had to change return types of DeserializationVisitorInterface to allow null.
This behaviour is compliant with e.g. DateHandler which returns null value for null json. So it all rather behaves as expected now.

public function deserializeDateTimeFromJson(DeserializationVisitorInterface $visitor, $data, array $type): ?\DateTimeInterface
{
if (null === $data) {
return null;
}
return $this->parseDateTime($data, $type);

(contains #1400)

@goetas
Copy link
Collaborator

goetas commented May 11, 2022

This seems ok, but how to make it developer friendly? how to let them choose between json and json strict? would it make sense to have it as some sort of option for the existing JsonDeserializationVisitor ?

@goetas
Copy link
Collaborator

goetas commented May 11, 2022

it would be good to add this visitor in the list of available visitors and let devs choose if use the struct or not strict deserialization

@simPod
Copy link
Contributor Author

simPod commented May 11, 2022

@goetas according to docs, I'd expect people do this

$serializer = \JMS\Serializer\SerializerBuilder::create()
    ->setDeserializationVisitor('json', new JsonDeserializationStrictVisitorFactory())
    ->build();

I'm using bundle so not really sure how people use jms without it. I'd expect they use the way mentioned in docs 🤔 .


Also, it would be great to make this default in v4.

@simPod
Copy link
Contributor Author

simPod commented May 11, 2022

list of available visitors

where do I find it?

@goetas
Copy link
Collaborator

goetas commented May 11, 2022

what about having something as this

$serializer->deserialize($data, 'SomeType', 'json');
$serializer->deserialize($data, 'SomeType', 'json_strict');

by adding it by default in

'json' => new JsonDeserializationVisitorFactory(),
?

(it would be nice to support it better in the bundle too)

@simPod
Copy link
Contributor Author

simPod commented May 11, 2022

Allowing $serializer->deserialize($data, 'SomeType', 'json_strict'); should add implicit support to bundle as well I guess.

@goetas
Copy link
Collaborator

goetas commented May 11, 2022

yea but unfortuantly this is not enough, you should add

foreach (['json', 'xml'] as $format) {
this to all the type handlers as well if we want to consistent json support for who uses the strict version.

The bundle does not use the SerializationBuilder, so there it is necessary to play with the symfony di.

@simPod
Copy link
Contributor Author

simPod commented May 11, 2022

I'll look into a bundle support once this is merged.

The tests found the issue you've mentioned, fixing it now.

@simPod
Copy link
Contributor Author

simPod commented May 11, 2022

@goetas This is rather unpleasant though: class JMS\Serializer\Handler\DateHandler does not have a method "deserializeDateTimeFromjson_strict" Do we want to go this way?

Looking at the implications, I'd stay with ->setDeserializationVisitor('json', new JsonDeserializationStrictVisitorFactory()) and adapt the bundle accordingly.

I think it is not really a new format so introducing json_strict seems hacky.

@goetas
Copy link
Collaborator

goetas commented May 11, 2022

Ok, why not addign then a constructor option to the "old" JsonDeserializationVisitorFactory that based on it returns a strict visitor or a non strict one? that would make the bundle support super easy

@simPod
Copy link
Contributor Author

simPod commented May 11, 2022

@goetas much better than json_strict. Check it out.

Copy link
Collaborator

@scyzoryck scyzoryck left a comment

Choose a reason for hiding this comment

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

I like strict idea for deserialization! Thanks 🙇‍♂️

@simPod
Copy link
Contributor Author

simPod commented Jul 20, 2022

@goetas is there anything else? Thanks

@goetas goetas merged commit d642a7d into schmittjoh:master Aug 6, 2022
@goetas
Copy link
Collaborator

goetas commented Aug 6, 2022

thank you!

@simPod
Copy link
Contributor Author

simPod commented Oct 11, 2022 via email

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

Successfully merging this pull request may close these issues.

4 participants