From f1ab7a2e4934012677027ccc76136f132663b7d7 Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Tue, 14 Dec 2021 17:53:09 -0800 Subject: [PATCH] Civi::pipe - Define option `apiError` This option balances two interests: - For generic plumbing code, it's easier to keep the API errors in their canonical format. - For bespoke JSON-RPC client code, it's easier to bind errors into JSON-RPC errors. This way downstream clients can adapt the errors to their own convention (PHP exceptions, JS exceptions, promise-failures, ad nauseum) --- Civi/Pipe/PublicMethods.php | 67 ++++++++++++++++--- .../phpunit/Civi/Pipe/JsonRpcSessionTest.php | 58 +++++++++++++++- 2 files changed, 113 insertions(+), 12 deletions(-) diff --git a/Civi/Pipe/PublicMethods.php b/Civi/Pipe/PublicMethods.php index c6cdd86f4a..9a9d844cf9 100644 --- a/Civi/Pipe/PublicMethods.php +++ b/Civi/Pipe/PublicMethods.php @@ -16,6 +16,15 @@ namespace Civi\Pipe; */ class PublicMethods { + /** + * How should API errors be reported? + * + * @var string + * - 'array': Traditional array format from civicrm_api(). Maximizes consistency of error data. + * - 'exception': Converted to an exception. Somewhat lossy. Improves out-of-box DX on stricter JSON-RPC clients. + */ + protected $apiError = 'array'; + /** * Send a request to APIv3. * @@ -26,7 +35,16 @@ class PublicMethods { */ public function api3($session, $request) { $request[2] = array_merge(['version' => 3, 'check_permissions' => TRUE], $request[2] ?? []); - return civicrm_api(...$request); + switch ($this->apiError) { + case 'array': + return civicrm_api(...$request); + + case 'exception': + return civicrm_api3(...$request); + + default: + throw new \CRM_Core_Exception("Invalid API error-handling mode: $this->apiError"); + } } /** @@ -39,7 +57,16 @@ class PublicMethods { */ public function api4($session, $request) { $request[2] = array_merge(['version' => 4, 'checkPermissions' => TRUE], $request[2] ?? []); - return civicrm_api(...$request); + switch ($this->apiError) { + case 'array': + return civicrm_api(...$request); + + case 'exception': + return civicrm_api4(...$request); + + default: + throw new \CRM_Core_Exception("Invalid API error-handling mode: $this->apiError"); + } } /** @@ -79,24 +106,42 @@ class PublicMethods { * If the list of updates was empty, then return all options. */ public function options($session, $request) { - $map = [ - 'responsePrefix' => $session, + $storageMap = [ + 'apiError' => $this, 'maxLine' => $session, + 'responsePrefix' => $session, ]; + $get = function($storage, $name) { + if (method_exists($storage, 'get' . ucfirst($name))) { + return $storage->{'get' . ucfirst($name)}(); + } + else { + return $storage->{$name}; + } + }; + + $set = function($storage, $name, $value) use ($get) { + if (method_exists($storage, 'set' . ucfirst($name))) { + $storage->{'set' . ucfirst($name)}($value); + } + else { + $storage->{$name} = $value; + } + return $get($storage, $name); + }; + $result = []; if (!empty($request)) { - foreach ($request as $option => $value) { - if (isset($map[$option])) { - $storage = $map[$option]; - $storage->{'set' . ucfirst($option)}($value); - $result[$option] = $storage->{'get' . ucfirst($option)}(); + foreach ($request as $name => $value) { + if (isset($storageMap[$name])) { + $result[$name] = $set($storageMap[$name], $name, $value); } } } else { - foreach ($map as $option => $storage) { - $result[$option] = $storage->{'get' . ucfirst($option)}(); + foreach ($storageMap as $name => $storage) { + $result[$name] = $get($storage, $name); } } return $result; diff --git a/tests/phpunit/Civi/Pipe/JsonRpcSessionTest.php b/tests/phpunit/Civi/Pipe/JsonRpcSessionTest.php index b1b2f34016..51add84143 100644 --- a/tests/phpunit/Civi/Pipe/JsonRpcSessionTest.php +++ b/tests/phpunit/Civi/Pipe/JsonRpcSessionTest.php @@ -81,7 +81,7 @@ class JsonRpcSessionTest extends \CiviUnitTestCase { public function testControl() { $this->assertRequestResponse([ - '{"jsonrpc":"2.0","id":"c","method":"options"}' => '{"jsonrpc":"2.0","result":{"responsePrefix":null,"maxLine":524288},"id":"c"}', + '{"jsonrpc":"2.0","id":"c","method":"options"}' => '{"jsonrpc":"2.0","result":{"apiError":"array","bufferSize":524288,"responsePrefix":null},"id":"c"}', '{"jsonrpc":"2.0","id":"c","method":"options","params":{"responsePrefix":"ZZ"}}' => 'ZZ{"jsonrpc":"2.0","result":{"responsePrefix":"ZZ"},"id":"c"}', '{"jsonrpc":"2.0","id":"c","method": "echo","params":123}' => 'ZZ{"jsonrpc":"2.0","result":123,"id":"c"}', ]); @@ -98,6 +98,34 @@ class JsonRpcSessionTest extends \CiviUnitTestCase { $this->assertEquals(\CRM_Utils_System::version(), $decode['result']['values'][0]['version']); } + public function testApi3ErrorModes() { + $responses = $this->runLines([ + // First call: Use default/traditional API error mode + '{"jsonrpc":"2.0","id":"bad1","method":"api3","params":["System","zznnzznnzz"]}', + // Second call: Bind API errors to JSON-RPC errors. + '{"jsonrpc":"2.0","id":"o","method":"options","params":{"apiError":"exception"}}', + '{"jsonrpc":"2.0","id":"bad2","method":"api3","params":["System","zznnzznnzz"]}', + ]); + + $this->assertEquals($this->standardHeader, $responses[0]); + + $decode = json_decode($responses[1], TRUE); + $this->assertEquals('2.0', $decode['jsonrpc']); + $this->assertEquals('bad1', $decode['id']); + $this->assertEquals(1, $decode['result']['is_error']); + $this->assertRegexp(';API.*System.*zznnzznnzz.*not exist;', $decode['result']['error_message']); + + $decode = json_decode($responses[2], TRUE); + $this->assertEquals('2.0', $decode['jsonrpc']); + $this->assertEquals('o', $decode['id']); + $this->assertEquals('exception', $decode['result']['apiError']); + + $decode = json_decode($responses[3], TRUE); + $this->assertEquals('2.0', $decode['jsonrpc']); + $this->assertEquals('bad2', $decode['id']); + $this->assertRegexp(';API.*System.*zznnzznnzz.*not exist;', $decode['error']['message']); + } + public function testApi4() { $responses = $this->runLines(['{"jsonrpc":"2.0","id":"a4","method":"api4","params":["Contact","getFields"]}']); @@ -111,6 +139,34 @@ class JsonRpcSessionTest extends \CiviUnitTestCase { $this->assertEquals('Number', $fields['id']['input_type']); } + public function testApi4ErrorModes() { + $responses = $this->runLines([ + // First call: Use default/traditional API error mode + '{"jsonrpc":"2.0","id":"bad1","method":"api4","params":["System","zznnzznnzz"]}', + // Second call: Bind API errors to JSON-RPC errors. + '{"jsonrpc":"2.0","id":"o","method":"options","params":{"apiError":"exception"}}', + '{"jsonrpc":"2.0","id":"bad2","method":"api4","params":["System","zznnzznnzz"]}', + ]); + + $this->assertEquals($this->standardHeader, $responses[0]); + + $decode = json_decode($responses[1], TRUE); + $this->assertEquals('2.0', $decode['jsonrpc']); + $this->assertEquals('bad1', $decode['id']); + $this->assertEquals(1, $decode['result']['is_error']); + $this->assertRegexp(';Api.*System.*zznnzznnzz.*not exist;', $decode['result']['error_message']); + + $decode = json_decode($responses[2], TRUE); + $this->assertEquals('2.0', $decode['jsonrpc']); + $this->assertEquals('o', $decode['id']); + $this->assertEquals('exception', $decode['result']['apiError']); + + $decode = json_decode($responses[3], TRUE); + $this->assertEquals('2.0', $decode['jsonrpc']); + $this->assertEquals('bad2', $decode['id']); + $this->assertRegexp(';Api.*System.*zznnzznnzz.*not exist;', $decode['error']['message']); + } + /** * @param array $requestResponse * List of requests and the corresponding responses. -- 2.25.1