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

When using inheritance api versions are not correctly versioned #1288

Open
giggio opened this issue Apr 19, 2018 · 16 comments
Open

When using inheritance api versions are not correctly versioned #1288

giggio opened this issue Apr 19, 2018 · 16 comments

Comments

@giggio
Copy link
Contributor

giggio commented Apr 19, 2018

In ASP.NET Core, if you have a base controller with operations, and an inherited controller, and you use versioning, the version is not correctly discovered.

@RicoSuter
Copy link
Owner

v11.17.2

@steji113
Copy link

This still seems to happen in 11.17.7 as far as I can tell.
I have a base controller like this:

[ApiVersion("1.0")]
[ApiController]
public class ApiV1BaseController : Controller
{
}

and an inherited controller:

[Route("api/v{version:apiVersion}/audio-devices")]
public class AudioDevicesController : ApiV1BaseController
{
}

@RicoSuter
Copy link
Owner

I hope the commit fixes this... what do you think?

@steji113
Copy link

It looks sensible to me! I would be happy to try it out if there was a way for me to get a build of the package?

@RicoSuter
Copy link
Owner

There is a CI nuget feed on MyGet but there will be a release soon.

@steji113
Copy link

Great, thanks!

@RicoSuter
Copy link
Owner

Will release a new version in 15 min

@RicoSuter
Copy link
Owner

v11.17.10

@steji113
Copy link

Thanks for turning this around so quickly. I am sorry to report it still does not seem to be working :( If I figure out how to get NSwag's code up and running, I may be able to try debugging it further to contribute a fix.

@RicoSuter RicoSuter reopened this May 24, 2018
@RicoSuter
Copy link
Owner

You can clone the project and start/test with one of the sample apps (installer can be unloaded)

@RicoSuter
Copy link
Owner

And sorry, didnt test it... another option is to copy the processor class code into your project, remove the existing one from the settings and add your own with a fix... then post here the fixes :-)

@RicoSuter
Copy link
Owner

https://github.com/RSuter/NSwag/blob/master/src/NSwag.SwaggerGeneration/Processors/ApiVersionProcessor.cs

Easy changeable in the middleware settings (GeneratorSettings property).

@steji113
Copy link

Outstanding, that sounds like the best option. I will give that a try.

@RicoSuter
Copy link
Owner

Are you using ASP.NET or ASP.NET Core? Just for your background: The processor should be working correctly for both as it is used for both...

@steji113
Copy link

It is ASP.NET Core. Last night when playing around with ApiVersionProcessor, I found that the GetCustomAttributes(inherit: true) was not working as we expected. I was able to get my scenario to work by doing this:

var controllerAttributes =
        Attribute.GetCustomAttributes(context.ControllerType.GetTypeInfo(), inherit: true);
var controllerBaseAttributes =
        Attribute.GetCustomAttributes(context.ControllerType.BaseType.GetTypeInfo(), inherit: true);
            
var versionAttributes = context.MethodInfo.GetCustomAttributes()
        .Concat(controllerAttributes)
        .Concat(controllerBaseAttributes)
        .Where(a => a.GetType().IsAssignableTo("MapToApiVersionAttribute", TypeNameStyle.Name) ||
                a.GetType().IsAssignableTo("ApiVersionAttribute", TypeNameStyle.Name))
        .Select(a => (dynamic)a)
        .ToArray();

However, this solution isn't very scalable because it directly inspect's the Controller's base type. I could have inherited from a deeper hierarchy and it still may not have picked up the version attribute. Also, another side note, it did seem that in the current code, context.MethodInfo.DeclaringType and context.ControllerType evaluated to the same thing. So the two calls were redundant there.

I am not sure what the right solution is to handle all of the desired cases. We may need to write some unit tests that exercise these various scenarios and see if we can come up with a robust solution.

@RicoSuter
Copy link
Owner

However, this solution isn't very scalable because it directly inspect's the Controller's base type. I could have inherited from a deeper hierarchy and it still may not have picked up the version attribute.

Maybe with a loop which loops through the BaseTypes until BaseType is null...

context.MethodInfo.DeclaringType and context.ControllerType evaluated to the same thing. So the two calls were redundant there.

Yep, when I looked at the code I also wondered if we can just remove this...

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