pact-net icon indicating copy to clipboard operation
pact-net copied to clipboard

Feat multipart file upload support

Open Inksprout opened this issue 2 years ago • 7 comments
trafficstars

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: https://github.com/pact-foundation/pact-net/issues/410

TODO

  • [x] Add documentation for the new method

Inksprout avatar Mar 23 '23 06:03 Inksprout

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

adamrodger avatar Mar 24 '23 16:03 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 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 :)

Inksprout avatar Mar 28 '23 05:03 Inksprout

Any estimate on when this will be completed?

annalauraa avatar Nov 09 '23 18:11 annalauraa

No, the issue is upstream here: https://github.com/pact-foundation/pact-reference/issues/171.

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

mefellows avatar Nov 09 '23 21:11 mefellows

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 - https://github.com/pact-foundation/pact-reference/pull/429

continuation of this PR https://github.com/YOU54F/pact-net/pull/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

YOU54F avatar May 28 '24 22:05 YOU54F

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.

adamrodger avatar May 29 '24 17:05 adamrodger

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

YOU54F avatar May 29 '24 17:05 YOU54F