From a689294ce222c6c9682023700045839504cfbd7a Mon Sep 17 00:00:00 2001 From: Coleman Watts Date: Thu, 26 Mar 2020 21:01:28 -0400 Subject: [PATCH] Api4SelectQuery - Refactor field handling for looser coupling and more flexibility Getting ready to support groupBy, having and sql functions --- .../Subscriber/PostSelectQuerySubscriber.php | 2 +- Civi/Api4/Query/Api4SelectQuery.php | 265 +++++++----------- .../Schema/Joinable/CustomGroupJoinable.php | 2 +- Civi/Api4/Service/Spec/CustomFieldSpec.php | 23 -- Civi/Api4/Service/Spec/FieldSpec.php | 24 +- Civi/Api4/Service/Spec/SpecFormatter.php | 2 +- Civi/Api4/Service/Spec/SpecGatherer.php | 4 +- .../api/v4/Action/ContactApiKeyTest.php | 24 +- .../phpunit/api/v4/Spec/SpecFormatterTest.php | 1 + 9 files changed, 149 insertions(+), 198 deletions(-) diff --git a/Civi/Api4/Event/Subscriber/PostSelectQuerySubscriber.php b/Civi/Api4/Event/Subscriber/PostSelectQuerySubscriber.php index 6419625239..e21c2dd381 100644 --- a/Civi/Api4/Event/Subscriber/PostSelectQuerySubscriber.php +++ b/Civi/Api4/Event/Subscriber/PostSelectQuerySubscriber.php @@ -210,7 +210,7 @@ class PostSelectQuerySubscriber implements EventSubscriberInterface { $selectFields = []; foreach ($selects as $select) { - $selectAlias = $query->getFkSelectAliases()[$select]; + $selectAlias = str_replace('`', '', $query->getField($select)['sql_name']); $fieldAlias = substr($select, strrpos($select, '.') + 1); $selectFields[$fieldAlias] = $selectAlias; } diff --git a/Civi/Api4/Query/Api4SelectQuery.php b/Civi/Api4/Query/Api4SelectQuery.php index 7e25516aaf..de08a7b152 100644 --- a/Civi/Api4/Query/Api4SelectQuery.php +++ b/Civi/Api4/Query/Api4SelectQuery.php @@ -77,6 +77,9 @@ class Api4SelectQuery extends SelectQuery { $baoName = CoreUtil::getBAOFromApiName($this->entity); $this->entityFieldNames = array_column($baoName::fields(), 'name'); $this->apiFieldSpec = $apiGet->entityFields(); + foreach ($this->apiFieldSpec as $key => $field) { + $this->apiFieldSpec[$key]['sql_name'] = '`' . self::MAIN_TABLE_ALIAS . '`.`' . $field['column_name'] . '`'; + } $this->constructQueryObject($baoName); @@ -93,26 +96,10 @@ class Api4SelectQuery extends SelectQuery { * @throws \Civi\API\Exception\UnauthorizedException */ public function getSql() { - $this->addJoins(); - $this->buildSelectFields(); + $this->buildSelectClause(); $this->buildWhereClause(); - - // Select - if (in_array('row_count', $this->select)) { - $this->query->select("count(*) as c"); - } - else { - foreach ($this->selectFields as $column => $alias) { - $this->query->select("$column as `$alias`"); - } - // Order by - $this->buildOrderBy(); - } - - // Limit - if (!empty($this->limit) || !empty($this->offset)) { - $this->query->limit($this->limit, $this->offset); - } + $this->buildOrderBy(); + $this->buildLimit(); return $this->query->toSQL(); } @@ -135,10 +122,12 @@ class Api4SelectQuery extends SelectQuery { break; } $results[$query->id] = []; - foreach ($this->selectFields as $column => $alias) { + foreach ($this->select as $alias) { $returnName = $alias; - $alias = str_replace('.', '_', $alias); - $results[$query->id][$returnName] = property_exists($query, $alias) ? $query->$alias : NULL; + if ($this->isOneToOneField($alias)) { + $alias = str_replace('.', '_', $alias); + $results[$query->id][$returnName] = property_exists($query, $alias) ? $query->$alias : NULL; + } }; } $event = new PostSelectQueryEvent($results, $this); @@ -147,55 +136,44 @@ class Api4SelectQuery extends SelectQuery { return $event->getResults(); } - /** - * Gets all FK fields and does the required joins - */ - protected function addJoins() { - $allFields = array_merge($this->select, array_keys($this->orderBy)); - $recurse = function($clauses) use (&$allFields, &$recurse) { - foreach ($clauses as $clause) { - if ($clause[0] === 'NOT' && is_string($clause[1][0])) { - $recurse($clause[1][1]); - } - elseif (in_array($clause[0], ['AND', 'OR', 'NOT'])) { - $recurse($clause[1]); - } - elseif (is_array($clause[0])) { - array_walk($clause, $recurse); - } - else { - $allFields[] = $clause[0]; - } - } - }; - $recurse($this->where); - $dotFields = array_unique(array_filter($allFields, function ($field) { - return strpos($field, '.') !== FALSE; - })); - - foreach ($dotFields as $dotField) { - $this->joinFK($dotField); + protected function buildSelectClause() { + if (empty($this->select)) { + $this->select = $this->entityFieldNames; } - } - - /** - * Populate $this->selectFields - * - * @throws \Civi\API\Exception\UnauthorizedException - */ - protected function buildSelectFields() { - $selectAll = (empty($this->select) || in_array('*', $this->select)); - $select = $selectAll ? $this->entityFieldNames : $this->select; - - // Always select the ID if the table has one. - if (array_key_exists('id', $this->apiFieldSpec) || strstr($this->entity, 'Custom_')) { - $this->selectFields[self::MAIN_TABLE_ALIAS . ".id"] = "id"; + elseif (in_array('row_count', $this->select)) { + $this->query->select("COUNT(*) AS `c`"); + return; } - - // core return fields - foreach ($select as $fieldName) { - if (strpos($fieldName, '.') === FALSE || !array_filter($this->getPathJoinTypes($fieldName))) { - $this->selectFields[$this->getFieldSqlName($fieldName)] = $fieldName; + else { + // Always select id field + $this->select = array_merge(['id'], $this->select); + + // Expand wildcards in joins (the api wrapper already expanded non-joined wildcards) + $wildFields = array_filter($this->select, function($item) { + return strpos($item, '*') !== FALSE && strpos($item, '.') !== FALSE; + }); + foreach ($wildFields as $item) { + $pos = array_search($item, array_values($this->select)); + $this->joinFK($item); + $matches = SelectUtil::getMatchingFields($item, array_keys($this->apiFieldSpec)); + array_splice($this->select, $pos, 1, $matches); + } + $this->select = array_unique($this->select); + } + foreach ($this->select as $fieldName) { + $field = $this->getField($fieldName); + if (!$this->isOneToOneField($fieldName)) { + continue; + } + elseif ($field) { + $this->query->select($field['sql_name'] . " AS `$fieldName`"); + } + // Remove unknown fields without raising an error + else { + $this->select = array_diff($this->select, [$fieldName]); + if (is_array($this->debugOutput)) { + $this->debugOutput['undefined_fields'][] = $fieldName; + } } } } @@ -218,7 +196,16 @@ class Api4SelectQuery extends SelectQuery { if ($dir !== 'ASC' && $dir !== 'DESC') { throw new \API_Exception("Invalid sort direction. Cannot order by $fieldName $dir"); } - $this->query->orderBy($this->getFieldSqlName($fieldName) . " $dir"); + $this->query->orderBy($this->getField($fieldName, TRUE)['sql_name'] . " $dir"); + } + } + + /** + * @throws \CRM_Core_Exception + */ + protected function buildLimit() { + if (!empty($this->limit) || !empty($this->offset)) { + $this->query->limit($this->limit, $this->offset); } } @@ -271,41 +258,17 @@ class Api4SelectQuery extends SelectQuery { protected function validateClauseAndComposeSql($clause) { // Pad array for unary operators list($fieldName, $operator, $value) = array_pad($clause, 3, NULL); - $fieldSpec = $this->getField($fieldName); - $sqlName = $this->getFieldSqlName($fieldName); + $field = $this->getField($fieldName, TRUE); - FormattingUtil::formatInputValue($value, $fieldSpec, $this->getEntity()); + FormattingUtil::formatInputValue($value, $field, $this->getEntity()); - $sql_clause = \CRM_Core_DAO::createSQLFilter($sqlName, [$operator => $value]); + $sql_clause = \CRM_Core_DAO::createSQLFilter($field['sql_name'], [$operator => $value]); if ($sql_clause === NULL) { throw new \API_Exception("Invalid value in where clause for field '$fieldName'"); } return $sql_clause; } - /** - * Translates an api fieldname to the table.column name used in the query. - * - * @param $fieldName - * @return string - * @throws \API_Exception - */ - protected function getFieldSqlName($fieldName) { - $tableName = $columnName = NULL; - if (in_array($fieldName, $this->entityFieldNames)) { - $field = $this->getField($fieldName); - $tableName = self::MAIN_TABLE_ALIAS; - $columnName = $field['column_name'] ?? $field['name']; - } - elseif (strpos($fieldName, '.') && isset($this->fkSelectAliases[$fieldName])) { - list($tableName, $columnName) = explode('.', $this->fkSelectAliases[$fieldName]); - } - if (!$tableName || !$columnName) { - throw new \API_Exception("Invalid field '$fieldName'."); - } - return "`$tableName`.`$columnName`"; - } - /** * @inheritDoc */ @@ -317,88 +280,70 @@ class Api4SelectQuery extends SelectQuery { * Fetch a field from the getFields list * * @param string $fieldName + * @param bool $strict * * @return string|null + * @throws \API_Exception */ - protected function getField($fieldName) { - if ($fieldName) { - $fieldPath = explode('.', $fieldName); - if (count($fieldPath) > 1) { - $fieldName = implode('.', array_slice($fieldPath, -2)); - } - return $this->apiFieldSpec[$fieldName] ?? NULL; + public function getField($fieldName, $strict = FALSE) { + // Perform join if field not yet available - this will add it to apiFieldSpec + if (!isset($this->apiFieldSpec[$fieldName]) && strpos($fieldName, '.')) { + $this->joinFK($fieldName); + } + $field = $this->apiFieldSpec[$fieldName] ?? NULL; + // Check if field exists and we have permission to view it + if ($field && (!$this->checkPermissions || empty($field['permission']) || \CRM_Core_Permission::check($field['permission']))) { + return $field; + } + elseif ($strict) { + throw new \API_Exception("Invalid field '$fieldName'"); } return NULL; } /** + * Joins a path and adds all fields in the joined eneity to apiFieldSpec + * * @param $key + * @return bool * @throws \API_Exception + * @throws \Exception */ protected function joinFK($key) { - $pathArray = explode('.', $key); - - if (count($pathArray) < 2) { - return; + if (isset($this->apiFieldSpec[$key])) { + return TRUE; } + $pathArray = explode('.', $key); + /** @var \Civi\Api4\Service\Schema\Joiner $joiner */ $joiner = \Civi::container()->get('joiner'); - $field = array_pop($pathArray); + // The last item in the path is the field name. We don't care about that; we'll add all fields from the joined entity. + array_pop($pathArray); $pathString = implode('.', $pathArray); if (!$joiner->canJoin($this, $pathString)) { - return; + return FALSE; } $joinPath = $joiner->join($this, $pathString); /** @var \Civi\Api4\Service\Schema\Joinable\Joinable $lastLink */ $lastLink = array_pop($joinPath); - $isWild = strpos($field, '*') !== FALSE; - if ($isWild) { - if (!in_array($key, $this->select)) { - throw new \API_Exception('Wildcards can only be used in the SELECT clause.'); - } - $this->select = array_diff($this->select, [$key]); + // Custom field names are already prefixed + if ($lastLink instanceof CustomGroupJoinable) { + array_pop($pathArray); } - + $prefix = $pathArray ? implode('.', $pathArray) . '.' : ''; // Cache field info for retrieval by $this->getField() - $prefix = array_pop($pathArray) . '.'; - if (!isset($this->apiFieldSpec[$prefix . $field])) { - $joinEntity = $lastLink->getEntity(); - // Custom fields are already prefixed - if ($lastLink instanceof CustomGroupJoinable) { - $prefix = ''; - } - foreach ($lastLink->getEntityFields() as $fieldObject) { - $this->apiFieldSpec[$prefix . $fieldObject->getName()] = $fieldObject->toArray() + ['entity' => $joinEntity]; - } - } - - if (!$isWild && !$lastLink->getField($field)) { - throw new \API_Exception('Invalid join'); - } - - $fields = $isWild ? [] : [$field]; - // Expand wildcard and add matching fields to $this->select - if ($isWild) { - $fields = SelectUtil::getMatchingFields($field, $lastLink->getEntityFieldNames()); - foreach ($fields as $field) { - $this->select[] = $pathString . '.' . $field; - } - $this->select = array_unique($this->select); + $joinEntity = $lastLink->getEntity(); + foreach ($lastLink->getEntityFields() as $fieldObject) { + $fieldArray = ['entity' => $joinEntity] + $fieldObject->toArray(); + $fieldArray['sql_name'] = '`' . $lastLink->getAlias() . '`.`' . $fieldArray['column_name'] . '`'; + $this->apiFieldSpec[$prefix . $fieldArray['name']] = $fieldArray; } - foreach ($fields as $field) { - // custom groups use aliases for field names - $col = ($lastLink instanceof CustomGroupJoinable) ? $lastLink->getSqlColumn($field) : $field; - // Check Permission on field. - if ($this->checkPermissions && !empty($this->apiFieldSpec[$prefix . $field]['permission']) && !\CRM_Core_Permission::check($this->apiFieldSpec[$prefix . $field]['permission'])) { - continue; - } - $this->fkSelectAliases[$pathString . '.' . $field] = sprintf('%s.%s', $lastLink->getAlias(), $col); - } + return TRUE; } /** @@ -524,13 +469,6 @@ class Api4SelectQuery extends SelectQuery { return $this->apiVersion; } - /** - * @return array - */ - public function getFkSelectAliases() { - return $this->fkSelectAliases; - } - /** * @return \Civi\Api4\Service\Schema\Joinable\Joinable[] */ @@ -567,6 +505,19 @@ class Api4SelectQuery extends SelectQuery { } } + /** + * Checks if a field either belongs to the main entity or is joinable 1-to-1. + * + * Used to determine if a field can be added to the SELECT of the main query, + * or if it must be fetched post-query. + * + * @param string $fieldPath + * @return bool + */ + public function isOneToOneField(string $fieldPath) { + return strpos($fieldPath, '.') === FALSE || !array_filter($this->getPathJoinTypes($fieldPath)); + } + /** * Separates a string like 'emails.location_type.label' into an array, where * each value in the array tells whether it is 1-1 or 1-n join type diff --git a/Civi/Api4/Service/Schema/Joinable/CustomGroupJoinable.php b/Civi/Api4/Service/Schema/Joinable/CustomGroupJoinable.php index 695c7b5662..fd5ab30f9d 100644 --- a/Civi/Api4/Service/Schema/Joinable/CustomGroupJoinable.php +++ b/Civi/Api4/Service/Schema/Joinable/CustomGroupJoinable.php @@ -59,7 +59,7 @@ class CustomGroupJoinable extends Joinable { if (!$this->entityFields) { $fields = CustomField::get() ->setCheckPermissions(FALSE) - ->setSelect(['custom_group.name', 'custom_group_id', 'name', 'label', 'data_type', 'html_type', 'is_required', 'is_searchable', 'is_search_range', 'weight', 'is_active', 'is_view', 'option_group_id', 'default_value', 'date_format', 'time_format', 'start_date_years', 'end_date_years']) + ->setSelect(['custom_group.name', '*']) ->addWhere('custom_group.table_name', '=', $this->getTargetTable()) ->execute(); foreach ($fields as $field) { diff --git a/Civi/Api4/Service/Spec/CustomFieldSpec.php b/Civi/Api4/Service/Spec/CustomFieldSpec.php index cc62986e27..54ff20ee11 100644 --- a/Civi/Api4/Service/Spec/CustomFieldSpec.php +++ b/Civi/Api4/Service/Spec/CustomFieldSpec.php @@ -37,11 +37,6 @@ class CustomFieldSpec extends FieldSpec { */ protected $tableName; - /** - * @var string - */ - protected $columnName; - /** * @inheritDoc */ @@ -116,22 +111,4 @@ class CustomFieldSpec extends FieldSpec { return $this; } - /** - * @return string - */ - public function getCustomFieldColumnName() { - return $this->columnName; - } - - /** - * @param string $customFieldColumnName - * - * @return $this - */ - public function setCustomFieldColumnName($customFieldColumnName) { - $this->columnName = $customFieldColumnName; - - return $this; - } - } diff --git a/Civi/Api4/Service/Spec/FieldSpec.php b/Civi/Api4/Service/Spec/FieldSpec.php index 90a6844036..31caa2abb7 100644 --- a/Civi/Api4/Service/Spec/FieldSpec.php +++ b/Civi/Api4/Service/Spec/FieldSpec.php @@ -104,6 +104,11 @@ class FieldSpec { */ protected $permission; + /** + * @var string + */ + protected $columnName; + /** * Aliases for the valid data types * @@ -122,7 +127,7 @@ class FieldSpec { */ public function __construct($name, $entity, $dataType = 'String') { $this->entity = $entity; - $this->setName($name); + $this->name = $this->columnName = $name; $this->setDataType($dataType); } @@ -428,6 +433,23 @@ class FieldSpec { return $this; } + /** + * @return string + */ + public function getColumnName() { + return $this->columnName; + } + + /** + * @param string $columnName + * + * @return $this + */ + public function setColumnName($columnName) { + $this->columnName = $columnName; + return $this; + } + /** * @param array $values * @return array diff --git a/Civi/Api4/Service/Spec/SpecFormatter.php b/Civi/Api4/Service/Spec/SpecFormatter.php index b09a47157a..b9d1cf5bcc 100644 --- a/Civi/Api4/Service/Spec/SpecFormatter.php +++ b/Civi/Api4/Service/Spec/SpecFormatter.php @@ -61,8 +61,8 @@ class SpecFormatter { } else { $field->setCustomTableName($data['custom_group.table_name']); - $field->setCustomFieldColumnName($data['column_name']); } + $field->setColumnName($data['column_name']); $field->setCustomFieldId($data['id'] ?? NULL); $field->setCustomGroupName($data['custom_group.name']); $field->setTitle($data['label'] ?? NULL); diff --git a/Civi/Api4/Service/Spec/SpecGatherer.php b/Civi/Api4/Service/Spec/SpecGatherer.php index dc7304731c..d9909c477d 100644 --- a/Civi/Api4/Service/Spec/SpecGatherer.php +++ b/Civi/Api4/Service/Spec/SpecGatherer.php @@ -131,7 +131,7 @@ class SpecGatherer { ->setCheckPermissions(FALSE) ->addWhere('custom_group.extends', 'IN', $extends) ->addWhere('custom_group.is_multiple', '=', '0') - ->setSelect(['custom_group.name', 'custom_group_id', 'name', 'label', 'data_type', 'html_type', 'is_searchable', 'is_search_range', 'weight', 'is_active', 'is_view', 'option_group_id', 'default_value', 'date_format', 'time_format', 'start_date_years', 'end_date_years', 'help_pre', 'help_post']) + ->setSelect(['custom_group.name', '*']) ->execute(); foreach ($customFields as $fieldArray) { @@ -147,7 +147,7 @@ class SpecGatherer { private function getCustomGroupFields($customGroup, RequestSpec $specification) { $customFields = CustomField::get() ->addWhere('custom_group.name', '=', $customGroup) - ->setSelect(['custom_group.name', 'custom_group_id', 'name', 'label', 'data_type', 'html_type', 'is_searchable', 'is_search_range', 'weight', 'is_active', 'is_view', 'option_group_id', 'default_value', 'custom_group.table_name', 'column_name', 'date_format', 'time_format', 'start_date_years', 'end_date_years', 'help_pre', 'help_post']) + ->setSelect(['custom_group.name', 'custom_group.table_name', '*']) ->execute(); foreach ($customFields as $fieldArray) { diff --git a/tests/phpunit/api/v4/Action/ContactApiKeyTest.php b/tests/phpunit/api/v4/Action/ContactApiKeyTest.php index 54bbd424c5..3e411514f3 100644 --- a/tests/phpunit/api/v4/Action/ContactApiKeyTest.php +++ b/tests/phpunit/api/v4/Action/ContactApiKeyTest.php @@ -33,9 +33,6 @@ class ContactApiKeyTest extends \api\v4\UnitTestCase { \CRM_Core_Config::singleton()->userPermissionClass->permissions = ['access CiviCRM', 'add contacts', 'edit api keys', 'view all contacts', 'edit all contacts']; $key = \CRM_Utils_String::createRandom(16, \CRM_Utils_String::ALPHANUMERIC); $isSafe = function ($mixed) use ($key) { - if ($mixed instanceof Result) { - $mixed = $mixed->getArrayCopy(); - } return strpos(json_encode($mixed), $key) === FALSE; }; @@ -69,28 +66,31 @@ class ContactApiKeyTest extends \api\v4\UnitTestCase { $this->assertFalse($isSafe($email), "Should reveal secret details ($key): " . var_export($email, 1)); // Remove permission and we should not see the key - \CRM_Core_Config::singleton()->userPermissionClass->permissions = ['access CiviCRM']; + \CRM_Core_Config::singleton()->userPermissionClass->permissions = ['access CiviCRM', 'view debug output', 'view all contacts']; $result = Contact::get() ->addWhere('id', '=', $contact['id']) ->addSelect('api_key') - ->execute() - ->first(); - $this->assertTrue(empty($result['api_key'])); - $this->assertTrue($isSafe($result), "Should NOT reveal secret details ($key): " . var_export($result, 1)); + ->setDebug(TRUE) + ->execute(); + $this->assertContains('api_key', $result->debug['undefined_fields']); + $this->assertArrayNotHasKey('api_key', $result[0]); + $this->assertTrue($isSafe($result[0]), "Should NOT reveal secret details ($key): " . var_export($result[0], 1)); // Also not available via join $email = Email::get() ->addSelect('contact.api_key') ->addWhere('id', '=', $contact['email']['id']) - ->execute()->first(); - $this->assertTrue(empty($email['contact.api_key'])); - $this->assertTrue($isSafe($email), "Should NOT reveal secret details ($key): " . var_export($email, 1)); + ->setDebug(TRUE) + ->execute(); + $this->assertContains('contact.api_key', $email->debug['undefined_fields']); + $this->assertArrayNotHasKey('contact.api_key', $email[0]); + $this->assertTrue($isSafe($email[0]), "Should NOT reveal secret details ($key): " . var_export($email[0], 1)); $result = Contact::get() ->addWhere('id', '=', $contact['id']) ->execute() ->first(); - $this->assertTrue(empty($result['api_key'])); + $this->assertArrayNotHasKey('api_key', $result); $this->assertTrue($isSafe($result), "Should NOT reveal secret details ($key): " . var_export($result, 1)); } diff --git a/tests/phpunit/api/v4/Spec/SpecFormatterTest.php b/tests/phpunit/api/v4/Spec/SpecFormatterTest.php index 8f6fb40b4f..4f3c34a379 100644 --- a/tests/phpunit/api/v4/Spec/SpecFormatterTest.php +++ b/tests/phpunit/api/v4/Spec/SpecFormatterTest.php @@ -68,6 +68,7 @@ class SpecFormatterTest extends UnitTestCase { 'name' => $name, 'data_type' => 'String', 'html_type' => 'Multi-Select', + 'column_name' => $name, ]; /** @var \Civi\Api4\Service\Spec\CustomFieldSpec $field */ -- 2.25.1