rc: Add operations/uploadfile to upload a file through rc using multipart/form-data #4221

negative0 · 2020-05-11T18:29:39Z

What is the purpose of this change?

Add functionality to upload a file to a remote using the rc.
This will allow a file to be uploaded directly through the web GUI.
May close #3412

Was the change discussed in an issue or in the forum before?

No

Checklist

I have read the contribution guidelines.
I have added tests for all changes in this PR if appropriate.
I have added documentation for the changes if appropriate.
All commit messages are in house style.
I'm done, this Pull Request is ready for review :-)

calisro · 2020-05-11T18:41:41Z

Interesting. I was actually considering doing this very thing in the httplib so that we could optionally upload files from the serve webdav/http templates.

negative0 · 2020-05-12T06:49:55Z

Interesting. I was actually considering doing this very thing in the httplib so that we could optionally upload files from the serve webdav/http templates.

Will this be of help to you?

calisro · 2020-05-12T12:50:13Z

@negative0 I believe so! When I have a chance i'm going to see what needs to be done on the http/webdav side but this may help me expedite that. Thanks!

negative0 · 2020-05-12T13:16:14Z

Nice @calisro . How to fix the failing race tests?

ncw

This looks interesting :-)

I've given it a very brief look over - comments inline.

Is a multipart form easiest from a browser?

From practically anything else a POST with parametesr fs=drive: remote=filename (the handler will already parse these) then put the file as the body of the request. That can then be streamed to Rcat without needing to be written to disk...

ncw · 2020-05-14T17:47:25Z

fs/operations/rc.go

@@ -150,6 +154,7 @@ func init() {
 		{name: "delete", title: "Remove files in the path", noRemote: true},
 		{name: "deletefile", title: "Remove the single file pointed to"},
 		{name: "copyurl", title: "Copy the URL to the object", help: "- url - string, URL to read from\n - autoFilename - boolean, set to true to retrieve destination file name from url"},
+		{name: "uploadfile", title: "Copy the file to path", help: "- url - string, URL to read from\n - autoFilename - boolean, set to true to retrieve destination file name from url"},


I think this is the wrong help here?

ncw · 2020-05-14T17:54:14Z

fs/rc/rcserver/rcserver.go

@@ -249,6 +249,24 @@ func (s *Server) handlePost(w http.ResponseWriter, r *http.Request, path string)
 		}
 	}

+	// Parse multipart/form-data (useful for file upload through rc)
+	if contentType != "" && strings.HasPrefix(contentType, "multipart/form-data") {
+		err := r.ParseMultipartForm(32 << 20)


32 MB might be too large here to store file in memory.

Apparently they will get stored on disk if they are bigger than this limit.

ncw · 2020-05-14T17:57:10Z

fs/operations/rc.go

+			return nil, err
+		}
+		fs.Debugf(obj, "File upload success")
+		return nil, CleanUp(ctx, f)


Don't think you wanted to Cleanup here!

ncw · 2020-05-14T17:59:31Z

fs/rc/rcserver/rcserver.go

+			return
+		}
+
+		for k, vs := range r.MultipartForm.Value {


I think I'd pass the whole MultipartForm on here as we need to call RemoveAll on it at some point to remove temporary files if any.

So something like

in["multipartForm"] = r.MultipartForm

calisro · 2020-05-14T19:32:13Z

I believe it may be inefficient to use a application/x-www-form-urlencoded to POST it which is, I believe, the option you're talking about. It would work but for binary data, I believe, each non-alpha character gets encoded by the browser. Thats 3:1 for each. For text and small amounts of data its fine though.

http://www.w3.org/TR/html401/interact/forms.html

REf: 17.13.4 Form content types

ncw · 2020-05-15T11:00:32Z

The problem with multipart uploads is they are quite difficult to stream (but not impossible - we do this in some of the rclone backends).

The actual file data is sent as binary data so it is efficient, but there are mime markers bounding it

Here is a really clear explanation of what actually gets sent: https://ec.haxx.se/http/http-multipart

Doing a bit more reading I think we need to call the Request.MultipartReader which makes receiving multipart uploads as a stream easy on the server.

So probably what we should do is put the entire http.Request into the input rc.Params and stream the input files to operations.Rcat like this example.

What do you think @negative0 ?

negative0 · 2020-05-15T17:01:03Z

So probably what we should do is put the entire http.Request into the input rc.Params and stream the input files to operations.Rcat like this example.

@ncw, This is an interesting approach. I will try to do that.

negative0 · 2020-05-15T17:06:26Z

Should we add the request into the params only if it's a multipart/formdata or regardless?

ncw · 2020-05-19T16:13:47Z

Should we add the request into the params only if it's a multipart/formdata or regardless?

A good question...

Maybe we should add the request in always with an _request input? That will break some things to for example the core/echo which is used for testing...

I suggest you try it and see how many tests break!

negative0 · 2020-05-21T14:16:26Z

Interesting :-p

ncw · 2020-06-04T13:47:43Z

Interesting :-p

What we should probably do is add a new flag here

rclone/fs/rc/registry.go

Lines 19 to 25 in 50e31c6

    
           type Call struct { 
        
           	Path         string // path to activate this RC 
        
           	Fn           Func   `json:"-"` // function to call 
        
           	Title        string // help for the function 
        
           	AuthRequired bool   // if set then this call requires authorisation to be set 
        
           	Help         string // multi-line markdown formatted help 
        
           }

Called NeedsRequest and only add the _request parameter it on rc methods which have that set.

That should avoid the suprises!

What do you think?

ncw · 2020-06-05T13:34:51Z

The needs request code looks great :-)

