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

Implicit vs Explicit Null (aka Null vs Not-Set) - JSON and WebAPI Related #53868

Closed
TonyValenti opened this issue Jun 8, 2021 · 11 comments
Closed
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json untriaged New issue has not been triaged by the area owner

Comments

@TonyValenti
Copy link

Background and Motivation

Dear .NET Team,
I do a lot of work in web systems for .NET and I've noticed a common problem that lots of web API developers bump into.
I believe that .net can easily solve this problem via a source generator or some other technology. I am opening this issue with the hope that you will build an official solution that will completely eliminate a significant source of problems and challenges.

Here is some background on the problem:
When building update APIs, there should be a differentiation between a null value and a not-set value.

For example, with the following request:

PUT /api/v1/contacts/21
{"last_name": "Smith", "parent": null}

We can expect a query like this to execute:

update contacts set last_name = 'Smith', parent = null where Id = 21

And with this request:

PUT /api/v1/contacts/21
{"last_name": "Smith"}

We can expect a query like this to execute:

update contacts set last_name = 'Smith' where Id = 21

However, in C# today, it takes a lot of special work to ensure that happens. Likely, both requests will result in the same query running. This is because a developer would likely write the following code:

public class UpdateContactRequest {
  public string Last_Name {get; set;}
  public long? Parent {get; set; }
}

public async Task UpdateContact(long ContactId, UpdateContactRequest Request){
  //Update contacts set last_name = Request.Last_Name, parent = request.Parent where Id = ContactId
}

And in the above, there will be no difference at all between what happens regardless of if Parent is null because it was not provided or it is null because it is explicitly set to null. As a result of this, a lot of .NET JSON "Update" APIs require that developers pass in every single property of their object again or risk it being set to null/default(t).

To work around this, I have started seeing developers keep track of what properties have had their setters invoked and being smart about only updating those properties.

Proposed API

The .NET Team can greatly simplify this challenge by introducing a new source generator:
[TrackSetProperties]

When applied to a class, code like this:

[TrackSetProperties]
    public class UpdateContactRequest
    {
        public string Last_Name { get; set; }
        public long? Parent { get; set; }
    }

Becomes:

public interface ITrackSetProperties {
  bool IsPropertySet(string PropertyName);
  IDictionary<string, object> GetSetProperties()
}

    public class UpdateContactRequest : ITrackSetProperties
    {
        private string __Last_Name;
        private long? __Parent;

        public string Last_Name
        {
            get
            {
                return __Last_Name;
            }
            set
            {
                __Last_Name = value;
                SetProperty();
            }
        }

        public long? Parent
        {
            get
            {
                return __Parent;
            }
            set
            {
                __Parent = value;
                SetProperty();
            }
        }

        private HashSet<string> SetPropertyNames { get; } = new();
        protected virtual bool SetProperty([CallerMemberName] string PropertyName = default)
        {
            return SetPropertyNames.Add(PropertyName);
        }

        public virtual bool IsPropertySet(string PropertyName)
        {
            return SetPropertyNames.Contains(PropertyName);
        }

        public virtual IDictionary<string, object> GetSetProperties()
        {
            var ret = new Dictionary<string, object>();

            if (IsPropertySet(nameof(Last_Name)))
            {
                ret[nameof(Last_Name)] = Last_Name;
            }

            if (IsPropertySet(nameof(Parent)))
            {
                ret[nameof(Parent)] = Parent;
            }


            return ret;
        }

    }

I hope the above information is clear and that you can see the value in this addition.

Alternative Designs

I expect that we will see some developers who will start to interrogate the JSON dom directly once dotnet/designs#163 is fully released. This has the limitation of tying a framework specifically to json and will likely be error prone due to working with magic strings.

Risks

None

Other Notes

The Json serializer could be enhanced such that if a serialized object implements ITrackSetProperties, it will only serialize set properties instead of all properties.

@steveharter @jkotas @ericstj

@TonyValenti TonyValenti added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jun 8, 2021
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jun 8, 2021
@Joe4evr
Copy link
Contributor

Joe4evr commented Jun 8, 2021

You could use a wrapper type, and #35649 from preventing it getting written out:

[JsonIgnore(DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingDefault)]
[JsonConverter(typeof(OptionalConverterFactory))]
public Optional<long?> Parent { get; set; }

@ghost
Copy link

ghost commented Jun 8, 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

Dear .NET Team,
I do a lot of work in web systems for .NET and I've noticed a common problem that lots of web API developers bump into.
I believe that .net can easily solve this problem via a source generator or some other technology. I am opening this issue with the hope that you will build an official solution that will completely eliminate a significant source of problems and challenges.

Here is some background on the problem:
When building update APIs, there should be a differentiation between a null value and a not-set value.

For example, with the following request:

PUT /api/v1/contacts/21
{"last_name": "Smith", "parent": null}

We can expect a query like this to execute:

update contacts set last_name = 'Smith', parent = null where Id = 21

And with this request:

PUT /api/v1/contacts/21
{"last_name": "Smith"}

We can expect a query like this to execute:

update contacts set last_name = 'Smith' where Id = 21

However, in C# today, it takes a lot of special work to ensure that happens. Likely, both requests will result in the same query running. This is because a developer would likely write the following code:

public class UpdateContactRequest {
  public string Last_Name {get; set;}
  public long? Parent {get; set; }
}

public async Task UpdateContact(long ContactId, UpdateContactRequest Request){
  //Update contacts set last_name = Request.Last_Name, parent = request.Parent where Id = ContactId
}

And in the above, there will be no difference at all between what happens regardless of if Parent is null because it was not provided or it is null because it is explicitly set to null. As a result of this, a lot of .NET JSON "Update" APIs require that developers pass in every single property of their object again or risk it being set to null/default(t).

To work around this, I have started seeing developers keep track of what properties have had their setters invoked and being smart about only updating those properties.

Proposed API

The .NET Team can greatly simplify this challenge by introducing a new source generator:
[TrackSetProperties]

When applied to a class, code like this:

[TrackSetProperties]
    public class UpdateContactRequest
    {
        public string Last_Name { get; set; }
        public long? Parent { get; set; }
    }

Becomes:

public interface ITrackSetProperties {
  bool IsPropertySet(string PropertyName);
  IDictionary<string, object> GetSetProperties()
}

    public class UpdateContactRequest : ITrackSetProperties
    {
        private string __Last_Name;
        private long? __Parent;

        public string Last_Name
        {
            get
            {
                return __Last_Name;
            }
            set
            {
                __Last_Name = value;
                SetProperty();
            }
        }

        public long? Parent
        {
            get
            {
                return __Parent;
            }
            set
            {
                __Parent = value;
                SetProperty();
            }
        }

        private HashSet<string> SetPropertyNames { get; } = new();
        protected virtual bool SetProperty([CallerMemberName] string PropertyName = default)
        {
            return SetPropertyNames.Add(PropertyName);
        }

        public virtual bool IsPropertySet(string PropertyName)
        {
            return SetPropertyNames.Contains(PropertyName);
        }

        public virtual IDictionary<string, object> GetSetProperties()
        {
            var ret = new Dictionary<string, object>();

            if (IsPropertySet(nameof(Last_Name)))
            {
                ret[nameof(Last_Name)] = Last_Name;
            }

            if (IsPropertySet(nameof(Parent)))
            {
                ret[nameof(Parent)] = Parent;
            }


            return ret;
        }

    }

I hope the above information is clear and that you can see the value in this addition.

Alternative Designs

I expect that we will see some developers who will start to interrogate the JSON dom directly once dotnet/designs#163 is fully released. This has the limitation of tying a framework specifically to json and will likely be error prone due to working with magic strings.

Risks

None

Other Notes

The Json serializer could be enhanced such that if a serialized object implements ITrackSetProperties, it will only serialize set properties instead of all properties.

@steveharter @jkotas @ericstj

Author: TonyValenti
Assignees: -
Labels:

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

Milestone: -

@TonyValenti
Copy link
Author

@Joe4evr , while that is possible, that results messy json being written out and necessary. You would essentially have the following:

{
  "first_name": {
    "value": "John"
  },
  "last_name": {
    "value": "Smith"
  },
  "parent": {
    "value": 21
  }
}

@FiniteReality
Copy link

FiniteReality commented Jun 8, 2021

{
  "first_name": {
    "value": "John"
  },
  "last_name": {
    "value": "Smith"
  },
  "parent": {
    "value": 21
  }
}

That's not the case if you write the converter properly: Optional<T> should just use the converter for T internally.

@FiniteReality
Copy link

FiniteReality commented Jun 8, 2021

I personally think that the proposed solution doesn't scale well, and is incredibly awkward to use. However, it is absolutely a scenario that happens often enough that I think System.Text.Json should provide a built-in solution. The Optional<T> solution isn't great, but it's a lot better than just meshing together both states when it's really important.

Along those lines then, how about something like this?

namespace System.Text.Json.Serialization;

public struct Optional<T>
{
    public bool IsPresent { get; }
    public T Value { get; }

    public static Optional<T> NotPresent => default;

    public Optional(T value) { throw null; }

    public T GetValueOrDefault(T @default) { throw null; }

    public static implicit operator Optional<T>(T value) { throw null; }
    public static explicit operator T(Optional<T> optional) { throw null; }

    // Override Equals, GetHashCode, ToString, equality operators, etc.
}

Then, internally, there'd be a built-in converter factory for Optional<T>, which would produce converters which delegate to the appropriate converter for T. Furthermore, JsonSerializer would be augmented to correctly handle properties of type Optional<T>, skipping the property entirely when writing if the value is equal to Optional<T>.NotPresent.

On the naming of IsPresent vs Nullable<T>'s HasValue, I think IsPresent communicates more clearly that Optional is communicating the presence or lack thereof of a field, rather than the presence or lack thereof of a value.

@TonyValenti
Copy link
Author

TonyValenti commented Jun 16, 2021

@Joe4evr @FiniteReality -
Thank you very much for your suggestions. I'm almost there!
Below is the code that I have thus far. It works perfectly with one caveat:

Right now, it seems like I have to specify:

[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)]

on every single property of type Optional<T>. Is there a way that I can apply that attribute globally to Optional? I want to do the following but it doesn't work:

    [JsonConverter(typeof(OptionalJsonConverter))]
    [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)]
    public struct Optional<T>  { 
        /*...*/ 
}
    

All my code thus far:

#nullable enable

using System;
using System.Diagnostics.CodeAnalysis;
using System.Reflection;
using System.Text.Json;
using System.Text.Json.Serialization;

namespace OptionalTests {
    class Program {
        static void Main(string[] args) {
            var Input = new Person() {
                Id = 24,
                Age = 31,
                FirstName = "John",
                //LastName = "Smith"
            };

            var InputText = JsonSerializer.Serialize(Input);

            var Output = JsonSerializer.Deserialize<Person>(InputText);
            
            var OutputText = JsonSerializer.Serialize(Output);

            Console.WriteLine(InputText);
            Console.WriteLine(OutputText);

        }
    }

    public record Person {
        [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)]
        public Optional<long> Id { get; set; }


        [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)]
        public Optional<string> FirstName { get; set; }


        [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)]
        public Optional<string> LastName { get; set; }
        
        
        [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)]
        public Optional<int> Age { get; set; }
    }

    public static class Optional {
        public static Optional<T> Create<T>(T Value) {
            return new Optional<T>(Value);
        }
    }

    

    [JsonConverter(typeof(OptionalJsonConverter))]
    public struct Optional<T> 
        : IOptional {
        public bool IsPresent { get; }
        public T? Value { get; }

        public static Optional<T> NotPresent => default;

        bool IOptional.IsPresent => IsPresent;
        object? IOptional.Value => Value;

        public Optional(T? Value) {
            this.Value = Value;
            this.IsPresent = true;
        }

        public static implicit operator Optional<T>(T Value) {
            return new Optional<T>(Value);
        }

        public static explicit operator T?(Optional<T> Optional) {
            var ret = default(T);
            if (Optional.IsPresent) {
                ret = Optional.Value;
            }

            return ret;
        }

        public bool TryGetValue(out T? Value) {
            Value = this.Value;

            return IsPresent;
        }

    }

    public interface IOptional {
        bool IsPresent { get; }
        object? Value { get; }
    }


    public class OptionalJsonConverter : JsonConverterFactory {
        public override bool CanConvert(Type typeToConvert) {
            var ret = false;

            if(typeToConvert.IsGenericType && typeToConvert.GetGenericTypeDefinition() == typeof(Optional<>)) {
                ret = true;
            }

            return ret;
        }

        public override JsonConverter CreateConverter(Type typeToConvert, JsonSerializerOptions options) {

            var TypeOfT = typeToConvert.GetGenericArguments()[0];
            var ConverterType = typeof(OptionalJsonConverter<>).MakeGenericType(TypeOfT);

            var ret = Activator.CreateInstance(ConverterType) as JsonConverter
                ?? throw new NullReferenceException()
                ;

            return ret;
        }
    }

    public class OptionalJsonConverter<T> : JsonConverter<Optional<T>> {
        public override Optional<T> Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) {

            var RawValue = (T?) JsonSerializer.Deserialize(ref reader, typeof(T), options);

            var ret = new Optional<T>(RawValue);

            return ret;
        }

        public override void Write(Utf8JsonWriter writer, Optional<T> value, JsonSerializerOptions options) {

            if (value.IsPresent) {
                JsonSerializer.Serialize(writer, value.Value, options);
            }
        }
    }


}

@eiriktsarpalis
Copy link
Member

Hi @TonyValenti, I see you also opened #54269. Am I right to assume that the other issue supersedes this request and if so, should we close this one?

@TonyValenti
Copy link
Author

Is there another way to accomplish this that you can see? Any chance the JsonIgnore enhancement can be wrapped into .net 6x?

@eiriktsarpalis
Copy link
Member

Is there another way to accomplish this that you can see?

I haven't looked into the details but the JsonIgnore enhancement seems like a reasonable addition. If you agree we should probably just close this one.

Any chance the JsonIgnore enhancement can be wrapped into .net 6x?

It's unlikely at this point given the size of our backlog, sorry.

@eiriktsarpalis
Copy link
Member

It's unlikely that we would implement a construct as proposed in the original post, however offering a means to control ignored property behavior on the type/converter level is certainly something we would consider supporting going forward.

Closing this issue in favor of #54269 and related issue requests.

@ghost ghost locked as resolved and limited conversation to collaborators Aug 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json untriaged New issue has not been triaged by the area owner
Projects
None yet
Development

No branches or pull requests

5 participants