System.Web adapter API to convert between ASP.NET Core and System.Web types

[twsouthwick]

Background and Motivation

Through Microsoft.AspNetCore.SystemWebAdapters, we can get a representation of the HttpContext that conforms to what people expect for System.Web.HttpContext. Currently, the way to do that is via implicit conversions. It would be nice to have a static method that can be called to convert it without needing to cast in times where implicit conversions aren't obvious.

For example, currently we need to do the following if we want to change views in a method:

public void Test(Microsoft.AspNetCore.Http.HttpContext contextCore)
{
  var context = (System.Web.HttpContext)contextCore;
}

Proposed API

using System.Diagnostics.CodeAnalysis;
using System.Web;

namespace Microsoft.AspNetCore.SystemWebAdapters;

public static class SystemWebAdapterExtensions
{
    [return: NotNullIfNotNull(nameof(request))]
    public static HttpRequest? GetSystemWebRequest(this Microsoft.AspNetCore.Http.HttpRequest? request);

    [return: NotNullIfNotNull(nameof(request))]
    public static HttpRequestBase? GetSystemWebRequestBase(this Microsoft.AspNetCore.Http.HttpRequest? request);

    [return: NotNullIfNotNull(nameof(request))]
    public static Microsoft.AspNetCore.Http.HttpRequest? GetAspNetCoreRequest(this HttpRequest? request);

    [return: NotNullIfNotNull(nameof(response))]
    public static HttpResponse? GetSystemWebResponse(this Microsoft.AspNetCore.Http.HttpResponse? response);

    [return: NotNullIfNotNull(nameof(response))]
    public static HttpResponseBase? GetSystemWebResponseBase(this Microsoft.AspNetCore.Http.HttpResponse? response);

    [return: NotNullIfNotNull(nameof(response))]
    public static Microsoft.AspNetCore.Http.HttpResponse? GetAspNetCoreResponse(this HttpResponse? response);

    [return: NotNullIfNotNull(nameof(context))]
    public static HttpContext? GetSystemWebHttpContext(this Microsoft.AspNetCore.Http.HttpContext? context);

    [return: NotNullIfNotNull(nameof(context))]
    public static Microsoft.AspNetCore.Http.HttpContext? GetAspNetCoreHttpContext(this HttpContext? context);

    [return: NotNullIfNotNull(nameof(context))]
    public static HttpContextBase? GetSystemWebHttpContextBase(this Microsoft.AspNetCore.Http.HttpContext? context);
}

Usage Examples

public void Test(Microsoft.AspNetCore.Http.HttpContext contextCore)
{
-  var context = (System.Web.HttpContext)contextCore;
+  var context = contextCore.GetSystemWebHttpContext();

-  var request = (System.Web.HttpRequest)contextCore.Request;
+  var request = contextCore.Request.GetSystemWebHttpRequest();
}

See implementation at dotnet/systemweb-adapters#353

Alternative Designs

Unsure of names here, or prefix (i.e. Get or As)

For example, for HttpContext conversions:

• GetSystemWebHttpContext()/GetCoreHttpContext()
• GetHttpContextAdapter()/GetHttpContextAdapter()
• AsSystemWebHttpContext()/AsCoreHttpContext()
• AsHttpContextAdapter()/AsHttpContextAdapter()
• ToSystemWebHttpContext()/ToCoreHttpContext()
• ToHttpContextAdapter()/ToHttpContextAdapter()

[joperezr]

@davidfowl @halter73 Do you know who would be the best person to take an initial look at this and/or mark it as ready for review?

[davidfowl]

@Tratcher thoughts on the name? I think GetSystemWebXis great but I'm not sure about GetCoreX.

[Tratcher]

@davidfowl GetAspNetCoreX would be more symmetric with GetSystemWebX, and more clear.

public static HttpRequest? GetSystemWebRequest(this HttpRequestCore? request); HttpRequestCore? Is this a local alias?

[Tratcher]

These explicit conversions would be good markers for code that needs cleanup in the future. We could even include an optional analyzer.

[twsouthwick]

@davidfowl GetAspNetCoreX would be more symmetric with GetSystemWebX, and more clear.

public static HttpRequest? GetSystemWebRequest(this HttpRequestCore? request); HttpRequestCore? Is this a local alias?

Yup HttpContextCore aliased to Microsoft.AspNetCore.Http.HttpContext to make it obvious which context we're referring to. I like these suggestions - I'll update the API proposal (and make that alias clearer)

[amcasey]

I think I'm missing something. As much as I dislike implicit conversions (in general, not just here), we can't get rid of them without breaking existing consumers. So the new model would be to have three methods of converting?

1. Call an explicit instance method. I gather this would be recommended.
2. Make an explicit cast. I'm not sure I understand why this is undesirable: because it will almost certainly require a qualified name? because you'd need extra parens to dot into the result?
3. Do nothing and let it get converted automatically. I gather this would be discouraged?

It sounds like we want to introduce analyzers to push style towards one of these options (1?) but we probably wouldn't be able to deprecate any of them?

Are there any cases where the conversion changes the results of (e.g.) overload resolution or is this purely for readability (also a very important consideration)?

[twsouthwick]

One of the principles of the adapters project was to enable incremental migration with minimal code changes. That's why we initially just exposed the implicit operator as it allows minimal changes to migrate; i.e. a controller is migrated to core, but can still call methods that take System.Web APIs without littering the code everywhere with conversion calls. The number of places that would have to be updated can be non-trivial in apps, so implicit conversions simplify the process with less code churn.

However, as people move to more in-box APIs, sometimes this requires translating between the representations in the same method. These are the scenarios the explicit conversion methods are being proposed for. The implicit operators work well for some of the source compatible magic we designed them for, but outside of that feel awkward to use. So this is geared both for readability as well as discoverability.

The comments about analyzers being able to guide people away from the adapters is something we do want to do and this may help them, but it is not the main goal of enabling these APIs (although it will make it more obvious for people if they see these explicit calls and is a nice side effect).

With that background, let me make sure to answer your questions:

So the new model would be to have three methods of converting?

To explicitly answer your question, yes, although the recommendations won't be so cut and dry like that. At the initial phases of migration, the implicit cast (3) would be the recommendation to just get something moved to .NET Standard/Core. As they need the ability to switch representations, they will find themselves using (1) or (2), but (2) is not obvious.

It sounds like we want to introduce analyzers to push style towards one of these options (1?) but we probably wouldn't be able to deprecate any of them?

As mentioned above, each pattern is appropriate depending on the point of migration they are in, so it's about guiding them to less-reliance on the adapters over time. We're not fully sure how we want to do this

Are there any cases where the conversion changes the results of (e.g.) overload resolution

Potentially, but that would require them to have methods that only differ in Microsoft.AspNetCore.Http.HttpContext and System.Web.HttpContext - something I expect they're aware of.

[twsouthwick]

@davidfowl @Tratcher any other thoughts or concerns? what are the next steps here?

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[halter73]

• It looks ugly when you have to explicitly cast, but should things look ugly until you fix it?

• There's also the discoverability aspect. The implicit conversion is hard to find.

  • Is the discoverability necessary or is it something people expect to work?
  • But it also makes it easier to document. Something to F12 on.

• Can we shorten the method names?

  • As vs Get? Translate?
  • Call them all the same thing?
    • AsSystemWeb()/AsAspNetCore()? ToSystemWeb(), ToAspNetCore()
    • @BrennanConroy suggests AsAncient() vs AsModern().
    • We like "As" because it implies there is no copying and is similar to an implicit cast indicating that it does the same thing.
    • This means we need to get rid of the "Base"-suffixed variations, but we don't think we need it.
    • We could always add AsSystemWebBase() or similar later.

• [return: NotNullIfNotNull(nameof(response))] on an extension method?

  • This makes sense on an implicit conversion, but calling an extension method on something that could be null is weird even if technically allowed.

API Approved!

// Microsoft.AspNetCore.SystemWebAdapters.CoreServices.dll

using System.Web;

namespace Microsoft.AspNetCore.SystemWebAdapters;

+ public static class SystemWebAdaptersConversionExtensions
+ {
+    public static HttpRequest AsSystemWeb(this Microsoft.AspNetCore.Http.HttpRequest request);
+    public static HttpResponse AsSystemWeb(this Microsoft.AspNetCore.Http.HttpResponse response);
+    public static HttpContext AsSystemWeb(this Microsoft.AspNetCore.Http.HttpContext context);

+    public static Microsoft.AspNetCore.Http.HttpRequest AsAspNetCore(this HttpRequest request);
+    public static Microsoft.AspNetCore.Http.HttpResponse AsAspNetCore(this HttpResponse response);
+    public static Microsoft.AspNetCore.Http.HttpContext AsAspNetCore(this HttpContext context);
+ }