Version changing between POSTs results in loss of POST data

[0b10011]

Reproducing results

1. Load a multi-step form
2. Submit to go on to the second step
3. Change version sent in response header
4. Submit to go on to third step

   Expected results

   • POST data from first/second step should be retained

     Actual results

   • POST data from first/second step is lost

---

A possible solution would be to send the version in a request header, require the server to check it before processing POST data (perhaps with a flag via response header to avoid repeating actions), and then resubmit the POST data the old-fashioned way. (See #357.)

[mislav]

Good point. (Sorry for late reply.)

Though, I'm not sure I understand how your proposed fix works even in theory (nevermind the code).

[0b10011]

We've implemented this fix on our production site and it works wonderfully. Basically:

1. PJAX sends the version in the request

   /* ... */
   xhr.setRequestHeader('X-PJAX', 'true')
   xhr.setRequestHeader('X-PJAX-Version', currentVersion)
   xhr.setRequestHeader('X-PJAX-Container', context.selector)
   /* ... */

2. The server checks the version header immediately, and if there is a version mismatch, 412 Precondition Failed header is returned:

   /* ... (php) */
   $pjax_version = $this->version;
   if(PJAX){
      header("x-pjax-version: $pjax_version");
   
      if($pjax_version !== $_SERVER['HTTP_X_PJAX_VERSION']){
          // Page was requested by PJAX, but older version of code is being used.
          // 412 seems to be the proper response for this particular issue.
          // Causes PJAX to fallback to having the browser load the url itself.
          header("HTTP/1.1 412 Precondition Failed");
          die;
      }
   }
   /* ... */

3. PJAX checks for the header, and if present, resubmits the form normally:

   // Request error
   $(document).on('pjax:error', function (event, xhr, textStatus, errorThrown, options) {
      // Links and forms
      switch (xhr.status) {
      case 404: // Not found
      case 410: // Gone
          // Not 200, but should still display response as if it were
          //
          // Fake a successful request
          options.success(xhr.responseText, textStatus, xhr);
   
          // Cancel default behavior
          return false;
      }
   
      // If a normal link, try loading it without PJAX
      if (!$(event.relatedTarget).is("form")) {
          return;
      }
   
      // Forms only
      switch (xhr.status) {
      case 412:
          // Outdated code on client, submit request again normally
          //
          // Disable pjax
          $.pjax.disable();
   
          // Turn off event handlers
          $(document).off('submit', 'form');
   
          // Don't double submit "../"
          $(event.relatedTarget).attr("action", window.location.href);
   
          // Submit form
          // This is an ugly hack to workaround submit buttons without a name that
          // automatically take the name 'submit', thereby breaking form.submit().
          HTMLFormElement.prototype.submit.call(event.relatedTarget);
   
          // Cancel default behavior
          return false;
      case 500:
          // Server error, probably too large of an upload
          //
          // Alert the user so they can try submitting the form again manually
          window.alert("Our server couldn't fully handle your request. If you " +
                          "are trying to upload a file, please make sure it is within " +
                          "the size requirements listed. If you continue to run into " +
                          "issues, give us a call at ###.###.#### and we'll help you " +
                          "out.");
          return;
      }
   });

Looking for the 412 Precondition Failed header allows for general sites (that don't send that header) to keep working as they always have, and only sites that send that header (and, hopefully, are sending it before processing any form data) will be affected.

[0b10011]

And, really, you could just send the X-PJAX-Version header and let the user handle the pjax:error listener. (Maybe throw an example in the docs?)

[mislav]

Thanks for the detailed explanation. I might have to think about this for a while. But I think it makes sense that it can be useful to send the version header back to the server, at least for non-GET requests.

The thing is, our implementation of pjaxed form sucks which is evident that many issues (like this one) related to it. I'm not sure whether we should keep fixing it or just nuke the feature altogether.

[0b10011]

Thanks for the detailed explanation. I might have to think about this for a while. But I think it makes sense that it can be useful to send the version header back to the server, at least for non-GET requests.

The header is useful for GET requests as well, since a version change can mean a change to stylesheets/scripts that will break the page if those aren't replaced as well. Whenever we push out a change to our site, we update the version, and then all users that load a page (PJAX or otherwise) get a fresh load with new scripts, styles, and content. Otherwise, PJAX users get the new content. Without the version header, the server would have to do double requests for all of these pages (which, during peak traffic, is no fun).

The thing is, our implementation of pjaxed form sucks which is evident that many issues (like this one) related to it. I'm not sure whether we should keep fixing it or just nuke the feature altogether.

We've been using PJAX for forms since mid-June, and have been able to take care of everything so far without too much trouble. Here's our modified version of PJAX and our script with some configuration/listeners. At this point, I'm not sure what all has been updated, but a diff should give a pretty good idea of what and why things changed. Forms were actually less of a pain than the cache was, at least for us. (I ended up just disabling it due to issues and lack of time to work on it. Could have been related to forms implementation... I can't remember.)

[mislav]

On Tue, Oct 14, 2014 at 6:14 PM, Brandon Frohs notifications@github.com wrote:

The header is useful for GET requests as well, since a version change can mean a change to stylesheets/scripts that will break the page if those aren't replaced as well. Whenever we push out a change to our site, we update the version

We already have a solution for that, and that is server setting the <meta http-equiv=X-PJAX-VERSION ...> tag on the initial page load with layout and subsequently responding with X-PJAX-VERSION header to pjax requests. When pjax detects that the version in the meta tag and the version from response doesn't match, it hard-reloads the URL. On GitHub.com, we update the value in this header every time we change any of our compiled assets.

This doesn't help POST requests, of course, since the POST data would be lost.

[0b10011]

We already have a solution for that, and that is server setting the <meta http-equiv=X-PJAX-VERSION ...> tag on the initial page load with layout and subsequently responding with X-PJAX-VERSION header to pjax requests. When pjax detects that the version in the meta tag and the version from response doesn't match, it hard-reloads the URL. On GitHub.com, we update the value in this header every time we change any of our compiled assets.

Yes, but this still causes the server to generate the page twice because the server isn't aware that the page is using an old version, which can have the side effect of pages/issues/etc being marked as "read" before they are. For example:

1. User clicks a link
2. PJAX requests a page
3. Server generates the page (with new meta)
4. PJAX reads the response, notices the new version, and resubmits the request (via .click() or however it does it, I forget)
5. Something goes wrong and page isn't loaded (eg, server taken down for maintenance, user moves out of range of network, etc)
6. User gets an error/blank page and leaves
7. Page/issue/etc is marked as "seen/read", but user never actually saw/read it. These are rare edge cases, yes, but still possible.

Sending the X-PJAX-VERSION header to the server allows the server to abort the request immediately, avoid generating a page twice, only respond with a 412 Precondition Failed header (instead of entire page's response, which is important for slow networks), and avoid any database reads/writes that are unnecessary/unwanted.

[mislav]

Yes, but this still causes the server to generate the page twice

True. I guess we treat these kind of refreshes as a rare occurrence and we're not terribly worried about their performance. But you're right that it can be more optimized.