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

[API Proposal]: Add more extensive control over ignoring properties #55781

Closed
rogerfar opened this issue Jul 15, 2021 · 10 comments
Closed

[API Proposal]: Add more extensive control over ignoring properties #55781

rogerfar opened this issue Jul 15, 2021 · 10 comments
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Text.Json
Milestone

Comments

@rogerfar
Copy link

Background and motivation

If I have this config:

services.AddControllers()
		.AddJsonOptions(options =>
		{
			options.JsonSerializerOptions.PropertyNamingPolicy = JsonNamingPolicy.CamelCase;
			options.JsonSerializerOptions.DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingDefault;
			options.JsonSerializerOptions.Converters.Add(new JsonStringEnumConverter());
		});

And I return the following:

[HttpGet]
[Route("Test")]
public ActionResult Test()
{
    return Ok(new
    {
        StringValue = "Prop",
        NullString = (string)null,
        DefaultNumber = 0,
        EnumValue = TestEnum.Val2,
        DefaultEnum = TestEnum.Val1
    });
}

The result is:

{
  "stringValue": "Prop",
  "enumValue": "Val2"
}

This can cause confusion because the value of DefaultEnum is omitted because it's default is, but because it's converted as a string this reduces clarity.

API Proposal

// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

namespace System.Text.Json.Serialization
{
    /// <summary>
    /// When specified on <see cref="JsonSerializerOptions.DefaultIgnoreCondition"/>,
    /// determines when properties and fields across the type graph are ignored.
    /// When specified on <see cref="JsonIgnoreAttribute.Condition"/>, controls whether
    /// a property is ignored during serialization and deserialization. This option
    /// overrides the setting on <see cref="JsonSerializerOptions.DefaultIgnoreCondition"/>.
    /// </summary>
    public enum JsonIgnoreCondition
    {
        /// <summary>
        /// Property is never ignored during serialization or deserialization.
        /// </summary>
        Never = 0,
        /// <summary>
        /// Property is always ignored during serialization and deserialization.
        /// </summary>
        Always = 1,
        /// <summary>
        /// If the value is the default, the property is ignored during serialization.
        /// This is applied to both reference and value-type properties and fields.
        /// </summary>
        WhenWritingDefault = 2,
        /// <summary>
        /// If the value is <see langword="null"/>, the property is ignored during serialization.
        /// This is applied only to reference-type properties and fields.
        /// </summary>
        WhenWritingNull = 3,
        /// <summary>
        /// If the value is the default, the property is ignored during serialization.
        /// This is applied to both reference and value-type properties and fields.
		/// When using JsonStringEnumConverter enum properties will be included.
        /// </summary>
        WhenWritingDefaultExceptEnums = 4,
    }
}

API Usage

services.AddControllers()
		.AddJsonOptions(options =>
		{
			options.JsonSerializerOptions.PropertyNamingPolicy = JsonNamingPolicy.CamelCase;
			options.JsonSerializerOptions.DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingDefaultExceptEnums;
			options.JsonSerializerOptions.Converters.Add(new JsonStringEnumConverter());
		});

Risks

No response

@rogerfar rogerfar added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jul 15, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Text.Json untriaged New issue has not been triaged by the area owner labels Jul 15, 2021
@ghost
Copy link

ghost commented Jul 15, 2021

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

If I have this config:

services.AddControllers()
		.AddJsonOptions(options =>
		{
			options.JsonSerializerOptions.PropertyNamingPolicy = JsonNamingPolicy.CamelCase;
			options.JsonSerializerOptions.DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingDefault;
			options.JsonSerializerOptions.Converters.Add(new JsonStringEnumConverter());
		});

And I return the following:

[HttpGet]
[Route("Test")]
public ActionResult Test()
{
    return Ok(new
    {
        StringValue = "Prop",
        NullString = (string)null,
        DefaultNumber = 0,
        EnumValue = TestEnum.Val2,
        DefaultEnum = TestEnum.Val1
    });
}

The result is:

{
  "stringValue": "Prop",
  "enumValue": "Val2"
}

This can cause confusion because the value of DefaultEnum is omitted because it's default is, but because it's converted as a string this reduces clarity.

API Proposal

// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

namespace System.Text.Json.Serialization
{
    /// <summary>
    /// When specified on <see cref="JsonSerializerOptions.DefaultIgnoreCondition"/>,
    /// determines when properties and fields across the type graph are ignored.
    /// When specified on <see cref="JsonIgnoreAttribute.Condition"/>, controls whether
    /// a property is ignored during serialization and deserialization. This option
    /// overrides the setting on <see cref="JsonSerializerOptions.DefaultIgnoreCondition"/>.
    /// </summary>
    public enum JsonIgnoreCondition
    {
        /// <summary>
        /// Property is never ignored during serialization or deserialization.
        /// </summary>
        Never = 0,
        /// <summary>
        /// Property is always ignored during serialization and deserialization.
        /// </summary>
        Always = 1,
        /// <summary>
        /// If the value is the default, the property is ignored during serialization.
        /// This is applied to both reference and value-type properties and fields.
        /// </summary>
        WhenWritingDefault = 2,
        /// <summary>
        /// If the value is <see langword="null"/>, the property is ignored during serialization.
        /// This is applied only to reference-type properties and fields.
        /// </summary>
        WhenWritingNull = 3,
        /// <summary>
        /// If the value is the default, the property is ignored during serialization.
        /// This is applied to both reference and value-type properties and fields.
		/// When using JsonStringEnumConverter enum properties will be included.
        /// </summary>
        WhenWritingDefaultExceptEnums = 4,
    }
}

