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

net/http: allow multiple spaces between method and path in mux patterns #64910

Closed
egtann opened this issue Dec 31, 2023 · 18 comments
Closed

net/http: allow multiple spaces between method and path in mux patterns #64910

egtann opened this issue Dec 31, 2023 · 18 comments

Comments

@egtann
Copy link

egtann commented Dec 31, 2023

Proposal Details

When testing the muxpatterns implementation which is landing in the stdlib Mux in Go 1.22, I was surprised to find that I could not add extra whitespace between the method and path. For instance, this does not work:

r.Handle("GET  /my-route", handler) // note the 2nd space!

Allowing extra spaces is useful when you have many routes and want to keep the code neatly aligned, so it's easy to scan the routes visually. As a small example:

r.Handle("GET    /my-route", handler1)
r.Handle("POST   /my-route", handler2)
r.Handle("DELETE /my-route", handler3)
@gopherbot gopherbot added this to the Proposal milestone Dec 31, 2023
@seankhliao seankhliao changed the title proposal: net/http: Allow multiple spaces in mux routes proposal: net/http: allow multiple spaces between method and path in mux patterns Dec 31, 2023
@adonovan
Copy link
Member

adonovan commented Jan 2, 2024

"Be strict in what you accept" is the usual (contemporary) advice for interface design, and the API documentation is clear that it must be a single space. Perhaps @jba can offer more insight.

@jba
Copy link
Contributor

jba commented Jan 2, 2024

I can't see any reason to enforce the single space, and aligning patterns seems like a good reason to drop it.
Any arguments against allowing [ \t]+ between the two? (In English, at least one character of horizontal whitespace.)

@adonovan
Copy link
Member

adonovan commented Jan 2, 2024

Any arguments against allowing [ \t]+ between the two? (In English, at least one character of horizontal whitespace.)

This is consistent with HTTP itself, which also allows [ \t]+ between GET and the URL in the syntax for a (single-line) request.

@earthboundkid
Copy link
Contributor

Seems fine to me since the verbs can't have spaces anyway.

@AlexanderYastrebov
Copy link
Contributor

This is consistent with HTTP itself, which also allows [ \t]+ between GET and the URL in the syntax for a (single-line) request.

Is it? From https://datatracker.ietf.org/doc/html/rfc9112#name-request-line

A request-line begins with a method token, followed by a single space (SP), the request-target, and another single space (SP), and ends with the protocol version.
request-line = method SP request-target SP HTTP-version

@adonovan
Copy link
Member

adonovan commented Jan 7, 2024

Is it? From https://datatracker.ietf.org/doc/html/rfc9112#name-request-line

A request-line begins with a method token, followed by a single space (SP), the request-target, and another single space (SP), and ends with the protocol version.
request-line = method SP request-target SP HTTP-version

Right, but the definition of SP in 2.2 Basic Rules also says:

HTTP/1.1 header field values can be folded onto multiple lines if the continuation line begins with a space or horizontal tab. All linear white space, including folding, has the same semantics as SP. A recipient MAY replace any linear white space with a single SP before interpreting the field value or forwarding the message downstream.

which I took to mean that multiple spaces are allowed in the first line of the request. Perhaps I misread it, and this rule applies only to line continuations.

Empirically some servers do permit multiple spaces---not that that is a reliable guide.

$ { echo "GET    /    HTTP/1.1"; echo; } | nc google.com 80 | head -n 1 
HTTP/1.1 200 OK

@rsc
Copy link
Contributor

rsc commented Jan 26, 2024

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@TheCoreMan
Copy link

My gut says this is not a great idea, so I tried to back that up with some concrete cases where this will be a bad idea. And this is what I came up with:

  1. Gives room to "argue" in CRs on whether we should or shouldn't use spaces - better to keep it unambiguous
  2. The moment you have two routes with the same path and different verbs, you ought to extract the path to a constant anyways (even with one path it makes sense for usage in clients, tests, etc.). So there isn't a ton of style benefit in lining them up
  3. Autogenerators will want to update the spaces to allow lining them up, causing unrelated changes when adding a new verb, making code review harder (see this example; the GET change is unrelated to the actual verb we added):
