Open tobymiller opened 9 months ago
I hope this project doesn't make the same design mistake that dotnet made. The required
modifier is a language feature that "indicates that the field or property it's applied to must be initialized by an object initializer" and that should be it. In my opinion, ensuring that a property appears in an object initializer and ensuring that a property is required for JSON deserialization are separate matters. It's extremely unfortunate that System.Text.Json
has piggybacked on this keyword to give it additional meaning.
For example, you could have your type defined like so for the purpose of ensuring its set in object initializers:
public class Thing
{
public required string? Field { get; init; }
}
...but System.Text.Json will now throw an error when Field
doesn't exist as a property in the JSON even though deserializing to null
is just fine in 99.99% of cases.
The main problem described in this issue is that a nullable value has been assigned to a non-nullable one and I think it would be nice to have better solutions around that instead. Then for marking a property as required for JSON deserialization, I believe it should be kept separate from the required
modifier and use the separate [JsonRequired]
attribute to say a property is required:
public class Thing
{
[JsonRequired] // required for JSON deserialization
public required string? Field { get; init; } // required in object initializers
}
I disagree. Default values are, in my view, a disaster of a language concept. C# is stuck with them for backward compatibility, but for the first time with the required keyword, we have a way to express to the type system that something must always have a meaningful value. The benefit of being able to rely on this without exception is huge, and can eliminate a large class of bugs. However, if there are common circumstances (such as when using this library), where the apparent guarantee of the required keyword is broken, then it significantly undermines this.
I don't know what the language designers had in mind for this keyword, but it seems to me far more useful if libraries such as this take the approach that I'm suggesting. It can always be optional configuration so that those like me that never want to see another number come through as 0 when it was actually misspelt can have our wish.
On the null issue, I don't really see the problem, because things can be explicitly set to null in json just as they can in an object initialiser. If they're missing, that usually indicates a problem, not a legitimate null value.
If they're missing, that usually indicates a problem, not a legitimate null value.
It's very common for null values to be omitted in serialized JSON in order to use less data. If the required
modifier is piggybacked on then it means there now need to be a way to mark something as required in object initializers (the required
modifier's intent) and not required in the JSON.
for the first time with the required keyword, we have a way to express to the type system that something must always have a meaningful value.
I disagree, this keyword was intended to be a way to mark a property as required in an object initializer. Now libraries (like System.Text.Json) are making it have additional meanings. Nullable types should be used to help enforce a non-nullable value and not the required
keyword.
Default values are, in my view, a disaster of a language concept.
The ideal behaviour for a C# deserialization library would be for a string
property to error if not present or null
and for string?
to not error if not present or null. This would be a really big breaking change though (I'm also not sure about the performance cost of checking for the NullableAttribute
and the docs specifically say libraries should not use it so... 🤷♂️).
Another point to note is that making the required
modifier have this new meaning would also be a breaking change.
I hope this project doesn't make the same design mistake that dotnet made. The
required
modifier is a language feature that "indicates that the field or property it's applied to must be initialized by an object initializer" and that should be it. In my opinion, ensuring that a property appears in an object initializer and ensuring that a property is required for JSON deserialization are separate matters. It's extremely unfortunate thatSystem.Text.Json
has piggybacked on this keyword to give it additional meaning.
I absolutely 100% agree that System.Text.Json should not have given runtime significance to what is a essentially a compile time feature. I have excluded serializing default values many times as a conscientious design choice.
I don't know what the language designers had in mind for this keyword
Just read this and this. I believe it's very clear what the spirit and extent of the feature would be.
Recently I made use of the new required
modifier on my classes properties, having studied the documentation for that new keyword and understanding that it has only to do that when using property initializers in the source code. I started to see very unexpected exceptions with System.Text.Json deserialization because of the use of required
modifier on my types. Despite coming to what I thought was a solid grasp of a language concept, I was forced to deal with the the whims of library designers that gave extra, inconsistent meaning to it, only discoverable at runtime.
On the null issue, I don't really see the problem, because things can be explicitly set to null in JSON just as they can in an object initializer. If they're missing, that usually indicates a problem, not a legitimate null value.
The problem is backwards compatibility with stored JSON or JSON coming from an external system that we may have absolutely no control whatsoever.
Default values are, in my view, a disaster of a language concept.
I understand, but the safest way to deal with distaste of default values is to use a language where that concept is not baked into its foundations.
To @dsherret's:
but System.Text.Json will now throw an error when Field doesn't exist as a property in the JSON even though deserializing to null is just fine in 99.99% of cases
I could not agree with you more! 🤓 After having got perhaps a bit too giddy with required
attribution a while back, I now find myself going through constructor-less record
type definitions and iterating to strip the required
modifiers away in order to work around downstream JSON deserialization issues when fancy serialization options are used (we don't use Newtonsoft any more, having migrated to System.Text.Json
some time ago).
"required but optional (nullable)" makes no sense in the JSON context, especially when using JsonIgnoreCondition.WhenWritingNull
(the aforementioned fancy feature) with your writers/encoders (i.e. you care about encoded payload size! 🤔).
@QuintinWillison I found a way to disable it for System.Text.Json here by overriding the TypeInfoResolver
. It's worked so far for me.
@QuintinWillison @dsherret Here's how I disabled the behavior:
public static JsonSerializerOptions IgnoreCSharpRequiredModifiers(this JsonSerializerOptions options)
{
options.TypeInfoResolver ??= new DefaultJsonTypeInfoResolver();
options.TypeInfoResolver =
options.TypeInfoResolver.WithAddedModifier(static typeInfo => typeInfo.IgnoreCSharpRequiredModifiers());
return options;
}
private static void IgnoreCSharpRequiredModifiers(this JsonTypeInfo info)
{
foreach (var p in info.Properties)
p.IsRequired &= p.AttributeProvider.HasJsonRequiredAttribute();
}
private static bool HasJsonRequiredAttribute(this ICustomAttributeProvider? attributes) =>
attributes is not null && attributes.IsDefined(typeof(JsonRequiredAttribute), inherit: false);
I disagree. Default values are, in my view, a disaster of a language concept. C# is stuck with them for backward compatibility, but for the first time with the required keyword, we have a way to express to the type system that something must always have a meaningful value.
@tobymiller - Its a bad assumption that the value will be meaningful and not just some unexpected/garbage. null
is only one of the many unexpected/garbage values that can break a function. The entire class of bugs will still exist thanks to all those other non-null values.
Is this the default value disaster of which you speak?
pubic class MyClass
{
public Dictionary<string, string> HumanDictionary {get; } = new(StringComparer.OrdinalIgnoreCase);
}
This seems like a win to me. NullReferenceExceptions avoided when someone calls HumanDictionary.Add(...
and bugs due to casing differences ("dog", "Dog") are avoided thanks to default values.
I think it would make sense for this behavior to be configurable. There are clearly people making different assumptions about what they would expect Newtonsoft's behavior to be, but to me the real issue isn't what developers would expect, it's what developers would want. Regardless of whether it's the expected default behavior, having the option to omit the JsonProperty(Required = Required.Always)
attribute in scenarios where you're putting it on every required
member on a large collection of models would be most welcome.
As a side note, I was one of the people expecting (and wanting) errors to be thrown when deserializing objects with missing required
members, so to me, that would be the sensible default behavior, but that's certainly not a hill I'd die on. I just want to be able to configure Newtonsoft to do what I need concisely.
In C# 11 (dotnet 7) the
required
modifier was introduced for fields and properties. It forces a caller to provide a value in, for example, an object initialiser. The compiler enforces this, and knows not to complain about non-nullable types without a default value, as a value must always be provided.System.Text.Json
throws an exception if arequired
field is omitted in source json. However, Newtonsoft does not, and silently fills this field with a default value, such asnull
. The desired behaviour can be achieved in Newtonsoft usingJsonProperty(Required = Required.Always)
, but this is verbose and should be redundant in these cases.I think it would be sensible default behaviour (or at least configurable behaviour) to respect
required
, as intuitively, this is what is expected.Source/destination types
Source JSON
Expected behavior
An error to be thrown, as
Field
isrequired
.Actual behavior
Newtonsoft deserialises happily, with
null
being the value ofField
, even though this is invalid as the field is non-nullable.Steps to reproduce