perf: make get requests cacheable

[blaggacao]

Context

• With the prior setting jquery always appends _=timestamp() to each GET request
• This busts all and any attempts at caching those requests
• The backend does not currently implement any cache control (actively denies to cache: https://github.com/frappe/frappe/pull/26737/files#diff-67ecddbbe3746cf9e7688078b93535b18e7c220c88a0aae0c4965de04c2a7cc1R260)
• Therfore even with cache enabled in jquery, nothing is cache by default

Proposed Solution

• Enable "the (still opt-in) possibility to configure cache" on specific GET requests by switching this flag

Additional Nginx config required

... to enable cache on a particular route

location "/api/method/erpnext.accounts.utils.get_fiscal_year" {
  proxy_hide_header Cache-Control;
  proxy_hide_header Set-Cookie;
  proxy_ignore_headers Cache-Control;
  proxy_ignore_headers Set-Cookie;
  add_header Cache-Control "max-age=2880";
  proxy_cache frappe;
  proxy_cache_valid 200 302 8h;
  proxy_cache_valid 404 1m;
  proxy_cache_lock on;
  add_header X-Cache-Status $upstream_cache_status;
  proxy_cache_key $scheme$host$request_uri;
}

Note: Firefox does cache this client side, ~~chrome apparently doesn't. Hence the proxy cache as fallback which is only suitable for non-private resources like fiscal year!~~ It does, apparently just not on a *.localhost domain?!? - In production, it does and that's what counts.

[blaggacao]

@akhilnarang @surajshetty3416 I'm ready to merge and own this change too.

We run it sucessfully in production together with https://github.com/frappe/frappe/pull/26753 ever since I opened this PR and with great results and speed ups.

I think it is prudent to incorporate this small change with some lead time ahead of https://github.com/frappe/frappe/pull/26753 to skim for unexpected side effects.

Do you know someone who should have a look at this from the team?

[akhilnarang]

@blaggacao so as-is this PR does nothing unless you modify the nginx configuration to explicitly cache a specific route, correct?

[blaggacao]

Correct, it removes the ...?_=timestamp() query string from GET (!) requests.

Prior to this PR, every GET request would have ...?_=timestamp(), e.g. ...?_=13627585175, appended by the client and thus would become uncachable by the proxy (except if you hack it in the proxy and transform the URL, removing that particular query string before calculating the cache key).

The client on the other hand, has no reason to (locally) cache these requests unless there is an appropriate Cache-Control header which would instruct it to do so.

I don't know the entire purpose of this setting, but it seems that it might be motivated to be used during development to ensure/force nothing is cached.

Writing down these considerations, I reckon, we could alternatively switch it only in production, wdyt? - So that even Cache-Control headers for local caching are made ineffective during development.

But I can add this back in the other PR (implementing Cache-Control) so that when someone reads the git history, it becomes clear in context why and when this setting here is flipped.

[ankush]

This is a bad idea :sweat_smile: It should be opted in for a few requests that can actually be cached. ALMOST every request is not cacheable.

This will introduce subtle bugs where the browser is deciding to use cached response when it really shouldn't. Think of all the interactive requests that happen that happen when you're modifying a document. This is why cache: false exists.

Thankfully default is POST but if it wasn't you'd have seen many many bugs in doctype pages.