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

Allow multiple files with the same name #81

Closed
dnik2000 opened this issue Mar 8, 2020 · 9 comments
Closed

Allow multiple files with the same name #81

dnik2000 opened this issue Mar 8, 2020 · 9 comments
Assignees
Labels
Breaking Change This change causes backward compatibility issue(s)
Milestone

Comments

@dnik2000
Copy link
Contributor

dnik2000 commented Mar 8, 2020

In a case with multiple files in one request with equal names, the parser concatenates the content of these files in one.
In the attached file an example request with 3 files. The 1st file has a unique name, but the 2nd and 3rd are had equal names. The result of parsing this stream is only 2 files with concatenated content of 2nd and 3rd files in one.
equalnamefiles.txt

@Jericho
Copy link
Collaborator

Jericho commented Mar 9, 2020

Thank you for providing the text file containing a sample. It allowed me to write a unit test that demonstrates that the content of attachments with same name are indeed concatenated.

I had a quick look at the file parsing code and it seems that the name of the attachment is used as a unique identifier and this behavior is deliberate. I don't know if this was intentional or not. Maybe there are situations where a given attachment is broken into multiple parts and concatenating the parts is actually desirable? Or maybe this is simply a bug? I don't know yet, I need to investigate a little more.

@Jericho
Copy link
Collaborator

Jericho commented Mar 9, 2020

Seems to be the same (or similar) problem as described in issue #59

@Vodurden
Copy link
Collaborator

Vodurden commented Mar 9, 2020

When I wrote the behavior I don't think I was aware that a valid multipart upload could have multiple parts with the same name. I didn't change it at the time as I believe it was an interface breaking change but it'd be great if a solution could be found :)

Edit: Actually, I might be thinking about #28 which is a similar but different issue 🤔

@Jericho
Copy link
Collaborator

Jericho commented Mar 9, 2020

@Vodurden thank you for the input. What I found so far is the file handler in MultipartFormDataParser.cs combines consecutive files if they share the same name by comparing the name of the file part with the name of the last file that was added to the Files list:

if (Files.Count == 0 || name != Files[Files.Count - 1].Name)
{
    Files.Add(new FilePart(name, fileName, Utilities.MemoryStreamManager.GetStream(), type, disposition));
}

Files[Files.Count - 1].Data.Write(buffer, 0, bytes);

This means that we have no issue when the parts that have same name are not consecutive. Like this sample:

--boundry
Content-Disposition: form-data; name=""file2.txt"";filename=""file2.txt"";
Content-Type: text/plain

THIS IS TEXT FILE 2 !!!
--boundry
Content-Disposition: form-data; name=""file1.txt"";filename=""file1.txt"";
Content-Type: text/plain

THIS IS TEXT FILE 1
--boundry
Content-Disposition: form-data; name=""file2.txt"";filename=""file2.txt"";
Content-Type: text/plain

This is text file 3 1234567890
--boundry--

I highly doubt that this behavior is intentional and I think it's fair to consider it a bug.

I will correct it and make sure that we don't combine files even when they have the same name.

@Jericho Jericho added the Bug This change resolves a defect label Mar 9, 2020
@Jericho
Copy link
Collaborator

Jericho commented Mar 9, 2020

Continuing my investigation I now understand that the file handler in MultipartFormDataParser.cs must indeed be able to handle reading a given attachment in chunks which explains why it checks if the name of the file matches the name of the previously processed chunk and, if so, it appends the content of the chunk to the previously processed chunks. This behavior is intentional and desirable.

@dnik2000 The more I research this issue, the more I think the PR you submitted makes sense. At first I didn't understand why you proposed keeping track of the "part number" of a given attachment but it's becoming clear to me that it allows keeping track of which chunks belong to which attachment without relying on the name of the attachment.

@dnik2000
Copy link
Contributor Author

dnik2000 commented Mar 9, 2020

I don't know the HTTP protocol very well. Maybe combining consecutive chunks under one name is described in the standard. But I have a device, which sends multiple files in one request under the same names. And I think, I should be able to send two files with equal names from different folders in one form.

@Jericho
Copy link
Collaborator

Jericho commented Mar 9, 2020

Thinking out loud: is adding a parameter to a delegate (FileStreamDelegate in this case) considered a breaking change which therefore requires increasing the major version in the next release?

@Jericho Jericho added this to the 4.0.0 milestone Mar 10, 2020
@Jericho Jericho changed the title Parser concatenate files with equal names Allow multiple files with the same name Mar 10, 2020
@Jericho Jericho added Breaking Change This change causes backward compatibility issue(s) and removed Bug This change resolves a defect labels Mar 10, 2020
@Jericho
Copy link
Collaborator

Jericho commented Mar 10, 2020

We changed a public delegate, therefore we must consider this solution a 'breaking change' and, accordingly, increase the next major version. I'm preparing release 4.0.0.

@Jericho
Copy link
Collaborator

Jericho commented Mar 11, 2020

🎉 This issue has been resolved in version 4.0.0 🎉

The release is available on:

Your GitReleaseManager bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change This change causes backward compatibility issue(s)
Projects
None yet
Development

No branches or pull requests

3 participants