NewtonsoftJsonInputFormatter: Deserialize error handling

[wserr]

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 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);
}

[davidfowl]

I think it makes sense to merge these 2 issues.

[Deleted user]

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.

[wserr]

Is it possible to merge this into RC 3.1 as well? We are using this version.

[davidfowl]

Can you put the design proposal in the other issue

[wserr]

@davidfowl sure, I will do that. Is this going to be discussed on the next API review meeting? And when is that exactly? I haven't contributed yet and I'm interested in doing so.