Skip to content
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

[ApiExplorerSettings(IgnoreApi = true)] isn't respected with ASP.NET Core applications using OData API Explorer #405

Closed
jimmy3912msncom opened this issue Nov 28, 2018 · 20 comments
Assignees

Comments

@jimmy3912msncom
Copy link

As the title mentions, [ApiExplorerSettings(IgnoreApi = true)] doesn't work for me.

I'm using the ASP.NET Core with OData API Explorer sample from https://github.com/Microsoft/aspnet-api-versioning/tree/master/samples/aspnetcore/SwaggerODataSample and adding the below line above the V1 OrdersController

...
[ApiExplorerSettings(IgnoreApi = true)]
public class OrdersController : ODataController
...

I assume this isn't by design and is something that just isn't supported yet?

@commonsensesoftware
Copy link
Collaborator

Actually, but perhaps to your surprise, this is by design. The ODataController base class already has the implementation:

[ODataFormatting]
[ODataRouting]
[ApiExplorerSettings(IgnoreApi = true)]
public abstract partial class ODataController : ControllerBase
{
  // omitted for brevity
}

This means that if you want to integrate with something like Swagger, all of the OData controllers that derive from ODataController would be ignored. As it would be unusual to not use ODataController as the base class (it's not actually a requirement), there needs to be a way to work around this issue or nothing is discovered by the API Explorer. This behavior is true in ASP.NET Web API and ASP.NET Core.

The root cause for this behavior is that the OData team(s) never provided first-class support for the API Explorer. As a result, API Versioning provides everything from the ground up. A guiding design principle has always been to make API Versioning bolt-on rather than make developers change the way they do things. To stay consistent with the design principles, but still provide this functionality, the OData API Explorer implementations ignore ApiExplorerSettingsAttribute.IgnoreApi property value by default.

Of course if the configured value was always ignored without any ability to change the behavior that would be limiting. If you want to honor the ApiExplorerSettingsAttribute.IgnoreApi, then you need only configure that in the API Explorer options:

services.AddODataApiExplorer( options => options.UseApiExplorerSettings = true );

This is also documented in the wiki topic Use API Explorer Settings.

Note that once you choose to honor the ApiExplorerSettingsAttribute.IgnoreApi, this will result in all other undecorated classes that inherit from ODataController to be ignored as well. To include them, you'll need to do one of following:

  • Decorate your controllers with [ApiExplorerSettings(IgnoreApi = false)] to override the inherited value
  • Create a new base class that includes [ApiExplorerSettings(IgnoreApi = false)] and use that instead of ODataController which isn't that special
    • You could subclass ODataController and just override [ApiExplorerSettings(IgnoreApi = false)], but that would be arguably an abuse of inheritance

I hope that helps.

@jimmy3912msncom
Copy link
Author

@commonsensesoftware Thanks Chris, I guess I never saw that. I tried to enable it but it failed in the sample with The action 'Microsoft.Examples.V1.OrdersController.Get (SwaggerODataSample)' has ApiExplorer enabled, but is using conventional routing. Only actions which use attribute routing support ApiExplorer.

All I did to the sample was:

As you mentioned added the below to Startup:

services.AddODataApiExplorer(
   options =>
   {
      ...
      options.UseApiExplorerSettings = true;
      ...
   } );

And changed the IgnoreApi to false in OrdersController:

...
[ApiExplorerSettings(IgnoreApi = false)]
public class OrdersController : ODataController
...

@commonsensesoftware
Copy link
Collaborator

Ah … yes - this is because the built-in API Explorer for ASP.NET Core only supports attribute routing. I think it's possible to still make the OData bits work by convention. I need to review this some more. Should that be the case, then I'll consider this a bug in the OData API Explorer.

It seems you are using OData conventions only. I'm pretty sure if you use OData attribute routing, you can workaround this issue. I'm sure that's not the answer you're looking for, but it may be a workable solution to unblock yourself while I investigate this. Thanks.

@jimmy3912msncom
Copy link
Author

@commonsensesoftware Correct me if I'm wrong but to my understanding the sample was using attribute based routing:

    ...
    [ApiVersion( "1.0" )]
    [ApiVersion( "0.9", Deprecated = true )]
    [ODataRoutePrefix( "Orders" )]
    public class OrdersController : ODataController
    {
        ...
        [Produces( "application/json" )]
        [ProducesResponseType( typeof( Order ), Status200OK )]
        [ProducesResponseType( Status404NotFound )]
        [ODataRoute( "({key})" )]
        public IActionResult Get( int key ) => Ok( new Order() { Id = key, Customer = "John Doe" } );
        ...

@commonsensesoftware
Copy link
Collaborator

In reviewing the sample, it actually has support for OData attribute and convention-based routing. I suspect that this may be a sequencing problem in the order the providers are configured. If you're curious, this is the place in ASP.NET Core that doesn't allow the API Explorer configuration if the attribute information is not available. I had to create a custom OData flavor of the route information parsed from the OData template. This is almost certainly a bug, but I want to verify first.

There are two interim workarounds (by leaving options.UseApiExplorerSettings = false):

  • Create and register a custom IApiDescriptionProvider which filters out the actions you don't want
  • Create a custom IOperationFilter to exclude operations you don't want in the Swagger document

Either approach will prevent the actions you don't want to discover from being present in the Swagger document. This is obviously not ideal, but it should work while I look into this futher.

@jimmy3912msncom
Copy link
Author

Thanks Chris. I am already doing the latter with a custom filter but was hoping to use something out of box :)

@commonsensesoftware
Copy link
Collaborator

Good to know. It definitely should be working that way out-of-the-box, but I want to make sure you're unblocked first.

@commonsensesoftware
Copy link
Collaborator

I've dug into this a bit more. This is definitely a bug. The catch is figuring out how to workaround the built-in semantics. Technically, an API Explorer for OData at all is unsupported. :( It would appear that I have to roll my own OData-specific attribute for this.

I've managed to convince ASP.NET Core that the OData actions are attribute-routed, even if they're not. Unfortunately, this only overcomes one hurdle and results in different failures further in the pipeline.

I'll chat with the ASP.NET team, but even if I managed to convince them that the restrictions should be relaxed, it probably won't change any time soon. Furthermore, I'd probably be the one having to do the pull request. Another caveat is that change would likely pull you (and anyone else) to a newer version of the API Explorer library (2.2+). I'm not sure how the ASP.NET team hands servicing on older versions. That may not necessarily be an issue, but it's something to be cognizant of.

Before I commit to introducing a new attribute, I'm going to research this some more and see if I can't work past the built-in limitations. Clearly, I can easily introduce a new attribute, but then OData developers have to learn a new way of including or ignoring APIs, which is against the fundamental design principles of this project. I don't want you to have to learn new things about how routing or ASP.NET works unless it's absolutely necessary.

@commonsensesoftware
Copy link
Collaborator

It took some trickery, but I've been able to beat things into submission. You'll be able to use [ApiExplorerSettings] at both the controller and action level as you always have. The fix will be included in my current, open PR and will go out when the next version of the package is published. You'll still need to set options.UseApiExplorerSettings = true, but now it will be properly respected. Thanks.

@jimmy3912msncom
Copy link
Author

@commonsensesoftware Thanks Chris for the update :) Do you have an estimate about when the next version will be released?

@commonsensesoftware
Copy link
Collaborator

The last of the issues have been wrapped up and things appear to be stable. I'm preparing the final release notes for 3.0. I expect to release within the next couple of days - possibly later today.

@ckuykendall81
Copy link

ckuykendall81 commented Jul 15, 2019

I am on version 3.2 and still cannot get this to respect [ApiExplorerSettings(IgnoreApi = false)] to have it show up in swagger documentation. Setting options.UseApiExplorerSettings = true; does ignore all of my odata controllers, but I am trying to show a subset of the odata controllers. Any thoughts if it may be broken again?

@commonsensesoftware
Copy link
Collaborator

Did you remember to update the API Explorer package too? I'm assuming this for OData on ASP.NET Core.

I took the OData Swagger sample, changed services.UseODataApiExplorer to have options.UseApiExplorerSettings = true, and added [ApiExplorerSettings(IgnoreApi = false)] only to the FunctionsController. The FunctionsController is the only ODataController that shows up in the Swagger document.

It sounds like you have the correct configuration. Maybe share a snippet from one of your controllers or try repro'ing on your end by modifying the OData Swagger sample.

@ckuykendall81
Copy link

ApiExplorer is 3.2.0 as well. I am working on sample solution to try to repro.

@commonsensesoftware
Copy link
Collaborator

Perfect! That will help narrow down the issue quickly. Thanks.

@ckuykendall81
Copy link

I have an example. Not sure how to send it to you.

@commonsensesoftware
Copy link
Collaborator

You can zip it up and attach it to the thread by simply dragging it to the browser. Make sure you exclude the bin and obj folders so the file stays small. ;)

@ckuykendall81
Copy link

ApiExplorerSettings.zip
Sorry it took so long. It was the end of the day.

@commonsensesoftware
Copy link
Collaborator

Thanks for the repo. It was helpful. There is definitely a bug. This only appears to happen if you mix and match OData with non-OData controllers. The OData API Explorer information is being lost - somehow. I haven't been able to tell if it's being overwritten or the existing information isn't be copied. It's quite painful to troubleshoot since OData doesn't intrinsically support the API Explorer. You can track this specific issue in #519.

@ckuykendall81
Copy link

Yeah, we definitely are mixing in our current project. Thanks for looking into this and am welcoming a fix at some point.

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

3 participants