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

Cannot run APIs with different controller names with same ControllerName attribute after migration #1033

Closed
1 task done
anime-shed opened this issue Oct 26, 2023 · 6 comments · Fixed by #1040 or #1042
Closed
1 task done

Comments

@anime-shed
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

I have migrated from
"Microsoft.AspNetCore.Mvc.Versioning" Version="5.1.0"
To
"Asp.Versioning.Mvc" Version="6.4.0"

But after this following has stopped working;

[Route("api/[controller]/[action]")]
[AllowAnonymous]
[ApiController]
[ControllerName("Attributes")]
public class InfoController : ControllerBase
{
    private readonly IEnumerable<EndpointDataSource> _endpointSources;

    public InfoController(IEnumerable<EndpointDataSource> endpointSources)
    {
        _endpointSources = endpointSources;
    }

    [HttpGet]
    public async Task<ActionResult> ListAllEndpoints()
    {
        var endpoints = _endpointSources
            .SelectMany(es => es.Endpoints)
            .OfType<RouteEndpoint>();
        var output = endpoints.Select(
            e =>
            {
                var controller = e.Metadata
                    .OfType<ControllerActionDescriptor>()
                    .FirstOrDefault();
                var action = controller != null
                    ? $"{controller.ControllerName}.{controller.ActionName}"
                    : null;
                var controllerMethod = controller != null
                    ? $"{controller.ControllerTypeInfo.FullName}:{controller.MethodInfo.Name}"
                    : null;
                return new
                {
                    Method = e.Metadata.OfType<HttpMethodMetadata>().FirstOrDefault()?.HttpMethods?[0],
                    Route = $"/{e.RoutePattern.RawText.TrimStart('/')}",
                    Action = action,
                    ControllerMethod = controllerMethod
                };
            }
                                     );

        return Ok(output);
    }
}

In the above:
image
image

Where As:
image
image

Expected Behavior

This should be working since I tried to follow guide.

Steps To Reproduce

No response

Exceptions (if any)

No response

.NET Version

8.0.100-rc.2.23502.2

Anything else?

ASP.NET Core : 6.0
Visual Studio: Microsoft Visual Studio Enterprise 2022 (64-bit) - Preview Version 17.8.0 Preview 4.0

@anime-shed
Copy link
Author

Have used this in the Startup.cs:

  services.AddApiVersioning(config =>
  {
      // if we don't specify an api version, assume the default version is 1.0
      config.DefaultApiVersion = new ApiVersion(1, 0);
      // if api version is not specified assume default version to be the one we specified earlier
      config.AssumeDefaultVersionWhenUnspecified = true;
      // so that we can get an api version information in the responses
      config.ReportApiVersions = true;
  });

@commonsensesoftware
Copy link
Collaborator

It appears you are missing the call to AddMvc(). With the introduction of Minimal APIs, AddApiVersioning() only provides support for bare metal by default now. AddMvc() will add the controller support. This is dependent upon MVC Core (as it always has been and not the full MVC stack (as some people have thought).

Revised Startup.cs:

services.AddApiVersioning(config =>
  {
      // if we don't specify an api version, assume the default version is 1.0
      config.DefaultApiVersion = new ApiVersion(1, 0);
      // if api version is not specified assume default version to be the one we specified earlier
      config.AssumeDefaultVersionWhenUnspecified = true;
      // so that we can get an api version information in the responses
      config.ReportApiVersions = true;
  }).AddMvc(); // ← add MVC Core to support controllers

IApiVersioningBuilder.AddMvc (API Versioning) should not be confused with IServiceCollection.AddMvc. (ASP.NET Core). They do different things.

@anime-shed
Copy link
Author

anime-shed commented Oct 27, 2023

I did add that was advised, but this is still not working for the ControllerName attribute that I have already had, in the above example:
http://localhost:44362/api/Attributes/ListAllEndpoints
image
image

But it does seem to ignore the number in the controller name if present.

@commonsensesoftware
Copy link
Collaborator

Thanks for all of the details. I've been able to repro your scenario, which was a bit surprising. I thought maybe I somehow introduced a "new" bug, but nothing has changed in this area in a long, long time; not even from 5.1.0 to 6.0.0.

I was able to figure out why this is happening. The ControllerNameAttribute just explicitly sets the "controller" route parameter value. At some point in time, this definitely worked as expected. I've tried to track down when things might have gone sideways. I've tried against both the new and old library builds, but I get the same results.

Poking around in the ASP.NET Core source, I see this addition from 5 years ago:

https://github.com/dotnet/aspnetcore/blob/c50ed73c7555d7a4895782a567b64e625f49cf09/src/Mvc/Mvc.Core/src/ApplicationModels/ApplicationModelFactory.cs#L131

This code uses the ControllerActionDescriptor.ControllerName as the value for the "controller" route parameter and does not allow it to be overridden. As a result, ControllerNameAttribute ends up having no effect.

The ControllerNameAttribute was originally meant to solve problems related to convention-based routing; particularly in OData. Attribute routing doesn't have that problem because you explicitly define the route template. I did find some OData code in the old library that would have leveraged ControllerNameAttribute correctly a la:

if ( controller.RouteValues.TryGetValue( "controller", out var name ) )
{
    controller.ControllerName = name;
}

It would seem the correct path is to add this logic back into the place where IControllerNameConvention.NormalizeName is used. This is the place where Info2 would become Info in your example. If you specify ControllerNameAttribute, then that value should be used verbatim without any normalization. In a simple test run, things indeed work as expected.

The crazy part is that this investigation seems to suggest that things have been broken for 5+ years. 😬 The reason it hasn't been detected is likely because the test cases use similar names rather than completely different ones; for example, "Info2Controller" -> "Info" (verbatim; not normalized) vs "Info2Controller" -> "Attributes". I do think it should be "Attributes" as you've articulated, but I'm really surprised this issue hasn't come up sooner.

The last part of this mystery is how you got it to work. You said that things were behaving as expected under 5.1.0; however, I was not able to reproduce that behavior. I found that both libraries to have this issue. Do you have the world's simplest repro that shows your implementation working? Before I commit to the fix/change, I'd like to make sure I haven't missed something else. If you had used [ControllerName("Info")] before and decided to change it to [ControllerName("Attributes")], then no repro should be needed; that would explain the discrepancy.

@anime-shed
Copy link
Author

Since I have been using this in a private repo, I may not be able to give too many details, but I do have a scenario where something similar used to work:

These are the two type of implementations that I had:

  • A completely different name for Controller than the controller name:
namespace SomethingApi.Controllers.Console.DeveloperCommands;

[Route("api/v{version:apiVersion}/[controller]/[action]")]
[ApiController]
[ControllerName("console")]
[ApiVersion("1.0")]
[Authorize]
public class GenerateOTP : ControllerBase
{
    private readonly OTPService otpService;

    public GenerateOTP(OTPService otpService)
    {
        this.otpService = otpService;
    }

    [HttpGet]
    public async Task<IActionResult> SendOTP()
    {
        var response = await otpService.SendOtp();
        if (response.IsSuccess)
            return Ok(response);
        return BadRequest(response);
    }
}
  • Having two Controller with same name in a single namespace:
namespace SomethingApi.Controllers.v1;

[Route("api/v{version:apiVersion}/[controller]/[action]")]
[ApiController]
[Authorize]
[ControllerName("Attribute")]
[ApiVersion("2.0")]
public class Attribute2Controller : ControllerBase
{
    private readonly AttributeService attributeService;

    public Attribute2Controller(AttributeService attributeService)
    {
        this.attributeService = AttributeService;
    }

    [HttpGet]
    public async Task<IActionResult> SearchAttributes([FromQuery] AttributeParameters filter)
    {
....
        return Ok(attribute);
    }
}

[Route("api/v{version:apiVersion}/[controller]/[action]")]
[ApiController]
[Authorize]
[ApiVersion("1.0")]
public class AttributeController : ControllerBase
{
 private readonly AttributeService attributeService;

    public Attribute2Controller(AttributeService attributeService)
    {
        this.attributeService = AttributeService;
    }

    [HttpGet]
    public async Task<IActionResult> FindAttributes([FromQuery] AttributeParameters filter)
    {
....
        return Ok(attribute);
    }
}

I am also attaching the packages that I had installed when things were working for me:
image

I hope this will help you find anything else.

@anime-shed
Copy link
Author

I did check Just now:

  • The first Implementation did not work previously either. I don't know when this got broken since I was not on the consumption end of the API.
    api/v1.0/GenerateOTP/SendOtp
  • But the second implementation was working fine since that was something I tested personally.
    api/v2.0/Attribute/SearchAttributes
    api/v1.0/Attribute/FindAttributes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants