Fix crash and simplify check for caching issues. #112

Closed
wants to merge 7 commits into
from

Projects

None yet

2 participants

@tobli
Collaborator
tobli commented Jan 11, 2017

Fixes #111.
Only issue a warning in the following case:

  • request is a GET
  • http status indicates a successful (potentially cacheable) response
  • the server doesn’t explicitly indicate the response shouldn’t be cached (e.g. a logged in page)
  • the page has no caching headers

The reasoning behind this is that this situation is likely a mistake (misconfigured server and/or forgetting to set cache headers in web app logic).

This commit does away with the following checks:

  • check for Pragma header - this is a request header, so has no effect on response cacheability
  • check for Content-Encoding header - this is irrelevant for cacheability, and looks like a copy-paste bug introduced in 11681eb (it matches the compressability check just below).
@tobli tobli Fix crash and simplify check for caching issues.
Fixes #111.
Only issue a warning in the following case:
* request is a GET
* http status indicates a successful (potentially cacheable) response
* the server doesn’t explicitly indicate the response shouldn’t be cached (e.g. a logged in page)
* the page has no caching headers

The reasoning behind this is that this situation is likely a mistake (misconfigured server and/or forgetting to set cache headers in web app logic).

This commit does away with the following checks:
* check for Pragma header -  this is a request header, so has no effect on response cacheability
* check for Content-Encoding header - this is irrelevant for cacheability, and looks like a copy-paste bug introduced in 11681eb (it matches the compressability check just below).
ef0526e
@tobli tobli requested review from soulgalore and micmro Jan 11, 2017
@tobli
Collaborator
tobli commented Jan 11, 2017

I'd love some extra eyes on this, since I didn't just fix the crasher, but reworked the meaning of the warning.

src/ts/helpers/heuristics.ts
const headers = harEntry.response.headers;
- return (!hasHeader(headers, "Content-Encoding") && isCachable(entry));
+ if (/no-cache/.test(getHeader(headers, "Cache-Control"))) {
@tobli
tobli Jan 11, 2017 Collaborator

I just realised that this check is redundant, since it's only the non-existence of caching headers that is an issue. Removing this if-statement will produce identical results.

@tobli
tobli Jan 11, 2017 edited Collaborator

I'll leave it in for now, but if we decide to merge this I'll force push an updated commit prior to the merge, that changes the commit message as well to reflect this simplification.

src/ts/helpers/heuristics.ts
+ if (/no-cache/.test(getHeader(headers, "Cache-Control"))) {
+ return false;
+ }
+ return !(hasHeader(headers, "Cache-Control") || hasHeader(headers, "Expires"));
@micmro
micmro Jan 11, 2017 Owner

Did you skip http 1.0's pragma on purpose?

@tobli
tobli Jan 11, 2017 Collaborator

Yep, its in the commit message

@micmro
micmro Jan 12, 2017 Owner

Oups sorry :) - my bad

@micmro
micmro approved these changes Jan 11, 2017 View changes
and others added some commits Jan 11, 2017
@micmro @tobli #95 encapsulte pagination state of multi-run HAR. 7404e34
@micmro @tobli fix comment fcb17e5
@tobli tobli Detect svg files as such, not generic images.
Update mime type check to classify image/svg+xml as svg, not a generic image. This fixes two issues:
- Svg images now show ”svg” when hovering over the icon (for now it reuses the same icon as ”image” files.
- Check for compression issues not applies to svg files as well.
6c20fbb
@micmro @tobli empty multi-run select box before population and type improvements f8cb316
@micmro @tobli Release v0.2.18 3e21998
@tobli tobli Fix crash and simplify check for caching issues.
Fixes #111.
Only issue a warning in the following case:
* request is a GET
* http status indicates a successful (potentially cacheable) response
* the page has no caching headers

The reasoning behind this is that this situation is likely a mistake (misconfigured server and/or forgetting to set cache headers in web app logic).

This commit does away with the following checks:
* check for Pragma header -  this is a request header, so has no effect on response cacheability
* check for Content-Encoding header - this is irrelevant for cacheability, and looks like a copy-paste bug introduced in 11681eb (it matches the compressability check just below).
d7c208a
@tobli
Collaborator
tobli commented Jan 12, 2017

Sorry, I messed up locally trying to amend the last commit with a minor tweak. Will close this, have opened #114 instead, and will merge that.

@tobli tobli closed this Jan 12, 2017
@tobli tobli deleted the check-for-cache-issues branch Jan 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment