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

EnsureValidNullability does not work with lists of values, whether nullable reference types are allowed or not #39

Open
ProductiveRage opened this issue Apr 13, 2020 · 1 comment

Comments

@ProductiveRage
Copy link
Contributor

ProductiveRage commented Apr 13, 2020

It seems that the contents of list types are not correctly checked for null whether or not non-nullable-reference-type behaviour is enabled - for example, the below code will throw a System.NullReferenceException if reference types are allowed to be null or not:

var json = "[ null ]";
var arrayOfString = JsonConvert.DeserializeObject<string[]>(json);
arrayOfString.EnsureValidNullability();

The expected behaviour for the above code is that an InvalidOperationException should be throw if the non-nullable-reference-types behaviour is enabled and nothing should happen if it's not.

I've created PR #40 to pull in some unit tests to help you diagnose.

I tried poking around with the code to see if I could create a PR with a possible solution but I'm afraid that I got lost! The first problem is that in "ValidateNullability" there is a call to obj.GetType() that doesn't check whether "obj" is null (which is why there is NullReferenceException regardless of whether the value should be allowed to be null or not) but I think that the "IEnumerable" handling has to take into account the type of items in the list and whether they're allowed to be null - I got stuck trying to do that; I could dig around and get the types of "T" that any given enumerable type might implement IEnumerable<T> for but I failed to correctly determine whether those "T" values should be Unknown, Nullable or NotNullable. I then had a quick look at the documentation here and got completely lost trying to track how to work out what generic type parameters should be marked as nullable or not!

Please let me know if I can include any additional information, example cases or other help.

@ProductiveRage
Copy link
Contributor Author

I still don't understand the code well enough to have made much head way but it looks like the ValidateNullability method should pass in the "property" reference when it calls ValidateNullability again when enumerating properties because there is valuable nullability information being lost at the moment.

For example, if I was trying to deserialise

var json = "{ \"Items\": [ null ] }";

into

public class Example
{
    public List<string?> Items { get; set; } = new List<string?>();
}

.. then it knows when it encounters the "Items" property that its nullability state should be:

{Items (Property) - List: NotNullable String: Nullable}

.. but it loses that when it calls ValidateNullability again and then has insufficient information to determine that the enumerable values of that property should not be nullable.

RicoSuter added a commit that referenced this issue Sep 28, 2020
* First unit tests relating to issue 39 - two fail to illustrate the problem and two pass already and only exist as regression tests

* Added more tests for issue 39 - this time with a List<string> that is a property instead of a string[] that is being directly deserialised to; again two tests that fail to illustrate the problem and two that pass to act as regression tests

When the code is updated to fix the problem in the previous changeset, I'm sure that it will fix these as well but I thought that it made sense to include a few extra cases

* Improve null check

Co-authored-by: Rico Suter <[email protected]>
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

No branches or pull requests

1 participant