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

Need support for multipart/form-data while writing interaction #410

Open
prasadsolanki opened this issue Jul 1, 2022 · 25 comments
Open
Labels
feature request Indicates new feature requests

Comments

@prasadsolanki
Copy link

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
image

@adamrodger
Copy link
Contributor

That's correct; we currently support JSON bodies only.

@prasadsolanki
Copy link
Author

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?

@mefellows
Copy link
Member

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.

@adamrodger
Copy link
Contributor

Note to future self - The FFI has a method for adding multipart messages named pactffi_with_multipart_file

@github-actions
Copy link

👋 Thanks, this ticket has been added to the PactFlow team's backlog as PACT-764

@mefellows
Copy link
Member

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.

@adamrodger
Copy link
Contributor

@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.

@mefellows
Copy link
Member

Thanks Adam, will do.

@Inksprout
Copy link

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

  • Interaction: this is already available in the existing code
  • part: this is already available in the existing code
  • content_type: We can hard code this as we've done with the 'WithJsonBody' Request builder, since this api will only be for multipart/form-data content type. (that's my understanding of the requirements at least)
  • file: User will need to pass in the absolute file path, as the current directory when the tests run could be inside bin/Debug.
  • part_name: This is the delimiter in the body content that marks the different parts. There is no matching performed on this string as part of pact file verification so it is needed to call the Rust library but can be an arbitrary string. For this reason we can set a sensible default as well.

So I'm proposing the response builder interface would look like this, with the other fields added programmatically:

IRequestBuilderV3 IRequestBuilderV3.withMultipartFileUpload(string filePath, string partName = "partDelimiter")
   => this.withMultipartFileUpload(string filePath, string partName);

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.

@adamrodger
Copy link
Contributor

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.

@adamrodger
Copy link
Contributor

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 WithMultiPartBody lambda would be responsible for setting the Content-Disposition properly.

An example from the Swagger docs shows a request body with multiple files and non-file parts:

POST /upload HTTP/1.1
Content-Length: 428
Content-Type: multipart/form-data; boundary=abcde12345

--abcde12345
Content-Disposition: form-data; name="id"
Content-Type: text/plain

123e4567-e89b-12d3-a456-426655440000
--abcde12345
Content-Disposition: form-data; name="address"
Content-Type: application/json

{
  "street": "3, Garden St",
  "city": "Hillsbery, UT"
}
--abcde12345
Content-Disposition: form-data; name="profileImage "; filename="image1.png"
Content-Type: application/octet-stream

{…file content…}
--abcde12345--

@mefellows
Copy link
Member

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.

@adamrodger
Copy link
Contributor

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 WithBody overload which lets you specify anything you want. I'm using that to verify multi-part requests in production usage currently (as shown above), so there's a known workaround that wasn't available when this was first raised.

@mefellows
Copy link
Member

mefellows commented Mar 8, 2023

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 body.WithFilePart would initially be a type with 0 methods on it, and if we later want to support the additional methods you proposed they could be added later.

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 (pactffi_with_multipart_file is not designed to be called multiple times) and essentially just re-using existing capability (i.e. the way you have worked around the issue, could also be used to implement your proposed API), but this adds additional complexity.

@adamrodger
Copy link
Contributor

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.

@adamrodger
Copy link
Contributor

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 WithJsonBody you'd have something like WithSingleFileBody.

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 text/csv and just basic text in the body, not with all the boundary markers and stuff).

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.

@adamrodger
Copy link
Contributor

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.

@mefellows
Copy link
Member

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:

withMultipartSingleFileUpload - same signature as above, but it specifies the intent clearer. It's multipart, it's a file being transferred and there only one of them. Upload probably implies multipart and not just a RESTful content transfer, but perhaps being more explicit is best.

What do you think?

Addressing some of your other concerns:

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.

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.

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.

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 text/csv and just basic text in the body, not with all the boundary markers and stuff).

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!).

@j-leeuw
Copy link

j-leeuw commented Jul 5, 2023

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?

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)

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.

@mefellows
Copy link
Member

Hi @j-leeuw!

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?

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.

@smalhotra2210
Copy link

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.
Thanks.

@mefellows
Copy link
Member

mefellows commented May 14, 2024

What has this got to do with Pactum.js? Pact != Pactum

@mefellows
Copy link
Member

Follow up to this. @YOU54F drew me attention to this PHP example the other day that supports multiple files

It uses repeated calls to pactffi_with_multipart_file_v2 to allow this.

@adamrodger
Copy link
Contributor

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 WithMultipartContent(Action<IMultipartBuilder>) method on the request/response builders, where the IMultipartBuilder lets you call AddPart multiple times. You'd probably need to be able to specify the separator as well.

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.

@mefellows
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Indicates new feature requests
Projects
Status: New Issue
Development

No branches or pull requests

6 participants