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

YamlDotNet doesn't respect non-nullable types #763

Closed
RobotsOnDrugs opened this issue Dec 31, 2022 · 11 comments
Closed

YamlDotNet doesn't respect non-nullable types #763

RobotsOnDrugs opened this issue Dec 31, 2022 · 11 comments

Comments

@RobotsOnDrugs
Copy link

Describe the bug
If the YAML has a value set to null, but the object does not declare the type as nullable, YamlDotNet forces it to be null anyway (reference types) or the default value (value types). This can lead to strange unexpected behavior (especially for value types) since the application has no way of knowing whether the default value was set or if it was set to null. It should throw a YamlException just as it would for any other wrong type (e.g., deserializing "hello" to a boolean).

To Reproduce
Define an object with a non-nullable property and deserialize to it with YAML where the property is set to null.

@EdwardCooke
Copy link
Collaborator

Can you provide example yaml and classes? If you can provide that, I'll look in to it. The nullable (question mark) on the classes is a compiler feature, not a runtime requirement, so suddenly instantiating a class and setting it using that instead of null may be considered a breaking change, I'd probably put that behind a feature flag. As for value types, if you declare a value type as nullable using Nullable<int> it should set it to null as expected.

@RobotsOnDrugs
Copy link
Author

public class MyClass
{
  public string SomeString { get; set; }
  public string? SomeNullableString { get; set; }
  public bool IsGood { get; set; }
  public bool IsNotGood { get; set; }
  public bool IsDefinitelyNotGood { get; set; }
}
SomeString: null
SomeNullableString: null
IsGood: true
IsNotGood: null
IsDefinitelyNotGood: hello

SomeString should not be null, and this yaml should cause an exception at deserialization because null is not valid for that property and setting it to null at compile time will result in an error, yet it gets set to null anyway.
SomeNullableString can be null, so this is fine. IsGood is true, so that's fine as well.
IsNotGood should also cause an exception because null is not valid here. It will get set to false even though the yaml has a different value. This is pretty problematic for the reasons I mentioned before.
IsDefinitelyNotGood should cause an exception because "hello" is not valid, but YamlDotNet will throw an exception in this case. This is inconsistent - null is no more valid than "hello" for IsNotGood, yet it is set to false in this case.

I could use Nullable<bool>, but null is not valid here. The code would communicate to a reader that null is valid, and there would be a need to check for null later on, the avoidance of which is a major reason the concept of nullability was implemented in the first place. True that this is not a strict requirement at runtime, but it goes against what the compiler enforces and what the code communicates. It's perfectly valid to initialize a non-nullable type to null, but marking it with a ! communicates that the value has no default and that it really needs to be set before it is used - the program is supposed have an error if it is not (especially when paired with the use of init).

Given that it is a change in behavior and that older versions of .NET would consider types not denoted with ? as nullable, I do agree that making it opt-in is perfectly reasonable, and I appreciate that you're willing to looking into it.

@EdwardCooke
Copy link
Collaborator

Code speaks a thousand words :). Thank you for the simple example. We may be limited to what reflection gives us in terms of nullability. I haven’t looked in to how we can determine that, so it’s an unknown if we can even do it, but, I will definitely look in to it. I’m working a couple of other issues at the moment so it’ll be after I finish them.

we also gladly accept pull requests, so if you would like to take a stab at it, go for it.

@MetaFight
Copy link
Contributor

I could be wrong, but I think this is typically solved in serialization by using the required keyword on properties that must not be null.

@tats-u
Copy link

tats-u commented Jun 27, 2024

@MetaFight
You're incorrect. This library doesn't seem to respect required or throw an exception if the entry is missing.

@EdwardCooke
Copy link
Collaborator

I’m going to try and get to this in the next few weeks. I’m planning nullable checks and required keywords. As long as they are accessible through the field/property attributes. If they aren’t there’s nothing I can do.

@mrubli
Copy link

mrubli commented Jul 5, 2024

There's a related problem where non-nullable lists are nulled out if an empty YAML key is given. Instead of the (empty) List<T> being left alone (or perhaps a new empty list being instantiated), it is assigned a null value.

This may also turn out to be a limitation of what reflection gives you, but I figured I'd add my observation and a small sample program. In the third case, the pre-initialized empty list gets overwritten.

#nullable enable

using YamlDotNet.Serialization;
using YamlDotNet.Serialization.NamingConventions;

var deserializer = new DeserializerBuilder()
    .IgnoreUnmatchedProperties()
    .WithNamingConvention(CamelCaseNamingConvention.Instance)
    .Build();

{
    const string YamlWithListItems = """
    strings:
    - foo
    - bar
    """;
    var yamlWithListItems = deserializer.Deserialize<MyYaml>(new StringReader(YamlWithListItems));
    PrintList(yamlWithListItems.Strings);       // foo|bar => OK
}

{
    const string YamlWithoutList = """
    something: else
    """;
    var yamlWithoutList = deserializer.Deserialize<MyYaml>(new StringReader(YamlWithoutList));
    PrintList(yamlWithoutList.Strings);         // (empty) => OK
}

{
    const string YamlWithoutListItems = """
    strings:
    """;
    var yamlWithoutListItems = deserializer.Deserialize<MyYaml>(new StringReader(YamlWithoutListItems));
    PrintList(yamlWithoutListItems.Strings);    // (null) => BAD
}

static void PrintList(List<string>? list)
{
    Console.WriteLine(
        list switch {
            null => "(null)",
            [] => "(empty)",
            [..] => string.Join("|", list),
        }
    );
}

class MyYaml
{
    public List<string> Strings { get; set; } = [];
}

Sample project: https://github.com/mrubli/yamldotnet-null-lists

@EdwardCooke
Copy link
Collaborator

Thanks. I’ll add that to the feature. I have code ready for the original feature in this issue. Just need better WiFi to push it up.

@EdwardCooke
Copy link
Collaborator

I’ve been thinking about the null list. Technically the spec says that if there’s nothing there then it should be treated as null. So it’s working as designed. Digging into the code it could be pretty complicated and hacky to create the list/enumerator object before it knows what’s in the yaml. For example, strings. That type is an enumerable char. It implements ienumerable and ienumerable. That’s just one example right off the top that complicates creating an object before the yaml specifies if it’s a list/sequence or if it’s a scalar. I’m thinking I’m going to leave this for now since it is following the spec.

@EdwardCooke
Copy link
Collaborator

This work is complete now.

@mrubli
Copy link

mrubli commented Jul 23, 2024

I’ve been thinking about the null list. Technically the spec says that if there’s nothing there then it should be treated as null. So it’s working as designed. […] I’m thinking I’m going to leave this for now since it is following the spec.

I understand. It's basically the YAML spec vs. the C# spec and something has to give. It was easy enough to work around in code once the problem was known. Perhaps adding a note to the documentation would be helpful but I don't know where the best place for that would be.

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

No branches or pull requests

5 participants