Skip to content

Parse Sabre Exception in OC.Files.Client and file-upload #6079

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
blizzz merged 1 commit into from
Dec 11, 2017
Merged

Parse Sabre Exception in OC.Files.Client and file-upload #6079

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 29 additions & 6 deletions apps/files/js/file-upload.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,23 @@ OC.FileUpload.prototype = {
*/
getResponse: function() {
var response = this.data.response();
if (typeof response.result !== 'string') {
if (response.errorThrown) {
// attempt parsing Sabre exception is available
var xml = response.jqXHR.responseXML;
if (xml.documentElement.localName === 'error' && xml.documentElement.namespaceURI === 'DAV:') {
var messages = xml.getElementsByTagNameNS('http://sabredav.org/ns', 'message');
var exceptions = xml.getElementsByTagNameNS('http://sabredav.org/ns', 'exception');
if (messages.length) {
response.message = messages[0].textContent;
}
if (exceptions.length) {
response.exception = exceptions[0].textContent;
}
return response;
}
}

if (typeof response.result !== 'string' && response.result) {
//fetch response from iframe
response = $.parseJSON(response.result[0].body.innerText);
if (!response) {
Expand Down Expand Up @@ -931,6 +947,7 @@ OC.Uploader.prototype = _.extend({
status = upload.getResponseStatus();
}
self.log('fail', e, upload);
self._hideProgressBar();
Copy link
Member

@danxuliu danxuliu Sep 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When several files are uploaded at once and one of them fails this hides the progress bar before those coming after the failed one have finished. Moreover, this should not be needed anyway, as the progres bar is only shown in the fileuploadstart event, so the current _hideProgressBar call in the fileuploadstop event should always hide it when there are no more files being uploaded, even in case of failures (in fact, the current call to _hideProgressBar when the upload is aborted is not strictly needed, as the fileuploadstop event will be eventually triggered, but in that case it does not harm to call it there as the pending uploads will be aborted too).

Having said that... they had to have a reason to add a call to _hideProgressBar in fail in ownCloud, so maybe I am missing something. Thus I would left it as is in this pull request and remove it in a different one so if the progress bar happens to not be hidden in some cases then the problem can be bisected to that removed call.


if (data.textStatus === 'abort') {
self.showUploadCancelMessage();
Expand All @@ -947,7 +964,12 @@ OC.Uploader.prototype = _.extend({
self.cancelUploads();
} else {
// HTTP connection problem or other error
OC.Notification.show(data.errorThrown, {type: 'error'});
var message = '';
if (upload) {
var response = upload.getResponse();
message = response.message;
Copy link
Member

@danxuliu danxuliu Sep 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be ensured that response is not undefined before using it, as there is at least one (obscure...) code path in which getResponse can return undefined: when a chunked upload fails to create the temporary directory before uploading the file itself the upload is aborted even before it was started, so in the end this causes fail to be called with an upload without a response. Chunked uploads do not work right now anyway, but just checking the object before using it makes no harm, and there could be other code paths that I have not found but need it too.

Also note that if the response is checked before being used, when a chunked upload fails to create the temporary directory before uploading the file itself just a message that reads abort is shown, but that is a different matter.

}
OC.Notification.show(message || data.errorThrown, {type: 'error'});
}

if (upload) {
Expand Down Expand Up @@ -1144,16 +1166,17 @@ OC.Uploader.prototype = _.extend({
upload.done().then(function() {
self._hideProgressBar();
self.trigger('done', e, upload);
}).fail(function(status) {
}).fail(function(status, response) {
var message = response.message;
self._hideProgressBar();
if (status === 507) {
// not enough space
OC.Notification.show(t('files', 'Not enough free space'), {type: 'error'});
OC.Notification.show(message || t('files', 'Not enough free space'), {type: 'error'});
self.cancelUploads();
} else if (status === 409) {
OC.Notification.show(t('files', 'Target folder does not exist any more'), {type: 'error'});
OC.Notification.show(message || t('files', 'Target folder does not exist any more'), {type: 'error'});
} else {
OC.Notification.show(t('files', 'Error when assembling chunks, status code {status}', {status: status}), {type: 'error'});
OC.Notification.show(message || t('files', 'Error when assembling chunks, status code {status}', {status: status}), {type: 'error'});
}
self.trigger('fail', e, data);
});
Expand Down
47 changes: 37 additions & 10 deletions core/js/files/client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,26 @@
return status >= 200 && status <= 299;
},

/**
* Parse the Sabre exception out of the given response, if any
*
* @param {Object} response object
* @return {Object} array of parsed message and exception (only the first one)
*/
_getSabreException: function(response) {
var result = {};
var xml = response.xhr.responseXML;
var messages = xml.getElementsByTagNameNS('http://sabredav.org/ns', 'message');
var exceptions = xml.getElementsByTagNameNS('http://sabredav.org/ns', 'exception');
if (messages.length) {
result.message = messages[0].textContent;
}
if (exceptions.length) {
result.exception = exceptions[0].textContent;
}
return result;
},

/**
* Returns the default PROPFIND properties to use during a call.
*
Expand Down Expand Up @@ -447,7 +467,8 @@
}
deferred.resolve(result.status, results);
} else {
deferred.reject(result.status);
result = _.extend(result, self._getSabreException(result));
deferred.reject(result.status, result);
}
});
return promise;
Expand Down Expand Up @@ -521,7 +542,8 @@
var results = self._parseResult(result.body);
deferred.resolve(result.status, results);
} else {
deferred.reject(result.status);
result = _.extend(result, self._getSabreException(result));
deferred.reject(result.status, result);
}
});
return promise;
Expand Down Expand Up @@ -560,7 +582,8 @@
if (self._isSuccessStatus(result.status)) {
deferred.resolve(result.status, self._parseResult([result.body])[0]);
} else {
deferred.reject(result.status);
result = _.extend(result, self._getSabreException(result));
deferred.reject(result.status, result);
}
}
);
Expand Down Expand Up @@ -590,7 +613,8 @@
if (self._isSuccessStatus(result.status)) {
deferred.resolve(result.status, result.body);
} else {
deferred.reject(result.status);
result = _.extend(result, self._getSabreException(result));
deferred.reject(result.status, result);
}
}
);
Expand Down Expand Up @@ -639,7 +663,8 @@
if (self._isSuccessStatus(result.status)) {
deferred.resolve(result.status);
} else {
deferred.reject(result.status);
result = _.extend(result, self._getSabreException(result));
deferred.reject(result.status, result);
}
}
);
Expand All @@ -663,7 +688,8 @@
if (self._isSuccessStatus(result.status)) {
deferred.resolve(result.status);
} else {
deferred.reject(result.status);
result = _.extend(result, self._getSabreException(result));
deferred.reject(result.status, result);
}
}
);
Expand Down Expand Up @@ -727,11 +753,12 @@
this._buildUrl(path),
headers
).then(
function(response) {
if (self._isSuccessStatus(response.status)) {
deferred.resolve(response.status);
function(result) {
if (self._isSuccessStatus(result.status)) {
deferred.resolve(result.status);
} else {
deferred.reject(response.status);
result = _.extend(result, self._getSabreException(result));
deferred.reject(result.status, result);
}
}
);
Expand Down
18 changes: 16 additions & 2 deletions core/js/tests/specs/files/clientSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,28 @@ describe('OC.Files.Client tests', function() {
promise.done(successHandler);
promise.fail(failHandler);

var errorXml =
'<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">' +
' <s:exception>Sabre\\DAV\\Exception\\SomeException</s:exception>' +
' <s:message>Some error message</s:message>' +
'</d:error>';

var parser = new DOMParser();

requestDeferred.resolve({
status: status,
body: ''
body: errorXml,
xhr: {
responseXML: parser.parseFromString(errorXml, 'application/xml')
}
});

promise.then(function() {
expect(failHandler.calledOnce).toEqual(true);
expect(failHandler.calledWith(status)).toEqual(true);
expect(failHandler.getCall(0).args[0]).toEqual(status);
expect(failHandler.getCall(0).args[1].status).toEqual(status);
expect(failHandler.getCall(0).args[1].message).toEqual('Some error message');
expect(failHandler.getCall(0).args[1].exception).toEqual('Sabre\\DAV\\Exception\\SomeException');

expect(successHandler.notCalled).toEqual(true);
});
Expand Down