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

CreatedAtAction & AcceptedAtAction Don't Work With Methods Whose Name Ends with "Async" #23582

Closed
ConnerPhillis opened this issue Jul 1, 2020 · 10 comments
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. Status: Resolved

Comments

@ConnerPhillis
Copy link

ConnerPhillis commented Jul 1, 2020

Describe the bug

When creating controller methods I often find myself using async / await. When I do, I usually have something in the method's name denoting this (e.g. CreateUserAsync) so I know to await the result of that method later on.

I have possibly found a bug where including the word Async in an action causes CreatedAtAction and AcceptedAtAction to fail to find whatever method you intended to match, giving the stack trace provided in the Exceptions section](#

To Reproduce

  • Create a new controller with a get / post
  • Have the post return a CreatedAtAction that targets the get, include route parameters and the model
  • Execute the post request, it will most likely fail.
  • Remove the Async from the ends of the get method
  • Execute the post request again, it should succeed

Sample controller for testing:

// Random `Task.Delay`s to suppress VS / R# warnings

using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;

using Microsoft.AspNetCore.Mvc;

namespace GhIssue.Controllers
{
	[ApiController]
	[Route("[controller]")]
	public class SamplesController : ControllerBase
	{
		private static readonly List<SampleElement> SampleElements = new List<SampleElement>();

		[HttpGet]
		public async Task<IActionResult> GetSamplesAsync()
		{
			await Task.Delay(100);
			return Ok(SampleElements);
		}

		[HttpGet("{id}")]
		public async Task<IActionResult> GetSampleAsync(int id)
		{
			await Task.Delay(100);
			var element = SampleElements.FirstOrDefault(v => v.Id == id);

			return element == null 
				? (IActionResult) NotFound() 
				: Ok(element);
		}

		[HttpPost]
		public async Task<IActionResult> CreateSampleAsync(SampleElement element)
		{
			await Task.Delay(100);
			element.Id = SampleElements.Count;
			SampleElements.Add(element);

			return CreatedAtAction(nameof(GetSampleAsync), new {id = element.Id}, element);
		}
	}

	public class SampleElement
	{
		public int Id { get; set; }

		public string Message { get; set; }
	}
}

Exceptions (if any)

System.InvalidOperationException: No route matches the supplied values.
   at Microsoft.AspNetCore.Mvc.CreatedAtActionResult.OnFormatting(ActionContext context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ObjectResultExecutor.ExecuteAsyncCore(ActionContext context, ObjectResult result, Type objectType, Object value)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ObjectResultExecutor.ExecuteAsync(ActionContext context, ObjectResult result)
   at Microsoft.AspNetCore.Mvc.ObjectResult.ExecuteResultAsync(ActionContext context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeResultAsync(IActionResult result)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.ResultNext[TFilter,TFilterAsync](State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeNextResultFilterAsync[TFilter,TFilterAsync]()
--- End of stack trace from previous location where exception was thrown ---
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ResultExecutedContextSealed context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.ResultNext[TFilter,TFilterAsync](State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeResultFilters()
--- End of stack trace from previous location where exception was thrown ---
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeFilterPipelineAsync>g__Awaited|19_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
   at Microsoft.AspNetCore.Routing.EndpointMiddleware.<Invoke>g__AwaitRequestTask|6_0(Endpoint endpoint, Task requestTask, ILogger logger)
   at Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)

Further technical details

  • ASP.NET Core version 3.1.4
  • dotnet --info
.NET Core SDK (reflecting any global.json):
 Version:   3.1.201
 Commit:    b1768b4ae7

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.18362
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\3.1.201\

Host (useful for support):
  Version: 3.1.3
  Commit:  4a9f85e9f8

.NET Core SDKs installed:
  2.1.802 [C:\Program Files\dotnet\sdk]
  2.2.202 [C:\Program Files\dotnet\sdk]
  3.1.201 [C:\Program Files\dotnet\sdk]

.NET Core runtimes installed:
  Microsoft.AspNetCore.All 2.1.13 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.All 2.1.17 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.All 2.2.3 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.App 2.1.13 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 2.1.17 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 2.2.3 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.2 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.3 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 2.1.13 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.1.17 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.2.3 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.2 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.3 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.WindowsDesktop.App 3.1.2 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 3.1.3 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  • Using Visual Studio 16.5.4

Sorry if there's documentation that I've missed about this or I've done something wrong, this was a three hour headache for me earlier today and I couldn't find anything that would provide a reason as to why it behaved this way. Thanks in advance!

@Bartmax
Copy link
Contributor

Bartmax commented Jul 1, 2020

which endpoint are you trying to post from html ? /samples/CreateSampleAsync or /samples/CreateSample

@ConnerPhillis
Copy link
Author

Just to be clear, the endpoint I'm hitting in postman is just /samples, the action that I'm backing it with is CreateSampleAsync

@pranavkm pranavkm added ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Jul 2, 2020
@ghost ghost added the Status: Resolved label Jul 2, 2020
@Bartmax
Copy link
Contributor

Bartmax commented Jul 2, 2020

I think there's a bug here @pranavkm
Code shows controller route as Route("[controller]") and [HttpPost] action, so action name should not affect the route resolution for the Post method at all.

@pranavkm
Copy link
Contributor

pranavkm commented Jul 2, 2020

			return CreatedAtAction(nameof(GetSampleAsync), new {id = element.Id}, element);

nameof(GetSampleAsync) wouldn't work with the trimming behavior since the action is GetSample with the 3.0 behavior. We'd wanted to fix this for 5.0, but it seems unlikely at this point.

@ConnerPhillis
Copy link
Author

Sorry if I'm misunderstanding what you guys are saying, just want to clarify a few things to make sure we're all on the same page.

The routes that I'm trying to hit are GET /Samples/{id} and POST /Samples. There is definitely a bug here, whether Async is at the end of the function name should have no bearing whatsoever when the routes are being set using attributes. I am not trying to go to /Samples/GetSampleAsync/{id}, the route specifies such by just having an [HttpGet("{Id}")], meaning the generated route should be (and is) /Samples/{id}.

I was able to find a workaround by setting the option SuppressAsyncSuffixInActionNames to false in ConfigureServices, but I had to do a lot of digging through associated issues to find that out. At the very least this should be mentioned somewhere in the docs, preferrably somewhere around here.

I'd like to close by saying that imo #4849 was handled improperly. The person who posted the issue likely had a misunderstanding about MVC that should have been corrected, not worked around, MVC was working properly. The problem could have been solved by telling them that they should add the attribute [ActionName("index")] and return View("index") instead of just View(). Yes, postfixing Async is a common practice, and yes, that's a pain, but this QOL change traded ease-of-use for another super opaque issue. There is plenty of discussion about this on #8998, one of the suggestions there was to make this an opt-in flag instead of an opt-out. That should be the behavior imo.

Additionally, I just tried this, renaming an action from Index to IndexAsync and renaming the razor page as well results in 404's all round. To get the right page I have to return View("IndexAsync") instead of View(). Not sure if that should be addressed, just another obscurity.

@Bartmax
Copy link
Contributor

Bartmax commented Jul 2, 2020

I'll try to explain what the exchange was all about @ConnerPhillis
One way to fix your issue in code without suppressing the async suffic would be instead of :

 return CreatedAtAction(nameof(GetSampleAsync), new {id = element.Id}, element);

change to:

return CreatedAtAction("GetSample"), new {id = element.Id}, element);

Because the actual action name will be GetSample instead of GetSampleAsync because the Async suffix is removed by default. As you found out, setting SuppressAsyncSuffixInActionNames to false also works.

If the async suffix removal was a right or wrong design, I think I'm with you but that's another issue.

I personally avoid the async suffix in my code, think it's a really bad design/practice. I would rather use Sync if I have a really weird need to provide a sync version of a natural async one.

@ConnerPhillis
Copy link
Author

ConnerPhillis commented Jul 2, 2020

@Bartmax thanks for the clarification, I thought about that but I ended up deciding that I couldn't get on board with introducing magic constants everywhere I wanted to use an async method, plus I think I prefer < Core 3.0's behavior.

As for the async suffix, I've always found it a useful flag to determine if I need to be using await or not. Design patterns are obviously out of scope here, but I find it nice to have a way to express that information when I first view the method's name. To each his own I guess 😄

@Bartmax
Copy link
Contributor

Bartmax commented Jul 2, 2020

That's totally fine! it's definitely a personal preference. When I said really bad practice I didn't mean that really, only that's it's bad for my habits, because it gives (to me) zero value. I totally understand it can be useful for other teams with different habits. wanted to clear that out, I guess I didn't pick the right wording.

I also agree that SuppressAsyncSuffixInActionNames as false is a better default.

@ghost
Copy link

ghost commented Jul 3, 2020

This issue has been resolved and has not had any activity for 1 day. It will be closed for housekeeping purposes.

See our Issue Management Policies for more information.

@ghost ghost closed this as completed Jul 3, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Aug 2, 2020
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. Status: Resolved
Projects
None yet
Development

No branches or pull requests

3 participants