Skip to content

Update RouteHandlerFilterContext.Parameters API #40514

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

Closed
captainsafia opened this issue Mar 3, 2022 · 5 comments · Fixed by #41406
Closed

Update RouteHandlerFilterContext.Parameters API #40514

captainsafia opened this issue Mar 3, 2022 · 5 comments · Fixed by #41406
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-perf Performance infrastructure issues 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
Milestone

Comments

@captainsafia
Copy link
Member

captainsafia commented Mar 3, 2022

Follow-up to #40491

- public sealed class RouteHandlerInvocationContext
+ public class RouteHandlerInvocationContext
{
-  public HttpContext HttpContext { get; }
+ public virtual HttpContext HttpContext { get; }

- public List<object?> Parameters { get; }
+ public virtual List<object?> Parameters { get; }

+ public virtual T GetParameter<T>(int index) { }
}
@captainsafia captainsafia 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 Mar 3, 2022
@captainsafia captainsafia self-assigned this Mar 3, 2022
@captainsafia captainsafia added this to the 7.0-preview3 milestone Mar 3, 2022
@captainsafia
Copy link
Member Author

Moving this back into .NET 7 Planning to prioritize completing the performance tests in #40655 for preview3.

@ghost
Copy link

ghost commented Mar 14, 2022

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@captainsafia
Copy link
Member Author

We've added performance tests for some basic scenarios with minimal APIs and route handler filters.

  • A route handler with no parameters and no filter
  • A route handler with no parameters and a "pass through filter"
  • A route handler with parameters and a "pass through filter"
  • A route handler with parameters and a filter

All the route handlers above tested on a string input/output for the route handler. While testing other input/output types might provide more "real world" values, the bare scenario is sufficient for observing the net impact of the barebones filters logic.

All told, adding a filter to an endpoint reduces throughput (measured in RPS) by about 9%. There's generally a bigger impact when adding parameters to an endpoint filter and parameter processing is a larger cost in the process than endpoint filters.

Regardless of the perf impact, IMO, this work is relevant to pursue because exposing parameters as an untyped list is 🤢 .

@rafikiassumani-msft rafikiassumani-msft added the area-perf Performance infrastructure issues label Apr 21, 2022
@captainsafia captainsafia added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Apr 28, 2022
@ghost
Copy link

ghost commented Apr 28, 2022

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
Copy link
Member

halter73 commented May 2, 2022

API Review Notes:

- public sealed class RouteHandlerInvocationContext
+ public abstract class RouteHandlerInvocationContext
{
-     public RouteHandlerInvocationContext(HttpContext httpContext, params object[] parameters)

-    public HttpContext HttpContext { get; }
+    public abstract HttpContext HttpContext { get; }

-     public IList<object?> Parameters { get; }
+     public abstract IList<object?> Arguments { get; }

+     public abstract T GetArgument<T>(int index) { }
}

+ public class DefaultRouteHandlerInvocationContext : RouteHandlerInvocationContext
+ {
+     public RouteHandlerInvocationContext(HttpContext httpContext, params object[] parameters)
+
+     public override HttpContext HttpContext { get; }
+     public override IList<object?> Arguments { get; }
+     public override T GetArgument<T>(int index) { }
+ }

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels May 2, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jun 3, 2022
@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 subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-perf Performance infrastructure issues 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 a pull request may close this issue.

4 participants