Introduce IBindableFromHttpContext<TSelf> #41100

DamianEdwards · 2022-04-08T00:13:26Z

Introduces the new IBindableFromHttpContext<TSelf> where TSelf : IBindableFromHttpContext<TSelf> interface that defines a static abstract BindAsync method.

Fixes #40927

DamianEdwards · 2022-04-08T15:42:02Z

@jmarolf after updating to Microsoft.CodeAnalysis.PublicApiAnalyzers 3.3.3 (from 3.3.0) we're getting a lot of new errors for existing APIs. Is this expected for a patch-level update?

src/Shared/ParameterBindingMethodCache.cs

jmarolf · 2022-04-08T18:45:40Z

@jmarolf after updating to Microsoft.CodeAnalysis.PublicApiAnalyzers 3.3.3 (from 3.3.0) we're getting a lot of new errors for existing APIs. Is this expected for a patch-level update?

whoops responded to you privately but should have just done so here. The change you are seeing was originally made in this PR six years ago: dotnet/roslyn-analyzers#984. At the time we considered this a bugfix on the grounds of "There are public APIs that we are not tracking that we need to track". We have taken a similar approach with the SDK analyzers in that bugfix releases are allowed to fail the build if the diagnostic should have always been reported.

BrennanConroy · 2022-04-08T19:32:31Z

We're also seeing other issues:

Forwarded types changed drastically, you now need to declare the entire surface area as forwarded, and likely even new APIs on the types would need to be copied here. Additionally, you have to have the correct generic name whereas before we didn't even need one, and there was no ctrl-. in VS for this.

-~Microsoft.AspNetCore.Http.Features.FeatureReference<> (forwarded, contained in Microsoft.Extensions.Features)
-~Microsoft.AspNetCore.Http.Features.FeatureReferences<> (forwarded, contained in Microsoft.Extensions.Features)
+Microsoft.AspNetCore.Http.Features.FeatureReference<T> (forwarded, contained in Microsoft.Extensions.Features)
+Microsoft.AspNetCore.Http.Features.FeatureReference<T>.Fetch(Microsoft.AspNetCore.Http.Features.IFeatureCollection! features) -> T? (forwarded, contained in Microsoft.Extensions.Features)
+Microsoft.AspNetCore.Http.Features.FeatureReference<T>.Update(Microsoft.AspNetCore.Http.Features.IFeatureCollection! features, T feature) -> T (forwarded, contained in Microsoft.Extensions.Features)
+static readonly Microsoft.AspNetCore.Http.Features.FeatureReference<T>.Default -> Microsoft.AspNetCore.Http.Features.FeatureReference<T> (forwarded, contained in Microsoft.Extensions.Features)
+Microsoft.AspNetCore.Http.Features.FeatureReference<T>.FeatureReference() -> void (forwarded, contained in Microsoft.Extensions.Features)
+Microsoft.AspNetCore.Http.Features.FeatureReferences<TCache> (forwarded, contained in Microsoft.Extensions.Features)
+Microsoft.AspNetCore.Http.Features.FeatureReferences<TCache>.Cache -> TCache? (forwarded, contained in Microsoft.Extensions.Features)
+Microsoft.AspNetCore.Http.Features.FeatureReferences<TCache>.Collection.get -> Microsoft.AspNetCore.Http.Features.IFeatureCollection! (forwarded, contained in Microsoft.Extensions.Features)
+Microsoft.AspNetCore.Http.Features.FeatureReferences<TCache>.FeatureReferences(Microsoft.AspNetCore.Http.Features.IFeatureCollection! collection) -> void (forwarded, contained in Microsoft.Extensions.Features)
+Microsoft.AspNetCore.Http.Features.FeatureReferences<TCache>.Fetch<TFeature, TState>(ref TFeature? cached, TState state, System.Func<TState, TFeature?>! factory) -> TFeature? (forwarded, contained in Microsoft.Extensions.Features)
+Microsoft.AspNetCore.Http.Features.FeatureReferences<TCache>.Fetch<TFeature>(ref TFeature? cached, System.Func<Microsoft.AspNetCore.Http.Features.IFeatureCollection!, TFeature?>! factory) -> TFeature? (forwarded, contained in Microsoft.Extensions.Features)
+Microsoft.AspNetCore.Http.Features.FeatureReferences<TCache>.Initalize(Microsoft.AspNetCore.Http.Features.IFeatureCollection! collection) -> void (forwarded, contained in Microsoft.Extensions.Features)
+Microsoft.AspNetCore.Http.Features.FeatureReferences<TCache>.Initalize(Microsoft.AspNetCore.Http.Features.IFeatureCollection! collection, int revision) -> void (forwarded, contained in Microsoft.Extensions.Features)
+Microsoft.AspNetCore.Http.Features.FeatureReferences<TCache>.FeatureReferences() -> void (forwarded, contained in Microsoft.Extensions.Features)
+Microsoft.AspNetCore.Http.Features.FeatureReferences<TCache>.Revision.get -> int (forwarded, contained in Microsoft.Extensions.Features)

