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

LukasReschke · 2017-08-11T07:18:48Z

In case of error, instead of a generic error message, an upload will
display whichever message is returned in the Sabre Exception, if
applicable.

To test this enable the antivirus app and upload a EICAR test virus file or so.

Ref owncloud/core#28620

MorrisJobke · 2017-09-18T12:25:52Z

@danxuliu Mind to help reviewing this one?

danxuliu

I think that it would be better to add chunked uploads first and, then, add this parsing of Sabre exceptions.

danxuliu · 2017-09-20T09:35:03Z

apps/files/js/file-upload.js

@@ -900,6 +916,7 @@ OC.Uploader.prototype = _.extend({
 						status = upload.getResponseStatus();
 					}
 					self.log('fail', e, upload);
+					self._hideProgressBar();


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.

danxuliu · 2017-09-20T09:36:45Z

apps/files/js/file-upload.js

+						var message = '';
+						if (upload) {
+							var response = upload.getResponse();
+							message = response.message;


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.

danxuliu · 2017-09-20T09:38:42Z

apps/files/js/file-upload.js

@@ -1105,6 +1127,19 @@ OC.Uploader.prototype = _.extend({
 					var upload = self.getUpload(data);
 					upload.done().then(function() {
 						self.trigger('done', e, upload);
+					}).fail(function(status, response) {
+						var message = response.message;
+						self._hideProgressBar();


Same as above; if several files are loaded at once and the upload of the first one fails when moving the temporal assembled file of a chunked upload to its final location the progress bar would be hidden even if there are other files being uploaded.

In any case, the additions in fileuploaddone do not really belong to this commit/pull request. They are error handling code for chunked uploads (which seems to be done separately from other error handling because it handles failures in the movement of the temporal assembled file to its final destination, which is outside the standard flow of upload failures), but currently chunked uploads do not work at all (because as far as I know it should use the v2 DAV backend to use the Upload collection in the backend, and currently it uses the v1).

MorrisJobke · 2017-10-27T12:33:01Z

I think that it would be better to add chunked uploads first and, then, add this parsing of Sabre exceptions.

@danxuliu Are you open to do that?

danxuliu · 2017-10-27T12:37:51Z

@MorrisJobke

I think that it would be better to add chunked uploads first and, then, add this parsing of Sabre exceptions.

@danxuliu Are you open to do that?

Yes, I have been working on it for quite some time now, but it is taking me way longer than expected :-S I hope to finish it soon, though ;-)

codecov · 2017-11-10T14:51:22Z

Codecov Report

Merging #6079 into master will increase coverage by 0.01%.
The diff coverage is 92.59%.

@@             Coverage Diff              @@
##             master    #6079      +/-   ##
============================================
+ Coverage     50.74%   50.76%   +0.01%     
  Complexity    24479    24479              
============================================
  Files          1581     1581              
  Lines         93578    93595      +17     
  Branches       1353     1355       +2     
============================================
+ Hits          47487    47509      +22     
+ Misses        46091    46086       -5

Impacted Files	Coverage Δ	Complexity Δ
core/js/files/client.js	`83.57% <92.59%> (+0.55%)`	`0 <0> (ø)`	⬇️
lib/private/Security/CertificateManager.php	`91.08% <0%> (-1%)`	`39% <0%> (ø)`
lib/private/Server.php	`83.41% <0%> (+0.12%)`	`125% <0%> (ø)`	⬇️
core/js/js.js	`63.55% <0%> (+0.56%)`	`0% <0%> (ø)`	⬇️
lib/private/Files/Cache/Propagator.php	`96.2% <0%> (+1.26%)`	`16% <0%> (ø)`	⬇️

danxuliu · 2017-11-10T14:56:31Z

Now that chunked uploads have been merged I have imported again this pull request from ownCloud onto current master.

Note that I have just imported the original pull request, and I have not addressed yet my own comments; I will do that later in other pull requests.

In case of error, instead of a generic error message, an upload will display whichever message is returned in the Sabre Exception, if applicable. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

danxuliu · 2017-11-13T11:20:59Z

Oops, I forgot to sign the commit. Fixed now ;-)

MorrisJobke · 2017-12-08T14:55:27Z

@danxuliu Should we try to get this in 13 or is 14 fine?

danxuliu · 2017-12-08T23:03:37Z

@MorrisJobke 13, please; I "need" this merged to import more fixes for chunked uploads in Web UI.

MorrisJobke · 2017-12-10T09:32:47Z

@MorrisJobke 13, please; I "need" this merged to import more fixes for chunked uploads in Web UI.

As far as I can see, this is ready for review, right?

danxuliu · 2017-12-10T13:35:30Z

@MorrisJobke

As far as I can see, this is ready for review, right?

Right :-)

blizzz · 2017-12-11T15:11:36Z

@danxuliu are they points you have had your remarks on resolved? Or are they as a note? then we should turn them into issues (after merge)?

danxuliu · 2017-12-11T15:44:11Z

@blizzz

are they points you have had your remarks on resolved? Or are they as a note? then we should turn them into issues (after merge)?

They are not solved in this pull request, but there is no need to fill issues about them, as I am already working on fixing them.

MorrisJobke

Tested and works 👍 Code makes sense

MorrisJobke · 2017-12-11T15:46:20Z

How to test:

for example add throw new BadRequest('Some problem'); to the beginning of public function put($data) { in apps/dav/lib/Connector/Sabre/File.php

blizzz

tested with files_accesscontrol, also working

tobiasKaminsky · 2018-02-22T12:00:58Z

I assumed that a sane message would also shown on android, but we get "unsupported media type".
This returned by the server.

Ref: nextcloud/android#1391

tobiasKaminsky · 2018-03-13T08:54:08Z

@rullzer please see above. Do you recall if you fixed this with latest antivirus update?

rullzer · 2018-03-13T09:34:43Z

yes the server throws an unsupported media type... but with a message as far as I can tell

tobiasKaminsky · 2018-03-13T12:27:37Z

Aha. This is probably the problem then, as we do not use the message, but only the main name.

tobiasKaminsky · 2018-03-15T06:51:40Z

@rullzer would it be possible to have a more meaningful throwable, like "virus detected", this way we could use it better on android.
Or is this the only occurrence where "unsupported media type" is used?

rullzer · 2018-03-15T07:40:10Z

We can't really do that. The app has to throw an exception that the server has predefined. Creating specialized exceptions for each app is not really scaleable.

Can't you just pass on the message (it should be translated anyways)?

LukasReschke added 3. to review Waiting for reviews bug labels Aug 11, 2017

tobiasKaminsky mentioned this pull request Aug 21, 2017

Proper handle result when uploading a virus file nextcloud/android#1391

Closed

rullzer added this to the Nextcloud 13 milestone Aug 25, 2017

MorrisJobke requested a review from danxuliu September 18, 2017 12:25

danxuliu reviewed Sep 20, 2017

View reviewed changes

MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Oct 27, 2017

danxuliu mentioned this pull request Nov 3, 2017

[oc] Enable chunking for bigger files in authenticated web upload #7056

Merged

danxuliu force-pushed the fix-antivirus branch from 7c0762f to e8bbc47 Compare November 10, 2017 14:51

danxuliu added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 10, 2017

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

dfc91a2

In case of error, instead of a generic error message, an upload will display whichever message is returned in the Sabre Exception, if applicable. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

danxuliu force-pushed the fix-antivirus branch from e8bbc47 to dfc91a2 Compare November 13, 2017 11:20

MorrisJobke mentioned this pull request Dec 8, 2017

13.0.0 beta 2 #7425

Merged

28 tasks

MorrisJobke approved these changes Dec 11, 2017

View reviewed changes

blizzz approved these changes Dec 11, 2017

View reviewed changes

blizzz merged commit 4fc8984 into master Dec 11, 2017

blizzz deleted the fix-antivirus branch December 11, 2017 16:12

danxuliu mentioned this pull request Apr 5, 2018

Fix progress bar hidden before the upload ends #9085

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

LukasReschke commented Aug 11, 2017 •

edited

Loading

MorrisJobke commented Sep 18, 2017

danxuliu left a comment

danxuliu Sep 20, 2017

danxuliu Sep 20, 2017

danxuliu Sep 20, 2017

MorrisJobke commented Oct 27, 2017

danxuliu commented Oct 27, 2017

codecov bot commented Nov 10, 2017 •

edited

Loading

danxuliu commented Nov 10, 2017

danxuliu commented Nov 13, 2017

MorrisJobke commented Dec 8, 2017

danxuliu commented Dec 8, 2017

MorrisJobke commented Dec 10, 2017

danxuliu commented Dec 10, 2017

blizzz commented Dec 11, 2017

danxuliu commented Dec 11, 2017

MorrisJobke left a comment

MorrisJobke commented Dec 11, 2017

blizzz left a comment

tobiasKaminsky commented Feb 22, 2018 •

edited

Loading

tobiasKaminsky commented Mar 13, 2018

rullzer commented Mar 13, 2018

tobiasKaminsky commented Mar 13, 2018

tobiasKaminsky commented Mar 15, 2018 •

edited

Loading

rullzer commented Mar 15, 2018

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

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

Conversation

LukasReschke commented Aug 11, 2017 • edited Loading

MorrisJobke commented Sep 18, 2017

danxuliu left a comment

Choose a reason for hiding this comment

danxuliu Sep 20, 2017

Choose a reason for hiding this comment

danxuliu Sep 20, 2017

Choose a reason for hiding this comment

danxuliu Sep 20, 2017

Choose a reason for hiding this comment

MorrisJobke commented Oct 27, 2017

danxuliu commented Oct 27, 2017

codecov bot commented Nov 10, 2017 • edited Loading

Codecov Report

danxuliu commented Nov 10, 2017

danxuliu commented Nov 13, 2017

MorrisJobke commented Dec 8, 2017

danxuliu commented Dec 8, 2017

MorrisJobke commented Dec 10, 2017

danxuliu commented Dec 10, 2017

blizzz commented Dec 11, 2017

danxuliu commented Dec 11, 2017

MorrisJobke left a comment

Choose a reason for hiding this comment

MorrisJobke commented Dec 11, 2017

blizzz left a comment

Choose a reason for hiding this comment

tobiasKaminsky commented Feb 22, 2018 • edited Loading

tobiasKaminsky commented Mar 13, 2018

rullzer commented Mar 13, 2018

tobiasKaminsky commented Mar 13, 2018

tobiasKaminsky commented Mar 15, 2018 • edited Loading

rullzer commented Mar 15, 2018

LukasReschke commented Aug 11, 2017 •

edited

Loading

codecov bot commented Nov 10, 2017 •

edited

Loading

tobiasKaminsky commented Feb 22, 2018 •

edited

Loading

tobiasKaminsky commented Mar 15, 2018 •

edited

Loading