Move IFeatureCollection to new Extensions.Features assembly #32043

Tratcher · 2021-04-21T22:40:17Z

Contributes #31723

Create new project Microsoft.Extensions.Features (netstandard2.0)
Move IFeatureCollection, FeatureCollection etc to this assembly. Add type-forwarding from Http.Features.
SignalR will reference this new project. This is a breaking change for SignalR clients but it's ok since this is already broken
Update Microsoft.AspNetCore.Http.Features to target net6.0 and stop publishing it as a NuGet package.

This allows us to use new language features like DIMs in Http.Features without cross compiling.
#31495
#31625

src/Http/Http.Features/src/Microsoft.AspNetCore.Http.Features.csproj

…csproj

davidfowl · 2021-04-21T23:27:46Z

@BrennanConroy please measure signalr impact

Tratcher · 2021-04-21T23:34:58Z

@BrennanConroy please measure signalr impact

Microsoft.AspNetCore.SignalR.Client ->
Microsoft.AspNetCore.SignalR.Client.Core ->
Microsoft.AspNetCore.SignalR.Common ->
Microsoft.AspNetCore.Connections.Abstractions ->
Microsoft.Extensions.Features

What other impact are you interested in?

davidfowl · 2021-04-21T23:44:10Z

New client on old asp.net core apps. Old client in new apps.

halter73 · 2021-04-22T01:23:53Z

~~I expect both will work fine. If Microsoft.Extensions.Features is loaded, the type forward takes precedence over anything else. Otherwise, the client will continue to use the old assembly.~~ Worth checking though. Edit: Not so sure of this anymore. Definitely worth checking 😄

halter73 · 2021-04-22T01:24:55Z

SignalR will reference this new project. This is a breaking change for SignalR clients but it's ok since this is already broken

Why is changing dependencies breaking?

src/Http/Http.Features/src/Properties/AssemblyInfo.cs

davidfowl · 2021-04-22T02:51:19Z

Are we keeping Http.Features as a package targeting netstandard?

Tratcher · 2021-04-22T03:27:29Z

Are we keeping Http.Features as a package targeting netstandard?

No, that's the main reason for this change, and the reason it's breaking.

davidfowl · 2021-04-22T03:40:29Z

No, that's the main reason for this change, and the reason it's breaking.

Lets consider this then:

App (.NET 6) -> Package1 -> Http.Features 3.1 (for IFeatureCollection)
App (.NET 6) -> Package2 -> Extensions.Features 6.0 (for IFeatureCollection)

Consider App is a console app and then App as an ASP.NET Core app.

Do you get a source time type conflict?

Tratcher · 2021-04-22T03:46:53Z

Keep in mind that Http.Features and Extensions.Features are also both in the shared framework and should unify to the latest version (needs verification). A standalone publish should also be able to resolve the conflict (needs verification). To really get into trouble I expect you'd need to not be using the shared framework. I'll do some experiments.

davidfowl · 2021-04-22T03:58:22Z

I just want to explore the breaks so we can document them and anticipate what and how packages will snap

benaadams · 2021-04-22T04:58:36Z

Create new project Microsoft.Extensions.Features (netstandard2.0)
Move IFeatureCollection, FeatureCollection etc to this assembly. Add type-forwarding from Http.Features.
...
This allows us to use new language features like DIMs in Http.Features without cross compiling.

Is a layering issue for DIMs if want to do the same thing with the FeatureCollection itself to get the same benefits as Headers?

So would maybe need to introduce a derived type IHttpFeatureCollection

IHttpFeatureCollection : IFeatureCollection
{
    IRouteValuesFeature RouteValuesFeature => Get<IRouteValuesFeature>();
    // ...
}

Which would then go viral and IFeatureCollection couldn't be used for the Http side as it wouldn't be the derived type etc

As I assume you wouldn't want to add Http DIMs to a more general non-Http IFeatures collection in Extensions?

davidfowl · 2021-04-22T05:04:48Z

As I assume you wouldn't want to add Http DIMs to a more general non-Http IFeatures collection in Extensions?

That's correct.

benaadams · 2021-04-22T05:34:06Z

As I assume you wouldn't want to add Http DIMs to a more general non-Http IFeatures collection in Extensions?

That's correct.

But an issue is if a IHttpFeaturesCollection then wouldn't be interopative with a IFeaturesCollection to get the benefits of DIMs without adding an is check; which would diminish the benefits of DIMs?

HttpFeatureCollection : IHttpFeatureCollection
{
    private IFeatureCollection _collection;

    public HttpFeatureCollection(IFeatureCollection collection)
    {
        _collection = collection;
    }

    IRouteValuesFeature RouteValuesFeature => 
        _collection is IHttpFeatureCollection httpCollection ? 
            httpCollection.RouteValuesFeature :  
            _collection.Get<IRouteValuesFeature>();

    // ...
}

Or do it in contructor and then null check

HttpFeatureCollection : IHttpFeatureCollection
{
    private IHttpFeatureCollection _httpCollection;
    private IFeatureCollection _collection;

    public HttpFeatureCollection(IFeatureCollection collection)
    {
        if (collection is IHttpFeatureCollection _httpCollection)
        {
            _httpCollection = collection;
        }
        else
        {
            _collection = collection;
        }
    }

    IRouteValuesFeature RouteValuesFeature => 
        _httpCollection is not null ? 
            _httpCollection.RouteValuesFeature :  
            _collection.Get<IRouteValuesFeature>();

    // ...
}

Ofc its more complicated as there are othertypes of IFeatureCollection in use; like the TransportConnection is one but it isn't part of the Http kinda stuff but is available via the Http collection, so if also wanted Transport DIMs to be usable to the Http then they would need to go on top

ITransportFeatureCollection : IFeatureCollection
{
    IConnectionLifetimeFeature ConnectionLifetimeFeature=> Get<IConnectionLifetimeFeature>();
    // ...
}

IHttpFeatureCollection : ITransportFeatureCollection
{
    IRouteValuesFeature RouteValuesFeature => Get<IRouteValuesFeature>();
    // ...
}

Then accept that Http was always over a connection (it probably is); but then that's quite a formal api heirachy using DIMs vs it being a bit more fluid currently (although is a guess if the featureCollection will resolve the type as its not guaranteeing types via contract; whereas the DIMs would be strongly suggesting it)

Just wondering if there is an opperturnity for DIMs over the IFeatureCollection itself; how that would work and what type DefaultHttpContext would be taking in is contructor (where it resolves IHttpRequestFeature, IHttpResponseFeature etc from); and then what would Kestrel's HttpProtocol be implementing?

Or would it not work?

davidfowl · 2021-04-22T06:27:39Z

Just wondering if there is an opperturnity for DIMs over the IFeatureCollection itself; how that would work and what type DefaultHttpContext would be taking in is contructor (where it resolves IHttpRequestFeature, IHttpResponseFeature etc from); and then what would Kestrel's HttpProtocol be implementing?

I don't think it would work with the layering we want. At least, not with the way that we want to push feature collection down into the stack. It feels bad to attempt to use the same trick here. We would need to replat everything on top of a more specific one and do the type check.

Tratcher · 2021-04-22T22:14:36Z

Lets consider this then:

App (.NET 6) -> Package1 -> Http.Features 3.1 (for IFeatureCollection)
App (.NET 6) -> Package2 -> Extensions.Features 6.0 (for IFeatureCollection)

Consider App is a console app and then App as an ASP.NET Core app.

Do you get a source time type conflict?

I was able to contrive a setup like this to produce the error.

Console App net6.0

Project reference to library1 net5.0
- Package reference to Microsoft.AspNetCore.Http.Features 5.0.0
Project reference to library2 net6.0
- Project reference (in repo) to Microsoft.Extensions.Features

Error	CS0433	The type 'FeatureCollection' exists in both 'Microsoft.AspNetCore.Http.Features, Version=5.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60' and 'Microsoft.Extensions.Features, Version=6.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60'

Interestingly I only get the error at compile time when I directly reference IFeatureCollection in the App Program.cs. If it's only referenced by type in the libraries everything compiles and runs fine with it treated as two distinct types. Then you only get compilation failures if you try to pass the IFeatureCollection from one library to the other.

Workaround: In the base App add a direct reference to Microsoft.AspNetCore.Http.Features 6.0 to lift the dependency and include the type forward. Note this works with in-repo references, we'll want to confirm it after we have a full build and real packages.

I get the same results testing a web app in-repo, but I think that's because it's not using the shared framework. I'll need to re-test after we have a full build.

I don't see anything surprising or blocking here. I'll plan to re-test after it's merged and we have a build.

@davidfowl is that enough coverage for us to proceed?

davidfowl · 2021-04-22T22:49:48Z

Workaround: In the base App add a direct reference to Microsoft.AspNetCore.Http.Features 6.0 to lift the dependency and include the type forward. Note this works with in-repo references, we'll want to confirm it after we have a full build and real packages.

This is why I wanted to ask if we should ship a package with type forwards for 6.0 as a way to bridge the gap until 7.

benaadams · 2021-04-22T23:05:40Z

Fair point. I wonder if the FrameworkReference will work just as well. I won't be able to test that until there's a real build.

The external Grpc tests were failing for me as they ref'd the previous framework (5.0) and the auto-update to 6.0 broke because the type moved, so a type forwards is probably recommended

pranavkm · 2021-04-22T23:26:45Z

For the announcement, technically it's every API in the current package, not just the few.

src/Http/Features/src/Microsoft.Extensions.Features.csproj

Tratcher · 2021-04-22T23:45:48Z

For the announcement, technically it's every API in the current package, not just the few.

@pranavkm true, but the template calls for specific links. Any suggestions for an abbreviated format to cover the others? The docs don't list APIs by assembly, only namespace, and it's not a one-to-one match.

pranavkm · 2021-04-22T23:58:40Z

Yeah, I'm not super sure there's a good way to present it. @JamesNK any suggestions?

JamesNK · 2021-04-23T00:14:21Z

Sorry, I'm not sure what the question for me is.

Fair point. I wonder if the FrameworkReference will work just as well. I won't be able to test that until there's a real build.

The external Grpc tests were failing for me as they ref'd the previous framework (5.0) and the auto-update to 6.0 broke because the type moved, so a type forwards is probably recommended

So with the change in this PR, the package Grpc.AspNetCore will break when used with .NET 6? If so that definitely needs to be improved because the package isn't doing anything special or hacky. If Grpc.AspNetCore is broken then tons of packages will be broken. This break would be too big.

benaadams · 2021-04-23T00:20:00Z

So with the change in this PR, the package Grpc.AspNetCore will break when used with .NET 6? If so that definitely needs to be improved because the package isn't doing anything special or hacky. If Grpc.AspNetCore is broken then tons of packages will be broken. This break would be too big.

Shouldn't do; the TypeFowarders are there, was just pointing out they were necessary

Sorry, I'm not sure what the question for me is.

Think its about the announcement? #32043 (comment)

For the announcement, technically it's every API in the current package, not just the few.

@pranavkm true, but the template calls for specific links. Any suggestions for an abbreviated format to cover the others? The docs don't list APIs by assembly, only namespace, and it's not a one-to-one match.

davidfowl · 2021-04-23T06:58:22Z

@Tratcher that looks good. I think we should also mention the types of errors that can show up:

Error	CS0433	The type 'FeatureCollection' exists in both 'Microsoft.AspNetCore.Http.Features, Version=5.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60' and 'Microsoft.Extensions.Features, Version=6.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60'

And likely a TypeLoadException runtime error because the type moved and you're missing the reference?

Tratcher · 2021-04-23T16:39:03Z

And likely a TypeLoadException runtime error because the type moved and you're missing the reference?

Updated. Actually getting a TypeLoadException would be tough, I wasn't able to trigger one in testing. If you had a nuget dependency you would at least have the old type. You'd have to manually copy your old library dll to a project that only referenced the new Extensions.Features and not the shared framework.

Tratcher · 2021-04-23T20:37:32Z

Reviewers: This should be ready. We think the Code Check issue is a false positive that @JunTaoLuo will review and override as needed.

halter73

I imagine testing just how breaking this is will be easier after we merge it. We can adjust/revert later.

davidfowl · 2021-04-23T20:42:37Z

I imagine testing just how breaking this is will be easier after we merge it. We can adjust/revert later.

Right, we just need to document all the things.

benaadams · 2021-04-23T20:45:27Z

src/Http/Features/src/PublicAPI.Shipped.txt

@@ -0,0 +1,2 @@
+#nullable enable


Should PublicAPI.Shipped.txt be changing?

Why? Nothing has shipped yet in this assembly. Did you think this was src/Http/Http.Features/? I wonder if we should rename the folders. That tripped me up earlier in the review too.

Well CodeCheck agrees with you. I guess it doesn't like the file at all even if empty. Still think the folder names are confusing.

@JunTaoLuo? This is a new assembly and the tools failed if it didn't have this file.

Why? Nothing has shipped yet in this assembly.

That's why I was asking why it was changing

Note Code Check is complaining about different files because they were deleted:

error : Modified API baseline files: error : src/Http/Http.Features/src/PublicAPI/net6.0/PublicAPI.Shipped.txt error : src/Http/Http.Features/src/PublicAPI/netstandard2.0/PublicAPI.Shipped.txt

Once you get the CI to green ignoring the Code Check job, let @dotnet/aspnet-build know and we'll override the required checks to squishy-merge your change.

From build perspective, the changes here make sense and simplify our PublicAPI files.

dougbu · 2021-04-23T22:12:11Z

src/Http/Features/src/PublicAPI.Shipped.txt

@@ -0,0 +1,2 @@
+#nullable enable


From build perspective, the changes here make sense and simplify our PublicAPI files.

dougbu · 2021-04-23T22:12:47Z

src/Http/Http.Features/src/Microsoft.AspNetCore.Http.Features.csproj

-  <ItemGroup>
-    <AdditionalFiles Include="PublicAPI/$(TargetFramework)/PublicAPI.Shipped.txt" />
-    <AdditionalFiles Include="PublicAPI/$(TargetFramework)/PublicAPI.Unshipped.txt" />
-  </ItemGroup>


dougbu · 2021-04-23T22:22:33Z

the Code Check issue is a false positive

Not exactly a "false positive" but a case where the error should be overridden.

Now that I think about it, is / are the relevant PublicAPI.Shipped.txt file or files moving along with the existing assembly name❔ Should still have files containing exactly what we shipped in 5.0 even if the project moved. That is, use *REMOVED* if necessary rather than letting VS edit the shipped files.

Tratcher · 2021-04-23T22:48:52Z

/azp run

azure-pipelines · 2021-04-23T22:49:15Z

Azure Pipelines successfully started running 2 pipeline(s).

Tratcher · 2021-04-24T02:00:06Z

Now that I think about it, is / are the relevant PublicAPI.Shipped.txt file or files moving along with the existing assembly name❔ Should still have files containing exactly what we shipped in 5.0 even if the project moved. That is, use *REMOVED* if necessary rather than letting VS edit the shipped files.

These files:
error : src/Http/Http.Features/src/PublicAPI/net6.0/PublicAPI.Shipped.txt
error : src/Http/Http.Features/src/PublicAPI/netstandard2.0/PublicAPI.Shipped.txt

Were recently added in 6.0 by #31625. We're removing them now that we've found a better solution.

The 5.0 APIs are still covered by src/PublicAPI.Shipped.txt

@JunTaoLuo @dougbu this is ready to merge.

PMExtra · 2024-11-26T08:30:59Z

Why don't we change its namespace?

The reason for extracting it from the original library is that it is, in fact, quite generic, which is also why the package is named Microsoft.Extensions.Features instead of something that contains AspNetCore or Http. However, I don't understand why it wasn't migrated to a corresponding namespace.

In fact, I am writing a message-bus library that borrows from ASP.NET Core's middleware design and provides a MessageHandlingContext. I reused the Microsoft.Extensions.Features library, which works awesome, but the namespace Microsoft.AspNetCore.Http might be confusing.

davidfowl · 2024-11-26T08:40:01Z

Because a breaking change (for cosmetic reasons) like that are world ending for a product of this size and maturity. It's just not worth the tradeoff. If we invent a way to rename without breaking then I'd agree with you.

PMExtra · 2024-11-26T09:14:28Z

I don’t quite understand which compatibilities have been broken.

I tend to think that Microsoft.Extensions.Features is primarily used internally within AspNetCore, and any internal compatibility breaks should be solvable by synchronizing the release of new versions for both.

Externally, I noticed that the discussion above repeatedly mentioned type-forward. I believe this is the solution to the problem. We can keep the current definitions in the original namespace, but mark them as [Obsolete], and then add a series of forward types in the correct namespace. These forward types would be empty and inherit from the original definitions. Then we could consider completely moving those obsolete definitions after a few major version transitions.

On the other hand, I’ve also hoped that .NET would offer a feature similar to export SomeThing from Another.Namespace;, which is common in NodeJS or Python. I believe this could solve such problems very effectively.

davidfowl · 2024-11-26T16:12:10Z

Agree, if we invent features to make this scenario work then we can consider it, until that day comes, we're not going to rename namespaces.

Move IFeatureCollection to new Extensions.Features assembly

7d6f155

Tratcher added the breaking-change This issue / pr will introduce a breaking change, when resolved / merged. label Apr 21, 2021

Tratcher added this to the 6.0-preview5 milestone Apr 21, 2021

Tratcher self-assigned this Apr 21, 2021

Tratcher commented Apr 21, 2021

View reviewed changes

src/Http/Http.Features/src/Microsoft.AspNetCore.Http.Features.csproj Outdated Show resolved Hide resolved

Update src/Http/Http.Features/src/Microsoft.AspNetCore.Http.Features.…

3cd5017

…csproj

Tratcher marked this pull request as ready for review April 21, 2021 23:17

Tratcher requested review from halter73, jkotalik and a team as code owners April 21, 2021 23:17

halter73 reviewed Apr 22, 2021

View reviewed changes

src/Http/Http.Features/src/Properties/AssemblyInfo.cs Show resolved Hide resolved

davidfowl mentioned this pull request Apr 22, 2021

Use default interfaces for a more x3 gain in getting and setting headers #31519

Merged

Pilchie added the area-runtime label Apr 22, 2021

pranavkm reviewed Apr 22, 2021

View reviewed changes

src/Http/Features/src/Microsoft.Extensions.Features.csproj Outdated Show resolved Hide resolved

Update Microsoft.Extensions.Features.csproj

1bd37ff

Regen props

b0b580f

halter73 approved these changes Apr 23, 2021

View reviewed changes

benaadams reviewed Apr 23, 2021

View reviewed changes

jkotalik approved these changes Apr 23, 2021

View reviewed changes

dougbu reviewed Apr 23, 2021

View reviewed changes

wtgodbe merged commit fc1f919 into dotnet:main Apr 26, 2021

Tratcher deleted the tratcher/split-features branch April 26, 2021 16:21

JunTaoLuo mentioned this pull request May 19, 2021

Arcade powered source build #32790

Merged

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023

Move IFeatureCollection to new Extensions.Features assembly #32043

Move IFeatureCollection to new Extensions.Features assembly #32043

Uh oh!

Conversation

Tratcher commented Apr 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

davidfowl commented Apr 21, 2021

Uh oh!

Tratcher commented Apr 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidfowl commented Apr 21, 2021

Uh oh!

halter73 commented Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

halter73 commented Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

davidfowl commented Apr 22, 2021

Uh oh!

Tratcher commented Apr 22, 2021

Uh oh!

davidfowl commented Apr 22, 2021

Uh oh!

Tratcher commented Apr 22, 2021

Uh oh!

davidfowl commented Apr 22, 2021

Uh oh!

benaadams commented Apr 22, 2021

Uh oh!

davidfowl commented Apr 22, 2021

Uh oh!

benaadams commented Apr 22, 2021

Uh oh!

davidfowl commented Apr 22, 2021

Uh oh!

Tratcher commented Apr 22, 2021

Uh oh!

davidfowl commented Apr 22, 2021

Uh oh!

benaadams commented Apr 22, 2021

Uh oh!

pranavkm commented Apr 22, 2021

Uh oh!

Uh oh!

Tratcher commented Apr 22, 2021

Uh oh!

pranavkm commented Apr 22, 2021

Uh oh!

JamesNK commented Apr 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benaadams commented Apr 23, 2021

Uh oh!

davidfowl commented Apr 23, 2021

Uh oh!

Tratcher commented Apr 23, 2021

Uh oh!

Tratcher commented Apr 23, 2021

Uh oh!

halter73 left a comment

Choose a reason for hiding this comment

Uh oh!

davidfowl commented Apr 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Tratcher commented Apr 21, 2021 •

edited

Loading

Tratcher commented Apr 21, 2021 •

edited

Loading

halter73 commented Apr 22, 2021 •

edited

Loading

halter73 commented Apr 22, 2021 •

edited

Loading

JamesNK commented Apr 23, 2021 •

edited

Loading