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

Support [ID]! arrays in ID formatter. #3440

Merged
merged 19 commits into from
Apr 13, 2021
Merged

Conversation

benmccallum
Copy link
Collaborator

@benmccallum benmccallum commented Apr 6, 2021

There was a subtle bug in the existing code when handling arrays with nullable items when passed a null item.

Edit: I've since realised/remembered that arrays with nullable items is a really weird thing to do; but in any case; since it's allowed we should support it.

Details:
In the case of the is IEnumerable<string>, since an enumerable of string or string? both pass this check, if a null was passed in the array, it'd be passed to the IIdSerializer Deserialize method which would throw. This can be fixed by typing that to IEnumerable<string?> and then handling nulls appropriately, just adding null to the array.

In the case of the is IEnumerable<IdValue>, since IEnumerable<IdValue?> doesn't pass the check, my polymorphic id formatter (which intercepts and sets an IEnumerable<IdValue?> can never have it's value picked up and used, so that did need a new block of code to handle.

Testing:

new List<IdValue>() is IEnumerable<IdValue>
true
new List<IdValue>() is IEnumerable<IdValue?>
false
new List<IdValue?>() is IEnumerable<IdValue?>
true
new List<IdValue?>() is IEnumerable<IdValue>
false

new List<string?>() is IEnumerable<string>
true
new List<string?>() is IEnumerable<string?>
true

@benmccallum benmccallum marked this pull request as ready for review April 6, 2021 17:40
@benmccallum benmccallum changed the title Support nullable ID arrays Support [ID]! arrays Apr 6, 2021
@benmccallum benmccallum requested a review from michaelstaib April 7, 2021 07:59
@michaelstaib
Copy link
Member

Is this ready for review?

Copy link
Collaborator Author

@benmccallum benmccallum left a comment

Choose a reason for hiding this comment

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

Hey @michaelstaib , it's ready now, sorry I thought I was done yesterday but had some more ideas to streamline this.

@michaelstaib michaelstaib changed the title Support [ID]! arrays Support [ID]! arrays in ID formatter. Apr 13, 2021
@michaelstaib michaelstaib merged commit 1e3b958 into main Apr 13, 2021
@michaelstaib michaelstaib deleted the benmc/nullable-id-arrays branch April 13, 2021 13:57
@sonarcloud
Copy link

sonarcloud bot commented Apr 13, 2021

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

Successfully merging this pull request may close these issues.

2 participants