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

set content type for multipart form #109

Closed
gh0st42 opened this issue Dec 18, 2017 · 12 comments
Closed

set content type for multipart form #109

gh0st42 opened this issue Dec 18, 2017 · 12 comments
Assignees

Comments

@gh0st42
Copy link

gh0st42 commented Dec 18, 2017

The SetFile() and SetFileReader() functions always set the content-type to application/octet-stream but I need to set it to text/plain. There is no obvious way to set individual content types for multiple parts attached to a request. SetHeader() with content-type affects the main request.
For the web-service I try to use I need a request that looks like the following:

POST http://127.0.0.1:4110/sendmessage
Content-Length = 187
Proxy-Connection = Keep-Alive
Accept = */*
User-Agent = curl/7.47.0
Host = 127.0.0.1:4110
Content-Type = multipart/form-data; boundary=------------------------ff98ce2f2af09ab5
Accept-Encoding = identity

--------------------------ff98ce2f2af09ab5
Content-Disposition: form-data; name="message"
Content-Type: text/plain;charset=utf-8

msg 1
--------------------------ff98ce2f2af09ab5--

The following code comes quite close:

message := "msg 1"
resp, err := resty.R().SetFileReader("message", "message1", bytes.NewReader([]byte(message))).Post("http://localhost:4110/sendmessage")

Producing the following POST request:

POST http://localhost:4110/restful/meshms/3B52DDA010E1ECF8BCBDFECF8B231A49759CEA71A452525C2EAAE96D7B5EEF5D/761353EE496B911E72BDFB390E124C8C03057BCB20E41EC68802502FBCF8330A/sendmessage
Content-Length = 248
Accept-Encoding = gzip
User-Agent = go-resty v1.0 - https://github.com/go-resty/resty
Host = localhost:4110
Content-Type = multipart/form-data; boundary=1086b448cee8019bec34a9b227a7f2073af301c3aba18ce8351c6ba1a76b

--1086b448cee8019bec34a9b227a7f2073af301c3aba18ce8351c6ba1a76b
Content-Disposition: form-data; name="message"; filename="message1"
Content-Type: application/octet-stream

msg 1
--1086b448cee8019bec34a9b227a7f2073af301c3aba18ce8351c6ba1a76b--

Am I overlooking something or is this functionality simply not implemented?

The rest of the API is very straight forward and easy to use, great work!

@jeevatkm
Copy link
Member

jeevatkm commented Dec 20, 2017

@gh0st42 Thank you for your appreciation. resty uses package mime/multipart to populate the request info and body. Resty doesn't explicitly set content type. Actually package mime/multipart sets the content type of the file internally.

@jeevatkm jeevatkm self-assigned this Dec 20, 2017
@gh0st42
Copy link
Author

gh0st42 commented Dec 21, 2017

Okay, adding a previously create multipart on the fly is not supported by resty so I either have to build the complete request myself OR patch resty. I opted for the latter and made a pull request implementing your solution in resty. So far I only did this for the SetFileReader function but this could easily be done for the others as well.
I hope my solution is clean enough and suits your API design but having more control over multipart mime is heavily needed to interact with picky web apps/APIs/servers.

@oadk
Copy link

oadk commented Jan 5, 2018

Hi, thanks for making Resty!

I just wanted to add that this feature would be extremely useful for me too. I will patch my source using gh0st42's PR for now (thanks gh0st42!), but do you know when you expect to have a fix implemented as part of the mainline?

Thanks

@jeevatkm
Copy link
Member

jeevatkm commented Jan 5, 2018

@oadk I'm glad to hear that. For now please use @gh0st42 patch.

I would be adding generic solution as earliest as possible for SetFile() and SetFileReader() inspired by @gh0st42 idea so additional method may not required. It would benefit everyone. Thanks again.

@oadk
Copy link

oadk commented Jan 5, 2018

Ok, I will do that for now. Looking forward to the generic solution too.

Thanks for your help.

@jeevatkm
Copy link
Member

@gh0st42 I have added implementation of autodetect multipart content type at branch multipart-content-type - 513d3b0

Can you please try it out and share your feedback? then I will create a PR.

@jeevatkm
Copy link
Member

@oadk can you please try it out and share your feedback?

@oadk
Copy link

oadk commented Jan 23, 2018

Hi, thanks for making the change!

I have tried it out and unfortunately there is a problem. It appears that in some circumstances, too much data is being written for the file data. I have looked through the code, and I think the problem is in writeMultipartFormFile() on this line:

_, err = partWriter.Write(cbuf)

The problem is that when we get to the end of the file, cbuf will not be completely full. Therefore, we should not write the entire buffer contents. I think the line should really be like this:

_, err = partWriter.Write(cbuf[:size])

Also, I think the other partwriter.Write() line further up in writeMultipartFormFile() (where the very first write is done) will need to do the same, to handle the case where the whole file is less than 512 bytes in size.

Hope this helps. Thank you for looking into the feature.

@jeevatkm
Copy link
Member

@oadk Thank you for your feedback and testing. Its very helpful.

@jeevatkm
Copy link
Member

@oadk I have created PR with changes #118, please have a look and let me know.

@oadk
Copy link

oadk commented Jan 24, 2018

Hi @jeevatkm, I have tried this version and it works great! Thank you :)

@jeevatkm
Copy link
Member

@oadk Nice, thank you. Glad to hear that. I'm gonna merge the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants