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

Include parameters in multipart POST request #338

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

Conversation

fluiddot
Copy link
Contributor

@fluiddot fluiddot commented Jan 28, 2021

Description

Fixes #333

Unfortunately Alamofire doesn't provide a way to serialise the parameters object to be included in the multipartFormData object, as a workaround I'm only adding the String and Int values that are converted to Data.

Testing Details

The easiest way to test this is by adding items from the free photo library as they include as the caption value its photo credit in the upload parameters.

  1. Go to the media screen.
  2. Tap on + button.
  3. Tap on Free Photo Library.
  4. Search any term.
  5. Tap one or multiple items.
  6. Tap on Add button.
  7. Wait for the items to be uploaded.
  8. Tap on one of the uploaded items.
  9. Check that the caption value is defined.
  • Please check here if your pull request includes additional test coverage.

@fluiddot fluiddot added the bug Something isn't working label Jan 28, 2021
@fluiddot fluiddot self-assigned this Jan 28, 2021
@fluiddot fluiddot requested a review from ceyhun January 28, 2021 11:28

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!

FilePart has been merged into BodyPart, a more generic class to handle part objects for this type of requests. Additionally all functions that are using multipartPOST requests haven been updated.
@fluiddot fluiddot requested a review from ceyhun January 29, 2021 15:55
@fluiddot fluiddot marked this pull request as ready for review January 29, 2021 15:56
Copy link
Contributor

@ceyhun ceyhun left a comment

Choose a reason for hiding this comment

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

Overall the changes look good. Thanks a lot for coming up with this and implementing. I would like to test all the affected paths inside WPiOS to be confident. I saw you mentioned under Testing Details to upload a photo from the Free Photo Library which should cover the MediaServiceRemoteREST parts and I guess testing to upload a local image would be necessary as well. What would be the best way to test PostServiceRemoteREST parts?

@fluiddot
Copy link
Contributor Author

fluiddot commented Feb 9, 2021

Overall the changes look good. Thanks a lot for coming up with this and implementing. I would like to test all the affected paths inside WPiOS to be confident. I saw you mentioned under Testing Details to upload a photo from the Free Photo Library which should cover the MediaServiceRemoteREST parts and I guess testing to upload a local image would be necessary as well. What would be the best way to test PostServiceRemoteREST parts?

Yeah sorry, due the latest changes I should update the Testing Details because we should cover more cases. For MediaServiceRemoteREST we should try to upload different types of media, for PostServiceRemoteREST it's a bit tricky because the function that invokes the upload media is not being used anywhere 😄 . So in this case unless we explicitly connect it somewhere in the app we can't test it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working [Status] In Progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parameters are not included in multipart POST requests
2 participants