Skip to content

Aspnet core attribute decorator not working after upgrade to v1.1.0 The controller never execute. #146

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

Closed
irowbin opened this issue Jun 16, 2017 · 23 comments

Comments

@irowbin
Copy link

irowbin commented Jun 16, 2017

capture

Works in v1.0.3 but after upgrade, not working. By downgrading to v1.0.3 works well, but v1.1.0 have some issue.

@commonsensesoftware
Copy link
Collaborator

I'm going to go out on a limb and assume you haven't looked at the release notes or the updated wiki. Not to worry, you're not the first. There is a behavioral change for ASP.NET Core that requires this in your setup now:

app.UseMvc();
app.UseApiVersioning();

I'm guessing that you don't have that, but if you add it to your configuration, things will start working again.

Since so many people are getting tripped up by this, I'll investigate whether there is a way to cause a runtime failure if it's missing and publish a patch. Let me know if this is not the problem. Thanks.

@walik92
Copy link

walik92 commented Jun 16, 2017

Irowbin is right, the latest version v1.1.0 not working

@commonsensesoftware
Copy link
Collaborator

I'm going to need more detail than simply "it's not working". Did you update your startup configuration to include app.UseApiVersioning()? What behavior are you seeing? Are you getting 400, 404, 500, or what?

@irowbin
Copy link
Author

irowbin commented Jun 17, 2017

I've added this to the startup class as well.

app.UseMvc();
app.UseApiVersioning();

I followed the release notes before upgrade to v1.1.0; No exceptions, nothing. If i navigate something like
api/v1.0/controller/method where i decorate with attribute, my controller never got hit. The same config except app.UseApiVersioning(); works well under v1.0.3. Thanks.

@irowbin
Copy link
Author

irowbin commented Jun 17, 2017

startup class

public void ConfigureServices(IServiceCollection services)
{
    services.AddApiVersioning(config =>
    {
        config.DefaultApiVersion = new ApiVersion(1, 0);
        config.AssumeDefaultVersionWhenUnspecified = true;
        config.ReportApiVersions = true;
    });
    services.AddMvc();
}
public void Configure(IApplicationBuilder app, IHostingEnvironment env, ILoggerFactory loggerFactory)
{
    app.UseMvc(routes =>
    {
        routes.MapRoute(
            name: "default",
            template: "{controller=Account}/{action=Index}");

        routes.MapSpaFallbackRoute(
           name: "spa-fallback",
           defaults: new { controller = "Home", action = "Index" });
    });
    
    app.UseApiVersioning();
}

controller

[EnableCors("AllowAllOrigins")]
[ApiVersion("1.0")]
[Route("api/v{version:apiVersion}/[controller]/[action]")] // <-- domain.com/api/v1.0/controller/get
[CustomExceptionFilter]
public class SearchController : Controller
{
    [HttpGet]
    public IActionResult Get([FromQuery] SearchDto search)
    {
        var content = _service.Search.Get(search);
        return new ObjectResult(content);
    }   
}

This config works under v1.0.3 except app.UseApiVersioning(); but not to v1.1.0 by including app.UseApiVersioning();

@irowbin irowbin changed the title Aspnet core not working attribute decorator after upgrade to v1.1.0 The controller never executes. Aspnet core not working attribute decorator after upgrade to v1.1.0 The controller never execute. Jun 17, 2017
@commonsensesoftware
Copy link
Collaborator

... and the fog begins to dissipate...

Your application appears to have both UI and APIs. That's is a supported scenario and I've even added acceptance tests for that, but there are still edge case scenarios. It would seem that this is one of them due to the use of the MapSpaFallbackRoute. My understanding of the implementation is that it defines a catch all route token {*clientRoute}.

Let me try to explain why this worked in 1.0, but not 1.1. In 1.0, I was under the impression that the IActionSelector was only called once by the routing infrastructure. In most cases, this was true, but not all. This resulted in some candidates never being considered because the entire selection process short-circuited too early. It would possible to support on matching a result in a single pass, but that would break other features such as reporting all API versions (which are aggregated from candidates) and 400 responses for unsupported API versions. To address this, API versioning needs it's own catch all route of sorts. In fact, that was even a suggestion by the ASP.NET team. I didn't quite like the idea of a special catch all route. Instead, I decided to use a custom router as the catch all, which uses assumed data collected from previous routers and makes a final decision. This is what UseApiVersioning actually does. This hides a bunch of internal mechanics, but does have a dependency on configuration order.

How does all this play into the mix? UseMvc builds an aggregated router for all MVC routes - UI or API. This also includes the support for defining custom user routes, which is where MapSpaFallbackRoute fits in. UseApiVersioning adds a new router to make the decision after this process. Since MapSpaFallbackRoute defines a catch all route token, it short-circuits the pipeline. The API versioning policy never gets a chance to evaluate. This would also explain why you never see the API controller execute.

So how do we address this? I haven't tried it yet myself, but I believe MapWhen will resolve your issue by ensuring the API routes are never considered for SPA controllers. Issue 973 in the JavaScript Services repo describes how to do this. I suspect that it should be a pretty small change for you. If you still can't get it to work, I'm happy to continue sorting it out with you by creating my own repro.

@irowbin
Copy link
Author

irowbin commented Jun 19, 2017

So i how do i configure the SPA routes to include API versioning? I have both mvc view to run SPA app and API to handle data. It would be helpful if any example is provided including MapSpaFallbackRoute with UseApiVersioning Thanks. @commonsensesoftware

@irowbin irowbin changed the title Aspnet core not working attribute decorator after upgrade to v1.1.0 The controller never execute. Aspnet core attribute decorator not working after upgrade to v1.1.0 The controller never execute. Jun 19, 2017
@irowbin
Copy link
Author

irowbin commented Jun 19, 2017

Getting 404 with any request. No exceptions.

    app.UseMvc(routes =>
    {
        routes.MapRoute(
            name: "default",
            template: "{controller=Account}/{action=Index}");
    });
    app.MapWhen(x => !x.Request.Path.Value.StartsWith("/api"), builder =>
    {
        builder.UseMvc(routes =>
        {
            routes.MapSpaFallbackRoute(
                name: "spa-fallback",
                defaults: new { controller = "Home", action = "Index" });
        });
    });
   app.UseApiVersioning();

It means we can't use MapSpaFallbackRoute and app.UseApiVersioning() to the SPA app after v1.1.0 upgrades or there might be a complex configuration then this? I will stay with the v1.0.3 until someone finds the solution. Thanks

@exiled78
Copy link

exiled78 commented Jun 19, 2017

Me too. [Now Resolved]

@commonsensesoftware
Copy link
Collaborator

Thanks for confirming. That's basically what I was looking for. Since that combination still doesn't seem to work, I'll start putting together a repro. If you already have something you can share, that would be great. I'll update the thread once I have more details.

@irowbin
Copy link
Author

irowbin commented Jun 19, 2017

Thanks.. waiting for the solution. 👍

@exiled78
Copy link

@irowbin I resolved my issue by adding app.UseApiVersioning(); to startup#configure and removing some of the custom stuff I had added with https://github.com/rh072005/SwashbuckleAspNetVersioningShim , so I have no outstanding issue.

@commonsensesoftware
Copy link
Collaborator

@exiled78 - Good to hear.

@irowbin - any chance you can help me put together a repro, including the URLs you tested? I tried taking the Basic Sample and just adding the standard MVC convention-based route as well as the MapSpaFallbackRoute extension, but I could not get the scenario to repro. Things continued to work along the API version paths as expected. I tried both setup configurations: one with the MapSpaFallbackRoute inside UseMvc and one inside MapWhen.

If you can help me boil down a repro that contains only the bare bones of your application, it would go miles in ensuring I'm setting up and using the same conditions as you. I know that's sometimes easier said than done. I think the ideal repro would have:

  • A close mirror of your startup configuration
  • A single API controller and action (Values)
  • A single UI controller, action, and view (Home + Index)
  • Anything else you think is relevant

If there is some esoteric configuration combinations that can make things go awry, I want to get to the bottom of it. I'm sure there are others that will have similar problems. It'll be worth improving the integration and/or expanding the wiki.

Thanks

@lzocateli
Copy link

@commonsensesoftware

I'm having a similar error, but my project only has WebApi Core.
I realized that after overloading "Microsoft.AspNetCore.Mvc.Versioning" to version 1.1.0, this method disappeared, and my controllers no longer worked.

image

image

@irowbin
Copy link
Author

irowbin commented Jun 20, 2017

Well, it's strange that only works at once in 100-200 rebuild. No idea why, only execute few methods. No exceptions, nothing. Config with the MapSpaFallbackRoute inside UseMvc.
Did i missed something? @commonsensesoftware
The controller -> https://github.com/irowbin/aspnet-api-versioning/blob/master/Controllers/ValuesController.cs

The startup class -> https://github.com/irowbin/aspnet-api-versioning/blob/master/Startup.cs

@commonsensesoftware
Copy link
Collaborator

@lincolnzocateli Your issue seems to be unrelated. The QueryStringOrHeaderApiVersionReader was a specific one-off implementation of merging API version readers using inheritance. This was refactored starting in the early 1.1 betas to use composition instead so that any set of API version readers can be composed together. This was outlined in the release notes and has also been updated in the wiki. You can simply replace this type with ApiVersionReader.Combine with the API version readers you want to merge. This approach also gives you more control over the order in which API version readers are considered.

@commonsensesoftware
Copy link
Collaborator

@irowbin thanks so much for the repro. It really helped me track down things quickly. There are a couple of odd issues, but I think I have a workable solution.

First, the route template in your ValuesController is wrong on Line 24. It should read [HttpGet( "{id}" )]. You should have been getting AmbigutionActionException, but you weren't due to the real systemic issue.

I'm sure you have your reasons (no need to justify to me), but combining the UI and API services together this way introduces a quite a bit of complexity, particularly with respect to routing. The ordered setup of routing is critical to make things work correctly. In your case, all the SPA stuff must be configured last. In addition to the above route template change, things seemed to all work when I change the setup to be as follows:

public void Configure(IApplicationBuilder app, IHostingEnvironment env, ILoggerFactory loggerFactory)
{
    loggerFactory.AddConsole( Configuration.GetSection( "Logging" ) );
    loggerFactory.AddDebug();

    if ( env.IsDevelopment() )
    {
        app.UseDeveloperExceptionPage();
        app.UseBrowserLink();
    }
    else
    {
        app.UseExceptionHandler( "/Home/Error" );
    }

    app.UseStaticFiles();
    app.UseMvc(routes =>
    {
        routes.MapRoute(
            name: "default",
            template: "{controller=Home}/{action=About}");
    });
    app.UseApiVersioning();
    app.MapWhen( x => !x.Request.Path.Value.StartsWith( "/api" ), builder =>
    {
        builder.UseMvc( routes =>
        {
            routes.MapSpaFallbackRoute(
                name: "spa-fallback",
                defaults: new { controller = "Home", action = "Index" } );
        } );
    } );
}

Try those changes for yourself and let me know. Thanks.

@YogirajA
Copy link

@commonsensesoftware Thanks for looking into this. I ran into the same problem. The weird part was the inconsistent behavior. Some routes worked a couple of times and weren't working after. Those same routes that worked on mine didn't work at all on my colleague's machine.

Is ASPNETCORE discovering routes dynamically? So, in other words, if I go to one pattern of routes, all of my subsequent calls may get affected because of that's one I visited first. For ex: /api/customers/12 worked but /api/customer/general didn't because it thought 'general' was supposed to be an integer id and I was getting model validation error. I resolved that issue by specifying datatype in template as {id:int}.

None of this was a problem before we introduced versioning nuget package. I don't know if you have any insights on that. With the template fixes and downgrading to 1.0.3 , everything seems to be working fine for now.

Let me know if you need more info. BTW, I had exact same code as you pasted above with a difference of app.UseApiVersioning(); was called after app.MapWhen

@commonsensesoftware
Copy link
Collaborator

@YogirajA, app.UseApiVersioning must be called before app.MapWhen. This will go long, but let me try to explain what's happening and the change from 1.0 to 1.1.

In 1.0, the ApiVersionActionSelector assumed that it would only be called once. It receives possible candidates and then selects the best one. If no match is found, an appropriate failure response is returned. It turns out any IActionSelector can be called multiple times. The built-in attribute-based router uses an internal tree model to discover candidates. In certain scenarios, this caused things to return unexpected results. For example, consider the following snippets:

// ~/api/values/42?api-version=1.0
[HttpGet( "{id}" )]
public IActionResult Get( string id ) => Ok( new { id = id } );

// ~/api/values/42?api-version=2.0
[HttpGet( "{id:int}" )]
public IActionResult Get( int id ) => Ok( new { id = id } );

When you send a request to /api/values/42?api-version=2.0, you expect to hit the V2 implementation of the service. Instead what happened is that you would get that the version is not supported. This happens because the internal tree router considers the candidates /api/values/{id} and /api/values/{id:int} to be different, even though they are semantically equivalent. Worse still, these candidates are presented to the action selector during different method invocations. As a result, the ApiVersionActionSelector can no longer make a final decision within SelectBestCandidate because not all of the candidates have been discovered yet. I could have punted and told people not to author their services that way, but I think that would be horrible. A key principle I've tried to hold to is that service authors shouldn't have to learn anything new about routing.

I consulted with the ASP.NET team directly, but there aren't any great answers for the time being. We are negotiating talks to make this better some time in the ASP.NET Core 2.0+ timeframe. In the interim, the recommended solution is to add a catch all route that can make the final decision. I decided to use a catch all router rather than a route to make sure things will work when combined with convention-based routes, which is very common in MVC. The call to app.UseApiVersioning adds this custom router to the pipeline. Starting in 1.1 it is required for things to work.

To understand why some folks are having trouble, let me try to break down the routing process. First, you must understand that routing uses a Chain of Responsibility given a pipeline of routers. The first router that provides a result, ends the pipeline. Let's start by excluding SPA support for a second. This is what the following setup will do:

