Reconsider the semantics of JsonProperty.ShouldSerialize null values

[eiriktsarpalis]

I'd expect ShouldSerialize = null brings me back default behavior from options and not be equivalent to always serialize.

Originally posted by @krwq in #71908 (comment)

Under the current implementation, a null ShouldSerialize value will result in an ignore policy being determined by either JsonSerializer.DefaultIgnoreCondition or JsonSerializer.IgnoreNullValues settings. This is despite the fact that we are documenting null as meaning "always serialize". While there are merits to the current approach there are a few downsides to be considered as well:

1. Type-specific configuration should override global configuration. As a user, explicitly setting ShouldSerialize = null; should clearly result in no serializability checks being made. This is similar to how JsonIgnoreAttribute overrides DefaultJsonIgnoreCondition.
2. As a custom contract resolver author, it is impossible to completely disable ShouldSerialize checks for every JsonSerializerOptions configuration unless I explicitly set a predicate that always returns true. Forcing an additional delegate on an otherwise very common usage scenario is counterintuitive and less efficient.
3. While a contract resolver does consult JsonSerializerOptions settings when doing its work, the resultant contract is no longer the single source of truth -- the final contract will instead depend on configuration that is non-modifiable by the resolver author.
4. The current handling of DefaultIgnoreCondition and IgnoreNullValues is inconsistent with how IgnoreReadOnlyProperties and IgnoreReadOnlyFields are treated. The latter group feeds into the contract model and is user-modifiable.

#71908 attempted to change this but it got reverted due to pushback. Nevertheless, I still think we should consider this.

[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

---

I'd expect ShouldSerialize = null brings me back default behavior from options and not be equivalent to always serialize.

Originally posted by @krwq in #71908 (comment)

Under the current implementation, a null ShouldSerialize value will result in an ignore policy being determined by either JsonSerializer.DefaultIgnoreCondition or JsonSerializer.IgnoreNullValues settings. This is despite the fact that we are documenting null as meaning "always serialize". While there are merits to the current approach there are a few downsides to be considered as well:

1. Type-specific configuration should override global configuration. As a user, explicitly setting ShouldSerialize = null; should clearly result in no serializability checks being made. This is similar to how JsonIgnoreAttribute overrides DefaultJsonIgnoreCondition.
2. As a custom contract resolver author, it is impossible to completely disable ShouldSerialize checks for every JsonSerializerOptions configuration unless I explicitly set a predicate that always returns true. Forcing an additional delegate on an otherwise very common usage scenario is counterintuitive and less efficient.
3. While a contract resolver does consult JsonSerializerOptions settings when doing its work, the resultant contract is no longer the single source of truth -- the final contract will instead depend on configuration that is non-modifiable by the resolver author.

#71908 attempted to change this but it got reverted due to pushback. Nevertheless, I still think we should consider this.

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

[krwq]

See my example here:

#71908 (comment)

also null defaulting to do what options do is more consistent with the rest of the framework (and also seems NSJ) than override default options behavior and always serialize

[eiriktsarpalis]

Given the issue surfaced by #72175 I think we can put this to bed for now. We can re-evaluate in the future.