okhttp icon indicating copy to clipboard operation
okhttp copied to clipboard

OkHttp Wishlist

Open swankjesse opened this issue 7 years ago • 39 comments

This is an ongoing list of things we’d like to change by breaking API compatibility.

  • [ ] Allow Cache-Control durations to be longs PR 2797
  • [ ] OkHttpServer
  • [ ] Merge ConnectionSpec and HandshakeCertificates, call it HandshakePlan
  • [ ] Response’s body should be empty instead of null for the cacheResponse and networkResponse

swankjesse avatar Oct 12 '16 02:10 swankjesse

You can check cache-control as the PR is now closed.

jaredsburrows avatar Oct 24 '16 01:10 jaredsburrows

It's closed because it's a breaking change to 3.x and it's unchecked on this list because this is a list of breaking changes that we want.

On Sun, Oct 23, 2016, 9:05 PM Jared Burrows [email protected] wrote:

You can check cache-control as the PR is now closed.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/square/okhttp/issues/2903#issuecomment-255627904, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEEESl6fPA2iyQfNUXCqCaYGw6kwa0_ks5q3ARXgaJpZM4KUTnq .

JakeWharton avatar Oct 25 '16 18:10 JakeWharton

I would to see ping pong api public available. Related issue: https://github.com/square/okhttp/issues/2902.

gengjiawen avatar Dec 02 '16 06:12 gengjiawen

That doesn't require a breaking change. Please file a separate issue with your use case.

On Fri, Dec 2, 2016, 1:31 AM Jiawen Geng [email protected] wrote:

I would to see ping pong api public available. Related issue: #2902 https://github.com/square/okhttp/issues/2902.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/square/okhttp/issues/2903#issuecomment-264384089, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEEEam-rXznlXx-K9FMB2OhUYHQLO0Sks5rD7tPgaJpZM4KUTnq .

JakeWharton avatar Dec 02 '16 13:12 JakeWharton

Implement support for "stale-while-revalidate" (#1083). This would be a great help for implementing very responsive apps without the need to have a custom DB cache (and usually some RxJava magic to deliver a cache response until the refreshed response from server is received). Though this may be quite difficult to implement since the response would be delivered twice :-/.

DanielNovak avatar Dec 09 '16 16:12 DanielNovak

If you're taking requests... cache interceptors and/or pluggable storage implementations would help with security. IMHO, anywhere OkHttp writes to disk, ideally there is a hook for app developers to supply some other storage implementation (e.g., save the data to an already-existing encrypted data store, for HIPAA compliance and whatnot).

If there is a way that you would like me to help with those, let me know.

commonsguy avatar Dec 21 '16 18:12 commonsguy

@commonsguy internally it’s pluggable via FileSystem.java. But that might not work as we go towards fancier things like Relay which will read and write simultaneously.

swankjesse avatar Dec 21 '16 19:12 swankjesse

Any possible we can poke on graphql ? Okhttp of course can use graphql, but multiline string support isn't good in java, anything solution similar with sqldelight can be a good option. And it would be nice to have auto-complete function if we provide if graphql schema file in Android Studio.(For js, we have js-graphql-intellij-plugin )

I am now handing graphql with retrofit, but it's a bit of awkward.

gengjiawen avatar Jan 03 '17 08:01 gengjiawen

@gengjiawen Get involved in the Apollo project? It aims to provide SQL Delight-like support for GraphQL over OkHttp.

JakeWharton avatar Feb 11 '17 06:02 JakeWharton

Thanks for the plug, We're working on exactly what you are asking for over at Apollo-Android. Code gen for the messy parts, Retrofit for the networking. Come join the fun, there's a working sample already and AutoValue support is coming shortly. https://github.com/apollographql/apollo-android

digitalbuddha avatar Feb 11 '17 06:02 digitalbuddha

Would be great to allow Source/BufferedSource as a body of chunked response in MockWebServer.

Currently MockWebServer only accepts Buffer/String which does not let you simulate responses with ability to write more data on the fly i.e. SSE over HTTP.

This will be breaking change for the MockResponse because it'll have to store body as Source/BufferedSource and that'll break MockResponse.body() behavior and return type or it could return null if body is not instance of Buffer and then it could be done in 3.x.

This was also mentioned in OkHttpServer issue but if you're going to have both MockWebServer and OkHttpServer this seems to belong to the 4.0 Wishlist.

artem-zinnatullin avatar May 24 '17 23:05 artem-zinnatullin

Another thing that probably deserves separate issue for discussion is async non-blocking Interceptors API based on callbacks rather than on synchronous method calls. This would work great with reactive libraries.

Currently if custom Interceptor can block Thread for relatively long time (i.e. session token is not ready yet so request can not be executed) the option to avoid that is to remove this logic from Interceptor and write it somewhere closer to application business logic which can greatly complicate things.

artem-zinnatullin avatar May 24 '17 23:05 artem-zinnatullin

I would really like to see WebSocketListener be an interface rather than an abstract class. Maybe have an abstract version for convenience. Why was the decision made for it to be an abstract class in the first place?

NoahAndrews avatar Jun 15 '17 03:06 NoahAndrews

It's too late to change. If we were to change it in OkHttp 4, what’s the benefit of using an interface?

swankjesse avatar Jun 15 '17 04:06 swankjesse

Dispatcher’s maxRequestsPerHost setting is more appropriate for HTTP/1.1. We may want to revisit for HTTP/2.

swankjesse avatar Aug 18 '17 01:08 swankjesse

I wish can receive Server Sent Event in OKHttp 4.0

allen026 avatar Sep 07 '17 09:09 allen026

You can do that currently if you parse the body format yourself. And we could provide a parser in 3.x. I don't think there's anything tied to 4.0 for that request.

On Thu, Sep 7, 2017 at 5:08 AM Allen [email protected] wrote:

I wish can receive Server Sent Event in OKHttp 4.0

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/square/okhttp/issues/2903#issuecomment-327739942, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEEEfH-SdLwmKv4yfRsbHmzEf4EgVJOks5sf7J6gaJpZM4KUTnq .

JakeWharton avatar Sep 07 '17 13:09 JakeWharton

@swankjesse answering your question to @NoahAndrews (June 15th 2017) the benefit of an abstract class over an interface would be (and this is just an example) that you could make an Android service to be "WebSocketListener" (given the lack of multiple inheritance, because an Android service already has to extend from another base class). Right now one needs to create a separate class, make it inherit from WebSocketListener, instantiate it, and wire it all together in a service, und so weiter. In the current state, the fact that WebSocketListener is an abstract class with pure virtual methods without any implementation whatsoever does not seem to bring any visible added value to the table, and it's hard to see why it could not be an interface instead. But then again, maybe I'm missing something that's not immediately obvious, and would love to learn more; this is not a criticism, just an observation.

akosma avatar Jun 29 '18 07:06 akosma

Is there a good way to implement the non-blocking I/O (i.e. java.nio or epoll or I/O Multiplexing) to save the number of threads when concurrently requesting, while preserving the interceptor design pattern? I like this design and API.

iTimeTraveler avatar Dec 05 '18 03:12 iTimeTraveler

Coroutines!

swankjesse avatar Dec 05 '18 04:12 swankjesse

kotlin ?

gengjiawen avatar Dec 05 '18 04:12 gengjiawen

For coroutines or other means of thread pooling to be helpful, Interceptors API will need to be non-blocking. Otherwise, interceptor can block execution thread for long time thus causing delays for other requests if thread pool is limited or causing additional thread spawns.

See also https://github.com/square/okhttp/issues/2903#issuecomment-303881386

artem-zinnatullin avatar Dec 06 '18 06:12 artem-zinnatullin

@commonsguy internally it’s pluggable via FileSystem.java. But that might not work as we go towards fancier things like Relay which will read and write simultaneously.

@swankjesse So... I'm able to use reflection to create an "EncryptingFileSystem.kt", and pass it into my builder, like so:

val okHttpClient = OkHttpClient.Builder()
        .cache(getCache(File(context.cacheDir.absolutePath + "/okhttp.encrypted"),10*1024*1024, key))
        .build()

This works, writing the iv to a separate file, and skipping encryption of the journal (not sure how appendingSink would interact), using the standard java cipher streams. (I was using CipherSink/CipherSource, but it appears there's some kind of issue that triggers Android's IV re-use check, along with forgetting to call response.close() in my test).

I'm sure you're aware this isn't exactly a new request (https://github.com/square/okhttp/issues/1605), so it would be really great if you could design and possibly test for this kind of use case and make the API for it public with 5.

mandrachek avatar Oct 27 '19 02:10 mandrachek

@mandrachek I’m surprised that doesn’t work. Our tests for this stuff all go through a fake file system. If there’s anything other than “non-public API” here I’d love to fix it for you immediately.

swankjesse avatar Oct 27 '19 04:10 swankjesse

@swankjesse Sorry, I edited my post about the same time as you replied - it actually does work. I wasn't calling response.close(), combine that with an android keystore incompatibility/bug in CipherSink/CipherSource (complaining about IV re-use)...and well, that was broken. I replaced CipherSink/CipherSource with CipherOutputStream/CipherInputStream, and ensured close() was being called, and it works now!

Posted a gist with the EncryptingFileSystem implementation. So, nothing wrong with Cache or FileSystem, was on my end. I exempted the journal from being encrypted - I don't think there's any benefit to encrypting it, and it's the only place that uses appendingSink, which I'm not sure would work well with the cipher streams.

mandrachek avatar Oct 27 '19 21:10 mandrachek

@mandrachek great! Agreed on not encrypting the journal. The URLs you hit are stored hashed in the journal, but they’re also on the file system directly. An attacker who has access to your cache but not your private key will be able to figure out what URLs you’ve visited.

What do you think about OkHttp offering an overload of Cache that takes a symmetric key? We could do encrypting internally and end-users like yourself would only need to provide the key. Our implementation would probably be similar to yours – wrap a file system to apply encryption.

swankjesse avatar Oct 28 '19 00:10 swankjesse

@swankjesse There's all sorts of factors at play with encryption that might make a one-size fits all approach not work so well - for example I'm using AES/GCM/NoPadding, generating my own 12byte IV (required for this transformation), while others may need to use a different transformation, a different sized IV (or use an IV generated automatically by the cipher), a different SecureRandom, a specific non-default JCE provider, or even asymmetric encryption.

This method is nice (to me anyway) because it's flexible and lets me leverage.the JCE -- I can use keys from the AndroidKeyStore where the actual key material is not exposed, unlike Realm, which does the encryption itself without the JCE (embedded native OpenSSL AES for better performance, using byte array as the key), and so which requires some hoop jumping (encrypt the key using key from AKS, store it on disk, decrypt with key from keystore, and pass to realm, or some similar set of steps).

Guess I could see you going that way too though, to optimize performance.

mandrachek avatar Oct 28 '19 02:10 mandrachek

Yeah. I think good security mostly comes by making it easy to do the right thing. Right now ~zero caches are encrypted. If we made it super easy, it’d be a lot more.

swankjesse avatar Oct 30 '19 03:10 swankjesse

Have you consider using Jetty or something of sorts or java.nio for the OkHttpServer ?

maiph avatar Dec 28 '19 00:12 maiph

@swankjesse is this list actively curated?

yschimke avatar Mar 10 '20 07:03 yschimke