Skip to content

Added support for type based parameter binding #35496

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 19, 2021

Conversation

davidfowl
Copy link
Member

@davidfowl davidfowl commented Aug 19, 2021

  • Added a convention that allows custom async binding logic to run for parameters that have a static BindAsync method that takes an HttpContext and return a ValueTask of object. This allows customers to write custom binders based solely on type (it's an extension of the existing TryParse pattern).
  • There's allocation overhead per request once there's a parameter binder for a delegate. This is because we need to box all of the arguments since we're not using generated code to compute data from the list of binders.
  • This also supports optionality like everything else since we're always boxing using object.
  • Changed TryParse tests to BindAsync tests and added more tests.

Usage:

var builder = WebApplication.CreateBuilder(args);
var app = builder.Build();

app.MapGet("/webhook", (CloudEvent cloudEvent) =>
{
    redis.Publish(cloudEvent);
});

app.Run();

public class CloudEvent
{
    public static async ValueTask<object?> BindAsync(HttpContext context)
    {
        return await CloudEventParser.ReadEventAsync(context)
    }
}

- Added a convention that allows custom async binding logic to run for parameters that have a static BindAsync method that takes an HttpContext and return a ValueTask of object. This allows customers to write custom binders based solely on type (it's an extension of the existing TryParse pattern).
- There's allocation overhead per request once there's a parameter binder for a delegate. This is because we need to box all of the arguments since we're not using generated code to compute data from the list of binders.
- Changed TryParse tests to BindAsync tests and added more tests.
@davidfowl davidfowl added feature-minimal-actions Controller-like actions for endpoint routing old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Aug 19, 2021
@davidfowl davidfowl changed the base branch from main to release/6.0-rc1 August 19, 2021 19:12
@davidfowl davidfowl requested review from dougbu and a team as code owners August 19, 2021 19:12
@davidfowl davidfowl changed the base branch from release/6.0-rc1 to main August 19, 2021 19:15
@davidfowl
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@davidfowl davidfowl enabled auto-merge (squash) August 19, 2021 20:38
@davidfowl davidfowl merged commit 1329a6f into main Aug 19, 2021
@davidfowl davidfowl deleted the davidfowl/typebased-model-binding branch August 19, 2021 22:53
@ghost ghost added this to the 7.0-preview1 milestone Aug 19, 2021
@davidfowl
Copy link
Member Author

/backport release/6.0-rc1

@davidfowl
Copy link
Member Author

/backport to release/6.0-rc1

@github-actions
Copy link
Contributor

Started backporting to release/6.0-rc1: https://github.com/dotnet/aspnetcore/actions/runs/1148811785

@github-actions
Copy link
Contributor

@davidfowl backporting to release/6.0-rc1 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Added support for type based parameter binding - Added a convention that allows custom async binding logic to run for parameters that have a static BindAsync method that takes an HttpContext and return a ValueTask of object. This allows customers to write custom binders based solely on type (it's an extension of the existing TryParse pattern). - There's allocation overhead per request once there's a parameter binder for a delegate. This is because we need to box all of the arguments since we're not using generated code to compute data from the list of binders. - Changed TryParse tests to BindAsync tests and added more tests.
Using index info to reconstruct a base tree...
M	src/Http/Http.Extensions/src/RequestDelegateFactory.cs
M	src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs
M	src/Http/Http.Extensions/test/TryParseMethodCacheTests.cs
M	src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs
M	src/Shared/TryParseMethodCache.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Shared/TryParseMethodCache.cs
CONFLICT (content): Merge conflict in src/Shared/TryParseMethodCache.cs
Auto-merging src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs
CONFLICT (content): Merge conflict in src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs
Auto-merging src/Http/Http.Extensions/test/TryParseMethodCacheTests.cs
CONFLICT (content): Merge conflict in src/Http/Http.Extensions/test/TryParseMethodCacheTests.cs
Auto-merging src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs
CONFLICT (content): Merge conflict in src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs
Auto-merging src/Http/Http.Extensions/src/RequestDelegateFactory.cs
CONFLICT (content): Merge conflict in src/Http/Http.Extensions/src/RequestDelegateFactory.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Added support for type based parameter binding - Added a convention that allows custom async binding logic to run for parameters that have a static BindAsync method that takes an HttpContext and return a ValueTask of object. This allows customers to write custom binders based solely on type (it's an extension of the existing TryParse pattern). - There's allocation overhead per request once there's a parameter binder for a delegate. This is because we need to box all of the arguments since we're not using generated code to compute data from the list of binders. - Changed TryParse tests to BindAsync tests and added more tests.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

davidfowl added a commit that referenced this pull request Aug 19, 2021
* Added support for type based parameter binding
- Added a convention that allows custom async binding logic to run for parameters that have a static BindAsync method that takes an HttpContext and return a ValueTask of object. This allows customers to write custom binders based solely on type (it's an extension of the existing TryParse pattern).
- There's allocation overhead per request once there's a parameter binder for a delegate. This is because we need to box all of the arguments since we're not using generated code to compute data from the list of binders.
- Changed TryParse tests to BindAsync tests and added more tests.
@JesperTreetop
Copy link

Now that static members are allowed in interfaces, this kind of thing could be specified as an interface: (sharplab)

public interface ITypeBindAsync {
    static abstract ValueTask<object?> BindAsync(HttpContext context);
}

It wouldn't make it easier to call, though, since you can't exactly go Type type; if (type is ITypeBindAsync T) { return T.BindAsync(context); }. (Making it have a "curiously recurring" type parameter which almost all static abstract examples involve, ie giving it a TSelf, also wouldn't make it easier to call.)

But it would be easier and probably quicker to find out if a type is compliant or not - look for the type implementing the interface instead of looking for the method implementation. Then again, you still have to have the method to actually call it the way this is implemented now, through expression tree compilation. And of course, if you wanted the pattern to be usable without implementing an interface, that change couldn't be made.

Basically, the patch is probably the best way to do it, but it's fun to finally be able to think about statics in interfaces!

@ghost
Copy link

ghost commented Aug 21, 2021

Hi @JesperTreetop. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@amcasey amcasey added the area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc label Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-minimal-actions Controller-like actions for endpoint routing old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants