Sample to customize what STJ considers a 'JSON null'/Ignoring property writing

[NinoFloris]

The following issue was closed #55781 (comment) with the mention this is solved by contract resolvers, I'm just not sure how this works (and ideally without boxing).

I'd be curious to the solution/sample to solve the following repro:

Given the following converter and code:

type JsonValueOptionConverter<'T>(converter: JsonConverter) =
    inherit JsonConverter<ValueOption<'T>>()

    let valueTy = typeof<'T>
    let valueConverter = converter :?> JsonConverter<'T>

    override _.Read(reader, _, options) =
        match reader.TokenType with
        | JsonTokenType.Null -> ValueNone
        | _ -> ValueSome(valueConverter.Read(&reader, valueTy, options))
    override _.Write(writer, value, options) =
        match value with
        | ValueNone ->
            // TODO revisit after https://github.com/dotnet/runtime/issues/33433
            // and https://github.com/dotnet/runtime/issues/50294
            writer.WriteNullValue()
        | ValueSome x -> valueConverter.Write(writer, x, options)

type JsonValueOptionConverter() =
    inherit JsonConverterFactory()
    static let valueTyDef = typedefof<ValueOption<_>>
    static let converterTyDef = typedefof<JsonValueOptionConverter<_>>
    override _.CanConvert(ty) = ty.IsConstructedGenericType && valueTyDef = (ty.GetGenericTypeDefinition())
    override _.CreateConverter(ty, options) = Activator.CreateInstance(converterTyDef.MakeGenericType(ty.GenericTypeArguments.[0]), options.GetConverter(ty.GenericTypeArguments.[0])) :?> JsonConverter
    
type Data =
    {
        Property: string voption
    }

let options =
    let options = JsonSerializerOptions()
    options.Converters.Add(JsonValueOptionConverter())
    options.DefaultIgnoreCondition <- JsonIgnoreCondition.WhenWritingNull // something here??

// Expect:
JsonSerializer.Serialize({ Property = None }, options) = "{}"
// Actual: 
JsonSerializer.Serialize({ Property = None }, options) = "{ "Property": null }"

How would I get to the expected output app wide?

@eiriktsarpalis

[eiriktsarpalis]

Ignore conditions are generally specified on the property level and not the type level. Converters are not the appropriate abstraction to configure this -- instead this is done globally using the DefaultIgnoreCondition property, on individual properties using the JsonIgnoreAttribute or via the contract model using the ShouldSerialize predicate. When configured by the user, the ShouldSerialize predicate does force boxing of the property value, but we plan on exposing strongly typed variants in the future.

Note that JsonIgnoreCondition.WhenWritingNull only impacts properties with reference types, you need JsonIgnoreCondition.WhenWritingDefault to extend this to value types. What's more STJ already supports F# voption OOTB, so I get the expected behavior by changing your code to the following:

type Data =
    {
        Property: string voption
    }

let options = JsonSerializerOptions(DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingDefault)
JsonSerializer.Serialize({ Property = ValueNone }, options) |> printfn "%s" // {}

[NinoFloris]

Let me add another constraint, we've been going in circles for years on this and it seems like there still isn't a decent way to do this in STJ.

type Data =
    {
        Property: string voption
        Number: int
    }

let options = JsonSerializerOptions(DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingDefault)
JsonSerializer.Serialize({ Property = ValueNone, Number = 1 }, options) |> printfn "%s" // expect { "Number": 1 }
JsonSerializer.Serialize({ Property = ValueNone; Number = 0 }, options) |> printfn "%s" // expect { "Number": 0 }

WhenWritingDefault is terrible, nobody wants default values for integers to disappear.

I don't want all default values to disappear, this is not friendly for untyped clients to deal with. I do want any json nulls to cause properties to be omitted.

I want some way to mark a converter as a 'nullable-ish' handler. ValueOption is a struct that has a specific value for null which should cause WhenWritingNull semantics to kick in for that particular value, just like it does for System.Nullable which is also not a reference type.

WhenWritingNull is already inconsistent, using that mode fully applies to System.Nullable. Why is there still no extensibility to do this at the converter level while all the issues pointing this out have been closed over the years as being duplicate, solved, etc?

