dotnet / aspnet-api-versioning

Provides a set of libraries which add service API versioning to ASP.NET Web API, OData with ASP.NET Web API, and ASP.NET Core.
MIT License
3.03k stars 702 forks source link

Add ApiVersion attribute to one Action brokes other Actions #487

Closed lansman closed 5 years ago

lansman commented 5 years ago

We have a .net core 2.1 WebAPI project with 80 controllers, 220 actions.

Want to start using versioning right now and gradually add new version Actions to exist Controllers. Old client must have access to old actions without specify api-version.

I configured:

Startup.cs:
services.AddApiVersioning(options => 
{
    options.AssumeDefaultVersionWhenUnspecified = true;                
});

All old actions still work without specifying api-version neither on controllers/actions, nor on client side, fine.

Let's look at one of our controllers:

DayMenuController.cs

[HttpGet]
public IActionResult Get() {}

[HttpPost]
public IActionResult Post([FromBody]Data data) {}

I added new, optimized Action, specifying new and old actions versions

DayMenuController.cs

[HttpGet]
[ApiVersion("1.1")]
public IActionResult GetFast() {}

[HttpGet]
[ApiVersion("1.0", Deprecated = true)]
public IActionResult Get() {}

[HttpPost]
public IActionResult Post([FromBody]Data data) {}

GetFast is called with api-version=1.1 Get is called with api-version=1.0 or without api-version, exactly what i need

But Post method (which i haven't touched at all) has broken suddenly!

POST DayMenu
The HTTP resource that matches the request URI 'http://localhost:5000/api/DayMenu' is not supported.

It seems when you specified ApiVersion at least on one of the Actions - you must specify it on all Controller's Actions as well. Option AssumeDefaultVersionWhenUnspecified is just ignored in this case.

Developer, adding new Action, must not forget to specify ApiVersion on ALL Actions (which could be dozens, in different files).

It seems very frustrating and illogical to me. How to force specify ApiVersion=1.0 on Actions without explicit version? Could you explain this?

commonsensesoftware commented 5 years ago

I'll look into this. What you have looks correct. There might be a bug in this particular combination.

As a workaround, the following should work just fine:

[ApiVersion("1.1")]
[ApiVersion("1.0", Deprecated = true)]
public class DayMenuController : ControllerBase
{
    [HttpGet]
    [MapToApiVersion("1.1")]
    public IActionResult GetFast() => Ok();

    [HttpGet]
    public IActionResult Get() {}

    [HttpPost]
    public IActionResult Post([FromBody]Data data) => StatusCode(201);
}

With so many controllers, you might consider using conventions, at least for your baseline. You can mix conventions and attributes. The result is a union of the two.

I've been really backed up lately, but I'll try to work on this ASAP. Thanks for your patience.

lansman commented 5 years ago

With so many controllers, you might consider using conventions,

I've tried

options.Conventions.Controller<ControllerBase>().HasApiVersion(1, 0);

(all controllers are derived from ControllerBase)

But is doesn't work.

I'm using Microsoft.AspNetCore.Mvc.Versioning (3.0.0), this is latest version supports netcore 2.1, afaik.

commonsensesoftware commented 5 years ago

Inheritance is intentionally not supported. You can, however, create a custom convention with relative ease (as noted on the wiki) that will apply whatever default version you want using a convention. This is one possible way you could make inheritance work if you want to. Something like:

public class MyApiConvention : IControllerConvention
{
    readonly ApiVersion version;

    public MyApiConvention(ApiVersion version) => this.version = version;

    public bool Apply( IControllerConventionBuilder controller, ControllerModel controllerModel )
    {
        controller.HasApiVersion(version);
    }
}

Then you can add it to the options as:

options.Conventions.Add( new MyApiConvention( options.DefaultApiVersion ) );
commonsensesoftware commented 5 years ago

I know it's been a while, but I'm finally circling back around on this. After some careful evaluation, I believe what you originally experienced is the expected behavior.

When you grandfather in existing services/APIs/code, the notion of assuming the default API version is to solve a scenario like yours such that you do not have to modify tens or even hunderds of touchpoints. All existing code maps to options.DefaultApiVersion. API versioning does not support a concept of no API version.

That being said, once you start making any type of modifications to an API, these rules go out the window. This is because you need to be able to sunset old APIs. Once any type of explicit mapping is done whether it be by attribute or convention, the implicit mappings are discarded. For this reason, once you start making changes, you should be holistical about applying all relevant API versions. In your case, you started interleaving new API versions beyond the default API version, which subsequently stops honoring the implicit API version convention.

If you're evolving APIs one at a time or in smaller sets, then the first solution I provided is probably the way to go. If you're trying to evolve APIs across a large set, then using a convention may be a better option (as shown in the second solution). You are also free to mix these approaches. The result is a union of all defined API versions. If you want to use interleaving (as it appears you do), then you can use a convention to easily apply API versions to all controllers and then use attributes to map API versions to specific actions.

I hope that helps. Let me know if you have more questions or whether this issue is resolved. Thanks.

commonsensesoftware commented 5 years ago

It appears this issue is resolved. If you continue to have issues, feel free to come back or reopen the issue. Thanks.

sjd2021 commented 2 years ago

I understand that everything defaults to the default api version, but I don't see how adding a 2.0 version to a new endpoint suddenly changes the default for every other action such that it has none at all. I wish there was a way around this.

commonsensesoftware commented 2 years ago

@sjd2021 In order for that to happen, you must be interleaving multiple versions on the same type. The default API version is required because it would otherwise be ambiguous between the original, unversioned API and the new, versioned variant. The original implementation wasn't really unversioned, there just wasn't any formal declaration. This is evident by adding versioning.

As you're aware, the implicit mapping addresses that and also prevents you from having to touch all your code once versioning is introduced. The next problem becomes, how do you remove this behavior? Since it is applied implicitly, there is really only one clear way to distinguish when that should be discarded - explicit versioning. Interleaving multiple API versions on a single type is a conscience choice. In doing so, you now own all the versioning declaration. This ensures that if you didn't intend to have the implicit version anymore, it's gone (ex: you're sunsetting the version). If you intend to keep it, then you now have to explicitly indicate that.

Consider:

[ApiController]
[Route("[controller]")]
public class ProductController : ControllerBase
{
    [HttGet]
    public IHttpResult Get() => Ok(new Product[]{ new(42) });

    [HttGet("{id}")]
    public IHttpResult Get(int id) => Ok(new Product(id));
}

If ApiVersioningOptions.DefaultApiVersion is 1.0, then ProductController implicitly has 1.0 applied to it. If I want to add and interleave a new API version on the same type, it might look like:

[ApiVersion("1.0")]
[ApiVersion("2.0")]
[ApiController]
[Route("[controller]")]
public class ProductController : ControllerBase
{
    [HttGet]
    public IHttpResult Get() => Ok(new Product[]{ new(42) });

    [HttGet("{id}")]
    public IHttpResult Get(int id) => Ok(new Product(id));

    [MapToApiVersion("2.0")]
    [HttpPost]
    public IHttpResult Post([FromBody] Product product) =>
        CreatedAtAction(nameof(Get), new { id = product.Id }, product);

In order to retain 1.0, I now have to explicitly declare it. There are multiple ways to achieve that, but here, I've simply added an attribute for it. I don't need to add any other metadata to retain the functionality. Both flavors of GET now support 1.0 and 2.0 (e.g. implicitly map to). The new POST action, however, is only supported in 2.0.

API versions can also be applied by convention and are mutually inclusive with any attributes. If you needed to apply the defaults explicitly to a bunch of controllers, you might find using a convention more manageable. Of course, if you don't interleave multiple versions on a type, this will never be a problem. That feature exists so you're in control, not because it's the best or even recommended way to do things. There are reasonable scenarios where interleaving makes sense. In general, keeping the implementation per API version separate is easier to manage.