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

Add support for MissingMemberHandling to System.Text.Json #37483

Closed
Tracked by #71967 ...
SigmundVik opened this issue Jun 5, 2020 · 29 comments · Fixed by #79945
Closed
Tracked by #71967 ...

Add support for MissingMemberHandling to System.Text.Json #37483

SigmundVik opened this issue Jun 5, 2020 · 29 comments · Fixed by #79945
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json
Milestone

Comments

@SigmundVik
Copy link

SigmundVik commented Jun 5, 2020

We would love to migrate our ASP.NET Core 3 application from using Newtonsoft.Json to System.Text.Json. However, the lack of support/workaround for MissingMemberHandling is a showstopper, because we need a robust way to detect and report deficient [FromBody] model binding. Without this feature, mistakes in the input JSON document can go undetected, and that is not acceptable.

Is adding MissingMemberHandling something that is planned for System.Text.Json?

EDIT @eiriktsarpalis: API proposal & usage examples added

API Proposal

namespace System.Text.Json;

public enum JsonMissingMemberHandling { Ignore = 0, Error = 1 }

public partial class JsonSerializerOptions
{
     // Global configuration
     public JsonMissingMemberHandling MissingMemberHandling { get; set; } = JsonMissingMemberHandling.Ignore;
}
namespace System.Text.Json.Serialization;

// Per-type attribute-level configuration
[AttributeUsageAttribute(AttributeTargets.Class | AttributeTargets.Interface | AttributeTargets.Struct, AllowMultiple=false, Inherited=false)]
public class JsonMissingMemberHandlingAttribute : JsonAttribute
{
       public JsonMissingMemberHandlingAttribute(JsonMissingMemberHandling missingMemberHandling);
       public JsonMissingMemberHandlingAttribute MissingMemberHandling { get; }
}

namespace System.Text.Json.Serialization.Metadata;

public partial class JsonTypeInfo
{
      // Per-type configuration via contract customization.
      public JsonMissingMemberHandling MissingMemberHandling { get; set; } = JsonMissingMemberHandling.Ignore;
}

API Usage

JsonSerializer.Deserialize<MyPoco>("""{ "jsonPropertyNotBindingToPocoProperty" : null}"""); // JsonException: member not found

[JsonMissingMemberHandling(JsonMissingMember.Error)]
public class MyPoco { }
@SigmundVik SigmundVik added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jun 5, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Text.Json untriaged New issue has not been triaged by the area owner labels Jun 5, 2020
@layomia layomia removed the untriaged New issue has not been triaged by the area owner label Jun 9, 2020
@layomia layomia modified the milestones: 5.0, Future Jun 9, 2020
@layomia
Copy link
Contributor

layomia commented Jun 9, 2020

Triage: this can be considered.

Removing api-suggestion as there's no API proposal here.

@layomia layomia removed the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jun 9, 2020
vslee added a commit to vslee/IEXSharp that referenced this issue Jun 20, 2020
- removed use of Newtonsoft.Json
- added some JSONConverters
	- Int32Converter and Int64Converter to allow string quoted numbers
	- DictionaryDatetimeTValueConverter to allow dictionaries with type of DateTime
	- both of the above are used with Account.UsageAsync()
- moved ExecutorRESTTest.cs to Helper folder
- System.Text.JSON doesn't yet support MissingMemberHandling, so commented that out for now until dotnet/runtime#37483 is implemented
@eiriktsarpalis eiriktsarpalis added api-suggestion Early API idea and discussion, it is NOT ready for implementation api-needs-work API needs work before it is approved, it is NOT ready for implementation wishlist Issue we would like to prioritize, but we can't commit we will get to it yet and removed api-needs-work API needs work before it is approved, it is NOT ready for implementation labels Oct 19, 2021
@eiriktsarpalis
Copy link
Member

FWIW the documentation does propose using JsonExtensionDataAttribute as a workaround to achieve this. Granted this would require running validation post-deserialization.

@eiriktsarpalis eiriktsarpalis added the api-needs-work API needs work before it is approved, it is NOT ready for implementation label Oct 19, 2021
@ghost
Copy link

ghost commented Oct 19, 2021

This issue has been marked with the api-needs-work label. This may suggest that the proposal requires further refinement before it can be considered for API review. Please refer to our API review guidelines for a detailed description of the process.

When ready to submit an amended proposal, please ensure that the original post in this issue has been updated, following the API proposal template and examples as provided in the guidelines.

@LetMeSleepAlready
Copy link

LetMeSleepAlready commented Oct 24, 2021

I just checked the of the json serializer. Maybe it could be as simple as the following (but then again, maybe not)

System.Text.Json.JsonSerializer.LookupProperty

if (jsonPropertyInfo == JsonPropertyInfo.s_missingProperty)
            {
                JsonPropertyInfo? dataExtProperty = state.Current.JsonTypeInfo.DataExtensionProperty;
                if (dataExtProperty != null && dataExtProperty.HasGetter && dataExtProperty.HasSetter)
                {
                    state.Current.JsonPropertyNameAsString = JsonHelpers.Utf8GetString(unescapedPropertyName);
                    if (createExtensionProperty)
                    {
                        CreateDataExtensionProperty(obj, dataExtProperty, options);
                    }
                    jsonPropertyInfo = dataExtProperty;
                    useExtensionProperty = true;
                }
// snippet goes here
            }

add the following snippet:

 else if (options.ThrowOnMissingMember) {
 ThrowHelper.ThrowJsonException_DeserializeUnableToBindMember();
}

Of course this would require an new ThrowHelper, and a new property in JsonSerializerOptions (ThrowOnMissingMember, default false = current way of working), both which are minor additions.

I figured the above might at least get this ticket going in some direction.

@eiriktsarpalis
Copy link
Member

Here's a potential workaround using .NET 6 features:

using System.Collections.Generic;
using System.Text.Json;
using System.Text.Json.Serialization;

JsonSerializer.Deserialize<MyPoco>(@"{ ""X"" : 42, ""Y"" : -1 }");
// Unhandled exception. System.Text.Json.JsonException: Contains extra properties: Y.

public class MyPoco : IJsonOnDeserialized
{
    public int X { get; set; }

    [JsonExtensionData]
    public IDictionary<string, object>? ExtraProperties { get; set; }

    void IJsonOnDeserialized.OnDeserialized()
    {
        if (ExtraProperties?.Count > 0)
        {
            throw new JsonException($"Contains extra properties: {string.Join(", ", ExtraProperties.Keys)}.");
        }
    }
}

@ShadyNagy
Copy link

All you need to do SetMissingMemberHandling and it will handle every thing for you but you need to install DevBetter.JsonExtensions

var deserializeOptions = new JsonSerializerOptions()
    .SetMissingMemberHandling(MissingMemberHandling.Ignore);

var weatherForecast = JsonSerializer.Deserialize<WeatherForecast>(jsonString, deserializeOptions);

@LetMeSleepAlready
Copy link

@eiriktsarpalis that will require us to alter each poco class, which while it would work is the opposite of what we are trying to achieve.. type-agnostic catching of extra members during deserialization.

@ShadyNagy that is exactly the opposite of this ticket.

@ShadyNagy
Copy link

@LetMeSleepAlready
Some bad APIs respond with something like this {} on a string or array and the Text.Json throw exceptions and there is not an option to control that turn on and turn off so we made the same as Newtonsoft.Json did
SetMissingMemberHandling(MissingMemberHandling.Ignore)
SetMissingMemberHandling(MissingMemberHandling.Error)

@LetMeSleepAlready
Copy link

@ShadyNagy Interesting extension, but I don't see any code that handles the 'MissingMemberHandling.Error' case. So that means it is the default functionality, which is not the intent of this raised ticket.

case:


POCO: class { public string member {get;set;} }
JSON: {"membah","some value"}

The above will not throw an exception, and there is no way to somehow trap this and act on it.

@ShadyNagy
Copy link

ShadyNagy commented Nov 2, 2021

@LetMeSleepAlready and @SiggyBar
The Devbetter Team Package updated so you can find cover for your problem here
https://github.com/DevBetterCom/DevBetter.JsonExtensions/blob/main/tests/DevBetter.JsonExtensions.Tests/SetMissingMemberErrorHandlingTests.cs
Nuget: DevBetter.JsonExtensions
Source Code: DevBetter.JsonExtensions

@ShadyNagy Interesting extension, but I don't see any code that handles the 'MissingMemberHandling.Error' case. So that means it is the default functionality, which is not the intent of this raised ticket.

case:


POCO: class { public string member {get;set;} }
JSON: {"membah","some value"}

The above will not throw an exception, and there is no way to somehow trap this and act on it.

@SigmundVik
Copy link
Author

@ShadyNagy, have you looked at Dahomey.Json? Maybe these efforts could be joined.

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

I recently stumbled upon this problem and I come to the same conclusion as @LetMeSleepAlready. @eiriktsarpalis could you provide some more insight, whether this feature has any chance of landing and if required is there something the community can do (so maybe pinpoint more areas that might need changing), this would be very helpful to start a PR, thanks 😉

@eiriktsarpalis
Copy link
Member

I like the idea of using a per-type handler, but how would the JSON of the missing property be deserialized into an object if we're missing metadata on how to do it?

@layomia
Copy link
Contributor

layomia commented Sep 7, 2022

Action<object, object, string>? SetMissingProperty { get; set; }, // declaring object, value, property name

Is this supposed to be implemented and set by users only (and not the serializer)?

// different name ideas: SetUnhandledProperty, MissingPropertyHandler

HandleMissingProperty seems nice.

@krwq
Copy link
Member

krwq commented Sep 8, 2022

Is this supposed to be implemented and set by users only (and not the serializer)?

yes, by default we'd do nothing or use extension data property

@eiriktsarpalis good point, I guess it would need to be Utf8JsonReader (which would require delegate since it would need to be ref) or JsonElement/JsonNode instead (perhaps latter makes more sense from usability point of view but since we're perf oriented we should consider first option as well).

@eiriktsarpalis
Copy link
Member

which would require delegate

Given the added complexity, we might want to hold off on adding this unless there is a proven use case. Perhaps focus on exposing enum support for now.

@eiriktsarpalis
Copy link
Member

Starting with .NET 7, it is possible enable missing member handling for every type without the added boilerplate using a custom resolver:

var options = new JsonSerializerOptions
{
    TypeInfoResolver = new DefaultJsonTypeInfoResolver
    {
        Modifiers = { AddMissingMemberHandling }
    }
};

string json = """{"Name" : "John", "Surname" : "Doe", "Age" : 99 }""";
// Fails with Unhandled exception. System.Text.Json.JsonException: JSON properties Surname, Age could not bind to any members of type MyPoco
JsonSerializer.Deserialize<MyPoco>(json, options);

static void AddMissingMemberHandling(JsonTypeInfo typeInfo)
{
    if (typeInfo.Kind == JsonTypeInfoKind.Object && 
        typeInfo.Properties.All(prop => !prop.IsExtensionData) &&
        typeInfo.OnDeserialized is null)
    {
        var cwt = new ConditionalWeakTable<object, Dictionary<string, object>>(); // Dynamically attach dictionaries to deserialized objects

        JsonPropertyInfo propertyInfo = typeInfo.CreateJsonPropertyInfo(typeof(Dictionary<string, object>), "__extensionDataAttribute");
        propertyInfo.Get = obj => cwt.TryGetValue(obj, out Dictionary<string, object>? value) ? value : null;
        propertyInfo.Set = (obj, value) => cwt.Add(obj, (Dictionary<string, object>)value!);
        propertyInfo.IsExtensionData = true;
        typeInfo.Properties.Add(propertyInfo);
        typeInfo.OnDeserialized = obj =>
        {
            if (cwt.TryGetValue(obj, out Dictionary<string, object>? dict))
            {
                cwt.Remove(obj);
                throw new JsonException($"JSON properties {String.Join(", ", dict.Keys)} could not bind to any members of type {typeInfo.Type}");
            }
        };
    }
}

public class MyPoco
{
    public string Name { get; set; }
}

@krwq krwq removed their assignment Oct 3, 2022
@eiriktsarpalis eiriktsarpalis self-assigned this Nov 8, 2022
@eiriktsarpalis eiriktsarpalis added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-needs-work API needs work before it is approved, it is NOT ready for implementation wishlist Issue we would like to prioritize, but we can't commit we will get to it yet labels Nov 8, 2022
@bartonjs
Copy link
Member

bartonjs commented Nov 15, 2022

Video

  • Since the proposal involved a two-enum we asked if a boolean would be better, and the conclusion was yes. And then it was no.
  • While Newtonsoft.Json calls it MissingMemberHandling, that's actually a debatable/confusing name. The property was specified, but it didn't map to a property/field on the target type (so reflection's MissingMember).
    • So we renamed "MissingMember" to "UnmappedMember". Sorry, future us.
namespace System.Text.Json
{
    public enum JsonUnmappedMemberHandling { Skip, Disallow }

    public partial class JsonSerializerOptions
    {
        // Global configuration
        public JsonUnmappedMemberHandling UnmappedMemberHandling { get; set; } = JsonUnmappedMemberHandling.Skip;
    }
}

namespace System.Text.Json.Serialization
{
    // Per-type attribute-level configuration
    [AttributeUsageAttribute(AttributeTargets.Class | AttributeTargets.Interface | AttributeTargets.Struct, AllowMultiple=false, Inherited=false)]
    public class JsonUnmappedMemberHandlingAttribute : JsonAttribute
    {
        public JsonUnmappedMemberHandlingAttribute(JsonUnmappedMemberHandling unmappedMemberHandling);
        public JsonUnmappedMemberHandling UnmappedMemberHandling { get; }
    }
}

namespace System.Text.Json.Serialization.Metadata
{
    public partial class JsonTypeInfo
    {
        // Per-type configuration via contract customization.
        public JsonUnmappedMemberHandling UnmappedMemberHandling { get; set; }; // defaults to the JsonSerializerOptions value
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Nov 15, 2022
@eiriktsarpalis eiriktsarpalis removed their assignment Nov 15, 2022
@layomia layomia self-assigned this Nov 17, 2022
@jeancallisti
Copy link

jeancallisti commented Nov 17, 2022

I don't understand. It's 2022. And we're still devising custom solutions in lengthy conversations, in order to be able to deserialize with the absolutely basic option "IF JSON NO MATCH CLASS, DESERIALIZE NO HAPPY" ?

For the last three years I read that Newtonsoft's Json is dying. and I keep trying to adopt System.Text.Json. And yet I'll yet again go back to Newtonsoft because I'm baffled by the absolutely wacky solutions proposed to address basic scenarios that were literally one-liners with Newtonsoft.

@eiriktsarpalis eiriktsarpalis self-assigned this Dec 20, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Dec 24, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 12, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json
Projects
None yet
Development

Successfully merging a pull request may close this issue.