From 17019d499ea63a9281d16c8c83a828e1c1a0071c Mon Sep 17 00:00:00 2001 From: Coleman Watts Date: Thu, 10 Dec 2020 14:24:29 -0500 Subject: [PATCH] APIv4 - Fix dynamic bridge joins Move ON clause conditions to the first join when they reference the bridge entity itself Ensure bridge joins work with joining the same table to itself (e.g. Contact to Contact via RelationshipCache) --- CRM/Core/DAO/AllCoreTables.php | 11 +++ CRM/Core/Reference/Basic.php | 8 ++ CRM/Core/Reference/Dynamic.php | 14 +++ Civi/Api4/Query/Api4SelectQuery.php | 104 ++++++++++++--------- ext/search/Civi/Search/Admin.php | 18 +--- tests/phpunit/api/v4/Action/FkJoinTest.php | 86 +++++++++++++++++ 6 files changed, 184 insertions(+), 57 deletions(-) diff --git a/CRM/Core/DAO/AllCoreTables.php b/CRM/Core/DAO/AllCoreTables.php index 9e4970c301..0e4b38b848 100644 --- a/CRM/Core/DAO/AllCoreTables.php +++ b/CRM/Core/DAO/AllCoreTables.php @@ -336,6 +336,17 @@ class CRM_Core_DAO_AllCoreTables { return self::getTableForClass(self::getFullName($entityBriefName)); } + /** + * Convert table name to brief entity name. + * + * @param string $tableName + * + * @return FALSE|string + */ + public static function getEntityNameForTable(string $tableName) { + return self::getBriefName(self::getClassForTable($tableName)); + } + /** * Reinitialise cache. * diff --git a/CRM/Core/Reference/Basic.php b/CRM/Core/Reference/Basic.php index a3e1fe73f8..ae8684837b 100644 --- a/CRM/Core/Reference/Basic.php +++ b/CRM/Core/Reference/Basic.php @@ -71,6 +71,14 @@ class CRM_Core_Reference_Basic implements CRM_Core_Reference_Interface { return ($this->getTargetTable() === $tableName); } + /** + * @return array + * [table_name => EntityName] + */ + public function getTargetEntities(): array { + return [$this->targetTable => CRM_Core_DAO_AllCoreTables::getEntityNameForTable($this->targetTable)]; + } + /** * @param CRM_Core_DAO $targetDao * diff --git a/CRM/Core/Reference/Dynamic.php b/CRM/Core/Reference/Dynamic.php index 6c5ba398ca..c7224922ab 100644 --- a/CRM/Core/Reference/Dynamic.php +++ b/CRM/Core/Reference/Dynamic.php @@ -13,9 +13,23 @@ class CRM_Core_Reference_Dynamic extends CRM_Core_Reference_Basic { * @return bool */ public function matchesTargetTable($tableName) { + // FIXME: Shouldn't this check against keys returned by getTargetEntities? return TRUE; } + /** + * @return array + * [table_name => EntityName] + */ + public function getTargetEntities(): array { + $targetEntities = []; + $bao = CRM_Core_DAO_AllCoreTables::getClassForTable($this->refTable); + foreach ($bao::buildOptions($this->refTypeColumn) as $table => $label) { + $targetEntities[$table] = CRM_Core_DAO_AllCoreTables::getEntityNameForTable($table); + } + return $targetEntities; + } + /** * Create a query to find references to a particular record. * diff --git a/Civi/Api4/Query/Api4SelectQuery.php b/Civi/Api4/Query/Api4SelectQuery.php index 2d1a73e265..b8478c6ddc 100644 --- a/Civi/Api4/Query/Api4SelectQuery.php +++ b/Civi/Api4/Query/Api4SelectQuery.php @@ -594,7 +594,9 @@ class Api4SelectQuery { } /** - * Join onto a Bridge table + * Join via a Bridge table + * + * This creates a double-join in sql that appears to the API user like a single join. * * @param array $joinTree * @param string $joinEntity @@ -606,75 +608,93 @@ class Api4SelectQuery { $bridgeEntity = array_shift($joinTree); /* @var \Civi\Api4\Generic\DAOEntity $bridgeEntityClass */ $bridgeEntityClass = '\Civi\Api4\\' . $bridgeEntity; - if (!in_array('EntityBridge', $bridgeEntityClass::getInfo()['type'], TRUE)) { - throw new \API_Exception("Illegal bridge entity specified: " . $bridgeEntity); - } $bridgeAlias = $alias . '_via_' . strtolower($bridgeEntity); - $bridgeTable = CoreUtil::getTableName($bridgeEntity); + $bridgeInfo = $bridgeEntityClass::getInfo(); + $bridgeFields = $bridgeInfo['bridge'] ?? []; + // Sanity check - bridge entity should declare exactly 2 FK fields + if (count($bridgeFields) !== 2) { + throw new \API_Exception("Illegal bridge entity specified: $bridgeEntity. Expected 2 bridge fields, found " . count($bridgeFields)); + } + /* @var \CRM_Core_DAO $bridgeDAO */ + $bridgeDAO = $bridgeInfo['dao']; + $bridgeTable = $bridgeDAO::getTableName(); + $joinTable = CoreUtil::getTableName($joinEntity); $bridgeEntityGet = $bridgeEntityClass::get($this->getCheckPermissions()); - $fkToJoinField = $fkToBaseField = NULL; - // Find the bridge field that links to the joinEntity (either an explicit FK or an entity_id/entity_table combo) - foreach ($bridgeEntityGet->entityFields() as $name => $field) { - if ($field['fk_entity'] === $joinEntity || (!$fkToJoinField && $name === 'entity_id')) { - $fkToJoinField = $name; + // Get the 2 bridge reference columns as CRM_Core_Reference_* objects + $joinRef = $baseRef = NULL; + foreach ($bridgeDAO::getReferenceColumns() as $ref) { + if (in_array($ref->getReferenceKey(), $bridgeFields)) { + if (!$joinRef && in_array($joinEntity, $ref->getTargetEntities())) { + $joinRef = $ref; + } + else { + $baseRef = $ref; + } } } - // Get list of entities allowed for entity_table - if (array_key_exists('entity_id', $bridgeEntityGet->entityFields())) { - $entityTables = (array) civicrm_api4($bridgeEntity, 'getFields', [ - 'checkPermissions' => FALSE, - 'where' => [['name', '=', 'entity_table']], - 'loadOptions' => TRUE, - ], ['options'])->first(); - } - // If bridge field to joinEntity is entity_id, validate entity_table is allowed - if (!$fkToJoinField || ($fkToJoinField === 'entity_id' && !array_key_exists($joinTable, $entityTables))) { + if (!$joinRef || !$baseRef) { throw new \API_Exception("Unable to join $bridgeEntity to $joinEntity"); } // Create link between bridge entity and join entity $joinConditions = [ - "`$bridgeAlias`.`$fkToJoinField` = `$alias`.`id`", + "`$bridgeAlias`.`{$joinRef->getReferenceKey()}` = `$alias`.`{$joinRef->getTargetKey()}`", ]; - if ($fkToJoinField === 'entity_id') { - $joinConditions[] = "`$bridgeAlias`.`entity_table` = '$joinTable'"; + // For dynamic references, also add the type column (e.g. `entity_table`) + if ($joinRef->getTypeColumn()) { + $joinConditions[] = "`$bridgeAlias`.`{$joinRef->getTypeColumn()}` = '$joinTable'"; } - // Register fields from the bridge entity as if they belong to the join entity + // Register fields (other than bridge FK fields) from the bridge entity as if they belong to the join entity + $fakeFields = []; foreach ($bridgeEntityGet->entityFields() as $name => $field) { - if ($name == 'id' || $name == $fkToJoinField || ($name == 'entity_table' && $fkToJoinField == 'entity_id')) { + if ($name === 'id' || $name === $joinRef->getReferenceKey() || $name === $joinRef->getTypeColumn() || $name === $baseRef->getReferenceKey() || $name === $baseRef->getTypeColumn()) { continue; } - if ($field['fk_entity'] || (!$fkToBaseField && $name == 'entity_id')) { - $fkToBaseField = $name; - } // Note these fields get a sql alias pointing to the bridge entity, but an api alias pretending they belong to the join entity $field['sql_name'] = '`' . $bridgeAlias . '`.`' . $field['column_name'] . '`'; $this->addSpecField($alias . '.' . $field['name'], $field); + $fakeFields[] = $alias . '.' . $field['name']; } // Move conditions for the bridge join out of the joinTree $bridgeConditions = []; - $joinTree = array_filter($joinTree, function($clause) use ($fkToBaseField, $alias, $bridgeAlias, &$bridgeConditions) { + $isExplicit = FALSE; + $joinTree = array_filter($joinTree, function($clause) use ($baseRef, $alias, $bridgeAlias, $fakeFields, &$bridgeConditions, &$isExplicit) { list($sideA, $op, $sideB) = array_pad((array) $clause, 3, NULL); - if ($op === '=' && $sideB && ($sideA === "$alias.$fkToBaseField" || $sideB === "$alias.$fkToBaseField")) { - $expr = $sideA === "$alias.$fkToBaseField" ? $sideB : $sideA; - $bridgeConditions[] = "`$bridgeAlias`.`$fkToBaseField` = " . $this->getExpression($expr)->render($this->apiFieldSpec); + // Skip AND/OR/NOT branches + if (!$sideB) { + return TRUE; + } + // If this condition makes an explicit link between the bridge and another entity + if ($op === '=' && $sideB && ($sideA === "$alias.{$baseRef->getReferenceKey()}" || $sideB === "$alias.{$baseRef->getReferenceKey()}")) { + $expr = $sideA === "$alias.{$baseRef->getReferenceKey()}" ? $sideB : $sideA; + $bridgeConditions[] = "`$bridgeAlias`.`{$baseRef->getReferenceKey()}` = " . $this->getExpression($expr)->render($this->apiFieldSpec); + $isExplicit = TRUE; return FALSE; } - elseif ($op === '=' && $fkToBaseField == 'entity_id' && ($sideA === "$alias.entity_table" || $sideB === "$alias.entity_table")) { - $expr = $sideA === "$alias.entity_table" ? $sideB : $sideA; - $bridgeConditions[] = "`$bridgeAlias`.`entity_table` = " . $this->getExpression($expr)->render($this->apiFieldSpec); + // Explicit link with dynamic "entity_table" column + elseif ($op === '=' && $baseRef->getTypeColumn() && ($sideA === "$alias.{$baseRef->getTypeColumn()}" || $sideB === "$alias.{$baseRef->getTypeColumn()}")) { + $expr = $sideA === "$alias.{$baseRef->getTypeColumn()}" ? $sideB : $sideA; + $bridgeConditions[] = "`$bridgeAlias`.`{$baseRef->getTypeColumn()}` = " . $this->getExpression($expr)->render($this->apiFieldSpec); + $isExplicit = TRUE; return FALSE; } + // Other conditions that apply only to the bridge table should be + foreach ([$sideA, $sideB] as $expr) { + if (is_string($expr) && in_array(explode(':', $expr)[0], $fakeFields)) { + $bridgeConditions[] = $this->composeClause($clause, 'ON'); + return FALSE; + } + } return TRUE; }); // If no bridge conditions were specified, link it to the base entity - if (!$bridgeConditions) { - $bridgeConditions[] = "`$bridgeAlias`.`$fkToBaseField` = a.id"; - if ($fkToBaseField == 'entity_id') { - if (!array_key_exists($this->getFrom(), $entityTables)) { - throw new \API_Exception("Unable to join $bridgeEntity to " . $this->getEntity()); - } - $bridgeConditions[] = "`$bridgeAlias`.`entity_table` = '" . $this->getFrom() . "'"; + if (!$isExplicit) { + if (!in_array($this->getEntity(), $baseRef->getTargetEntities())) { + throw new \API_Exception("Unable to join $bridgeEntity to " . $this->getEntity()); + } + $bridgeConditions[] = "`$bridgeAlias`.`{$baseRef->getReferenceKey()}` = a.`{$baseRef->getTargetKey()}`"; + if ($baseRef->getTypeColumn()) { + $bridgeConditions[] = "`$bridgeAlias`.`{$baseRef->getTypeColumn()}` = '" . $this->getFrom() . "'"; } } diff --git a/ext/search/Civi/Search/Admin.php b/ext/search/Civi/Search/Admin.php index b223b321f0..e7abc0e0ff 100644 --- a/ext/search/Civi/Search/Admin.php +++ b/ext/search/Civi/Search/Admin.php @@ -142,25 +142,13 @@ class Admin { ) { continue; } - // Dynamic references use a column like "entity_table" - $dynamicCol = $reference->getTypeColumn(); - if ($dynamicCol) { - $targetTables = $daoClass::buildOptions($dynamicCol); - if (!$targetTables) { - continue; - } - $targetTables = array_keys($targetTables); - } - else { - $targetTables = [$reference->getTargetTable()]; - } - foreach ($targetTables as $targetTable) { - $targetDao = \CRM_Core_DAO_AllCoreTables::getClassForTable($targetTable); - $targetEntityName = \CRM_Core_DAO_AllCoreTables::getBriefName($targetDao); + foreach ($reference->getTargetEntities() as $targetTable => $targetEntityName) { if (!isset($allowedEntities[$targetEntityName]) || $targetEntityName === $entity['name']) { continue; } $targetEntity = $allowedEntities[$targetEntityName]; + // Dynamic references use a column like "entity_table" + $dynamicCol = $reference->getTypeColumn(); // Non-bridge joins directly between 2 entities if (!$bridge) { // Add the straight 1-1 join diff --git a/tests/phpunit/api/v4/Action/FkJoinTest.php b/tests/phpunit/api/v4/Action/FkJoinTest.php index 9f62f79c59..39db4a7ae6 100644 --- a/tests/phpunit/api/v4/Action/FkJoinTest.php +++ b/tests/phpunit/api/v4/Action/FkJoinTest.php @@ -25,6 +25,7 @@ use Civi\Api4\Contact; use Civi\Api4\Email; use Civi\Api4\EntityTag; use Civi\Api4\Phone; +use Civi\Api4\Relationship; use Civi\Api4\Tag; /** @@ -202,4 +203,89 @@ class FkJoinTest extends UnitTestCase { $this->assertEquals(1, (int) $reverse[$tag3]['contacts']); } + public function testBridgeJoinRelationshipContactActivity() { + $cid1 = Contact::create()->setCheckPermissions(FALSE) + ->addValue('first_name', 'Aaa') + ->addChain('activity', Activity::create() + ->addValue('activity_type_id:name', 'Meeting') + ->addValue('source_contact_id', '$id') + ->addValue('target_contact_id', '$id') + ) + ->execute() + ->first()['id']; + $cid2 = Contact::create()->setCheckPermissions(FALSE) + ->addValue('first_name', 'Bbb') + ->addChain('activity', Activity::create() + ->addValue('activity_type_id:name', 'Phone Call') + ->addValue('source_contact_id', $cid1) + ->addValue('target_contact_id', '$id') + ) + ->addChain('r1', Relationship::create() + ->setValues(['contact_id_a' => '$id', 'contact_id_b' => $cid1, 'relationship_type_id' => 1]) + ) + ->execute() + ->first()['id']; + $cid3 = Contact::create()->setCheckPermissions(FALSE) + ->addValue('first_name', 'Ccc') + ->addChain('activity', Activity::create() + ->addValue('activity_type_id:name', 'Meeting') + ->addValue('source_contact_id', $cid1) + ->addValue('target_contact_id', '$id') + ) + ->addChain('activity2', Activity::create() + ->addValue('activity_type_id:name', 'Phone Call') + ->addValue('source_contact_id', $cid1) + ->addValue('target_contact_id', '$id') + ) + ->addChain('r1', Relationship::create() + ->setValues(['contact_id_a' => '$id', 'contact_id_b' => $cid1, 'relationship_type_id' => 1]) + ) + ->addChain('r2', Relationship::create() + ->setValues(['contact_id_a' => '$id', 'contact_id_b' => $cid2, 'relationship_type_id' => 2]) + ) + ->execute() + ->first()['id']; + + $result = Contact::get(FALSE) + ->addSelect('id', 'act.id') + ->addJoin('Activity AS act', TRUE, 'ActivityContact', ['act.record_type_id:name', '=', "'Activity Targets'"]) + ->addWhere('id', 'IN', [$cid1, $cid2, $cid3]) + ->execute(); + $this->assertCount(4, $result); + + $result = Contact::get(FALSE) + ->addSelect('id', 'act.id') + ->addJoin('Activity AS act', TRUE, 'ActivityContact', ['act.activity_type_id:name', '=', "'Meeting'"], ['act.record_type_id:name', '=', "'Activity Targets'"]) + ->addWhere('id', 'IN', [$cid1, $cid2, $cid3]) + ->execute(); + $this->assertCount(2, $result); + + $result = Activity::get(FALSE) + ->addSelect('id', 'contact.id') + ->addJoin('Contact', FALSE, 'ActivityContact') + ->addWhere('contact.id', 'IN', [$cid1, $cid2, $cid3]) + ->execute(); + $this->assertCount(8, $result); + + $result = Activity::get(FALSE) + ->addSelect('id', 'contact.id', 'rel.id') + ->addJoin('Contact', FALSE, 'ActivityContact', ['contact.record_type_id:name', '=', "'Activity Targets'"]) + ->addJoin('Contact AS rel', FALSE, 'RelationshipCache', ['rel.far_contact_id', '=', 'contact.id'], ['rel.near_relation:name', '=', '"Child of"']) + ->addWhere('contact.id', 'IN', [$cid1, $cid2, $cid3]) + ->addOrderBy('id') + ->execute(); + $this->assertCount(5, $result); + $this->assertEquals($cid1, $result[0]['contact.id']); + $this->assertEquals($cid2, $result[0]['rel.id']); + $this->assertEquals($cid1, $result[1]['contact.id']); + $this->assertEquals($cid3, $result[1]['rel.id']); + $this->assertEquals($cid2, $result[2]['contact.id']); + $this->assertNull($result[2]['rel.id']); + $this->assertEquals($cid3, $result[3]['contact.id']); + $this->assertNull($result[3]['rel.id']); + $this->assertEquals($cid3, $result[4]['contact.id']); + $this->assertNull($result[3]['rel.id']); + + } + } -- 2.25.1