Added a method to perform POST Form Upload #69

embeddedmz · 2016-12-24T20:45:39Z

Some REST platforms (e.g. Symphony) require a POST Form upload in some requests to send data to them. So I added code to perform post form upload.

I created a helper object to wrap HTML form information. Then we pass it to postForm method to perform the upload.

The unit test I wrote uses Henry's HTTP Post dumping server http://posttestserver.com/
The details of the upload of the dummy test file can be found under http://posttestserver.com/data (/[year]/[month]/[day]/restclientcpptests/)

There's no memory leaks.

…coverage and fixed code style.

… headers.

mrtazz · 2016-12-27T15:56:48Z

Thanks for the contribution! It seems like this PR contains some of the changes that were in your other PR already. Can you rebase this onto master and make it only contain the actual related changes please.

embeddedmz · 2016-12-27T16:43:40Z

I have no idea how to do that, but I think you can do it more easily on you side: https://github.com/blog/2243-rebase-and-merge-pull-requests Just choose "Rebase and merge" instead of "Merge pull request". Thank you.

mrtazz · 2016-12-27T21:18:57Z

I would love to have a clean PR for reviewing before I merge anything.

Added TestInvalidProxy test. Reduced a line length to comply with CI rule. Updated README, updated test proxy address, added a test to increase coverage and fixed code style. Added a method to set SSL Key password and added some missing Doxygen headers. Updated an info in README.md

Fixed code indentation to comply with Cpplint. Added web proxy tunneling support. this adds support for using proxies corresponding to the CURLOPT_PROXY and CURLOPT_HTTPPROXYTUNNEL config options in libcurl. closes mrtazz#68 default to 10s timeout in unit tests this makes the conn object in unit tests default to a 10s timeout in order to fail fast if something is up with the connection or configured hosts.

embeddedmz · 2016-12-28T00:35:13Z

I squashed the commits of Proxy support and POST Form, but I don't know how to clean this PR. git is still obscure for me.

mrtazz · 2016-12-28T04:00:08Z

@embeddedmz no worries, it looks great now. Thanks for the cleanup. I'll take a look at it within the next couple of days.

mrtazz · 2017-04-27T00:51:47Z

Ok this was definitely more than a couple of days. Sorry about that @embeddedmz!

I looked over the code and I think it looks great. Given the API and how it's intended to be used, I would prefer PostFormInfo to be a class and not a struct. It exposes an API that makes it clear that it's supposed to be used via the methods only and there is no benefit in having all those variables public by making it a struct.

I also think accessing it via the Helpers namespace makes the API a little bit weird. postForm() should be a first class citizen of the exposed Restclient API and as such PostFormInfo should be a proper class under the Restclient namespace.

embeddedmz · 2017-05-06T11:31:15Z

I ll try to refactor the code according to your advices before the end of this month.

dfons · 2017-06-09T19:21:55Z

@embeddedmz @mrtazz is some help needed for this issue? I'm actually looking forward to the 0.5.0 release, mainly because of the SSL security feature. If there is anything I can help with, please tell me.

Regards.

embeddedmz · 2017-06-09T19:37:05Z

I will work that this weekend, I have time for it.

Regards.

dfons · 2017-06-09T19:42:18Z

Ok, I'll be here anyway... Regards.

…

On Jun 9, 2017 16:37, "embeddedmz" ***@***.***> wrote: I will work that this weekend, I have time for it. Regards. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#69 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AD0gmKAhSqkXeskMGBAwJSsYZa7f3YBIks5sCZ7jgaJpZM4LVSuU> .

embeddedmz · 2017-06-09T22:49:48Z

@mrtazz I think I made the necessary changes, but the Travis-CI's build is failing for a strange reason... Your feedback is needed to speed things up :) Thanks.

mrtazz · 2017-06-12T13:10:29Z

awesome, I'll take a look. The nightly CI build has started to fail a couple of days ago on master as well, so it might be that the travis config needs a change.

dfons · 2017-06-12T13:18:02Z

It seems the fail is related to some coding style definition. From the CI job output:

include/restclient-cpp/restclient.h:49: public: should be indented +1 space inside class PostFormInfo [whitespace/indent] [3]
include/restclient-cpp/restclient.h:61: private: should be indented +1 space inside class PostFormInfo [whitespace/indent] [3]

embeddedmz · 2017-06-12T13:38:42Z

@dfons I ll check that tonight. Thank you.

embeddedmz · 2017-06-12T19:14:22Z

@mrtazz It's all good ! please don't tell me to squash commits because last time it was hard for me to do that and I forgot how to do it (unless you give me the commands).

mrtazz · 2017-06-13T01:53:25Z

@embeddedmz no worries. Last time was just about having a clean PR to review. Sorry you had a bad time. I'll review and take care of any squashing before it goes into master.

embeddedmz · 2017-06-13T12:26:19Z

@mrtazz Excellent, the description of the unit test is on the top of that page, I used http://posttestserver.com/ to upload a dummy text file, and it still works very great, you can also see the uploaded file... In the URL http://posttestserver.com/data (/[year]/[month]/[day]/restclientcpptests/) the day is related to the time zone of the server, so if you launch the unit test at 14/06/17 - 00:05 (GMT +1) for example, you will find the directory under http://posttestserver.com/data/2017/06/13/restclientcpptests because of the difference in the time zone !

dfons · 2017-06-14T18:25:12Z

@mrtazz What's the ETA we can expect for the release? Do you need some help? If so, do not hesitate in asking me...

mrtazz · 2017-06-15T14:35:41Z

@dfons thanks, I just need to find a bit of time to sit down and read through it. But will let you know!

mrtazz · 2019-05-27T14:19:31Z

Seems like http://posttestserver.com got deprecated in favor of http://ptsv2.com/. So we'll need to update this before we can merge it

embeddedmz · 2019-05-27T17:24:58Z

Yes, we will need to create a "toilet" in https://ptsv2.com (e.g. kv6od-1543167696) and send the data to it (e.g. "http://ptsv2.com/t/kv6od-1543167696/post").

What do you think about rule 2.4 in https://ptsv2.com/s/somerules.html ? Your project is popular, I think we will break this rule.

mrtazz · 2019-06-03T09:05:17Z

oh that's a good point. I hadn't seen that. Maybe this will need another integration test setup much like the proxy docker container.

hamzehnasajpour · 2020-08-04T07:21:23Z

@mrtazz What's the status of this PR? Honestly I'm using this library and just now I need the multipart post for uploading the files on the server. Can you clarify the status?

chpatton013 · 2020-08-04T22:56:30Z

include/restclient-cpp/restclient.h

@@ -53,6 +74,8 @@ Response get(const std::string& url);
 Response post(const std::string& url,
              const std::string& content_type,
              const std::string& data);
+Response postForm(const std::string& url,


Why not just use an overload of post() instead of this newly named function?

chpatton013 · 2020-08-04T22:57:31Z

include/restclient-cpp/restclient.h

+  * @brief This class represents the form information to send on
+  * POST Form requests
+  */
+class PostFormInfo {


This name doesn't accurately reflect the notion that a form can be submitted with any HTTP method. Why not just name this FormData instead?

Sounds good.

chpatton013 · 2020-08-04T22:59:35Z

source/restclient.cc

+RestClient::Response RestClient::postForm(const std::string& url,
+                    const PostFormInfo& data) {
+  RestClient::Response ret;
+  RestClient::Connection *conn = new RestClient::Connection("");


There shouldn't be any need to use new; you only need the connection during the lifetime of the function. Just use a stack allocation instead.

Yes, that's true. That will allow the remove of a lot of extraneous code and improve performance of RestClient interface. It should be done for all RestClient methods.

include/restclient-cpp/restclient.h

chpatton013 · 2020-08-04T23:02:12Z

test/test_restclient.cc

+  time_t rawtime;
+  tm * timeinfo;
+  time(&rawtime);
+  timeinfo = localtime( &rawtime );


The system time is not a reliable way to ensure you have a unique file name. You should just use mkstemp() to get a unique filepath instead.

Agreed, but it's reliable enough. If it really bugs you, it can be changed. Mostly this needs to run on dev systems on and a clean container that's spun up in CI so no worries there.

chpatton013 · 2020-08-04T23:06:48Z

Apologies for the fly-by code review. I am interested in using this feature when it eventually merges and was hoping to clean up a few of the rough edges I noticed.

Do you need help getting this over the finish line? I'm happy to help if this is simply a matter of finishing known tasks.

hamzehnasajpour · 2020-08-05T08:48:24Z

@chpatton013 Thanks for your quick reply. As I noticed there are some tasks for renaming the functions and classes only. yes? So it's not a big task.

chpatton013 · 2020-08-05T16:45:19Z

@hamzehnasajpour I'm by no means an authority figure here. I only meant to offer suggestions, not requirements.

edwinpjacques · 2020-10-30T17:39:27Z

@embeddedmz, I'm a new contributor and I want to see your change get merged. It seemed that @chpatton013 had some good suggestions. Can you please reply to them? I'd like to merge this change in and cut a release soon. You should rebase to get CI to be fixed. Thanks!

embeddedmz · 2020-10-31T17:10:54Z

@edwinpjacques I can't really find free time to take care of this, so if someone else wants to take care of it, it doesn't bother me.

Regards.

edwinpjacques · 2020-10-31T23:24:33Z

@embeddedmz OK I will look to resolve the outstanding issue. I will make another PR and close this one. Your feedback on it would be appreciated. Thanks!

edwinpjacques · 2020-11-01T00:02:36Z

Replaced with #163

embeddedmz added 8 commits December 16, 2016 01:02

Added web proxy tunneling support.

cfb825d

Added TestInvalidProxy test.

8b117a5

Reduced a line length to comply with CI rule.

553c238

Updated README, updated test proxy address, added a test to increase …

66f59c5

…coverage and fixed code style.

Added a method to set SSL Key password and added some missing Doxygen…

75d61ce

… headers.

Updated an info in README.md

9c2913b

Added a method to perform a post form upload.

5a6b127

Fixed code indentation to comply with Cpplint.

85b2e74

embeddedmz added 4 commits December 28, 2016 00:24

Merge remote-tracking branch 'upstream/master'

62e25d2

Merge branch 'master' of https://github.com/embeddedmz/restclient-cpp

52a6275

mrtazz added this to the v0.5.0 milestone Apr 27, 2017

Moved PostFormInfo from Helpers to RestClient namespace.

037efba

Fixed PostFormInfo indentation.

47171ad

Merge branch 'master' into master

de18575

mrtazz mentioned this pull request Feb 12, 2020

POST Multipart-Form #88

Open

chpatton013 reviewed Aug 4, 2020

View reviewed changes

edwinpjacques mentioned this pull request Nov 1, 2020

Added a method to perform POST Form Upload #163

Open

5 tasks

edwinpjacques closed this Nov 1, 2020

Added a method to perform POST Form Upload #69

Added a method to perform POST Form Upload #69

Conversation

embeddedmz commented Dec 24, 2016 • edited Loading

mrtazz commented Dec 27, 2016

embeddedmz commented Dec 27, 2016

mrtazz commented Dec 27, 2016

embeddedmz commented Dec 28, 2016

mrtazz commented Dec 28, 2016

mrtazz commented Apr 27, 2017

embeddedmz commented May 6, 2017

dfons commented Jun 9, 2017

embeddedmz commented Jun 9, 2017

dfons commented Jun 9, 2017 via email

embeddedmz commented Jun 9, 2017

mrtazz commented Jun 12, 2017

dfons commented Jun 12, 2017

embeddedmz commented Jun 12, 2017

embeddedmz commented Jun 12, 2017

mrtazz commented Jun 13, 2017

embeddedmz commented Jun 13, 2017

dfons commented Jun 14, 2017

mrtazz commented Jun 15, 2017

mrtazz commented May 27, 2019

embeddedmz commented May 27, 2019

mrtazz commented Jun 3, 2019

hamzehnasajpour commented Aug 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chpatton013 commented Aug 4, 2020

hamzehnasajpour commented Aug 5, 2020

chpatton013 commented Aug 5, 2020

edwinpjacques commented Oct 30, 2020 • edited Loading

embeddedmz commented Oct 31, 2020

edwinpjacques commented Oct 31, 2020

edwinpjacques commented Nov 1, 2020

embeddedmz commented Dec 24, 2016 •

edited

Loading

edwinpjacques commented Oct 30, 2020 •

edited

Loading