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

Support custom error messages when validation fails on minimal APIs #35501

Open
captainsafia opened this issue Aug 19, 2021 · 17 comments
Open

Support custom error messages when validation fails on minimal APIs #35501

captainsafia opened this issue Aug 19, 2021 · 17 comments
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-minimal-actions Controller-like actions for endpoint routing Priority:3 Work that is nice to have triage-focus Add this label to flag the issue for focus at triage
Milestone

Comments

@captainsafia
Copy link
Member

This issue captures user feedback from https://twitter.com/davidfowl/status/1428228518953984005?s=20.

Currently, the feature to support treating parameters that are nullable or have default values as optional will return a status code and no error message as part of the response. This is largely because (1) it builds on top of the existing try-parse parsing logic which does nothing and (2) when we were working through the feature we had punted on the problem of what message to send back.

Users want to be be able to customize the message to:

  • Provide clearer info to the client
  • Conform to a specific API implementation
@captainsafia captainsafia added area-web-frameworks feature-minimal-actions Controller-like actions for endpoint routing labels Aug 19, 2021
@davidfowl
Copy link
Member

I think this goes beyond the optionality feature. It's anytime the framework automagically returns a response (e.g. if TryParse fails).

@captainsafia
Copy link
Member Author

I think this goes beyond the optionality feature. It's anytime the framework automagically returns a response (e.g. if TryParse fails).

True, true. The fact that both the TryParse and optionality checks both build on top of the CheckParams logic means that they both provide status codes with no messages to the user.

@DamianEdwards
Copy link
Member

Folks can customize the message via a middleware, using a marker object in metadata or items to enable/disable per endpoint (inspired by this reply tweet)

image

@davidfowl
Copy link
Member

What's nice about this is that we don't need to add anymore configuration or concepts. It's just middleware.

@poke
Copy link
Contributor

poke commented Aug 20, 2021

It would still be good if there was some global way to affect this. If the validating behavior is there by default with non-nullable parameters, then there should also be some way to control this default behavior.

I could imagine some callback that can be registered via options somewhere which default value is just the current 400 response behavior?

@DamianEdwards
Copy link
Member

We literally have no extensibility points like that today in Minimal APIs and have been very hesitant to do so for .NET 6. We want to let people use and see what the wider feedback is before we start committing to API and more layers of extensibility (complexity). One of the reasons MVC is so complex today is because so much is customizable. While powerful it makes it difficult to learn and limits what we can do regarding performance improvements.

If it can be done with middleware and other existing concepts that don't require new API in a reasonable way, we intend to leave it that way, for now.

@poke
Copy link
Contributor

poke commented Aug 20, 2021

While powerful it makes it difficult to learn

If “learning” or discoverability is a goal of the new minimal API, then I don’t think it should automatically respond with a 400 if it wasn’t actually produced in user code. Because that really is undiscoverable magic from the framework.

Especially if it returns just a blank 400 without any details, then that really makes it very difficult to diagnose and learn about this too. I deal often enough with users that aren’t even aware of how to access their server logs, who mostly rely on the content of the HTTP responses for figuring out what’s wrong. If there’s now a blank response, then imo this will do more harm than good.

And if you decide to produce some content, e.g. using ProblemDetails, then there needs to be a way to reconfigure this since ProblemDetails, being a relatively new spec, is only one of many ways to encode errors. And you wouldn’t want to have the framework be limiting here, especially since the minimal API is mostly a “do whatever you want, manually” way.

It would probably better to just throw an exception then which people could catch in a custom middleware to do custom handling, or which would bubble up until some default thing handles it.

@davidfowl
Copy link
Member

Maybe a decent tradeoff could be using a response header. That would:

  • Put something in the response by default
  • Allow users to customize the response with above approach

We'd also continue logging what happened.

@DamianEdwards
Copy link
Member

@davidfowl by that you mean the framework would set a header by default? What would it be (name/value)?

@davidfowl
Copy link
Member

@davidfowl by that you mean the framework would set a header by default? What would it be (name/value)?

Yea, maybe just the exception text.

@DamianEdwards
Copy link
Member

So a custom header with the exception message? Which exceptions are we talking about as I assume it would be constrained to some specific scenarios?

@davidfowl
Copy link
Member

davidfowl commented Aug 24, 2021

Or the problem details message? It's basically a way to get the data into the response without hampering the ability to change the response. When we do the problem details middleware it'll take care of these transformations I hope

@BrennanConroy BrennanConroy added this to the Backlog milestone Aug 27, 2021
@ghost
Copy link

ghost commented Aug 27, 2021

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.

@DamianEdwards
Copy link
Member

OK so this is pretty gnarly I admit but I added some examples to my playground repo (rc.2 branch) that use a few tricks to enable suppressing the default behavior (binding and/or response defaults).

The magic is basically a helper type DefaultBinder<TValue> that allows you to invoke the framework's default logic explicitly, returning you the TValue value and the status code the framework would have replied with. It's not completely fool-proof but goes a pretty long way. It has to use ref emit right now to generate the dummy route handler delegates to invoke the binding but I imagine in a future version of the framework we could actually include a type like this.

It means you can write route handlers like this where the logic to handle any issues is in the handler itself:

app.MapPost("/suppress-defaults", (SuppressDefaultResponse<Todo?> todo, HttpContext httpContext) =>
    {
        if (todo.Exception != null)
        {
            // There was an exception during binding, handle it however you like
            throw todo.Exception;
        }

        if (todo.StatusCode != 200)
        {
            // The default logic would have auto-responded, do what you like instead
            throw new BadHttpRequestException("Your request was bad and you should feel bad", todo.StatusCode);
        }

        return Results.Ok(todo.Value);
    })
    .WithTags("Examples")
    .Accepts<Todo>("application/json");

@DamianEdwards
Copy link
Member

For .NET 6 there won't be any first-class feature/option for changing the behavior including status codes and messages. However as noted above one can use middleware or a custom implementation of BindAsync to affect this behavior and ultimately control the response. We'll look at potential framework changes in .NET 7 based on how we see this being used.

@DamianEdwards DamianEdwards modified the milestones: 6.0.0, .NET 7 Planning Oct 4, 2021
@DamianEdwards DamianEdwards removed their assignment Oct 4, 2021
@rafikiassumani-msft rafikiassumani-msft added triage-focus Add this label to flag the issue for focus at triage Priority:3 Work that is nice to have labels Jan 11, 2022
@DamianEdwards
Copy link
Member

This is something we should revisit as part of the endpoint filters feature, i.e. it should be possible to author a filter that runs before the default logic and return a custom error message.

@captainsafia captainsafia changed the title Support custom error messages for optionality via nullability feature Support custom error messages when validation fails on minimal APIs May 3, 2022
@captainsafia
Copy link
Member Author

We have this set to a priority 3. We should re-evaluate since this is coming in a few discussions.

@amcasey amcasey added the area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc label Jun 2, 2023
@captainsafia captainsafia modified the milestones: .NET 8 Planning, Backlog Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-minimal-actions Controller-like actions for endpoint routing Priority:3 Work that is nice to have triage-focus Add this label to flag the issue for focus at triage
Projects
None yet
Development

No branches or pull requests

7 participants