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: add Request.CookiesNamed #61472

Closed
timofurrer opened this issue Jul 20, 2023 · 16 comments
Closed

net/http: add Request.CookiesNamed #61472

timofurrer opened this issue Jul 20, 2023 · 16 comments

Comments

@timofurrer
Copy link
Contributor

timofurrer commented Jul 20, 2023

I'd like to propose to expose (by wrapping) the readCookies method on http.Request as CookiesNamed that takes a single argument which is the filter for the cookie name.

The function signature would be:

func (r *Request) CookiesNamed(name string) []*Cookie

... and the implementation would be as simple as something like this:

// CookiesNamed parses and returns the named HTTP cookies sent with the request
// or an empty slice if none matched.
func (r *Request) CookiesNamed(name string) []*Cookie {
	return readCookies(r.Header, name)
}

Problem statement

Modern browsers may send multiple cookies with the same name if they are present in the cookie storage and are allowed to send. See RFC 6265.

However, there is no convenient way so far to get a slice of Cookies that matches a specific name.
Today, there is only r.Cookies() to get all cookies from the Cookie header and r.Cookie(name) to get the "first" cookie from the Cookie header that matches the given name.

The proposed r.CookiesNamed(name) would solve that by providing a simple way to just read all cookies with a given name.

Proposed Implementation

I have a draft PR ready here, which I'd like to contribute for this proposal: #61473

Edits

@gopherbot gopherbot added this to the Proposal milestone Jul 20, 2023
@timofurrer timofurrer changed the title proposal: net/http: Expose ReadCookies() method for http.Request proposal: net/http: Expose ReadCookies(name) method for http.Request Jul 20, 2023
timofurrer added a commit to timofurrer/go that referenced this issue Jul 20, 2023
This commit implements the new API proposed with
golang#61472

This change set implements the ReadCookies function
on the net/http Request so that it's conveniently possible
to retrieve all cookies from the Cookie header that match
a given name.

It does so by just wrapping the already existing readCookies
function.

Closes golang#61472
timofurrer added a commit to timofurrer/go that referenced this issue Jul 20, 2023
This commit implements the new API proposed with
golang#61472

This change set implements the ReadCookies function
on the net/http Request so that it's conveniently possible
to retrieve all cookies from the Cookie header that match
a given name.

It does so by just wrapping the already existing readCookies
function.

Closes golang#61472
@neild
Copy link
Contributor

neild commented Jul 20, 2023

If we add this, perhaps call it CookiesNamed, to keep the cookie-fetching functions lexically adjacent?

This doesn't seem unreasonable; it's currently possible to get this information by using req.Cookies and looking for matches, but that does require parsing all cookies rather than just the ones of interest.

timofurrer added a commit to timofurrer/go that referenced this issue Jul 21, 2023
This commit implements the new API proposed with
golang#61472

This change set implements the CookiesNamed function
on the net/http Request so that it's conveniently possible
to retrieve all cookies from the Cookie header that match
a given name.

It does so by just wrapping the already existing readCookies
function.

Fixes golang#61472
@timofurrer timofurrer changed the title proposal: net/http: Expose ReadCookies(name) method for http.Request proposal: net/http: add Request.CookiesNamed Jul 21, 2023
@timofurrer
Copy link
Contributor Author

If we add this, perhaps call it CookiesNamed, to keep the cookie-fetching functions lexically adjacent?

Sounds good. I've renamed it in the proposal and in #61473

@AlexanderYastrebov
Copy link
Contributor

AlexanderYastrebov commented Jul 21, 2023

We recently wrote a routine to remove a cookie by name zalando/skipper#2410

func removeCookie(request *http.Request, name string) bool {
	cookies := request.Cookies()
	hasCookie := false
	for _, c := range cookies {
		if c.Name == name {
			hasCookie = true
			break
		}
	}

	if hasCookie {
		request.Header.Del("Cookie")
		for _, c := range cookies {
			if c.Name != name {
				request.AddCookie(c)
			}
		}
	}
	return hasCookie
}

This would be a bit simpler if we could get all cookies except cookie we'd like to remove (OTOH we would like to preserve hasCookie check to avoid touching headers if there is no match).
Another example is selecting cookies having common prefix.

With that in mind and considering that api for trivial case to get single cookie is there I am wondering if it would make sense to support predicate instead of exact match:

func (r *Request) CookiesByName(filter func(string) bool) []*Cookie

though it is not a common pattern, I could only find https://pkg.go.dev/reflect#Value.FieldByNameFunc and https://pkg.go.dev/go/ast#Filter

@timofurrer
Copy link
Contributor Author

timofurrer commented Jul 22, 2023

@AlexanderYastrebov I totally see your point. However, I'm wondering where to draw the line between what the library should provide in terms of filter functionality and what it shouldn't.

For example, if a function like the one you provided would exist, why isn't there one where I could filter for the cookie value? (EDIT: okay, maybe because all cookies would need to be parsed anyways for that and you could just use Cookies() and then filter manually. However, that may be considered an implementation detail and the user wouldn't really care / want to know, but just expects the function)

I think that the proposed CookiesNamed provides a very low-maintenance / low-risk API for a standard use case in addition to Cookie(name string) - that is getting ALL cookies with a specific name (standard use case because the Cookie header may have multiple cookies with the same name, it's basically part of the protocol, but no convenient way to retrieve them without parsing all cookies).

Actually, CookiesNamed could be considered as a "confusion-free" replacement of Cookie(name string) which returns "just" one Cookie with the given name (pretty much randomly).
However, there may be more than just one with that given name. The current Cookie(name string) kinda suggest otherwise, which is wrong.

IMHO, the existing Cookies(name string) should behave like the proposed CookiesNamed(name string) and the former shouldn't exist, because it's just a source for errors.

@neild
Copy link
Contributor

neild commented Jul 23, 2023

The Cookies method returns all cookies. That's nice and simple, and you can filter out whatever you need from that list using whatever criteria you want.

A proliferation of cookie-fetching-and-filtering methods doesn't seem like a good idea. As @timofurrer says, where do we draw the line?

I think the argument in favor of CookiesNamed is fairly compelling, though: It's common to want the cookie with a specific name, but the Cookie method only returns a subset of data associated with that name. Adding a way to get all of it seems reasonable.

@earthboundkid
Copy link
Contributor

Another thought: should we wait to see if the iterator proposal lands and add a cookie iterator instead?

@timofurrer
Copy link
Contributor Author

Another thought: should we wait to see if the iterator proposal lands and add a cookie iterator instead?

@carlmjohnson I generally like the idea. However, I still think that the proposed CookiesNamed method is a standard-enough use case to have it's own method that works in the simplest way possible.

IMHO, the cookie iterator could be another proposal to solve problems like the one @AlexanderYastrebov described in #61472 (comment). WDYT?

@timofurrer
Copy link
Contributor Author

@neild has there been any progress on the proposal status? Is there anything I can assist with to get this over the line?

@gopherbot
Copy link

Change https://go.dev/cl/511516 mentions this issue: net/http: add Request.CookiesNamed

timofurrer added a commit to timofurrer/go that referenced this issue Sep 14, 2023
This commit implements the new API proposed with
golang#61472

This change set implements the CookiesNamed function
on the net/http Request so that it's conveniently possible
to retrieve all cookies from the Cookie header that match
a given name.

It does so by just wrapping the already existing readCookies
function.

Fixes golang#61472
@timofurrer
Copy link
Contributor Author

Since this one missed Go 1.12 I've retargeted the PR to Go 1.22. Any chance of getting this in?

timofurrer added a commit to timofurrer/go that referenced this issue Oct 2, 2023
This commit implements the new API proposed with
golang#61472

This change set implements the CookiesNamed function
on the net/http Request so that it's conveniently possible
to retrieve all cookies from the Cookie header that match
a given name.

It does so by just wrapping the already existing readCookies
function.

Fixes golang#61472
@rsc
Copy link
Contributor

rsc commented Nov 2, 2023

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

In net/http, add a new Request method CookiesNamed, so that the full set of cookie methods is:

func (r *Request) Cookies() []*Cookie // all cookies
func (r *Request) Cookie(name string) (*Cookie, error) // first with name, or ErrNoCookie
func (r *Request) CookiesNamed(name string) []*Cookie // all with name

@rsc
Copy link
Contributor

rsc commented Nov 10, 2023

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

In net/http, add a new Request method CookiesNamed, so that the full set of cookie methods is:

func (r *Request) Cookies() []*Cookie // all cookies
func (r *Request) Cookie(name string) (*Cookie, error) // first with name, or ErrNoCookie
func (r *Request) CookiesNamed(name string) []*Cookie // all with name

@rsc rsc changed the title proposal: net/http: add Request.CookiesNamed net/http: add Request.CookiesNamed Nov 10, 2023
@rsc rsc modified the milestones: Proposal, Backlog Nov 10, 2023
timofurrer added a commit to timofurrer/go that referenced this issue Jan 8, 2024
This commit implements the new API proposed with
golang#61472

This change set implements the CookiesNamed function
on the net/http Request so that it's conveniently possible
to retrieve all cookies from the Cookie header that match
a given name.

It does so by just wrapping the already existing readCookies
function.

Fixes golang#61472
@odeke-em
Copy link
Member

@timofurrer thanks for the patience and for the hard work! I've just added some comments to your change and after addressing I shall review again and then if gucci, I shall approve; we are almost there and thank you!

timofurrer added a commit to timofurrer/go that referenced this issue Feb 26, 2024
This commit implements the new API proposed with
golang#61472

Iimplements a new method http.Request.CookiesName, that allows
retrieving all cookies that match the given name.

Fixes golang#61472
timofurrer added a commit to timofurrer/go that referenced this issue Feb 26, 2024
This commit implements the new API proposed with
golang#61472

Iimplements a new method http.Request.CookiesName, that allows
retrieving all cookies that match the given name.

Fixes golang#61472
timofurrer added a commit to timofurrer/go that referenced this issue Feb 28, 2024
This commit implements the new API proposed with
golang#61472

Implements a new method http.Request.CookiesName, that allows
retrieving all cookies that match the given name.

Fixes golang#61472
@dmitshur dmitshur modified the milestones: Backlog, Go1.23 Mar 20, 2024
@odeke-em
Copy link
Member

Reverted in CL https://go-review.googlesource.com/c/go/+/571277. @timofurrer once the revert lands, please prepare your CL again rebased from the latest master.

@odeke-em odeke-em reopened this Mar 20, 2024
@gopherbot
Copy link

Change https://go.dev/cl/571278 mentions this issue: doc/go1.23: document "net/http".Request.CookiesNamed method

gopherbot pushed a commit that referenced this issue Mar 20, 2024
CL 511516 added the method but didn't include a release note for it
because it was authored and tested before the new release note flow.

For #61472.

Change-Id: I38f73e97093a2badaea658ed430e174b73e35b3a
Reviewed-on: https://go-review.googlesource.com/c/go/+/571278
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Commit-Queue: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
@odeke-em
Copy link
Member

Actually nevermind, @dmitshur helped us send a fix to add the api update in commit 364687b. Thank you Dmitri!

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.

8 participants