[BUG]: Request caching returns invalid responses for GET endpoints

stestagg commented 1 year ago

What happened?

The standard GET request caching (for example when calling repos.getContent), doesn't handle the case where the same resource is requested with different media types.

If you request a file with format 'raw', then a file with format 'json' (for example), then the content of the response will depend on whatever was most recently cached.

It could be argued that this is a bug/issue with the github rest api implementation (issuing the same etag for both formats), or some issue with browser header handling, but it's not clear from the documentation, or a quick look at the code, how to disable the caching behaviour for octokit rest methods.

I've put together a jsfiddle showing the behaviour: https://jsfiddle.net/qvtkmnwp/44/ (click go, with browser request caching disabled/enabled to see difference).

Versions

https://esm.sh/@octokit/rest@20.0.1

Relevant log output

▶️▶️▶️ WITH CACHE ENABLED 
Request 1 (raw): etag: W/"495a13f66a7b267e5a3d9b7ac795289c60d79003"
Request 2 (json): etag: W/"495a13f66a7b267e5a3d9b7ac795289c60d79003"
Request 1 data type: object (expecting string)
Request 2 data type: object (expecting object)

----- Request 1 data (raw) (expect text of file) ----
[object Object]

----- Request 2 data (json) (expect [object Object])----
[object Object]

▶️▶️▶️ WITH BROWSER CACHE DISABLED:
Request 1 (raw): etag: "495a13f66a7b267e5a3d9b7ac795289c60d79003"
Request 2 (json): etag: W/"495a13f66a7b267e5a3d9b7ac795289c60d79003"
Request 1 data type: string (expecting string)
Request 2 data type: object (expecting object)

----- Request 1 data (raw) (expect text of file) ----
# rest.js

> GitHub REST API client for JavaScript

[![@latest](https://img.shields.io/npm/v/@octoki

----- Request 2 data (json) (expect [object Object])----
[object Object]

Code of Conduct

[X] I agree to follow this project's Code of Conduct

github-actions[bot] commented 1 year ago

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

wolfy1339 commented 1 year ago

I can't find any code doing any cache handling on the Octokit side. This is all handled on GitHub's server side.

stestagg commented 1 year ago

I can't find any code doing any cache handling on the Octokit side.

I think this is ultimately the problem. The issue here falls into a bit of a gap in how caching is specified/implemented. It seems that If-None-Match etag comparisons aren't required to honour the Vary headers, and browsers don't do that for other reasons. So:

GH correctly populates the Vary header, and doesn't change the Etag value based on the Accept field
And browsers don't key their caches based on the values of the Vary header (for performance reasons it seems), which doesn't seem technically incorrect(https://stackoverflow.com/questions/60799116/if-none-match-headers-ignore-content-type-and-vary) when using etags, but isn't ideal.
GH then returns a 304 because the etag matches in both requests, and the browser serves the cached content (wrong content type)

So (as far as I can tell) there's no obvious finger of blame to point to any particular system, it's just a quirk of how REST APIs implemented like this expose this issue.

The result is that this client library appears to be wrong, as evidenced by the fiddle. Allowing the developer to control caching is fairly standard for a REST clients, as caching is always complex, and escape-hatches are needed way more often than feels ideal.

The underlying fetch/request APIs allow for cache behaviour to be controlled, but it's not clear through the rest.js code or the docs how to achieve this.

In the end, I was able to hack the issue by adding: headers: {'If-None-Match': ''}, to the method call, but this was only possible by stepping through the code to understand the injection points.

So maybe this is just a gap in the docs? or it might be nice to have a tiny addition to tune cache behaviour on a per-call basis?

nickfloyd commented 1 year ago

Hey @stestagg thanks for the legwork here! ❤️ Would you happen to have a code sandbox or a repo with source where you proved this out? I'd love to get, at least, some docs on this behavior.

As always, you're more than welcome to add the docs and submit a PR. I feel like your findings could potentially be beneficial to lots of folks!

wolfy1339 commented 1 year ago

There are docs that mention request options, right in the Node usage. https://octokit.github.io/rest.js/v19#custom-requests

stestagg commented 1 year ago

Hi @nickfloyd, thanks for the message!

There's a jsfiddle in the report: https://jsfiddle.net/qvtkmnwp/44/

The fiddle works for me in firefox and safari on osx (I can't see why it wouldn't reproduce in other envs too).

In essence, when calling (note the different format):

let res1 = await ok.repos.getContent({
    owner: 'octokit',
    repo: 'rest.js',
    ref: 'main',
    path: 'README.md',
    mediaType: {format: 'json'}
  });
let res2 = await ok.repos.getContent({
    owner: 'octokit',
    repo: 'rest.js',
    ref: 'main',
    path: 'README.md',
    mediaType: {format: 'raw'}
  });

One of res1 or res2 will hold the wrong value. (depending on what's cached :), also, often browsers will disable request caching when dev tools are open!!).

I did think of trying to contribute back with a fix, but honestly I'm not up to speed with the best knowledge here wrt browser caching. I found that adding: headers: {'If-None-Match': ''}, worked for me, but there is almost certainly a better way if this were to be done as a formal fix. I know there are cache control headers, and that there are rules on when/how they are allowed/enforced, but it's all a bit foreign to me.

Thanks

Steve

gr2m commented 1 year ago

I can't find any code doing any cache handling on the Octokit side

The only cache behavior we do is to throw an error in order to implement caching: https://github.com/octokit/request.js/blob/9e00cf5695f9e3bf100e2b9123c549b7fae9985e/src/fetch-wrapper.ts#L90-L100. But there is no official plugin for caching yet, there really should be though, it's probably one of the last recommended best practices that is not yet codified in the JavaScript Octokit SDK

octokit / rest.js