From 66465aaa09ec6ae8705c0af4dd80db1cdd61f128 Mon Sep 17 00:00:00 2001 From: colemanw Date: Tue, 19 Sep 2023 12:07:48 -0400 Subject: [PATCH] APIv4 - Add dfk metadata to getfields (mostly) fixes some bad guesswork in the APIv4 conformance test, and now the dfk_entities metadata is available in getFields --- CRM/Core/Reference/Dynamic.php | 2 +- Civi/Api4/Generic/DAOGetFieldsAction.php | 5 ++ Civi/Api4/Service/Spec/SpecFormatter.php | 13 ++++-- Civi/Api4/Service/Spec/SpecGatherer.php | 46 ++++++++++--------- Civi/Schema/Traits/DataTypeSpecTrait.php | 22 +++++++++ Civi/Test/Api4TestTrait.php | 21 ++++----- .../FinancialItemCreationSpecProvider.php | 9 +--- tests/phpunit/api/v4/Action/GetFieldsTest.php | 5 ++ 8 files changed, 76 insertions(+), 47 deletions(-) diff --git a/CRM/Core/Reference/Dynamic.php b/CRM/Core/Reference/Dynamic.php index b806a89398..dd192d58c6 100644 --- a/CRM/Core/Reference/Dynamic.php +++ b/CRM/Core/Reference/Dynamic.php @@ -24,7 +24,7 @@ class CRM_Core_Reference_Dynamic extends CRM_Core_Reference_Basic { public function getTargetEntities(): array { $targetEntities = []; $bao = CRM_Core_DAO_AllCoreTables::getClassForTable($this->refTable); - $targetTables = (array) $bao::buildOptions($this->refTypeColumn); + $targetTables = $bao::buildOptions($this->refTypeColumn) ?: []; foreach ($targetTables as $table => $label) { $targetEntities[$table] = CRM_Core_DAO_AllCoreTables::getEntityNameForTable($table); } diff --git a/Civi/Api4/Generic/DAOGetFieldsAction.php b/Civi/Api4/Generic/DAOGetFieldsAction.php index af58d1fc06..a3e2072d4c 100644 --- a/Civi/Api4/Generic/DAOGetFieldsAction.php +++ b/Civi/Api4/Generic/DAOGetFieldsAction.php @@ -122,6 +122,11 @@ class DAOGetFieldsAction extends BasicGetFieldsAction { public function fields() { $fields = parent::fields(); + $fields[] = [ + 'name' => 'dfk_entities', + 'description' => 'List of possible entity types this field could be referencing.', + 'data_type' => 'Array', + ]; $fields[] = [ 'name' => 'help_pre', 'data_type' => 'String', diff --git a/Civi/Api4/Service/Spec/SpecFormatter.php b/Civi/Api4/Service/Spec/SpecFormatter.php index 1b21b781e2..d2c699192a 100644 --- a/Civi/Api4/Service/Spec/SpecFormatter.php +++ b/Civi/Api4/Service/Spec/SpecFormatter.php @@ -19,18 +19,18 @@ class SpecFormatter { /** * @param array $data - * @param string $entity + * @param string $entityName * * @return FieldSpec */ - public static function arrayToField(array $data, $entity) { + public static function arrayToField(array $data, string $entityName): FieldSpec { $dataTypeName = self::getDataType($data); $hasDefault = isset($data['default']) && $data['default'] !== ''; // Custom field if (!empty($data['custom_group_id'])) { - $field = new CustomFieldSpec($data['name'], $entity, $dataTypeName); - if (strpos($entity, 'Custom_') !== 0) { + $field = new CustomFieldSpec($data['name'], $entityName, $dataTypeName); + if (strpos($entityName, 'Custom_') !== 0) { $field->setName($data['custom_group_id.name'] . '.' . $data['name']); } else { @@ -62,7 +62,7 @@ class SpecFormatter { // Core field else { $name = $data['name'] ?? NULL; - $field = new FieldSpec($name, $entity, $dataTypeName); + $field = new FieldSpec($name, $entityName, $dataTypeName); $field->setType('Field'); $field->setColumnName($name); $field->setNullable(empty($data['required'])); @@ -70,6 +70,9 @@ class SpecFormatter { $field->setTitle($data['title'] ?? NULL); $field->setLabel($data['html']['label'] ?? NULL); $field->setLocalizable($data['localizable'] ?? FALSE); + if (!empty($data['DFKEntities'])) { + $field->setDfkEntities(array_values($data['DFKEntities'])); + } if (!empty($data['pseudoconstant'])) { // Do not load options if 'prefetch' is disabled if (($data['pseudoconstant']['prefetch'] ?? NULL) !== 'disabled') { diff --git a/Civi/Api4/Service/Spec/SpecGatherer.php b/Civi/Api4/Service/Spec/SpecGatherer.php index 80ce71555a..56fa359ef1 100644 --- a/Civi/Api4/Service/Spec/SpecGatherer.php +++ b/Civi/Api4/Service/Spec/SpecGatherer.php @@ -47,7 +47,7 @@ class SpecGatherer extends AutoService { $specification = new RequestSpec($entity, $action, $values); // Real entities - if (strpos($entity, 'Custom_') !== 0) { + if (!str_starts_with($entity, 'Custom_')) { $this->addDAOFields($entity, $action, $specification, $values); if ($includeCustom) { $this->addCustomFields($entity, $specification, $checkPermissions); @@ -77,18 +77,18 @@ class SpecGatherer extends AutoService { /** * @param \Civi\Api4\Service\Spec\Provider\Generic\SpecProviderInterface $provider */ - public function addSpecProvider(SpecProviderInterface $provider) { + public function addSpecProvider(SpecProviderInterface $provider): void { $this->specProviders[] = $provider; } /** - * @param string $entity + * @param string $entityName * @param string $action * @param \Civi\Api4\Service\Spec\RequestSpec $spec * @param array $values */ - private function addDAOFields($entity, $action, RequestSpec $spec, array $values) { - $DAOFields = $this->getDAOFields($entity); + private function addDAOFields(string $entityName, string $action, RequestSpec $spec, array $values) { + $DAOFields = $this->getDAOFields($entityName); foreach ($DAOFields as $DAOField) { if (array_key_exists('contactType', $DAOField) && $spec->getValue('contact_type') && $DAOField['contactType'] != $spec->getValue('contact_type')) { @@ -100,32 +100,36 @@ class SpecGatherer extends AutoService { if ($DAOField['name'] == 'is_active' && empty($DAOField['default'])) { $DAOField['default'] = '1'; } - $this->setDynamicFk($DAOField, $entity, $values); - $field = SpecFormatter::arrayToField($DAOField, $entity); + $this->setDynamicFk($DAOField, $values); + $field = SpecFormatter::arrayToField($DAOField, $entityName); $spec->addFieldSpec($field); } } /** - * Cleverly enables getFields to report dynamic FKs if a value is supplied for the entity type. + * Adds metadata about dynamic foreign key fields. + * + * E.g. some tables have a DFK with a pair of columns named `entity_table` and `entity_id`. + * This will gather the list of 'dfk_entities' to add as metadata to the e.g. `entity_id` column. * - * E.g. many tables have a DFK with a pair of `entity_table` and `entity_id` columns. - * If you supply a value for `entity_table`, then getFields will output the correct `fk_entity` for the `entity_id` field. + * Additionally, if $values contains a value for e.g. `entity_table`, + * then getFields will also output the corresponding `fk_entity` for the `entity_id` field. * * @param array $DAOField - * @param string $entityName * @param array $values */ - private function setDynamicFk(array &$DAOField, string $entityName, array $values): void { - if (empty($field['FKClassName']) && $values) { - $bao = CoreUtil::getBAOFromApiName($entityName); - // Check all dynamic FKs for entity for a match with this field and a supplied value - foreach ($bao::getReferenceColumns() ?? [] as $reference) { - if ($reference instanceof \CRM_Core_Reference_Dynamic - && $reference->getReferenceKey() === $DAOField['name'] - && array_key_exists($reference->getTypeColumn(), $values) - ) { - $DAOField['FKClassName'] = \CRM_Core_DAO_AllCoreTables::getClassForTable($values[$reference->getTypeColumn()]); + private function setDynamicFk(array &$DAOField, array $values): void { + if (empty($DAOField['FKClassName']) && !empty($DAOField['bao']) && $DAOField['type'] == \CRM_Utils_Type::T_INT) { + // Check if this field is a key for a dynamic FK + foreach ($DAOField['bao']::getReferenceColumns() ?? [] as $reference) { + if ($reference instanceof \CRM_Core_Reference_Dynamic && $reference->getReferenceKey() === $DAOField['name']) { + $entityTableColumn = $reference->getTypeColumn(); + $DAOField['DFKEntities'] = $reference->getTargetEntities(); + $DAOField['html']['controlField'] = $entityTableColumn; + // If we have a value for entity_table then this field can pretend to be a single FK too. + if (array_key_exists($entityTableColumn, $values)) { + $DAOField['FKClassName'] = \CRM_Core_DAO_AllCoreTables::getClassForTable($values[$entityTableColumn]); + } break; } } diff --git a/Civi/Schema/Traits/DataTypeSpecTrait.php b/Civi/Schema/Traits/DataTypeSpecTrait.php index 06b38dd82b..1c4b9bef1c 100644 --- a/Civi/Schema/Traits/DataTypeSpecTrait.php +++ b/Civi/Schema/Traits/DataTypeSpecTrait.php @@ -41,6 +41,11 @@ trait DataTypeSpecTrait { */ public $fkEntity; + /** + * @var string + */ + public $dfkEntities; + /** * Aliases for the valid data types * @@ -97,6 +102,23 @@ trait DataTypeSpecTrait { return $this; } + /** + * @return string + */ + public function getDfkEntities() { + return $this->dfkEntities; + } + + /** + * @param string $dfkEntities + * + * @return $this + */ + public function setDfkEntities($dfkEntities) { + $this->dfkEntities = $dfkEntities; + return $this; + } + /** * @return int */ diff --git a/Civi/Test/Api4TestTrait.php b/Civi/Test/Api4TestTrait.php index 0bfe6bc9d1..1165723a4c 100644 --- a/Civi/Test/Api4TestTrait.php +++ b/Civi/Test/Api4TestTrait.php @@ -133,7 +133,7 @@ trait Api4TestTrait { !isset($values[$fieldName]) && ($field['required'] || AbstractAction::evaluateCondition($field['required_if'], $values + $extraValues)) ) { - $extraValues[$fieldName] = $this->getRequiredValue($field); + $extraValues[$fieldName] = $this->getRequiredValue($field, $requiredFields); } } @@ -233,22 +233,19 @@ trait Api4TestTrait { if (!empty($field['fk_entity'])) { return $this->getFkID($field['fk_entity']); } + if (!empty($field['dfk_entities'])) { + return $this->getFkID($field['dfk_entities'][0]); + } if (isset($field['default_value'])) { return $field['default_value']; } - if ($field['name'] === 'contact_id') { + if ($field['name'] === 'contact_id' || $field['name'] === 'entity_id') { + // Obviously an FK field, but if we get here it's missing FK metadata :( + // FIXME: This is what we SHOULD do here... + // throw new \CRM_Core_Exception($field['name'] . ' should have foreign key information defined.'); + // ... instead this is how it was done, so we're stuck with it until FK metadata for every field gets fixed return $this->getFkID('Contact'); } - if ($field['name'] === 'entity_id') { - // What could possibly go wrong with this? - switch ($field['table_name'] ?? NULL) { - case 'civicrm_financial_item': - return $this->getFkID(FinancialItemCreationSpecProvider::DEFAULT_ENTITY); - - default: - return $this->getFkID('Contact'); - } - } // If there are no options but the field is supposed to have them, we may need to // create a new option if (!empty($field['suffixes']) && !empty($field['table_name'])) { diff --git a/ext/civi_contribute/Civi/Api4/Service/Spec/Provider/FinancialItemCreationSpecProvider.php b/ext/civi_contribute/Civi/Api4/Service/Spec/Provider/FinancialItemCreationSpecProvider.php index a29d133f2f..9a266b9622 100644 --- a/ext/civi_contribute/Civi/Api4/Service/Spec/Provider/FinancialItemCreationSpecProvider.php +++ b/ext/civi_contribute/Civi/Api4/Service/Spec/Provider/FinancialItemCreationSpecProvider.php @@ -20,20 +20,13 @@ use Civi\Api4\Service\Spec\RequestSpec; */ class FinancialItemCreationSpecProvider extends \Civi\Core\Service\AutoService implements Generic\SpecProviderInterface { - // I'm not sure it makes sense to have a default `entity_table`... actually, I don't even know if it makes - // sense to expose `FinancialItem` as a public API, for what that's worth. But it's there, so clearly it does. - // And the ConformanceTests require that you be able to create (and read-back) a record using metadata. - - const DEFAULT_TABLE = 'civicrm_line_item'; - const DEFAULT_ENTITY = 'LineItem'; - /** * @param \Civi\Api4\Service\Spec\RequestSpec $spec */ public function modifySpec(RequestSpec $spec) { + // TODO: These fields ought to be required in the schema. $spec->getFieldByName('entity_table')->setRequired(TRUE); $spec->getFieldByName('entity_id')->setRequired(TRUE); - $spec->getFieldByName('entity_table')->setDefaultValue(self::DEFAULT_TABLE); } /** diff --git a/tests/phpunit/api/v4/Action/GetFieldsTest.php b/tests/phpunit/api/v4/Action/GetFieldsTest.php index f919dfec0b..7f08331e24 100644 --- a/tests/phpunit/api/v4/Action/GetFieldsTest.php +++ b/tests/phpunit/api/v4/Action/GetFieldsTest.php @@ -204,16 +204,21 @@ class GetFieldsTest extends Api4TestBase implements TransactionalInterface { $tagFields = EntityTag::getFields(FALSE) ->execute()->indexBy('name'); $this->assertEmpty($tagFields['entity_id']['fk_entity']); + $this->assertContains('Activity', $tagFields['entity_id']['dfk_entities']); + $this->assertEquals('entity_table', $tagFields['entity_id']['input_attrs']['control_field']); $tagFields = EntityTag::getFields(FALSE) ->addValue('entity_table', 'civicrm_activity') ->execute()->indexBy('name'); + // fk_entity should be specific to specified entity_table, but dfk_entities should still contain all values $this->assertEquals('Activity', $tagFields['entity_id']['fk_entity']); + $this->assertContains('Contact', $tagFields['entity_id']['dfk_entities']); $tagFields = EntityTag::getFields(FALSE) ->addValue('entity_table:name', 'Contact') ->execute()->indexBy('name'); $this->assertEquals('Contact', $tagFields['entity_id']['fk_entity']); + $this->assertContains('SavedSearch', $tagFields['entity_id']['dfk_entities']); } public function testFiltersAreReturned(): void { -- 2.25.1