Skip to content

Add RouteValueDictionary overloads to Http.Results #46542

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

Closed
JamesNK opened this issue Feb 9, 2023 · 3 comments · Fixed by #46594
Closed

Add RouteValueDictionary overloads to Http.Results #46542

JamesNK opened this issue Feb 9, 2023 · 3 comments · Fixed by #46594
Labels
api-approved API was approved in API review, it can be implemented old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels
Milestone

Comments

@JamesNK
Copy link
Member

JamesNK commented Feb 9, 2023

Background and Motivation

Results and TypedResults have methods that use reflection to create route values. These have all been annotated as unsafe.

public static partial class Results
{
    [RequiresUnreferencedCode(RouteValueDictionaryTrimmerWarning.Warning)]
    public static IResult CreatedAtRoute(string? routeName = null, object? routeValues = null, object? value = null);
}

We have previously worked around this problem by adding overloads that take RouteValueDictionary. Values can be explicitly added to the dictionary, and the dictionary is then passed to routing. This is a trimming and AOT-safe alternative.

Proposed API

namespace Microsoft.AspNetCore.Http;

public static partial class Results
{
+    public static IResult RedirectToRoute(string? routeName, RouteValueDictionary? routeValues, bool permanent = false, bool preserveMethod = false, string? fragment = null);
+    public static IResult CreatedAtRoute(string? routeName, RouteValueDictionary routeValues, object? value = null);
+    public static IResult CreatedAtRoute<TValue>(string? routeName, RouteValueDictionary routeValues, TValue? value = default);
+    public static IResult AcceptedAtRoute(string? routeName, RouteValueDictionary routeValues, object? value = null);
+    public static IResult AcceptedAtRoute<TValue>(string? routeName, RouteValueDictionary routeValues, TValue? value = default);
}

public static partial class TypedResults
{
+    public static RedirectToRouteHttpResult RedirectToRoute(string? routeName, RouteValueDictionary? routeValues, bool permanent = false, bool preserveMethod = false, string? fragment = null);
+    public static CreatedAtRoute CreatedAtRoute(string? routeName, RouteValueDictionary routeValues);
+    public static CreatedAtRoute<TValue> CreatedAtRoute<TValue>(TValue? value, string? routeName, RouteValueDictionary routeValues);
+    public static AcceptedAtRoute AcceptedAtRoute(string? routeName, RouteValueDictionary routeValues);
+    public static AcceptedAtRoute<TValue> AcceptedAtRoute<TValue>(TValue? value, string? routeName, RouteValueDictionary routeValues);
}

Usage Examples

app.MapGet("/old-path", () => Results.CreatedAtRoute("RouteName", new RouteValueDictionary { ["routeValue1"] = "value!" }));

Alternative Designs

Generic type parameter for route values was explored in #46082. For example:

public static partial class Results
{
+    public static IResult CreatedAtRoute<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] TRouteValues>(string? routeName, TRouteValues routeValues, object? value = null);
}

It doesn't require converting the route values object into a RouteValueDictionary, but it has some problems:

  1. It doesn't work with polymorphism. Property values are gotten from the route value type based on its runtime type. If the route value instance is cast to object and then used with CreatedAtRoute<TRouteValues>, its properties aren't preserved.
  2. It could be a source-breaking change. There will now be a CreatedAtRoute<TRouteValues> overload and a CreatedAtRoute<TValue> overload. Code that explicitly specifies the generic type when calling CreatedAtRoute could cause an ambiguous method compiler error.

For example:

return Results.CreatedAtRoute<object>("RouteName", new { param1 = "param1Value" }, new User());

Risks

There are a lot of overloads, optional parameters and nullable types. It is possible to create method overloads that are ambiguous.

To avoid that:

  1. RouteValueDictionary args never have a default value. It must be specified.
  2. Tests for methods must be extensive.
@JamesNK JamesNK added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Feb 9, 2023
@JamesNK JamesNK added this to the 8.0-preview2 milestone Feb 9, 2023
@ghost
Copy link

ghost commented Feb 9, 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.

@JamesNK
Copy link
Member Author

JamesNK commented Feb 9, 2023

cc @davidfowl

@halter73
Copy link
Member

API Review Notes:

  • Do we take the potentially-source-breaking DynamicallyAccessedMembers approach? No.
  • Should the RouteValueDictionary parameters all be nullable or non-nullable? It sounds like we prefer nullable.
    • This means that we need to update the nullability of RedirectToRouteHttpResult.RouteValues to be non-nullable.
  • We're leaving off default values for the dictionary to avoid source-breaking changes.
  • Results.AcceptedAtRoute(null, null) prefers the new overload surprisingly, but this should be fine.
  • Should we obsolete the object? routeValues overloads? No. If you're not AOT or trimming, there's no need to change your code. If you are doing either of those things, you'll get warnings.

API Approved!

namespace Microsoft.AspNetCore.Http;

public static partial class Results
{
+    public static IResult RedirectToRoute(string? routeName, RouteValueDictionary? routeValues, bool permanent = false, bool preserveMethod = false, string? fragment = null);
+    public static IResult CreatedAtRoute(string? routeName, RouteValueDictionary? routeValues, object? value = null);
+    public static IResult CreatedAtRoute<TValue>(string? routeName, RouteValueDictionary? routeValues, TValue? value = default);
+    public static IResult AcceptedAtRoute(string? routeName, RouteValueDictionary? routeValues, object? value = null);
+    public static IResult AcceptedAtRoute<TValue>(string? routeName, RouteValueDictionary? routeValues, TValue? value = default);
}

public static partial class TypedResults
{
+    public static RedirectToRouteHttpResult RedirectToRoute(string? routeName, RouteValueDictionary? routeValues, bool permanent = false, bool preserveMethod = false, string? fragment = null);
+    public static CreatedAtRoute CreatedAtRoute(string? routeName, RouteValueDictionary? routeValues);
+    public static CreatedAtRoute<TValue> CreatedAtRoute<TValue>(TValue? value, string? routeName, RouteValueDictionary? routeValues);
+    public static AcceptedAtRoute AcceptedAtRoute(string? routeName, RouteValueDictionary? routeValues);
+    public static AcceptedAtRoute<TValue> AcceptedAtRoute<TValue>(TValue? value, string? routeName, RouteValueDictionary? routeValues);
}

namespace Microsoft.AspNetCore.Http.HttpResults;

public sealed partial class RedirectToRouteHttpResult : IResult
{
-    public RouteValueDictionary? RouteValues { get; }
+    public RouteValueDictionary RouteValues { get; }
}

@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 Feb 17, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants