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

Fix multipart to set its header even when other headers are provided without overwriting provided content type #608

Conversation

meuble
Copy link

@meuble meuble commented Aug 24, 2018

In regards to issues opened about Content Type issue with multipart requests, also mentioned here in comments by @jirutka, there was a fix implemented.

I think this fix is incorrect as it prevent any Content Type customisation: if the request is multipart, it is always overwritten.
I propose a fix here that allow for Content Type customisation, but also makes sure that if we have custom headers but no Content Type, the content type is set to multipart/form-data.

@meuble
Copy link
Author

meuble commented Aug 24, 2018

We may want to include @TheSmartnik in the discussion to assess the viability of this pull request.

@TheSmartnik
Copy link
Collaborator

TheSmartnik commented Aug 24, 2018

@meuble I've looked at the issue you've mentioned. Thanks for the pr, but It seems the problem in the issue is in this line. I simply hardcoded octet-stream, without detecting content-type of the file

@meuble
Copy link
Author

meuble commented Aug 24, 2018

@TheSmartnik That might be another issue.
The fact that we can't customise the Content Type header for requests with attachment remains, unless with the fix in my pr.

I don't claim to fix issue #595 here, but as it was a related issue, I though better to mention it.

@TheSmartnik
Copy link
Collaborator

@meuble To be honest, I can't think of any case where one would need to send a file content with any different content-type. Could give an example, please?

@meuble
Copy link
Author

meuble commented Aug 24, 2018

Yes.
Please refer to the following documentation http://rasa.com/docs/nlu/master/http/
As they specify:

The request should always be sent as application/x-yml regardless of wether you use json or md for the data format. Do not send json as application/json for example.

To do so, the HTTParty code I should write to query this API is the following

response = HTTParty.post("#{BASE_URL}/train", 
  body: {:file => File.open("intent_utterances.md")}, # Yes, it's a markdown file, here. Not a Yaml...
  headers: {'Content-Type' => "application/x-yml"}
)

In the current state of HTTParty, the Content Type that is sent to the API is multipart/form-data and I get an error. After my PR, the Content Type is application/x-yml and every thing is fine.
I agree that HTTParty should extract the content type from the file, if none is provided.
But even if HTTParty does it, I still want to have the freedom to specify a custom Content Type.

@TheSmartnik
Copy link
Collaborator

TheSmartnik commented Aug 26, 2018

@meuble I see. Thanks!

You don't need multipart in this case. Following documentation, what you need to do is to send file directly. With httparty, you would do so, by streaming content of a file. Like so

HTTParty.put(
  'http://localhost:5000/train',
  body_stream: File.open('sample_configs/config_train_server_md.yml', 'r'),
  headers: { 'Content-Type' => 'application/x-yml', 'Transfer-Encoding' => 'chunked' }
)

This should produce expected payload.


Uploading files and uploading files through multipart is two different things. If we encode body params as multipart, but change content-type to anything else. Server won't be able to decode it

@meuble
Copy link
Author

meuble commented Aug 28, 2018

Ok, I wasn't aware of body_stream as a way to add files to HTTP requests.
I close the pull request. Sorry for the inconvenience...

@meuble meuble closed this Aug 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants