Apollo URLRequest.CachePolicy not respecting Cache-Control header

[marksvend]

Summary

The fix for Issue 3432 introduced a new caching bug. Before this change, NSURLSession was using the default URLRequest.CachePolicy of .useProtocolCachePolicy. After the change, the default policy for using cached data changed to .returnCacheDataElseFetch.

The .returnCacheDataElseFetch value ignores the Cache-Control header of the document and does not ever expire the data. This leads to NSURLCache returning stale data long after data has expired. From Apple's documentation:

NSURLRequest.CachePolicy.returnCacheDataElseLoad
Use existing cache data, regardless or age or expiration date, loading from originating source only if there is no cached data.

Instead, it should be using .useProtocolCachePolicy:

NSURLRequest.CachePolicy.useProtocolCachePolicy

Use the caching logic defined in the protocol implementation, if any, for a particular URL load request.

Discussion
This is the default policy for URL load requests.

HTTP caching behavior
For the HTTP and HTTPS protocols, NSURLRequest.CachePolicy.useProtocolCachePolicy performs the following behavior:
If a cached response does not exist for the request, the URL loading system fetches the data from the originating source.
Otherwise, if the cached response does not indicate that it must be revalidated every time, and if the cached response is not stale (past its expiration date), the URL loading system returns the cached response.

Version

1.15.3

Steps to reproduce the behavior

I reproduced the issue by loading a query with Charles to intercept network requests. The first time the page loads with a clean cache, the network request loads returning a response containing a Cache-Control header with a max age of something like 60 seconds. After waiting several minutes and loading the same URL through the Apollo SDK, Charles does not log a new request on the network. Instead, the NSURLCache is returning stale data every time.

Logs

No response

Anything else?

I believe the code should be updated in JSONRequest to look like this instead:

  /// Convert the Apollo iOS cache policy into a matching cache policy for URLRequest.
  private var requestCachePolicy: URLRequest.CachePolicy {
    switch cachePolicy {
    case .returnCacheDataElseFetch:
      return .useProtocolCachePolicy
    case .fetchIgnoringCacheData:
      return .reloadIgnoringLocalCacheData
    case .fetchIgnoringCacheCompletely:
      return .reloadIgnoringLocalAndRemoteCacheData
    case .returnCacheDataDontFetch:
      return .returnCacheDataDontLoad
    case .returnCacheDataAndFetch:
      return .reloadRevalidatingCacheData
    }
  }

[calvincestari]

I believe URLRequest.CachePolicy.returnCacheDataElseLoad is being used as a default value because it matches ApolloClient.CachePolicy.returnCacheDataElseFetch which is the default Apollo request cache policy

Practically these are the same behaviour because the Apollo cache has no notion of expiration, so whatever is in the Apollo cache will always be returned before attempting a network request when combined with the default Apollo client cache policy. It should only be that if you disable the Apollo-side cache or manually clear it out, with the iOS HTTP cache still having that record, that you encounter this problem.

We don't have any equivalent of useProtocolCachePolicy, so I'm not sure how we match it to any of our existing Apollo cache policy types? 🤔

You're using GET for GraphQL requests right?

[marksvend]

In this code change, Apollo began setting a cachePolicy for the request:

request.cachePolicy = requestCachePolicy

Previously, it did not set a cachePolicy. So the NSURLRequest always used Apple's default value of useProtocolCachePolicy, which respects the expiration date. Apollo SDK was effectively using useProtocolCachePolicy prior to this change.

After this change, Apollo is now over-caching content indefinitely, causing stale data to show in the app. This was a new bug introduced by that code change.

We had a custom in-memory Apollo cache that did check for expiration, but we were still having reports of stale content in the app showing for months until we finally tracked down that this change caused it.

We've fixed this by overriding the Apollo cache policy in our app. I just don't think the default for NSURLRequest should be to ignore the caching headers indicated by the server response. It's not going to be obvious to other consumers of Apollo who need to respect cache time-to-live that they need to override this cache policy to get what Apple thinks should be the default behavior of NSURL caching.

[calvincestari]

Previously, it did not set a cachePolicy. So the NSURLRequest always used Apple's default value of useProtocolCachePolicy, which respects the expiration date. Apollo SDK was effectively using useProtocolCachePolicy prior to this change.

That's correct, but it also meant that there was no way to opt-out of the iOS caching behaviour - which is also a bug for requests that do not want any cached data served.

I just don't think the default for NSURLRequest should be to ignore the caching headers indicated by the server response.

I don't think NSURLRequest.CachePolicy should even have a value that completely disregards cache control values for the data in the cache..but that doesn't help us.

Given that if the Apollo iOS cache store had expiration features it would obey them I can support the argument that useProtocolCachePolicy is an equivalent default more-so than returnCacheDataElseLoad. This shouldn't change the behaviour that was implemented for #3432 either; I'll get a PR up for the change and let you know when it's been merged.

[github-actions]

Do you have any feedback for the maintainers? Please tell us by taking a one-minute survey. Your responses will help us understand Apollo iOS usage and allow us to serve you better.

[calvincestari]

@marksvend - apollographql/apollo-ios-dev#550 has been merged and will be included with the next release.