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
redirect.patch.txt

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.


CategoryFeatureRequest

MoinMoin: FeatureRequests/PostShouldRedirect (last edited 2014-02-01 00:41:01 by cy)