QSD caching potential fix #196
Conversation
|
Initially, you had a call to purge_page() in both qsd() and ajax_qsd(). Why'd you remove it from ajax_qsd? Did it not work or something? I'm thinking that if it works, we can use it for QSD pages and the more general varnish_purge page for everything else. Arguably "everything else" does in fact need an admin to approve it. Our website allows (allowed?) teachers to create per-class QSD pages (for class websites), so there are in fact occasions for non-admins to be editing QSD pages. |
|
For the specific permissions question, you could always do "admin or has QSD-editing bits" :-) Also, as pteromys already sort of pointed out implicitly, it's quite possible to wrap a general function in other functions that do permissions-checking. As one example, see "def catalog()" in esp/esp/program/modules/handlers/studentclassregmodule.py. (The general function is actually catalog_render().) That tries to put everything in one view; having multiple views for different purposes / user types works too. |
|
In response to Andrew's comment, I removed it from ajax_qsd because it didn't work. The reason it didn't work was that I couldn't figure out getting the URL that needed to be purged from within the function. I then realized that I could do it by calling a view from JS, so I implemented it that way instead. If there's a good way to get the URL that needs to be purged I could do that instead, and it would probably be neater. In response to Adam, I didn't think about doing both for some reason. Regarding wrapping general functions in functions with permissions checking, that's essentially what I'm going currently. Purge_page is a general function. The Varnish purge view, which is publicly exposed should do the permissions checking before calling purge_page for the actual functionality. |
… pages still needs work.
It currently has issues testing for administrator. I'm guessing a syntax error. I also removed the giant comment in qsd.js.
It seems like authentication works. It essentially just checks to see if the user is either an administrator or has QSD editing bits. Tests will be written.
|
I think I'm thoroughly confused by rebasing. Can someone please explain it to me? I looked it up online and it made sense. Then I tried to practically apply it and got really confused. I think I my changes were committed correctly though. I think I have authentication working. I plan to write tests for this whole thing next, but I wanted someone to look it over meanwhile and make sure I didn't do something stupid. |
|
I just realized that testing this would required setting up selenium on the buildbot tests servers. Should I go ahead and do this? |
|
Ummm... so turns out hippo doesn't have anything on the ebs1 partition anymore... did we lose access to this for some reason or something? (As a result the build slave wasn't running on hippo anymore) The build slave also wasn't running on diogenes anymore, but I restarted it and currently it's testing. We'll see if it succeeds. If I get testing working again, then I'll concentrate on setting up slave2 with varnish so I can do some testing with caching. |
|
Although on second thoughts my tests don't really require varnish. I could just put up something that receives the PURGE request and returns some packet with no content but the proper headers (I think) and it should still succeed in Django. In the end I think having Varnish set up would probably be a good idea for testing. |
|
"hippo": That's quite odd... I'd send mail to web-team@; there are a bunch of people on that list but not this who might know. If you could set up selenium on the buildbot servers, though, that'd be great. Are you still confused about rebasing? The idea is basically that you take a stack of diffs and apply them one by one as patches to another branch. (The stack of diffs is constructed from a series of commits on some other branch.) It's often simpler to merge instead of rebasing, though both are useful tools to know how to use. |
Still need to finish writing the test. Finding the location of some things also needs to be done.
|
Re: hippo - good idea, I sent out the email Do you mean varnish? I got a varnish server running (I think) correctly on slave2 on Diogenes. I'm in the process of writing a test for QSD caching which would involve testing against the Varnish server. Once I have this running on my own computer and commit, we can see if the auto tester works with it or not. Re: rebasing - I think I understood the general concept, just not how to practically apply it. I ran git rebase -i main (IIRC) and that did something. Then I tried to push to github and it said that there were conflicts needing to be resolved. I went in and it looked like the code in HEAD had the lastest and the conflicting code from my end was out of date for some reason. I resolved the conflicts and it then committed correctly. I don't know why this would be the case, and I think I might have missed something when I ran it, because it didn't really give me any option to like combine commits, change the comments, and generally anything that Andrew mentioned is a good idea to do with this... |
|
It looks like hippo was restarted at some point, so the drive got unmounted and buildbot wasn't running anymore. I remounted the drive and restarted the buildslave. I may need to tweak a little bit to get testing working again. |
|
So as I was writing tests I ran into the issue of how to check that the AJAX request to purge the page actually returns the correct value. Then I realized there were really two separate tests here. One to test if the QSD page correctly calls purge_page, and one that tests purge_page and varnish_purge and makes sure they correctly purge. With regards to the latter, I don't know how to test whether an AJAX request returned correctly. I could maybe use httplib and send a post command in order to get the return status. Or I could run some javascript in the webdriver. I'm not what the best option is here. With regards to checking if the right AJAX request occurs, I don't know how to test this at all. One really really hackish way of doing this could be to have the call to the view set some javascript flag, and then check this. I don't really like this idea (I already did it in my csrf selenium test and I'm thinking of trying to do it a different way at some point). Any thoughts on this? |
|
With regard to the latter, rather than using httplib, that sounds like a great use for Django's built-in test client. With regard to testing this feature overall: Personally, I as a user am not too worried if purge_page returns the right value, or if the AJAX request went through as expected, etc. What I do care about is whether the site immediately reflects my changes. So I think it'd be reasonable to have a test that simply updates a page that's cached and then tries to access that page to see if it contains whatever new text was added. If it does, then regardless of how the intermediate machinery works, it must be doing something right... I'm not sure whether you can get at HTTP headers through Selenium?, but I believe I remember that there's a header that indicates whether a page is coming from a cache, so you can be sure that it's not just a complete lack of caching. If you do that and you do a simple test of purge_cache with the Django test client, I think (unless I'm missing something?) that's pretty close to a guarantee that everything's working as expected; good enough, I think, given the cost in hackery to doing something more precise. |
|
Ah I forgot about Django's test client. Yes that's perfect for testing purge_page. With regards to QSD, that makes sense. I'll work on implementing this tonight. |
|
Gah! So in the process of writing the test I ran into a general issue. Django itself caches objects, which means it affects this. Here's the scenario:
This makes it impossible to test via selenium. Also I was fairly certain I wasn't having this issue before but all of a sudden I am. Any thoughts as to why this is coming up/fixes? |
|
I take back the "I wasn't having this issue before". This may have always been the case now that I think back on it. |
I have a test that theoretically should work. Currently it's failing because Django's caching is getting in the way. I posted a comment to Github, waiting for a response. I also moved some of the common selenium testing functions out of the specific CSRF selenium test class and into the general module. The CSRF test now just includes and uses the functions (and still works). My new test for QSD caching uses these as well.
|
hm... I don't see this issue on my dev server. I also don't see anything in the code that would cause this issue. (Django itself does no caching; it provides infrastructure to help implement caching, but it leaves the implementation up to us. It doesn't immediately look like any of the relevant functions, in esp/qsd/views.py, are even @cache_function'ed. They are @cache_control'ed, but that simply sets an HTTP header for Varnish.) Can you reproduce with a straight-up dev server, no intermediate Varnish or Squid? |
|
Apparently the change hadn't been taken into account, so I just incorporated it into this other commit. At this point I think this pull request is complete. Btw, when looking at inline qsd, it's failing for me with no proxy cache at all. When I change to it works again. This seems to be some sort of Django caching. Adam, you said there was no Django caching by default, but that it has the framework for specifying some caching. Do we normally not specify caching? It might be that I set up my dev server wrong so it does cache using memcached for Django. |
|
... and I take that back. After looking more deeply, my solution with stripping all cookies but the session doesn't work. We don't want the page to be cached per user in any way. Deleting the cookies, however, does result in the page not thinking it's logged in, namely that the admin portions don't display. I guess there's several options that I can think of.
|
|
For what it's worth, MIT's site already does (1) on this list. Or at least, has templates that can handle (1); stuff is always on the page and always the same for all users but is hidden based on some JavaScript/cookie magic. |
|
Yeah, (1) would be pretty easy to implement. What was already in place was similar to this, except the visibility was only based on admin, so people with QSD bits but who were not admins didn't get the hidden stuff to show up, even though they still could edit it. I might try to go through and change it to this, though I don't think I can get this fully working before the Dec 15 deadline. Should I go ahead and try anyway? |
Previously, inline QSD editing had checked whether or not the user was an admin to determine whether or not to show the edit header. This was a problem because users which were not admins but had QSD editing bits would not see this even though they were able to edit QSD. I fixed this by adding another cookie, cur_qsd_bits, and two new css classes, qsd_bits and qsd_bits_hidden, which are analogous to cur_admin, admin, and admin_hidden respectively. The editing header now appears correctly for users with QSD bits but not admin privileges.
|
Well that was easier than expected. I think I have it working (and my test doesn't break! :D). I'm going to go through tomorrow and simplify my qsd caching test to account for this. Also maybe write a test for this feature itself. Oh, also the loginbox seems to break when cached with the cookies stripped (since it's AJAX, it should show it as logged in once the page is rendered, because it has the cookies then). I might look into this. |
...again. I did something with git rebase earlier that undid this I think, so I redid this. I'm putting it in this branch because my test depends on this.
The tests now test for both inline and regular QSD. They also pass. I was having authentication issues before which I fixed as well. I had some boolean logic messed up.
|
Fixed up my test to a point that I'm satisfied with it. I also redid the change that moved commonly used seltest functions to the general seltests module. I also noticed that the "Edit Page" link showed up at the bottom of QSD no matter whether you were logged in or not. I fixed this to use the Javascript hidden QSD stuff that I did eariler. The login box still doesn't work, and I didn't bother trying to fix it. If Mike is working on redoing the default site, this is probably getting fixed/changed. |
|
Well, that's a whole bunch of commits! I think this branch looks reasonable to me. Andrew, any further comments? Otherwise "day then merge". |
|
Actually, sorry, I didn't read my own comments: There's still a bug here, that I'd really like to see resolved first, since it's both simple and causes things to not work on non-MIT sites. |
|
I actually had nothing to do with that commit. That was a commit that Mike made at some point that got included through rebasing (or rather failing thereof). |
|
Ah, ok. Sorry; having trouble picking out whose commit was whose, with this UI... Michael, thoughts? Has this been fixed? |
|
This pull request contains some commits (and associated comments) from my onsite work leading up to MIT Splash. I'll address those in a separate pull request. It sounds like this one is ready to go; we'll have to do some careful testing, but I think this is best done after a merge. |
|
In that case, I'll go ahead and merge :-) Michael, I think I'd argue for fixing at least this issue for SR1, since it makes the particular feature entirely unuseful for programs that aren't MIT Splash 2010. (Or just remove that codepath altogether, so that it doesn't trip anyone up somehow.) Thoughts on a minor late addition there? |
|
To respond to an earlier comment on this thread, in case it wasn't already clear: The cache_inclusion_tag decorator has already been converted to use the caching API; this was done back around February. So when you use cache_inclusion_tag you can specify cache dependencies like '[name_of_tag].cached_function.depend_on_...'; see esp/esp/program/templatetags/class_render.py for examples |
|
We will need to install selenium on diogenes.learningu.org and esp.mit.edu (if this hasn't already been done; I haven't been following Gurtej's work with selenium) before deploying this to live sites. We should also update INSTALL and dev_server_setup.sh to reflect this new dependency. |
|
It has been added to INSTALL and dev_server_setup.sh (though you guys should check it over), and I think I set it up on diogenes.learningu.org, but not esp.mit.edu so far. |
|
Can you point me to the commit where it was added? I don't see it. |
|
Thanks! I see the error of my ways now. I was typing 'git grep -n "selenium"' while I was in esp/esp, so I wasn't seeing esp/INSTALL or esp/useful_scripts. |
|
Ah that would make sense :P |
|
If you're looking for changes in a specific file, you can also type I use this all the time to track the history of bits of code that look like they shouldn't work. |
|
Yeah, I was actually going to use that to find this until I realized that just looking at the commit history of the file would be easier, but git blame is pretty useful overall. |
* Initial docs for stable release 13 * Docs for #3116, #3117, and #3118 * Added docs about django upgrade * Docs for #3128 * Docs for #3129, #3133, #3134, and #3137 * Docs for #3156 and #3153 * Docs for #3174, #3163, and #3184 * Docs for #3139, #3140, and #3141 * Docs for #3143, #3150, #3154, #3160, #3162, and #3168 * Docs for #3171, #3185, #3186, and #3188 * Docs for #3131 and #3189 * Docs for #3149 and #3190 * Docs for #3193, #3194, #3195, #196, and #3197 * Clarification * Docs for #3192, #3201, #3209, and #2248 * Docs for #3204, #3212, #3214, #3205, 9fd073c, and #3226 * Docs for #3232, de5861c, #3231, #3233, #3234, #3237, #3238, and #3239 * Fix indent * Docs for #3227 and #3235 * Add missing word * spelling * Docs for e57581f, #3255, #3253, #3257, and #3249 * Docs for #3254, #3260, and #3262 * Docs for #3263, #3272, #3240, #3264, #3266, and #3270 * clarifications * Docs for #3283 and #3252 * Docs for #3288 and misc commits * Docs for #3292, #3311, #3286, #3289, and #3279 * Docs for a377f0d; move note * Docs for #3315, #3290, and #3322 * Docs for #3273 and #3317 * Final edits
This is the fix I have right now. In the javascript it makes an additional AJAX request to the varnish_purge page that I created. This page then purges the correct page from the cache.
The gaping hole is that varnish_purge is currently accessible by anybody. I couldn't think of a good way to limit it to the correct subset of people. If I made it admin required, then anybody who could edit QSD but wasn't an admin would have issues (I don't know if there's any of these users, or if this contingency should be handled at all... if not then this is probably the best solution). If I made it based on qsd editing userbits, then it would take a perfectly general function and make it very restricted. If I made it just check for logged in, then any student could purge any page.
Any thoughts on good ways to resolve this?
Thanks!