[Discussion] 3.0 - Removing Microsoft.AspNetCore.Mvc.WebApiCompatShim

[pranavkm]

The Microsoft.AspNetCore.Mvc.WebApiCompatShim (WebApiCompatShim) package provides partial compatibility in ASP.NET Core with ASP.NET Web API 2 to simplify migrating existing Web API implementations to ASP.NET Core. However, apps using the WebApiCompatShim do not benefit from the API related features shipping in recent ASP.NET Core releases, such as improved Open API Spec genertaion, standardized error handling, and client code generation. To better focus our API efforts in 3.0, we plan to remove WebApiCompatShim. We recommend that existing applications using the WebApiCompatShim migrate to the newer [ApiController] model.

[kevinchalet]

This package was downloaded about 600 000 times. It's a fairly high number when you know it's not referenced by the Microsoft.AspNetCore/Microsoft.AspNetCore.All metapackages.

Are you sure that removing this package - that shouldn't be that time-consuming to maintain in its current state - is worth the pain it will cause to the ones relying on it, who will be forced to choose between migrating away from the Web API 2 primitives or staying on 2.2 forever?

[Sebazzz]

@pinpointtownes: 2.x is an LTS release. This gives a few years to remove the shim dependency from your code.

[kevinchalet]

@Sebazzz sure, but it doesn't make the migration less painful: you just have more time to do it.

This package was a (great) way to encourage Web API 2 users to migrate without having to dedicate massive human/financial resources to rewrite all their controllers (apps with hundreds of controllers are not that rare).

Removing it at this point is like abandoning them in the middle of the road and telling them they must now migrate the hard way, for a maintenance gain that seems inexistant to me (the history seems pretty clear: https://github.com/aspnet/Mvc/tree/release/2.2/src/Microsoft.AspNetCore.Mvc.WebApiCompatShim).

[glennc]

I've been looking at the usage of this and at this point I still think we will remove it in 3.0. But there's still time to change that, I'm not locked in.

I'm willing to talk about either not removing it or porting some of the functionality in the shim. Some of it could become features you can turn on when using ApiController if we had a better understanding of what parts of the shim were most useful to folks. If you have opinions or thoughts on that then let me know.

[kevinchalet]

Some of it could become features you can turn on when using ApiController if we had a better understanding of what parts of the shim were most useful to folks.

IMHO, the ability to return instances of HttpResponseMessage and throw HttpResponseException exceptions (independently of whether it's a good or bad practice) exactly like the way you did in Web API 2 is probably one of the most important aspects, as you don't have to rewrite all your controllers to work with the new world model.

This part could certainly be replaced by something like a Roslyn-based rewriter, that would help move from the old pattern to the new IActionResult-based one. I'm a bit surprised this wasn't mentioned and proposed as a way to make the migration less painful once the shim package is gone.

[pranavkm]

@PinpointTownes I've filed a couple of follow up issues (https://github.com/aspnet/Mvc/issues/8703, https://github.com/aspnet/Mvc/issues/8704) based on this discussion.

A rewriter (Roslyn codefix?) certainly sounds like an interesting idea, I'm not entirely sure how well it'd for all but the most trivial of user codes. That said, the nice part about writing Roslyn extensibility is that the community can step up and contribute something.

[synesthesia]

At present AFAIK the DurableOrchestrationClient in Azure Durable Functions needs to take an HttpRequestMessage as a parameter to CreateCheckStatusResponse, so when writing in Functions v 2 you need the shim:

[FunctionName("MyStarter")]
public static async Task<HttpResponseMessage> HttpStart(
            [HttpTrigger(AuthorizationLevel.Function, "post")]HttpRequest req,
            [OrchestrationClient]DurableOrchestrationClient starter,
            ILogger log)
{
    // Function input comes from the request content.

    string name = req.Query["name"];
    var requestBody = await new StreamReader(req.Body).ReadToEndAsync();
    dynamic data = JsonConvert.DeserializeObject(requestBody);
    name = name ?? data?.name;

    string instanceId = await starter.StartNewAsync("MyOrchestration", name);

    log.LogInformation($"Started orchestration with ID = '{instanceId}'.");

    var shim = new HttpRequestMessageFeature(req.HttpContext);
    var httpRequestMessage = shim.HttpRequestMessage;
    return starter.CreateCheckStatusResponse(httpRequestMessage, instanceId);
}

So please at the very least co-ordinate this change with other Microsoft teams :)

[pranavkm]

@synesthesia WebJobs is not built on ASP.NET Core Mvc, consequently removing this shim should not affect Durable Functions. Are you seeing otherwise?

[synesthesia]

@pranavkm apologies for late reply. What I have since realised is that regardless of the starter code created by VS, we can choose for the function to take either a HttpRequestMessage or a HttpRequest and the runtime injects the correct object...

[deckertron9000]

@pranavkm A project I'm working on is currently using WebApiOverloadingApplicationModelConvention and WebApiParameterConventionsApplicationModelConvention as MVC options. Are those options supported in any way by AspNetCore.MVC without the use of the compat shim? If so is it documented somewhere?

[pranavkm]

@deckertron9000 for 3.0 the actual types aren't available. However we very rarely changed these types. You could very well copy the code from our 2.2 branches in to your application and they should work:

• https://github.com/aspnet/AspNetCore/blob/release/2.2/src/Mvc/Mvc.WebApiCompatShim/src/Conventions/WebApiOverloadingApplicationModelConvention.cs
• https://github.com/aspnet/AspNetCore/blob/release/2.2/src/Mvc/Mvc.WebApiCompatShim/src/Conventions/WebApiParameterConventionsApplicationModelConvention.cs

Feel free to open a new issue if you have run in to issues with these conventions, we'd be happy to help.