-
Notifications
You must be signed in to change notification settings - Fork 234
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
Need support for multipart/form-data while writing interaction #410
Comments
That's correct; we currently support JSON bodies only. |
Is there any ETA for this? If its going to be too far, I can switch to other language e.g. PactJS. But again can someone confirm if Pact JS supports this multipart/form-data file upload? |
Yes Pact JS (the beta) supports multi-part, but I don't see how that would help. You can't write unit tests for a .NET language in JS. |
Note to future self - The FFI has a method for adding multipart messages named |
👋 Thanks, this ticket has been added to the PactFlow team's backlog as PACT-764 |
I've just added a label "smartbear supported" to the ticket, which will create an internal tracking issue in PactFlow's Jira (see that comment above). We will use this to prioritise and assign a team member to this task. All activity will be public on this ticket. For now, sit tight and we'll update this ticket once we have more information on the next steps. |
@mefellows the preferred approach is to create a ticket to propose an API so that we can discuss prior to implementing. That way we can make sure we're all on the same page so we don't waste any effort implementing or go round the code review loop loads of times. |
Thanks Adam, will do. |
Hi @adamrodger, I'm looking into implementing this now. I'll propose an API for it here and we can discuss before I get deep into implementing it. Let me know if you would rather I raise the API proposal elsewhere. The fields needed for the pactffi_with_multipart_file Rust core method are
So I'm proposing the response builder interface would look like this, with the other fields added programmatically:
I am not sure if It is required to create seperate ones for V2 and V3, as from what I can see they seem to be mostly the same? Let me know if both need to be included. I believe in this case the V2 interface will be the same as for V3. I'm only just starting to look at this repo so let me know if you think I am missing anything important. |
I think the problem with that is it only lets you upload one part. Multi-part bodies can contain many parts, some of which may be files and some may be simple form fields or other values. For example, I have an API which takes a file upload and also some extra form fields which tell you how to process the file (e.g. it could be a CSV file and the user would specify the format for date strings, but you can imagine many other scenarios). In that case the body contains 2 parts - one file and one form field - and the above API wouldn't allow for that. At the moment I'm controlling the body in the consumer test by writing the entire thing out using the multi part format with a known separator, but it'd be nice to have a proper API for it. |
So here's how I'm doing it at the moment, which shows that bodies can contain many parts (this is a made up example so as not to copy real code so may not work exactly, but the principle is the same): const string contentType = "multipart/form-data; boundary=\"boundary\"";
string requestBody = string.Join("\r\n",
"--boundary",
"Content-Disposition: form-data; name=file; filename=import.csv; filename*=utf-8''import.csv",
string.Empty,
"header1,header2",
"value1,value2",
"--boundary--",
"Content-Disposition: form-data; name=”dateStyle”
string.Empty,
"dd/MM/yyyy"
"--boundary--",
string.Empty);
this.PactBuilder
.UponReceiving("a file import")
.WithRequest(HttpMethod.Post, "/api/import")
.WithHeader("Content-Length", Match.Number(200))
.WithHeader("Content-Type", contentType)
.WithBody(requestBody, contentType)
.WillRespond()
.WithStatus(HttpStatusCode.OK) In the above proposed API, representing this body wouldn't be possible because only the file part can be added, and only one file as well. Multi-part bodies can also contain multiple files, e.g. if you are uploading images to a photo sharing site and want to upload multiple at once. So the API needs to be really flexible so that each part can be added separately whilst we maintain a single overall body. Something like: this.PactBuilder
.UponReceiving("a file import")
.WithRequest(HttpMethod.Post, "/api/import")
// this number would have to be calculated automatically from all the parts that you've added, otherwise the verifier will fail
//.WithHeader("Content-Length", Match.Number(200))
// this isn't needed because the overload below will set it
//.WithHeader("Content-Type", contentType)
.WithMultiPartBody(body =>
{
body.WithFilePart("import.csv", "text/csv", "header1,header2\r\nvalue1,value2")
// this accept a byte[] and sets the content type to application/octet-stream
.WithFilePart("mypic.png", File.ReadAllBytes("/path/to/example.png"))
.WithFieldPart("dateStyle", "dd/MM/yyyy") // ideally you'd want to use a matcher here
})
// or if you want to choose the boundary separator yourself:
.WithMultiPartBody("my-separator", body =>
{
body.WithFilePart("import.csv", "text/csv", "header1,header2\r\nvalue1,value2")
.WithFilePart("mypic.png", File.ReadAllBytes("/path/to/example.png"))
.WithFieldPart("dateStyle", "dd/MM/yyyy")
})
.WillRespond()
.WithStatus(HttpStatusCode.OK) The various extensions inside the An example from the Swagger docs shows a request body with multiple files and non-file parts:
|
Thanks Adam. I like what you're suggesting, however, I'm not sure the core supports it yet. @uglyog would be able to advise if we can achieve that with the current FFIs available to us. |
Yeah we'd probably have to make core changes to support that, but we can't really add it to PactNet until that's done because otherwise it would be an incomplete solution and a breaking API change for the proper solution. Since this issue was raised we've added the |
Adam just thinking this through. Most users will only need to upload a single file (this current feature request), and most other implementations only support this feature as well (including JS and Java, the two most adopted languages). Would implementing a subset of your proposed API suffice for this request so we can incrementally build this out - e.g. something like this: this.PactBuilder
.UponReceiving("a file import")
.WithRequest(HttpMethod.Post, "/api/import")
.WithMultiPartBody("my-separator", body =>
{
body.WithFilePart("import.csv", "text/csv", "header1,header2\r\nvalue1,value2")
})
.WillRespond()
.WithStatus(HttpStatusCode.OK) This is where I'm a little grey on .NET, but the type returned from Would this be considered a breaking change as far as the .NET client DSL is concerned? Alternatively, implementing the above would be best not mapped onto FFI methods ( |
Yeah that would be a breaking change because the return type of that would have to be void to prevent people trying to add a second file, because that would be undefined behaviour (does the first file win? The second? Is it an error? If so, why did it let you do that in the first place?) Then in future when it should support multiple parts the return type would have to change, so that would be a breaking API change. |
What we could try to do is something like a half way house which only allows you to specify a file and nothing else, e.g. instead of If/when we have proper multi part support in the future, that API would just become a shortcut for the more expressive API, and thus wouldn't be a breaking change. That would need a proper API definition also though, because I think there's quite a lot of nuance there. That API doesn't imply multi part at all (e.g you could imagine a content type of It might turn out that it requires too many assumptions to support properly or be flexible enough, but we can certainly sketch it out and see where we get to. For example, specifying a boundary marker string doesn't make a lot of sense because nothing implies that it's in the multi-part format. |
One thing that stands out in my mind is the content length of such a request. The user is allowed to specify any headers they want, so they can specify a content length header that might just be wrong but they won't know why. They never see the full body format, they're just specifying a file, and they don't know all the assumptions/requirements we've built in (crlf line endings, mandatory blank lines in parts, the boundary and related syntax all count towards the content length, but the user doesn't see any of that). If the content length header is wrong then the verifier tests fail (personal experience with that 😂) because the verifier reports X bytes but sends Y and then the API doesn't receive it properly. So that means we have to calculate the content length for them in order to not make this really awkward to use, and that means we'd need to ignore any one supplied by the user. That probably violates principle of least surprise - the user told us one number but we used a different one in an opaque way. |
Thanks for the (very detailed) suggestions and apologies for the delay (we had a long weekend here in Victoria and I had an extended one). So we had a chat about this and think the halfway house is a decent compromise, and came up with:
What do you think? Addressing some of your other concerns:
Yes, I supposed it might be surprising, but after brief consideration would have to make sense to any user. The only alternative I can think of is to prevent that header from being set if we detect the use of one of these APIs, which I think is a bit overkill personally. I can't think of a good reason as to why a user would want to explicitly set that header. I suspect you may be concerned about it because you previously had to do all of the leg work to set it up using the existing API primitives, wheres in any of the proposed APIs it would be automatically calculated.
We should probably raise a separate ticket for this - being able to support arbitrary content types without multipart boundaries is a basic use case (I'm surprised it hasn't been asked already!). |
I tried the implementation that Adam suggested for uploading one image but my test keeps on failing because the boundary is different each time, any ideas how to overcome the changing boundary?
I also tried to configure the boundary to have a specific value, to prevent it from changing each time. So far my attempts at this have not succeeded. |
Hi @j-leeuw!
What HTTP client are you using? My guess is that it's generating a dynamic boundary name on each request which means it will mismatch what you've put in the expectation. You'll need to be able to set this in your tests so that it's consistent and lines up with the test expectation. |
Hi @adamrodger Is this implemented? And ready to be used in Pactum.js? I am pretty new to Pactum and I have a similar request, POST request with multipart form-data, in which one param is a file attachment and the second one is a JSON object. Let me know if I can do it using pactum.js. |
What has this got to do with Pactum.js? Pact != Pactum |
Follow up to this. @YOU54F drew me attention to this PHP example the other day that supports multiple files It uses repeated calls to |
I think that FFI call must be new because at the time this was originally raised the FFI only supported adding a single file part. That's good to know and probably unblocks implementation of this feature now. I'd have to check out the new API to be sure and we'd also need to design the PactNet API. Likely there'd be some kind of It's been a while since this was first discussed, but I think I remember MIME auto-detection being an issue also, so that may still complicate this feature. If I remember correctly there was no way to turn it off and it gave inconsistent results across different OS and versions. I'd want that to be part of the API when building parts, so it depends what the new FFI offers. |
Yep, it's newer. I think @tienvx added it to support the case. I think the MIME detection is still an open item, albeit there are at least some workarounds for this. I think an option that gives the user some control over the behaviour of the auto-detection makes sense, perhaps that could be contributed to the core if this is picked up. |
This issue has been linked to a Canny post: Need support for multipart/form-data while writing interaction 🎉 |
I am writing a contract test for a post method where I need to upload a file in request Body as 'form-data'. I didn't see any multipart content specific function in PactNet4. All the available functions expecting JSON body
The text was updated successfully, but these errors were encountered: