Skip to content

Updated KeyValuePairSerializer to allow null value #982

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

iae610
Copy link

@iae610 iae610 commented Nov 28, 2022

Updated KeyValuePairSerializer (used by MapSerializer) to allow value to be null. It still makes sense to not allow null for key since that is a requirement of the C# Dictionary.

This change fixes an issue I saw while upgrading from V2 to V3. V3 didn't properly deserialize a Dictionary<> when the value in the KeyValuePair was null however this worked fine on version 2.4.4. I found that this only happened when deserializing as a Dictionary<> and if you deserialize as a KeyValuePair<> array it worked as expected. I tracked this down to the internal KeyValuePairSerializer.

Updated KeyValuePairSerializer (used by MapSerializer) to allow value to be null. It still makes sense to not allow null for key since that is a requirement of the C# Dictionary.
@mgravell
Copy link
Member

Did you see the test failures? Each of those needs considering here, as they relate to this change. Tests can be updated, of course - it just needs to be very deliberate.

@iae610
Copy link
Author

iae610 commented Nov 29, 2022

Sorry, got pulled into something else today. I think the change should be straightforward since we have both the new and old dictionary in code and should be able to verify the KVPs are equal but not the same object. I think the only potential situation that isn't straightforward is an empty list since it gets deserialized as null instead of an empty list. I'll look at fixing them tomorrow and will update this request with the code changes and an explanation. Thanks.

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.

2 participants