Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feat multipart file upload support #447
base: master
Are you sure you want to change the base?
Feat multipart file upload support #447
Changes from 11 commits
a268fdb
001dcbf
1fc59a8
12dd92f
1a95e95
d4acbe0
22f05d4
b41dfc0
39b1304
dad71c8
058e67f
5b5296b
ab813e6
1cb75df
29d719b
d0b9dc5
18502aa
3246f34
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this isn't what I expected to see... How can we validate that the body is actually a multi-part one in the correct format if the entire body is base64 encoded? That's made the interaction pretty opaque, which will make it hard to debug if the verifier fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've decoded this body and I'm confused by something. The body is:
but where does the
Content-Type: image/jpeg
come from? The public API doesn't allow you to specify the content type of a part as far as I can see. Is that being auto-detected or something? Either way, you should probably be able to set it yourself.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a similar issue to the "file" one you mentioned, I have updated the code to include this as a parameter the user sets. There is a limitation here I am planning to document however, the content type matching in the Rust library uses the Shared MIME-info Database (https://specifications.freedesktop.org/shared-mime-info-spec/shared-mime-info-spec-latest.html) to do the binary comparison. This library is GNU licensed, so we can't embed it in the framework, it has to be installed in the system. It is not supported in windows, so while content matching will work as expected in a Unix type build pipeline, the content type will only come through as 'application/octet-stream' when using windows. You can see the test I added reflects this, as I have to check and set the content type appropriately so that test can pass in each of the test environments the repo has.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow, that's quite the gotcha I wasn't aware of. So running the same test on Windows and Linux will produce different pact files? That doesn't sound good.
What if you run it on Linux with and without the mime detection library installed? Does it error or give you different results also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just realised the auto detect probably breaks my use case as well, because we support multiple types of upload (e.g. you could upload a CSV or XLSX file) and we need control over the mime type so we can handle the file properly on the receiver side. There's no way to guarantee that the auto detect generates the mime we need, especially if it's a vendor specific mime.
We've had issues raised by other users when we hard coded the content type for JSON bodies as well, because sometimes people want to add extensions such as
application/json+patch
or they have to integrate with other systems which don't behave themselves.Also, the round trip integration test that I've asked for will fail in CI because we run it on all 3 major OSs and verify the file contents afterwards. A test library that produces different results on different OSs isn't really desirable. We shouldn't be building OS specific switches into the tests either, that's an obvious code smell.
I think that's big enough to be a blocker to me if the tests aren't reproducible and the reason for that is unclear to the user. That's a foot gun that's too easy to trigger, and then all the issues will start coming in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really willing to skip the tests on Windows for a .Net library.
Windows is the main development OS for .Net by far, but obviously they also need to work in other OSs as people increasingly run in dev containers or run their CI on different OS.
A pretty typical workflow, and the one everyone uses at my work, is for people to develop on Windows but run CI and production in Linux containers. We need the same tests to produce the same results in that situation.
I honestly think this change is blocked into we get an FFI behaviour change. Fortunately the issue is linked now so we can track it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, the issue here will be that the CI will publish a contract with the correct MIME type e.g.
image/jpeg
and any verification process on a Windows development machine will fail, because the type will be detected asapplication/octet-stream
.If I further understand the implementation detail, even if you attempted to do the manual workaround you suggested Adam (setting content-type header, length and explicitly laying out the multi-part boundaries) the FFI will still do the mime byte check and fail with the same issue.
Am I right in assuming that @rholshausen ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth noting that I don't think the current behaviour conforms to the spec, which says
That means I'd expect to be always be able to control the content type by passing the appropriate header. In the case of multi-part then obviously the content type header applies to each part separately.
It also doesn't conform to this part:
Because the default is
application/octet-stream
instead oftext/plain
, and the spec notes that the magic byte test (although the spec incorrectly calls it 'magic number test') is optional.I think the default should be that the user has to specify a content type, with an opt-in for auto-detection. That way we can document that it may work differently on different OSs and the user can conciously make that choice. If it doesn't work, e.g. because they use different OS or the OS fails auto-detection, then they've always got the option to specify manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That section of the spec is referring to the Content-Type of the body of a request, however the issue here is with the Content-Type of the file being uploaded. Requests with a file as part of them include both content types. In the case of using the pactffi_with_multipart_file method from the ffi, the detection of the body Content-Type should work according to the spec. The Content-type of the file being uploaded is what the pactffi_with_multipart_file method helps match. This is the content type that is not able to be detected correctly when using Windows. Hope that helps clear up the issue.