Error when using RouteAttribute and HttpPost together on same action

[DamianEdwards]

Microsoft.AspNet.Mvc.Routing (1.0.0-beta1-10401)

Repro Steps

• Create a new ASP.NET vNext MVC project using the project template
• Add the following action method declaration to the HomeController:

[Route("foo")]
[HttpPost]
public IActionResult Foo()
{
    return View();
}

• Launch the application from VS

Expected Results

The site runs

Actual Results

A Helios error is displayed with the following details:

System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> System.InvalidOperationException: The following errors occurred with attribute routing information:

Error 1:
A method 'MusicStore.Apis.AlbumsApiController.CreateAlbum' must not define attribute routed actions and non attribute routed actions at the same time:
Action: 'MusicStore.Apis.AlbumsApiController.CreateAlbum' - Template: 'api/albums'
Action: 'MusicStore.Apis.AlbumsApiController.CreateAlbum' - Template: '(none)'
 at Microsoft.AspNet.Mvc.ReflectedActionDescriptorProvider.Build(ReflectedApplicationModel model)
 at Microsoft.AspNet.Mvc.ReflectedActionDescriptorProvider.GetDescriptors()
 at Microsoft.AspNet.Mvc.ReflectedActionDescriptorProvider.Invoke(ActionDescriptorProviderContext context, Action callNext)
 at Microsoft.Framework.DependencyInjection.NestedProviders.NestedProviderManager`1.CallNext.CallNextProvider()
 at Microsoft.Framework.DependencyInjection.NestedProviders.NestedProviderManager`1.Invoke(T context)
 at Microsoft.AspNet.Mvc.DefaultActionDescriptorsCollectionProvider.GetCollection()
 at Microsoft.AspNet.Mvc.DefaultActionDescriptorsCollectionProvider.get_ActionDescriptors()
 at Microsoft.AspNet.Mvc.Routing.AttributeRouting.GetActionDescriptors(IServiceProvider services)
 at Microsoft.AspNet.Mvc.Routing.AttributeRouting.CreateAttributeMegaRoute(IRouter target, IServiceProvider services)
 at Microsoft.AspNet.Builder.BuilderExtensions.UseMvc(IApplicationBuilder app, Action`1 configureRoutes)
 at Microsoft.AspNet.Builder.BuilderExtensions.UseMvc(IApplicationBuilder app)
 at MusicStore.Spa.Startup.Configure(IApplicationBuilder app) in C:\src\GitHub\AspNet\MusicStore\src\MusicStore.Spa\Startup.cs:line 66
 --- End of inner exception stack trace ---
 at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor)
 at System.Reflection.RuntimeMethodInfo.UnsafeInvokeInternal(Object obj, Object[] parameters, Object[] arguments)
 at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
 at Microsoft.AspNet.Hosting.Startup.StartupLoader.<>c__DisplayClass0.<LoadStartup>b__3(IApplicationBuilder builder)
 at Microsoft.AspNet.Loader.IIS.KlrHttpApplication.HeliosStartupLoaderProvider.WrappingStartupLoader.<>c__DisplayClass0.<LoadStartup>b__1(IApplicationBuilder builder)
 at Microsoft.AspNet.Hosting.HostingEngine.EnsureApplicationDelegate(HostingContext context)
 at Microsoft.AspNet.Hosting.HostingEngine.Start(HostingContext context)
 at Microsoft.AspNet.Loader.IIS.KlrHttpApplication.ApplicationStart(IHttpApplication application)
 at Microsoft.AspNet.Loader.IIS.HttpApplicationBase.InvokeApplicationStart(IHttpApplication application)

[danroth27]

We should definitely improve the error message and also see if we can come up with a behavior that is more compatible with the MVC 5 behavior.

[javiercn]

The current behavior is by design, [Route] defines an attribute routed action without a constraint. [HttpGet] without a valid template defines an IHttpConstraint. In the new behavior a method can define attributed actions or non attributed actions, but not both at the same time.

The same behavior can be achieved by just doing

[HttpPost("foo")]

We can discuss changing this when @rynowak is around.

[DamianEdwards]

@javiercn was it really intended that HttpPost without a template argument defines a new route? You say that without a template it defines an IHttpConstraint. That does not appear to be what's happening right now. If that were the case, I'd expect the example above to result in a single route (foo) with a constraint of POST added.

[javiercn]

@DamianEdwards Sorry, I didn't explain myself properly. Let me try to reword it in clearer terms. An action like Foo can define one or more actions that can either be conventionally routed or attribute routed, but not both at the same time.

If a method defines a valid attribute routed action (like the one produced by route) we consider that all the actions defined by that method have to be attribute routed (we don't allow mixing conventionally routed and attribute routed actions on the same method, that's the error message).

That said, If during action discovery we see that you don't define a valid route template (by providing a template using [Route] on the controller, or [Http_] or [Route] on the action) we just treat [Http_] as plain IHttpConstraints so that you can still have conventionally routed actions that use [HttpGet], [AcceptVerbs], etc.

The recomendation is not to do

[Route("template")]
[Http*]
public void MyAction(){}

but to just do

[Http*("template")]
public void MyAction(){}

or

[AcceptVerbs("POST, GET", Route = "template")]
public void MyAction(){}

if you wan to support multiple HTTP methods.

If you want to enable this scenario

[Route("template")]
[HttpPost]
public void MyAction(){}

you can easily do so by writing your own IActionDiscoveryConvention or by writing an IReflectedApplicationModelConvention that tweaks the orginal model we produce.

The reason why we chose this approach is that you can achieve the scenario above more succinctly by just using [Http*("template")] and with the other option, scenarios like the following

[HttpPut("v1/template/{id}")]
[HttpPatch("v2/template/{id}")]
public void MyAction(int id, ...){}

won't work as expected out of the box.

I hope this makes things clearer, but I completely agree with you that the error message could be improved.

[rynowak]

Suggested change here is to change the way grouping of attributes is done for routing

Rules:
1). Treat an IRouteTemplateProvider without a route template as a non-IRouteTemplateProvider
2). When an IRouteTemplateProvider is not an IHttpMethodProvider, group IHttpMethodProvider attributes that aren't IRouteTemplateProvider with it.

Examples: - Note that scenario 1 below is the only change from current code

1). Back compat

[HttpGet]
[Route("api/Products")]
public void DoThing() { }

Route - Verb
"api/Products" - GET

Rule 1 says that [HttpGet] is not a route template provider
Rule 2 says to put it together with [Route(...)]

2). More routes/verbs

[HttpGet]
[HttpPost]
[Route("api/Products")]
[Route("api/v2/Products")]
public void DoThing() { }

Route - Verb
"api/Products" - GET, POST
"api/v2/Products" - GET, POST

Rule 1 says that [HttpGet]/[HttpPost] is not a route template provider
Rule 2 says to put them together with [Route(...)]

3). Separate urls/verbs

[Route("api/Products"]
[HttpPost("api/Products/Buy")]
[HttpPut("api/Products/Buy")]
public void DoThing() { }

Route - Verb
"api/Products" - *
"api/Products/Buy" - POST
"api/Products/Buy" - PUT

Rule 1 says that [HttpPut(...)]/[HttpPost(...)] are route template providers
Rule 2 says not to combine them with [Route(...)]

4). Something that's still broken

[HttpPost]
[HttpPut("api/Products/Buy")]
public void DoThing() { }

Route - Verb
(no route) - POST - THIS WILL THROW
"api/Products/Buy" - PUT

Rule 1 says that [HttpPost] is not a route template provider
Rule 2 says not to combine it with [HttpPut(...)]

[Encosia]

Thanks for addressing the back compat scenario.

To make sure I understand, will combining separate methods on the same route continue working too? Rough example:

[Route("Products/{ProductId:int}")]
[HttpGet]
public ActionResult Index(int ProductId) {
  // Get Product by Id.
}

[Route("Products/{ProductId:int}")]
[HttpPost]
public ActionResult Index(int ProductId, [FromBody]Product Product) {
  // Update Product based on POST.
}

[rynowak]

@Encosia glad we could help!

Yes, your example is valid. You'll end up with two actions

Index(int ProductId) - GET - Products/{ProductId:int}
Index(int ProductId, [FromBody]Product Product) - POST - Products/{ProductId:int}

Action selection will disambiguate based on the HTTP verb of the request.

The following are also valid if you want multiple verbs in the same action.

[HttpGet(...)]
[HttpPost(...)]
public void DoStuff() { }

[HttpGet]
[HttpPost]
[Route(...)]
public void DoStuff() { }

[AcceptVerbs("GET", "POST")]
[Route(...)]
public void DoStuff() { }

Does that answer your question?

[Encosia]

Yes. That's perfect. Just wanted to be sure I understood.

[rynowak]

ed8ba5a