Versioned OData attribute routing not working on ASP.NET Core 3.1 and ASP.NET Core 5.0 (with or without Endpoint routing enabled)

[deepakku-work]

Hi,

I'm using Microsoft.AspNetCore.OData.Versioning version 5.0.0.

I just created a sample WeatherForecastsController project for ASP.NET Core 5.0, added the recommended startup settings (along with the ones for versioning the API), decorated the controller and the action methods with the OData routing attributes but it's not working as expected. Note: This used to work for me in ASP.NET Core 2.2 with the API versioning. I tried with Endpoint routing enabled as well as disabled with no luck. I don't know what changed or if I'm doing anything wrong.

I have 3 methods defined in the controller:

• CreateWeatherForecast (POST)
• GetWeatherForecasts (essentially a "List")
• GetWeatherForecast (GET by ID)

As you can see from the REST requests below, it's not honoring the OData routes and seems to be only going by convention:

GET /v1 HTTP/1.1
HTTP/1.1 200 OK
{
	"@odata.context": "https://localhost:5001/v1/$metadata",
	"value": [
		{
			"name": "WeatherForecasts",
			"kind": "EntitySet",
			"url": "WeatherForecasts"
		}
	]
}

GET https://localhost:5001/v1/weatherforecasts/1ad7afe0-49df-48f4-be0c-d9c1452865a4 HTTP/1.1
HTTP/1.1 200 OK
{
	"@odata.context": "https://localhost:5001/v1/$metadata#WeatherForecasts/$entity",
	"id": "1ad7afe0-49df-48f4-be0c-d9c1452865a4",
	"date": "2020-12-18T19:31:12.5176592-08:00",
	"temperatureC": -15,
	"summary": "Cool"
}

GET https://localhost:5001/v1/weatherforecasts HTTP/1.1
HTTP/1.1 404 Not Found
Content-Length: 0

When I rename the List method (GetWeatherForecasts) to just "Get", the List call starts working again:

GET /v1/weatherforecasts HTTP/1.1
HTTP/1.1 200 OK
{
    "@odata.context": "https://localhost:5001/v1/$metadata#WeatherForecasts",
    "value": [
    {
	    "id": "80a99a08-fd0b-4b75-8875-c1aa4693fde3",
	    "date": "2020-12-18T19:34:20.3323084-08:00",
	    "temperatureC": 38,
	    "summary": "Sweltering"
    },
    {
	    "id": "3912b265-0ada-4ac9-a32d-af799e7fa4c7",
	    "date": "2020-12-19T19:34:20.3959579-08:00",
	    "temperatureC": 44,
	    "summary": "Hot"
    },
    {
	    "id": "3acc83a1-dafb-4f56-9e44-c6172873a23a",
	    "date": "2020-12-20T19:34:20.3963945-08:00",
	    "temperatureC": 48,
	    "summary": "Cool"
    },
    {
	    "id": "351ef82f-c18f-48b7-b057-dd6f67f940b1",
	    "date": "2020-12-21T19:34:20.3965475-08:00",
	    "temperatureC": 50,
	    "summary": "Sweltering"
    },
    {
	    "id": "5ff56225-4f22-49a6-b00a-9a3277e8195d",
	    "date": "2020-12-22T19:34:20.3966482-08:00",
	    "temperatureC": 43,
	    "summary": "Cool"
    }
    ]
}

Same with POST - when I rename it to PostWeatherForecast or just Post, it works; not otherwise.

The test code is available at: https://github.com/deepakku-work/WeatherForecast. For reference, the relevant details are shared below. Any pointers on what I'm doing wrong / if this is a known issue / pointers on how I can debug this further will be very much appreciated.

Project file:

  <PropertyGroup>
    <TargetFramework>net5.0</TargetFramework>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.AspNetCore.OData" Version="7.5.2" />
    <PackageReference Include="Microsoft.AspNetCore.OData.Versioning" Version="5.0.0" />
    <PackageReference Include="Microsoft.AspNetCore.OData.Versioning.ApiExplorer" Version="5.0.0" />
  </ItemGroup>

