Criollo icon indicating copy to clipboard operation
Criollo copied to clipboard

Multi-file uploads fail when placed under same 'name' attribute in html form

Open itsjunetime opened this issue 3 years ago • 3 comments

It appears, in my limited testing, that when you have an html form with multiple files being uploaded under the same name attribute, only one of the files is available to manipulate in the request completion handler. Example code:

HTML form (with which multiple files are uploaded):

<form action="uploads" method="POST" enctype="multipart/form-data">
    <input type="file" name="files" multiple></input>
    <button type="submit">
</form>

Swift handler:

server.add("/uploads") { (req, res, next) in 
    print(req.files) /// Only prints `["files": <CRUploadedFile: 0x2823a9b80>]`
}

When I upload multiple files the input above, only the one file (as shown in the swift bit) appears to be available. I believe this is due to -(BOOL)appendFileData:(NSData *)data forKey:(NSString *)key in CRRequest.m. It seems that it treats the name attribute's value of the file's input tag as the dictionary key, and so overwrites each new file that comes along with that same name attribute. Is there a simple fix to this (or did I accidentally read over some crucial part of the documentation)?

itsjunetime avatar Aug 13 '20 22:08 itsjunetime

Hi @iandwelker,

That's a very accurate assessment. To tell you the truth I don't think I considered HTML 5 multiple file input fields back when I wrote this.

Log story short, yes this could be an easy enhancement to make. I see three options:

  1. The values in the files dict could be either CRUploadedFile or [CRUploadedFile], depending on the scenario ["files": [<CRUploadedFile: 0x...>, <CRUploadedFile: 0x...>], "another_file": <CRUploadedFile: 0x...>]
  2. It could add more entries in the dictionary under different keys ["files[0]": <CRUploadedFile: 0x...>, "files[1]": <CRUploadedFile: 0x...>]
  3. The values in the files dict would always be a [CRUploadedFile], containing one or more files ["files": [<CRUploadedFile: 0x...>, <CRUploadedFile: 0x...>], "another_file": [<CRUploadedFile: 0x...>]]

Do you have another idea? Which one of these would seem more natural to you?

Is there an accepted practice? What do web languages/frameworks do? (rails, php, express, .net)

Thanks for using Criollo and taking the time to dig and write.

Best, Cătălin

thecatalinstan avatar Aug 13 '20 22:08 thecatalinstan

I'm not an experienced web developer (I've only ever used php out of those you listed), so I really don't know what the accepted practice would be, but I'm fairly certain php's solution is similar to option 3 in your comment above. I like that solution, since it makes the return value easier to deal with (so you don't have to check if every file is an array or just a single file), but I feel it could break apps that currently use Criollo if every file is now treated as an array of files.

I feel like option 1, with a dynamic return type based on how many files were uploaded, would be much better for use here. Anyone who was previously using this framework to upload files would still have it work just fine, but it would also support multi-file uploads for those who would like to use it for that. Option 2 would probably work as well, but the framework would then be inserting values into the files that the user didn't ask for, and although it probably wouldn't be too bad to deal with, I think option 1 just does the job more efficiently and cleaner. It also seems to better line up with how the corresponding html tags are formatted.

itsjunetime avatar Aug 14 '20 01:08 itsjunetime

I made a pull request that should fix this issue, implementing option 1 that you mentioned before. Could you check it out and let me know what you think?

itsjunetime avatar Aug 14 '20 19:08 itsjunetime