NewtonsoftJsonInputFormatter has incorrect behavior in case of user-added EventHandler

[gbtb]

Describe the bug

I want to add global ErrorHandler for Newtonsoft deserializer(as described at https://www.newtonsoft.com/json/help/html/SerializationErrorHandling.htm) . Purpose of my error handler is to ignore some exceptions, which are raised during deserialization of partially incorrect json. Simply put, I want to skip some wrong fields in json and leave their values in POCO uninitialized (default). This can be achieved with following code:

new JsonSerializerSettings
{
    Error = (sender, eventArgs) =>
    {
        if (eventArgs.ErrorContext.Error is JsonSerializationException ex &&
            ex.InnerException is InvalidCastException)
            eventArgs.ErrorContext.Handled = true;
}

This error handler works as intended in case of standalone usage with JsonConvert.DeserializeObject, but if I add the same handler to Asp.Net Core Startup at AddNewtonsoftJson, it doesn't work. This happens because ErrorHandler, which is attached by Asp.Net at NewtonsoftJsonInputFormatter does not respect ErrorContext.Handled. I think it is an error on Asp.Net part, and it's internal ErrorHandler should check this flag and exit from handler if it is already set to true.

Further technical details

• ASP.NET Core 5
• Include the output of dotnet --info:

dotnet --info Output

Пакет SDK для .NET (отражающий любой global.json):
 Version:   6.0.100-rc.1.21463.6
 Commit:    e627d556a1

Среда выполнения:
 OS Name:     Windows
 OS Version:  10.0.19043
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\6.0.100-rc.1.21463.6\

Host (useful for support):
  Version: 6.0.0-rc.1.21451.13
  Commit:  d7619cd4b1

.NET SDKs installed:
  2.1.525 [C:\Program Files\dotnet\sdk]
  3.1.402 [C:\Program Files\dotnet\sdk]
  5.0.100 [C:\Program Files\dotnet\sdk]
  5.0.101 [C:\Program Files\dotnet\sdk]
  5.0.400 [C:\Program Files\dotnet\sdk]
  5.0.401 [C:\Program Files\dotnet\sdk]
  6.0.100-rc.1.21463.6 [C:\Program Files\dotnet\sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.All 2.1.15 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.All 2.1.29 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.App 2.1.15 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 2.1.29 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.8 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.18 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 5.0.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 5.0.9 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 5.0.10 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 6.0.0-rc.1.21452.15 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 2.1.15 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.1.29 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.1 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.8 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.18 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.9 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.10 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.0-rc.1.21451.13 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.WindowsDesktop.App 3.1.8 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 3.1.18 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 5.0.0 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 5.0.9 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 5.0.10 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 6.0.0-rc.1.21451.3 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

[shirhatti]

Might be a bug:

aspnetcore/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs

Line 177 in d39e1ed

var jsonSerializer = CreateJsonSerializer(context);

[wserr]

I experience the same issue in AspNetCore.Mvc.NewtonsoftJson version 3.1.19. It seems to work correctly within Newtonsoft.Json version 13.0.1

[wserr]

I have created a design proposal to resolve this issue #37977

[wserr]

@davidfowl

Problem

See also issue #37323

We need to read the input model of a request, even if some properties of the model are not deserialized correctly. See this example. This works correctly using just Newtonsoft.Json version 13.0.1, but not with Microsoft.AspNetCore.Mvc.NewtonsoftJson, using the same assembly.

Cause

In the NewtonsoftJsonInputFormatter, a custom ErrorHandler is added to the JsonSerializer. This ErrorHandler sets a boolean, successful, to false. If this boolean is false, the model is not returned.

Proposal

To create a new property on the MvcNewtonsoftJsonOptions: IgnoreDeserializationErrors. If this setting is true, the input model will be returned, even when errors occur during deserializing. By default, the parameter should be false to avoid breaking changes.

namespace Microsoft.AspNetCore.Mvc
{
       public class MvcNewtonsoftJsonOptions
       {
+        public bool IgnoreDeserializationErrors { get; set; }
       }
}

Current situation:

NewtonsoftJsonInputFormatter.cs

if (!(exception is JsonException || exception is OverflowException || exception is FormatException))
{
    // At this point we've already recorded all exceptions as an entry in the ModelStateDictionary.
    // We only need to rethrow an exception if we believe it needs to be handled by something further up
    // the stack.
    // JsonException, OverflowException, and FormatException are assumed to be only encountered when
    // parsing the JSON and are consequently "safe" to be exposed as part of ModelState. Everything else
    // needs to be rethrown.
    var exceptionDispatchInfo = ExceptionDispatchInfo.Capture(exception);
    exceptionDispatchInfo.Throw();
}

New situation:

if (!(exception is JsonException || exception is OverflowException || exception is FormatException))
{
    // At this point we've already recorded all exceptions as an entry in the ModelStateDictionary.
    // We only need to rethrow an exception if we believe it needs to be handled by something further up
    // the stack.
    // JsonException, OverflowException, and FormatException are assumed to be only encountered when
    // parsing the JSON and are consequently "safe" to be exposed as part of ModelState. Everything else
    // needs to be rethrown.
    var exceptionDispatchInfo = ExceptionDispatchInfo.Capture(exception);
    exceptionDispatchInfo.Throw();
}
else if (_jsonOptions.IgnoreDeserializationErrors)
{
    // In case JsonException, Overflow Exception or FormatException occur,
    // still return the model if the IgnoreDeserializationErrors parameter is set to true
    return InputFormatterResult.Success(model);
}

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.