Fixed #1711

I think i missed the point that cookies can be Dict or CookieJar.
This fix will work for MozillaCookieJar.


Looks good to me. Thanks for reacting to this so fast! :cake:


I think there is no proper test case when `CookieJar` is provided as params.(And it's the reason that i missed that point before :( )
I added tase case that fails without this patch.
Thanks.


I thought @gazpachoking or I had commented the old way of merging these fairly explicitly to talk about accepting a CookieJar. I just can't seem to find what I remember writing. Something like `merge_cookies` or `merge_session_cookies`. 


Can we move this behavior into `cookiejar_from_dict`?


@kennethreitz It's easy to do so. But in my opinion, if `CookieJar` updated by another `CookieJar` happens in `cookiejar_from_dict`, it may harm originality(converting `dict` to `CookieJar`) of `cookiejar_from_dict`. (If that happens, method name may should be changed too)
Do you have any good idea about moving into `cookiejar_from_dict`?
What about moving whole logic into another new method like `merge_cookies`?


Sounds good to me :)


1. Added 'merge_cookies'. 
   I noticed that 'update' method is not implemented basic 'CookieJar'.
   So to use 'update' method in RequestsCookieJar, create `RequestsCookieJar` instance when merging cookies.
2. Changed test file.
   If new instance is created when merging cookie, `assert s.cookies is cj` in test fails even if cookies are still a custom one. So changed it to pass test.
   Please review this, thanks.


I don't think we can do cookie merging this way. People habitually use unusual `CookieJar`s on their `Session` objects: for example, `MozillaCookieJar` is very popular. We can't replace their chosen `CookieJar`, which this method does.


@Lukasa Is it good if i update that method not to create new instance?


@daftshady Yes, I think so. As you've pointed out, the basic `CookieJar` doesn't have a `.update()` method. But I think that's fine, we can either let the `AttributeError` bubble up or rethrow something nicer, e.g. `requests.exceptions.CookieJarError`.


@Lukasa I pushed updated commit. 'merge_cookies' method does not create new `CookieJar` anymore.


Comments inline.


@Lukasa Thanks for reviewing. 
I think build has been failed because 'httpbin' does not be responded in one test :(


@Lukasa I tried to unnecessary re-throw because you pointed out that :). Your `right code` is exactly same as my first version of code. I will attach fixed code. thanks.


Yeah, it's totally my fault, sorry. =(


@daftshady can you do a rebase? I'd love to get this merged in today if @Lukasa or @sigmavirus24 +1


I'm definitely +1, though I'd like it if @sigmavirus24 could give his input too. =)


So, this issue was not introduced by this pull request, but it looks like somewhere along the line cookies sent along with requests started getting persisted to session? https://github.com/kennethreitz/requests/blob/012f0334ce43fe23044fc58e4246a804db88650d/requests/sessions.py#L326

My feeling is that calling `session.request` should not alter the cookies for the session. (unless of course the response contains set-cookie header.) I'll open up a separate ticket about it, just wanted to poll here first to see if the change was purposeful and I just missed the discussion.

And FWIW I'm also +1 on this change.


I'll look at this when I get to a computer but I trust @gazpachoking 's judgment. Also, Chase can you pinpoint which commit started that? I'll have to see if I remember the context for it.


Looks like it was part of #1630. Specifically 39fe0068aa6577 I'll play around and open a separate ticket on this.

EDIT: Made #1728 for this.


Figured this was my fault somehow ;).

We can follow up over there.


@kennethreitz Sure, I rebased it.


Forgot to add my +1


@daftshady can you rebase? It's been a while :)


@kennethreitz yes it's been a while. I rebased it again :)