HTTP POST should redirect to the result, not generate it
I noticed when editing the wiki that often after you edit and save changes or cancel, it would return you to the page you just edited, but with a catch.
If you ever reloaded that page, your browser would not reload the page and show you its current contents. It would ask you if you want to resubmit the form data. Then if you did, it would redirect you to the edit page with an error, saying that you tried to edit the page without changing its contents.
If you went back in history to that page, it would say the page expired.
The only way to view the page once that happens is to manually go up into the URL bar copy and paste in the URL and hit enter to fool your browser into thinking it was going to that URL for the first time.
This is what happens when POST returns 200 with content. Excepting on one-off errors (and even then), POST should always redirect to a page that returns 200 with content. Currently when a POST is submitted, moinmoin seems to generate the page as if it were a GET request, as a returned document. I've never had that ever be the right thing to do. Instead upon POST, moinmoin should only redirect to the page URL, that will then be generated as with any GET request and sent via code 200.
Sorry if this sounds cranky. I probably should be in bed right now.
The patch I am suggesting is roughly as follows:
1 diff -r 8ab046023ff1 MoinMoin/action/edit.py
2 --- a/MoinMoin/action/edit.py Sun Jan 05 04:38:29 2014 +0100
3 +++ b/MoinMoin/action/edit.py Fri Jan 31 02:56:37 2014 -0800
4 @@ -12,8 +12,20 @@
5 from MoinMoin.Page import Page
6 from MoinMoin.web.utils import check_surge_protect
7
8 +def redirectSelf(request):
9 + request.reset()
10 + path = request.environ['PATH_INFO'] + '#'
11 + request.http_redirect(request.environ['SCRIPT_NAME']+path)
12 +
13 def execute(pagename, request):
14 """ edit a page """
15 +
16 + # did user hit cancel button?
17 + cancelled = 'button_cancel' in request.form
18 +
19 + if cancelled:
20 + return redirectSelf(request)
21 +
22 _ = request.getText
23
24 if 'button_preview' in request.form and 'button_spellcheck' in request.form:
25 @@ -76,9 +88,6 @@
26 pg.sendEditor()
27 return
28
29 - # did user hit cancel button?
30 - cancelled = 'button_cancel' in request.form
31 -
32 from MoinMoin.error import ConvertError
33 try:
34 if lasteditor == 'gui':
35 @@ -182,8 +191,10 @@
36 pg.sendEditor(preview=savetext, comment=comment, staytop=1)
37 return
38
39 - # Send new page after successful save
40 - request.reset()
41 + # Redirect to new page after successful save
42 + # Do not send, or reload will re-post the edit data and be annoying
43 + redirectSelf(request)
44 + return
45 pg = Page(request, pagename)
46
47 # sets revision number to default for further actions
It also makes a small adjustment to the location of the cancellation check, putting it as early as possible so to avoid calculating all the potential changes to be made and then just cancelling and throwing them away. But mostly it just calls redirectSelf instead of generating a page as a response. The idea being to redirect to the URL you just POSTed to, which will result in a GET request for that URL, which will end up calling Page(request, pagename) as is desired.
This is a bit of a hack but it works, and I wanted to get something out right away, and small patches are always good to start out with. It only fixes the edit form, for saving and cancellation. Preview and Check Spelling are unaltered, since I didn't go and figure out how to redirect to the same page with the same queries in the URL. To redirect back to an edit page you'd have to preserve that query string, but of course when you want to redirect back to the "viewing mode" of the page, the query string should be left out. I didn't figure that out because state machines are hard and I'm tired. It also leaves some stuff about cancellation that could just be deleted. Also "redirectSelf" or "redirectToSelf" or "reload" (or whatever it should be called) should probably be in a utility module separate from "edit.py" so that other actions can use it to redirect instead of generating pages themselves.
Also, raising an exception to redirect works particularly well for cutting out pointless post-processing code. But that's not how werkzeug does it so I don't want to rock the boat until I'm ready to laboriously go through and change all the "set the redirect flag" actions to "raise a redirect exception."
Thanks for your suggestion.
The usual advice with browser back button or using the browser history to navigate was "don't do it, use the wiki user interface" (e.g. the trail or the navibar links).
But as you analyzed the issue and what you found seems valid, we maybe can (carefully) try changing it. Consider moin is stable, so it shouldn't break unexpectedly in a maintenance release. Also changes should be careful and minimal. Your redirectSelf() code looks a bit suspicious, maybe you could do it in the way as it is done at other places in moin (grep for http_redirect) - esp. computing the URL. Also, if you redirect anyway, is the call to .reset() necessary?
I wouldn't have changes any other way. Isn't .reset() necessary just in case the response has already been given a 200 and page reply? That shouldn't ever happen if the code is designed right, but it's a good precaution for general cases. The "way" I formed the URL in redirectSelf is idiomatic for WSGI code in general, but it's true that it's a bit lower level than strictly necessary. The trick is to get the URL for the page, without generating the page, since that happens after the redirect. I'm not sure if there's overhead in creating a Page/PageEditor object just to get the URL for it. I wouldn't want to create a deadlock if creating the PageLock and then just aborting on page generation and redirecting to the page left that lock locked.