Better JSON deserialization errors. Implementation for #4607 #7015

SteveSandersonMS · 2017-11-01T17:49:07Z

No description provided.

rynowak · 2017-11-02T04:46:25Z

src/Microsoft.AspNetCore.Mvc.Core/MvcOptions.cs

@@ -170,11 +170,20 @@ public int MaxModelValidationErrors
        public bool AllowBindingUndefinedValueToEnumType { get; set; }

        /// <summary>
-        /// Gets or sets the option to determine if model binding should convert all exceptions(including ones not related to bad input)
+        /// Gets or sets the option to determine if model binding should convert all exceptions (including ones not related to bad input)


rynowak · 2017-11-02T04:47:41Z

src/Microsoft.AspNetCore.Mvc.Core/MvcOptions.cs

+        /// <see langword="true"/> by default, meaning that clients may receive details about
+        /// why the JSON they posted is considered invalid.
+        /// </summary>
+        public bool UseJsonDeserializationExceptionMessagesInModelState { get; set; } = true;


Would it be possible to state this as a negative? IE SuppressJsonDeserialization... and default to false?

The world is a simpler place when flags default to false.

rynowak · 2017-11-02T04:50:01Z

src/Microsoft.AspNetCore.Mvc.Core/SerializableError.cs

@@ -48,8 +49,20 @@ public SerializableError(ModelStateDictionary modelState)
                {
                    var errorMessages = errors.Select(error =>
                    {
-                        return string.IsNullOrEmpty(error.ErrorMessage) ?
-                            Resources.SerializableError_DefaultError : error.ErrorMessage;
+                        if (error.Exception is InputFormatterException)


What do you think about doing this here instead? https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelStateDictionary.cs#L236

We already have code here that 'classifies' exceptions.

I did originally start implementing it there, but then found a number of drawbacks:

Main issue is that it's a bigger breaking change. Previously, ModelState was being populated with an Exception instance, but this change would cause it to start getting populated with an ErrorMessage string instead. That's more likely to break people's applications than merely changing the text content that comes out of a SerializableError. And up until now, ModelError instances have only been populated with either an Exception or an ErrorMessage - not both.

As a smaller drawback, it doesn't fit neatly for ModelStateDictionary to make behavioural choices depending on MvcOptions, because it has no natural way to be populated with an MvcOptions at the time of creation, nor does it have access to an IServiceProvider.

Finally, it would necessitate moving InputFormatterException into Abstractions so it could be referenced by ModelStateDictionary. We could set up a type forwarder to minimise the breakingness but it's still more noise.

On the other hand, putting this into SerializableError is very safe and minimises API churn. From a layering perspective, it works well conceptually since SerializableError is the point where we decide how to represent a ModelError to the outside world.

However, I've just noticed that ValidationProblemDetails doesn't use SerializableError to interpret the ModelStateDictionary, but rather duplicates the concept of replacing blank messages with Resources.SerializableError_DefaultError. It would be preferable if ValidationProblemDetails got this behaviour from SerializableError instead of reimplementing it.

So, options:

(My preferred option) We continue to have this in SerializableError, but I'll also update ValidationProblemDetails to use that too.

We change to put the behaviour into ModelStateDictionary at the cost of a more impactful breaking change (plus I'm not sure how we'll get the options to it).

What do you think?

Main issue is that it's a bigger breaking change.

I think this is a breaking change that we want. The point is that this is an error message you should use - which is a distinction we already make inside MSD when it's appropriate.

As a smaller drawback, it doesn't fit neatly for ModelStateDictionary to make behavioural choices depending on MvcOptions

This doesn't seem relevant. Neither SerializableError nor MSD use options or need to. This is done by the formatter in your PR.

Finally, it would necessitate moving InputFormatterException into Abstractions so it could be referenced by ModelStateDictionary. We could set up a type forwarder to minimise the breakingness but it's still more noise.

This probably should be anyway. IFE is part of the documented contract of IInputFormatter - which is in abstractions.

I agree ValidationProblemDetails should be cleaned up.

OK, if this is a breaking change we want, that's totally fine with me. I've updated the implementation as discussed. The InputFormatterException is now recognised by ModelStateDictionary instead of SerializableError. As a further effect, this changes how XML formatter exceptions get reported (they are now phrased better in my opinion, but see the functional test diff for details).

Neither SerializableError nor MSD use options or need to. This is done by the formatter in your PR.

Yes, you're right - I was totally misremembering the implementation when I wrote the previous comment.

IFE is part of the documented contract of IInputFormatter - which is in abstractions.

OK, it's moved to abstrations and there's now a type forwarder in the old location.

I agree ValidationProblemDetails should be cleaned up.

I've added a test to make it explicit how it handles adding default error messages, but I haven't changed its implementation to wrap SerializableError, because on closer inspection, SerializableError doesn't make any promises that its "serializable" errors are strings - they could be anything that's serializable in some sense. So ValidationProblemDetails can't use that without making further assumptions that I don't want to add. I don't really think it matters a great deal, and in any case there's now a test to clarify what the net result should be.

So this should all be ready to go now I think.

rynowak · 2017-11-02T04:50:56Z

src/Microsoft.AspNetCore.Mvc.Formatters.Json/JsonInputFormatter.cs

+            ArrayPool<char> charPool,
+            ObjectPoolProvider objectPoolProvider,
+            bool suppressInputFormatterBuffering,
+            bool treatJsonDeserializationExceptionsAsSafe)


Aren't publicly constructable types fun 😆

rynowak

Overall the design seems on point here 👍 I still want to get an explicit buy-in on this from our overlord @blowdart who was not in the office today

rynowak

Cool beans 👍

dnfclas added the cla-already-signed label Nov 1, 2017

SteveSandersonMS requested a review from rynowak November 1, 2017 17:49

rynowak reviewed Nov 2, 2017

View reviewed changes

rynowak approved these changes Nov 6, 2017

View reviewed changes

Better JSON deserialization errors. Implements #4607, #4862

06153a9

SteveSandersonMS force-pushed the stevesa/json-errors branch from 37ee02c to 06153a9 Compare November 6, 2017 22:27

SteveSandersonMS merged commit d9825d1 into dev Nov 6, 2017

SteveSandersonMS deleted the stevesa/json-errors branch November 6, 2017 23:02

This was referenced Nov 6, 2017

JsonInputFormatter, ModelState and error response message. #4607

Closed

Report good error messages when JSON fails to deserialize #4862

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Better JSON deserialization errors. Implementation for #4607 #7015

Better JSON deserialization errors. Implementation for #4607 #7015

Uh oh!

SteveSandersonMS commented Nov 1, 2017

Uh oh!

rynowak Nov 2, 2017

Uh oh!

rynowak Nov 2, 2017

Uh oh!

SteveSandersonMS Nov 3, 2017

Uh oh!

rynowak Nov 2, 2017

Uh oh!

SteveSandersonMS Nov 3, 2017

Uh oh!

rynowak Nov 4, 2017

Uh oh!

SteveSandersonMS Nov 6, 2017

Uh oh!

rynowak Nov 2, 2017

Uh oh!

rynowak left a comment

Uh oh!

rynowak left a comment

Uh oh!

Uh oh!

Better JSON deserialization errors. Implementation for #4607 #7015

Better JSON deserialization errors. Implementation for #4607 #7015

Uh oh!

Conversation

SteveSandersonMS commented Nov 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rynowak left a comment

Choose a reason for hiding this comment

Uh oh!

rynowak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!