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

Make Content-Type optional in file upload #1385

Closed
tinexw opened this issue Jun 19, 2021 · 6 comments
Closed

Make Content-Type optional in file upload #1385

tinexw opened this issue Jun 19, 2021 · 6 comments

Comments

@tinexw
Copy link
Contributor

tinexw commented Jun 19, 2021

When doing a file upload via withFileUpload the Content-Type header in the individual parts is always set.

This causes an issue when it is used for a multipart/form request in combination with a HTTP client that never sets the header e.g. khttp.

According to the multipart/form-data RFC, the Content-Type is optional. Thus, I would suggest to make it optional here as well.

Example:

See failing test in this repo for an example with pact-jvm and kttps.

The error for the failing test is body - /: Expected a multipart header ‘Content-Type’, but was missing.

Looking at both requests in their string representation in MultipartMessageBodyMatcher while debugging, you can see that the body created by khttp really does not contain the Content-Type header whereas the one created by pact-jvm does:

Actual (created by khttp):

PRESENT(--eb249037d1d440898b25677da6d2c242
Content-Disposition: form-data; name="file"; filename="foobar.gz"

this-is-the-content-of-a-file
--eb249037d1d440898b25677da6d2c242--
)

Expected (created by Pact):

PRESENT(--JTmlQv1PPxLqcuysfPw5dtChH7Foi5pqycujul8E
Content-Disposition: form-data; name="file"; filename="foobar.gz"
Content-Type: text/plain

this-is-the-content-of-a-file
--JTmlQv1PPxLqcuysfPw5dtChH7Foi5pqycujul8E--
)
@tinexw
Copy link
Contributor Author

tinexw commented Jun 20, 2021

I had a quick look and it doesn't seem to be easily possible because the underlying library used to write the body (Apache HttpClient) always sets the inner Content-Type. I tried replacing it with OkHttp but that always included a Content-Length header inside each body part. Which - btw - also means that people using OkHttp as a HTTP client for actual will run in a similar problem as those using khttp.

Maybe the best way would be to just write it ourselves instead of using a library?

Just for future reference the patch for using OkHttp instead:
okhttp-for-multipart.txt

@uglyog
Copy link
Member

uglyog commented Jun 27, 2021

4.2.7 has been released

@tinexw
Copy link
Contributor Author

tinexw commented Jun 27, 2021

Thanks for the quick fix @uglyog
Just to be sure: My example now passes no matter what I specify as Content-Type in the pact file e.g. the following three options all result in a passing test. Is that on purpose?

  • .withFileUpload("file", "foobar.gz", null, fileContent)
  • .withFileUpload("file", "foobar.gz", "text/plain", fileContent)
  • .withFileUpload("file", "foobar.gz", "foobar", fileContent)

@uglyog
Copy link
Member

uglyog commented Jun 27, 2021

Yes, because if the actual request does not have a content type, it will ignore it (i.e. there is nothing to compare the value to).

However, if the actual request does contain a content type for the part, then that should fail if the content type is different.

@uglyog
Copy link
Member

uglyog commented Jun 27, 2021

BTW, using null will just default it to text/plain

@tinexw
Copy link
Contributor Author

tinexw commented Jun 27, 2021

Thanks for the clarification!

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

No branches or pull requests

2 participants