Repository navigation
SOLR-18615 don't cache admin resources - #5300
Conversation
| assert_output --partial "Cache-Control: no-cache" | ||
|
|
||
| run curl -s http://localhost:${SOLR_PORT}/solr/ | ||
| refute_output --regexp '(src|href)="[^"]+\.(js|css)"' |
There was a problem hiding this comment.
No lookahead in bash regex makes strict version of this validation ugly. Basically we are making sure there is some query string.
There was a problem hiding this comment.
Would this be better as a Selenium based test?
There was a problem hiding this comment.
Selenium tests build their own jetty and don't read the web.xml we actually ship in the distro (which is the actual source of the bug). I suppose we could have it parse the web.xml instead of handcrafting it in the tests but that is a bigger undertaking as some of the tests touch jetty internals directly (i.e. configureCluster). I am not sure if that is a direction worth taking? I don't have any experience with the selenium test so am looking for guidance. Theoretically it should be possible but not sure what roadblocks we could hit trying to parse the distro web.xml in the selenium tests rather than having it be a separate test resource. There is also the question of adding jetty-ee10-webapp as a dependency to the test framework.
There was a problem hiding this comment.
You could also compare the test configuration with the actual distro in a test to make sure they are the same but that seems messy.
There was a problem hiding this comment.
yeah, that doesn't seem worthe the effort! THe juice is not worth the squeeze!
| <script src="libs/d3.js?_=${version}"></script> | ||
| <script src="libs/ui-grid.min.js?_=${version}"></script> | ||
| <script src="libs/solr/index.js"></script> | ||
| <script src="libs/solr/index.js?_=${version}"></script> |
There was a problem hiding this comment.
explicit cache busters are still valuable, i.e. if your proxy strips cache-control header for some reason.
There was a problem hiding this comment.
feels like we should pick one or the other.... I mean, shouldn't a proxy somehow deal with expiring resources?
There was a problem hiding this comment.
the version thing always seemed like a janky hack.
There was a problem hiding this comment.
I don't have a strong opinion. The other thing to consider is that the cache control header is per resource so when going from 10.1 to 10.2 the cache control header won't be available on the existing resources so you'll run into this problem one more time potentially.
|
What about using etags? Can we add etags via the servlet so then the server knows? It only has to do the head request... |
Etags can tell if the resource changed but they won't tell the browser to actually run the check (pretty sure you need cache control for that) |
There was a problem hiding this comment.
We don't need the cache busting anymore, right?
There was a problem hiding this comment.
We don't need it but it will be helpful for 10.1->10.2 at least since index.html is cache busted so it will bootstrap the new cache-busted index.js reference right away. Cache control settings are per resource so you can still hit the same bug when upgrading to 10.2 without it. Also, in the off-chance you run through a proxy which alters headers it is a failsafe. That being said I can remove it as @epugh also seemed eager to get rid of it per #5300 (comment)
There was a problem hiding this comment.
well, if we want to bikeshed this some more, we could merge this as is, and then on main remove it?
Admin UI static files are now served with `Cache-Control: no-cache`, so after an upgrade the browser revalidates them instead of mixing in files cached from the previous Solr version. Such mixing could break the UI, for example by showing bogus "Permission failure" banners. (cherry picked from commit a2bfbf7)
|
somebody set the Milestone here for 10x but this looks like a nice 9x fix; no? |
I don't have any objections but how long do you plan on keeping 9.X going? |
Until some dev-list annoncement is made on the matter, one can assume another 9.x will occur. It |
https://issues.apache.org/jira/browse/SOLR-18615
Description
Browser caches admin resources across major versions and this could cause problems during upgrades. The latest were random banners that appeared because angular didn't evaluate
ng-showflags after something broke upstream of it. This, in turn, was caused by new UI code mixing with stale, cached UI code.Solution
Add a no-cache header so the browser periodically refreshes the assets. I've also updated the explicit cache buster as this is still valuable in certain circumstances, i.e. if a proxy overrides said header.
Tests
Validate that no-cache header is set and that we always pass some query string. The version buster validation is not strict as it is difficult to achieve nicely with bash's ERE regex which doesn't have lookahead.
Checklist
Please review the following and check all that apply:
mainbranch../gradlew check.