CookieStore

[theduke]

This a very much WIP start for #14.

It contains a working implementation of the cookie store for the AsyncClient, an accessor on the Response and a wrapper type for Cookie but no tests and no methods on the CookieStore wrapper.

Before I go any further there are a few questions / discussion points:

• I've added the store to the client via a Option<RwLock<CookieStore>>, which seems most sensible to me since we need a read-lock before sending a request and a write lock after receiving.

• Naming: I added the cookies top level module. I'd prefer cookie but that's already the name of the dependency. I don't think there is a way to disambiguate a top level module and a dependency in the 2015 edition, is there? (would be possible with 2018 I think). We could rename the dependency to something, but that is a fairly new Rust feature.

• Is it OK to use impl Iterator<...> as a return value for Response::cookies? (I reckon this depends on the minimum supported Rust version) Otherwise we'd need a custom Iterator type or just return a Vec.

• Expiration: cookie::Cookie relies on the old time crate to handle expiration time. How would you like to handle this?

  • the canonical crate would be chrono, but that's a heavy new dependency.
  • time is probably not a good choice for a public dependency
  • the best Option might be a opaque ExpirationTime type that only converts to/from unix epoch as u64 (or std::time::SystemTime).

• Wrapping Cookie is easy, but CookieStore is a bit more awkward: we'd need something like a CookieRef type to represent a cookie borrowed from the store (for get, iter, ... methods)

• Should a Request / RequestBuilder have an easy way to manually add cookies? As in a RequestBuilder::cookie() method? It would be convenient. The only little stumbling block here is that duplicate cookie values already present in the store would need to be dropped. Also, what should be the behaviour when manually adding a Cookie header after adding some cookies with the builder? Drop the manually added ones? Merge together?

• As the TODO in the code states, must verify that the cookie::EncodedCookie type and Display implementation guarantees valid ASCII only header so the unwrap() is safe to use. ( it should )

• How much of the API surface from cookie_store::CookieStore is desirable to include? Especially regarding the load/save methods

[seanmonstar]

Thank you soooo much!

I don't think there is a way to disambiguate a top level module and a dependency in the 2015 edition, is there?

You can do extern crate cookie as cookie_crate; or whatever rename is appropriate.

Is it OK to use impl Iterator<...> as a return value for Response::cookies?

Sounds like a good idea to me! The async client already returns impl Future in some cases.

Expiration

I agree that we shouldn't expose time, nor pull in chrono. Do we need a special type, or can it work with just SystemTime?

Should a Request / RequestBuilder have an easy way to manually add cookies?

Those are some good questions, we could punt for now if you don't want to get bogged down figuring that out, simply having automatic session support is a big win!

How much of the API surface from cookie_store::CookieStore is desirable to include?

Preferably, the dependency itself shouldn't be exposed at all. That allows us to upgrade to new versions without it being breaking change for reqwest.

[theduke]

Thank you soooo much!

It really wasn't much work, the primary thanks should go to @pfernie for cookie_store.

@seanmonstar this is ready for review now.

I have punted on the CookieStore API for now and made it private, so all users can do for now is enable the cookie store via ClientBuilder::cookie_store(true);.

I have added a few tests that cover the most important functionality, seems like cookie_store does it's job well.

Quoting my commit message:

    Implement cookie store support

    This commit introduces a cookie store / session support
    for both the async and the sync client.

    Functionality is based on the cookie crate,
    which provides a HTTP cookie abstraction,
    and the cookie_store crate which provides a
    store that handles cookie storage and url/expiration
    based cookie resolution for requests.

    Changes:
    * adds new private dependencies: time, cookie, cookie_store
    * a new cookie module which provides wrapper types around
        the dependency crates
    * a Response::cookies() accessor for iterating over response cookies
    * a ClientBuilder::cookie_store() method that enables session functionality
    * addition of a cookie_store member to the async client
    * injecting request cookies and persisting response cookies
    * cookie tests

    NOTE: this commit DOES NOT expose the CookieStore itself,
    limiting available functionality.

    This is desirable, but omitted for now due to API considerations that should be fleshed out in the future.
    This means users do not have direct access to the cookie session for now.

[seanmonstar]

Super exciting stuff! Just a few comments inline.

[seanmonstar]

Thanks, this looks superb!

Hm, looks like CI shows 2 errors:

1. Looks like cookie-store uses 2018 edition, breaking reqwest's minimum rust version. I'm not that concerned about that, we can probably just bump it in the CI file (since it's still nice to know when an upgrade is required).
2. Something about the Android build blew up. I don't yet know why, I can look more in a bit.

[theduke]

I pushed a commit bumping the minimum version to 1.31 + also rebased.

[seanmonstar]

I've since learned that it's likely that reqwest's Android CI build isn't fully setup for cross compilation, so that could be something else we try to fix instead.

[seanmonstar]

Merged this in #488, thanks again!