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

Adding a new Enhanced Connect :protocol pseudo-header #42002

Closed
Daniel-Genkin-MS-2 opened this issue Jun 2, 2022 · 2 comments
Closed

Adding a new Enhanced Connect :protocol pseudo-header #42002

Daniel-Genkin-MS-2 opened this issue Jun 2, 2022 · 2 comments
Assignees
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Milestone

Comments

@Daniel-Genkin-MS-2
Copy link
Contributor

Background and Motivation

I would like to propose adding a new protocol pseudo-header to HeaderNames. This will be helpful for adding support for new protocols to the stack that use the new Enhanced CONNECT method (e.g WebTransport (#41877) or WebSockets over HTTP/2 (#41558)). Currently there isn't such a pseudo-header. Therefore, we would need to hardcode it in the code currently. This proposed change would abstract the actual value into the HeaderNames file and thus help with maintainability as well as keep it more consistent with how the repo handles all the other headers and pseudo-headers.

Proposed API

/// <summary>Gets the <c>Protocol</c> HTTP header name.</summary>
public static readonly string Protocol = ":protocol";

Usage Examples

HeaderNames.Protocol

Alternative Designs

As this is a very tiny change, the only other reasonable possible designs are just to rename the variable or to place it in some other file. The proposed name, Protocol, seems to be the most consistent with the other values that are already defined and seems like all the pseudo-headers are already defined in this HeaderNames.cs file.

Risks

I don't think there are any risks here besides maybe choosing a bad variable name and confusing the customers who try to use it.

@Daniel-Genkin-MS-2 Daniel-Genkin-MS-2 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jun 2, 2022
@Tratcher Tratcher added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Jun 2, 2022
@ghost
Copy link

ghost commented Jun 2, 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.

@Tratcher Tratcher added this to the 7.0-preview6 milestone Jun 3, 2022
@halter73 halter73 removed the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Jun 6, 2022
@halter73
Copy link
Member

halter73 commented Jun 6, 2022

API Review Notes:

  1. Can this be used by code outside of Kestrel?
  • No.
  1. Can we make it internal?
  • Yes if we move the class and assembly for the ":protocol" psuedo-header name.
  1. Should we obsolete other pseudo header names?
  • Yes. Will need new API proposal.
  • At least make them non-browsable in the editor.

@halter73 halter73 added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jun 6, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 11, 2022
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

No branches or pull requests

5 participants