dotnet/aspnetcore

NewtonsoftJsonInputFormatter has incorrect behavior in case of user-added EventHandler

gbtb opened this issue · 11 comments

gbtb commented

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 commented

Might be a bug:

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

Line 177 in d39e1ed

var jsonSerializer = CreateJsonSerializer(context);

wserr commented

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 commented

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

wserr commented

@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);
}
msftbot commented

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.
msftbot commented

Thanks for contacting us.
We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. Because it's not immediately obvious that this is a bug in our framework, we would like to keep this around to collect more feedback, which can later help us determine the impact of it. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

pranavkm commented

This happens because ErrorHandler, which is attached by Asp.Net at NewtonsoftJsonInputFormatter does not respect ErrorContext.Handled

#39132 is a similar issue and the suggestion there was to skip over errors that are marked as handled. Does that address your concerns?

pranavkm commented

Comment by @DaveLaa

Advanced Newtonsoft parsing error handling

I have a suggestion to the Microsoft.AspNetCore.Mvc.NewtonsoftJson project to improve the possibility of parsing error handling.

In my services project I have the need to be able to decide which parsing error of json objects sent to my service are tollerable and can be safely ignored. I achieve this goal by adding an error handler in the SerializerSettings of NewtonsoftJson and setting the Handled property to true.
This was no problem as long as my service project was compiled against .Net Framework. But now we refactored the services to compile against .Net Core 5 and here I found out that there is no way to have this decision.

In my new Startup I added the NewtonsoftJson support and also my error handler like this:

public class Startup {
	
	...

	public void ConfigureServices( IServiceCollection services ) {
	
		...

		services.AddControllers().AddNewtonsoftJson( opts => {
				...
				opts.SerializerSettings.Error = ( sender, args ) => {
					if(...){
						...
						args.ErrorContext.Handled = true;
					}
				};
		} );
		
		...
	}
}

But whatever i declare handeld, the error was allways thrown anyways by the AspNet framework. I debugged the NewtonsoftJsonInputFormatter and found out that here all errors are passed to the ModelStateDictionary regardless of being handled before or not. More important is that the parsed model ist not returned (always return InputFormatterResult.Failure();) and my service entdpoint gets a null object instead. There is also no way to configure a different behaviour.

My suggestion ist now to look into the Handled property and ignore errors that are already handled by user in external code:

public class NewtonsoftJsonInputFormatter : TextInputFormatter, IInputFormatterExceptionPolicy
{
	...

	public override async Task<InputFormatterResult> ReadRequestBodyAsync(
		
		...
		
		void ErrorHandler(object sender, Newtonsoft.Json.Serialization.ErrorEventArgs eventArgs)
		{
>>			if( eventArgs.ErrorContext.Handled ){
>>				// Error was already handeld by User in external code
>>				return;
>>			}
		
			successful = false;
			....
		}
	}

	...
}

Is it possible to implement this feature in future versions of .Net Core 5?

  • 👍1
gbtb commented

This happens because ErrorHandler, which is attached by Asp.Net at NewtonsoftJsonInputFormatter does not respect ErrorContext.Handled

#39132 is a similar issue and the suggestion there was to skip over errors that are marked as handled. Does that address your concerns?

Yes, it does. @DaveLaa case is identical to mine, and I agree with proposed solution. It would work for me, if Asp.Net Core checked eventArgs.ErrorContext.Handled flag before throwing Exception.

pranavkm commented

@gbtb we'd be happy to accept a PR for it if you'd like to send one.

msftbot commented

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.