Skip to content
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

Feature/httpendpoint #57565

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Conversation

Phumi-codes
Copy link

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

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Aug 28, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Aug 28, 2024
@gfoidl gfoidl mentioned this pull request Aug 28, 2024
package-lock.json Outdated Show resolved Hide resolved
src/Http/Http.Abstractions/src/HttpContext.cs Outdated Show resolved Hide resolved
src/Http/Http/src/DefaultHttpContext.cs Outdated Show resolved Hide resolved
Phumi-codes and others added 5 commits August 29, 2024 20:33
Co-authored-by: Günther Foidl <gue@korporal.at>
Co-authored-by: Günther Foidl <gue@korporal.at>
Co-authored-by: Günther Foidl <gue@korporal.at>
…nsions.cs

Co-authored-by: Günther Foidl <gue@korporal.at>
@gfoidl
Copy link
Member

gfoidl commented Aug 29, 2024

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,

@Phumi-codes
Copy link
Author

@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"

@amcasey
Copy link
Member

amcasey commented Sep 5, 2024

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

@captainsafia
Copy link
Member

@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.

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Sep 24, 2024
@Phumi-codes
Copy link
Author

@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 Phumi-codes reopened this Sep 25, 2024
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-preview1 milestone Sep 25, 2024
@martincostello
Copy link
Member

The team usually review new APIs weekly.

@Phumi-codes Phumi-codes reopened this Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add HttpContext.Endpoint property
7 participants