Fix/Rationalize transports #143

egeloen · 2015-03-07T22:15:33Z

Hey!

This PR rationalizes/fixes some issues I encounter with the curl/fsockopen transports + add some new features.

Context

I'm currently integrating this library in an http adapter (See egeloen/ivory-http-adapter#80). This library has a pretty robust test suite which have spotted some issues. This PR tries to fix all of them :)

Issues

The curl transport initializes a curl resource at initialization. Then, if this resource is closed, the transport is not usable anymore. Instead of creating it at initialization, I have moved it in the setup_handle method which fixes the issue.
The curl transport merges the data in the uri for DELETE request but some service like AWS needs some extra info in DELETE request which needs to be passed by message body. According to DELETE does not allow POST-style data #91, I have added the data_as_query option and updated the transport accordingly.
The fsockopen transport added arbitrary headers without looking for existence in the passed headers. This result of a duplication of headers.

New features

Added support for the OPTIONS and TRACE http verbs.
Added support for 1.0 and 1.1 protocol versions in request/response.

Btw, this PR fixes #133, #137, #142 and #91.

Waiting your feedbacks!

egeloen · 2015-04-17T19:56:11Z

@rmccue ping?

ozh · 2015-04-17T20:56:06Z

I think @rmccue will want unit tests before considering a merge, as always :)

egeloen · 2015-04-17T21:18:34Z

Thank you @ozh :) I will add them in the next days!

egeloen · 2015-04-18T12:18:27Z

I'm currently adding tests but I can't get them passing for the master branch on my computer. Here, the PHPunit output:

PHPUnit 4.6.4 by Sebastian Bergmann and contributors.

Configuration read from /home/gelo/Workspace/Rmccue/Requests/tests/phpunit.xml.dist

....FF....F....F...F..F........................................  63 / 814 (  7%)
............................................................... 126 / 814 ( 15%)
..................................................F...F....F... 189 / 814 ( 23%)
.F...F..F...................................................... 252 / 814 ( 30%)
............................................................... 315 / 814 ( 38%)
....................................F...F...................... 378 / 814 ( 46%)
............................................................... 441 / 814 ( 54%)
............................................................... 504 / 814 ( 61%)
............................................................... 567 / 814 ( 69%)
............................................................... 630 / 814 ( 77%)
............................................................... 693 / 814 ( 85%)
............................................................... 756 / 814 ( 92%)
..........................................................

Time: 6.9 minutes, Memory: 41.25Mb

There were 14 failures:

1) RequestsTest_Auth_Basic::testPOSTUsingInstantiation with data set #0 ('Requests_Transport_fsockopen')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'test'
+''

/home/gelo/Workspace/Rmccue/Requests/tests/Auth/Basic.php:77

2) RequestsTest_Auth_Basic::testPOSTUsingInstantiation with data set #1 ('Requests_Transport_cURL')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'test'
+''

/home/gelo/Workspace/Rmccue/Requests/tests/Auth/Basic.php:77

3) RequestsTest_Transport_cURL::testGETWithNestedData
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'http://httpbin.org/get?test=true&test2%5Btest3%5D=test&test2%5Btest4%5D=test-too'
+'http://httpbin.org/get?test=true&test2[test3]=test&test2[test4]=test-too'

/home/gelo/Workspace/Rmccue/Requests/tests/Transport/Base.php:71

4) RequestsTest_Transport_cURL::testRawPOST
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'test'
+''

/home/gelo/Workspace/Rmccue/Requests/tests/Transport/Base.php:119

5) RequestsTest_Transport_cURL::testRawPUT
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'test'
+''

/home/gelo/Workspace/Rmccue/Requests/tests/Transport/Base.php:164

6) RequestsTest_Transport_cURL::testRawPATCH
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'test'
+''

/home/gelo/Workspace/Rmccue/Requests/tests/Transport/Base.php:194

7) RequestsTest_Transport_cURL::testMultipleWithDifferingMethods
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'test'
+''

/home/gelo/Workspace/Rmccue/Requests/tests/Transport/Base.php:571

8) RequestsTest_Transport_cURL::testMultipleToFile
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'test'
+''

/home/gelo/Workspace/Rmccue/Requests/tests/Transport/Base.php:671

9) RequestsTest_Transport_fsockopen::testGETWithNestedData
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'http://httpbin.org/get?test=true&test2%5Btest3%5D=test&test2%5Btest4%5D=test-too'
+'http://httpbin.org/get?test=true&test2[test3]=test&test2[test4]=test-too'

/home/gelo/Workspace/Rmccue/Requests/tests/Transport/Base.php:71

10) RequestsTest_Transport_fsockopen::testRawPOST
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'test'
+''

/home/gelo/Workspace/Rmccue/Requests/tests/Transport/Base.php:119

11) RequestsTest_Transport_fsockopen::testRawPUT
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'test'
+''

/home/gelo/Workspace/Rmccue/Requests/tests/Transport/Base.php:164

12) RequestsTest_Transport_fsockopen::testRawPATCH
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'test'
+''

/home/gelo/Workspace/Rmccue/Requests/tests/Transport/Base.php:194

13) RequestsTest_Transport_fsockopen::testMultipleWithDifferingMethods
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'test'
+''

/home/gelo/Workspace/Rmccue/Requests/tests/Transport/Base.php:571

14) RequestsTest_Transport_fsockopen::testMultipleToFile
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'test'
+''

/home/gelo/Workspace/Rmccue/Requests/tests/Transport/Base.php:671

FAILURES!
Tests: 814, Assertions: 1219, Failures: 14.

Majority of the failling tests are related to the data test which is sent as application/x-www-form-urlencoded which is then part of the form json node instead oe being part of the data node. Do you have any idea why they are green on travis but not on my computer? /cc @ozh

ozh · 2015-04-18T12:45:57Z

Yep, noticed the same on my computer, no idea why and honestly I didn't bother. When I submit a PR with new tests, I just make sure my new tests are OK 😄

egeloen · 2015-04-18T13:18:55Z

Thanks for your feedback, I will then rely on travis to be sure my tests are OK :)

ozh · 2015-04-18T13:34:52Z

Actually, I have started looking at it. The problem is that, IMHO, the tests here are doing it wrong: we're testing httpbin and the values it returns, not Requests. I suspect that this line explains the discrepancies

ozh · 2015-04-18T13:40:02Z

Additionally, I'm not even sure what httpbin is supposed to return for instance with test testRawPOST() : I get different results with https://www.hurl.it/ and http://requestmaker.com/

egeloen · 2015-04-18T13:40:22Z

Seems related to postmanlabs/httpbin#178

egeloen · 2015-04-18T19:12:52Z

Tests added! Undortunatelly, httpbin does not allow me to test as much feature as I want. For example, http spec specifies that an OPTIONS request can have a body but httpbin does not deal with such case. Even worse, it does not deal with TRACE request at all.

Feedbacks welcome!

ozh · 2015-04-20T22:29:32Z

@egeloen FYI, regarding tests: #149

rmccue · 2015-04-27T02:23:26Z

Thanks for the PR, and apologies for the delay in responding!

The curl transport initializes a curl resource at initialization. Then, if this resource is closed, the transport is not usable anymore. Instead of creating it at initialization, I have moved it in the setup_handle method which fixes the issue.

I think this may break backwards compatibility with people using hooks that occur between __construct and setup_handle, but I need to look into that more.

What would cause the handle to be closed? I'm assuming the main issue is reusing the transport?

The curl transport merges the data in the uri for DELETE request but some service like AWS needs some extra info in DELETE request which needs to be passed by message body. According to #91, I have added the data_as_query option and updated the transport accordingly.

I like the concept, but let's make the option a bit more generic. I'm thinking data_format with the options of default (query args for GET/HEAD/DELETE/OPTIONS, body for PUT/POST), query, and body. Later, we may want to add json, etc as well.

(The original reason we didn't support this was because RFC2616 didn't specify that DELETE et al could take a request body, and the general consensus was to treat them similar to GET requests. RFC7231 clarifies that it's allowed, so happy to add this.)

The fsockopen transport added arbitrary headers without looking for existence in the passed headers. This result of a duplication of headers.

Good catch.

Further comments inline as well.

rmccue · 2015-04-27T02:24:09Z

library/Requests/Transport/cURL.php

+			if ($options['data_as_query']) {
+				$url = self::format_get($url, $data);
+				$data = '';
+			} elseif (!is_string($data)) {


The elseif should be on the next line after the closing brace

egeloen · 2015-05-08T20:51:10Z

I think this may break backwards compatibility with people using hooks that occur between __construct and setup_handle, but I need to look into that more.

What would cause the handle to be closed? I'm assuming the main issue is reusing the transport?

I have changed my approach to make it reusable without BC break :) Basically, the issue was the fp is initialized in the constructor and then closed with the first request. Instead of closing it for each request, I have closed it when the transport is destructed.

I like the concept, but let's make the option a bit more generic. I'm thinking data_format with the options of default (query args for GET/HEAD/DELETE/OPTIONS, body for PUT/POST), query, and body. Later, we may want to add json, etc as well.

Done!

We'll eventually change this to be a proper CaseInsensitiveDictionary

Done!

Btw, also adding protocol version extraction for the response.

ping @rmccue

egeloen · 2015-05-08T21:15:39Z

If all is fine, I will squash my commits before merging.

egeloen · 2015-09-22T17:21:24Z

ping @rmccue

rmccue · 2015-10-13T02:02:22Z

@egeloen Apologies for the delay. If anything, I'd actually prefer the commits split out and in separate pull requests, as it makes it easier to manage and merge parts as they're ready.

I'm going to do a full review of this, but it'll also need updating from the latest master in any case.

rmccue · 2015-10-26T02:58:31Z

Thanks again for this @egeloen :) I've updated this with some other small fixes in #188, so closing this one out.

egeloen mentioned this pull request Mar 7, 2015

Add Requests http adapter egeloen/ivory-http-adapter#80

Merged

rmccue reviewed Apr 27, 2015
View reviewed changes

rmccue added this to the 1.7 milestone Apr 27, 2015

rmccue added the Component: Transports label Apr 27, 2015

GeLoLabs added 6 commits May 7, 2015 19:02

Fix/Rationalize transports

41803e3

Add tests

8776a6b

Fix CS + missing headers check + rely on case insensitive dictionary

fcbfd58

Replace data_as_query by data_format

69a4be0

Add support for response protocol version

e49ec32

Change approach for marking curl transport reusable

24b11f1

Fix protocol version formatting

f21760a

This was referenced Oct 26, 2015

Fix/Rationalize transports (v2) #188

Merged

GET request should respect query data parameter #133

Closed

rmccue closed this Oct 26, 2015

egeloen deleted the transports branch December 12, 2015 21:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/Rationalize transports #143

Fix/Rationalize transports #143

egeloen commented Mar 7, 2015

egeloen commented Apr 17, 2015

ozh commented Apr 17, 2015

egeloen commented Apr 17, 2015

egeloen commented Apr 18, 2015

ozh commented Apr 18, 2015

egeloen commented Apr 18, 2015

ozh commented Apr 18, 2015

ozh commented Apr 18, 2015

egeloen commented Apr 18, 2015

egeloen commented Apr 18, 2015

ozh commented Apr 20, 2015

rmccue commented Apr 27, 2015

rmccue Apr 27, 2015

egeloen commented May 8, 2015

egeloen commented May 8, 2015

egeloen commented Sep 22, 2015

rmccue commented Oct 13, 2015

rmccue commented Oct 26, 2015

Fix/Rationalize transports #143

Fix/Rationalize transports #143

Conversation

egeloen commented Mar 7, 2015

Context

Issues

New features

egeloen commented Apr 17, 2015

ozh commented Apr 17, 2015

egeloen commented Apr 17, 2015

egeloen commented Apr 18, 2015

ozh commented Apr 18, 2015

egeloen commented Apr 18, 2015

ozh commented Apr 18, 2015

ozh commented Apr 18, 2015

egeloen commented Apr 18, 2015

egeloen commented Apr 18, 2015

ozh commented Apr 20, 2015

rmccue commented Apr 27, 2015

rmccue Apr 27, 2015

Choose a reason for hiding this comment

egeloen commented May 8, 2015

egeloen commented May 8, 2015

egeloen commented Sep 22, 2015

rmccue commented Oct 13, 2015

rmccue commented Oct 26, 2015