- r.Handle("GET /my-route", handler1)
+ r.Handle("GET  /my-route", handler1)
+ r.Handle("POST /my-route", handler2)

@rsc
Copy link
Contributor

rsc commented Feb 2, 2024

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

The proposal is to accept multiple spaces in patterns like “GET /foo”.

@rsc
Copy link
Contributor

rsc commented Feb 8, 2024

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

The proposal is to accept multiple spaces in patterns like “GET /foo”.

@rsc rsc changed the title proposal: net/http: allow multiple spaces between method and path in mux patterns net/http: allow multiple spaces between method and path in mux patterns Feb 8, 2024
@rsc rsc modified the milestones: Proposal, Backlog Feb 8, 2024
@mibk
Copy link
Contributor

mibk commented Feb 9, 2024

Based on the emoji votes of #64910 (comment) and #64910 (comment), is there really a consensus?

@Malix-off
Copy link

Malix-off commented Feb 20, 2024

@rsc in #64910 (comment):

Based on the discussion above, this proposal seems like a likely accept.

The discussion above from @TheCoreMan was about

some concrete cases where this will be a bad idea

Thus, the upvotes are to highlight the cases where this proposal would be a bad idea, not to upvote the proposal.

@rsc in #64910 (comment):

consensus

Where?

There is no consensus I could find,
But if there were some, it is more towards a decline.

@Malix-off
Copy link

Malix-off commented Feb 20, 2024

In my opinion, it's not a bad idea if it doesn't slow down compilation parsing (would there be any way to test that?), and if gofmt and gofumpt apply the same rule to them at they apply for extra spaces in general.

@Malix-off
Copy link

@earthboundkid may I ask why you downvoted #64910 (comment)?
Did I miss something?

@mvdan
Copy link
Member

mvdan commented Feb 20, 2024

I don't think we should worry about parse performance at all. What matters is the performance for routing and serving HTTP requests, and that's not affected at all, as far as I can tell.

Personally I find the argument at #64910 (comment) compelling in terms of allowing any whitespace. I'm not as worried in terms of consistent formatting noise, to be entirely honest - the same arguments could be made against the way gofmt already aligns var () or const () blocks, for example. Another existing example is struct field tags, which also allow extra spaces like `json:"foo" yaml:"bar"`, and which can also be used for some form of alignment if a user wishes to.

Perhaps consider diff tools which support ignoring whitespace in some way. For example, GitHub already has a knob to ignore all whitespace changes like git diff -w, and Gerrit has similar settings as well. This is also useful when not wanting to see alignment noise due to gofmt, for example. Personally, I've just learned to visually skip over formatting changes relatively quickly, so I don't mind.

When it comes to the reactions or votes, my impression is they area helpful signal, but they shouldn't be used directly to measure consensus or reach a decision for a proposal. I personally value the opinion of the package maintainers above reactions, and in this case, both Jonathan and Russ (Neil hasn't commented) appear to lean in favor, and I'd tend to agree with them.

@Malix-off
Copy link

Malix-off commented Feb 20, 2024

@mvdan in #64910 (comment):

I don't think we should worry about parse performance at all

If it's marginal, then there is nothing to worry about.

@mvdan in #64910 (comment):

Personally I find the argument at #64910 (comment) compelling in terms of allowing any whitespace

Same.

@mvdan in #64910 (comment):

⋯reactions or votes ⋯ shouldn't be used directly to measure consensus or reach a decision for a proposal ⋯ I personally value the opinion of the package maintainers above reactions ⋯

Understood.

@earthboundkid
Copy link
Contributor

@Malix-off, you edited your comment, but before it said,

However, I surely hope gofmt and gofumpt strip-out those space

I think gofmt editing strings is a bad idea.

I don't really understand the opposition to this proposal, tbh. Seems harmless to me. I don't think I will do it, but I also wouldn't change a codebase I encounter that does.

@gopherbot
Copy link

Change https://go.dev/cl/565916 mentions this issue: net/http: allow multiple spaces between method and path in mux patterns

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

Successfully merging a pull request may close this issue.