aspnetcore icon indicating copy to clipboard operation
aspnetcore copied to clipboard

Feature/httpendpoint

Open Phumi-codes opened this issue 1 year ago • 2 comments

Add HttpContext.Endpoint property

Overview of the Problem The goal was to introduce an Endpoint property directly on the HttpContext class. This change aims to make endpoint handling more intuitive and seamless, improving the developer experience by avoiding the need for extension methods like GetEndpoint and SetEndpoint.

Description

Changes to the HttpContext Class New Endpoint Property: Justification: Adding Endpoint as a direct property on HttpContext allows for more straightforward access to the current endpoint associated with the request. This change enhances usability and aligns with the way other intrinsic properties (like Request, Response) are accessed. Details: The Endpoint property uses the existing feature collection to fetch or initialize an IEndpointFeature instance, which stores the current endpoint.

Modifications to the DefaultHttpContext Class Implementation of Endpoint Property: Getter: Retrieves the Endpoint from the IEndpointFeature. If the feature doesn't exist, it initializes a new EndpointFeature instance. Setter: Sets the Endpoint on the IEndpointFeature. If the endpoint is null, it resets the feature accordingly. Justification: This approach maintains backward compatibility, as the existing feature mechanism is preserved. It ensures that endpoints are handled uniformly across different contexts and allows for easy future extensions.

Removal of the SetEndpoint Method from EndpointHttpContextExtensions Justification: With the Endpoint property now directly on HttpContext, the SetEndpoint method in the EndpointHttpContextExtensions class became redundant. Removing it reduces code duplication and potential confusion.

Adjustments to Unit Tests Updated Test Cases: Justification: The test cases were modified to align with the new Endpoint property implementation. Previously, the tests relied on the SetEndpoint method, but now they test the direct interaction with the Endpoint property. Details: The test ensuring that no IEndpointFeature is set on a context without a feature has been adjusted. Now, it verifies that an EndpointFeature is always created if accessed, even if the endpoint is set to null.

Any feedback would be appreciated

Fixes #50522

Phumi-codes avatar Aug 28 '24 08:08 Phumi-codes

ATM there are build errors, as the public API changes, but there's no entry for that change.

So please follow along How to use Microsoft.CodeAnalysis.PublicApiAnalyzers to fix the build erros. The needed files reside in src/Http/Http.Abstractions/src,

gfoidl avatar Aug 29 '24 19:08 gfoidl

@Phumi-codes please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@dotnet-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@dotnet-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@dotnet-policy-service agree company="Microsoft"

Contributor License Agreement

@dotnet-policy-service agree company="Retro Rabbit"

Phumi-codes avatar Sep 02 '24 08:09 Phumi-codes

Just noticed that this is a draft. Feel free to ignore my comments if they don't make sense yet.

amcasey avatar Sep 05 '24 21:09 amcasey

@Phumi-codes Thanks for opening this issue! Since this pull request introduces API changes, the associated issue will need to undergo API review first before we can merge this.

captainsafia avatar Sep 17 '24 09:09 captainsafia

@Phumi-codes Thanks for opening this issue! Since this pull request introduces API changes, the associated issue will need to undergo API review first before we can merge thi

@Phumi-codes Thanks for opening this issue! Since this pull request introduces API changes, the associated issue will need to undergo API review first before we can merge this.

Thank you for the update! Just to clarify, will the API review be conducted by the team, or is there anything specific I need to do to help initiate or prepare for the review? Please let me know if there's anything further required from my side. Thanks!

Phumi-codes avatar Sep 25 '24 07:09 Phumi-codes

The team usually review new APIs weekly.

martincostello avatar Sep 25 '24 08:09 martincostello

@Phumi-codes It looks like this needs a rebase now that the PublicAPI files have been shipped.

The current build is outdated so I haven't been able to evaluate if the test failures are legit or not. They seem to come from the template tests, so it's highly likely they are flakes.

captainsafia avatar Dec 16 '24 22:12 captainsafia

Hi @Phumi-codes. It seems you haven't touched this PR for the last two weeks. To avoid accumulating old PRs, we're marking it as stale. As a result, it will be closed if no further activity occurs within 4 days of this comment. You can learn more about our Issue Management Policies here.

any update on this?

WeihanLi avatar Mar 06 '25 07:03 WeihanLi