Fix versioning error when using composite key

[mlunelli]

I had an error when try to implement a Get action with a model with composite key, whis is the error:

 at Asp.Versioning.ApiExplorer.ODataApiDescriptionProvider.TryMatchModelVersion(ApiDescription description, IReadOnlyList`1 items, IODataRoutingMetadata& metadata)
   at Asp.Versioning.ApiExplorer.ODataApiDescriptionProvider.OnProvidersExecuted(ApiDescriptionProviderContext context)
   at Microsoft.AspNetCore.Mvc.ApiExplorer.ApiDescriptionGroupCollectionProvider.GetCollection(ActionDescriptorCollection actionDescriptors)
   at Microsoft.AspNetCore.Mvc.ApiExplorer.ApiDescriptionGroupCollectionProvider.get_ApiDescriptionGroups()
   at Swashbuckle.AspNetCore.SwaggerGen.SwaggerGenerator.GetSwaggerDocumentWithoutFilters(String documentName, String host, String basePath)
   at Swashbuckle.AspNetCore.SwaggerGen.SwaggerGenerator.<GetSwaggerAsync>d__6.MoveNext()
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Swashbuckle.AspNetCore.Swagger.SwaggerMiddleware.<Invoke>d__4.MoveNext()
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.GetResult()

I added a new example project ( SomeODataOpenApiExampleCompositeKey ) to reproduce the error.

In the second commit you can find a possible fix to the issue.

Thanks
Michele

[dnfadmin]

All CLA requirements met.

[commonsensesoftware]

As much as I appreciate PRs (because they don't come often), I think there is a misunderstanding of this example project. The SomeODataOpenApiExample is meant to highlight how to use OpenAPI with only some OData. Honestly, I struggled to come up with a good name. The reason that composite keys were not working is because not all of the OData features were on or enabled.

There are some people out there that want to use vanilla endpoints, but still have OData query capabilities (without an EDM, etc). This means there is no base class or it's likely ControllerBase (as opposed to ODataController). That is a supported scenario by the OData libraries, albeit not particularly well-known. There had been a long-time ask to support exploring OData query options for OpenAPI when not using OData. In fact, before it was supported, encountering ODataQueryOptions would explode into 1600+ parameters! The SomeODataOpenApiExample project is meant to show how that needs to be setup.

If you take all of the bits for the Book entity set and add it to the ODataOpenApiExample, things work as expected. You'll see an entry added for api/Books/idFirst={keyidFirst},idSecond={keyidSecond}?api-version=1.0 and making a request works as intended.

I'm happy to continue the discussion, but this PR doesn't appear to fix or enhance anything.

[mlunelli]

Thanks for your quick reply!

In the ODataOpenApiExample if you comment out the line 20 in Program.cs (options.RouteOptions.EnableKeyInParenthesis = false;) you get the error too (with an entity with composite key).

[commonsensesoftware]

Hmm... I tested that out and got a different error. The clue is a NRE that happens due to a null return value here. This should be impossible.

I think this definitely is a 🐞 , but I don't think it's specific to composite keys. That was simply a symptom. My observations were as follows:

Works

options.RouteOptions.EnableKeyInParenthesis = false;
options.RouteOptions.EnableKeyAsSegment = true;

Works

options.RouteOptions.EnableKeyInParenthesis = true;
options.RouteOptions.EnableKeyAsSegment = false;

Fails

// this is also the default and same result as if nothing was set
options.RouteOptions.EnableKeyInParenthesis = true;
options.RouteOptions.EnableKeyAsSegment = true;

I know that OData is adding different SelectorModel instances, when only one is expected. There's even some comments about that in the ASP.NET Core codebase (which simply states that it's never been a problem 😶‍🌫️). This is problematic for API Versioning because the endpoint metadata isn't copied by default. I thought I found all the edge cases and properly copied the metadata over; clearly not.

I think it's probably worth opening a new issue that calls out the bug. I don't have a good description yet. I suspect there are scenarios beyond composites keys that will cause this behavior. The only possible workaround at the moment will be to use either parenthesis or as segment for keys, but not both.

I'll provide some more details once I've had to dig in a little more. Good find. 😃

[commonsensesoftware]

This was a great find and discussion. Alas, this PR doesn't address the crux of the problem; it's a bit broader. I tracked this with a bit more detail in #891. I've put a new PR which will address this issue and be published with 6.2. Thank you for your help and contribution.