Skip to content

Include parameters in multipart POST request #338

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from
Open

Include parameters in multipart POST request #338

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
df2dffb
Include parameters in multipart POST request
fluiddot Jan 28, 2021
8477576
Add body parts to multipartPOST request
fluiddot Jan 29, 2021
648bb7f
Merge branch 'develop' into fix/333-multipart-post-parameters
fluiddot Jan 29, 2021
d5c505c
Update WordPressKit/PostServiceRemoteREST.m
fluiddot Feb 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions WordPressKit/WordPressComRestApi.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,11 +290,11 @@ open class WordPressComRestApi: NSObject {
}

/**
Executes a multipart POST using the current serializer, the parameters defined and the fileParts defined in the request
Executes a multipart POST to the specified endpoint defined on URLString, including the parameters and the fileParts defined in the request.
This request will be streamed from disk, so it's ideally to be used for large media post uploads.

- parameter URLString: the endpoint to connect
- parameter parameters: the parameters to use on the request
- parameter parameters: the parameters to use on the request (only String and Int parameters will be included)
- parameter fileParts: the file parameters that are added to the multipart request
- parameter requestEnqueued: callback to be called when the fileparts are serialized and request is added to the background session. Defaults to nil
- parameter success: callback to be called on successful request
Expand Down Expand Up @@ -327,6 +327,15 @@ open class WordPressComRestApi: NSObject {
for filePart in fileParts {
multipartFormData.append(filePart.url, withName: filePart.parameterName, fileName: filePart.filename, mimeType: filePart.mimeType)
}

if let parameters = parameters {
for (key, value) in parameters {
if value is String || value is Int {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking for one of the use cases for https://developer.wordpress.com/docs/api/1.1/post/sites/%24site/media/new/,
I think treating parameters as Query Parameters and adding another function parameter specifically for Request Parameters could be beneficial in case we also want to send Query Parameters next to Request Parameters in the future.
One other option could be to change the type of parameters to [String: String]? instead of checking for types here.
Yet another option could be adding another parameter to the method that takes a function like multipartFormData -> Void and the caller can deal with how to append those?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ceyhun for the feedback!

I think treating parameters as Query Parameters and adding another function parameter specifically for Request Parameters could be beneficial in case we also want to send Query Parameters next to Request Parameters in the future.

I though about this option but I wasn't sure if having both could be confusing. In regular POST requests we only have parameters and they're considered as the Request Parameters, in that case Query Parameters should be previously added to the URL.

One other option could be to change the type of parameters to [String: String]? instead of checking for types here.

Yeah that would prevent having to check the types as I did, I included Int type because I saw a couple of cases where there were arguments of that type. In any case since we're converting them into String when parsing the value to Data we could use [String: String]? and let the callers explicitly do the conversion.

Yet another option could be adding another parameter to the method that takes a function like multipartFormData -> Void and the caller can deal with how to append those?

I think this one is probably the best since it will let the caller to include more complex values than primitives. The downside is that callers would have to do the parsing to Data but I think that's expected since it's a multipart POST request.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yet another option could be adding another parameter to the method that takes a function like multipartFormData -> Void and the caller can deal with how to append those?

I tried to apply option 3 but unfortunately the class MultipartFormData that Alamofire provides is not compatible with ObjC and the places where this type of request is being used are ObjC, however this gave me an idea.

The idea was to unify the Request Parameters into one argument, including the file parts which in the end are also body parts. Following this I created a new argument named bodyParts that is an array of BodyPart, a new class I added (based on the previous one FilePart) but more generic that represents a body part of the multipartPOST requests.

I'd appreciate feedback of this change since I'm unaware of the implications of such a refactor, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great idea! But there appears to be a BodyPart class in Alamofire as well. Should we maybe rename this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'm not familiar of class name conflicts on iOS but yeah if it could produce it let's rename it. I was thinking to use FormPart or RequestPart 🤔 .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think FormPart could be better since it's used in MultipartFormData.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah makes sense, I'll rename it then, thanks!

multipartFormData.append("\(value)".data(using: .utf8)!, withName: key)
}
}
}

}, to: URLString, encodingCompletion: { (encodingResult) in
switch encodingResult {
case .success(let upload, _, _):
Expand Down