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

Bug when the same Controller name #630

Closed
spatxos opened this issue May 8, 2020 · 8 comments
Closed

Bug when the same Controller name #630

spatxos opened this issue May 8, 2020 · 8 comments

Comments

@spatxos
Copy link

spatxos commented May 8, 2020

There is a project :Test.ApiVersioning.

When I request the following URLs separately, the following result appears:

namespace Test.ApiVersioning.Controllers.v1
{
    [ApiVersion("1.0")]
    [Route("[controller]/[action]")]
    [ApiController]
    public class HomeController : Controller
    {
         public JsonResult GetJson()
        {
            return Json(System.Reflection.MethodBase.GetCurrentMethod().DeclaringType.Namespace);
        }
    }
}
namespace Test.ApiVersioning.Controllers.v2
{
          [ApiVersion("2.0")]
          [Route("[controller]/[action]")]
          [ApiController]
          public class HomeController : Controller
         {
                 public JsonResult GetJson()
                 {
                         return Json(System.Reflection.MethodBase.GetCurrentMethod().DeclaringType.Namespace);
                  }
          }
}

image

namespace Test.ApiVersioning.Areas.User.Controllers.v1
{
    [Area("User")]
    [ApiVersion("1.0")]
    [Route("[area]/[controller]/[action]")]
    [ApiController]
    public class TestController : Controller
    {
        private readonly ILogger<HomeController> _logger;

        public TestController(ILogger<HomeController> logger)
        {
            _logger = logger;
        }

        public JsonResult GetJson()
        {
            return Json(System.Reflection.MethodBase.GetCurrentMethod().DeclaringType.Namespace);
        }

    }
}

image

namespace Test.ApiVersioning.Areas.User.Controllers.v1
{
    [Area("User")]
    [ApiVersion("1.0")]
    [Route("[area]/[controller]/[action]")]
    [ApiController]
    public class HomeController : Controller
    {
        private readonly ILogger<HomeController> _logger;

        public HomeController(ILogger<HomeController> logger)
        {
            _logger = logger;
        }

        public JsonResult GetJson()
        {
            return Json(System.Reflection.MethodBase.GetCurrentMethod().DeclaringType.Namespace);
        }

    }
}

image

When the same Controller Name exists, version control seems to add version information that should not be added to other same Controllers, resulting in the inability to normally access the logically accessible Controller.

Startup.ConfigureServices:
image

image

emmmm....

It seems that Api.Versioning controls the available versions only based on the name of the Controller. When I add a v3 HomeComtroller in the User directory, the https://localhost:44311/home/getjson is not accessible. The api-supported-versions are 1.0, 2.0, 3.0

@commonsensesoftware
Copy link
Collaborator

I've heard about people trying to version their UIs, but I've never seen it. It seems strange to me, but I'm sure you have your reasons.

API versioning doesn't care about controller names. In fact, it doesn't really care all that much about controllers. Grouping and matching is done at the action level as defined by it's route template. The thing that is versioned is the resource behind a path, not the controller (though it may seem that way to the implementer).

The failing request is for users/home/getjson?api-version=2.0, but that is the one controller that you have not shown yet. Having a look at that might help explain why it's not matching. You didn't indicate or show the 400 error response, but I presume it says that it couldn't match this route.

@spatxos
Copy link
Author

spatxos commented May 9, 2020

Thank you, but according to
https://github.com/microsoft/aspnet-api-versioning/wiki/API-Version-Selector#current-implementation-api-selector

 o.ApiVersionSelector = new CurrentImplementationApiVersionSelector(o);

According to the described logic, when the version property of my Test.ApiVersioning.Areas.User.Controllers.HomeController is set to 1.0, when I do n’t consider other Controllers, I will access 1.0 when I visit /home/getjson/ Version, and api-supported-version: 1.0.

But because I added Test.ApiVersioning.Controllers.HomeController and specified version 1.0 and 2.0, the api-supported-version of Test.ApiVersioning.Areas.User.Controllers.HomeController will be changed to 1.0,2.0, So I think the problem should be caused by the same Controller name

Maybe it's the problem I described, I should change it. This problem will occur when the name of the Controller under my area is the same as the name of the Controller in the root directory, and apiversion is not passed, but only according to CurrentImplementationApiVersionSelector to let mvc judge the version by itself

@commonsensesoftware
Copy link
Collaborator

The controllers you mentioned have 2 different routes:

  • /home/getjson (versions: 1.0, 2.0)
  • /user/home/getson (versions: 1.0)

Regardless of the API version value, API versioning sees these as different routes and APIs. In a RESTful application, the URL (path) is the resource identifier; therefore, the resources and API versions are expected to be potentially different.

The CurrentImplementationApiVersionSelector selects the current (e.g. maximum) API version for a given route/API.

From the configuration I can see, I expect the following:

Request 1

GET home/getjson HTTP/1.1
Host: localhost
HTTP/1.1 200 OK
api-supported-versions: 1.0, 2.0
content-type: application/json
content-length: 35

"Test.ApiVersioning.Controllers.v2"

Request 2

GET user/home/getjson HTTP/1.1
Host: localhost
HTTP/1.1 200 OK
api-supported-versions: 1.0
content-type: application/json
content-length: 46

"Test.ApiVersioning.Areas.User.Controllers.v2"

API Versioning doesn't care about folder names or how you organize your code. It only cares about routes, which map to actions, and may take some shared data from controllers. The mapping is pre-computed at startup.

@spatxos
Copy link
Author

spatxos commented May 10, 2020

In fact, the folder "Test.ApiVersioning.Areas.User.Controllers.v2" is an empty folder. I only added the v1 folder and the corresponding Controller.

"API Versioning doesn't care about folder names or how you organize your code. It only cares about routes, which map to actions, and may take some shared data from controllers. The mapping is pre-computed at startup."

According to the understanding of this sentence, I can think that Request 2 is established, because Request 1 and Request 2 are different routes ( v1 and v2 folders are just for differentiation, and have no practical purpose, please do not entangle here )

Request 2

GET user/home/getjson HTTP/1.1
Host: localhost
HTTP/1.1 200 OK
api-supported-versions: 1.0
content-type: application/json
content-length: 46

"Test.ApiVersioning.Areas.User.Controllers.v2"

But in the actual test, I get the following result

Request 1

GET home/getjson HTTP/2
Host: localhost
HTTP/2 200 OK
api-supported-versions: 1.0, 2.0
content-type: application/json
content-length: 35

"Test.ApiVersioning.Controllers.v2"

Request 2

GET user/home/getjson HTTP/2
Host: localhost
HTTP/2 200 OK
api-supported-versions: 1.0, 2.0
content-type: application/json
content-length: 181

"{"error":{"code":"UnsupportedApiVersion","message":"The HTTP resource that matches the request URI 'https://localhost:44392/user/home/getjson' is not supported.","innerError":null}}"

Request 3

GET user/test/getjson HTTP/2
Host: localhost
HTTP/2 200 OK
api-supported-versions: 1.0
content-type: application/json
content-length: 46

"Test.ApiVersioning.Areas.User.Controllers.v1"

Using Request 1 and Request 3 for comparison, it can be seen that API Versioning is related to routing, and the returned api-supported-versions are normal. But when compared with Request 2, you will find that the duplicate name causes problems in the results of api-supported-versions.

I added the v3 directory under /User/Controller in the code, you can exclude it and then test.

@spatxos
Copy link
Author

spatxos commented May 12, 2020

Hello, I added support for Area in https://github.com/wangpengzong/aspnet-api-versioning.

In https://github.com/microsoft/aspnet-api-versioning/blob/master/src/Microsoft.AspNetCore.Mvc.Versioning/Versioning/ApiVersionCollator.cs line 102

        IEnumerable<IEnumerable<ActionDescriptor>> GroupActionsByController( IEnumerable<ActionDescriptor> actions )
        {
            var groups = new Dictionary<string, List<ActionDescriptor>>( StringComparer.OrdinalIgnoreCase );

            foreach ( var action in actions )
            {
102                var key = GetControllerName( action );
            ...
       }
        protected virtual string GetControllerName( ActionDescriptor action )
        {
            if ( action == null )
            {
                throw new ArgumentNullException( nameof( action ) );
            }

            if ( !action.RouteValues.TryGetValue( "controller", out var key ) )
            {
                if ( action is ControllerActionDescriptor controllerAction )
                {
                    key = controllerAction.ControllerName;
                }
            }

            return TrimTrailingNumbers( key );
        }

The GetControllerName method does not judge the area, which causes the homecontroller under the user route to add invalid ImplementedApiVersions and SupportedApiVersions in the action.SetProperty of the OnProvidersExecuted method.

At the same time I added the AutoVersioningAndSupportAreaSample example for display

@commonsensesoftware
Copy link
Collaborator

Ah ... good find. Been quite some time since I looked at this. Now, I remember why this is. Given the set of action descriptors and their routes, the only thing to really key off of is the controller name, which can be extracted from the route parameter or fall back to the descriptor's controller name.

You certainly have a unique setup. I haven't seen any versioning attempts with a UI in a while and never using areas too. I'm not yet convinced this is a behavior that should be intrinsically baked in. I find two different logical resources that have the same name to be strange. I see how it can happen for "Home", but not for others. OData users run into similar problems trying to use different route prefixes. I've seen somewhat similar setups solved by using nested applications.

Grouping by route template would be more accurate, but it comes with its own problems. For example, order/{id} and order/{id:int} are semantically equivalent. This type of matching would require manipulation of the original template. It gets a lot more complex when considering the, now legacy, convention-based routing. An appropriate template would have to be constructed before it can be evaluated.

The best solution for you is to extend and change the ApiVersionCollator as you've done. You can then replace the default implementation in the service collection which is registered here any time after AddApiVersioning() is called.

If a compelling argument can be made that this scenario is somehow common, I'd entertain the idea of an enhancement. I need to be conservative in adding such things because it can easily turn into a slippery slope of special cases. I also have to consider down-level support in Web API, which is still heavily used. For the moment, things are behaving exactly as expected.

If you need assistance adjusting your setup to replace the default collator, feel free to ask. It seems like you're on the path to a working solution.

Thanks

@spatxos
Copy link
Author

spatxos commented May 13, 2020

Yes, this question is not a universal one, but the main point is whether to distinguish between area and non-area controllers. I will consider expanding or changing ApiVersionCollator after AddApiVersioning (), so that the code changes are minimal

thank you very much!

@spatxos spatxos closed this as completed May 13, 2020
@commonsensesoftware
Copy link
Collaborator

No problem. Good discussion. This may be useful to someone else. ;)

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

No branches or pull requests

2 participants