Skip to content

Add dynamic controller/page routes #8955

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

Merged
merged 1 commit into from
Jun 24, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Http/Routing/src/Matching/HttpMethodMatcherPolicy.cs
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,8 @@ public Task ApplyAsync(HttpContext httpContext, CandidateSet candidates)
{
// We do this check first for consistency with how 405 is implemented for the graph version
// of this code. We still want to know if any endpoints in this set require an HTTP method
// even if those endpoints are already invalid.
var metadata = candidates[i].Endpoint.Metadata.GetMetadata<IHttpMethodMetadata>();
// even if those endpoints are already invalid - hence the null-check.
var metadata = candidates[i].Endpoint?.Metadata.GetMetadata<IHttpMethodMetadata>();
if (metadata == null || metadata.HttpMethods.Count == 0)
{
// Can match any method.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ public static partial class ControllerEndpointRouteBuilderExtensions
public static Microsoft.AspNetCore.Builder.ControllerActionEndpointConventionBuilder MapControllerRoute(this Microsoft.AspNetCore.Routing.IEndpointRouteBuilder endpoints, string name, string pattern, object defaults = null, object constraints = null, object dataTokens = null) { throw null; }
public static Microsoft.AspNetCore.Builder.ControllerActionEndpointConventionBuilder MapControllers(this Microsoft.AspNetCore.Routing.IEndpointRouteBuilder endpoints) { throw null; }
public static Microsoft.AspNetCore.Builder.ControllerActionEndpointConventionBuilder MapDefaultControllerRoute(this Microsoft.AspNetCore.Routing.IEndpointRouteBuilder endpoints) { throw null; }
public static void MapDynamicControllerRoute<TTransformer>(this Microsoft.AspNetCore.Routing.IEndpointRouteBuilder endpoints, string pattern) where TTransformer : Microsoft.AspNetCore.Mvc.Routing.DynamicRouteValueTransformer { }
public static Microsoft.AspNetCore.Builder.IEndpointConventionBuilder MapFallbackToAreaController(this Microsoft.AspNetCore.Routing.IEndpointRouteBuilder endpoints, string action, string controller, string area) { throw null; }
public static Microsoft.AspNetCore.Builder.IEndpointConventionBuilder MapFallbackToAreaController(this Microsoft.AspNetCore.Routing.IEndpointRouteBuilder endpoints, string pattern, string action, string controller, string area) { throw null; }
public static Microsoft.AspNetCore.Builder.IEndpointConventionBuilder MapFallbackToController(this Microsoft.AspNetCore.Routing.IEndpointRouteBuilder endpoints, string action, string controller) { throw null; }
Expand Down Expand Up @@ -2909,6 +2910,11 @@ public ValidatorCache() { }
}
namespace Microsoft.AspNetCore.Mvc.Routing
{
public abstract partial class DynamicRouteValueTransformer
{
protected DynamicRouteValueTransformer() { }
public abstract System.Threading.Tasks.Task<Microsoft.AspNetCore.Routing.RouteValueDictionary> TransformAsync(Microsoft.AspNetCore.Http.HttpContext httpContext, Microsoft.AspNetCore.Routing.RouteValueDictionary values);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ValueTask

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? Is the guidance to just use ValueTask everywhere now?

Copy link

@SoftwareDreamer SoftwareDreamer Jun 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am aware that you most probably know this, and your question is partially rhetoric, but..
I think the guidance (as of "Stephen Toub says so") is that if the method is mostly expected to return synchronously (like in a "streaming over bytes that are most of the time in memory but rarely have to be fetched over the wire" scenario), use a ValueTask, but if the method is expected to do real async work reasonably often (like let's say > 20% of the time, I'm not sure where I got this number from, but it stuck) use Task, as ValueTask comes with it's own struct-based overhead and just wraps a real Task in that case..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ValueTask<T> is still the guidance, there's almost no reason to use Task<T> as it forces an allocation. You can also now reuse allocations with ValueTask<T> so it's an extensibility point as well.

Why? Is the guidance to just use ValueTask everywhere now?

Yes, generic ValueTask<T> absolutely, for ValueTask is more questionable and has more overhead and is only really valuable as an extensibility point (using IValueTaskSource)

Copy link

@SoftwareDreamer SoftwareDreamer Jun 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hrm didn't sound like it here: https://devblogs.microsoft.com/dotnet/understanding-the-whys-whats-and-whens-of-valuetask/#user-content-should-every-new-asynchronous-api-return-valuetask--valuetasktresult
"In short, no: the default choice is still Task / Task."
...
" There are also some minor costs associated with returning a ValueTask instead of a Task, e.g. in microbenchmarks it’s a bit faster to await a Task than it is to await a ValueTask, so if you can use cached tasks (e.g. you’re API returns Task or Task), you might be better off performance-wise sticking with Task and Task. ValueTask / ValueTask are also multiple words in size, and so when these are awaitd and a field for them is stored in a calling async method’s state machine, they’ll take up a little more space in that state machine object."

Honstely that#s a bit over my head though... back then I read "use ValueTask only when it completes mosty synchronous" out of it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, ValueTask / ValueTask are great choices when a) you expect consumers of your API to only await them directly, b) allocation-related overhead is important to avoid for your API, and c) either you expect synchronous completion to be a very common case, or you’re able to effectively pool objects for use with asynchronous completion. When adding abstract, virtual, or interface methods, you also need to consider whether these situations will exist for overrides/implementations of that method.

If you have a virtual member, you can't assume what derived implementations will do. To optimize for common synchronous cases and offer extensibility for derived types, use ValueTask<T>.

The framework is the one that is awaiting this so it's mostly up to the implementation to just return synchronously or asynchronously. The other downsides don't apply.

I really want to go back the change a bunch of APIs to return ValueTask<T> 😄 . I could squash so many needless allocations in the auth stack..

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appreciate the time you took to answer that. Hopefully someone finds this hidden conversation, reads it and learns, too.

}
[System.AttributeUsageAttribute(System.AttributeTargets.Method, AllowMultiple=true, Inherited=true)]
public abstract partial class HttpMethodAttribute : System.Attribute, Microsoft.AspNetCore.Mvc.Routing.IActionHttpMethodProvider, Microsoft.AspNetCore.Mvc.Routing.IRouteTemplateProvider
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using Microsoft.AspNetCore.Mvc.Routing;
using Microsoft.AspNetCore.Routing;
using Microsoft.AspNetCore.Routing.Constraints;
using Microsoft.AspNetCore.Routing.Patterns;
using Microsoft.Extensions.DependencyInjection;

namespace Microsoft.AspNetCore.Builder
Expand Down Expand Up @@ -212,7 +213,7 @@ public static IEndpointConventionBuilder MapFallbackToController(
EnsureControllerServices(endpoints);

// Called for side-effect to make sure that the data source is registered.
GetOrCreateDataSource(endpoints);
GetOrCreateDataSource(endpoints).CreateInertEndpoints = true;

// Maps a fallback endpoint with an empty delegate. This is OK because
// we don't expect the delegate to run.
Expand Down Expand Up @@ -288,7 +289,7 @@ public static IEndpointConventionBuilder MapFallbackToController(
EnsureControllerServices(endpoints);

// Called for side-effect to make sure that the data source is registered.
GetOrCreateDataSource(endpoints);
GetOrCreateDataSource(endpoints).CreateInertEndpoints = true;

// Maps a fallback endpoint with an empty delegate. This is OK because
// we don't expect the delegate to run.
Expand Down Expand Up @@ -356,7 +357,7 @@ public static IEndpointConventionBuilder MapFallbackToAreaController(
EnsureControllerServices(endpoints);

// Called for side-effect to make sure that the data source is registered.
GetOrCreateDataSource(endpoints);
GetOrCreateDataSource(endpoints).CreateInertEndpoints = true;

// Maps a fallback endpoint with an empty delegate. This is OK because
// we don't expect the delegate to run.
Expand Down Expand Up @@ -434,7 +435,7 @@ public static IEndpointConventionBuilder MapFallbackToAreaController(
EnsureControllerServices(endpoints);

// Called for side-effect to make sure that the data source is registered.
GetOrCreateDataSource(endpoints);
GetOrCreateDataSource(endpoints).CreateInertEndpoints = true;

// Maps a fallback endpoint with an empty delegate. This is OK because
// we don't expect the delegate to run.
Expand All @@ -447,6 +448,48 @@ public static IEndpointConventionBuilder MapFallbackToAreaController(
return builder;
}

/// <summary>
/// Adds a specialized <see cref="RouteEndpoint"/> to the <see cref="IEndpointRouteBuilder"/> that will
/// attempt to select a controller action using the route values produced by <typeparamref name="TTransformer"/>.
/// </summary>
/// <param name="endpoints">The <see cref="IEndpointRouteBuilder"/> to add the route to.</param>
/// <param name="pattern">The URL pattern of the route.</param>
/// <typeparam name="TTransformer">The type of a <see cref="DynamicRouteValueTransformer"/>.</typeparam>
/// <remarks>
/// <para>
/// This method allows the registration of a <see cref="RouteEndpoint"/> and <see cref="DynamicRouteValueTransformer"/>
/// that combine to dynamically select a controller action using custom logic.
/// </para>
/// <para>
/// The instance of <typeparamref name="TTransformer"/> will be retrieved from the dependency injection container.
/// Register <typeparamref name="TTransformer"/> with the desired service lifetime in <c>ConfigureServices</c>.
/// </para>
/// </remarks>
public static void MapDynamicControllerRoute<TTransformer>(this IEndpointRouteBuilder endpoints, string pattern)
where TTransformer : DynamicRouteValueTransformer
{
if (endpoints == null)
{
throw new ArgumentNullException(nameof(endpoints));
}

EnsureControllerServices(endpoints);

// Called for side-effect to make sure that the data source is registered.
GetOrCreateDataSource(endpoints).CreateInertEndpoints = true;

endpoints.Map(
pattern,
context =>
{
throw new InvalidOperationException("This endpoint is not expected to be executed directly.");
})
.Add(b =>
{
b.Metadata.Add(new DynamicControllerRouteValueTransformerMetadata(typeof(TTransformer)));
});
}

private static DynamicControllerMetadata CreateDynamicControllerMetadata(string action, string controller, string area)
{
return new DynamicControllerMetadata(new RouteValueDictionary()
Expand Down
14 changes: 7 additions & 7 deletions src/Mvc/Mvc.Core/src/Infrastructure/ActionSelectionTable.cs
Original file line number Diff line number Diff line change
Expand Up @@ -74,23 +74,23 @@ public static ActionSelectionTable<ActionDescriptor> Create(ActionDescriptorColl
});
}

public static ActionSelectionTable<RouteEndpoint> Create(IEnumerable<Endpoint> endpoints)
public static ActionSelectionTable<Endpoint> Create(IEnumerable<Endpoint> endpoints)
{
return CreateCore<RouteEndpoint>(
return CreateCore<Endpoint>(

// we don't use version for endpoints
version: 0,

// Only include RouteEndpoints and only those that aren't suppressed.
items: endpoints.OfType<RouteEndpoint>().Where(e =>
// Exclude RouteEndpoints - we only process inert endpoints here.
items: endpoints.Where(e =>
{
return e.Metadata.GetMetadata<ISuppressMatchingMetadata>()?.SuppressMatching != true;
return e.GetType() == typeof(Endpoint);
}),

getRouteKeys: e => e.RoutePattern.RequiredValues.Keys,
getRouteKeys: e => e.Metadata.GetMetadata<ActionDescriptor>().RouteValues.Keys,
getRouteValue: (e, key) =>
{
e.RoutePattern.RequiredValues.TryGetValue(key, out var value);
e.Metadata.GetMetadata<ActionDescriptor>().RouteValues.TryGetValue(key, out var value);
return Convert.ToString(value, CultureInfo.InvariantCulture) ?? string.Empty;
});
}
Expand Down
124 changes: 80 additions & 44 deletions src/Mvc/Mvc.Core/src/Routing/ActionEndpointFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ namespace Microsoft.AspNetCore.Mvc.Routing
internal class ActionEndpointFactory
{
private readonly RoutePatternTransformer _routePatternTransformer;
private readonly RequestDelegate _requestDelegate;

public ActionEndpointFactory(RoutePatternTransformer routePatternTransformer)
{
Expand All @@ -29,14 +30,16 @@ public ActionEndpointFactory(RoutePatternTransformer routePatternTransformer)
}

_routePatternTransformer = routePatternTransformer;
_requestDelegate = CreateRequestDelegate();
}

public void AddEndpoints(
List<Endpoint> endpoints,
HashSet<string> routeNames,
ActionDescriptor action,
IReadOnlyList<ConventionalRouteEntry> routes,
IReadOnlyList<Action<EndpointBuilder>> conventions)
IReadOnlyList<Action<EndpointBuilder>> conventions,
bool createInertEndpoints)
{
if (endpoints == null)
{
Expand All @@ -63,6 +66,26 @@ public void AddEndpoints(
throw new ArgumentNullException(nameof(conventions));
}

if (createInertEndpoints)
{
var builder = new InertEndpointBuilder()
{
DisplayName = action.DisplayName,
RequestDelegate = _requestDelegate,
};
AddActionDataToBuilder(
builder,
routeNames,
action,
routeName: null,
dataTokens: null,
suppressLinkGeneration: false,
suppressPathMatching: false,
conventions,
Array.Empty<Action<EndpointBuilder>>());
endpoints.Add(builder.Build());
}

if (action.AttributeRouteInfo == null)
{
// Check each of the conventional patterns to see if the action would be reachable.
Expand All @@ -81,18 +104,21 @@ public void AddEndpoints(

// We suppress link generation for each conventionally routed endpoint. We generate a single endpoint per-route
// to handle link generation.
var builder = CreateEndpoint(
var builder = new RouteEndpointBuilder(_requestDelegate, updatedRoutePattern, route.Order)
{
DisplayName = action.DisplayName,
};
AddActionDataToBuilder(
builder,
routeNames,
action,
updatedRoutePattern,
route.RouteName,
route.Order,
route.DataTokens,
suppressLinkGeneration: true,
suppressPathMatching: false,
conventions,
route.Conventions);
endpoints.Add(builder);
endpoints.Add(builder.Build());
}
}
else
Expand All @@ -109,18 +135,21 @@ public void AddEndpoints(
throw new InvalidOperationException("Failed to update route pattern with required values.");
}

var endpoint = CreateEndpoint(
var builder = new RouteEndpointBuilder(_requestDelegate, updatedRoutePattern, action.AttributeRouteInfo.Order)
{
DisplayName = action.DisplayName,
};
AddActionDataToBuilder(
builder,
routeNames,
action,
updatedRoutePattern,
action.AttributeRouteInfo.Name,
action.AttributeRouteInfo.Order,
dataTokens: null,
action.AttributeRouteInfo.SuppressLinkGeneration,
action.AttributeRouteInfo.SuppressPathMatching,
conventions,
perRouteConventions: Array.Empty<Action<EndpointBuilder>>());
endpoints.Add(endpoint);
endpoints.Add(builder.Build());
}
}

Expand Down Expand Up @@ -262,49 +291,17 @@ private static (RoutePattern resolvedRoutePattern, IDictionary<string, string> r
return (attributeRoutePattern, resolvedRequiredValues ?? action.RouteValues);
}

private RouteEndpoint CreateEndpoint(
private void AddActionDataToBuilder(
EndpointBuilder builder,
HashSet<string> routeNames,
ActionDescriptor action,
RoutePattern routePattern,
string routeName,
int order,
RouteValueDictionary dataTokens,
bool suppressLinkGeneration,
bool suppressPathMatching,
IReadOnlyList<Action<EndpointBuilder>> conventions,
IReadOnlyList<Action<EndpointBuilder>> perRouteConventions)
{

// We don't want to close over the retrieve the Invoker Factory in ActionEndpointFactory as
// that creates cycles in DI. Since we're creating this delegate at startup time
// we don't want to create all of the things we use at runtime until the action
// actually matches.
//
// The request delegate is already a closure here because we close over
// the action descriptor.
IActionInvokerFactory invokerFactory = null;

RequestDelegate requestDelegate = (context) =>
{
var routeData = new RouteData();
routeData.PushState(router: null, context.Request.RouteValues, dataTokens);

var actionContext = new ActionContext(context, routeData, action);

if (invokerFactory == null)
{
invokerFactory = context.RequestServices.GetRequiredService<IActionInvokerFactory>();
}

var invoker = invokerFactory.CreateInvoker(actionContext);
return invoker.InvokeAsync();
};

var builder = new RouteEndpointBuilder(requestDelegate, routePattern, order)
{
DisplayName = action.DisplayName,
};

// Add action metadata first so it has a low precedence
if (action.EndpointMetadata != null)
{
Expand Down Expand Up @@ -399,8 +396,47 @@ private RouteEndpoint CreateEndpoint(
{
perRouteConventions[i](builder);
}
}

return (RouteEndpoint)builder.Build();
private static RequestDelegate CreateRequestDelegate()
{
// We don't want to close over the Invoker Factory in ActionEndpointFactory as
// that creates cycles in DI. Since we're creating this delegate at startup time
// we don't want to create all of the things we use at runtime until the action
// actually matches.
//
// The request delegate is already a closure here because we close over
// the action descriptor.
IActionInvokerFactory invokerFactory = null;

return (context) =>
{
var endpoint = context.GetEndpoint();
var dataTokens = endpoint.Metadata.GetMetadata<IDataTokensMetadata>();

var routeData = new RouteData();
routeData.PushState(router: null, context.Request.RouteValues, new RouteValueDictionary(dataTokens?.DataTokens));

// Don't close over the ActionDescriptor, that's not valid for pages.
var action = endpoint.Metadata.GetMetadata<ActionDescriptor>();
var actionContext = new ActionContext(context, routeData, action);

if (invokerFactory == null)
{
invokerFactory = context.RequestServices.GetRequiredService<IActionInvokerFactory>();
}

var invoker = invokerFactory.CreateInvoker(actionContext);
return invoker.InvokeAsync();
};
}

private class InertEndpointBuilder : EndpointBuilder
{
public override Endpoint Build()
{
return new Endpoint(RequestDelegate, new EndpointMetadataCollection(Metadata), DisplayName);
}
}
}
}
4 changes: 2 additions & 2 deletions src/Mvc/Mvc.Core/src/Routing/ConsumesMatcherPolicy.cs
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,8 @@ public Task ApplyAsync(HttpContext httpContext, CandidateSet candidates)
{
// We do this check first for consistency with how 415 is implemented for the graph version
// of this code. We still want to know if any endpoints in this set require an a ContentType
// even if those endpoints are already invalid.
var metadata = candidates[i].Endpoint.Metadata.GetMetadata<IConsumesMetadata>();
// even if those endpoints are already invalid - hence the null check.
var metadata = candidates[i].Endpoint?.Metadata.GetMetadata<IConsumesMetadata>();
if (metadata == null || metadata.ContentTypes.Count == 0)
{
// Can match any content type.
Expand Down
Loading