Nullable and WhenWritingNull:

    type Data =
        {
            NullableInt: Nullable<int>
        }

    [<EntryPoint>]
    let main args =
        let options = JsonSerializerOptions(DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull)
        JsonSerializer.Serialize({ NullableInt = Nullable() }, options) |> printfn "%s" // {}

[eiriktsarpalis]

WhenWritingDefault is terrible, nobody wants default values for integers to disappear.

I don't want all default values to disappear, this is not friendly for untyped clients to deal with. I do want any json nulls to cause properties to be omitted.

Maybe, but that ship has sailed since the feature got shipped years ago. If you don't want that behaviour, apply it on a per-property basis using an attribute. If that is too involved, write a contract resolver that applies a ShouldSerialize predicate to properties that are of a specific type.

WhenWritingNull is already inconsistent, using that mode fully applies to System.Nullable.

Why do you think it is inconsistent? Doesn't System.Nullable allow null?

Why is there still no extensibility to do this at the converter level while all the issues pointing this out have been closed over the years as being duplicate, solved, etc?

We made the determination that this extensibility should live at the JsonPropertyInfo level and not converter level. Are there any specific issues with JsonPropertyInfo.ShouldSerialize that are preventing you from using it?

[NinoFloris]

Why do you think it is inconsistent? Doesn't System.Nullable allow null?

Yes we can get into the technicalities here (and believe me I understand the nuances) but the xmldoc for WhenWritingNull speaks of 'reference types' and not 'nullable types'. System.Nullable is certainly special but it's not a reference type.

What was designed incorrectly IMO in the serializer is the choice to equate only CLR nulls to json nulls. This was not done for any of the other token types like numbers, arrays, objects etc.

You write a custom converter to make STJ respect the converted type's particularities by taking control over its handling. For a converter during serialization what matters is the output, so naturally you want your converter be able to take into account its json null behavior as well, not just when they happen to be CLR nulls. (just like we can on the deserialization side!)

If that would have been the approach then naturally it would fall out that there should be an api on converters to tell the serializer what values in the converted type's domain map to a json null. Either as an equality predicate or as a configurable set of values. The STJ assumption that CLR null and only CLR null = json null is bolted into the serializer and that is precisely what I want to be able to customize. Skippable<'T> in FSharp.SystemTextJson is precisely one of these other types that STJ can't serialize correctly (when it comes to respecting its 'nulls' and JsonIgnoreCondition) https://github.com/Tarmil/FSharp.SystemTextJson/blob/master/src/FSharp.SystemTextJson/Skippable.fs

We made the determination that this extensibility should live at the JsonPropertyInfo level and not converter level. Are there any specific issues with JsonPropertyInfo.ShouldSerialize that are preventing you from using it?

Most prominently the boxing, why would somebody use ValueOption or Skippable if it gets boxed at the edges due to a STJ mandated callback anyway... (exactly the same would be true for System.Nullable if it didn't have builtin support)

I strongly believe that to properly deal with mapping structs to json there needs to be extensibility in determining which of its values map to json null values. This mapping is a type level detail and should live with the other type level conversion logic in the converter.

[eiriktsarpalis]

Why do you think it is inconsistent? Doesn't System.Nullable allow null?

Yes we can get into the technicalities here (and believe me I understand the nuances) but the xmldoc for WhenWritingNull speaks of 'reference types' and not 'nullable types'. System.Nullable is certainly special but it's not a reference type.

I think this is an issue of API documentation being incomplete. We should update it to say that it applies to nullable types and not reference types specifically. The source for truth for API documentation is the dotnet-api-docs repo.

What was designed incorrectly IMO in the serializer is the choice to equate only CLR nulls to json nulls. This was not done for any of the other token types like numbers, arrays, objects etc.

As designed, the ignore feature makes a determination on whether a property should be skipped based on the .NET value it contains. It does not apply to root values and it does not apply to collection elements. I wouldn't agree that it's incorrect, if anything making an "ignore" judgement based on what JSON tokens a converter wrote on the wire definitely sounds like putting the cart before the horse.

You write a custom converter to make STJ respect the converted type's particularities by taking control over its handling.

I get your point, however this would need to be new virtual method added to the converter type and not something that could be articulated in Write method overrides: the invariants of the Write method dictate that a complete JSON value must be written to the underlying Utf8JsonWriter no matter what -- it does not support writing nothing and signalling to the parent object converter that it should erase the JSON property name that it just wrote (which may have already been flushed by the underlying stream).

Because ignoring only really applies to properties, and ignore predicates are scoped to individual properties and not types, we made the conscious decision to expose the predicate on JsonPropertyInfo instead.

Most prominently the boxing, why would somebody use ValueOption or Skippable if it gets boxed at the edges due to a STJ mandated callback anyway...

As already mentioned, we are planning on addressing this in the future via a strongly typed JsonPropertyInfo<T>. The infrastructure is there already but it's marked internal for the moment. That being said, it would be interesting to see benchmarks showcasing performance regression associated with boxing in user-specified predicates.

[NinoFloris]

I think this is an issue of API documentation being incomplete. We should update it to say that it applies to nullable types and not reference types specifically. The source for truth for API documentation is the dotnet-api-docs repo.

I'll open an issue, maybe even a PR.

As designed, the ignore feature makes a determination on whether a property should be skipped based on the .NET value it contains

I understand this is the design but this doesn't have much to do with what CLR values trigger this property ignore behavior.

Where - just on properties - the behavior applies is OK as far as I'm concerned. It's CLR centric as it does not apply to ExtensionData 'properties' for instance (I'm not fully happy with this but that's ok, you have a serviceable workaround of removing the key in such cases). When a user wants more ad-hoc ignore behaviors/predicates that is indeed something that belongs in the contract. We're aligned on this so far.

However 'when' (i.e. which values apply to this behavior) should IMO be expanded to respect some configurable nullability on a converter.

I get your point, however this would need to be new virtual method added to the converter type and not something that could be articulated in Write method overrides

Correct, I was not implying that it should be rolled into Write.

Maybe we can even come to a compromise and instead of having a full blown predicate api like IsValueJsonNull on converters we could introduce an attribute or a flag on converters that would signal its default value should be interpreted as null. This would work for ValueOption given ValueNone = default and it would also work for Skippable given Skip = default. I haven't seen much evidence that there is a need for more than one json null <-> clr value mapping.

As already mentioned, we are planning on addressing this in the future via a strongly typed JsonPropertyInfo. The infrastructure is there already but it's marked internal for the moment.

We have numerous api types where over 70% of their properties are specified as some instantiation of ValueOption/Skippable. Our most extreme use of Skippable would be our patch endpoints where effectively every property is optional. Though on other endpoints there are quite some as well, either for properties we apply a default for if not provided (think ids or flags) or where it really just is optional data.

That being said, it would be interesting to see benchmarks showcasing performance regression associated with boxing in user-specified predicates.

As you know any heap allocation always has some impact, it's just a question of how much total machine throughput you're fine with sacrificing. This is a general truth which doesn't need minutia benchmarks to be performed before it can be applied. Completely agreed that there is value in having a benchmark to evaluate the effectiveness of moving to strongly typed predicates though as that implies more than just eliding a box.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

---

The following issue was closed #55781 (comment) with the mention this is solved by contract resolvers, I'm just not sure how this works (and ideally without boxing).

I'd be curious to the solution/sample to solve the following repro:

Given the following converter and code:

type JsonValueOptionConverter<'T>(converter: JsonConverter) =
    inherit JsonConverter<ValueOption<'T>>()

    let valueTy = typeof<'T>
    let valueConverter = converter :?> JsonConverter<'T>

    override _.Read(reader, _, options) =
        match reader.TokenType with
        | JsonTokenType.Null -> ValueNone
        | _ -> ValueSome(valueConverter.Read(&reader, valueTy, options))
    override _.Write(writer, value, options) =
        match value with
        | ValueNone ->
            // TODO revisit after https://github.com/dotnet/runtime/issues/33433
            // and https://github.com/dotnet/runtime/issues/50294
            writer.WriteNullValue()
        | ValueSome x -> valueConverter.Write(writer, x, options)

type JsonValueOptionConverter() =
    inherit JsonConverterFactory()
    static let valueTyDef = typedefof<ValueOption<_>>
    static let converterTyDef = typedefof<JsonValueOptionConverter<_>>
    override _.CanConvert(ty) = ty.IsConstructedGenericType && valueTyDef = (ty.GetGenericTypeDefinition())
    override _.CreateConverter(ty, options) = Activator.CreateInstance(converterTyDef.MakeGenericType(ty.GenericTypeArguments.[0]), options.GetConverter(ty.GenericTypeArguments.[0])) :?> JsonConverter
    
type Data =
    {
        Property: string voption
    }

let options =
    let options = JsonSerializerOptions()
    options.Converters.Add(JsonValueOptionConverter())
    options.DefaultIgnoreCondition <- JsonIgnoreCondition.WhenWritingNull // something here??

// Expect:
JsonSerializer.Serialize({ Property = None }, options) = "{}"
// Actual: 
JsonSerializer.Serialize({ Property = None }, options) = "{ "Property": null }"

How would I get to the expected output app wide?

@eiriktsarpalis

Author:	NinoFloris
Assignees:	-
Labels:	area-System.Text.Json, untriaged
Milestone:	-

[eiriktsarpalis]

Maybe we can even come to a compromise and instead of having a full blown predicate api like IsValueJsonNull on converters we could introduce an attribute or a flag on converters that would signal its default value should be interpreted as null. This would work for ValueOption given ValueNone = default and it would also work for Skippable given Skip = default. I haven't seen much evidence that there is a need for more than one json null <-> clr value mapping.

I'll point out that "IsValueJsonNull" is not the same as "SkipIfIsValueJsonNull". I don't think there could be any general-purpose converter that unconditionally signals that the JSON nulls it produces should be skipped if placed on a property. At the same time, I don't think that property skippability is inherently tied to the .NET value being null/default or even its JSON encoding being null.

Generally speaking I find that skippability is context-specific, depending on JsonSerializerOptions configuration or property-level configuration. In other words, it is configuration inherent in the property contract, not the type contract. For opinionated custom converters that absolutely need to mandate that every null manifestation on property must be skipped regardless of context, we might consider exposing a contract customization callback on converters:

public class JsonConverter<T>
{
     public virtual void OnPropertyInfoCreated(JsonPropertyInfo propertyInfo);
}

which can be used as follows:

type JsonValueOptionConverter<'T>(converter: JsonConverter) =
    inherit JsonConverter<'T voption>()

    override _.OnPropertyInfoCreated(JsonPropertyInfo propertyInfo) =
         propertyInfo.ShouldSerialize <- new Func<obj,obj,_>(fun _ value -> match value :?> 'T voption with ValueSome _ -> true | _ -> false)

But I don't see much value of such a gadget over just using contract customization directly. The semantics also become a bit hairy once you take into account that converters can be configured on a per-property basis using the JsonPropertyInfo.CustomConverter property.

Another possibility is that we regard rendering of null properties as a formatting issue, similar to how we configure indentation. That might be exposed as a property on JsonWriterOptions:

public struct JsonWriterOptions
{
    public bool SkipNullProperties { get; set; } = false;
    public bool SkipNullArrayElements { get; set; } = false;
}

[gregsdennis]

What was designed incorrectly IMO in the serializer is the choice to equate only CLR nulls to json nulls.

This has been my argument for a long time.

See also #68128 (and all associated and linked issues).

[NinoFloris]

Skipping is indeed a property level concern. But to reiterate: when to skip is hardcoded or immediately so flexible it brings boxing overhead making all current approaches unfit.

Today we have these broad categories for skipping on write:

• CLR null (WhenWritingNull, I propose to slightly widen the potential set this applies to)
• CLR default (WhenWritingDefault, I can't be convinced this is actually what anybody wants)
• ShouldSerialize, delegate that boxes (I don't know the typed approach well enough to know if in return for no boxing it doesn't add some new overhead)

My proposal really comes down to allowing converters to redefine their 'null' value. Unconfigured this will just be a CLR null (or nothing if its a struct type, just like today) but configured it could point to default(T) or some other value. What the api for this would look like could be (among many other welcome variations):

• IsValueNull: bool (T value)
• NullValue(s): T or T[]
• DefaultAsNull: bool

As you see it is decoupled from skipping entirely. Skipping just taps into this to understand when to skip if WhenWritingNull is specified. IMO decoupling this has the added benefit that when there are other concerns that want to understand a type's nullability mapping they can do so. The values which should be treated as null are a type invariant.

All of these could even be designed to throw if they're supplied for reference types in my opinion, I really just care about the structs here.

My ideal mechanism would be:

• no allocations
• minimal added overhead per property to have the desired behavior (ideally just a branch)
• converter driven

The writer solution you proposed is an example that probably won't work because I assume it would require entire properties to be buffered when this mode is enabled, or a full second pass to 'format'. There is no problem with the writer as is, it's the serializer which should check the precondition of some form of 'should this property be serialized' after all.

I don't think there could be any general-purpose converter that unconditionally signals that the JSON nulls it produces should be skipped if placed on a property

I had some difficulty grasping the nuance you tried to add here. Just to be sure, I'm not suggesting the serializer should in one or the other form 'inspect' what calls the converter makes/made to the writer to determine whether it should ignore anything or not.

[eiriktsarpalis]

My proposal really comes down to allowing converters to redefine their 'null' value.

I'm not sure what that means. Is it effectively a way for the converter to declare that it will be deserializing the particular values as JSON null ahead of time? If that's the case, how would properties opt into skipping when it detects that said values are contained in the property? By repurposing the meaning of WhenWritingNull and WhenWritingDefault to work with arbitrary values? That doesn't sound right to me.

What's more, it would be a breaking change. When designing the contract model, we realized that there are limits to what can be expressed with JsonIgnoreCondition (and it has issues, for one it conflates .NET nulls with JSON nulls as you pointed out earlier). So it does not exist at all in that layer, instead it maps the enum to ShouldSerialize delegates, which are the sole source of truth when ignoring properties:

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs

Lines 344 to 387 in 2bdc3cb

	private protected override void ConfigureIgnoreCondition(JsonIgnoreCondition? ignoreCondition)
	{
	switch (ignoreCondition)
	{
	case null:
	break;
	case JsonIgnoreCondition.Never:
	ShouldSerialize = ShouldSerializeIgnoreConditionNever;
	break;
	case JsonIgnoreCondition.Always:
	ShouldSerialize = ShouldSerializeIgnoreConditionAlways;
	break;
	case JsonIgnoreCondition.WhenWritingNull:
	if (PropertyTypeCanBeNull)
	{
	ShouldSerialize = ShouldSerializeIgnoreWhenWritingDefault;
	IgnoreDefaultValuesOnWrite = true;
	}
	else
	{
	ThrowHelper.ThrowInvalidOperationException_IgnoreConditionOnValueTypeInvalid(MemberName!, DeclaringType);
	}
	break;
	case JsonIgnoreCondition.WhenWritingDefault:
	ShouldSerialize = ShouldSerializeIgnoreWhenWritingDefault;
	IgnoreDefaultValuesOnWrite = true;
	break;
	default:
	Debug.Fail($"Unknown value of JsonIgnoreCondition '{ignoreCondition}'");
	break;
	}
	static bool ShouldSerializeIgnoreConditionNever(object _, T? value) => true;
	static bool ShouldSerializeIgnoreConditionAlways(object _, T? value) => false;
	static bool ShouldSerializeIgnoreWhenWritingDefault(object _, T? value)
	{
	return default(T) is null ? value is not null : !EqualityComparer<T>.Default.Equals(default, value);
	}
	}

I think your best bet might be to wait for the strongly-typed JsonPropertyInfo<T> and ShouldSerialize delegate to be user-accessible, although like I said it would be interesting to see real benchmarks showing the effect of boxing. The serializer itself does a whole lot of allocating/boxing of its own when walking sufficiently complex object graphs anyway, so it might not be as pronounced as you might have expected.

[NinoFloris]

Is it effectively a way for the converter to declare that it will be deserializing the particular values as JSON null ahead of time?

Correct it would be a way to configure WhenWritingNull to allow converters to point to a null value for struct types. I understand it's probably undesirable but it shouldn't be a breaking change for user converters given they would need to opt into overriding the new member.

For the converters that come with STJ I see how that may be a different story when people already use WhenWritingNull and ValueOption and have tests to check their output. However as ValueOption is semantically the same as Option and one ends up with one result, a property with null, while the other has no property it could be argued it's ok to break to bring consistency, but that's not up to me.

So it does not exist at all in that layer, instead it maps the enum to ShouldSerialize delegates, which are the sole source of truth when ignoring properties:

I assumed as much. It doesn't help much at this moment without access to the generic form though.

[eiriktsarpalis]

I don't think controlling this behaviour on the converter level is viable given the current shipped behavior. You would need to rely on the property contract model instead.

I've created #82720 since apparently we hadn't created a tracking issue specifically for generic JsonPropertyInfo support.