Enable chunking for bigger files in authenticated web upload #28415

PVince81 · 2017-07-17T11:51:35Z

Resend of #26306.
See original post there.

Remaining bugs:

BUG: progress bar disappears during final MOVE

PVince81 · 2017-07-17T15:33:09Z

TODO: fix JS unit tests

PVince81 · 2017-07-20T15:15:22Z

I just realized that using the new chunking endpoint also means we can't detect errors early, so uploading to a folder that doesn't exist will only error at the very end during the final MOVE. I don't want to add an extra PROPFIND on the client just for that and would prefer a more streamlined solution. Raised here: #28452

I have fixed the progress bar, it now stays until the end of the final MOVE.

jvillafanez · 2017-07-24T07:41:23Z

core/js/config.php


 // Allow hooks to modify the output values
 OC_Hook::emit('\OCP\Config', 'js', ['array' => &$array]);
+$event = \OC::$server->getEventDispatcher()->dispatch('OCP\Config::js', new GenericEvent());


I don't think this is a good approach:

The overall approach seems weird. You usually don't send an event and expect the listeners to overwrite the arguments of the event. You send an event to let the listeners react to that event.

Overwriting data inside a listener doesn't seem a good idea. I think this is a similar case of attaching something to a data stream in order to overwrite its data. While the files app will likely be always active, other apps that could overwrite values could be enabled and disabled at will. This might change the values unexpectedly for the users.

Taking into account several listeners could modify the same key, there is no explicit information about what should be the expected value. If I create another app and modify the chunk size, what should be the expected chunk size value? It will depend on which is the last app that has overwrite the value, but that's not enough to know if it will be mine or yours.

The main problem I see is that fetching external information (from other apps for example) using events doesn't seem to be a good choice.

I think for this PR I'd just go for the simplest approach and wherever place you'll use the max chunk size value, fetch whatever is in the configuration and overwrite it there.

For a more complex approach, in order to overwrite configuration values I'd include some methods in the Config object to register specific IConfigListener objects, whose interface would have a fetching($key, $value) method which should return the expected overwritten value for that config key.
There are 2 extra things we should take care of: how can we properly extend the current Config class with this functionality, and also the priority of the registered IConfigListener objects in order to pick the value we want in the case we have more than one object modifying the configuration.

I think for this PR I'd just go for the simplest approach and wherever place you'll use the max chunk size value, fetch whatever is in the configuration and overwrite it there.

I need it in the JS code and this seemed like a good choice. I could also have used the old hook that does just that: overwrite the array values.

The other option is to embed the value of the config inside the DOM in the files app template, which I found less elegant.

When I asked @DeepDiver1975 about overwriting values in the symfony event he seemed to agree. We also used this approach in the config report app.

@jvillafanez your concern is legitimate, so I'd like us to have a common decision on whether to allow this or not and make it a guideline. @DeepDiver1975

PVince81 · 2017-07-24T09:17:14Z

sort out passing the config value

Note: the reason I chose to have a config value is mostly because I'm pretty sure someone will want to change it and having it hard-coded would result in "integrity check" errors when changed.

PVince81 · 2017-07-31T13:35:16Z

@jvillafanez adjusted. I'm now using the old hook for passing the value.

Inventing a new better clean way to pass values to be done separately in the future.

jvillafanez · 2017-08-01T07:18:18Z

apps/files/appinfo/app.php

@@ -1,4 +1,6 @@
 <?php
+
+use Symfony\Component\EventDispatcher\GenericEvent;


jvillafanez · 2017-08-01T07:18:47Z

core/js/config.php

 *
 */

+use Symfony\Component\EventDispatcher\GenericEvent;


jvillafanez · 2017-08-01T07:20:09Z

core/js/files/client.js


 		var client = new OC.Files.Client({
 			host: OC.getHost(),
-			port: OC.getPort(),


do we want to remove this, or add the option in the client?

I removed it because it's unused by OC.Files.Client. getHost() already contains the port.

PVince81 · 2017-08-01T07:45:17Z

@jvillafanez removed unused imports. I also adjusted @since 9.2 to say @since 10.0.3

PVince81 · 2017-08-01T08:08:03Z

restarted jenkins build

PVince81 · 2017-08-01T08:13:12Z

stable10: #28547

Emil-G · 2017-11-15T15:43:07Z

@PVince81 : why not implemented for anonymous uploads as well?

PVince81 · 2017-11-15T17:14:50Z

@Emil-G see #8955 (comment)

Emil-G · 2017-11-15T21:58:29Z

@PVince81 - Thanks for that. It seems that there now are a lot of different endpoints, would it not be beneficial to standardize? We are currently having some issues with OC Enterprise, where anonymous users struggle with large file uploads, if chunking was used in the same way as web, we would not have any issues as auth users works flawless. As we cannot allow users "guest management", and we can't do guest management from an administrative perspective, the Guest addon cannot be used in our scenario.

Are you sure that this endpoint cannot have chunking in the very near future?

Cheers,
Emil

PVince81 · 2017-11-16T08:07:27Z

No, because we will move away from public.php/webdav. We are standardizing on "remote.php/dav" for everything already.

Emil-G · 2017-11-16T08:10:36Z

So chuncking will be done for non-auth users, by opening up "remote.php/dav" from the webinterface?

PVince81 · 2017-11-16T08:20:47Z

No, this is a technical detail. The public link page will stop using public.php/webdav and will use "remote.php/dav/public-files" for any file access. This will make it possible to use "remote.php/dav/uploads" which web UI chunking already uses in the future and instead of putting the assembled file to "remote.php/dav/files" it will put it to "remote.php/dav/public-files".

Currently "public-files" is not ready: #29369

@Emil-G as you are having issues with the enterprise edition, please go through the support channels

lock · 2019-08-01T22:02:42Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

PVince81 added the 3 - To Review label Jul 17, 2017

PVince81 added this to the development milestone Jul 17, 2017

PVince81 self-assigned this Jul 17, 2017

This was referenced Jul 17, 2017

Use chunking in web upload #26306

Closed

Web Based File Upload Chunking #8955

Closed

PVince81 force-pushed the wwebui-uploadchunking-resend branch from 4250deb to 4d0e49f Compare July 19, 2017 08:25

PVince81 force-pushed the wwebui-uploadchunking-resend branch from 4d0e49f to 33a0084 Compare July 20, 2017 15:15

PVince81 requested a review from DeepDiver1975 July 20, 2017 15:23

PVince81 added the backport-request label Jul 24, 2017

jvillafanez reviewed Jul 24, 2017

View reviewed changes

PVince81 force-pushed the wwebui-uploadchunking-resend branch from 33a0084 to 6f9fceb Compare July 31, 2017 13:29

jvillafanez reviewed Aug 1, 2017

View reviewed changes

Enable chunking for bigger files in authenticated web upload

5560c20

PVince81 force-pushed the wwebui-uploadchunking-resend branch from 6f9fceb to 5560c20 Compare August 1, 2017 07:45

jvillafanez approved these changes Aug 1, 2017

View reviewed changes

PVince81 added 4 - To release and removed 3 - To Review labels Aug 1, 2017

PVince81 mentioned this pull request Aug 1, 2017

[stable10] Enable chunking for bigger files in authenticated web upload #28547

Merged

PVince81 removed the backport-request label Aug 1, 2017

PVince81 merged commit 195e6fb into master Aug 1, 2017

PVince81 deleted the wwebui-uploadchunking-resend branch August 1, 2017 15:06

danxuliu mentioned this pull request Nov 3, 2017

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

Merged

ownclouders added the status/STALE label Dec 20, 2017

lock bot locked as resolved and limited conversation to collaborators Aug 1, 2019

		@@ -1,4 +1,6 @@
		<?php

		use Symfony\Component\EventDispatcher\GenericEvent;

Enable chunking for bigger files in authenticated web upload #28415

Enable chunking for bigger files in authenticated web upload #28415

Uh oh!

Conversation

PVince81 commented Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PVince81 commented Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PVince81 commented Jul 20, 2017

Uh oh!

jvillafanez Jul 24, 2017

Choose a reason for hiding this comment

Uh oh!

jvillafanez Jul 24, 2017

Choose a reason for hiding this comment

Uh oh!

PVince81 Jul 24, 2017

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Jul 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PVince81 commented Jul 31, 2017

Uh oh!

jvillafanez Aug 1, 2017

Choose a reason for hiding this comment

Uh oh!

jvillafanez Aug 1, 2017

Choose a reason for hiding this comment

Uh oh!

jvillafanez Aug 1, 2017

Choose a reason for hiding this comment

Uh oh!

PVince81 Aug 1, 2017

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Aug 1, 2017

Uh oh!

PVince81 commented Aug 1, 2017

Uh oh!

PVince81 commented Aug 1, 2017

Uh oh!

Emil-G commented Nov 15, 2017

Uh oh!

PVince81 commented Nov 15, 2017

Uh oh!

Emil-G commented Nov 15, 2017

Uh oh!

PVince81 commented Nov 16, 2017

Uh oh!

Emil-G commented Nov 16, 2017

Uh oh!

PVince81 commented Nov 16, 2017

Uh oh!

lock bot commented Aug 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

PVince81 commented Jul 17, 2017 •

edited

Loading

PVince81 commented Jul 17, 2017 •

edited

Loading

PVince81 commented Jul 24, 2017 •

edited

Loading