Regression on default routing

[vbray1979]

Hi,

The default built in route "Products/NS.Book" targeting "GetFromBook" trigger a 404 Not Found.
Working fine in 4.1.1

Vincent

[commonsensesoftware]

I'm afraid your issue will require more information. This appears to be OData, but on which platform and version? There are no "default, built-in routes". Is that some OData example you are copying from or something else? A repro is always best, but the controller and application setup would be helpful.

[vbray1979]

Built in route conventions are coming from https://docs.microsoft.com/en-us/odata/webapi/built-in-routing-conventions
Version used:

• Microsoft.AspNetCore.OData: 7.5.7
• Microsoft.AspNetCore.OData.Versioning: 5.0.0
• Microsoft.AspNetCore.OData.Versioning.ApiExplorer: 5.0.0

While

• Microsoft.AspNetCore.OData.Versioning: 4.1.1
• Microsoft.AspNetCore.OData.Versioning.ApiExplorer: 4.1.1

allow accessing the controller below using https://localhost:44338/v1/ThirdPartyBases/AMI.ThirdPartyModel the version 5.0.0 trigger a 404 not found

[ODataRoute]
[Produces("application/json")]
[ProducesResponseType(typeof(ODataValue<IEnumerable<ThirdPartyModel>>), Status200OK)]
[ProducesResponseType(Status404NotFound)]
[EnableQuery(AllowedQueryOptions = All, MaxTop = 100)]
public IQueryable<ThirdPartyModel> GetFromThirdPartyModel()
{
     var thirdPartyModels = _Query.ThirdPartiesQueryProcessor.Get();
     return thirdPartyModels;
}

[commonsensesoftware]

Slightly insightful. So you intend to use the default OData conventions, but you are you using OData attributes (ex: [ODataRoute]). I strongly recommend you choose one or the other. While it might work, it's very confusing to mix routing methods. When things do (or don't) work, it's not obvious why.

This looks like the action maps to an OData function. It's hard to say without the controller definition, but this might be related to the naming of the controller. There was an optimization made that can cause this behavior. I would expect the controller to look something like:

[ApiVersion("1.0")]
[ODataRoutePrefix("ThirdPartyBases")]
public class ThirdPartyBasesController : ODataController
{
    [ODataRoute("ThirdPartyModel")]  // ← attribute routing supersedes convention-based routing
    [Produces("application/json")]
    [ProducesResponseType(typeof(ODataValue<IEnumerable<ThirdPartyModel>>), Status200OK)]
    [EnableQuery(AllowedQueryOptions = All, MaxTop = 100)]
    public IQueryable<ThirdPartyModel> GetFromThirdPartyModel() => _Query.ThirdPartiesQueryProcessor.Get();
}

OData now supports Endpoint Routing, which is generally the preferred approach, but you are not using that method. You can choose whichever method you prefer, but I recommend sticking to a single lane. The Endpoint Routing approach will allow interleaving partial sets of attributes, but they are the standard routing attributes (e.g. RouteAttribute, HttpGetAttribute, and so on).

Based on your example URL, I presume your setup looks like:

public void Configure( IApplicationBuilder app, VersionedODataModelBuilder modelBuilder )
{
    app.UseRouting();
    app.UseEndpoints(
        endpoints =>
        {
            endpoints.MapControllers();
            endpoints.MapVersionedODataRoute( "odata", "v{version:apiVersion}", modelBuilder );
        } );
}

The basic OData sample demonstrates how to use Endpoint Routing. I hope that helps you identify how your application is different from the expected setup.

[vbray1979]

This is indeed my setup, but I am targeting a cast and not a function (https://localhost:44338/v1/ThirdPartyBases/AMI.ThirdPartyModel)

• Base class: ThirdPartyBase
• Derived: ThirdPartyModel
• NameSpace: AMI

I only find a way to process this request using the Built in route conventions with end point name "GetFromThirdPartyModel". This was working until version 5.0.0. Is there a way to process this request using OData attributes ?

[commonsensesoftware]

I haven't seen this particular issue before. I've never found myself needing this OData capability, but it should work. OData has a history of making underlying changes so it's sometimes hard to tell the source of the issue. If you have a repro, that's always helpful. Do things work correctly if you remove API Versioning? There's a lot of open issues in OData. This link may be related.

The convention of GetFrom<Entity> is a naming convention to match the corresponding method that is only considered when there are no attributes. When there are any attributes, the name of the method is irrelevant.

I guess technically you can all this a cast, but it's not the cast function. This by way of specifying a qualified entity type name. I believe the default configuration now allows unqualified type names. That should mean that ~/ThirdPartyBases/ThirdPartyModel will work. You might also experiment with explicitly enabling or disabling that option in the configuration.

I recommend starting with that and I'll see if I'm able to determine anything else. The investigation queue is a bit backed up.

If I'm not mistaken, the native attribute routing method for OData Endpoint Routing would look like:

[ApiVersion("1.0")]
[Route("v{version:apiVersion}/[controller]")] // ← the prefix here has to match the endpoint registration
public class ThirdPartyBasesController : ODataController
{
    [HttpGet("[action]")]  // I've not tested if OData properly honors this built-in token; it 'should'
    [Produces("application/json")]
    [ProducesResponseType(typeof(ODataValue<IEnumerable<FirstPartyModel>>), Status200OK)]
    [EnableQuery(AllowedQueryOptions = All, MaxTop = 100)]
    public IQueryable<FirstPartyModel> FirstPartyModel() => _Query.FirstPartiesQueryProcessor.Get();

    [HttpGet(nameof(SecondPartyModel))]  // if [action] doesn't work, you can use an explicit name
    [Produces("application/json")]
    [ProducesResponseType(typeof(ODataValue<IEnumerable<SecondPartyModel>>), Status200OK)]
    [EnableQuery(AllowedQueryOptions = All, MaxTop = 100)]
    public IQueryable<SecondPartyModel> SecondPartyModel() => _Query.SecondPartiesQueryProcessor.Get();

    [HttpGet("[action]")]
    [Produces("application/json")]
    [ProducesResponseType(typeof(ODataValue<IEnumerable<ThirdPartyModel>>), Status200OK)]
    [EnableQuery(AllowedQueryOptions = All, MaxTop = 100)]
    public IQueryable<ThirdPartyModel> ThirdPartyModel() => _Query.ThirdPartiesQueryProcessor.Get();
}

Remember that the AMI namespace is part of the EDM, not the route template. OData parses the URL to process the namespace and resolve an OData Path. There is a close relationship between OData Path and route template, but they are not identical. For example, the prefix (e.g. v{version:apiVersion}) is not part of the OData Path.

[vbray1979]

I see that that Microsoft.AspNetCore.OData.Versioning: 5.0.0 and 4.1.1 are tageting the same version of Microsoft.AspNetCore.OData 7.5.7. It is certainly not the root cause.
I tried the proposed controller definition without any sucess. I still have 404 not found. I will provide a repro asap.

[commonsensesoftware]

This appears to be the same issue discovered in #689 and #710. There is bug in matching the long convention form of action names. This should be fixed in the next patch. In the meantime, you should have success using the short convention form - e.g. Get vs GetBook.

[vbray1979]

Hi Chris,

This is a good news. I will be able to upgrade as I was using the previous version for compatibility reason.

[icnocop]

Hi.

I've attached an example project that can reproduce the issue.

Steps to reproduce:

1. Download and unzip OData7WebApplication.zip.
2. Open OData7WebApplication.sln in Visual Studio.
3. Right-click on the OData7WebApplication project and select Set as Startup Project.
4. Set breakpoints in each of the action methods in .\Controllers\ProductsController.cs.
5. Start debugging (ex. IIS Express).
6. Make HTTP requests to each endpoint and notice that each break point is hit as expected. See the sample HTTP requests below.
7. Stop debugging.
8. Right-click on the OData7WebApplicationWithVersioning project and select Set as Startup Project.
9. Set breakpoints in each of the action methods in .\Controllers\ProductsController.cs.
10. Start debugging.
11. Make HTTP requests to each endpoint and notice that the break points in the GetFromBook and GetFromMagazine action methods are not called. Calls to odata/Products(1)/MyNamespace.Book and odata/Products(2)/MyNamespace.Magazine are unexpectedly routed to the Get(int key) action method instead.

I expected the calls to be routed in the same way when versioning is not applied.

I'm hoping for an in-place work-around because I'm unable to upgrade to OData 8 at the moment.
Otherwise, I may try to apply a "hot-fix" somehow.

Thank you!

Sample requests which can be used directly with Visual Studio Code REST Client:

@baseUrl = http://localhost:25601/odata

################
# Get Products #
################

GET {{baseUrl}}/Products

###############
# Get Product #
###############

GET {{baseUrl}}/Products(1)

############
# Get Book #
############

GET {{baseUrl}}/Products(1)/MyNamespace.Book

################
# Get Magazine #
################

GET {{baseUrl}}/Products(2)/MyNamespace.Magazine

[commonsensesoftware]

@icnocop Thanks for the comprehensive repro. I'll give it a gander. This issue might be resolved in the 5.1 build. I'm still not able to publish due to code signing, but I'm due for a follow up by 6/24 😞. If it's not fixed, then I'll want this issue to be resolved in the 5.1 release.

I don't intend to continue servicing OData 7.x long-term so I'd want it to be fixed. OData 7.x is a 🐶 and 🐴 show that still uses the legacy routing approach. OData 8.0 is yet another mulligan and I presume there is something unsupported or otherwise broke you in the reimplementation (I know the feeling). The routing has at least improved in OData 8.0.

[icnocop]

Hi @commonsensesoftware

I was able to modify .\src\Common.OData.ApiExplorer\AspNet.OData\Routing\ODataRouteBuilderContext.cs and resolve the issue for this specific scenario, an additional "GET ~/entityset/key/cast" entity route convention.

The commit can be found here and based on the odata-v5.0.0 tag.

Since aspnet-api-versioning no longer supports OData 7.x, I can't migrate to OData v8 yet, and the code fix cannot be applied in any of the aspnet-api-versioning hooks, I'll have to maintain a copy of the code in a separate repository.

I was able to verify the code works as expected after migrating to OData v8 and using Asp.Versioning.OData.ApiExplorer 6.0.0-preview.3.

Thank you.

[commonsensesoftware]

Coincidence... The Universe was listening. I'm literally debugging this very line of code and just figured it out it's in this spot. Effectively, the entity set is not being resolved. You saved me some time. I'll look at backporting your solution into the 5.1 unless you want to submit a PR (happy to give credit where due). I've haven't fully gone through your solution, but I'm sure it's on the right track.

There's not a whole lot I can do or control with respect to OData's versioning strategy and policies. Every major version seems to introduce significant, breaking changes. While a major version should be able to do that by definition, it seems to almost always be the case with OData since 5.x and more often than most other cases. ☹️

On the positive side, OData 8.x should solve much of the routing template definition and matching process forevermore 🤞🏽. I'll double-check, but I'm highly confident that this issue also exists in ASP.NET Web API, even though no one has reported. There aren't as many as there once was, but there are still quite of few people using OData with Web API. Those users can benefit from this fix as well. 😄

[icnocop]

Would you like me to submit a PR against the ms branch?

Will 5.1 support .netcoreapp3.1 and OData v7.x?

Thank you.

[commonsensesoftware]

The ms branch is still netcoreapp3.1 and OData 7.x; the same as 5.0. The 5.1 release will contain all the backlog of fixes including this one. I'll continue supporting servicing on it for a bit, but I don't plan to add or backport anything that isn't absolutely necessary.

No matter what the official documentation, SO, or any source says, only the code tells the truth (as mentioned earlier). Looking at the source it seems the convention for a cast is any of the following:

• {http method} {entity set type} From {entity type} (ex: GetProductFromBook)
• {http method} From entityType.Name (ex: GetFromBook)

It also seems that GET or POST are a valid HTTP method.

[commonsensesoftware]

@icnocop I incorporated your implementation with a few modifications. It solved one problem, but now I'm seeing ambiguous matches between the cast and normal GET operations. This is probably due to a flaw in ODataApiVersionActionSelector.cs which is a fork of the original implementation (because it wasn't extensible). I'm not sure if you've already made other changes to address that. I'd be curious what is different between our implementations. That might need to be included as well.

Calling it a night, but I'll pick this back up ASAP. If you didn't change the action selector, I'm not sure what else I missed. For your edification, I came up with this:

var entity = entitySet.EntityType();

const string ActionMethod = "Post";
const string FunctionMethod = "Get";

if ( ( FunctionMethod.Equals( method, OrdinalIgnoreCase ) ||
        ActionMethod.Equals( method, OrdinalIgnoreCase ) ) &&
        actionName != ActionMethod )
{
    foreach ( var derivedType in model.FindAllDerivedTypes( entity ).OfType<EdmEntityType>() )
    {
        var fromTypeName = "From" + derivedType.Name;

        if ( actionName.StartsWith( method + fromTypeName, OrdinalIgnoreCase ) ||
             actionName.StartsWith( method + entity.Name + fromTypeName, OrdinalIgnoreCase ) )
        {
            return true;
        }
    }
}

And also:

var httpMethods = GetHttpMethods( action );

if ( IsCast( EdmModel, EntitySet, action.ActionName, httpMethods ) )
{
    return ODataRouteActionType.EntitySet;
}
else if ( IsActionOrFunction( EntitySet, Singleton, action.ActionName, httpMethods ) )

The HTTP methods need only be resolved once.

If all that works for you and passes the test suite, I think it's the correct thing for a PR; otherwise, we can continue hashing out. I'll be sure to give you credit for the find and solution, regardless of where we land. Thanks.

[icnocop]

Thank you for the clarifications.

For the ~/entityset/key/cast route as indicated here, the code seems to use the convention GetBook or Get as indicated here.

This is different than the ~/entityset/cast route which uses the convention GetProductsFromBook or GetFromBook as indicated in the source you referenced.

After I renamed the methods in my example this way, they started to work correctly without requiring changes to the action selector:

        // GET /odata/Products(1)/MyNamespace.Book
        [HttpGet]
        [ODataRoute( "({key})/MyNamespace.Book" )]
        public Book GetBook(int key)
        {
            return Products.OfType<Book>().Single(x => x.Id == key);
        }

        // GET /odata/Products(1)/MyNamespace.Magazine
        [HttpGet]
        [ODataRoute( "({key})/MyNamespace.Magazine" )]
        public Magazine GetMagazine(int key)
        {
            return Products.OfType<Magazine>().Single(x => x.Id == key);
        }

Thank you.