Skip to content

Conversation

@brunolins16
Copy link
Member

NewtonsoftJsonInputFormatter checks for ErrorContext.Handled flag

Added check for eventArgs.ErrorContext.Handled flag before reporting error.

Description

Newtonsoft.Json supports error handling serialization/deserialization that lets you catch an error and choose whether to handle it and continue with serialization or let the error bubble up and be thrown in your application. ASP.NET Core allows you to do this configuration through the MvcNewtonsoftJsonOptions.JsonSerializerSettings.

services
    .AddControllers()
    .AddNewtonsoftJson(options =>
    {
        options.SerializerSettings.Error = (s, args) => { args.ErrorContext.Handled = true;  }
    });

However, the built-in input formatter is not checking if the ErrorContext is already Handled causing the exception to be reported to the ModelState. This fix is adding a check for eventArgs.ErrorContext.Handled flag before reporting error.

Also, probably users are not using since it is not working, however, the new behavior must be opted-in using the compat flag Microsoft.AspNetCore.Mvc.NewtonsoftJson.EnableSkipHandledError.

Fixes #37323

Customer Impact

Since ASP.NET (.NET Framework) does not report the error, this been reported as a block issue during the migration to ASP.NET Core (.NET 6) (Eg.: #41883)

Regression?

  • Yes
  • No

[If yes, specify the version the behavior has regressed from]

Risk

  • High
  • Medium
  • Low

We are introducing a compat flag Microsoft.AspNetCore.Mvc.NewtonsoftJson.EnableSkipHandledError that users need to opt-in.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

@brunolins16 brunolins16 requested a review from javiercn as a code owner June 1, 2022 22:19
@ghost ghost added the old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Jun 1, 2022
@ghost ghost added this to the 6.0.x milestone Jun 1, 2022
@ghost
Copy link

ghost commented Jun 1, 2022

Hi @brunolins16. If this is not a tell-mode PR, please make sure to follow the instructions laid out in the servicing process document.
Otherwise, please add tell-mode label.

@brunolins16 brunolins16 requested a review from a team June 2, 2022 17:00
@brunolins16 brunolins16 added the Servicing-consider Shiproom approval is required for the issue label Jun 3, 2022
@ghost
Copy link

ghost commented Jun 3, 2022

Hi @brunolins16. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

@rbhanda rbhanda added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Jun 7, 2022
@rbhanda rbhanda modified the milestones: 6.0.x, 6.0.7 Jun 7, 2022
@dougbu
Copy link
Contributor

dougbu commented Jun 8, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@vinod3091
Copy link

vinod3091 commented Jun 8, 2022 via email

@dougbu dougbu merged commit 5e08f80 into dotnet:release/6.0 Jun 8, 2022
@brunolins16 brunolins16 deleted the backport/37323-to-release/6.0 branch August 2, 2022 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels Servicing-approved Shiproom has approved the issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants