Skip to content

Commit

Permalink
Better error trapping for WebDAV saver
Browse files Browse the repository at this point in the history 
Without these checks we get a startup crash when using TiddlyWiki in
client-server configuration.
  • Loading branch information
@Jermolene
Jermolene committed Feb 18, 2017
1 parent f0ff1f9 commit 91b341e
Showing 1 changed file with 13 additions and 9 deletions.
22 changes: 13 additions & 9 deletions core/modules/savers/put.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,19 @@ var PutSaver = function(wiki) {
type: "OPTIONS",
callback: function(err, data, xhr) {
// Check DAV header http://www.webdav.org/specs/rfc2518.html#rfc.section.9.1
self.serverAcceptsPuts = xhr.status === 200 && !!xhr.getResponseHeader("dav");
if(!err) {
self.serverAcceptsPuts = xhr.status === 200 && !!xhr.getResponseHeader("dav");
}
}
});
// Retrieve ETag if available
$tw.utils.httpRequest({
url: uri,
type: "HEAD",
callback: function(err, data, xhr) {
self.etag = xhr.getResponseHeader("ETag");
if(!err) {
self.etag = xhr.getResponseHeader("ETag");
}
}
});
};
Expand All @@ -50,12 +54,12 @@ PutSaver.prototype.uri = function() {
// Prompt: Do you want to save over this? Y/N
// Merging would be ideal, and may be possible using future generic merge flow
PutSaver.prototype.save = function(text, method, callback) {
if (!this.serverAcceptsPuts) {
if(!this.serverAcceptsPuts) {
return false;
}
var self = this;
var headers = { "Content-Type": "text/html;charset=UTF-8" };
if (this.etag) {
if(this.etag) {
headers["If-Match"] = this.etag;
}
$tw.utils.httpRequest({
Expand All @@ -64,15 +68,15 @@ PutSaver.prototype.save = function(text, method, callback) {
headers: headers,
data: text,
callback: function(err, data, xhr) {
if (xhr.status === 200 || xhr.status === 201) {
if(err) {
callback(err);
} if(xhr.status === 200 || xhr.status === 201) {

This comment has been minimized.

Sign in to view

Copy link
@pmario

pmario Oct 9, 2017

Contributor

Is there a missing else or is it just a formatting problem? @Jermolene ... pointed out by #2989

This comment has been minimized.

Sign in to view

Copy link
@sukima

sukima Oct 9, 2017

Contributor

I'm pretty sure this is a bug. If err were truthy then callback() would be called twice in a row! I don't think call back is meant to be called twice. Usually the if(err) { ... } pattern has a return:

if(err) {
  return callback(err);
}

Also it is very odd that the value passed to callback changes type. What you get is not consistent.

  • In the case of err you likely get a JavaScript Error object of some type (expected)
  • In the case of if(xhr.status === 200 || xhr.status === 201) you get null (expected)
  • In the case of if(xhr.status === 412) you get a String (Unexpected)
  • In the case of none of the above you again get a String (Unexpected)

Lastly, an empty string '' in JS is considered falsey which means that if callback() is passed a string that for some reason ends up becoming empty then this typical pattern will not work as expected:

new Promise(function (resolve, reject) {
  putServer.save('blah', 'blah', function (err) {
    if(err) {
      reject(err);
    } else {
      resolve();
    }
  });
});

If the server responds with a 500 or 422 and doesn't provide any content (Content-Length: 0, Bad server?) then the above example will resolve even though the server failed.
self.etag = xhr.getResponseHeader("ETag");
callback(null); // success
}
else if (xhr.status === 412) { // edit conflict
} else if(xhr.status === 412) { // edit conflict
var message = $tw.language.getString("Error/EditConflict");
callback(message);
}
else {
} else {
callback(xhr.responseText); // fail
}
}
Expand Down

0 comments on commit 91b341e

Please sign in to comment.