API Usage

services.AddControllers()
		.AddJsonOptions(options =>
		{
			options.JsonSerializerOptions.PropertyNamingPolicy = JsonNamingPolicy.CamelCase;
			options.JsonSerializerOptions.DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingDefaultExceptEnums;
			options.JsonSerializerOptions.Converters.Add(new JsonStringEnumConverter());
		});

Risks

No response

Author: rogerfar
Assignees: -
Labels:

api-suggestion, area-System.Text.Json, untriaged

Milestone: -

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Jul 16, 2021

Exposing a separate setting only to exempt JsonStringEnumConverter in particular seems a bit ad-hoc to me. Perhaps one alternative might be to expose a setting that disables ignored defaults when a custom converter is being used for a type.

Related to #50294. It seems clear that WhenWritingDefault is too much of a blunt instrument when applied at the JsonSerializerOptions level and too noisy when applied at the individual property level.

What we could do instead is expose a mechanism for omitting properties at the converter level, for example:

public class JsonConverter<T>
{
    public virtual bool IgnoreProperty(T propertyValue, JsonSerializerOptions options) => 
       // naive, partial implementation adopted for brevity
       options.DefaultIgnoreCondition == JsonIgnoreCondition.WhenWritingDefault &&
       EqualityComparer<T>.Default.Equals(propertyValue, default);
}

and thus completely opt out JsonStringEnumConverter by default:

public class StringEnumConverter<TEnum>
{
    public override bool IgnoreProperty(TEnum _, JsonSerializerOptions options) => false;
}

cc @layomia @NinoFloris

@eiriktsarpalis eiriktsarpalis added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed untriaged New issue has not been triaged by the area owner labels Jul 16, 2021
@eiriktsarpalis eiriktsarpalis added this to the 7.0.0 milestone Jul 16, 2021
@NinoFloris
Copy link
Contributor

@eiriktsarpalis the proposal looks good to me, the only question to me would be whether the IgnoreProperty method is additive to or entirely overriding JsonSerializerOptions.JsonIgnoreCondition. Overriding is more powerful but I'd rather not come across too many 'constant false' converters that completely fail to respect any JsonIgnoreCondition. So it'd need some good guiding examples of how to do it right, either by looking up JsonIgnoreCondition in options or having specific converter options for it.

@rogerfar
Copy link
Author

rogerfar commented Jul 16, 2021

Would it be an idea to make DefaultIgnoreCondition not just an enum but an options object that accept multiple conditions? It will be a breaking change but not a very difficult one to fix.

@eiriktsarpalis
Copy link
Member

We might also want to consider #40395 for ignoring properties on the property level.

@nathan-alden-sr
Copy link

I would love this capability, especially as outlined in #55781 (comment). In my case, I am trying to serialize null or empty IList<T> references to null, but I want to honor the JsonIgnoreCondition, meaning I don't want the property serialized at all if the result of my algorithm would write null.

public static void Main()
{
    string json = JsonSerializer.Serialize(
        new Test(),
        new JsonSerializerOptions { DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, WriteIndented = true });

    Console.WriteLine(json);
}

public class Test
{
    [JsonPropertyName("foos")]
    [JsonInclude]
    [JsonConverter(typeof(EmptyOrNullListConverter<string>))]
    public IList<string> Foos { get; } = new List<string>();
}

public class EmptyOrNullListConverter<T> : JsonConverter<IList<T>>
{
    public override bool HandleNull => true;

    public override IList<T> Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) => default;

    public override void Write(Utf8JsonWriter writer, IList<T>? value, JsonSerializerOptions options) =>
        JsonSerializer.Serialize(writer, value?.Count > 0 ? value : null, options);
}

This gets me just about all the way there, but always writes "foos": null regardless of JsonIgnoreCondition.

@eiriktsarpalis eiriktsarpalis removed the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 16, 2021
@scottdorman
Copy link

I'm in need of a similar solution to what @nathan-alden-sr mentioned. As there seem to be multiple issues relating to this scenario, there is a definite need for the functionality built-in to STJ. It's not currently possible with converters (or anything else that I can find) today. I commented on a related issue #36785 (comment) that has more specifics for the scenario I'm encountering.

@DavidErben
Copy link

@nathan-alden-sr I have posted a way to do this, maybe it works for you. You can check it our here:

#36785 (comment)

@robertmclaws
Copy link

See my solution here: #593 (comment)

@eiriktsarpalis
Copy link
Member

I believe this could be addressed by #63686. We should make sure that this particular use case is covered and potential write a code sample demonstrating it.

Regarding converter-level predicates (#55781 (comment)), I'm increasingly of the opinion that it should be addressed on the contract model instead. Closing in favor of #63686.

@ghost ghost locked as resolved and limited conversation to collaborators Feb 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Text.Json
Projects
None yet
Development

No branches or pull requests

7 participants