Feat multipart file upload support

[Inksprout]

This PR aims to add partial support for Multipart/form-data requests to be added to the Pact File.
It introduces a WithMultipartSingleFileUpload() method that allows the user to to specify a request of multipart/form-data in which a single part is uploaded, which is a file.

This functionality is a common use case and uses the underlying Rust FFI method designed for this express purpose.
Note this update does not enable Pact-Net to fully support all types of multipart/form-data requests as it does not support requests with multiple parts included.

see issue: #410

TODO

• Add documentation for the new method

[mefellows]

Looks good so far. I only have bike shed comments to offer at this stage :)

[adamrodger]

Nice, I'll give this a review later on 👍

[adamrodger]

In addition to the comments, I'd like to see an additional test for the verifier side so that we can confirm the entire round-trip works.

[adamrodger]

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.

[adamrodger]

I've decoded this body and I'm confused by something. The body is:

--LCSjq3qukfbg6Z2S
Content-Disposition: form-data; name="file"; filename="test_file.jpeg"
Content-Type: image/jpeg

<binary data>
--LCSjq3qukfbg6Z2S--

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.

[Inksprout]

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.

[adamrodger]

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?

[adamrodger]

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.

[adamrodger]

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.

[mefellows]

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.

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 as application/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 ?

[rholshausen]

Yes, that is correct

[adamrodger]

Worth noting that I don't think the current behaviour conforms to the spec, which says

If there is a Content-Type header, the value of the header determines the content type.

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:

default to either application/json (as V1) or text/plain.

Because the default is application/octet-stream instead of text/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.

[Inksprout]

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.

[Inksprout]

In addition to the comments, I'd like to see an additional test for the verifier side so that we can confirm the entire round-trip works.

@adamrodger I wasn't 100% sure where to add these but I have added tests into Samples/EventAPI consumer and provider to generate and verify a pact file with this type of body. It includes the API endpoint used. let me know what you think or if you had something else in mind :)

[annalauraa]

Any estimate on when this will be completed?

[mefellows]

No, the issue is upstream here: pact-foundation/pact-reference#171.

If you are willing to help we'd appreciate it.

[YOU54F]

Will need to split out these changes but just wanted to say I revisited this, with some updates to pact-ref which are proposed here for consistent cross platform payload matching - pact-foundation/pact-reference#429

continuation of this PR YOU54F#1

also adds in linux arm64 + musl arm64/x64 targets and slimmed binaries and tests them with docker (exception for arm64, which is tested locally as dotnet doesn't support running under qemu), so I need to pull those out. Just wanted to show that we are getting to a better place and hopefully closer to getting this merged.

Thanks @Inksprout for starting it off! Hope you are doing well.

Thanks for the review so far and concerns raised @adamrodger

[adamrodger]

I think this PR should be closed and an RFC raised with a proposed new API so we can discuss. Nobody likes pushing up a PR only to be told the API won't work or something, so let's get agreement on that first.

I think I started outlining a potential API in the feature request issue for this, so we can carry on the discussion there. The important thing is that there needs to be some kind of fluent builder to allow you to add multiple parts with independent content types and that needs to fit into the existing typestate pattern to prevent you from then being able to set any other body type.

[YOU54F]

I think this PR should be closed and an RFC raised with a proposed new API so we can discuss.

Yeah that is fine with me, I'm not attached to this API in particular, I just used it as a test bed to test out the content type matching in order to get consistent behaviour across platforms and archs

It would be nice to thank the original contributor for their attempt however, when it is closed.

Nobody likes pushing up a PR only to be told the API won't work or something, so let's get agreement on that first.

I don't think that is a universal statement, I don't mind at all. I tend to experiment on my own and propose something via code, and take from that the learning of getting up to that point, rather than heavily investing in proposing something relatively concrete