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

Add overloads for validation problem results that accept IReadOnlyDictionary<string, string[]> #41899

Open
DamianEdwards opened this issue May 27, 2022 · 5 comments
Labels
api-approved API was approved in API review, it can be implemented area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-minimal-actions Controller-like actions for endpoint routing feature-problem-details
Milestone

Comments

@DamianEdwards
Copy link
Member

Sometimes all you have is a IReadOnlyDictionary and you don't want to have to downcast it or wrap it to pass it to the helper methods.

namespace Microsoft.AspNetCore.Http;

public static class Results
{
+    public static IResult ValidationProblem(IReadOnlyDictionary<string, string[]> errors,
+        string? detail = null,
+        string? instance = null,
+        int? statusCode = null,
+        string? title = null,
+        string? type = null,
+        IReadOnlyDictionary<string, object?>? extensions = null);

+    public static IResult Problem(
+        string? detail = null,
+        string? instance = null,
+        int? statusCode = null,
+        string? title = null,
+        string? type = null,
+        IReadOnlyDictionary<string, object?>? extensions = null);
}

public static class TypedResults
{
+    public static ValidationProblem ValidationProblem(IReadOnlyDictionary<string, string[]> errors,
+        string? detail = null,
+        string? instance = null,
+        string? title = null,
+        string? type = null,
+        IReadOnlyDictionary<string, object?>? extensions = null);

+    public static ProblemHttpResult Problem(
+        string? detail = null,
+        string? instance = null,
+        string? title = null,
+        string? type = null,
+        IReadOnlyDictionary<string, object?>? extensions = null);
}
@DamianEdwards DamianEdwards added feature-minimal-actions Controller-like actions for endpoint routing area-web-frameworks api-suggestion Early API idea and discussion, it is NOT ready for implementation labels May 27, 2022
@rafikiassumani-msft rafikiassumani-msft added this to the .NET 7 Planning milestone May 31, 2022
@ghost
Copy link

ghost commented May 31, 2022

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@captainsafia captainsafia added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Feb 7, 2023
@ghost
Copy link

ghost commented Feb 7, 2023

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@captainsafia
Copy link
Member

Triage: Marking this as ready for API review. Once it is approved for API review, this would be a good candidate for help wanted.

@halter73
Copy link
Member

API Review Notes:

  • Is this a breaking change? Yes. It's ambiguous which overload to pick if you pass in Dictionary<string, string[]>.
  • Can we make it IEnumerable<KeyValuePair<string, string[]>> instead? Yes.
  • Do we want to change both the errors and extensions parameters? Might as well. It will accept more arguments, and we're not modifying either dictionary.

API Approved!

namespace Microsoft.AspNetCore.Http;

public static class Results
{
+    public static IResult ValidationProblem(IEnumerable<KeyValuePair<string, string[]>> errors,
+        string? detail = null,
+        string? instance = null,
+        int? statusCode = null,
+        string? title = null,
+        string? type = null,
+        IEnumerable<KeyValuePair<string, object?>>? extensions = null);

+    public static IResult Problem(
+        string? detail = null,
+        string? instance = null,
+        int? statusCode = null,
+        string? title = null,
+        string? type = null,
+        IEnumerable<KeyValuePair<string, object?>>? extensions = null);
}

public static class TypedResults
{
+    public static ValidationProblem ValidationProblem(IEnumerable<KeyValuePair<string, string[]>> errors,
+        string? detail = null,
+        string? instance = null,
+        string? title = null,
+        string? type = null,
+        IEnumerable<KeyValuePair<string, object?>>? extensions = null);

+    public static ProblemHttpResult Problem(
+        string? detail = null,
+        string? instance = null,
+        string? title = null,
+        string? type = null,
+        IEnumerable<KeyValuePair<string, object?>>? extensions = null);
}

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Mar 20, 2023
@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
@joegoldman2
Copy link
Contributor

joegoldman2 commented Apr 19, 2024

I'm trying to implement this proposal but I have some questions:

  • For all new overloads I'm getting the following warning: RS0026 Symbol 'ValidationProblem' violates the backcompat requirement: 'Do not add multiple overloads with optional parameters'. See 'https://github.com/dotnet/roslyn/blob/main/docs/Adding%20Optional%20Parameters%20in%20Public%20API.md'. Is it ok?
  • I think the parameter int? statusCode = null is missing for the new Problem overload in TypedResults. It exists for the existing overload that accepts a IDictionary<string, object?>?.
    public static ProblemHttpResult Problem(
    string? detail = null,
    string? instance = null,
    int? statusCode = null,
    string? title = null,
    string? type = null,
    IDictionary<string, object?>? extensions = null)
  • I think a new construtor overload that accepts a IEnumerable<KeyValuePair<string, string[]>> for HttpValidationProblemDetails is welcome in addition to the existing one which accepts a IDictionary<string, string[]>. This will make it easier to create a new instance from a IEnumerable<KeyValuePair<string, string[]>> and will also ensure a degree of consistency.
    public HttpValidationProblemDetails(IDictionary<string, string[]> errors)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-minimal-actions Controller-like actions for endpoint routing feature-problem-details
Projects
No open projects
Status: Committed
Development

No branches or pull requests

7 participants