Make-method-async code fix breaks ASP.NET MVC action routing after 'Async' suffix is appended to method name

[terrajobst]

From @jeremymeng on August 28, 2017 18:31

Version Used:

Steps to Reproduce:

1. create a ASP.NET Core project
2. select MVC template, create the project, and F5 to make sure the home page is shown
3. open HomeController.cs
4. changing return type of Index() to Task<IActionResult>
5. Add await Task.Delay(1); inside Index()
6. Invoke code fix to make the method async

Now the method looks like

        public async Task<IActionResult> IndexAsync()
        {
            await Task.Delay(300);
            return View();
        }

7. F5
   Actual Behavior:
   HTTP 404

if rename the method back to Index() F5 works again.

Copied from original issue: dotnet/roslyn#21770

[terrajobst]

From @jeremymeng on August 28, 2017 18:36

ASP.NET uses the controller action names to locate the views. If there's an IndexAsync.cshtml then http://localhost:55100/Home/IndexAsync would work

[terrajobst]

It should be added that Roslyn allows project systems to fix up identifiers after a rename occurs (by providng an event). However, that doesn't help in this case as routing is rule based. One could argue that the routing engine could handle that, but this could have potential for breaking changes.

So as written, I think this should be moved to https://github.com/aspnet/Mvc/issues. However, one could also argue that the "make this method async"-fixer shouldn't also rename the method, but that seems like the wrong direction to me.

/cc @CyrusNajmabadi

[terrajobst]

From @CyrusNajmabadi on August 28, 2017 18:57

However, one could also argue that the "make this method async"-fixer shouldn't also rename the method, but that seems like the wrong direction to me.

I agree. While things break in this case, we do the right thing for the majority of cases. 'Async' suffix is a strong pattern in the framework, and should be something we push code toward. If we don't do this, it just means that users have to take the extra step of having to rename the method. If you have to do a lot of this, it's an onerous process.

Here it looks like we have an API that uses stringly typed connections. If that framework wants to work that way, it should hook into the existing roslyn notifications for when this happens so it can appropriately update things. Also, it should consider providing an analyzer to tell the user when the names don't match the routing so that things can be clearer.

Finally, it seems to me like MVC should just support the Async extension. It's a core .NET naming style.

[terrajobst]

From @jeremymeng on August 28, 2017 19:5

Agree on the value of "Async" suffix. It would be great if ASP.NET MVC can handle this. Can you please help move the issue over to them?

[Eilon]

This is currently by-design; MVC with conventional routes does matching based on method names, regardless of any perceived "suffix."

@rynowak - I think we used to have some rules that relaxed some of these requirements in MVC 1-5, but I guess we got rid of them. Is it because everything is now async, so there was no point trying to distinguish async methods from non-async methods, so we supported just exact name matches?

It would be a breaking change to start supporting this conditional match, but we could consider it in 3.0.

[Eilon]

We'll look at making this change in 3.0.0, which is the next time we can do this kind of breaking change.

[dazhao-msft]

My team enforces 'Async' suffix in the async methods so we do have to suppress the warnings in all the async action methods.

@Eilon is this still on radar of ASP.NET Core 3.0?

@rynowak before 3.0, would it possible to provide a workaround by implementing IOutboundParameterTransformer introduced in ASP.NET Core 2.2? I did a quick prototype but it turns out the input of TransformOutbound is very limited. It is not enough to safely determine if the 'Async' suffix can be removed from the route value. DI more context via constructor seems not the right way either.

Thanks,
-David

[rynowak]

If you need an unblocker, the easiest way to do this would be to write a convention.

public class MyConvention : IActionModelConvention
{
    public void Apply(ActionModel action)
    {
        if (action.ActionName.EndsWith("Async", StringComparison.Ordinal))
        {
            action.ActionName = action.ActionName.Substring(0, action.ActionName.Length - "Async".Length);
        }
    }
}

You can register this like:

services.AddMvc(options =>
{
    options.Conventions.Add(new MyConvention());
});

[pranavkm]

@Eilon \ @mkArtakMSFT would this entail an announcement or would we just make note of it in the 3.0 migration notes?

[Eilon]

Seems pretty minor, so I'd say not worth it.