Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom error handling in .NET6 #1019

Closed
1 task done
thoma01 opened this issue Aug 22, 2023 · 13 comments
Closed
1 task done

Custom error handling in .NET6 #1019

thoma01 opened this issue Aug 22, 2023 · 13 comments

Comments

@thoma01
Copy link

thoma01 commented Aug 22, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

In the releases of v6.x.x of this package, IErrorResponseProvider was removed in favor of ProblemDetails.

All the examples show how to .AddProblemDetails() but unfortunately, they all target .NET7.

How is this supposed to be handled in .NET6? I need to be able to support backwards compatibility for my error messages, but this change does not really explain how to achieve this?

Expected Behavior

No response

Steps To Reproduce

No response

Exceptions (if any)

No response

.NET Version

6

Anything else?

No response

@commonsensesoftware
Copy link
Collaborator

In .NET 6 and, hence, 6.x there was no common interface for Problem Details. Unfortunately, this meant I had to create my own, but it was quickly fixed and supplanted in .NET 7.

What you are looking for is the IProblemDetailsFactory service. The change and customization will depend on whether you are using Minimal APIs or MVC Core. The DefaultProblemDetailsFactory is used for Minimal APIs, whereas MvcProblemDetailsFactory provides a decorated adapter over ProblemDetailsFactory which was only available for MVC Core.

ProblemDetails exists across the board and should be the same for .NET 6+. The only difference is how you customize their generation. If you've already customized MVC Core, then it should just work; otherwise, you can choose whether to replace IProblemDetailsFactory, ProblemDetailsFactory, or potential both.

@commonsensesoftware
Copy link
Collaborator

This question has been asked before, but I guess it's not in the documentation. There have never been any specific examples for changing the behavior. I've updated the Error Responses topic in the wiki to highlight the differences.

@thoma01
Copy link
Author

thoma01 commented Aug 22, 2023

Actually the reason that I ask is, I am looking for a way to customize the error response format to continue following the Microsoft REST Guidelines error response format, as we have already released our v1 API using this format.

In earlier versions, the error responses bodies complied with the Microsoft REST Guidelines error response format, which is itself the error response format used by the OData protocol (see OData JSON Format §21.1). There wasn't a broad standard at that time, which made any common error response format sensible.

We are using OData so we are unable to use the official Microsoft versioning package. We tried to switch to this package, but we lost the ability to register our implementation of IErrorResponseProvider which we were using to format our errors before.

We were hoping to not have to use Problem Details as that would break our API compatibility for existing customers in our v1. Is it possible to intercept/override something to continue to format the errors to match the Microsoft REST Guidelines.

@commonsensesoftware
Copy link
Collaborator

I'm not sure I follow:

We are using OData so we are unable to use the official Microsoft versioning package

There is no other package or project for ASP.NET API Versioning. This is the one and only. You can thank Microsoft for the name change. ☹️What is the specific issue or gap with OData?

Now that I know what you are trying to achieve, let me see if I can't provide an example of how to achieve the results you're looking for. 😄

@commonsensesoftware
Copy link
Collaborator

It was surprisingly easier than I thought it would be. Here is a rough solution that would work:

using Asp.Versioning;
using Microsoft.AspNetCore.Mvc;
using System.Text.Json;
using System.Text.Json.Serialization;

public class ErrorObjectConverter : JsonConverter<ProblemDetails>
{
    public override void Write(
        Utf8JsonWriter writer,
        ProblemDetails value,
        JsonSerializerOptions options )
    {
        if ( IsSupported( value ) )
        {
            WriteErrorObject( writer, value, options );
        }
        else
        {
            options = new JsonSerializerOptions( options );
            options.Converters.Remove( this );
            JsonSerializer.Serialize( writer, value, options );
        }
    }

    protected virtual void WriteErrorObject(
        Utf8JsonWriter writer,
        ProblemDetails problemDetails,
        JsonSerializerOptions options )
    {
        writer.WriteStartObject();
        writer.WriteStartObject( "error" );

        if ( problemDetails.Extensions.TryGetValue( "code", out var value ) )
        {
            writer.WriteString( "code", value.ToString() );
        }

        writer.WriteString( "message", problemDetails.Title );

        if ( !string.IsNullOrEmpty( problemDetails.Instance ) )
        {
            writer.WriteString( "target", problemDetails.Instance );
        }

        if ( !string.IsNullOrEmpty( problemDetails.Detail ) )
        {
            writer.WriteStartArray( "details" );
            writer.WriteStringValue( problemDetails.Detail );
            writer.WriteEndArray();
        }

        writer.WriteEndObject();
        writer.WriteEndObject();
    }

    public override ProblemDetails Read(
        ref Utf8JsonReader reader,
        Type typeToConvert,
        JsonSerializerOptions options ) => throw new NotSupportedException();

    private static bool IsSupported( ProblemDetails problemDetails )
    {
        if ( problemDetails.Type == ProblemDetailsDefaults.Unsupported.Type )
        {
            return true;
        }

        if ( problemDetails.Type == ProblemDetailsDefaults.Unspecified.Type )
        {
            return true;
        }

        if ( problemDetails.Type == ProblemDetailsDefaults.Invalid.Type )
        {
            return true;
        }

        if ( problemDetails.Type == ProblemDetailsDefaults.Ambiguous.Type )
        {
            return true;
        }

        return false;
    }
}

There are a few ways you can register the converter, but the simplest method would look like:

// CAUTION: note the ambiguity between:
// - Microsoft.AspNetCore.Http.Json.JsonOptions
// - Microsoft.AspNetCore.Mvc.JsonOptions
//
// You need the former as that is purely routing without any part of MVC
builder.Services.Configure<Microsoft.AspNetCore.Http.Json.JsonOptions>(
    options => options.SerializerOptions.Converters.Insert( 0, new ErrorObjectConverter() ) );

// this should not be necessary, but note that you can cover MVC Core paths with the same converter
// builder.Services.Configure<Microsoft.AspNetCore.Mvc.JsonOptions>(
//    options => options.JsonSerializerOptions.Converters.Insert( 0, new ErrorObjectConverter() ) );

Let's say a client requests https://localhost:5001/api/values?api-version=42.0. This would normally produce the response:

{
    "type": "https://docs.api-versioning.org/problems#unsupported",
    "title": "Unsupported API version",
    "status": 400,
    "detail": "The HTTP resource that matches the request URI 'https://localhost:5001/api/values' does not support the API version '42.0'.",
    "code": "UnsupportedApiVersion",
    "traceId": "00-9956265f482c96f8478349bb1403427e-f7267eaaa2a7891d-00"
}

However, with the converter in place, it will now produce:

{
    "error":
    {
        "code": "UnsupportedApiVersion",
        "message": "Unsupported API version",
        "details":
        [
            "The HTTP resource that matches the request URI 'https://localhost:5001/api/values' does not support the API version '42.0'."
        ]
    }
}

The example provided will only convert ProblemDetails generated by API Versioning. ProblemDetails produced through other methods will be left unchanged.

I haven't received many requests for it, but this should be a generic solution that can provide a solid backward compatibility story, which - to be honest - is missing. I need a little bit more time to think about the edge cases, configuration, and testing, but this approach should unblock you in the meantime. Any other thoughts or feedback you have are welcome.

@thoma01
Copy link
Author

thoma01 commented Aug 23, 2023

I have tried enabling ProblemDetails in .NET6 by adding Hellang.Middleware.ProblemDetails nuget package. By doing this, when I specify an unsupported/invalid API version, I simply get a response:
{ "type": "https://httpstatuses.io/404", "title": "Not Found", "status": 404, "traceId": "00-5341acdcb4838e1a124c2700a1d20633-4056df7593bf3343-00" }

How are you enabling ProblemDetails for API versioning in .NET 6?

@commonsensesoftware
Copy link
Collaborator

Danger Will Robinson! <you are going down the proverbial rabbit hole 😉>

ProblemDetails existed in .NET 6. Despite the namespace, the class in located in the HTTP extensions, not MVC Core:

https://github.com/dotnet/aspnetcore/blob/42af1fe8be1869b034feeec5aaefc0efb561564f/src/Http/Http.Extensions/src/ProblemDetails.cs#L13

This means it was possible to generate ProblemDetails in .NET 6, ASP.NET Core 6, and API Versioning 6.x. Problem Details are generated for all API Versioning Endpoint types. You're getting into the bowels of routing, but it's located here:

[EdgeKey.Malformed] = new( capacity: 1 ) { new MalformedApiVersionEndpoint( logger ) },

Unfortunately, this doesn't cover every single edge case because ASP.NET Core itself doesn't provide Problem Details or hooks to provide one for some cases such as 415. As linked above, API Versioning introduced IProblemDetailsFactory so that it didn't have a hard dependency on MVC Core a la the ProblemDetailsFactory abstract class. I presumed the ASP.NET team would fix the issue at some point and they did in .NET 7 (sooner than I expected). The DefaultProblemDetailsFactory (also linked above) essentially does the same thing as the MVC Core implementation, but can be used for Minimal APIs and only requires the core routing system. The detail for each scenario can be found in:

If the routing system lands on any one of these endpoints, you'll get a ProblemDetails instance. The MVC extensions replace and decorate DefaultProblemDetailsFactory with MvcProblemDetailsFactory (also linked above). You can see that happen here:

services.TryReplace( typeof( DefaultProblemDetailsFactory ), Singleton<IProblemDetailsFactory, MvcProblemDetailsFactory>() );

The idea is that you may have already changed MVC Core's default implementation of ProblemDetailsFactory or you may just want to completely replace API Versioning's IProblemDetailsFactory. There shouldn't be a need to do both.

Using the JsonConverter<T> that I provided, you have a path to convert the ProblemDetails instances into the Error Object format (from OData) that is used in the Microsoft REST API Guidelines. You can go with that approach at any time. I should have a working out-of-the-box extension that can retain the old behavior that you are looking for in a few days.

@thoma01
Copy link
Author

thoma01 commented Aug 24, 2023

I believe to have found the confusion of why I thought ProblemDetails were not enabled.

In API Versioning 5.x, we were able to set ErrorResponses in ApiVersioningOptions. This allowed us to customize the response format as we saw fit.

In API Versioning 6.x, we no longer have that ability and you are proposing we implement a JsonConverter<T> to intercept these ProblemDetails and write them as we please.

However, when versioning by URL segment only, we get a 404 response, with no body (ie. no ProblemDetails). When we enable the other versioning reader methods (query string and/or http header), we do get ProblemDetails when providing the version by 1 of these methods.

We are only using URL segment versioning, and in API Versioning 5.x we were able to rewrite the errors using the ErrorResponses field from ApiVersioningOptions. Now in API Versioning 6.x, we can no longer do that, and if there are no ProblemDetails for URL segment only versioning responses, then we cannot maintain consistency in our errors. This is a breaking change.

Additionally, the HTTP Status Code has changed between 5.x and 6.x for URL segment only, from 400 to 404. This is also a breaking change.

@thoma01
Copy link
Author

thoma01 commented Aug 24, 2023

We would continue to use 5.x if we could, but we need the OData changes in 6.x.

This is coming to light since we are now moving onto version 2 of our API; this was not a problem when we only had the 1 version.

@commonsensesoftware
Copy link
Collaborator

Much of this was called out in discussion #808; specifically the breaking changes. You might have missed it or perhaps it was TL;DR, which I can understand.

The routing behavior is a bit tricky. Due some other bugs (specifically versioning by media type), I had to push things down further into the routing system. Aside from being not RESTful, versioning by URL segment has been the bane of the codebase. There are lot of strange and painful edge cases to solve that only apply to that approach. The issue here is that route constraints have not been yet evaluated. Processing it out of the route constraint is painful enough, but the real issue is at this stage in the routing system, there's no clear way to know if there might be a match versus not. This is why you now get 404. There really isn't a better answer. You could also think of it in another light. Consider the template:

api/v{ver:int}/test

If you request api/vABC/test, you'll get 404, even without API Versioning. A significant number of people wanted it that way. It terms of consistency with the expectations of matching route templates, it's honestly probably better aligned.

It's not lost on me that, however, creates a behavioral breaking change. Another feature that was lost was reporting API versions, when the request is unmatched. If you don't match an endpoint, how can you possibly report supported or deprecated API versions? The community gave me a challenge and I was eventually able to come up with a best effort report scheme, which to my knowledge always (or almost always) reports the expected API versions due to how the routing system groups endpoints.

In the case of ProblemDetails for an unmatched API version in a URL segment, I'm not 100% sure how you'd reliably detect that versus the resource requested really does not exist. If there were a way to do that, I would certainly entertain the idea and provide a way for you to control whether you want 400 or 404 behavior. It might seem conceptually simple, but the implementation most certainly is not. One solution you could apply is middleware that looks for 404 without a body and adds one if the requested URL could match a known route. It's less than ideal, but it's workable.

Another approach would be to keep your V1 on the 5.x codebase and the V2+ on the 6.x+ codebase. It's understandably painful for you during that transition period, but it gives you clear separation and allows for breaking changes in a safe way. If V1 stays locked and you have a versioning policy (say N-2), then you should be able to just shutdown the V1 codebase at version V4 or whatever your policy is. In terms of making sure things do not break in unexpected ways across implementations, this probably the safest approach. Investigating YARP, or any other reverse proxy, is also on my radar to support this type of situation.

You may not have to implement a JsonConverter<T> after all. ProblemDetails supports extensions so I think there is a way you (or I) can make it look like an Error Object without any special conversion. The one issue I found is that simply shaping the output is not enough. Problem Details has it's own media type (e.g. application/problem+json) and might not be handled or understood by clients. I need to figure out to support providing an alternate media type and there should a generic solution.

5.x does not support .NET 6. It specifically targets .NET 5.0. You should be very careful about using versions that do not align to the .NET or ASP.NET Core runtime. While it might work, know that it may not. I've seen new features cause runtime failures. The 5.x and Microsoft.* packages are essentially EOL and will not have any more updates save for critical servicing updates. The powers that be decided to not retain co-ownership so it's impossible to for me to continue evolving the project under the Microsoft branding. It was quite painful just getting this far. I considered forking the repo, but I think that would have just been even more confusing to the community. I will likely upload a patch package that indicates it is deprecated, which I think I can do - now. Hopefully, that will bring more additional the banner and other announcements that have been made over the last 18+ months. There are a lot of people out there that thought or still think this project is owned and operated by the ASP.NET team. It isn't. It never was. I've run the project virtually solo the whole time. When I left Microsoft, it caused a lot of churn for the community unfortunately. I fought for nearly a year to do the right thing, but alas my hands are tied.

I continue to be committed to helping the community navigate these changes. I'm not sure that I can fix the 404 issue, but I'm open to ideas. I can certainly come up with a way to address the Error Object result issue.

@commonsensesoftware
Copy link
Collaborator

It's unfortunate that there is a breaking change in design between 6.x and 7.0+, but I have something in the works for you. PR #1021 will highlight how to support Error Objects in .NET 6.0. You'll just need to registry a new factory. This should occur before all other registrations so that the same setup will cover Minimal APIs and MVC Core (with controllers). For example:

ASP.NET Core 6.0 Setup

builder.Services.AddSingleton<IProblemDetailsFactory, ErrorObjectFactory>();
builder.Services.AddApiVersioning().AddMvc();

Starting in .NET 7.0 and beyond, it will be an ever-so-slightly different setup. Unfortunately, registration order still matters, but this is more about how the IProblemDetailsService works. This can seen in PR #1022. For example:

ASP.NET Core 7.0+ Setup

builder.Services.AddControllers();
builder.Services.TryAddEnumerable( ServiceDescriptor.Singleton<IProblemDetailsWriter, ErrorObjectWriter>() );
builder.Services.AddProblemDetails();
builder.Services.AddApiVersioning().AddMvc();

I'll let this settle for a few days so that you have a chance to look things over and provide feedback - if you want to. I don't have a good out-of-the-box solution for the 400 vs 404 behavioral issue. I have some constraints on how the routing system itself works. This might be something you can live with as the error is unrecoverable and typically means the client doesn't know how to call you or you might be able to use middleware to provide a 400 when appropriate. That's just not something I could possibly do in a generic way, but you would be capable of doing by knowing the internal mechanics of your application.

@commonsensesoftware
Copy link
Collaborator

6.5.0 and 7.1.0 of Asp.Versioning.Http have been published with backward compatibility support for Error Objects as they were in previous versions. I've updated the wiki with additional details as well. Hopefully this helps advance your migration journey.

@commonsensesoftware
Copy link
Collaborator

I believe this issue has reached its logical conclusion. The older error responses are now supported with a path to customize it further. This is a great addition to helping people migrate and will be added to the migration guide. Thanks for pushing it. If I've missed something or additional assistance is required, don't hesitate to reply or reopen the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants