Skip to content

MapToApiVersion not descovered but still able to hit the route #735

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

Closed
schadowfax opened this issue Mar 15, 2021 · 7 comments
Closed

MapToApiVersion not descovered but still able to hit the route #735

schadowfax opened this issue Mar 15, 2021 · 7 comments

Comments

@schadowfax
Copy link

schadowfax commented Mar 15, 2021

Please consider the following case.

Because version 1.0 is not defined on the controller in a ApiVersionAttribute it is not discovered a not declare an API version. Just like i want because from this moment on i don't want to support this version any longer. My swagger documentation that is dependen on the IApiVersionDescriptionProvider works correctly and is no longer generating documentation for version1. When i make a api call to version 2.1 and check the returned version headers there is also only the supported version 2.1 & the deprecated version 2.0. Just as I excpected.

But when i make a call to version 1.0 i still hit the endpoint and the apiversion still doesn't return 1.0 as a supported version. I was hoping this would return a 400 bad request with the "UnsupportedApiVersion" error message and with the headers descriping the versions 2.0&2.1.

The reason i was exploring this behavior is that we want to make it possible to upgrade our microservice versions through adding the ApiVersionAttribute for certain version at runtime through some configuration. So we don't need to make a new build because the Getdefault route will be mapped for those higher apiversions.

// 👇 Declare both versions
[ApiVersion("2.0", Deprecated = true)]
[ApiVersion("2.1")]
[Route("v{version:apiVersion}/HelloWorld")] // Support path versioning
[ApiController]
public class HelloWorldController : ControllerBase
{

    // 👇 Map to v1.0
    [MapToApiVersion("1.0")]
    public string Get1() => "v1.0";

    // 👇 Map to v2.0
    [MapToApiVersion("2.0")]
    public string Get2() => "v2.0";

    // 👇 Map to all versions defined on the controller that not already have a mapping
    [MapToApiVersion("2.1")]
    public string Getdefault() => $"{forExamplePerposeHereIsVersionReturned}";
}
@commonsensesoftware
Copy link
Collaborator

Can you share how you've configured services.AddApiVersioning? I suspect the issue is related to that.

@schadowfax
Copy link
Author

schadowfax commented Mar 15, 2021

@commonsensesoftware as requested

        services.AddApiVersioning(options =>
        {
            options.UseApiBehavior = true;
            options.ReportApiVersions = true;
            options.AssumeDefaultVersionWhenUnspecified = true;
            options.DefaultApiVersion = new ApiVersion(2, 0);
            options.ErrorResponses = new UnsupportedApiVersionError();
        });

@commonsensesoftware
Copy link
Collaborator

As expected, you have options.AssumeDefaultVersionWhenUnspecified = true. This isn't doing what you think it's doing. I guess I'll never fully understand why people try to use this feature like this, but it's not what it was meant for. Unless you have routing configured somewhere else that I don't see, this setting currently does nothing because you are versioning by URL segment. In the same way, you cannot have a default value in a template such as order/{id}/items.

Something else that is strange is that you still have an action mapped to API 1.0. Why? Unless I misunderstood, you want to completely eliminate/sunset that version. All of that code should be removed.

You didn't say, but I assume the URL that is causing you unexpected results is v1.0/HelloWorld. Since the 1.0 is not formally declared, but is mapped, it is no longer discoverable, but can be reached if you know to ask for it. If you simply remove the action from the controller, you should get the results you expect.

@schadowfax
Copy link
Author

@commonsensesoftware ty for the responds i will look further into it.

On why there is still an action mapped to 1.0. Let me try to explain this. We have multiple microservice that we always want to keep to all have the same ApiVersions able to hit. So i thought that since ApiVersion 1.0 isnt declared it would fail to map and wouldn't be reachable. This is to explore the following case that in the future could occure. Whe we have an update for microservices x and we need/want to stop supporting version 1.0 because of that. But don't want to make a new build of the HelloWorldApi. So then we want to remove the version 1.0 support with configuration the thing i was investigating was this.

