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

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

Closed
terrajobst opened this issue Aug 28, 2017 · 10 comments

Comments

@terrajobst
Copy link
Member

commented Aug 28, 2017

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();
        }
  1. F5
    Actual Behavior:
    HTTP 404

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

Copied from original issue: dotnet/roslyn#21770

@terrajobst

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2017

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

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2017

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

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2017

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

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2017

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 comment has been minimized.

Copy link
Member

commented Aug 29, 2017

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

This comment has been minimized.

Copy link
Member

commented Aug 31, 2017

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

This comment has been minimized.

Copy link

commented Dec 7, 2018

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

This comment has been minimized.

Copy link
Member

commented Dec 7, 2018

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());
});
@aspnet-hello aspnet-hello transferred this issue from aspnet/Mvc Dec 13, 2018
@aspnet-hello aspnet-hello added this to the 3.0.0-preview2 milestone Dec 13, 2018
@mkArtakMSFT mkArtakMSFT added the cost: S label Jan 15, 2019
@pranavkm pranavkm added Working and removed 1 - Ready labels Feb 6, 2019
pranavkm added a commit that referenced this issue Feb 6, 2019
Fixes #4849
pranavkm added a commit that referenced this issue Feb 9, 2019
Fixes #4849
pranavkm added a commit that referenced this issue Feb 12, 2019
Fixes #4849
pranavkm added a commit that referenced this issue Feb 12, 2019
Fixes #4849
@pranavkm pranavkm added Done and removed Working labels Feb 12, 2019
@pranavkm

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

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

@Eilon

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.