Add session as optional parameter to safe_urlopen

[HazAT]

When we merge this we can close this PR
#4574

We will move it into a plugin
getsentry/sentry-plugins#135 (comment)

[mattrobenolt]

I don't see a reason to allow this. If we pass in a session explicitly, there's no point in using safe_urlopen. Just use the methods on your session directly.

[mitsuhiko]

@mattrobenolt but the methods on the session don't do what safe_urlopen does.

[mitsuhiko]

(The reason we want to pass in a session is because we pour cookies into the session cookie jar before calling this)

[mattrobenolt]

Like what? It was intended just to be a wrapper around the build_session() to prevent poking internal urls. Other than that, it doesn't do much. And if we're allowing passing in a Session, you could just bypass any of the value of "safe".

I see in the related PR that you're using build_session() directly. What can't you do just calling methods off of that?

[mitsuhiko]

@mattrobenolt i thought there was a reason for it. I see that it catches and reraises errors, that it sets a default encoding to utf-8 and sets a default timeout. Obviously we can duplicate that behavior but seems like that would be easy to miss then.

[mitsuhiko]

Original code comes from the WIP code here: #4574

[mattrobenolt]

Yeah, totally get what you're saying. I just don't particularly want to conflate safe_urlopen to allow something not safe. In this case, there's nothing preventing you from passing in your own Session that just does anything. I know you're not doing that, but it allows it.

I'd almost rather just have a urlopen function of our own that does all the stuff you mention, in which safe_urlopen calls. Then you can explicitly use urlopen instead to leave safe mean safe.

[mitsuhiko]

Summary from chat: we should probably make the build_session thing return a SafeSession subclass we can assert on, then make sure that safe_urlopen only ever gets a safe session.

Then maybe also rename build_session to build_safe_session and leave an alias for BW-compat behind.

[mitsuhiko]

Bonus points for moving as much logic as possible from safe_urlopen into the session directly.

[mitsuhiko]

Closed in favour of #4945