Skip to content
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.

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

Feature: Http Multipart (File upload) #694

Closed
patrickjahns opened this issue Apr 12, 2016 · 12 comments
Closed

Feature: Http Multipart (File upload) #694

patrickjahns opened this issue Apr 12, 2016 · 12 comments

Comments

@patrickjahns
Copy link
Member

In the branch "feature/http-multipart-post" I am currently extending the webserver to include multipart requests (file uploading).

Please help to test the feature/branch and provide feedback in this issue


Copy of the Initial PR:

Currently it provides functionality to:

  • upload single files
  • upload multiple files at once
  • upload files with additional vars

To enable uploading for a certain path, a delegate has to be provided:
void addPath(String path, HttpPathDelegate callback, HttpUploadDelegate uploadcallback);

The uploadcallback function is defined as following:

void functionName(HttpRequest&, HttpUpload&)

The function has thus access to the current request in the sense of all Header and Query Paramters (as well as path) can be accessed to decide if we accept the upload (or not)

In the pathdelegate information about the uploaded files is accessible by calling:

if(request.hasUpload()) {
    Vector<HttpUpload*> uploads = request.getUploads();
}

HttpUpload is defined as

struct HttpUpload {
    String filename;
    String name;
    String type;
    size_t totalSize;
    size_t curSize; // size of the current data block
    char* bufferdata; //pointer to the data
    file_t file;
    HttpUploadStatus status;
};

