API Version Model is Not Extensible in ASP.NET Core

[commonsensesoftware]

Synopsis

Customizing or otherwise extending the way API version metadata is provided in ASP.NET Core is not extensible. This is because the required constructors to provide extensibility on the ApiVersionModel are marked as internal.

Proposed Solution

Change the visibility of the constructors that accept ControllerModel and ActionModel to have public scope. For symmetry, the complementary constructors in the ASP.NET Web API implementation should be made public as well.

[pkiers]

Can't you move the IControllerModelConvention implementation to the ApiVersionsBaseAttribute or does that not work with .net core? That way you can keep ApiVersionModel internal.

[commonsensesoftware]

I could, but that doesn't really address the design flaw. IControllerModelConvention is specific to ASP.NET Core, but it's available in .NET 4.5.1 and .NET Core. The main issue with moving the logic into the ApiVersionsBaseAttribute would be that you're still stuck with how it wants to build-up the ApiVersionModel.

The proposed visibility change exposes two shortcut constructors that use the models to build the version information. Maybe that's not required either.

Since it's not really documented (because this is pretty low-level), it's probably worth discussing the constituent parts:

• Declared API Versions - these are the API versions explicitly declared on a controller or action. This is the set of API versions used for disambiguation.
• Supported API Versions - these are all the API versions, potentially aggregated, supported by the controller or action. These are aggregated during the selection process.
• Deprecated API Versions - these are all the API versions, potentially aggregated, deprecated by the controller or action. These are aggregated during the selection process.
• Implemented API Versions - the unioned set of supported and deprecated API versions.

When two or more ApiVersionModel instances are aggregated, it's important to make sure the declared versions are never aggregated. Doing so would break the internal logic. Again, having to know this and not make a mistake is why it's internal to begin with.

Perhaps the right direction is to move the IControllerModelConvention implementation as you suggested - only not to the base class, but out of the attributes all together. There's already an implementation in ApiVersionConvention that is used to apply the imperative conventions. I think this could be refactored to apply both the imperative and declarative API version conventions. This would remove the need to change the ApiVersionModel and would allow extension/customization by only having to implement IApiVersionProvider, inherit from ApiVersionAttribute, or inherit from ApiVersionsBaseAttribute. This would also have the benefit of strong affinity with the ASP.NET Web API implementation.

Thoughts?

[pkiers]

Well having to implement the IControllerModelConvention feels weird and is not obvious. So having that moved somewhere else seems best option to me.

But besides that the IApiVersionProvider feels bit weird still, I have to implement that and also inherit from ApiVersionsBaseAttribute. But I can never implement that without the base class because of the internal constructor of ApiVersionModel. Why not put that interface on the ApiVersionsBaseAttribute and implement that there with abstract implementations? So IApiVersionProvider is just internal logic also?
Just my first thoughts from the little code I saw so far.

[commonsensesoftware]

The reason that the IApiVersionProvider is not directly implemented on the ApiVersionsBaseAttribute is so you have control over how the attribute implementation shows up. Specifically, you may not want the AdvertiseOnly and/or Deprecated members to show up. Both the ApiVersionAttribute and AdvertiseApiVersionsAttribute intentionally implement the AdvertiseOnly property explicitly so that it's not visible.

[commonsensesoftware]

I'll leave PR #90 open for a bit so that folks can take a peek at it. The internal changes ended up being a little more than I thought, but I'm satisfied with the result. I was able to not only make the extensibility model simple (as intended), but I was able to also reduce some of the convention complexity as well. :)

[commonsensesoftware]

The fix is in. The first public release that will contain it will be in the forthcoming 1.1.0 milestone release. I'm working diligently to get it out as fast as possible. Thanks again for those waiting for this to get released.

[pkiers]

Thanks!