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

Add CONNECT constant to HttpMethod #34195

Closed

Conversation

v-perfilev
Copy link
Contributor

@v-perfilev v-perfilev commented Jan 4, 2025

While reviewing issue #34044, I noticed that the HttpMethod class does not include a constant for the HTTP method CONNECT, which was introduced in RFC 7231. This PR adds the CONNECT constant to the HttpMethod class and updates the test class HttpMethodTests.

Changes in other files:

  • CONNECT method excluded in initAllowedHttpMethods of RequestMappingInfoHandlerMapping and in initAllowHeader of WebContentGenerator.

  • CONNECT method is not supported by HttpComponentsClientHttpConnector and JdkClientHttpConnector, so ClientHttpConnectorTests was updated to dynamically skip CONNECT for these connectors.

  • CONNECT method is primarily used for establishing tunnels, so it is not relevant for RequestMethod class and was excluded from RequestMethodTests.

These changes prepare the repository for a follow-up PR addressing issue #34044, where a conditional check will be introduced for WebSocket handshakes: if the HTTP version is HTTP/2 the method must be CONNECT, as specified in RFC 8441, otherwise it should be GET.

Replaced outdated references to HTTP 1.1 section links (RFC 2616)
with updated references to RFC 9110. The original document
has been superseded.
Introduced a new constant CONNECT with reference to RFC 9110.
Updated values array and valueOf method.
Modified the values test in HttpMethodTests to include
the CONNECT method.
The CONNECT method is not supported by HttpComponentsClientHttpConnector
and JdkClientHttpConnector.
The CONNECT http method is primarily used for establishing tunnels
and is not relevant for RequestMethod.
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 4, 2025
Updated initAllowedHttpMethods in WebContentGenerator and
RequestMappingInfoHandlerMapping to explicitly exclude the CONNECT
method.
@snicoll
Copy link
Member

snicoll commented Jan 6, 2025

Additionally the documentation references for existing constants in HttpMethod have been updated to point to the RFC 9110, as the previous links referenced a superseded document.

Please revert, this is unrelated to your proposal of adding CONNECT.

This reverts commit 5489d56 as it is unrelated to the addition
of CONNECT to HttpMethod.
@v-perfilev v-perfilev force-pushed the connnect-httpmethod-support branch from 822c6cf to 9a1a181 Compare January 6, 2025 09:41
@v-perfilev
Copy link
Contributor Author

Hello @snicoll !

Done: the commit with link updates has been reverted as requested.

  1. Would you like me to create a separate PR for this change, or should I leave it for now?
  2. Do the same 55-character length rule apply to revert commit messages?

@snicoll
Copy link
Member

snicoll commented Jan 6, 2025

Let's see where this PR leads us and we can revisit the other change. The rule (not sure where that comes from) is irrelevant as your commits are going to be squashed if we merge this.

@v-perfilev
Copy link
Contributor Author

@snicoll
Okay, I got it.
This rule comes from CONTRIBUTING.md:

Format commit messages using 55 characters for the subject line, 72 characters per line for the description.

@bclozel bclozel added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Jan 6, 2025
@bclozel
Copy link
Member

bclozel commented Jan 27, 2025

@v-perfilev I understand that CONNECT is an HTTP method defined in specs, but is there a use case in Spring Framework where we can send or receive HTTP CONNECT requests? I'm asking because adding it in HttpMethod has little value if we're ignoring this value totally in our HTTP support.

@bclozel bclozel added the status: waiting-for-feedback We need additional information before we can continue label Jan 27, 2025
@v-perfilev
Copy link
Contributor Author

v-perfilev commented Jan 27, 2025

Hi @bclozel !

I noticed that the CONNECT method was missing while trying to resolve the issue #34044. At the moment, WebSocket handshake over HTTP/2 is not supported in Spring Framework. According to RFC the CONNECT method must be used for HTTP/2 WebSocket handshakes instead of GET. The HttpMethod class is the most logical place to add this constant. Otherwise, we would need to define and use the CONNECT constant directly in AbstractHandshakeHandler in the following way:

private static final String CONNECT_HTTP_METHOD = "CONNECT"; // DEFINITION

@Override
public final boolean doHandshake(ServerHttpRequest request, ServerHttpResponse response,
        WebSocketHandler wsHandler, Map<String, Object> attributes) throws HandshakeFailureException {
    String protocol = request.getHeaders().getFirst("x-http2");
    boolean isHttp2 = "h2".equals(protocol);

    // some code

    if (isHttp2 && CONNECT_HTTP_METHOD != request.getMethod()) { // USAGE
        // some code
    }

    if (!isHttp2 && HttpMethod.GET != request.getMethod()) {
        // some code
    }

    // some code
}

This is not the only case where the CONNECT method is used. CONNECT is indispensable for building proxy-based and tunneling architectures, Spring Framework can also be used for.

Finally, having the CONNECT method ensures consistency within the industry standards. Similar rarely used method TRACE is already part of HttpMethod, and libraries like Apache HttpClient and Netty already support the CONNECT method.

Regards,
Vladimir

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jan 27, 2025
@bclozel
Copy link
Member

bclozel commented Jan 27, 2025

I'm not sure how we could produce CONNECT requests with RestTemplate or RestClient as this method is meant to be used by transport protocols, not high level clients.

As for #34044, I've already commented there that I don't think we can use this method in our support as Servlets do not support this HTTP method at the moment. I'll leave this opened for now until we hear from the reporter on #34044

@rstoyanchev
Copy link
Contributor

Thanks for the PR, but we are going to make a more targeted change in AbstractHandshakeHandler. It makes sense to support the method of upgrade in RFC 8441, but there is no intent to support HTTP CONNECT more widely.

@rstoyanchev rstoyanchev closed this Feb 3, 2025
@rstoyanchev rstoyanchev added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged or decided on status: feedback-provided Feedback has been provided labels Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants