From 60a6221505ce7914efb615df14ae664a04a7773f Mon Sep 17 00:00:00 2001 From: Coleman Watts Date: Sat, 11 Sep 2021 15:16:18 -0400 Subject: [PATCH] APIv4 - Default select clause to exclude "Extra" fields Recently a "type" property was added to getFieldSpec to allow "Extra" calculated fields to be declared. This updates api Get to *not* select those fields by default, as their calculation can be expensive. --- Civi/Api4/Generic/AbstractGetAction.php | 24 ++++++-- Civi/Api4/Generic/DAOGetAction.php | 4 +- .../core/Civi/Api4/Action/Afform/Get.php | 2 +- ext/afform/core/Civi/Api4/Afform.php | 4 ++ .../phpunit/Civi/Afform/AfformGetTest.php | 57 +++++++++++++++++++ .../api/v4/AfformCustomFieldUsageTest.php | 1 + .../mock/tests/phpunit/api/v4/AfformTest.php | 12 +++- ext/oauth-client/Civi/Api4/OAuthProvider.php | 3 + .../api/v4/Action/BasicActionsTest.php | 2 +- .../api/v4/Action/CurrentFilterTest.php | 6 ++ .../api/v4/Mock/Api4/MockBasicEntity.php | 3 + 11 files changed, 106 insertions(+), 12 deletions(-) create mode 100644 ext/afform/core/tests/phpunit/Civi/Afform/AfformGetTest.php diff --git a/Civi/Api4/Generic/AbstractGetAction.php b/Civi/Api4/Generic/AbstractGetAction.php index 0d5b27739b..5fd9274683 100644 --- a/Civi/Api4/Generic/AbstractGetAction.php +++ b/Civi/Api4/Generic/AbstractGetAction.php @@ -66,18 +66,32 @@ abstract class AbstractGetAction extends AbstractQueryAction { } /** - * Adds all fields matched by the * wildcard + * Adds all standard fields matched by the * wildcard + * + * Note: this function only deals with simple wildcard expressions. + * It ignores those containing special characters like dots or parentheses, + * they are handled separately in Api4SelectQuery. * * @throws \API_Exception */ protected function expandSelectClauseWildcards() { + if (!$this->select) { + $this->select = ['*']; + } + // Get expressions containing wildcards but no dots or parentheses $wildFields = array_filter($this->select, function($item) { return strpos($item, '*') !== FALSE && strpos($item, '.') === FALSE && strpos($item, '(') === FALSE && strpos($item, ' ') === FALSE; }); - foreach ($wildFields as $item) { - $pos = array_search($item, array_values($this->select)); - $matches = SelectUtil::getMatchingFields($item, array_column($this->entityFields(), 'name')); - array_splice($this->select, $pos, 1, $matches); + if ($wildFields) { + // Wildcards should not match "Extra" fields + $standardFields = array_filter(array_map(function($field) { + return $field['type'] === 'Extra' ? NULL : $field['name']; + }, $this->entityFields())); + foreach ($wildFields as $item) { + $pos = array_search($item, array_values($this->select)); + $matches = SelectUtil::getMatchingFields($item, $standardFields); + array_splice($this->select, $pos, 1, $matches); + } } $this->select = array_unique($this->select); } diff --git a/Civi/Api4/Generic/DAOGetAction.php b/Civi/Api4/Generic/DAOGetAction.php index aedc6362ca..e8a46ce5b5 100644 --- a/Civi/Api4/Generic/DAOGetAction.php +++ b/Civi/Api4/Generic/DAOGetAction.php @@ -29,9 +29,9 @@ class DAOGetAction extends AbstractGetAction { use Traits\DAOActionTrait; /** - * Fields to return. Defaults to all non-custom fields `['*']`. + * Fields to return. Defaults to all standard (non-custom, non-extra) fields `['*']`. * - * The keyword `"custom.*"` selects all custom fields. So to select all core + custom fields, select `['*', 'custom.*']`. + * The keyword `"custom.*"` selects all custom fields. So to select all standard + custom fields, select `['*', 'custom.*']`. * * Use the dot notation to perform joins in the select clause, e.g. selecting `['*', 'contact.*']` from `Email::get()` * will select all fields for the email + all fields for the related contact. diff --git a/ext/afform/core/Civi/Api4/Action/Afform/Get.php b/ext/afform/core/Civi/Api4/Action/Afform/Get.php index 6a9ae6e681..f02b12410c 100644 --- a/ext/afform/core/Civi/Api4/Action/Afform/Get.php +++ b/ext/afform/core/Civi/Api4/Action/Afform/Get.php @@ -17,7 +17,7 @@ class Get extends \Civi\Api4\Generic\BasicGetAction { public function getRecords() { /** @var \CRM_Afform_AfformScanner $scanner */ $scanner = \Civi::service('afform_scanner'); - $getComputed = $this->_isFieldSelected('has_local') || $this->_isFieldSelected('has_base'); + $getComputed = $this->_isFieldSelected('has_local', 'has_base'); $getLayout = $this->_isFieldSelected('layout'); $values = []; diff --git a/ext/afform/core/Civi/Api4/Afform.php b/ext/afform/core/Civi/Api4/Afform.php index ec76fd3df2..8ccf9eb54d 100644 --- a/ext/afform/core/Civi/Api4/Afform.php +++ b/ext/afform/core/Civi/Api4/Afform.php @@ -201,19 +201,23 @@ class Afform extends Generic\AbstractEntity { if ($self->getAction() === 'get') { $fields[] = [ 'name' => 'module_name', + 'type' => 'Extra', 'readonly' => TRUE, ]; $fields[] = [ 'name' => 'directive_name', + 'type' => 'Extra', 'readonly' => TRUE, ]; $fields[] = [ 'name' => 'has_local', + 'type' => 'Extra', 'data_type' => 'Boolean', 'readonly' => TRUE, ]; $fields[] = [ 'name' => 'has_base', + 'type' => 'Extra', 'data_type' => 'Boolean', 'readonly' => TRUE, ]; diff --git a/ext/afform/core/tests/phpunit/Civi/Afform/AfformGetTest.php b/ext/afform/core/tests/phpunit/Civi/Afform/AfformGetTest.php new file mode 100644 index 0000000000..2268cb1938 --- /dev/null +++ b/ext/afform/core/tests/phpunit/Civi/Afform/AfformGetTest.php @@ -0,0 +1,57 @@ +installMe(__DIR__)->apply(); + } + + public function tearDown(): void { + Afform::revert(FALSE)->addWhere('name', '=', $this->formName)->execute(); + parent::tearDown(); + } + + public function testGetReturnFields() { + Afform::create() + ->addValue('name', $this->formName) + ->addValue('title', 'Test Form') + ->execute(); + + // Omitting select should return regular fields but not extra fields + $result = Afform::get() + ->addWhere('name', '=', $this->formName) + ->execute()->single(); + $this->assertEquals($this->formName, $result['name']); + $this->assertArrayNotHasKey('directive_name', $result); + $this->assertArrayNotHasKey('has_base', $result); + + // Select * should also return regular fields only + $result = Afform::get() + ->addSelect('*') + ->addWhere('name', '=', $this->formName) + ->execute()->single(); + $this->assertEquals($this->formName, $result['name']); + $this->assertArrayNotHasKey('module_name', $result); + $this->assertArrayNotHasKey('has_local', $result); + + // Selecting * and has_base should return core and that one extra field + $result = Afform::get() + ->addSelect('*', 'has_base') + ->addWhere('name', '=', $this->formName) + ->execute()->single(); + $this->assertEquals($this->formName, $result['name']); + $this->assertFalse($result['has_base']); + $this->assertArrayNotHasKey('has_local', $result); + } + +} diff --git a/ext/afform/mock/tests/phpunit/api/v4/AfformCustomFieldUsageTest.php b/ext/afform/mock/tests/phpunit/api/v4/AfformCustomFieldUsageTest.php index 3ece0fb88d..e1e848a208 100644 --- a/ext/afform/mock/tests/phpunit/api/v4/AfformCustomFieldUsageTest.php +++ b/ext/afform/mock/tests/phpunit/api/v4/AfformCustomFieldUsageTest.php @@ -50,6 +50,7 @@ EOHTML; // Creating a custom group should automatically create an afform block $block = \Civi\Api4\Afform::get() ->addWhere('name', '=', 'afblockCustom_MyThings') + ->addSelect('layout', 'directive_name') ->setLayoutFormat('shallow') ->setFormatWhitespace(TRUE) ->execute()->single(); diff --git a/ext/afform/mock/tests/phpunit/api/v4/AfformTest.php b/ext/afform/mock/tests/phpunit/api/v4/AfformTest.php index 3d57c90bb0..ffb62766f8 100644 --- a/ext/afform/mock/tests/phpunit/api/v4/AfformTest.php +++ b/ext/afform/mock/tests/phpunit/api/v4/AfformTest.php @@ -53,7 +53,9 @@ class api_v4_AfformTest extends api_v4_AfformTestCase { Civi\Api4\Afform::revert()->addWhere('name', '=', $formName)->execute(); $message = 'The initial Afform.get should return default data'; - $result = Civi\Api4\Afform::get()->addWhere('name', '=', $formName)->execute(); + $result = Civi\Api4\Afform::get() + ->addSelect('*', 'has_base', 'has_local') + ->addWhere('name', '=', $formName)->execute(); $this->assertEquals($formName, $result[0]['name'], $message); $this->assertEquals($get($originalMetadata, 'title'), $get($result[0], 'title'), $message); $this->assertEquals($get($originalMetadata, 'description'), $get($result[0], 'description'), $message); @@ -75,7 +77,9 @@ class api_v4_AfformTest extends api_v4_AfformTestCase { $this->assertEquals('The temporary description', $result[0]['description'], $message); $message = 'After updating, the Afform.get API should return blended data'; - $result = Civi\Api4\Afform::get()->addWhere('name', '=', $formName)->execute(); + $result = Civi\Api4\Afform::get() + ->addSelect('*', 'has_base', 'has_local') + ->addWhere('name', '=', $formName)->execute(); $this->assertEquals($formName, $result[0]['name'], $message); $this->assertEquals($get($originalMetadata, 'title'), $get($result[0], 'title'), $message); $this->assertEquals('The temporary description', $get($result[0], 'description'), $message); @@ -88,7 +92,9 @@ class api_v4_AfformTest extends api_v4_AfformTestCase { Civi\Api4\Afform::revert()->addWhere('name', '=', $formName)->execute(); $message = 'After reverting, the final Afform.get should return default data'; - $result = Civi\Api4\Afform::get()->addWhere('name', '=', $formName)->execute(); + $result = Civi\Api4\Afform::get() + ->addSelect('*', 'has_base', 'has_local') + ->addWhere('name', '=', $formName)->execute(); $this->assertEquals($formName, $result[0]['name'], $message); $this->assertEquals($get($originalMetadata, 'title'), $get($result[0], 'title'), $message); $this->assertEquals($get($originalMetadata, 'description'), $get($result[0], 'description'), $message); diff --git a/ext/oauth-client/Civi/Api4/OAuthProvider.php b/ext/oauth-client/Civi/Api4/OAuthProvider.php index fc556d5192..726248815f 100644 --- a/ext/oauth-client/Civi/Api4/OAuthProvider.php +++ b/ext/oauth-client/Civi/Api4/OAuthProvider.php @@ -59,6 +59,9 @@ class OAuthProvider extends Generic\AbstractEntity { [ 'name' => 'options', ], + [ + 'name' => 'contactTemplate', + ], ]; }); return $action->setCheckPermissions($checkPermissions); diff --git a/tests/phpunit/api/v4/Action/BasicActionsTest.php b/tests/phpunit/api/v4/Action/BasicActionsTest.php index 4dae4f5f9d..d8d5e9d1c1 100644 --- a/tests/phpunit/api/v4/Action/BasicActionsTest.php +++ b/tests/phpunit/api/v4/Action/BasicActionsTest.php @@ -158,7 +158,7 @@ class BasicActionsTest extends UnitTestCase { public function testGetFields() { $getFields = MockBasicEntity::getFields()->execute()->indexBy('name'); - $this->assertCount(7, $getFields); + $this->assertCount(8, $getFields); $this->assertEquals('Identifier', $getFields['identifier']['title']); // Ensure default data type is "String" when not specified $this->assertEquals('String', $getFields['color']['data_type']); diff --git a/tests/phpunit/api/v4/Action/CurrentFilterTest.php b/tests/phpunit/api/v4/Action/CurrentFilterTest.php index febac7383b..aa214029a0 100644 --- a/tests/phpunit/api/v4/Action/CurrentFilterTest.php +++ b/tests/phpunit/api/v4/Action/CurrentFilterTest.php @@ -83,6 +83,12 @@ class CurrentFilterTest extends UnitTestCase { $this->assertArrayNotHasKey($expiring['id'], $notCurrent); $this->assertArrayHasKey($past['id'], $notCurrent); $this->assertArrayHasKey($inactive['id'], $notCurrent); + + // Assert that "Extra" fields like is_current are not returned with select * + $defaultGet = Relationship::get()->setLimit(1)->execute()->single(); + $this->assertArrayNotHasKey('is_current', $defaultGet); + $starGet = Relationship::get()->addSelect('*')->setLimit(1)->execute()->single(); + $this->assertArrayNotHasKey('is_current', $starGet); } } diff --git a/tests/phpunit/api/v4/Mock/Api4/MockBasicEntity.php b/tests/phpunit/api/v4/Mock/Api4/MockBasicEntity.php index b991947cd7..a9c832e050 100644 --- a/tests/phpunit/api/v4/Mock/Api4/MockBasicEntity.php +++ b/tests/phpunit/api/v4/Mock/Api4/MockBasicEntity.php @@ -60,6 +60,9 @@ class MockBasicEntity extends Generic\BasicEntity { [ 'name' => 'size', ], + [ + 'name' => 'foo', + ], [ 'name' => 'weight', 'data_type' => 'Integer', -- 2.25.1