REMOVED doesn't seem to be working anymore, or not as previously expected
- Before, nullable changes were added to unshipped and REMOVED was used for the previous API. Now it complains about the same API being defined already
Having both ~abstract Microsoft.AspNetCore.Mvc.ApplicationParts.ApplicationPart.Name.get -> string
and abstract Microsoft.AspNetCore.Mvc.ApplicationParts.ApplicationPart.Name.get -> string! in the Shipped file complains about the API being defined already. This one I'm actually sort of ok with because it cleans the file up, but still annoying when dealing with dozens of files that are now erroring.

dougbu · 2022-04-08T21:44:48Z

The change you are seeing was originally made in this PR six years ago: dotnet/roslyn-analyzers#984.

I'm also confused by this. We were at Microsoft.CodeAnalysis.PublicApiAnalyzers v3.3.0 before the small bump in this PR to v3.3.3. The dotnet/roslyn-analyzers#984 changes should have been part of every public release of that package (because those releases started in 2019). What led to us hitting the new behaviours at this time❔

dougbu · 2022-04-08T21:47:58Z

More generally, can we back up to v3.3.1 or v3.3.2 and get the fix that avoids NREs when parsing the IBindableFromHttpContext<TSelf> interface w/o dealing with these regressions in the analyzer❔

BrennanConroy · 2022-04-08T21:56:22Z

Or we could just accept the current changes needed and file issues for future improvements: #41115

DamianEdwards · 2022-04-08T21:58:00Z

More generally, can we back up to v3.3.1 or v3.3.2 and get the fix that avoids NREs

I tried that and it seems unfortunately only v3.3.3 fixes the stack overflow 😞

dougbu · 2022-04-08T22:12:58Z

Or we could just accept the current changes needed and file issues for future improvements: #41115

Except that seems to throw away our intent of keeping the PublicAPI.Shipped.txt files aligned w/ what we shipped in .NET 6.0.0 (modulo nullability) e.g. the additions in src/Http/Http.Features/src/PublicAPI.Shipped.txt. It also leaves us using an analyzer that doesn't seem to grok *REMOVED*, limiting our options going forward.

dougbu · 2022-04-08T22:14:45Z

Oh, bad example. That's the forwarded case.

BrennanConroy · 2022-04-08T22:14:53Z

e.g. the additions in src/Http/Http.Features/src/PublicAPI.Shipped.txt

What additions? New APIs are still going to be in unshipped.

dougbu · 2022-04-08T22:17:37Z

What additions? New APIs are still going to be in unshipped.

That file contains a bunch of +s that I didn't initially relate to your forwarded complaint.

dougbu · 2022-04-08T22:20:07Z

But my more important point was that *REMOVED* support is mandatory for how we use these files. Otherwise, might as well put everything in PublicAPI.Unshipped.txt files.

DamianEdwards · 2022-04-11T22:26:39Z

The code in this PR is ready for review, but is still blocked from merging pending #41115

src/Http/Http.Extensions/test/ParameterBindingMethodCacheTests.cs

src/Http/Http.Abstractions/src/IBindableFromHttpContextOfT.cs

DamianEdwards · 2022-04-18T18:51:01Z

Blocked by #41115

src/Http/Http.Abstractions/src/IBindableFromHttpContextOfT.cs

halter73

LGTM. I hope resolving the merge conflict and build issues won't be too much more effort.

src/Shared/ParameterBindingMethodCache.cs

dougbu · 2022-06-09T00:43:02Z

@DamianEdwards we just merged #41115 🎉 Resolve conflicts && you should be good to go.

(Many thanks to @BrennanConroy for his perseverance.)

src/Http/Http.Extensions/test/ParameterBindingMethodCacheTests.cs

BrennanConroy · 2022-06-09T21:57:47Z

src/Http/Http.Extensions/test/ParameterBindingMethodCacheTests.cs

@@ -1188,6 +1242,28 @@ public static void BindAsync(HttpContext context)
            => throw new NotImplementedException();
    }

+    private class BindAsyncFromImplicitStaticAbstractInterface : IBindableFromHttpContext<BindAsyncFromImplicitStaticAbstractInterface>


Add a test that has both IBindableFromHttpContext and an implemented BindAsync method (without Parameter) to see if it chooses the IBindableFromHttpContext.

Co-authored-by: Stephen Halter <[email protected]>

src/Components/Web.JS/dist/Release/blazor.server.js

src/Http/Http.Extensions/test/ParameterBindingMethodCacheTests.cs

dougbu · 2022-06-10T23:39:50Z

🥳 🎉 🎈 and so on❕

* Added IBindableFromHttpContext<TSelf> interface * Discover BindAsync via IBindableFromHttpContext interface * Added tests Co-authored-by: Stephen Halter <[email protected]>

DamianEdwards linked an issue Apr 8, 2022 that may be closed by this pull request

Introduce interface with static abstract BindAsync method for custom bound parameters of route handler delegates #40927

Closed

Pilchie added the old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Apr 8, 2022

DamianEdwards force-pushed the damianedwards/ibindablefromhttpcontext branch from 29ddebf to 77bc86e Compare April 8, 2022 17:01

DamianEdwards commented Apr 8, 2022

View reviewed changes

src/Shared/ParameterBindingMethodCache.cs Show resolved Hide resolved

dougbu mentioned this pull request Apr 9, 2022

Updated Microsoft.CodeAnalysis.PublicApiAnalyzers to latest stable #41115

Merged

DamianEdwards marked this pull request as ready for review April 11, 2022 22:25

DamianEdwards requested review from Tratcher, BrennanConroy, a team, dougbu, wtgodbe and Pilchie as code owners April 11, 2022 22:25

DamianEdwards requested a review from halter73 April 11, 2022 22:25

halter73 reviewed Apr 11, 2022

View reviewed changes

src/Http/Http.Extensions/test/ParameterBindingMethodCacheTests.cs Show resolved Hide resolved

halter73 reviewed Apr 14, 2022

View reviewed changes

src/Http/Http.Abstractions/src/IBindableFromHttpContextOfT.cs Outdated Show resolved Hide resolved

DamianEdwards requested a review from a team April 16, 2022 20:22

DamianEdwards added the * NO MERGE * Do not merge this PR as long as this label is present. label Apr 18, 2022

halter73 reviewed Apr 20, 2022

View reviewed changes

src/Http/Http.Abstractions/src/IBindableFromHttpContextOfT.cs Outdated Show resolved Hide resolved

halter73 approved these changes May 20, 2022

View reviewed changes

src/Shared/ParameterBindingMethodCache.cs Outdated Show resolved Hide resolved

BrennanConroy reviewed Jun 9, 2022

View reviewed changes

DamianEdwards requested a review from a team as a code owner June 9, 2022 23:22

DamianEdwards and others added 10 commits June 9, 2022 16:52

Added IBindableFromHttpContext<TSelf> interface

f1ff78a

Discover BindAsync via IBindableFromHttpContext interface

ebf426a

Refactoring

6761625

Refactor IBindableFromHttpContext discovery & add tests

13868fc

Replace LINQ use with a foreach loop + tests

50dc0fb

Another test

08daeb0

Added tests for structs implementing IBindableFromHttpContext<T>

08107c3

PR feedback

47cf7c2

Remove nullability from generic type arg on IBindableFromHttpContext

4beb219

Update src/Shared/ParameterBindingMethodCache.cs

a0263ba

Co-authored-by: Stephen Halter <[email protected]>

BrennanConroy reviewed Jun 10, 2022

View reviewed changes

src/Components/Web.JS/dist/Release/blazor.server.js Outdated Show resolved Hide resolved

Update public API

3e86f30

DamianEdwards force-pushed the damianedwards/ibindablefromhttpcontext branch from dbb8138 to 3e86f30 Compare June 10, 2022 17:28

DamianEdwards removed blocked The work on this issue is blocked due to some dependency * NO MERGE * Do not merge this PR as long as this label is present. labels Jun 10, 2022

Add tests from PR feedback

ec3a988

DamianEdwards enabled auto-merge (squash) June 10, 2022 20:58

BrennanConroy reviewed Jun 10, 2022

View reviewed changes

src/Http/Http.Extensions/test/ParameterBindingMethodCacheTests.cs Outdated Show resolved Hide resolved

Fix arg nullability

1afeeb8

DamianEdwards merged commit 6a8cc0d into main Jun 10, 2022

DamianEdwards deleted the damianedwards/ibindablefromhttpcontext branch June 10, 2022 23:33

ghost added this to the 7.0-preview6 milestone Jun 10, 2022

Introduce IBindableFromHttpContext<TSelf> #41100

Introduce IBindableFromHttpContext<TSelf> #41100

Uh oh!

Conversation

DamianEdwards commented Apr 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DamianEdwards commented Apr 8, 2022

Uh oh!

Uh oh!

jmarolf commented Apr 8, 2022

Uh oh!

BrennanConroy commented Apr 8, 2022

Uh oh!

dougbu commented Apr 8, 2022

Uh oh!

dougbu commented Apr 8, 2022

Uh oh!

BrennanConroy commented Apr 8, 2022

Uh oh!

DamianEdwards commented Apr 8, 2022

Uh oh!

dougbu commented Apr 8, 2022

Uh oh!

dougbu commented Apr 8, 2022

Uh oh!

BrennanConroy commented Apr 8, 2022

Uh oh!

dougbu commented Apr 8, 2022

Uh oh!

dougbu commented Apr 8, 2022

Uh oh!

DamianEdwards commented Apr 11, 2022

Uh oh!

Uh oh!

Uh oh!

DamianEdwards commented Apr 18, 2022

Uh oh!

Uh oh!

halter73 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dougbu commented Jun 9, 2022

Uh oh!

Uh oh!

BrennanConroy Jun 9, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dougbu commented Jun 10, 2022

Uh oh!

Uh oh!

DamianEdwards commented Apr 8, 2022 •

edited

Loading