[Design] Add support for API error response type based on RFC 7807 #6695

pranavkm · 2017-08-22T16:56:02Z

No description provided.

pranavkm · 2017-08-22T17:00:18Z

src/Microsoft.AspNetCore.Mvc.Core/ApiExplorer/IErrorPolicy.cs

+
+namespace Microsoft.AspNetCore.Mvc.ApiExplorer
+{
+    public interface IErrorPolicy


Should this be an IApiDescriptorConvention instead? There's nothing restrictive about it being for errors only here.

Unsure currently. I would rather see the O/M for problem fleshed out first rather than focus on the deets of how the conventions for errors work.

pranavkm · 2017-08-22T17:00:44Z

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

+    /// <summary>
+    /// A machine-readable format for specifying errors in HTTP API responses based on https://tools.ietf.org/html/rfc7807.
+    /// </summary>
+    public class Problem : Dictionary<string, object>


Support for Extension Members

Ugh, is there another way here besides extending dictionary?

I'll play around with it. I just went with what we do in SerializableError because it looked easy

khellang · 2017-08-22T17:28:03Z

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

+        public string Type
+        {
+            get => GetValue<string>(nameof(Type));
+            set => Add(nameof(Type), value);


By using Add here, it'll throw if the Type property has already been set.

Good catch!

khellang · 2017-08-22T17:30:36Z

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

+                        StatusCode = statusCodeResult.StatusCode,
+                    };
+                }
+                else if (statusCode == StatusCodes.Status400BadRequest)


This is already checked above and will never be true.

Don't blame @pranavkm - blame my bad prototype code 😆

khellang · 2017-08-22T17:39:34Z

I ❤️ this! I noticed @rynowak's KodKod repo a few days ago and saw this coming. I use something like this (problem+json) in all my APIs.

I can't help thinking this would be even better as middleware, so you could add it to the start of the pipeline and make sure all non-200 responses would be returned in a proper representation.

It would also be nice to be able to map different exceptions to different problem types. Something like options.Handle<ValidationException>(ex => new ValidationProblem(ex)) where ValidationProblem is a derived type from Problem.

rynowak · 2017-08-22T17:41:07Z

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

@@ -50,6 +50,17 @@ public ActionResult(ActionResult result)
            return new ActionResult<TValue>(result);
        }

+        public static implicit operator ActionResult<TValue>(Problem problem)


Ooh, this is nice.

rynowak · 2017-08-22T17:42:43Z

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

+        /// Initializes a new instance of the <see cref="SerializableError"/> class.
+        /// </summary>
+        public Problem()
+            : base(StringComparer.OrdinalIgnoreCase)


pranavkm · 2017-08-22T17:42:52Z

@khellang there's a discussion about moving it further up the stack - aspnet/Diagnostics#346. Our plan was to get something going in Mvc and see if we can move the contract to diagnostics so it does the right thing for API responses.

rynowak · 2017-08-22T17:44:18Z

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

+            }
+
+            return default(TValue);
+        }


This is kinda a minefield. The 'dictionary' is mutable, so it's just as sensible to add the status code as a string, which would throw if you use the getter.

Should we just write a custom serializer for this?

rynowak · 2017-08-22T17:44:58Z

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

+namespace Microsoft.AspNetCore.Mvc
+{
+    [AttributeUsage(AttributeTargets.Method | AttributeTargets.Class, AllowMultiple = false, Inherited = true)]
+    public class ProblemErrorPolicyAttribute : Attribute, IErrorPolicy, IExceptionFilter, IResultFilter


I'm not paying too much attention to this type right now because it will be a bigger item to flesh this out. I want to focus on the O/M for problem

khellang · 2017-08-22T17:46:26Z

A problem with the dictionary approach and the way it's currently implemented (using nameof) is that you loose the ability to follow the user's preferred casing for JSON since it won't, by default, change casing for dictionary keys. This means that all JSON properties will always be PascalCased using this approach.

rynowak · 2017-08-22T17:47:42Z

I can't help thinking this would be even better as middleware, so you could add it to the start of the pipeline and make sure all non-200 responses would be returned in a proper representation.

Yep. We're starting here.

rynowak · 2017-08-22T17:48:20Z

@khellang - since you've been doing something similar do you have an O/M for problem that you like already? Wondering if we can borrow some inspiration...

khellang · 2017-08-22T18:09:03Z

Not really much for inspiration...

/// <summary>
/// Represents "Problem Details for HTTP APIs"
/// </summary>
/// <remarks>
/// See RFC 7807 - https://tools.ietf.org/html/rfc7807
/// </remarks>
public class ProblemDetails
{
    public static readonly Uri AboutBlank = new Uri("about:blank");

    /// <summary>
    /// A URI reference that identifies the problem type.
    /// </summary>
    /// <remarks>
    /// This specification encourages that, when dereferenced, it provide
    /// human-readable documentation for the problem type (e.g., using HTML).
    /// </remarks>
    public Uri Type { get; set; } = AboutBlank;

    /// <summary>
    /// A short, human-readable summary of the problem type.
    /// </summary>
    /// <remarks>
    /// It SHOULD NOT change from occurrence to occurrence of the problem, except
    /// for purposes of localization (e.g., using proactive content negotiation).
    /// </remarks>
    public string Title { get; set; }

    /// <summary>
    /// The HTTP status code generated by the origin server for this occurrence of the problem.
    /// </summary>
    public int Status { get; set; }

    /// <summary>
    /// A human-readable explanation specific to this occurrence of the problem.
    /// </summary>
    public string Detail { get; set; }

    /// <summary>
    /// A URI reference that identifies the specific occurrence of the problem.
    /// It may or may not yield further information if dereferenced.
    /// </summary>
    public Uri Instance { get; set; } = AboutBlank;
}

I basically inherit from that to add properties specific to the different problem types. Something like

public class ValidationProblemDetails : ProblemDetails
{
    public ValidationProblemDetails()
    {
        Errors = new Dictionary<string, string[]>();
    }

    public IDictionary<string, string[]> Errors { get; }
}

I then have an exception filter to map different exception type to different problem types, like I mentioned above. It's working pretty nicely for us 😄

khellang · 2017-08-22T18:19:50Z

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

+                var problem = new Problem
+                {
+                    Type = context.Exception.HelpLink,
+                    Title = context.Exception.Message,


I'm not sure this is best as a title. The RFC states

It SHOULD NOT change from occurrence to occurrence of the problem, except for purposes of localization (e.g., using proactive content negotiation).

I'm not sure an exception message would qualify 😉

I guess we could use a standard "An unknown error has occurred." That said, "SHOULD" is somewhat more lenient than "MUST", so maybe this is fine as it is.

Yeah, we're not going to do what this code does today. We don't return exception messages to users in general.

pranavkm · 2017-08-23T17:56:20Z

🆙 📅

Problem no longer derives from a Dictionary, so that's somewhat cleaner. I made it a property instead to stash arbitrary additional values. Derived types like the ones @khellang uses (sans the error mapping) is supported if you'd rather use strongly typed models. I tried playing around with dyanamic here but since all you do is assign stuff, the programming model doesn't seem nicer.
Wrote a custom Json formatter. Also works with Xml.
Need to figure out how to get it use the right Content-Type. Right now it says application/json. We want it to say application/problem+json.

rynowak · 2017-08-23T17:57:39Z

If you're creating the ObjectResult you can put the content type there.

dougbu

Wondering a bit about the future of SerializableError in this Problematic world…

dougbu · 2017-09-06T17:25:45Z

samples/MvcSandbox/Controllers/HomeController.cs

+        [HttpPost("/problem")]
+        public ActionResult<Person> Problem(int cooks)
+        {
+            if (cooks > 10)


Shouldn't this be 2? 😸

dougbu · 2017-09-06T17:29:33Z

src/Microsoft.AspNetCore.Mvc.ApiExplorer/ActionResultApiDescriptionProvider.cs

+{
+    public class ActionResultApiDescriptionProvider : IApiDescriptionProvider
+    {
+        public int Order => -1000 + 10;


Suggest a comment about the significance of this value.

dougbu · 2017-09-06T17:41:55Z

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

+            set => AdditionalProperties[key] = value;
+        }
+
+        public IDictionary<string, object> AdditionalProperties { get; } = new Dictionary<string, object>(StringComparer.Ordinal);


Should this be lazily-initialized with a property indicating it's been created? Not sure always creating the dictionary is much better than subclassing SerializableError or another dictionary type.

pranavkm · 2017-09-07T01:14:13Z

Closing this in favor of focused set of PRs starting from #6742.

pranavkm requested a review from rynowak August 22, 2017 16:56

dnfclas added the cla-already-signed label Aug 22, 2017

pranavkm force-pushed the prkrishn/problem branch from 11e4166 to 0630ae5 Compare August 22, 2017 16:58

pranavkm commented Aug 22, 2017

View reviewed changes

khellang suggested changes Aug 22, 2017

View reviewed changes

rynowak reviewed Aug 22, 2017

View reviewed changes

khellang reviewed Aug 22, 2017

View reviewed changes

pranavkm force-pushed the prkrishn/problem branch 2 times, most recently from a31063c to ce5f887 Compare August 23, 2017 17:50

pranavkm force-pushed the feature/kodkod branch from d6fd641 to d04c748 Compare August 24, 2017 23:25

pranavkm added 2 commits August 24, 2017 16:27

Add support for API error response type based on RFC 7807

9db1d28

WIP

ba812b5

pranavkm force-pushed the prkrishn/problem branch from ce5f887 to ba812b5 Compare August 24, 2017 23:30

pranavkm mentioned this pull request Aug 30, 2017

Suggestions on Web API action results metadata #6558

Closed

dougbu reviewed Sep 6, 2017

View reviewed changes

pranavkm closed this Sep 7, 2017

dougbu deleted the prkrishn/problem branch July 15, 2018 03:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Design] Add support for API error response type based on RFC 7807 #6695

[Design] Add support for API error response type based on RFC 7807 #6695

pranavkm commented Aug 22, 2017

pranavkm Aug 22, 2017

rynowak Aug 22, 2017

pranavkm Aug 22, 2017

rynowak Aug 22, 2017

pranavkm Aug 22, 2017

khellang Aug 22, 2017

pranavkm Aug 22, 2017

khellang Aug 22, 2017

rynowak Aug 22, 2017

khellang commented Aug 22, 2017 •

edited

Loading

rynowak Aug 22, 2017

rynowak Aug 22, 2017

pranavkm commented Aug 22, 2017

rynowak Aug 22, 2017

rynowak Aug 22, 2017

khellang commented Aug 22, 2017

rynowak commented Aug 22, 2017

rynowak commented Aug 22, 2017

khellang commented Aug 22, 2017 •

edited

Loading

khellang Aug 22, 2017 •

edited

Loading

pranavkm Aug 23, 2017

rynowak Aug 24, 2017

pranavkm commented Aug 23, 2017

rynowak commented Aug 23, 2017

dougbu left a comment

dougbu Sep 6, 2017

dougbu Sep 6, 2017

dougbu Sep 6, 2017

pranavkm commented Sep 7, 2017

[Design] Add support for API error response type based on RFC 7807 #6695

[Design] Add support for API error response type based on RFC 7807 #6695

Conversation

pranavkm commented Aug 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khellang commented Aug 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavkm commented Aug 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khellang commented Aug 22, 2017

rynowak commented Aug 22, 2017

rynowak commented Aug 22, 2017

khellang commented Aug 22, 2017 • edited Loading

khellang Aug 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavkm commented Aug 23, 2017

rynowak commented Aug 23, 2017

dougbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavkm commented Sep 7, 2017

khellang commented Aug 22, 2017 •

edited

Loading

khellang commented Aug 22, 2017 •

edited

Loading

khellang Aug 22, 2017 •

edited

Loading