From 74c3da5365bfbbd272081b7fe055ac747a232981 Mon Sep 17 00:00:00 2001 From: Paul Mitchum Date: Sun, 1 Sep 2024 12:41:33 -0700 Subject: [PATCH 1/3] harvestutility catches exceptions --- modules/harvest/src/HarvestUtility.php | 26 ++++++-- .../tests/src/Kernel/HarvestUtilityTest.php | 65 +++++++++++++++++++ 2 files changed, 85 insertions(+), 6 deletions(-) diff --git a/modules/harvest/src/HarvestUtility.php b/modules/harvest/src/HarvestUtility.php index 54cee32996..342b106347 100644 --- a/modules/harvest/src/HarvestUtility.php +++ b/modules/harvest/src/HarvestUtility.php @@ -203,9 +203,16 @@ public function harvestHashUpdate() { ); foreach ($plan_ids as $plan_id) { $this->logger->notice('Converting hashes for ' . $plan_id); - $this->convertHashTable($plan_id); - $this->storeFactory->getInstance('harvest_' . $plan_id . '_hashes') - ->destruct(); + try { + $this->convertHashTable($plan_id); + $this->storeFactory->getInstance('harvest_' . $plan_id . '_hashes') + ->destruct(); + } + catch (\Exception $e) { + $this->logger->error('Unable to convert hashes for ' . $plan_id); + $this->logger->error($e->getMessage()); + $this->logger->debug($e->getTraceAsString()); + } } } @@ -237,9 +244,16 @@ public function harvestRunsUpdate() { ); foreach ($plan_ids as $plan_id) { $this->logger->notice('Converting runs for ' . $plan_id); - $this->convertRunTable($plan_id); - $this->storeFactory->getInstance('harvest_' . $plan_id . '_runs') - ->destruct(); + try { + $this->convertRunTable($plan_id); + $this->storeFactory->getInstance('harvest_' . $plan_id . '_runs') + ->destruct(); + } + catch (\Exception $e) { + $this->logger->error('Unable to convert runs for ' . $plan_id); + $this->logger->error($e->getMessage()); + $this->logger->debug($e->getTraceAsString()); + } } } diff --git a/modules/harvest/tests/src/Kernel/HarvestUtilityTest.php b/modules/harvest/tests/src/Kernel/HarvestUtilityTest.php index 9362eb49ff..b4ec887358 100644 --- a/modules/harvest/tests/src/Kernel/HarvestUtilityTest.php +++ b/modules/harvest/tests/src/Kernel/HarvestUtilityTest.php @@ -2,6 +2,7 @@ namespace Drupal\Tests\harvest\Kernel; +use Drupal\harvest\HarvestUtility; use Drupal\KernelTests\KernelTestBase; /** @@ -197,4 +198,68 @@ public function testHarvestRunsUpdate() { $this->assertEquals('AWESOME', $run_entity->get('extract_status')->getString()); } + /** + * Ensure an exception during DB handling does not stop further updates. + * + * @covers ::harvestRunsUpdate + */ + public function testHarvestRunException() { + // Mock HarvestUtility so we can have one method throw an exception. + $utility = $this->getMockBuilder(HarvestUtility::class) + ->setConstructorArgs([ + $this->container->get('dkan.harvest.service'), + $this->container->get('dkan.harvest.storage.database_table'), + $this->container->get('dkan.harvest.storage.hashes_database_table'), + $this->container->get('dkan.harvest.storage.harvest_run_repository'), + $this->container->get('database'), + $this->container->get('dkan.harvest.logger_channel'), + ]) + ->onlyMethods(['findOrphanedHarvestDataIds', 'convertRunTable']) + ->getMock(); + + // findOrphanedHarvestDataIds will return two fake IDs to loop over. + $utility->expects($this->atLeast(1)) + ->method('findOrphanedHarvestDataIds') + ->willReturn(['FAKE_PLAN_ID', 'ANOTHER_FAKE_PLAN_ID']); + // DB work will throw an exception. We expect it to be called twice because + // there are two plan IDs. + $utility->expects($this->atLeast(2)) + ->method('convertRunTable') + ->willThrowException(new \Exception('Hello.')); + + $utility->harvestRunsUpdate(); + } + + /** + * Ensure an exception during DB handling does not stop further updates. + * + * @covers ::harvestHashUpdate + */ + public function testHarvestHashException() { + // Mock HarvestUtility so we can mock some methods. + $utility = $this->getMockBuilder(HarvestUtility::class) + ->setConstructorArgs([ + $this->container->get('dkan.harvest.service'), + $this->container->get('dkan.harvest.storage.database_table'), + $this->container->get('dkan.harvest.storage.hashes_database_table'), + $this->container->get('dkan.harvest.storage.harvest_run_repository'), + $this->container->get('database'), + $this->container->get('dkan.harvest.logger_channel'), + ]) + ->onlyMethods(['findOrphanedHarvestDataIds', 'convertHashTable']) + ->getMock(); + + // findOrphanedHarvestDataIds will return two fake IDs to loop over. + $utility->expects($this->atLeast(1)) + ->method('findOrphanedHarvestDataIds') + ->willReturn(['FAKE_PLAN_ID', 'ANOTHER_FAKE_PLAN_ID']); + // DB work will throw an exception. We expect it to be called twice because + // there are two plan IDs. + $utility->expects($this->atLeast(2)) + ->method('convertHashTable') + ->willThrowException(new \Exception('Hello.')); + + $utility->harvestHashUpdate(); + } + } From fab0c97511109cd2315bc82182210aad4038f516 Mon Sep 17 00:00:00 2001 From: Paul Mitchum Date: Thu, 5 Sep 2024 15:37:15 -0700 Subject: [PATCH 2/3] refactor for cc --- modules/harvest/src/HarvestUtility.php | 46 ++++++++++++++------------ 1 file changed, 25 insertions(+), 21 deletions(-) diff --git a/modules/harvest/src/HarvestUtility.php b/modules/harvest/src/HarvestUtility.php index 342b106347..515574bd15 100644 --- a/modules/harvest/src/HarvestUtility.php +++ b/modules/harvest/src/HarvestUtility.php @@ -197,23 +197,7 @@ public function convertHashTable(string $plan_id) { * Outdated tables will be removed. */ public function harvestHashUpdate() { - $plan_ids = array_merge( - $this->harvestService->getAllHarvestIds(), - array_values($this->findOrphanedHarvestDataIds()) - ); - foreach ($plan_ids as $plan_id) { - $this->logger->notice('Converting hashes for ' . $plan_id); - try { - $this->convertHashTable($plan_id); - $this->storeFactory->getInstance('harvest_' . $plan_id . '_hashes') - ->destruct(); - } - catch (\Exception $e) { - $this->logger->error('Unable to convert hashes for ' . $plan_id); - $this->logger->error($e->getMessage()); - $this->logger->debug($e->getTraceAsString()); - } - } + $this->convertTables('hashes'); } /** @@ -238,19 +222,39 @@ public function convertRunTable(string $plan_id) { * Outdated tables will be removed. */ public function harvestRunsUpdate() { + $this->convertTables('runs'); + } + + /** + * Update all the harvest run tables to use entities. + * + * Outdated tables will be removed. + * + * @param string $type + * The type of table to convert. Must be one of 'hashes' or 'runs'. + */ + protected function convertTables(string $type) { + if (!in_array($type, ['hashes', 'runs'])) { + throw new \InvalidArgumentException('Must be the right thing.'); + } $plan_ids = array_merge( $this->harvestService->getAllHarvestIds(), array_values($this->findOrphanedHarvestDataIds()) ); foreach ($plan_ids as $plan_id) { - $this->logger->notice('Converting runs for ' . $plan_id); + $this->logger->notice('Converting ' . $type . ' for ' . $plan_id); try { - $this->convertRunTable($plan_id); - $this->storeFactory->getInstance('harvest_' . $plan_id . '_runs') + if ($type === 'hashes') { + $this->convertHashTable($plan_id); + } + if ($type === 'runs') { + $this->convertRunTable($plan_id); + } + $this->storeFactory->getInstance('harvest_' . $plan_id . '_' . $type) ->destruct(); } catch (\Exception $e) { - $this->logger->error('Unable to convert runs for ' . $plan_id); + $this->logger->error('Unable to convert ' . $type . ' for ' . $plan_id); $this->logger->error($e->getMessage()); $this->logger->debug($e->getTraceAsString()); } From adb07f82269864e1ace7d521b9a58c784b1d7713 Mon Sep 17 00:00:00 2001 From: Paul Mitchum Date: Mon, 9 Sep 2024 14:22:40 -0700 Subject: [PATCH 3/3] complexity... --- modules/harvest/src/HarvestUtility.php | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/modules/harvest/src/HarvestUtility.php b/modules/harvest/src/HarvestUtility.php index 515574bd15..755dd5f7dc 100644 --- a/modules/harvest/src/HarvestUtility.php +++ b/modules/harvest/src/HarvestUtility.php @@ -226,7 +226,7 @@ public function harvestRunsUpdate() { } /** - * Update all the harvest run tables to use entities. + * Convert the hashes or runs tables to use entities. * * Outdated tables will be removed. * @@ -234,8 +234,13 @@ public function harvestRunsUpdate() { * The type of table to convert. Must be one of 'hashes' or 'runs'. */ protected function convertTables(string $type) { - if (!in_array($type, ['hashes', 'runs'])) { - throw new \InvalidArgumentException('Must be the right thing.'); + // Types mapped to conversion methods. + $types = [ + 'hashes' => 'convertHashTable', + 'runs' => 'convertRunTable', + ]; + if (!in_array($type, array_keys($types))) { + throw new \InvalidArgumentException('Type must be one of: ' . implode(', ', array_keys($types))); } $plan_ids = array_merge( $this->harvestService->getAllHarvestIds(), @@ -244,12 +249,7 @@ protected function convertTables(string $type) { foreach ($plan_ids as $plan_id) { $this->logger->notice('Converting ' . $type . ' for ' . $plan_id); try { - if ($type === 'hashes') { - $this->convertHashTable($plan_id); - } - if ($type === 'runs') { - $this->convertRunTable($plan_id); - } + $this->{$types[$type]}($plan_id); $this->storeFactory->getInstance('harvest_' . $plan_id . '_' . $type) ->destruct(); }