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

Provide MimeEntity.Load with a contentType parameter #26

Closed
abock opened this issue Jan 3, 2014 · 4 comments
Closed

Provide MimeEntity.Load with a contentType parameter #26

abock opened this issue Jan 3, 2014 · 4 comments
Labels
enhancement New feature or request

Comments

@abock
Copy link

abock commented Jan 3, 2014

When using MimeKit for parsing HTTP content, the Content-Type header will have been removed from the stream already as part of the HTTP header parsing, leaving only the request body data in the stream to consume (at least in all sane/popular frameworks).

This is problematic because MimeKit needs this particular header when parsing. While easy to work around (especially thanks to ChainedStream to avoid copying the entire request body into memory first), it is very non-obvious what the problem is when one might first decide to mix HTTP and MimeKit.

I suggest an API overload for MimeEntity.Load:

MimeEntity.Load (request.InputStream, contentType: request.ContentType)

As a workaround, I have done the following in my production code for now:

var contentType = String.Format ("Content-Type: {0}\r\n\r\n", request.ContentType);
var chainedStream = new ChainedStream ();
chainedStream.Add (new MemoryStream (Encoding.UTF8.GetBytes (contentType)));
chainedStream.Add (request.InputStream);

var multipart = MimeEntity.Load (chainedStream) as Multipart;
...
@jstedfast
Copy link
Owner

Are you using System.Net.WebResponse? I'm just thinking that having a MimeEntity.Load (WebResponse) might be a nicer API than passing in a contentType string and a stream if the assumption is that people will be using WebResponses.

Otherwise, if you are using a 3rd party http stack, then it probably makes sense that I should have an API that takes a stream and content-type parameter.

jstedfast added a commit that referenced this issue Jan 5, 2014
@jstedfast
Copy link
Owner

hmmm, just found that link you sent me with your example usage and it does not appear that you are using System.Net.WebRequest/Response, so I'll probably need to have a contentType parameter.

jstedfast added a commit that referenced this issue Jan 5, 2014
…e,Stream)

This patch better reflects what is needed in issue #26
@jstedfast
Copy link
Owner

I dropped the WebResponse parameter and went with ContentType and Stream, which I think is closer to what you need. I hesitate to use string contentType because it's less obvious what is needed, although maybe it's not so bad.

@abock
Copy link
Author

abock commented Jan 6, 2014

Yeah, System.Net.WebRequest/Response is the old HTTP stack. Best not to write new code/APIs around that. Load(ContentType, Stream) is great. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants