ODataRoutingMatcherPolicy Does Not Consider All Routing Metadata

[commonsensesoftware]

Assemblies affected ASP.NET Core OData 8.x

Describe the bug Various paths through ODataRoutingApplicationModelProvider can add multiple IODataRoutingMetadata to an Endpoint, but ODataRoutingMatcherPolicy will only ever consider the first one:

https://github.com/OData/AspNetCoreOData/blob/61ae3232d4ad687bc552fc583b32103f1aeff41a/src/Microsoft.AspNetCore.OData/Routing/ODataRoutingMatcherPolicy.cs#L86

This can result in some endpoints to not match when they should. There doesn't appear to be any documentation that indicates this is the expected behavior.

Reproduce steps Consider a versioned API, which defines:

• 1.0
• 2.0
• 3.0
• 0.9 (deprecated)

Each version has its own EDM and ultimately adds 4 ODataRoutingMetadata instances to the Endpoint. The appropriate EDM is matched by the applied ApiVersionAnnotation to the incoming request. Consider the following controller.

[ApiVersionNeutral]
public class FunctionsController : ODataController
{
    [HttpGet( "api/GetSalesTaxRate(PostalCode={postalCode})" )]
    [ProducesResponseType( typeof( double ), 200 )]
    public IActionResult GetSalesTaxRate( int postalCode ) => Ok( 5.6 );
}

A version-neutral controller can match any API version, including none at all. OData, however, must have an EDM. In this scenario, a developer is expected to use the same function definition for each version, but that's their discretion. This configuration yields the following results:

Request	Result
api/GetSalesTaxRate(PostalCode=98052)	200
api/GetSalesTaxRate(PostalCode=98052)?api-version=1.0	200
api/GetSalesTaxRate(PostalCode=98052)?api-version=2.0	404
api/GetSalesTaxRate(PostalCode=98052)?api-version=3.0	404

This happens because once an explicit version is specified, it cannot match up to the correct EDM as only the first set of metadata is considered. This is only one example, but there are any number of other cases where this could happen.

Expected behavior ODataRoutingMatcherPolicy.ApplyAsync should consider all IODataRoutingMetadata before invalidating a candidate.

- IODataRoutingMetadata metadata = candidate.Endpoint.Metadata.OfType<IODataRoutingMetadata>().FirstOrDefault();
+ IODataRoutingMetadata[] metadata = candidate.Endpoint.Metadata.OfType<IODataRoutingMetadata>().ToArray();
- if (metadata == null)
+ if (metadata.Length == 0)
{
    continue;
}

if (odataFeature.Path != null)
{
    // If it's odata endpoint, and we have a path set, let other odata endpoints invalid.
    candidates.SetValidity(i, false);
    continue;
}

- ODataTemplateTranslateContext translatorContext =
-     new ODataTemplateTranslateContext(httpContext, candidate.Endpoint, candidate.Values, metadata.Model);

- ODataPath odataPath = _translator.Translate(metadata.Template, translatorContext);

+ ODataPath odataPath = null;

+ for (var j = 0; odataPath == null && j < metadata.Length; i++)
+ {
+     ODataTemplateTranslateContext translatorContext =
+         new ODataTemplateTranslateContext(httpContext, candidate.Endpoint, candidate.Values, metadata[j].Model);
+     odataPath = _translator.Translate(metadata[j].Template, translatorContext);
+ }

if (odataPath != null)
{
    odataFeature.RoutePrefix = metadata.Prefix;
    odataFeature.Model = metadata.Model;
    odataFeature.Path = odataPath;

    MergeRouteValues(translatorContext.UpdatedValues, candidate.Values);

    // Shall we break the remaining candidates?
    // So far the answer is no. Because we can use this matcher to obsolete the unmatched endpoint.
    // break;
}
else
{
    candidates.SetValidity(i, false);
}

[xuzhg]

@commonsensesoftware Hi, Chris Martinez very glad to see you again!

It seems you are trying our 8.x. I'd like to learn from your perspective on API-versioning. Please don't hesitate to file new issues or discussions for us. Thanks.

[commonsensesoftware]

Hey Sam (@xuzhg)

Indeed. I've had 8.0 working for a while now. Overall, you've done some great work and OData is much easier to integrate now. There are still pain points. This particular case only happens with a pretty specific setup, but it shouldn't happen IMO. 😉 Unless I missed something, if multiple IODataRoutingMetadata are added to the endpoint metadata via the SelectorModel, then they should all be considered before giving up. They can be different, even if they usually aren't.

Happy to spin up meeting some time if you want discuss paint points or limitations that I've hit. The API Versioning support for OData is significantly simpler with a lot less code. The number one issue is still the limitation of one EDM per registration of AddRouteComponents. It was a little more straight forward when it was possible to resolve IEdmModel out of the container, but now I've had to do some sorcery under the covers. It also requires a separate, version-specific AddRouteComponents implementation because that was only way to get in front of and customize what happens. I would have preferred to keep things as natural to OData users as possible. Perhaps it's an area that can be improved upon in OData 9.0 or beyond.