Startup.cs:

    public void ConfigureServices(IServiceCollection services)
    {
        // Note: I have tried services.AddMvcCore() with EnableEndpointRouting set to false here as well.
        services.AddControllers();
        services.AddApiVersioning();
        services.AddOData().EnableApiVersioning();
    }

    public void Configure(IApplicationBuilder app, IWebHostEnvironment env, VersionedODataModelBuilder modelBuilder)
    {
        app.UseRouting();

        // Note: I have tried app.UseMvc() here (when I disabled endpoint routing) as well with no luck.
        app.UseEndpoints(endpoints =>
        {
            endpoints.MapControllers();
            // Note: I have tried endpoints.MapODataRoute() here as well and it does not work.
            endpoints.MapVersionedODataRoute("versioned-odata", "v{version:apiVersion}", modelBuilder);
        });
    }

WeatherForecast model:

      public class WeatherForecast
      {
              public string Id { get; set; }
              public DateTime Date { get; set; }
              public int TemperatureC { get; set; }
              public int TemperatureF => 32 + (int)(TemperatureC / 0.5556);
              public string Summary { get; set; }
      }

WeatherForecastODataConfiguration:

    public class WeatherForecastODataConfiguration : IModelConfiguration
    {
        public void Apply(ODataModelBuilder builder, ApiVersion apiVersion, string routePrefix)
        {
            switch (apiVersion.MajorVersion)
            {
                default:
                    ConfigureV1(builder);
                    break;
            }
        }

        private static void ConfigureV1(ODataModelBuilder builder)
        {
            var weatherForecastType = builder.EntitySet<WeatherForecast>("WeatherForecasts").EntityType;
            weatherForecastType.HasKey(r => r.Id);
            weatherForecastType.Property(r => r.Id).IsOptional();
        }
    }

WeatherForecastsController:

    [ApiVersion("1.0")]
    [ODataRoutePrefix("WeatherForecasts")]
    [ApiExplorerSettings(IgnoreApi = false)]
    public class WeatherForecastsController : ODataController
    {
        private static readonly string[] Summaries = new[]
        {
            "Freezing", "Bracing", "Chilly", "Cool", "Mild", "Warm", "Balmy", "Hot", "Sweltering", "Scorching"
        };

        private readonly ILogger<WeatherForecastsController> _logger;

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

        [HttpPost]
        [ODataRoute]
        [ProducesResponseType(typeof(WeatherForecast), StatusCodes.Status200OK)]
        public WeatherForecast CreateWeatherForecast()
        {
            var rng = new Random();
            return new WeatherForecast
            {
                Id = Guid.NewGuid().ToString(),
                Date = DateTime.Now.AddDays(Enumerable.Range(1, 5).First()),
                TemperatureC = rng.Next(-20, 55),
                Summary = Summaries[rng.Next(Summaries.Length)]
            };
        }

        [HttpGet]
        [ODataRoute]
        [ProducesResponseType(typeof(IEnumerable<WeatherForecast>), StatusCodes.Status200OK)]
        public IEnumerable<WeatherForecast> GetWeatherForecasts()
        {
            var rng = new Random();
            return Enumerable.Range(1, 5).Select(index => new WeatherForecast
            {
                Id = Guid.NewGuid().ToString(),
                Date = DateTime.Now.AddDays(index),
                TemperatureC = rng.Next(-20, 55),
                Summary = Summaries[rng.Next(Summaries.Length)]
            });
        }

        [HttpGet]
        [ODataRoute("{id}")]
        [ProducesResponseType(typeof(WeatherForecast), StatusCodes.Status200OK)]
        public WeatherForecast GetWeatherForecast(string id)
        {
            var rng = new Random();
            return new WeatherForecast
            {
                Id = id,
                Date = DateTime.Now.AddDays(Enumerable.Range(1, 5).First()),
                TemperatureC = rng.Next(-20, 55),
                Summary = Summaries[rng.Next(Summaries.Length)]
            };
        }

[anranruye]

Duplicate of #689 and #698. Do not use odata attribute routing with Microsoft.AspNetCore.OData.Versioning 5.0.0, it just behave same as convention routing (and GetWeatherForecasts is not allowed). If you decide to continue to use Microsoft.AspNetCore.OData.Versioning 5.0.0, also look at #695 and #693

[sliekens]

I'm seeing the same behavior on ASP.NET Core 3.1 with Microsoft.AspNetCore.OData.Versioning 5.0.0, worked fine in 4.1.1.

It appears that ODataRouteAttribute is completely ignored in this release. Additionally, Get() and GetWeatherForecasts() mean the same thing in OData conventions, not sure how adding ApiVersion attributes can break the latter.

[commonsensesoftware]

It's taken a while to circle back on this, but I can confirm it's a bug. There is a copy and paste error here. This tests whether a controller action maps to an OData action or function. In other words, Get or Get<EntitySet> can never be confused with an OData function, which must be Get<Name>. The test was incorrectly testing for Post<EntitySet>. This is why Get works, but not Get<EntitySet>.

The reason things likely worked in earlier versions was due to a different bug that didn't have this test. In that issue, it was possible that a function was incorrectly matched for a entity set.

OData has a huge number of combinations and test matrix. Unfortunately, this is further conflated by 3 routing models now! >_< I try to cover as many as I can, but it's a lot of work. OData has historically continued to choose not to adopt the standard attribute routing system (though 8.0 will get much, much closer), so API Versioning creates a ODataTemplate and ODataAttributeRouteInfo that essentially looks like AttributeRouteInfo to the rest of ASP.NET. OData's prefix concept requires API Versioning to fan out a clone of each matching action descriptor for each version and prefix combination as there can be more than one. This further illustrates how OData doesn't really care able the prefix; at least, not by the time it gets to your controller. This bug caused the fanned out results to yield no results, which effectively removed some actions as candidates (yikes!). This would happen regardless of which routing approach you used.

This will be fixed in the next patch. In the meantime, you can work around the by using the short, method-only form. Honestly, I find it not only more succinct, but clearer in meaning. GET, for example, is not a verb, it's a method. HTTP is the API. C#, ASP.NET, etc is just an impedance mismatch facade over it. It helps me to think of it as <response> Http.Get(<request>). Others mileage may vary, but a C# method is definitely not the method or API. 😜

[ntonyho]

@commonsensesoftware we ran into the same. Do you have a timeline on when you would be able to release a quick patch?
We are trying to do ~/entitySet/key/navigationProperty/key which isn't one of the default routing convention. We tried implementing a custom routing convention and it wasn't trivial

P.S. are you still working for Microsoft?

[commonsensesoftware]

@ntonyho

1. Unfortunately, I don't have a timeline. I'm burning the issues down as fast as my 1-man army, no funding, no pay, side job will allow
2. I can, however, push the working branch that will contain the fix(es) if you want a private build
3. The release will include a ton of fixes and should burn down 80%+ of all open issues. I'm down to the last few
4. After 14 glorious years, I've left the company. It's making things difficult to continue on the project despite my attempts to coordinate before and after my departure. If I can't silently get things rolling for the community, I'll yield to scream test so the community's voice is heard. That's the only reason I've made no other public announcement. 😉

[ntonyho]

@commonsensesoftware

1. Is there any workaround to get attribute routing to work without the fix?
2. If not, private branch would be nice if you don't mind
3. Is there any pointers to add our own routing convention to handle ~/entitySet/key/navigationProperty/key. We tried adding our own custom convention routing but it says something along the line of version 1.0 isn't supported

[commonsensesoftware]

@ntonyho

1. As previously suggested, using the short names will probably work for you
2. The WIP branch is here
3. Honestly, you don't. This is meant to be solved by $expand. If you really want to go off the rails, I would suggest:
   a. Use a bound or unbound function
   b. Use vanilla routing instead, but with the OData formatters

Getting custom routing conventions to work is a real pain. The built-in conventions are hard enough to deal with. The pain will only get worse if you ever want to support OpenAPI/Swagger.

[ntonyho]

@commonsensesoftware

1. By short name, do you mean to use conventional routing and name my method in a specific way? Or are you referring to something else?
2. What's the $expand equivalent for ~/Persons('id1')/CreditCards('id2') to get a specific credit card for a specific person?

[commonsensesoftware]

@ntonyho

1. Long name = GetWeatherForecast, Short name = Get
2. ~/people/42?$expand=creditcards/2 or something like that. there a number of ways to traverse or project child through expansion

[ntonyho]

@commonsensesoftware

2. Interesting. All of MSGraph is around nested navigation properties with a long keyAsSegment convention which is what I am trying to achieve

[commonsensesoftware]

@ntonyho

1. keyAsSegment is now the default and just means ~/entityset/{key} vs ~/entityset({key})
2. dotnet pack 😉 , but since the versions haven't been bumped and checked in - yet - you probably want something more like dotnet pack --version-suffix beta.1 -p:VersionPrefix=5.1.0;AssemblyVersion=5.1.0.0

[ntonyho]

@commonsensesoftware

I am not sure if it is fixed.

https://github.com/microsoft/aspnet-api-versioning/blob/f9ab22d443d3b0f2f5908cd9a4323ed461027bff/samples/aspnetcore/SwaggerODataSample/V3/OrdersController.cs#L30

I am running the ASP.NET Core SwaggerODataSample
Just change method name on line 30 from Get() to Wet; add back [HttpGet]

        /// <summary>
        /// Retrieves all orders.
        /// </summary>
        /// <returns>All available orders.</returns>
        /// <response code="200">Orders successfully retrieved.</response>
        /// <response code="400">The order is invalid.</response>
        [HttpGet]
        [ODataRoute]
        [Produces( "application/json" )]
        [ProducesResponseType( typeof( ODataValue<IEnumerable<Order>> ), Status200OK )]
        [EnableQuery( MaxTop = 100, AllowedQueryOptions = Select | Top | Skip | Count )]
        public IQueryable<Order> Wet()
        {
            var orders = new[]
            {
                new Order(){ Id = 1, Customer = "John Doe" },
                new Order(){ Id = 2, Customer = "John Doe" },
                new Order(){ Id = 3, Customer = "Jane Doe", EffectiveDate = DateTime.UtcNow.AddDays( 7d ) }
            };

            return orders.AsQueryable();
        }

Swagger wouldn't recognize the method.

http://localhost:59918/api/Orders?api-version=3.0 would give
{"error":{"code":"UnsupportedApiVersion","message":"The HTTP resource that matches the request URI 'http://localhost:59918/api/Orders' does not support the API version '3.0'.","innerError":null}}

[commonsensesoftware]

@ntonyho OData is heavily dependent on specific conventions and provides virtually no support for OpenAPI/Swagger. To achieve any of that functionality I've had to implement my own Attribute Routing, which builds the OData template from attributes or just the type information. Even with the Endpoint Routing support, it's still a 🐶 and 🐴 show under the hood in 7.x because all paths lead back to the original OData IActionSelector implementation. I'd be surprised if this would work in plain old OData, but maybe.

The issue here is that the code can't tell if this action is an entity set operation, a function, or an action. This is internally nasty to check. Without the check, it's possible to incorrectly match an action/function to an entity set operation. Technically, that would be a developer mistake, but there's no way to really distinguish that without strictly enforcing the convention. I manually debugged this path and explicitly made it resolve this to an entity set operation. The action is correctly discovered with the appropriate OData route template, but it didn't match at runtime (e.g. 404 + Unsupported API Version). That could be a different issue, but it's hard to tell at this point.

I'll continue to consider whether this is a problem because I know there are a few other edge case issues in the IActionSelector implementation. This behavior might be related. In general, I would encourage you not to deviate from the expected OData conventions; they're complex and confusing enough as it is. There are now 3 different ways of expressing the route template! 🤦🏽

Speaking of, be careful adding [HttpGet] or any IRouteTemplateProvider. OData 7.x, and more so in 8.0+, will finally align closer to the standard routing system, which heavily relies on IRouteTemplateProvider. Building up OData route templates will use the following rules:

1. If [ODataRoute] is present, use the Template
2. If any IRouteTemplateProvider is present, use the Template
3. Assume the standard, implicit OData conventions

In the specific scenario that you changed, there would be no dire consequences since the route template should be "". That won't always be the case. A found a bunch of cases where this was previously wrong.

[ntonyho]

@commonsensesoftware

Thanks you for repeated quick response. Much appreciated.

To give a bit more context, I am trying to do ~/entitySet/id2/SomeContainedNativationProperty/id2/SomeOtherContainedNavigationProperty/id3
(And potentially even longer route with more levels of navigation properties

While you did mention it could be done via $expand, that isn't what I need. I need to do something similar to many routes in Microsoft Graph. e.g.

GET /me/mailFolders/{id}/messages
GET /users/{id | userPrincipalName}/mailFolders/{id}/messages

As far as I can tell, the out-of-box OData convention routing doesn't support that. Hence, my original attempt in using AttributeRouting (using ODataRoutePrefix + ODataRoute).
If I understand correctly, in Versioning v5, you are simply ignoring ODataRoutePrefix and ODataRoute, and have your own convention?

Slightly off topic, did you take a look at OData v8 RC3? It seems to support some form of versioning, defining multiple EdmModels, and have support for OpenAPI/Swagger (granted, swagger will render all the versions in the same views). Do they finally incorporate your work into OData release or are they doing things very differently?

[commonsensesoftware]

@ntonyho

If you don't want to go the route of $expand, then what you probably want is containment. This can achieve the route templates you're looking for without a custom routing convention:

• Containment in OData v4
• Use OData Containment

Sorry, you have misunderstood. I definite do not ignore any routing configuration a user provides. A long-standing design principle is to not change the functional setup and understanding of routing. The issue is that OData only provides a means of matching an action. It provides no out-of-the-box mechanism to document or otherwise provide route template information. This has to be computed at app start-up time because it's a one-time operation that is read-only in the app's lifecycle. This impacts OpenAPI/Swagger the most, which makes it an unfortunate cost if you're not using it. However, since the route templates are constructed, it makes the matching logic much easier. I suggested that the OData team use a similar approach and I think 8.0 does something similar along those lines. There are 3 ways in which information can be expressed as outlined above. That is the order in which they are processed. The first one specified is the one used. There is no fallback.

I've seen various parts of 8.0, but I haven't looked at it deeply for a few months. I've seen nothing related to versioning. I've seen a would-be opinionated approach by the use of a prefix (e.g. ~/v1); however that is not versioning and is the worst possible way you could version an API (it's not RESTful). The multiple EDMs has always been there. It's still effectively one EDM per OData route configuration. 8.0 is more flexible about how things match up. There's been some level of support for OpenAPI/Swagger for a while (see ODataSwaggerConverter); however, what is actually output is a far cry from what comprehensive OData integration should look like. It doesn't support query options, links, and so on. It also only considers the EDM, when in reality it needs to be cross-referenced with the supported APIs (e.g. services don't have to implement everything an EDM or OData offers). If that were properly supported then OData could be easily integrated into OpenAPI/Swagger generators like Swashbuckle via the API Explorer (which it doesn't).

The OData team has not incorporated any of my work that I'm aware of unless they silently forked something in. I've tried collaborating with them a few times, but it's mostly a one-way conversation. I'm happy to expand the thread or move it over to discussions.

[ntonyho]

This can achieve the route templates you're looking for without a custom routing convention:

(please correct me if i am not getting the terminologies right)
Don't I need attribute routing to do containment?

        [ODataRoute("Customers({CustomerId})/OrdersShipped({OrderId})")]  <-- don't i need this? which is what this issue is about?
        public IHttpActionResult GetSingleOrderShipped(int CustomerId, int OrderId)
        {
            var OrdersShipped = _Customers.Single(a => a.CustomerID == CustomerId).OrdersShipped;
            var OrderShipped = OrdersShipped.Single(pi => pi.OrderID == OrderId);
            return Ok(OrderShipped);
        }