after a successfull upload curSize and bufferdata will be NULL and not accessible anymore (to avoid tampering with it
type contains the content-type field values
name is the variable name defined in the form field

If we receive a http-multipart-request without a filename or if the filename is empty - the data will be treated as normal http-post parameters and is accessible via getPostParameter - the limit NETWORK_MAX_HTTP_PARSING_LEN applies to all data that is not a file.

For testing see the working examples in samples/HttpServer_Upload

Please test it thoroughly and provide feedback. If you encounter any issues, it would be great if you document what has lead to the issue and provide a debug log.

Do you have any suggestion/remarks?

Known limitations/issues:

  • when providing a filename but no content - a file with the filename but 0 size will be created
    (it seems that chrome sends the header again when one presses go-back to the fileupload page)
  • max lenght for filenames is 31chars (current spiffs setting)
    how should we handle this use case? how to inform the user - automatically or should the "user of the framework" take care of this in the callback?
@hreintke
Copy link
Contributor

For all upload requests I get a Bad Request reply.
Error log :

path=/upload
host === 10.0.0.210
parsed
Request: GET, nodata
TCP received: 327 bytes
onReadyToSendData: 1
response sendHeader
response sendBody
Stream completed
TCP connection closing
~TCP connection
-TCP connection
onAccept state: 0 K=0
Free heap size=38568, K=0
+TCP connection
timeout updating: 70 -> 90
TcpServer onClient: 10.0.0.219

path=/upload
host === 10.0.0.210
multipart - boundary === ---------------------------191781408624973
content-type === multipart/form-data
content-length === 5615
content-type === application/octet-stream
parsed
Request: POST, 5615 bytes
NETWORK_MAX_HTTP_PARSING_LEN
POST FAILED
SEND ERROR PAGE
TCP received: 1460 bytes
onReadyToSendData: 1
TCP connection closing
~TCP connection
-TCP connection
onAccept state: 0 K=0
Free heap size=38568, K=0
+TCP connection
timeout updating: 70 -> 90
TcpServer onClient: 10.0.0.219

TCP received: (null)
TCP connection closing
~TCP connection
-TCP connection

It is filesize dependent. I presume it is hitting the NETWORK_MAX_HTTP_PARSING_LEN boundary.
With smaller files I get :
No uploaded data

@patrickjahns
Copy link
Member Author

I am just on mobile.. But I think I know where this issue is related to:
what browser / OS are you using?

The first paket does include the header and already data. (the reverse to
what I am experiencing on Windows 10 and chrome). Thus the header parsing
function reads the http header already into the multupart header and thus
treats it as normal post which runs into memory limitations
On Apr 12, 2016 16:46, "hreintke" [email protected] wrote:

For all upload requests I get a Bad Request reply.
Error log :

path=/upload
host === 10.0.0.210
parsed
Request: GET, nodata
TCP received: 327 bytes
onReadyToSendData: 1
response sendHeader
response sendBody
Stream completed
TCP connection closing
~TCP connection
-TCP connection
onAccept state: 0 K=0
Free heap size=38568, K=0
+TCP connection
timeout updating: 70 -> 90
TcpServer onClient: 10.0.0.219

path=/upload
host === 10.0.0.210
multipart - boundary === ---------------------------191781408624973
content-type === multipart/form-data
content-length === 5615
content-type === application/octet-stream
parsed
Request: POST, 5615 bytes
NETWORK_MAX_HTTP_PARSING_LEN
POST FAILED
SEND ERROR PAGE
TCP received: 1460 bytes
onReadyToSendData: 1
TCP connection closing
~TCP connection
-TCP connection
onAccept state: 0 K=0
Free heap size=38568, K=0
+TCP connection
timeout updating: 70 -> 90
TcpServer onClient: 10.0.0.219

TCP received: (null)
TCP connection closing
~TCP connection
-TCP connection

It is filesize dependent. I presume it is hitting the
NETWORK_MAX_HTTP_PARSING_LEN boundary.
With smaller files I get :
No uploaded data


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#694 (comment)

@hreintke
Copy link
Contributor

I am on windows 8.1, Firefox 44.

Tried with Chrome
-> works for small & large textfiles.(not much tested)
-> fails for binary files

@patrickjahns
Copy link
Member Author

Can you send me the binary files? I tested with images and pdfs
On Apr 12, 2016 18:07, "hreintke" [email protected] wrote:

I am on windows 8.1, Firefox 44.

Tried with Chrome
-> works for small & large textfiles.(not much tested)
-> fails for binary files


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#694 (comment)

@patrickjahns
Copy link
Member Author

I can confirm that firefox handles it differently.
Interestingly I figured out why for me header and body is split - it`s because of Fiddler (a tool to inspect http requests)

Will need to dive deeper into this

@alonewolfx2
Copy link
Member

OK. another idea. can you extend your http upload example with sdcard ? ;)

@patrickjahns
Copy link
Member Author

Currently not - there are several things to consider before extending the upload to anything but spiffs.

  • before we allow uploading files to sdcard - we need to extend the webserver to serve files from sdcard as well. Atleast I didn`t find any example
  • in order to keep my sanity it would be good to have an easy way to writing to sdcard - I don`t see it viable to use the methods from the SDCARD example. (it would bloat the upload example and in my opinion cause more trouble in the long run)
    AFAIK @hreintke is working on an interface for this
  • currently I check for the free (filesystem) space directly in the first paket in order to terminate the request as early as possible. Moving to a later part needs to be tested - most probably the interface for the upload callback needs to include then also a response object to be able to terminate any request.
    Also handling the free space check in the callback creates another issue: we only know the total of all files and data transmitted by the http client - so we could only check for free space in the first call of all callbacks (an upload could have more than one file)

For now lets keep things as simple as possible and extend it once this part is stable

@hreintke
Copy link
Contributor

@alonewolfx2 :
I agree with @patrickjahns on this. first implementation of http_post is spiffs files and flash to allow for ota updates. That is functionality completely within Sming Core.

The SDCard is a library, not part of Sming Core and as such does not have a "Sming designed" interface. We can not use that for core functionality.
It MIGHT be possible to define a generic interface which libraries then can implement to facilitate data transfer

@patrickjahns
Copy link
Member Author

I pushed some fixes into #697 for the issues @hreintke noticed so far.

Currently the code has some "extensive" logging inside - please test it some more and if an error occurs it would be great to provide the logfile.

@Matheus-Garbelini
Copy link

Matheus-Garbelini commented Dec 8, 2016

@patrickjahns Hello, i can't compile your example with the branch you supplied. Could you try to update sming to the latest version, so we can compile your example with the updated Sming? Or could i just replace some Sming core files to match the modifications you did? if yes, wich files? Thanks

@anakod
Copy link
Member

anakod commented Feb 11, 2017

In my opinion it's important feature and should be merged to Sming.

@slaff
Copy link
Contributor

slaff commented May 4, 2017

Post parameters with multipart encoding should be implemented with the upcoming Http refactoring (#1112).

@slaff slaff closed this as completed May 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants