AndroidShimResponseCache: Dealing with bad ResponseCaches

[nfuller]

Opening for discussion. Given OkHttp doesn't officially support ResponseCache via the public APIs any more I appreciate this may not be easily fixable.

This bug was raised against Lollipop:
https://code.google.com/p/android/issues/detail?id=160522

AndroidShimResponseCache is the reincarnation of some of the code mentioned in the bug.
https://github.com/square/okhttp/blob/master/okhttp-android-support/src/main/java/com/squareup/okhttp/AndroidShimResponseCache.java#L58

Ultimately, the bug comes down to a ResponseCache implementation not returning the status line as a CacheResponse header with a null key as it is expected to do.

In recent versions of OkHttp used on Android (the ones after OkHttp stopped using ResponseCache internally) there are a few layers that convert back and forth from Java API and OkHttp classes to keep things working. This means the data is subject to more checks and conversions, so the absence of a status line is a problem. In earlier version I think this manifests as a -1 response code (and the API docs on Android state something along the lines that a malformed CacheResponse will not get the response code wrong). On recent versions of Android the lack of a status line causes a NullPointerException.

In order to work around this without an exception I would have to either invent a status line (i.e. 200 / HTTP/1.1) for a malformed cache response, or use an invalid status code (e.g. 0, but not -1, which is now forbidden by the Response builder code).

Since KitKat-era, OkHttp now adds its own checks for response code for whether a response is cacheable, so returning a 0 status code would lead to a cache miss, where previously (e.g. <= KitKat) it would have been a cache hit. That seems undesirable.

Using a status code of 200 (and the protocol HTTP/1.1 or HTTP/1.0) may also have implications, but I'm less clear on what they are.

I'm tempted to suggest we replace the NullPointerException with a more obvious exception that encourages developers to fix their response cache. This is in preference to inventing a status line / causing a miss. Replacing / fixing third-party ResponseCache implementations is likely to be comparatively easy for app developers and there is an implementation that would work on all versions of Android I know about (i.e. one where the response code is correctly recorded). I would have to update the documentation for Android to point out that it's no longer acceptable to miss out the status line on CacheResponse. I may have to do something anyway because Lollipop is effectively broken in this regard and that ship has sailed.

At a stretch, I could instead invent a new "magic" response code that OkHttp is happy to accept (e.g. resurrect -1 or some other invalid HTTP status code like 0) and change the builder / cache checks to treat it as cacheable, but this involves more changes to OkHttp.

Before I do anything, I was wondering what OkHttp would be happy to accept. Thoughts?

[swankjesse]

I like your recommendation. Throw an exception that makes it clear what needs to be fixed.

[swankjesse]

No further action here.