diff --git a/modules/custom/dkan_data/dkan_data.services.yml b/modules/custom/dkan_data/dkan_data.services.yml index ea21b11cf..776ef3c01 100644 --- a/modules/custom/dkan_data/dkan_data.services.yml +++ b/modules/custom/dkan_data/dkan_data.services.yml @@ -10,6 +10,7 @@ services: - '@dkan_data.uuid5' - '@config.factory' - '@queue' + - '@logger.factory' dkan_data.config_overrider: class: \Drupal\dkan_data\ConfigurationOverrider tags: diff --git a/modules/custom/dkan_data/src/Service/Uuid5.php b/modules/custom/dkan_data/src/Service/Uuid5.php index 26cec18f3..2a5f57c10 100644 --- a/modules/custom/dkan_data/src/Service/Uuid5.php +++ b/modules/custom/dkan_data/src/Service/Uuid5.php @@ -35,4 +35,17 @@ public function generate($schema_id, $value) { return $uuid->toString(); } + /** + * Check if a string is a valid UUID. + * + * @param string $uuid + * The uuid being tested. + * + * @return bool + * TRUE if valid, FALSE otherwise. + */ + public function isValid(string $uuid) { + return Uuid::isValid($uuid); + } + } diff --git a/modules/custom/dkan_data/src/ValueReferencer.php b/modules/custom/dkan_data/src/ValueReferencer.php index cad265791..16cae18fa 100644 --- a/modules/custom/dkan_data/src/ValueReferencer.php +++ b/modules/custom/dkan_data/src/ValueReferencer.php @@ -8,6 +8,7 @@ use Drupal\Core\Config\ConfigFactoryInterface; use Drupal\dkan_data\Service\Uuid5; use Drupal\Core\Queue\QueueFactory; +use Drupal\Core\Logger\LoggerChannelFactory; use stdClass; /** @@ -66,23 +67,33 @@ class ValueReferencer { */ protected $queueService; + /** + * The logger factory service. + * + * @var \Drupal\Core\Logger\LoggerChannelFactory + */ + protected $loggerService; + /** * ValueReferencer constructor. * * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entityTypeManager * Injected entity type manager. - * @param Drupal\dkan_data\Service\Uuid5 $uuidService + * @param \Drupal\dkan_data\Service\Uuid5 $uuidService * Injected uuid service. * @param \Drupal\Core\Config\ConfigFactoryInterface $configService * Injected config service. * @param \Drupal\Core\Queue\QueueFactory $queueService * Injected queue service. + * @param \Drupal\Core\Logger\LoggerChannelFactory $loggerService + * Injected logger factory service. */ - public function __construct(EntityTypeManagerInterface $entityTypeManager, Uuid5 $uuidService, ConfigFactoryInterface $configService, QueueFactory $queueService) { + public function __construct(EntityTypeManagerInterface $entityTypeManager, Uuid5 $uuidService, ConfigFactoryInterface $configService, QueueFactory $queueService, LoggerChannelFactory $loggerService) { $this->entityTypeManager = $entityTypeManager; $this->uuidService = $uuidService; $this->configService = $configService; $this->queueService = $queueService; + $this->loggerService = $loggerService; } /** @@ -120,7 +131,7 @@ protected function referenceProperty(string $property_id, $data) { return $this->referenceMultiple($property_id, $data); } else { - // Object or string. + // Case for $data being an object or a string. return $this->referenceSingle($property_id, $data); } } @@ -139,7 +150,10 @@ protected function referenceProperty(string $property_id, $data) { protected function referenceMultiple(string $property_id, array $values) : array { $result = []; foreach ($values as $value) { - $result[] = $this->referenceSingle($property_id, $value); + $data = $this->referenceSingle($property_id, $value); + if (NULL !== $data) { + $result[] = $data; + } } return $result; } @@ -164,9 +178,14 @@ protected function referenceSingle(string $property_id, $value) { return $uuid; } else { - // In the unlikely case we neither found an existing reference nor could - // create a new reference, return the unchanged value. - return $value; + $this->loggerService->get('value_referencer')->error( + 'Neither found an existing nor could create a new reference for property_id: @property_id with value: @value', + [ + '@property_id' => $property_id, + '@value' => var_export($value, TRUE), + ] + ); + return NULL; } } @@ -175,21 +194,22 @@ protected function referenceSingle(string $property_id, $value) { * * @param string $property_id * The dataset property id. - * @param string $data + * @param string|object $data * The property's value used to find an existing reference. * * @return string|null * The existing reference's uuid, or null if not found. + * + * @throws \Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException + * @throws \Drupal\Component\Plugin\Exception\PluginNotFoundException */ protected function checkExistingReference(string $property_id, $data) { $nodes = $this->entityTypeManager ->getStorage('node') - ->loadByProperties( - [ - 'field_data_type' => $property_id, - 'title' => md5(json_encode($data)), - ] - ); + ->loadByProperties([ + 'field_data_type' => $property_id, + 'title' => md5(json_encode($data)), + ]); if ($node = reset($nodes)) { return $node->uuid(); @@ -202,11 +222,15 @@ protected function checkExistingReference(string $property_id, $data) { * * @param string $property_id * The dataset property id. - * @param mixed $value + * @param string|object $value * The property's value. * * @return string|null * The new reference's uuid, or null. + * + * @throws \Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException + * @throws \Drupal\Component\Plugin\Exception\PluginNotFoundException + * @throws \Drupal\Core\Entity\EntityStorageException */ protected function createPropertyReference(string $property_id, $value) { // Create json metadata for the reference. @@ -217,15 +241,13 @@ protected function createPropertyReference(string $property_id, $value) { // Create node to store this reference. $node = $this->entityTypeManager ->getStorage('node') - ->create( - [ - 'title' => md5(json_encode($value)), - 'type' => 'data', - 'uuid' => $data->identifier, - 'field_data_type' => $property_id, - 'field_json_metadata' => json_encode($data), - ] - ); + ->create([ + 'title' => md5(json_encode($value)), + 'type' => 'data', + 'uuid' => $data->identifier, + 'field_data_type' => $property_id, + 'field_json_metadata' => json_encode($data), + ]); $node->save(); return $node->uuid(); @@ -272,27 +294,28 @@ public function dereference(stdClass $data, int $method = self::DEREFERENCE_OUTP * * @param string $property_id * The dataset property id. - * @param string|array $data - * A single reference uuid string, or an array reference uuids. + * @param string|array $uuid + * A single reference uuid string, or an array of reference uuids. * - * @return string|array - * An array of dereferenced values, or a single one. + * @return mixed + * An array of dereferenced values, a single one, or NULL. */ - protected function dereferenceProperty(string $property_id, $data) { - if (is_array($data)) { - return $this->dereferenceMultiple($property_id, $data); + protected function dereferenceProperty(string $property_id, $uuid) { + if (is_array($uuid)) { + return $this->dereferenceMultiple($property_id, $uuid); } - elseif (is_string($data)) { - return $this->dereferenceSingle($property_id, $data); + elseif (is_string($uuid) && $this->uuidService->isValid($uuid)) { + return $this->dereferenceSingle($property_id, $uuid); } else { - Drupal::logger('value_referencer')->error( - 'Unexpected data type when dereferencing property_id @property_id with data "@data"', + $this->loggerService->get('value_referencer')->error( + 'Unexpected data type when dereferencing property_id: @property_id with uuid: @uuid', [ - '@property_id' => property_id, - '@$data' => var_export($data, TRUE), + '@property_id' => $property_id, + '@uuid' => var_export($uuid, TRUE), ] ); + return NULL; } } @@ -310,7 +333,10 @@ protected function dereferenceProperty(string $property_id, $data) { protected function dereferenceMultiple(string $property_id, array $uuids) : array { $result = []; foreach ($uuids as $uuid) { - $result[] = $this->dereferenceSingle($property_id, $uuid); + $data = $this->dereferenceSingle($property_id, $uuid); + if (NULL !== $data) { + $result[] = $data; + } } return $result; } @@ -325,16 +351,17 @@ protected function dereferenceMultiple(string $property_id, array $uuids) : arra * * @return object|string * The data from this reference. + * + * @throws \Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException + * @throws \Drupal\Component\Plugin\Exception\PluginNotFoundException */ protected function dereferenceSingle(string $property_id, string $uuid) { $nodes = $this->entityTypeManager ->getStorage('node') - ->loadByProperties( - [ - 'field_data_type' => $property_id, - 'uuid' => $uuid, - ] - ); + ->loadByProperties([ + 'field_data_type' => $property_id, + 'uuid' => $uuid, + ]); if ($node = reset($nodes)) { if (isset($node->field_json_metadata->value)) { $metadata = json_decode($node->field_json_metadata->value); @@ -346,9 +373,16 @@ protected function dereferenceSingle(string $property_id, string $uuid) { } } } - // If str was not found, it's unlikely it was a uuid to begin with. It was - // most likely never referenced to begin with, so return unchanged. - return $uuid; + // If a property node was not found, it most likely means it was deleted + // while still being referenced. + $this->loggerService->get('value_referencer')->error( + 'Property @property_id reference @uuid not found', + [ + '@property_id' => $property_id, + '@uuid' => var_export($uuid, TRUE), + ] + ); + return NULL; } /** @@ -368,6 +402,11 @@ public function processReferencesInDeletedDataset(stdClass $data) { /** * Private. + * + * @param string $property_id + * The dataset property. + * @param array|string $uuids + * The uuids to process. */ protected function processReferencesInDeletedProperty($property_id, $uuids) { // Treat single uuid as an array of one uuid. @@ -382,20 +421,28 @@ protected function processReferencesInDeletedProperty($property_id, $uuids) { /** * Private. * + * @param string $property_id + * The dataset property. + * @param string $uuid + * The uuid to queue for removal. + * * @codeCoverageIgnore */ protected function queueReferenceForRemoval($property_id, $uuid) { $this->queueService->get('orphan_reference_processor') - ->createItem( - [ - $property_id, - $uuid, - ] - ); + ->createItem([ + $property_id, + $uuid, + ]); } /** * Public. + * + * @param object $old_dataset + * Old dataset. + * @param object $new_dataset + * Updated dataset. */ public function processReferencesInUpdatedDataset(stdClass $old_dataset, stdClass $new_dataset) { // Cycle through the dataset properties being referenced, check for orphans. @@ -414,6 +461,13 @@ public function processReferencesInUpdatedDataset(stdClass $old_dataset, stdClas /** * Private. + * + * @param string $property_id + * The dataset property. + * @param mixed $old_value + * The old value to be replaced. + * @param mixed $new_value + * The new value to replaced it with. */ protected function processReferencesInUpdatedProperty($property_id, $old_value, $new_value) { if (!is_array($old_value)) { @@ -427,6 +481,12 @@ protected function processReferencesInUpdatedProperty($property_id, $old_value, /** * Private. + * + * @param mixed $data + * Data whose type we want to match. + * + * @return array|string + * Either the empty string or an empty array. */ protected function emptyPropertyOfSameType($data) { if (is_array($data)) { diff --git a/modules/custom/dkan_data/tests/src/Unit/ValueReferencerTest.php b/modules/custom/dkan_data/tests/src/Unit/ValueReferencerTest.php index 230244f9f..67d12268a 100644 --- a/modules/custom/dkan_data/tests/src/Unit/ValueReferencerTest.php +++ b/modules/custom/dkan_data/tests/src/Unit/ValueReferencerTest.php @@ -12,6 +12,8 @@ use Drupal\Core\Entity\EntityStorageInterface; use Drupal\node\NodeInterface; use Drupal\Core\Entity\EntityInterface; +use Drupal\Core\Logger\LoggerChannelFactory; +use Drupal\Core\Logger\LoggerChannelInterface; use stdClass; /** @@ -36,22 +38,33 @@ public function testConstruct() { $mockUuidInterface = $this->createMock(Uuid5::class); $mockConfigInterface = $this->createMock(ConfigFactoryInterface::class); $mockQueueFactory = $this->createMock(QueueFactory::class); + $mockLoggerFactory = $this->createMock(LoggerChannelFactory::class); // Assert. - $mock->__construct($mockEntityTypeManager, $mockUuidInterface, $mockConfigInterface, $mockQueueFactory); + $mock->__construct($mockEntityTypeManager, $mockUuidInterface, $mockConfigInterface, $mockQueueFactory, $mockLoggerFactory); $this->assertSame($mockEntityTypeManager, $this->readAttribute($mock, 'entityTypeManager')); $this->assertSame($mockUuidInterface, $this->readAttribute($mock, 'uuidService')); $this->assertSame($mockConfigInterface, $this->readAttribute($mock, 'configService')); $this->assertSame($mockQueueFactory, $this->readAttribute($mock, 'queueService')); + $this->assertSame($mockLoggerFactory, $this->readAttribute($mock, 'loggerService')); } /** * Provides data for testing checkExistingReference function. */ public function dataTestCheckExistingReference() { - $mockNode = $this->createMock(NodeInterface::class); + $mockNode = $this->getMockBuilder(NodeInterface::class) + ->setMethods(['uuid']) + ->disableOriginalConstructor() + ->getMockForAbstractClass(); + $expected = uniqid('a-uuid'); + + $mockNode->expects($this->any()) + ->method('uuid') + ->willReturn($expected); + $mockNode->uuid = (object) ['value' => $expected]; return [ @@ -60,6 +73,52 @@ public function dataTestCheckExistingReference() { ]; } + /** + * Tests function checkExistingReference. + * + * @param string $property_id + * The property name. + * @param string $data + * The value of the property. + * @param array $nodes + * The expected $nodes array internally. + * @param string $expected + * The expected return value of referenceSingle. + * + * @dataProvider dataTestCheckExistingReference + */ + public function testCheckExistingReference(string $property_id, string $data, array $nodes, $expected) { + // Setup. + $mock = $this->getMockBuilder(ValueReferencer::class) + ->disableOriginalConstructor() + ->setMethods(NULL) + ->getMock(); + $mockEntityTypeManager = $this->getMockBuilder(EntityTypeManagerInterface::class) + ->setMethods(['getStorage']) + ->getMockForAbstractClass(); + $this->writeProtectedProperty($mock, 'entityTypeManager', $mockEntityTypeManager); + $mockNodeStorage = $this->getMockBuilder(EntityStorageInterface::class) + ->setMethods(['loadByProperties']) + ->getMockForAbstractClass(); + + // Expect. + $mockEntityTypeManager->expects($this->any()) + ->method('getStorage') + ->with('node') + ->willReturn($mockNodeStorage); + $mockNodeStorage->expects($this->any()) + ->method('loadByProperties') + ->with([ + 'field_data_type' => $property_id, + 'title' => md5(json_encode($data)), + ]) + ->willReturn($nodes); + + // Assert. + $actual = $this->invokeProtectedMethod($mock, 'checkExistingReference', $property_id, $data, $nodes, $expected); + $this->assertEquals($expected, $actual); + } + /** * Tests the createPropertyReference function. */ @@ -72,43 +131,26 @@ public function testCreatePropertyReference() { $mockUuidInterface = $this->getMockBuilder(Uuid5::class) ->disableOriginalConstructor() - ->setMethods( - [ - 'generate', - ] - ) + ->setMethods(['generate']) ->getMockForAbstractClass(); $this->writeProtectedProperty($mock, 'uuidService', $mockUuidInterface); $mockEntityTypeManager = $this->getMockBuilder(EntityTypeManagerInterface::class) - ->setMethods( - [ - 'getStorage', - ] - ) + ->setMethods(['getStorage']) ->getMockForAbstractClass(); $this->writeProtectedProperty($mock, 'entityTypeManager', $mockEntityTypeManager); $mockNodeStorage = $this->getMockBuilder(EntityStorageInterface::class) - ->setMethods( - [ - 'create', - ] - ) + ->setMethods(['create']) ->getMockForAbstractClass(); $mockEntityInterface = $this->getMockBuilder(EntityInterface::class) - ->setMethods( - [ - 'save', - 'uuid', - ] - ) + ->setMethods(['save', 'uuid']) ->getMockForAbstractClass(); $property_id = uniqid('some-property-'); $value = uniqid('some-value-'); - $uuid = Uuid5::generate($property_id, $value); + $uuid = uniqid('some-uuid-'); $data = new stdClass(); $data->identifier = $uuid; $data->data = $value; @@ -123,15 +165,13 @@ public function testCreatePropertyReference() { ->willReturn($mockNodeStorage); $mockNodeStorage->expects($this->once()) ->method('create') - ->with( - [ - 'title' => md5(json_encode($value)), - 'type' => 'data', - 'uuid' => $uuid, - 'field_data_type' => $property_id, - 'field_json_metadata' => json_encode($data), - ] - ) + ->with([ + 'title' => md5(json_encode($value)), + 'type' => 'data', + 'uuid' => $uuid, + 'field_data_type' => $property_id, + 'field_json_metadata' => json_encode($data), + ]) ->willReturn($mockEntityInterface); $mockEntityInterface->expects($this->once()) ->method('save') @@ -161,7 +201,7 @@ public function dataTestReferenceSingle() { $property_id, $value, NULL, $uuid, $uuid, ], 'neither found existing nor created new reference' => [ - $property_id, $value, NULL, NULL, $value, + $property_id, $value, NULL, NULL, NULL, ], ]; } @@ -188,6 +228,11 @@ public function testReferenceSingle(string $property_id, $value, $checkExisting, ->disableOriginalConstructor() ->setMethods(['checkExistingReference', 'createPropertyReference']) ->getMock(); + $mockLoggerFactory = $this->getMockBuilder(LoggerChannelFactory::class) + ->disableOriginalConstructor() + ->setMethods(['get']) + ->getMockForAbstractClass(); + $this->writeProtectedProperty($mock, 'loggerService', $mockLoggerFactory); // Expect. $mock->expects($this->exactly(1)) @@ -198,6 +243,18 @@ public function testReferenceSingle(string $property_id, $value, $checkExisting, ->method('createPropertyReference') ->with($property_id, $value) ->willReturn($createProperty); + $mockLoggerChannelInterface = $this->getMockBuilder(LoggerChannelInterface::class) + ->disableOriginalConstructor() + ->setMethods(['error']) + ->getMockForAbstractClass(); + $mockLoggerChannelInterface->expects($this->any()) + ->method('error') + ->withAnyParameters() + ->willReturn(""); + $mockLoggerFactory->expects($this->any()) + ->method('get') + ->withAnyParameters() + ->willReturn($mockLoggerChannelInterface); // Assert. $actual = $this->invokeProtectedMethod($mock, 'referenceSingle', $property_id, $value); @@ -429,14 +486,24 @@ public function dataTestDereferenceProperty() { $uuid1, NULL, $value1_retrieved, + TRUE, $value1_retrieved, ], 'dereferencing an array of uuid' => [ $property_id, - [$uuid1, $uuid2], - [$value1_retrieved, $value2_retrieved], + [$uuid1, $uuid2], + [$value1_retrieved, $value2_retrieved], + NULL, + TRUE, + [$value1_retrieved, $value2_retrieved], + ], + 'uuid is not a valid string' => [ + $property_id, + "invalid-uuid", + NULL, + NULL, + FALSE, NULL, - [$value1_retrieved, $value2_retrieved], ], ]; } @@ -452,17 +519,29 @@ public function dataTestDereferenceProperty() { * The expected value of dereferenceMultiple. * @param string $deRefSingle * The expected value of dereferenceSingle. + * @param bool $uuidIsValid + * Whether a uuid string is valid or not. * @param string|array $expected * The expected return value of dereferenceProperty. * * @dataProvider dataTestDereferenceProperty */ - public function testDereferenceProperty(string $property_id, $uuids, $deRefMultiple, $deRefSingle, $expected) { + public function testDereferenceProperty(string $property_id, $uuids, $deRefMultiple, $deRefSingle, $uuidIsValid, $expected) { // Setup. $mock = $this->getMockBuilder(ValueReferencer::class) ->disableOriginalConstructor() ->setMethods(['dereferenceMultiple', 'dereferenceSingle']) ->getMock(); + $mockUuidInterface = $this->getMockBuilder(Uuid5::class) + ->disableOriginalConstructor() + ->setMethods(['isValid']) + ->getMockForAbstractClass(); + $this->writeProtectedProperty($mock, 'uuidService', $mockUuidInterface); + $mockLoggerFactory = $this->getMockBuilder(LoggerChannelFactory::class) + ->disableOriginalConstructor() + ->setMethods(['get']) + ->getMockForAbstractClass(); + $this->writeProtectedProperty($mock, 'loggerService', $mockLoggerFactory); // Expect. $mock->expects($this->any()) @@ -473,6 +552,22 @@ public function testDereferenceProperty(string $property_id, $uuids, $deRefMulti ->method('dereferenceSingle') ->with($property_id, $uuids) ->willReturn($deRefSingle); + $mockUuidInterface->expects($this->any()) + ->method('isValid') + ->with($uuids) + ->willReturn($uuidIsValid); + $mockLoggerChannelInterface = $this->getMockBuilder(LoggerChannelInterface::class) + ->disableOriginalConstructor() + ->setMethods(['error']) + ->getMockForAbstractClass(); + $mockLoggerChannelInterface->expects($this->any()) + ->method('error') + ->withAnyParameters() + ->willReturn(""); + $mockLoggerFactory->expects($this->any()) + ->method('get') + ->withAnyParameters() + ->willReturn($mockLoggerChannelInterface); // Assert. $actual = $this->invokeProtectedMethod($mock, 'dereferenceProperty', $property_id, $uuids); @@ -521,18 +616,36 @@ public function testDereferenceMultiple() { } /** - * Provides data for testing checkExistingReference function. + * Provides data for testing testDereferenceSingle function. */ - public function datatestDereferenceSingle() { + public function dataTestDereferenceSingle() { $mockNode = $this->createMock(NodeInterface::class); $uuid = uniqid('some-property-uuid-'); $expected = "Some Property Value"; $mockNode->field_json_metadata = (object) ['value' => '{"uuid": "' . $uuid . '", "data": "Some Property Value"}']; return [ - ['someProperty', $uuid, [$mockNode], 1, $expected], - ['someProperty', $uuid, [$mockNode], 2, (object) ['uuid' => $uuid, 'data' => $expected]], - ['someProperty', $uuid, [], 0, $uuid], + [ + 'someProperty', + $uuid, + [$mockNode], + 1, + $expected, + ], + [ + 'someProperty', + $uuid, + [$mockNode], + 2, + (object) ['uuid' => $uuid, 'data' => $expected], + ], + [ + 'someProperty', + $uuid, + [], + 0, + NULL, + ], ]; } @@ -573,6 +686,11 @@ public function testDereferenceSingle(string $property_id, string $uuid, array $ ] ) ->getMockForAbstractClass(); + $mockLoggerFactory = $this->getMockBuilder(LoggerChannelFactory::class) + ->disableOriginalConstructor() + ->setMethods(['get']) + ->getMockForAbstractClass(); + $this->writeProtectedProperty($mock, 'loggerService', $mockLoggerFactory); // Expect. $mockEntityTypeManager->expects($this->once()) @@ -589,6 +707,18 @@ public function testDereferenceSingle(string $property_id, string $uuid, array $ ) ->willReturn($nodes); $this->writeProtectedProperty($mock, 'dereferenceMethod', $dereferenceMethod); + $mockLoggerChannelInterface = $this->getMockBuilder(LoggerChannelInterface::class) + ->disableOriginalConstructor() + ->setMethods(['error']) + ->getMockForAbstractClass(); + $mockLoggerChannelInterface->expects($this->any()) + ->method('error') + ->withAnyParameters() + ->willReturn(""); + $mockLoggerFactory->expects($this->any()) + ->method('get') + ->withAnyParameters() + ->willReturn($mockLoggerChannelInterface); // Assert. $actual = $this->invokeProtectedMethod($mock, 'dereferenceSingle', $property_id, $uuid);