YamlDotNet icon indicating copy to clipboard operation
YamlDotNet copied to clipboard

YamlDotNet doesn't respect non-nullable types

Open RobotsOnDrugs opened this issue 2 years ago • 5 comments

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.

RobotsOnDrugs avatar Dec 31 '22 15:12 RobotsOnDrugs

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.

EdwardCooke avatar Jan 10 '23 15:01 EdwardCooke

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.

RobotsOnDrugs avatar Jan 10 '23 21:01 RobotsOnDrugs

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.

EdwardCooke avatar Jan 10 '23 23:01 EdwardCooke

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.

MetaFight avatar Jun 14 '24 13:06 MetaFight

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

tats-u avatar Jun 27 '24 10:06 tats-u

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.

EdwardCooke avatar Jul 03 '24 08:07 EdwardCooke

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

mrubli avatar Jul 05 '24 18:07 mrubli

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 avatar Jul 06 '24 08:07 EdwardCooke

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 avatar Jul 06 '24 14:07 EdwardCooke

This work is complete now.

EdwardCooke avatar Jul 14 '24 17:07 EdwardCooke

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.

mrubli avatar Jul 23 '24 13:07 mrubli