How are you at re-writing history?

Can you re-arrange these commits into just two commits, so the first one is your needs request code and the second one is the upload file?

negative0 · 2020-06-05T14:10:34Z

The needs request code looks great :-)

How are you at re-writing history?

Can you re-arrange these commits into just two commits, so the first one is your needs request code and the second one is the upload file?

I'll do that :-)

negative0 · 2020-06-05T15:12:06Z

Done :)

ncw

Code looks great now :-)

One minor nit inline.

Can can you change the first commit to be "rc: ..." as that is what you are changing rather than "uploadfile: ..."

Thanks!

fs/rc/params.go

…ing multipart/form-data

negative0 · 2020-06-14T20:24:48Z

@ncw This PR is ready to be merged!

ncw

The code looks great now :-)

How about doing a test?

The tests for these live in https://github.com/rclone/rclone/blob/master/fs/operations/rc_test.go

If you want I can merge this now as long as you promise me a test later!

negative0 · 2020-06-17T13:49:27Z

I am not very good at writing tests. I will give it my best!.

Expect updates this week.

rc: Go import file rc_test.go

ncw

Looks great now - nice test :-)

I'll merge it now - thank you!

negative0 · 2020-06-25T11:48:26Z

Thank you :-)

negative0 force-pushed the rc/uploadfile branch from e0abf9c to 1f48b33 Compare May 11, 2020 18:34

ncw reviewed May 14, 2020

View reviewed changes

negative0 force-pushed the rc/uploadfile branch from e598b95 to 5d9578c Compare June 5, 2020 15:11

ncw reviewed Jun 10, 2020

View reviewed changes

fs/rc/params.go Outdated Show resolved Hide resolved

negative0 added 2 commits June 12, 2020 11:33

rc: add NeedsRequest to call.

19c388e

rc: Add operations/uploadfile to upload a file through rc using encod…

a3bb55e

…ing multipart/form-data

negative0 force-pushed the rc/uploadfile branch from 5d9578c to a3bb55e Compare June 12, 2020 06:04

ncw reviewed Jun 16, 2020

View reviewed changes

negative0 requested a review from ncw June 18, 2020 10:38

rc: Add tests for operations/uploadfile

b2fa4b1

rc: Go import file rc_test.go

negative0 force-pushed the rc/uploadfile branch from 68e0ee7 to b2fa4b1 Compare June 18, 2020 10:49

ncw approved these changes Jun 25, 2020

View reviewed changes

ncw merged commit a2dd23e into rclone:master Jun 25, 2020

negative0 deleted the rc/uploadfile branch June 25, 2020 13:23

negative0 mentioned this pull request Aug 9, 2020

bug: Using a non-existent operation when uploading file rclone/rclone-webui-react#102

Open

negative0 mentioned this pull request Aug 21, 2020

Feature Request: Upload files rclone/rclone-webui-react#32

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rc: Add operations/uploadfile to upload a file through rc using multipart/form-data #4221

rc: Add operations/uploadfile to upload a file through rc using multipart/form-data #4221

negative0 commented May 11, 2020 •

edited

calisro commented May 11, 2020

negative0 commented May 12, 2020

calisro commented May 12, 2020

negative0 commented May 12, 2020

ncw left a comment

ncw May 14, 2020

ncw May 14, 2020

ncw May 14, 2020

ncw May 14, 2020

calisro commented May 14, 2020 •

edited

ncw commented May 15, 2020

negative0 commented May 15, 2020

negative0 commented May 15, 2020

ncw commented May 19, 2020

negative0 commented May 21, 2020

ncw commented Jun 4, 2020

ncw commented Jun 5, 2020

negative0 commented Jun 5, 2020

negative0 commented Jun 5, 2020

ncw left a comment

negative0 commented Jun 14, 2020

ncw left a comment

negative0 commented Jun 17, 2020

ncw left a comment

negative0 commented Jun 25, 2020

rc: Add operations/uploadfile to upload a file through rc using multipart/form-data #4221

rc: Add operations/uploadfile to upload a file through rc using multipart/form-data #4221

Conversation

negative0 commented May 11, 2020 • edited

What is the purpose of this change?

Was the change discussed in an issue or in the forum before?

Checklist

calisro commented May 11, 2020

negative0 commented May 12, 2020

calisro commented May 12, 2020

negative0 commented May 12, 2020

ncw left a comment

Choose a reason for hiding this comment

ncw May 14, 2020

Choose a reason for hiding this comment

ncw May 14, 2020

Choose a reason for hiding this comment

ncw May 14, 2020

Choose a reason for hiding this comment

ncw May 14, 2020

Choose a reason for hiding this comment

calisro commented May 14, 2020 • edited

ncw commented May 15, 2020

negative0 commented May 15, 2020

negative0 commented May 15, 2020

ncw commented May 19, 2020

negative0 commented May 21, 2020

ncw commented Jun 4, 2020

ncw commented Jun 5, 2020

negative0 commented Jun 5, 2020

negative0 commented Jun 5, 2020

ncw left a comment

Choose a reason for hiding this comment

negative0 commented Jun 14, 2020

ncw left a comment

Choose a reason for hiding this comment

negative0 commented Jun 17, 2020

ncw left a comment

Choose a reason for hiding this comment

negative0 commented Jun 25, 2020

negative0 commented May 11, 2020 •

edited

calisro commented May 14, 2020 •

edited