app.UseMvc();                  // adds tree router for attribute-based routes
app.UseMvcWithDefaultRoute();  // adds a new router with the default MVC convention-based template
app.UseApiVersioning();        // adds a new router that applies the API versioning policy

On the first pass of any request, the ApiVersionActionSelector will always return that there was no match, even if there was. This is the only way to continue the pipeline and consider additional candidates. Once all candidate passes have been exhausted, the API version router will be triggered. At this point, the pipeline is heading for 404 even when there is a possible match. The IApiVersionRoutePolicy is applied in the router, which uses the selection process that was previously in the ApiVersionActionSelector. The final result is as it was before. This is also required to make sure things like reporting API versions to clients still works. That feature has always worked by aggregating the API versions of all candidates at runtime (something else to consider a different design for in 2.0). It's also worth noting that this process could slow down your application significantly. To mitigate that, the best match has results cached after a match. This allows the ApiVersionActionSelector to short-circuit the pipeline for a matching candidate that has already been selected as the best candidate at least once.

When we throw SPA support into the mix, it adds a new catch all route template to the MVC convention-based route table that is designed to be an ultimate fallback. As a result, the order in which it is registered is paramount. When the fallback route is declared before the API versioning router, it is considered first. The ApiVersionActionSelector effectively selects no candidate, which triggers SPA fallback template. The fallback route short-circuits the pipeline, which results in the API controller never being executed. Keep in mind that ASP.NET Core makes no distinction between a UI and API controller.

This where things start to get really weird. Say that you requested /api/v1/values, but the ApiVersionActionSelector says no match. The SPA route template says go ahead and match / instead. Then the IApiVersionRoutePolicy evaluates in the API version router and sees that the candidate / is the best match for originally requested URL /api/v1/values. @_@ It leaves us all scratching our head. Other similar behaviors happen when you shuffle around the configuration order. This is why you were sometimes seeing it work and then not or vice versa. I experienced similar behaviors.

The true configuration fix is to make sure that the SPA fallback is setup after the API versioning router is added. To further ensure that API controller routes aren't even considered, it's wise to use app.MapWhen.

At least from the repro I was given, I was able to make all the routes work consistency as far as I can tell. Apologies for the long response, but hopefully that will help explain why things are behaving the way they are. If anyone on this thread can confirm whether my proposed configuration solves the problem for you, that would be great. I also want to know if it doesn't solve your problem, so I can update the repro with new variations and continue the investigation as needed.

Thanks

@irowbin
Copy link
Author

irowbin commented Jun 21, 2017

This configuration works for me. @commonsensesoftware

app.UseMvc(routes =>
{
    routes.MapRoute(
        name: "default",
        template: "{controller=Home}/{action=account}");
});

app.UseApiVersioning();
app.MapWhen(x => !x.Request.Path.Value.StartsWith("/api"), builder =>
{
    builder.UseMvc(routes =>
    {
        routes.MapSpaFallbackRoute(
            name: "spa-fallback",
            defaults: new { controller = "Home", action = "Index" });
    });
});

The controller

[ApiVersion("1.0")]
[Route("api/v{version:apiVersion}/[controller]/[action]")] // working 
//[Route("api/v{version:apiVersion}/[controller]")] // not working.
[Produces("application/json")]
public class ValuesController : Controller
{
        
    [HttpGet]
    public IEnumerable<string> Get()
    {
        return new string[] { "value1", "value2" };
    }

        
    [HttpGet("{id}")] // must specify the route parameter otherwise AmbiguousActionException will occur. 
    public string Get(int id)
    {
        return "value";
    }
}

There must be a better way to exclude this app.MapWhen(x => !x.Request.Path.Value.StartsWith("/api") line.

@YogirajA
Copy link

@commonsensesoftware Thanks for the very detailed response. I now have a better understanding of why we were running into issues inconsistently. I changed the order per your suggestion with 1.1.0 and it worked. I will run it through a couple of test environments and report back if it breaks. Thanks again!

@commonsensesoftware
Copy link
Collaborator

@irowbin, @YogirajA thanks for verifying. This behavior and setup is unfortunate. The systemic platform limitation has the same characteristics for why 405 is not supported for APIs out-of-the-box. This is something the ASP.NET team has acknowledged and are considering for 2.0. I'm hoping we'll find a solution that supplants the need for app.UseApiVersioning once again and make it hard to mess up the route registration order.

I appreciate the lengthy, but healthy discussion. I'll try to collate all of the talking points here into a FAQ topic for others that may be using a setup similar to yours, in case they also hit this issue. Thanks.

@commonsensesoftware
Copy link
Collaborator

@irowbin , @YogirajA I think you guys are sorted out now. Thanks again for the discussion. There was an issue reported in #148 that might affect you. The fix is in and published. I'll take a stab at adding something about this setup to the wiki. Feel free to come back or reopen this issue if something is unresolved. Thanks.

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

6 participants