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: unexpected behavour of client.Do() with cookie jar #36535

Closed
ololosha228 opened this issue Jan 13, 2020 · 6 comments · May be fixed by ololosha228/go#1 or #36537
Closed

net/http: unexpected behavour of client.Do() with cookie jar #36535

ololosha228 opened this issue Jan 13, 2020 · 6 comments · May be fixed by ololosha228/go#1 or #36537

Comments

@ololosha228
Copy link

@ololosha228 ololosha228 commented Jan 13, 2020

What version of Go are you using (go version)?

$ go version go1.12.7 linux/amd64

Does this issue reproduce with the latest release?

Yes, stable 1.13.6 version has same problem too.

What operating system and processor architecture are you using (go env)?

Any platform with any enviroment

What did you do?

code is here: https://play.golang.org/p/M7q1GhoW1wV

So, for my opinion, the problem is in r.AddCookie() function, which don't check request header is nil and forcely call Headers methods. 🤔

What did you expect to see?

My projects must not panicing for situations like this 👍

i think, that request initializing with &http.Request{} instead http.NewRequest() is more convenient, cause http.NewRequest() does too many things. Declaration of request object looks more readable and simple.

What did you see instead?

They are panicing! 😮

ololosha228 added a commit to ololosha228/go that referenced this issue Jan 13, 2020
ololosha228 added a commit to ololosha228/go that referenced this issue Jan 13, 2020
ololosha228 added a commit to ololosha228/go that referenced this issue Jan 13, 2020
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 13, 2020

Change https://golang.org/cl/214557 mentions this issue: fix of #36535 issue

@smasher164

This comment has been minimized.

Copy link
Member

@smasher164 smasher164 commented Jan 14, 2020

If you initialize an http.Request without NewRequest, you must create the map yourself. This is Working As Intended.

@smasher164 smasher164 closed this Jan 14, 2020
@ololosha228

This comment has been minimized.

Copy link
Author

@ololosha228 ololosha228 commented Jan 14, 2020

i understand that now i need to call NewRequest(), but as i said, it is not obvious, and code becomes bloated. I wrote in detail why this is inconvenient and illogical, so I do not think that the problem is resolved by NewRequest().

you know, add 3 lines of checking code is not so bad proposal fmyo

@smasher164

This comment has been minimized.

Copy link
Member

@smasher164 smasher164 commented Jan 14, 2020

The issue is not with adding three lines of checking code to client.Do. The issue is that it sets a precedent for net/http to do nil-checking any functions that interact with a request. In general for Go, if a struct has members that need to be initialized, the responsibility is either for the package to provide a factory function like NewRequest or for the user of the package to initialize its members themselves.

@ololosha228

This comment has been minimized.

Copy link
Author

@ololosha228 ololosha228 commented Jan 14, 2020

I agree with you, didn't think about it at this first, this situation is really not a concern of stdlib. well, what if we can add some function similar to newrequest which will not require me parse the same uri twice time?

i mean, this is really important for me, cause i think, that same code must be simple and concise, adding line with call request initializer and handling error feels a bit overheaded.

@smasher164

This comment has been minimized.

Copy link
Member

@smasher164 smasher164 commented Jan 15, 2020

An alternative in your case could be to wrap the http.Client type, and override the Do method to initialize the request before actually doing the request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.