Server should provide meaningful `Cache-Control` header on cacheable endpoints

oliverlockwood commented 8 years ago

As described in https://github.com/cyclestreets/android/pull/142, the V2 APIs don't return meaningful cache-control headers, preferring to send Cache-Control:no-store, no-cache, must-revalidate.

~APIs which the Android app caches, which we should return meaningful cache-control for:~

~Some endpoints (/v2/pois.types and /v2/photomap.categories) return "validuntil": 1469190075 in the JSON body - where the validuntil field is a Unix timestamp 7 days in the future. It seems a no-brainer that these endpoints should return Cache-Control: max-age=604800 (or possibly Cache-Control: public, max-age=604800 where 604800 is the number of seconds in a week.~

Other APIs that I believe we probably should apply caching to:

~The Android app (this project!) also caches the blog post API (/blog/feed/) for 24 hours. This API doesn't currently return any Cache-Control header, but it could (though I appreciate that this may or may not be more debatable than the previous two!)~
Image downloads, https://www.cyclestreets.net/location/* (e.g. https://www.cyclestreets.net/location/1330/cyclestreets1330-size640.jpg) - I don't see why we wouldn't apply caching to these endpoints, especially since images are probably the largest things a client will access from the server!

~Other APIs that we could apply caching to~


-  ~~~`/api/journey.xml?plan=<routestyle>&itinerary=1234` - the retrieve previous itinerary API - if the app is likely to reload an itinerary e.g. on sleep / restore or whatever.~~~
-  ~~~`/v2/pois.locations`, `/v2/photomap.locations` - if any clients are likely to reload POIs _in the same geographic area_~~~

mvl22 commented 8 years ago

Will get the proper cache headers in soon-as (won't be before Monday though); agree this should be done there (even though the validuntil value should also still remain visible), as we want people to have to avoid the kind of workaround you mention for what should quite rightly be available at HTTP level.

mvl22 commented 7 years ago

APIs which the Android app caches, which we should return meaningful cache-control for

Both cache-control and expires are now sent in the response, whenever the validuntil token is present in the request body - e.g.:

Cache-Control: public, max-age=604800
Expires: Sun, 23 Apr 2017 15:38:51 +0100

Could you try now and let me know if these seem correct?

oliverlockwood commented 7 years ago

Looks like we're getting Cache-Control properly in the /v2/pois.types and /v2/photomap.categories endpoints. Good stuff! I've crossed out that section in the description of this issue.

@mvl22 Do you have an opinion as to whether or not we should address the other caching suggestions I made above?

mvl22 commented 7 years ago

Do you have an opinion as to whether or not we should address the other caching suggestions I made above?

In terms of each of the suggestions:

blog post API (/blog/feed/)

This is basically up to Wordpress, and I definitely don't have any desire to fiddle with its code.

Image downloads

I agree this would be sensible, and I'll add this to the ticket list for the server. I assume this doesn't need any changes at your end as presumably the HTTP library will just deal with these when such changes are made.

retrieve previous itinerary API

This might be worth doing, but in practice I think this would be rarely used, so I'm not proposing to change this at this time.

/v2/pois.locations, /v2/photomap.locations - if any clients are likely to reload POIs in the same geographic area

These would have to be the exact geographical area, and given that the area depends on the number of pixels a person drags the map to with their thumb, I think it's unlikely that would ever catch.

I'll have a think in general to what extent we should be adding caching to the API, as a lot of it relates heavily to data updates, so there are some potential opportunities there.

oliverlockwood commented 7 years ago

blog post API (/blog/feed/)

This is basically up to Wordpress, and I definitely don't have any desire to fiddle with its code.

Fine. I'll leave that caching workaround in place as-is.

Image downloads

I agree this would be sensible, and I'll add this to the ticket list for the server. I assume this doesn't need any changes at your end as presumably the HTTP library will just deal with these when such changes are made.

In the app, image downloads are done in the ImageDownloader class, which is separate from the general API client stuff. To implement caching, we'd have to configure the cache for that classes OkHttpClient instance, in a manner similar to what we do in the main API client. I'm happy to do this pro-actively - and potentially plumb in the same workaround mechanism with, say, a 7-day default cache - if there's an intention to move in this direction? All we'd need to decide is a maximum size for that cache, and then after that it would "just work" and later we could remove the interceptor workaround once the correct headers are being returned.

retrieve previous itinerary API

This might be worth doing, but in practice I think this would be rarely used, so I'm not proposing to change this at this time.

Agreed, fine by me to ignore this. Have crossed out accordingly.

/v2/pois.locations, /v2/photomap.locations - if any clients are likely to reload POIs in the same geographic area

These would have to be the exact geographical area, and given that the area depends on the number of pixels a person drags the map to with their thumb, I think it's unlikely that would ever catch.

Agreed, fine by me to ignore this. Have crossed out accordingly.

I'll have a think in general to what extent we should be adding caching to the API, as a lot of it relates heavily to data updates, so there are some potential opportunities there.

OK. I suggest we track those as new issues in here as and when you make those changes.

mvl22 commented 7 years ago

I'm happy to do this pro-actively - and potentially plumb in the same workaround mechanism with, say, a 7-day default cache

I'd prefer that clients don't implement caching that isn't specified by the API - this makes problems harder to debug.

I agree it would be sensible to move the code to using the more modern HTTP library, so that when caching is added, it will just work.

oliverlockwood commented 7 years ago

OK, I won't plumb in the workaround mechanism for the photo image downloader.

In terms of what size of image cache to keep... Based on a couple of test cases, 40KB seems like a reasonable average image size, so 2MB would allow for 50 images. I'll go for this as a rule of thumb unless you suggest otherwise.

cyclestreets / android