We apply the (deprecated)apiversions at each controller from sorth of configuration/environmentsetting

    public class ApiVersionConvention : IApplicationModelConvention
    {
        public void Apply(ApplicationModel application)
        {
            foreach (var controllerModel in application.Controllers)
            {
                var builder = new ControllerApiVersionConventionBuilder<ControllerModel>();
                //👇 but read from configuration passed in
                builder.HasDeprecatedApiVersion(new ApiVersion(1, 0));
                builder.HasApiVersion(new ApiVersion(2, 0));

                builder.ApplyTo(controllerModel);
            }
        }
    }

Then some kind of MapToApiVersionRangeAttribute that can add a range from/to or if no to defined to the max apiversion defined (need to change to support minior versions here still)

    [AttributeUsage(AttributeTargets.Method, AllowMultiple = false)]
    public class MapToApiVersionRangeAttribute : ApiVersionsBaseAttribute, IApiVersionProvider
    {
        ApiVersionProviderOptions options = ApiVersionProviderOptions.Mapped;

         //👇 for when the last version is not know +10 now hard coded but should be the max apiversion
        public MapToApiVersionRangeAttribute(int start, int step = 1) :
  base(GenerateRange(start, start + 10, step))
        { }

        public MapToApiVersionRangeAttribute(int start, int end, int step = 1) :
          base(GenerateRange(start, end, step))
        { }
        public ApiVersionProviderOptions Options => options;

        static ApiVersion[] GenerateRange(int start, int end, int step)
        {
            var versions = new List<ApiVersion>();
            for (var major = start; major <= end; major += step)
            {
                versions.Add(new ApiVersion(major, 0));
            }
            return versions.ToArray();
        }
    }

if we then had the HelloWordController configured like this

// 👇 no versions declared they are applied through the ApiVersionConvention 
[Route("v{version:apiVersion}/HelloWorld")] // Support path versioning
[ApiController]
public class HelloWorldController : ControllerBase
{

    // 👇 Map to v1.0
    [MapToApiVersion("1.0")]
    public string Get1() => "v1.0";

    // 👇 Map to v2.0
    [MapToApiVersion("2.0")]
    public string Get2() => "v2.0";

    // 👇 Map to all versions starting from 2.1 and upwards by 0.1 until the max supported ApiVersion
    [MapToApiVersionRange(start: 2.1, step: 0.1)]
    public string Getdefault() => $"{forExamplePerposeHereIsVersionReturned}";
}

So for this to work i need the not formally declared versions no longer able to reach them. i can solve this on other ways ofcourse but i'm starting to guesse this is a wrong approach

@commonsensesoftware
Copy link
Collaborator

It sounds like you are trying to achieve Version Symmetry by applying uniform API versions, even when nothing has changed so that a client only uses a single API version. You can achieve that with a convention, but API Versioning provides a formal Conventions API where you can use a fluent API and/or define your own conventions. That will be more natural and be simpler to implement than trying to use IApplicationModelConvention.

In truth, the action probably should not match. Unfortunately, the application model and routing policies are disconnected by design. The endpoint matcher policy doesn't know how an action was mapped to a particular API version; it just knows that it has some associated API version(s). The most it can infer is whether an API version is explicitly defined (for precedence) as in the case of [MapToApiVersion] or implicitly defined from the containing controller.

To move the API version forward for symmetry, I can see how you would want to drive that with configuration. You shouldn't assume that you can easily remove implementation code without a change though. This is a similar problem to people using inheritance in controllers. You cannot uninherit an action across API versions.

If you truly want to ignore something by way of convention, you'll have to try something else. There's a few ways that could be done:

  1. Inject NonActionAttribute into the ActionModel you want to ignore. This is yucky, but achievable. I guess you would inject it for any action mapped to a version that is no longer defined in your configuration. ASP.NET will no longer consider this action method as a candidate with that metadata present.
  2. Define your own metadata and then use a convention to bridge the mapping when applicable. You might define something like [MinimumApiVersion("1.0")] and only map the action if the complete version set includes 1.0. If it doesn't, then it's not mapped.

It's also worth noting that the API Versioning metadata is computed upfront. This information cannot be refreshed without restarting the host application. That's by design. You are probably already aware of that, but I just thought I'd call it out.

There are some things attractive about Version Symmetry, but if your APIs are not external customer facing, the juice is not worth the squeeze in my opinion. Even then you can define sensible groups of APIs and versions without them all being the same. Not every service in Azure or AWS has the version number. Food for thought.

@commonsensesoftware
Copy link
Collaborator

Looks like suggestion #1 won't work after all. Have a look at DefaultApplicationModelProvider. It uses MethodInfo directly. It is probably possible to get in front of that, but it will be painful.

Moving to a pure convention-based model without using any attributes will likely be required to get the flexibility you want. Everything that can be done with attributes can be done with conventions, including MapToApiVersion. It's probably simpler to define your conventions using the fluent API. You can wrap it up in a custom convention if that makes it easier to manage. Ultimately, all of the controllers and actions that can be mapped are statically known (based on your description). Mapping things down to that action level without static types is possible, but not straight forward. That is information you'd have to provide in your configuration.

@commonsensesoftware
Copy link
Collaborator

Circling back around on this issue... I'm promoting it to a bug as it should not be possible to route to an action that is only mapped.

I've come up with a solution, but it's trickier than expected. Essentially, if an action has some declaration of an API version, but there is no corresponding implementation found, then the action doesn't match. There's very little difference between declaring an API version and mapping an API version, but the key difference is that mapping is not discoverable. The previous implementation did not consider a scenario where everything matched up to the request, but there were actually no discoverable (e.g. implemented) corresponding versions.

This behavior has actually lead to another bizarre edge case that I don't have a solution for. If an API version is, in fact, implemented, but is split across multiple controller types, it is technically possible to map/match API versions across the types. This just feels wrong and is almost certainly a developer mistake, but there's no real way to catch such a behavior. Things will only successfully execute if there is a single mapping. The case where this would most likely happen will be from copy and paste, which would be caught as runtime mistake due to multiple matches. I've never seen this happen, it's a very rare edge case, I can't see how anyone would want to do that on purpose, and - ultimately - it isn't technically wrong from filtering out a list of possible candidates. I prefer to clear the minefield of such things. Perhaps an analyzer in the future could help detect and warn about such possible misconfigurations.

In rereading this issue, I realized there is another approach that might be easier for you. Rather than necessarily change API version mapping, you can simply filter out controllers altogether. In the default configuration and implementation, this is achieved via the IApiControllerFilter service. The default implementation accepts a sequence of injected IApiControllerSpecification instances. If you're only using vanilla MVC (e.g. not OData), then the only built-in filter is ApiBehaviorSpecification, which looks for [ApiController]. In your case, it might be simpler to just replace the entire IApiControllerFilter service. It only has a single method Filter which accepts an original list of controllers and returns a filtered set of controllers. The advantage of this approach is that this applies a filter to candidate controllers before any API version discovery or collation occurs. This would mean you don't have to worry about unmapping versions that have been removed because from API Versioning's perspective, the controllers simply don't exist.

In order for that approach to work, you'd want a type-per-controller without interleaving; otherwise, it's not possible to split things out. You could use the VersionByNamespaceConvention, which would apply an API version to each controller in a namespace. Your filter can then filter out controllers in the namespace that is no longer supported. A final thing that you'll likely want with this approach would be a custom IActionDescriptorProvider that removes the filters controllers from MVC entirely. The filter will keep things hidden from the API versioning meta model, but the corresponding actions would technically still be reachable without versioning. That is how things like UI controllers can live side-by-side without versioning. MVC has a few ways to add/remove controllers and action dynamically. Any of those should work as well.

Hopefully that is useful to you. The suggestions I've previously suggested all still apply. The next patch will contain a fix for this issue so no one else runs into it. In the meantime, there are several alternatives that should lead you toward a working solution.

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

2 participants