From 17e45e20021084f84d07d0c7590ebc2645b5d889 Mon Sep 17 00:00:00 2001 From: Tom Needham Date: Wed, 13 Nov 2013 00:45:49 +0000 Subject: [PATCH 01/10] Pick any none 100 status code before defaulting to 100 --- lib/private/api.php | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/lib/private/api.php b/lib/private/api.php index eac4a825e07..8307f209d21 100644 --- a/lib/private/api.php +++ b/lib/private/api.php @@ -170,7 +170,7 @@ class OC_API { $response = reset($thirdparty['failed']); return $response; } else { - $responses = array_merge($shipped['succeeded'], $thirdparty['succeeded']); + $responses = $thirdparty['succeeded']; } // Merge the successful responses $meta = array(); @@ -182,8 +182,19 @@ class OC_API { } else { $data = array_merge_recursive($data, $response->getData()); } + $codes[] = $response->getStatusCode(); } - $result = new OC_OCS_Result($data, 100); + + // Use any non 100 status codes + $statusCode = 100; + foreach($codes as $code) { + if($code != 100) { + $statusCode = $code; + break; + } + } + + $result = new OC_OCS_Result($data, $statusCode); return $result; } From f19caeed3312e9d9f935008884ef9afe300cdda4 Mon Sep 17 00:00:00 2001 From: tomneedham Date: Wed, 13 Nov 2013 22:46:24 +0000 Subject: [PATCH 02/10] Remove OC_App dependancy from OC_API::mergeResponses() --- lib/private/api.php | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/lib/private/api.php b/lib/private/api.php index eac4a825e07..ac9bb8b0688 100644 --- a/lib/private/api.php +++ b/lib/private/api.php @@ -96,6 +96,7 @@ class OC_API { $responses[] = array( 'app' => $action['app'], 'response' => new OC_OCS_Result(null, OC_API::RESPOND_UNAUTHORISED, 'Unauthorised'), + 'shipped' => OC_App::isShipped($action['app']), ); continue; } @@ -103,6 +104,7 @@ class OC_API { $responses[] = array( 'app' => $action['app'], 'response' => new OC_OCS_Result(null, OC_API::RESPOND_NOT_FOUND, 'Api method not found'), + 'shipped' => OC_App::isShipped($action['app']), ); continue; } @@ -110,6 +112,7 @@ class OC_API { $responses[] = array( 'app' => $action['app'], 'response' => call_user_func($action['action'], $parameters), + 'shipped' => OC_App::isShipped($action['app']), ); } $response = self::mergeResponses($responses); @@ -127,7 +130,7 @@ class OC_API { * merge the returned result objects into one response * @param array $responses */ - private static function mergeResponses($responses) { + public static function mergeResponses($responses) { $response = array(); // Sort into shipped and thirdparty $shipped = array( @@ -140,17 +143,17 @@ class OC_API { ); foreach($responses as $response) { - if(OC_App::isShipped($response['app']) || ($response['app'] === 'core')) { + if($response['shipped'] || ($response['app'] === 'core')) { if($response['response']->succeeded()) { - $shipped['succeeded'][$response['app']] = $response['response']; + $shipped['succeeded'][$response['app']] = $response; } else { - $shipped['failed'][$response['app']] = $response['response']; + $shipped['failed'][$response['app']] = $response; } } else { if($response['response']->succeeded()) { - $thirdparty['succeeded'][$response['app']] = $response['response']; + $thirdparty['succeeded'][$response['app']] = $response; } else { - $thirdparty['failed'][$response['app']] = $response['response']; + $thirdparty['failed'][$response['app']] = $response; } } } @@ -177,10 +180,10 @@ class OC_API { $data = array(); foreach($responses as $app => $response) { - if(OC_App::isShipped($app)) { - $data = array_merge_recursive($response->getData(), $data); + if($response['shipped']) { + $data = array_merge_recursive($response['response']->getData(), $data); } else { - $data = array_merge_recursive($data, $response->getData()); + $data = array_merge_recursive($data, $response['response']->getData()); } } $result = new OC_OCS_Result($data, 100); From 959513fdc866910855b886051394962ab0ee582e Mon Sep 17 00:00:00 2001 From: tomneedham Date: Thu, 14 Nov 2013 00:40:09 +0000 Subject: [PATCH 03/10] Add tests for OC_API::mergeResponses() --- tests/lib/api.php | 159 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 159 insertions(+) create mode 100644 tests/lib/api.php diff --git a/tests/lib/api.php b/tests/lib/api.php new file mode 100644 index 00000000000..6cff1823d50 --- /dev/null +++ b/tests/lib/api.php @@ -0,0 +1,159 @@ + + * This file is licensed under the Affero General Public License version 3 or + * later. + * See the COPYING-README file. + */ + +class Test_API extends PHPUnit_Framework_TestCase { + + // Helps build a response variable + public function buildResponse($shipped=true, $data=null, $code=100) { + return array( + 'shipped' => $shipped, + 'response' => new OC_OCS_Result($data, $code), + 'app' => uniqid('testapp_', true), + ); + } + + // Validate details of the result + public function checkResult($result, $success=true) { + // Check response is of correct type + $this->assertEquals('OC_OCS_Result', get_class($result)); + // CHeck if it succeeded + $this->assertEquals($success, $result->succeeded()); + } + + // Test the merging of multiple responses + public function testMergeResponses(){ + // Tests that app responses are merged correctly + // Setup some data arrays + $data1 = array( + 'users' => array( + 'tom' => array( + 'key' => 'value', + ), + 'frank' => array( + 'key' => 'value', + ), + )); + + $data2 = array( + 'users' => array( + 'tom' => array( + 'key' => 'newvalue', + ), + 'jan' => array( + 'key' => 'value', + ), + )); + // Test merging one success result + $response = $this->buildResponse(true, $data1); + $result = OC_API::mergeResponses(array($response)); + $this->assertEquals($response['response'], $result); + $this->checkResult($result); + + $response = $this->buildResponse(true, $data1, 101); + $result = OC_API::mergeResponses(array($response)); + $this->assertEquals($response['response'], $result); + $this->checkResult($result); + + $response = $this->buildResponse(true, $data1, 997); + $result = OC_API::mergeResponses(array($response)); + $this->assertEquals($response['response'], $result); + $this->checkResult($result, false); + + // Two shipped success results + $result = OC_API::mergeResponses(array( + $this->buildResponse(true, $data1O), + $this->buildResponse(true, $data2), + )); + $this->checkResult($result); + $resultData = $result->getData(); + $this->assertArrayHasKey('jan', $resultData['users']); + + // Two shipped results, one success and one failure + $result = OC_API::mergeResponses(array( + $this->buildResponse(true, $data1), + $this->buildResponse(true, $data2, 997), + )); + $this->checkResult($result, false); + $resultData = $result->getData(); + $this->assertArrayHasKey('jan', $resultData['users']); + + // Two shipped results, both failure + $result = OC_API::mergeResponses(array( + $this->buildResponse(true, $data1, 997), + $this->buildResponse(true, $data2, 997), + )); + $this->checkResult($result, false); + $resultData = $result->getData(); + $this->assertArrayHasKey('jan', $resultData['users']); + + // Two third party success results + $result = OC_API::mergeResponses(array( + $this->buildResponse(false, $data1), + $this->buildResponse(false, $data2), + )); + $this->checkResult($result); + $resultData = $result->getData(); + $this->assertArrayHasKey('jan', $resultData['users']); + + // Two third party results, one success and one failure + $result = OC_API::mergeResponses(array( + $this->buildResponse(false, $data1), + $this->buildResponse(false, $data2, 997), + )); + $this->checkResult($result, false); + $resultData = $result->getData(); + $this->assertArrayHasKey('jan', $resultData['users']); + + // Two third party results, both failure + $result = OC_API::mergeResponses(array( + $this->buildResponse(false, $data1, 997), + $this->buildResponse(false, $data2, 997), + )); + $this->checkResult($result, false); + $resultData = $result->getData(); + $this->assertArrayHasKey('jan', $resultData['users']); + + // One of each, both success + $result = OC_API::mergeResponses(array( + $this->buildResponse(false, $data1), + $this->buildResponse(true, $data2), + )); + $this->checkResult($result); + $resultData = $result->getData(); + $this->assertArrayHasKey('jan', $resultData['users']); + + // One of each, both failure + $result = OC_API::mergeResponses(array( + $this->buildResponse(false, $data1, 997), + $this->buildResponse(true, $data2, 997), + )); + $this->checkResult($result, false); + $resultData = $result->getData(); + $this->assertArrayHasKey('jan', $resultData['users']); + + // One of each, shipped success + $result = OC_API::mergeResponses(array( + $this->buildResponse(false, $data1, 997), + $this->buildResponse(true, $data2), + )); + $this->checkResult($result); + $resultData = $result->getData(); + $this->assertArrayHasKey('jan', $resultData['users']); + + // One of each, third party success + $result = OC_API::mergeResponses(array( + $this->buildResponse(false, $data1), + $this->buildResponse(true, $data2, 997), + )); + $this->checkResult($result, false); + $resultData = $result->getData(); + $this->assertArrayHasKey('jan', $resultData['users']); + + } + +} From 790055571db1a7ebb0cd93d0ec8d1a5e75cbc702 Mon Sep 17 00:00:00 2001 From: tomneedham Date: Thu, 14 Nov 2013 01:00:14 +0000 Subject: [PATCH 04/10] Update lib/private/api.php after merge conflict --- lib/private/api.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/api.php b/lib/private/api.php index 45e7f18bd4e..913b3ff0a18 100644 --- a/lib/private/api.php +++ b/lib/private/api.php @@ -185,7 +185,7 @@ class OC_API { } else { $data = array_merge_recursive($data, $response['response']->getData()); } - $codes[] = $response->getStatusCode(); + $codes[] = $response['response']->getStatusCode(); } // Use any non 100 status codes From a39f3fdbf9fb20d9fa4c9cbba4765f9989f850ec Mon Sep 17 00:00:00 2001 From: tomneedham Date: Thu, 14 Nov 2013 01:10:56 +0000 Subject: [PATCH 05/10] Return result object when only one successful response is returned --- lib/private/api.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/private/api.php b/lib/private/api.php index 913b3ff0a18..76e00958a24 100644 --- a/lib/private/api.php +++ b/lib/private/api.php @@ -167,11 +167,11 @@ class OC_API { // Which reponse code should we return? // Maybe any that are not OC_API::RESPOND_SERVER_ERROR $response = reset($shipped['failed']); - return $response; + return $response['response']; } elseif(!empty($thirdparty['failed'])) { // Return the third party failure result $response = reset($thirdparty['failed']); - return $response; + return $response['response']; } else { $responses = $thirdparty['succeeded']; } From 5a2d7008667242836b00260da73497849f2d6464 Mon Sep 17 00:00:00 2001 From: tomneedham Date: Thu, 14 Nov 2013 01:11:22 +0000 Subject: [PATCH 06/10] Fix type in unit test --- tests/lib/api.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/lib/api.php b/tests/lib/api.php index 6cff1823d50..a1f8d2459a0 100644 --- a/tests/lib/api.php +++ b/tests/lib/api.php @@ -66,7 +66,7 @@ class Test_API extends PHPUnit_Framework_TestCase { // Two shipped success results $result = OC_API::mergeResponses(array( - $this->buildResponse(true, $data1O), + $this->buildResponse(true, $data1), $this->buildResponse(true, $data2), )); $this->checkResult($result); From ca5c39a3a1ff624a0bbcb39456c93856b8983b51 Mon Sep 17 00:00:00 2001 From: tomneedham Date: Thu, 14 Nov 2013 01:14:37 +0000 Subject: [PATCH 07/10] Return failed shipped responses over succedded shipped responses --- lib/private/api.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/private/api.php b/lib/private/api.php index 76e00958a24..8ff6c3e7eeb 100644 --- a/lib/private/api.php +++ b/lib/private/api.php @@ -159,15 +159,15 @@ class OC_API { } // Remove any error responses if there is one shipped response that succeeded - if(!empty($shipped['succeeded'])) { - $responses = array_merge($shipped['succeeded'], $thirdparty['succeeded']); - } else if(!empty($shipped['failed'])) { + if(!empty($shipped['failed'])) { // Which shipped response do we use if they all failed? // They may have failed for different reasons (different status codes) // Which reponse code should we return? // Maybe any that are not OC_API::RESPOND_SERVER_ERROR $response = reset($shipped['failed']); return $response['response']; + } elseif(!empty($shipped['succeeded'])) { + $responses = array_merge($shipped['succeeded'], $thirdparty['succeeded']); } elseif(!empty($thirdparty['failed'])) { // Return the third party failure result $response = reset($thirdparty['failed']); From bb182bbfb2d77a565e758e9478be285a3be999a3 Mon Sep 17 00:00:00 2001 From: tomneedham Date: Thu, 14 Nov 2013 01:19:46 +0000 Subject: [PATCH 08/10] Merge multiple shipped failures before responding. --- lib/private/api.php | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/lib/private/api.php b/lib/private/api.php index 8ff6c3e7eeb..3df2095ad4f 100644 --- a/lib/private/api.php +++ b/lib/private/api.php @@ -164,8 +164,16 @@ class OC_API { // They may have failed for different reasons (different status codes) // Which reponse code should we return? // Maybe any that are not OC_API::RESPOND_SERVER_ERROR - $response = reset($shipped['failed']); - return $response['response']; + // Merge failed responses if more than one + $data = array(); + $meta = array(); + foreach($shipped['failed'] as $failure) { + $data = array_merge_recursive($data, $failure['response']->getData()); + } + $picked = reset($shipped['failed']); + $code = $picked['response']->getStatusCode(); + $response = new OC_OCS_Result($data, $code); + return $response; } elseif(!empty($shipped['succeeded'])) { $responses = array_merge($shipped['succeeded'], $thirdparty['succeeded']); } elseif(!empty($thirdparty['failed'])) { From 1449437c9ec2e1ad9a58ba12abd9a3b27d8e39f1 Mon Sep 17 00:00:00 2001 From: tomneedham Date: Thu, 14 Nov 2013 01:21:54 +0000 Subject: [PATCH 09/10] Merge multiple failed third party responses when returning the result --- lib/private/api.php | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/lib/private/api.php b/lib/private/api.php index 3df2095ad4f..03d7b7382a5 100644 --- a/lib/private/api.php +++ b/lib/private/api.php @@ -177,9 +177,16 @@ class OC_API { } elseif(!empty($shipped['succeeded'])) { $responses = array_merge($shipped['succeeded'], $thirdparty['succeeded']); } elseif(!empty($thirdparty['failed'])) { - // Return the third party failure result - $response = reset($thirdparty['failed']); - return $response['response']; + // Merge failed responses if more than one + $data = array(); + $meta = array(); + foreach($thirdparty['failed'] as $failure) { + $data = array_merge_recursive($data, $failure['response']->getData()); + } + $picked = reset($thirdparty['failed']); + $code = $picked['response']->getStatusCode(); + $response = new OC_OCS_Result($data, $code); + return $response; } else { $responses = $thirdparty['succeeded']; } From 7e637225349250cf6844f1d9c762e2110bc46a7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= Date: Thu, 14 Nov 2013 15:37:30 +0100 Subject: [PATCH 10/10] make use of data providers --- tests/lib/api.php | 191 +++++++++++++++++++--------------------------- 1 file changed, 80 insertions(+), 111 deletions(-) diff --git a/tests/lib/api.php b/tests/lib/api.php index a1f8d2459a0..95d75f15311 100644 --- a/tests/lib/api.php +++ b/tests/lib/api.php @@ -9,7 +9,7 @@ class Test_API extends PHPUnit_Framework_TestCase { // Helps build a response variable - public function buildResponse($shipped=true, $data=null, $code=100) { + function buildResponse($shipped, $data, $code) { return array( 'shipped' => $shipped, 'response' => new OC_OCS_Result($data, $code), @@ -18,142 +18,111 @@ class Test_API extends PHPUnit_Framework_TestCase { } // Validate details of the result - public function checkResult($result, $success=true) { + function checkResult($result, $success) { // Check response is of correct type - $this->assertEquals('OC_OCS_Result', get_class($result)); - // CHeck if it succeeded + $this->assertInstanceOf('OC_OCS_Result', $result); + // Check if it succeeded + /** @var $result OC_OCS_Result */ $this->assertEquals($success, $result->succeeded()); } - // Test the merging of multiple responses - public function testMergeResponses(){ + function dataProviderTestOneResult() { + return array( + array(100, true), + array(101, true), + array(997, false), + ); + } + + /** + * @dataProvider dataProviderTestOneResult + * + * @param $statusCode + * @param $succeeded + */ + public function testOneResult($statusCode, $succeeded) { + // Setup some data arrays + $data1 = array( + 'users' => array( + 'tom' => array( + 'key' => 'value', + ), + 'frank' => array( + 'key' => 'value', + ), + )); + + // Test merging one success result + $response = $this->buildResponse(true, $data1, $statusCode); + $result = OC_API::mergeResponses(array($response)); + $this->assertEquals($response['response'], $result); + $this->checkResult($result, $succeeded); + } + + function dataProviderTestMergeResponses() { + return array( + // Two shipped success results + array(true, 100, true, 100, true), + // Two shipped results, one success and one failure + array(true, 100, true, 997, false), + // Two shipped results, both failure + array(true, 997, true, 997, false), + // Two third party success results + array(false, 100, false, 100, true), + // Two third party results, one success and one failure + array(false, 100, false, 997, false), + // Two third party results, both failure + array(false, 997, false, 997, false), + // One of each, both success + array(false, 100, true, 100, true), + array(true, 100, false, 100, true), + // One of each, both failure + array(false, 997, true, 997, false), + // One of each, shipped success + array(false, 997, true, 100, true), + // One of each, third party success + array(false, 100, true, 997, false), + ); + } + /** + * @dataProvider dataProviderTestMergeResponses + * + * Test the merging of multiple responses + * @param $statusCode1 + * @param $statusCode2 + * @param $succeeded + */ + public function testMultipleMergeResponses($shipped1, $statusCode1, $shipped2, $statusCode2, $succeeded){ // Tests that app responses are merged correctly // Setup some data arrays $data1 = array( 'users' => array( 'tom' => array( 'key' => 'value', - ), + ), 'frank' => array( 'key' => 'value', - ), + ), )); $data2 = array( 'users' => array( 'tom' => array( 'key' => 'newvalue', - ), + ), 'jan' => array( 'key' => 'value', - ), + ), )); - // Test merging one success result - $response = $this->buildResponse(true, $data1); - $result = OC_API::mergeResponses(array($response)); - $this->assertEquals($response['response'], $result); - $this->checkResult($result); - - $response = $this->buildResponse(true, $data1, 101); - $result = OC_API::mergeResponses(array($response)); - $this->assertEquals($response['response'], $result); - $this->checkResult($result); - - $response = $this->buildResponse(true, $data1, 997); - $result = OC_API::mergeResponses(array($response)); - $this->assertEquals($response['response'], $result); - $this->checkResult($result, false); // Two shipped success results $result = OC_API::mergeResponses(array( - $this->buildResponse(true, $data1), - $this->buildResponse(true, $data2), - )); - $this->checkResult($result); + $this->buildResponse($shipped1, $data1, $statusCode1), + $this->buildResponse($shipped2, $data2, $statusCode2), + )); + $this->checkResult($result, $succeeded); $resultData = $result->getData(); $this->assertArrayHasKey('jan', $resultData['users']); - - // Two shipped results, one success and one failure - $result = OC_API::mergeResponses(array( - $this->buildResponse(true, $data1), - $this->buildResponse(true, $data2, 997), - )); - $this->checkResult($result, false); - $resultData = $result->getData(); - $this->assertArrayHasKey('jan', $resultData['users']); - - // Two shipped results, both failure - $result = OC_API::mergeResponses(array( - $this->buildResponse(true, $data1, 997), - $this->buildResponse(true, $data2, 997), - )); - $this->checkResult($result, false); - $resultData = $result->getData(); - $this->assertArrayHasKey('jan', $resultData['users']); - - // Two third party success results - $result = OC_API::mergeResponses(array( - $this->buildResponse(false, $data1), - $this->buildResponse(false, $data2), - )); - $this->checkResult($result); - $resultData = $result->getData(); - $this->assertArrayHasKey('jan', $resultData['users']); - - // Two third party results, one success and one failure - $result = OC_API::mergeResponses(array( - $this->buildResponse(false, $data1), - $this->buildResponse(false, $data2, 997), - )); - $this->checkResult($result, false); - $resultData = $result->getData(); - $this->assertArrayHasKey('jan', $resultData['users']); - - // Two third party results, both failure - $result = OC_API::mergeResponses(array( - $this->buildResponse(false, $data1, 997), - $this->buildResponse(false, $data2, 997), - )); - $this->checkResult($result, false); - $resultData = $result->getData(); - $this->assertArrayHasKey('jan', $resultData['users']); - - // One of each, both success - $result = OC_API::mergeResponses(array( - $this->buildResponse(false, $data1), - $this->buildResponse(true, $data2), - )); - $this->checkResult($result); - $resultData = $result->getData(); - $this->assertArrayHasKey('jan', $resultData['users']); - - // One of each, both failure - $result = OC_API::mergeResponses(array( - $this->buildResponse(false, $data1, 997), - $this->buildResponse(true, $data2, 997), - )); - $this->checkResult($result, false); - $resultData = $result->getData(); - $this->assertArrayHasKey('jan', $resultData['users']); - - // One of each, shipped success - $result = OC_API::mergeResponses(array( - $this->buildResponse(false, $data1, 997), - $this->buildResponse(true, $data2), - )); - $this->checkResult($result); - $resultData = $result->getData(); - $this->assertArrayHasKey('jan', $resultData['users']); - - // One of each, third party success - $result = OC_API::mergeResponses(array( - $this->buildResponse(false, $data1), - $this->buildResponse(true, $data2, 997), - )); - $this->checkResult($result, false); - $resultData = $result->getData(); - $this->assertArrayHasKey('jan', $resultData['users']); - } }