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

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

Open
itsjunetime opened this issue Aug 13, 2020 · 3 comments
Milestone

Comments

@itsjunetime
Copy link

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 itsjunetime changed the title Multi-file uploads fail when place under same 'name' tag in html form Multi-file uploads fail when placed under same 'name' attribute in html form Aug 13, 2020
@thecatalinstan
Copy link
Owner

thecatalinstan commented Aug 13, 2020

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

@itsjunetime
Copy link
Author

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
Copy link
Author

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?

@thecatalinstan thecatalinstan added this to the 2.0.0 milestone Jun 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants