From 2cfe873cb6daa44fff62b86acb9151617310089a Mon Sep 17 00:00:00 2001 From: Coleman Watts Date: Sun, 24 Jan 2016 17:12:48 -0500 Subject: [PATCH] CRM-17645 - Tests and better joins for selectWhereClause --- Civi/API/SelectQuery.php | 62 ++++++++++------- tests/phpunit/api/v3/SelectQueryTest.php | 84 ++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 22 deletions(-) create mode 100644 tests/phpunit/api/v3/SelectQueryTest.php diff --git a/Civi/API/SelectQuery.php b/Civi/API/SelectQuery.php index 728f9377a2..a31feff2a5 100644 --- a/Civi/API/SelectQuery.php +++ b/Civi/API/SelectQuery.php @@ -66,6 +66,10 @@ class SelectQuery { * @var \CRM_Utils_SQL_Select */ protected $query; + /** + * @var array + */ + private $joins = array(); /** * @var array */ @@ -136,7 +140,7 @@ class SelectQuery { $select_fields[self::MAIN_TABLE_ALIAS . ".{$field['name']}"] = $field['name']; } elseif ($include && strpos($field_name, '.')) { - $fkField = $this->addFkField($field_name); + $fkField = $this->addFkField($field_name, 'LEFT'); if ($fkField) { $select_fields[implode('.', $fkField)] = $field_name; } @@ -154,7 +158,7 @@ class SelectQuery { // This is a tested format so we support it. !empty($this->options['return']['custom']) ) { - list($table_name, $column_name) = $this->addCustomField($custom_field); + list($table_name, $column_name) = $this->addCustomField($custom_field, 'LEFT'); if ($custom_field["data_type"] != "ContactReference") { // 'ordinary' custom field. We will select the value as custom_XX. @@ -227,10 +231,10 @@ class SelectQuery { $column_name = $key; } elseif (($cf_id = \CRM_Core_BAO_CustomField::getKeyID($key)) != FALSE) { - list($table_name, $column_name) = $this->addCustomField($custom_fields[$cf_id]); + list($table_name, $column_name) = $this->addCustomField($custom_fields[$cf_id], 'INNER'); } elseif (strpos($key, '.')) { - $fkInfo = $this->addFkField($key); + $fkInfo = $this->addFkField($key, 'INNER'); if ($fkInfo) { list($table_name, $column_name) = $fkInfo; $this->validateNestedInput($key, $value); @@ -329,12 +333,14 @@ class SelectQuery { * Enforces permissions at the api level and by appending the acl clause for that entity to the join. * * @param $fkFieldName + * @param $side + * * @return array|null * Returns the table and field name for adding this field to a SELECT or WHERE clause * @throws \API_Exception * @throws \Civi\API\Exception\UnauthorizedException */ - private function addFkField($fkFieldName) { + private function addFkField($fkFieldName, $side) { $stack = explode('.', $fkFieldName); if (count($stack) < 2) { return NULL; @@ -356,7 +362,7 @@ class SelectQuery { if ($depth > self::MAX_JOINS) { throw new UnauthorizedException("Maximum number of joins exceeded in parameter $fkFieldName"); } - if (!isset($fkField['FKApiName']) && !isset($fkField['FKClassName'])) { + if (!isset($fkField['FKApiName']) || !isset($fkField['FKClassName'])) { // Join doesn't exist - might be another param with a dot in it for some reason, we'll just ignore it. return NULL; } @@ -377,18 +383,17 @@ class SelectQuery { } $fkTable = \CRM_Core_DAO_AllCoreTables::getTableForClass($fkField['FKClassName']); $tableAlias = implode('_to_', $subStack) . "_to_$fkTable"; - $joinClause = "LEFT JOIN $fkTable $tableAlias ON $prev.$fk = $tableAlias.id"; // Add acl condition - $joinCondition = $this->getAclClause($tableAlias, \_civicrm_api3_get_BAO($fkField['FKApiName']), $subStack); - if ($joinCondition !== NULL) { - $joinClause .= " AND $joinCondition"; - } + $joinCondition = array_merge( + array("$prev.$fk = $tableAlias.id"), + $this->getAclClause($tableAlias, \_civicrm_api3_get_BAO($fkField['FKApiName']), $subStack) + ); - $this->query->join($tableAlias, $joinClause); + $this->join($side, $fkTable, $tableAlias, $joinCondition); if (strpos($fieldName, 'custom_') === 0) { - list($tableAlias, $fieldName) = $this->addCustomField($fieldInfo, $tableAlias); + list($tableAlias, $fieldName) = $this->addCustomField($fieldInfo, $side, $tableAlias); } // Get ready to recurse to the next level @@ -405,15 +410,16 @@ class SelectQuery { * Adds a join to the query to make this field available for use in a clause. * * @param array $customField + * @param string $side * @param string $baseTable * @return array * Returns the table and field name for adding this field to a SELECT or WHERE clause */ - private function addCustomField($customField, $baseTable = self::MAIN_TABLE_ALIAS) { + private function addCustomField($customField, $side, $baseTable = self::MAIN_TABLE_ALIAS) { $tableName = $customField["table_name"]; $columnName = $customField["column_name"]; $tableAlias = "{$baseTable}_to_$tableName"; - $this->query->join($tableAlias, "LEFT JOIN `$tableName` `$tableAlias` ON `$tableAlias`.entity_id = `$baseTable`.id"); + $this->join($side, $tableName, $tableAlias, array("`$tableAlias`.entity_id = `$baseTable`.id")); return array($tableAlias, $columnName); } @@ -506,26 +512,24 @@ class SelectQuery { * @param string $tableAlias * @param string $baoName * @param array $stack - * @return null|string + * @return array */ private function getAclClause($tableAlias, $baoName, $stack = array()) { if (!$this->checkPermissions) { - return NULL; + return array(); } // Prevent (most) redundant acl sub clauses if they have already been applied to the main entity. // FIXME: Currently this only works 1 level deep, but tracking through multiple joins would increase complexity // and just doing it for the first join takes care of most acl clause deduping. if (count($stack) === 1 && in_array($stack[0], $this->aclFields)) { - return NULL; + return array(); } $clauses = $baoName::getSelectWhereClause($tableAlias); if (!$stack) { // Track field clauses added to the main entity $this->aclFields = array_keys($clauses); } - - $clauses = array_filter($clauses); - return $clauses ? implode(' AND ', $clauses) : NULL; + return array_filter($clauses); } /** @@ -552,7 +556,7 @@ class SelectQuery { $orderBy[] = self::MAIN_TABLE_ALIAS . '.' . $field['name'] . $direction; } elseif (strpos($words[0], '.')) { - $join = $this->addFkField($words[0]); + $join = $this->addFkField($words[0], 'LEFT'); if ($join) { $orderBy[] = "`{$join[0]}`.`{$join[1]}`$direction"; } @@ -565,4 +569,18 @@ class SelectQuery { $this->query->orderBy($orderBy); } + /** + * @param string $side + * @param string $tableName + * @param string $tableAlias + * @param array $conditions + */ + public function join($side, $tableName, $tableAlias, $conditions) { + // INNER JOINs take precedence over LEFT JOINs + if ($side != 'LEFT' || !isset($this->joins[$tableAlias])) { + $this->joins[$tableAlias] = $side; + $this->query->join($tableAlias, "$side JOIN `$tableName` `$tableAlias` ON " . implode(' AND ', $conditions)); + } + } + } diff --git a/tests/phpunit/api/v3/SelectQueryTest.php b/tests/phpunit/api/v3/SelectQueryTest.php new file mode 100644 index 0000000000..16ada5e4d7 --- /dev/null +++ b/tests/phpunit/api/v3/SelectQueryTest.php @@ -0,0 +1,84 @@ +useTransaction(TRUE); + CRM_Utils_Hook::singleton()->setHook('civicrm_selectWhereClause', array($this, 'hook_civicrm_selectWhereClause')); + } + + public function testHookPhoneClause() { + $person1 = $this->callAPISuccess('Contact', 'create', array('contact_type' => 'Individual', 'first_name' => 'Bob', 'last_name' => 'Tester')); + $cid = $person1['id']; + for ($number = 1; $number < 6; ++$number) { + $this->callAPISuccess('Phone', 'create', array( + 'contact_id' => $cid, + 'phone' => $number, + )); + } + $this->hookEntity = 'Phone'; + $this->hookCondition = array( + 'phone' => array('= 3'), + ); + $phone = $this->callAPISuccessGetSingle('Phone', array('contact_id' => $cid, 'check_permissions' => 1)); + $this->assertEquals(3, $phone['phone']); + } + + public function testHookContactClause() { + $person1 = $this->callAPISuccess('Contact', 'create', array('contact_type' => 'Individual', 'first_name' => 'Bob', 'last_name' => 'Tester', 'email' => 'bob@test.er')); + $person2 = $this->callAPISuccess('Contact', 'create', array('contact_type' => 'Individual', 'first_name' => 'Tom', 'last_name' => 'Tester', 'email' => 'tom@test.er')); + $person3 = $this->callAPISuccess('Contact', 'create', array('contact_type' => 'Individual', 'first_name' => 'Tim', 'last_name' => 'Tester', 'email' => 'tim@test.er')); + $this->hookEntity = 'Contact'; + $this->hookCondition = array('id' => array('= ' . $person2['id'])); + $email = $this->callAPISuccessGetSingle('Email', array('check_permissions' => 1)); + $this->assertEquals($person2['id'], $email['contact_id']); + } + + /** + * Implements hook_civicrm_selectWhereClause(). + */ + public function hook_civicrm_selectWhereClause($entity, &$clauses) { + if ($entity == $this->hookEntity) { + foreach ($this->hookCondition as $field => $clause) { + $clauses[$field] = array_merge(CRM_Utils_Array::value($field, $clauses, array()), $clause); + } + } + } + +} -- 2.25.1