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

Open
timofurrer opened this issue Jul 20, 2023 · 12 comments · May be fixed by #61473
Open

net/http: add Request.CookiesNamed #61472

timofurrer opened this issue Jul 20, 2023 · 12 comments · May be fixed by #61473

Comments

@timofurrer
Copy link

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 linked a pull request Jul 20, 2023 that will close this issue
@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
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
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.

@carlmjohnson
Copy link
Contributor

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

@timofurrer
Copy link
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
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
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
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.

6 participants