From e24842bf4cf15b921a52d9e6422d5ad99bb2175e Mon Sep 17 00:00:00 2001 From: Stefan Korn Date: Mon, 4 Dec 2023 17:17:47 +0100 Subject: [PATCH 1/4] #4075: Catch Guzzle Exception to avoid breaking harvest --- modules/metastore/src/Reference/Referencer.php | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/modules/metastore/src/Reference/Referencer.php b/modules/metastore/src/Reference/Referencer.php index 50eaf23517..a9c596681d 100644 --- a/modules/metastore/src/Reference/Referencer.php +++ b/modules/metastore/src/Reference/Referencer.php @@ -13,6 +13,7 @@ use Drupal\metastore\ResourceMapper; use GuzzleHttp\Client as GuzzleClient; +use GuzzleHttp\Exception\GuzzleException; /** * Metastore referencer service. @@ -342,7 +343,12 @@ private function getRemoteMimeType(string $downloadUrl): ?string { // Perform HTTP Head request against the supplied URL in order to determine // the content type of the remote resource. $client = new GuzzleClient(); - $response = $client->head($downloadUrl); + try { + $response = $client->head($downloadUrl); + } + catch (GuzzleException $exception) { + return $mime_type; + } // Extract the full value of the content type header. $content_type = $response->getHeader('Content-Type'); // Attempt to extract the mime type from the content type header. From 9cb98494d41c6c85826f9ed09ec7b141c234fe13 Mon Sep 17 00:00:00 2001 From: Paul Mitchum Date: Tue, 19 Dec 2023 16:07:22 -0800 Subject: [PATCH 2/4] first try --- modules/metastore/metastore.services.yml | 1 + .../metastore/src/Reference/Referencer.php | 16 +- .../src/Kernel/Reference/ReferencerTest.php | 239 ++++++++++++++++++ .../src/Unit/Reference/ReferencerTest.php | 6 + 4 files changed, 258 insertions(+), 4 deletions(-) create mode 100644 modules/metastore/tests/src/Kernel/Reference/ReferencerTest.php diff --git a/modules/metastore/metastore.services.yml b/modules/metastore/metastore.services.yml index 70a907082d..d9e2542e9c 100644 --- a/modules/metastore/metastore.services.yml +++ b/modules/metastore/metastore.services.yml @@ -41,6 +41,7 @@ services: - '@config.factory' - '@dkan.metastore.storage' - '@dkan.metastore.url_generator' + - '@http_client' calls: - [setLoggerFactory, ['@logger.factory']] diff --git a/modules/metastore/src/Reference/Referencer.php b/modules/metastore/src/Reference/Referencer.php index 8856d8b1a3..606df80afc 100644 --- a/modules/metastore/src/Reference/Referencer.php +++ b/modules/metastore/src/Reference/Referencer.php @@ -12,7 +12,7 @@ use Drupal\metastore\MetastoreService; use Drupal\metastore\ResourceMapper; -use GuzzleHttp\Client as GuzzleClient; +use GuzzleHttp\Client; use GuzzleHttp\Exception\GuzzleException; /** @@ -43,18 +43,27 @@ class Referencer { */ public MetastoreUrlGenerator $metastoreUrlGenerator; + /** + * Guzzle HTTP client. + * + * @var \GuzzleHttp\Client + */ + private Client $httpClient; + /** * Constructor. */ public function __construct( ConfigFactoryInterface $configService, FactoryInterface $storageFactory, - MetastoreUrlGenerator $metastoreUrlGenerator + MetastoreUrlGenerator $metastoreUrlGenerator, + Client $httpClient ) { $this->setConfigService($configService); $this->storageFactory = $storageFactory; $this->setLoggerFactory(\Drupal::service('logger.factory')); $this->metastoreUrlGenerator = $metastoreUrlGenerator; + $this->httpClient = $httpClient; } /** @@ -343,9 +352,8 @@ private function getRemoteMimeType(string $downloadUrl): ?string { // Perform HTTP Head request against the supplied URL in order to determine // the content type of the remote resource. - $client = new GuzzleClient(); try { - $response = $client->head($downloadUrl); + $response = $this->httpClient->head($downloadUrl); } catch (GuzzleException $exception) { return $mime_type; diff --git a/modules/metastore/tests/src/Kernel/Reference/ReferencerTest.php b/modules/metastore/tests/src/Kernel/Reference/ReferencerTest.php new file mode 100644 index 0000000000..0f6de49b76 --- /dev/null +++ b/modules/metastore/tests/src/Kernel/Reference/ReferencerTest.php @@ -0,0 +1,239 @@ + 0, + 'distribution' => 'distribution', + 'title' => 0, + 'identifier' => 0, + 'description' => 0, + 'accessLevel' => 0, + 'modified' => 0, + ]; + + private function mockReferencer($existing = TRUE) { + if ($existing) { + $node = (new Chain($this)) + ->add(Node::class, 'get', FieldItemListInterface::class) + ->addd('uuid', '0398f054-d712-4e20-ad1e-a03193d6ab33') + ->add(FieldItemListInterface::class, 'getString', 'orphaned') + ->add(Node::class, 'set') + ->add(Node::class, 'save') + ->getMock(); + } + else { + $node = (new Chain($this)) + ->add(Node::class, 'get', FieldItemListInterface::class) + ->addd('uuid', null) + ->add(FieldItemListInterface::class, 'getString', 'orphaned') + ->add(Node::class, 'set') + ->add(Node::class, 'save') + ->add(Node::class, 'setRevisionLogMessage') + ->getMock(); + } + + $storageFactory = (new Chain($this)) + ->add(DataFactory::class, 'getInstance', NodeData::class) + ->add(NodeData::class, 'getEntityStorage', NodeStorage::class) + ->add(NodeStorage::class, 'loadByProperties', [$node]) + ->add(NodeData::class, 'getEntityIdFromUuid', "1") + ->add(NodeData::class, 'getEntityLatestRevision', NULL) + ->add(NodeData::class, 'store', "abc") + ->getMock(); + + $immutableConfig = (new Chain($this)) + ->add(ImmutableConfig::class, 'get', self::REFERENCEABLE_PROPERTY_LIST) + ->getMock(); + + $configService = (new Chain($this)) + ->add(ConfigFactoryInterface::class, 'get', $immutableConfig) + ->getMock(); + + $urlGenerator = (new Chain($this)) + ->add(MetastoreUrlGenerator::class, 'uriFromUrl', 'dkan://metastore/schemas/data-dictionary/items/111') + ->getMock(); + + $referencer = new Referencer($configService, $storageFactory, $urlGenerator); + return $referencer; + } + + private function getContainer() { + $options = (new Options()) + ->add('stream_wrapper_manager', StreamWrapperManager::class) + ->add('logger.factory', LoggerChannelFactory::class) + ->add('request_stack', RequestStack::class) + ->add('dkan.metastore.resource_mapper', ResourceMapper::class) + ->add('file_system', FileSystem::class) + ->index(0); + + $container_chain = (new Chain($this)) + ->add(Container::class, 'get', $options) + ->add(RequestStack::class, 'getCurrentRequest', Request::class) + ->add(Request::class, 'getHost', 'test.test') + ->add(ResourceMapper::class, 'register', TRUE, 'resource') + ->add(FileSystem::class, 'getTempDirectory', '/tmp'); + + return $container_chain; + } + + protected function setUp(): void { + parent::setUp(); + $this->installEntitySchema('node'); + $this->installConfig(['metastore']); + } + + /** + * Create a test dataset using the supplied download URL. + */ + private function getData(string $downloadUrl, string $mediaType = NULL): object { + return (object) [ + 'title' => 'Test Dataset No Media Type', + 'description' => 'Hi', + 'identifier'=> '12345', + 'accessLevel'=> 'public', + 'modified'=> '06-04-2020', + 'keyword'=> ['hello'], + 'distribution'=> [ + (object) array_filter([ + 'title'=> 'blah', + 'mediaType' => $mediaType, + 'downloadURL'=> $downloadUrl, + ]), + ], + ]; + } + + public function provideData() { + return [ + // Test Mime Type detection using the resource `mediaType` property. + 'mediaType' => [$this->getData(self::HOST . '/' . self::FILE_PATH, self::MIME_TYPE)], +// $referencer->reference($data); +// $this->assertEquals(self::MIME_TYPE, $container_chain->getStoredInput('resource')[0]->getMimeType(), 'Unable to fetch MIME type from `mediaType` property'); + // Test Mime Type detection on a local file. + 'local file' => [$this->getData(self::HOST . '/' . self::FILE_PATH)], +// $referencer->reference($data); +// $this->assertEquals(self::MIME_TYPE, $container_chain->getStoredInput('resource')[0]->getMimeType(), 'Unable to fetch MIME type for local file'); + // Test Mime Type detection on a remote file. + 'remote file' => [$this->getData('https://dkan-default-content-files.s3.amazonaws.com/phpunit/district_centerpoints_small.csv')], +// $referencer->reference($data); +// $this->assertEquals(self::MIME_TYPE, $container_chain->getStoredInput('resource')[0]->getMimeType(), 'Unable to fetch MIME type for remote file'); + // Test Mime Type detection on a invalid remote file path. + 'bad remote file' => [$this->getData('http://invalid')], +// $this->expectException(ConnectException::class); + ]; + } + + /** + * Test the remote/local file mime type detection logic. + * + * @dataProvider provideData + */ + public function testMimeTypeDetection(object $data): void { + /** @var Referencer $referencer */ + $referencer = $this->container->get('dkan.metastore.referencer'); + + $referenced = json_decode($referencer->reference($data)); +// $this->assertEquals(self::MIME_TYPE, $container_chain->getStoredInput('resource')[0]->getMimeType(), 'Unable to fetch MIME type for remote file'); + + $this->assertIsObject($referenced); + } + + public function provideDataDictionaryData() { + return [ + [ + (object) ["describedBy" => "http://local-domain.com/api/1/metastore/schemas/data-dictionary/items/111"], + "dkan://metastore/schemas/data-dictionary/items/111", + ], + [ + (object) ["describedBy" => "http://remote-domain.com/dictionary.pdf"], + "http://remote-domain.com/dictionary.pdf", + ], + [ + (object) ["describedBy" => "dkan://metastore/schemas/data-dictionary/items/111"], + "dkan://metastore/schemas/data-dictionary/items/111", + ], + [ + (object) ["describedBy" => "s3://local-domain.com/api/1/metastore/schemas/data-dictionary/items/111"], + "s3://local-domain.com/api/1/metastore/schemas/data-dictionary/items/111", + ], + [ + (object) ["describedBy" => "dkan://metastore/schemas/data-dictionary/items/222"], + new \DomainException("is not a valid data-dictionary URI"), + ], + ]; + } + +} diff --git a/modules/metastore/tests/src/Unit/Reference/ReferencerTest.php b/modules/metastore/tests/src/Unit/Reference/ReferencerTest.php index adc4b7792d..88991c526f 100644 --- a/modules/metastore/tests/src/Unit/Reference/ReferencerTest.php +++ b/modules/metastore/tests/src/Unit/Reference/ReferencerTest.php @@ -35,6 +35,12 @@ use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\RequestStack; +/** + * @group dkan + * @group metastore + * + * @see Drupal\Tests\metastore\Kernel\Reference\ReferencerTest + */ class ReferencerTest extends TestCase { /** From 82fffca99929069de849578b391a7809bf1cf81b Mon Sep 17 00:00:00 2001 From: Paul Mitchum Date: Wed, 20 Dec 2023 09:24:06 -0800 Subject: [PATCH 3/4] fixed unit test --- .../metastore/src/Reference/Referencer.php | 7 +- .../src/Kernel/Reference/ReferencerTest.php | 239 ------------------ .../src/Unit/Reference/ReferencerTest.php | 45 +++- 3 files changed, 41 insertions(+), 250 deletions(-) delete mode 100644 modules/metastore/tests/src/Kernel/Reference/ReferencerTest.php diff --git a/modules/metastore/src/Reference/Referencer.php b/modules/metastore/src/Reference/Referencer.php index 606df80afc..ef00cd4a54 100644 --- a/modules/metastore/src/Reference/Referencer.php +++ b/modules/metastore/src/Reference/Referencer.php @@ -27,7 +27,7 @@ class Referencer { * * @var string */ - protected const DEFAULT_MIME_TYPE = 'text/plain'; + public const DEFAULT_MIME_TYPE = 'text/plain'; /** * Storage factory interface service. @@ -380,7 +380,7 @@ private function getRemoteMimeType(string $downloadUrl): ?string { * @todo Update the UI to set mediaType when a format is selected. */ private function getMimeType($distribution): string { - $mimeType = "text/plain"; + $mimeType = self::DEFAULT_MIME_TYPE; // If we have a mediaType set, use that. if (isset($distribution->mediaType)) { @@ -398,7 +398,8 @@ private function getMimeType($distribution): string { elseif (isset($distribution->downloadURL)) { // Determine whether the supplied distribution has a local or remote // resource. - $is_local = $distribution->downloadURL !== UrlHostTokenResolver::hostify($distribution->downloadURL); + $is_local = $distribution->downloadURL !== + UrlHostTokenResolver::hostify($distribution->downloadURL); $mimeType = $is_local ? $this->getLocalMimeType($distribution->downloadURL) : $this->getRemoteMimeType($distribution->downloadURL); diff --git a/modules/metastore/tests/src/Kernel/Reference/ReferencerTest.php b/modules/metastore/tests/src/Kernel/Reference/ReferencerTest.php deleted file mode 100644 index 0f6de49b76..0000000000 --- a/modules/metastore/tests/src/Kernel/Reference/ReferencerTest.php +++ /dev/null @@ -1,239 +0,0 @@ - 0, - 'distribution' => 'distribution', - 'title' => 0, - 'identifier' => 0, - 'description' => 0, - 'accessLevel' => 0, - 'modified' => 0, - ]; - - private function mockReferencer($existing = TRUE) { - if ($existing) { - $node = (new Chain($this)) - ->add(Node::class, 'get', FieldItemListInterface::class) - ->addd('uuid', '0398f054-d712-4e20-ad1e-a03193d6ab33') - ->add(FieldItemListInterface::class, 'getString', 'orphaned') - ->add(Node::class, 'set') - ->add(Node::class, 'save') - ->getMock(); - } - else { - $node = (new Chain($this)) - ->add(Node::class, 'get', FieldItemListInterface::class) - ->addd('uuid', null) - ->add(FieldItemListInterface::class, 'getString', 'orphaned') - ->add(Node::class, 'set') - ->add(Node::class, 'save') - ->add(Node::class, 'setRevisionLogMessage') - ->getMock(); - } - - $storageFactory = (new Chain($this)) - ->add(DataFactory::class, 'getInstance', NodeData::class) - ->add(NodeData::class, 'getEntityStorage', NodeStorage::class) - ->add(NodeStorage::class, 'loadByProperties', [$node]) - ->add(NodeData::class, 'getEntityIdFromUuid', "1") - ->add(NodeData::class, 'getEntityLatestRevision', NULL) - ->add(NodeData::class, 'store', "abc") - ->getMock(); - - $immutableConfig = (new Chain($this)) - ->add(ImmutableConfig::class, 'get', self::REFERENCEABLE_PROPERTY_LIST) - ->getMock(); - - $configService = (new Chain($this)) - ->add(ConfigFactoryInterface::class, 'get', $immutableConfig) - ->getMock(); - - $urlGenerator = (new Chain($this)) - ->add(MetastoreUrlGenerator::class, 'uriFromUrl', 'dkan://metastore/schemas/data-dictionary/items/111') - ->getMock(); - - $referencer = new Referencer($configService, $storageFactory, $urlGenerator); - return $referencer; - } - - private function getContainer() { - $options = (new Options()) - ->add('stream_wrapper_manager', StreamWrapperManager::class) - ->add('logger.factory', LoggerChannelFactory::class) - ->add('request_stack', RequestStack::class) - ->add('dkan.metastore.resource_mapper', ResourceMapper::class) - ->add('file_system', FileSystem::class) - ->index(0); - - $container_chain = (new Chain($this)) - ->add(Container::class, 'get', $options) - ->add(RequestStack::class, 'getCurrentRequest', Request::class) - ->add(Request::class, 'getHost', 'test.test') - ->add(ResourceMapper::class, 'register', TRUE, 'resource') - ->add(FileSystem::class, 'getTempDirectory', '/tmp'); - - return $container_chain; - } - - protected function setUp(): void { - parent::setUp(); - $this->installEntitySchema('node'); - $this->installConfig(['metastore']); - } - - /** - * Create a test dataset using the supplied download URL. - */ - private function getData(string $downloadUrl, string $mediaType = NULL): object { - return (object) [ - 'title' => 'Test Dataset No Media Type', - 'description' => 'Hi', - 'identifier'=> '12345', - 'accessLevel'=> 'public', - 'modified'=> '06-04-2020', - 'keyword'=> ['hello'], - 'distribution'=> [ - (object) array_filter([ - 'title'=> 'blah', - 'mediaType' => $mediaType, - 'downloadURL'=> $downloadUrl, - ]), - ], - ]; - } - - public function provideData() { - return [ - // Test Mime Type detection using the resource `mediaType` property. - 'mediaType' => [$this->getData(self::HOST . '/' . self::FILE_PATH, self::MIME_TYPE)], -// $referencer->reference($data); -// $this->assertEquals(self::MIME_TYPE, $container_chain->getStoredInput('resource')[0]->getMimeType(), 'Unable to fetch MIME type from `mediaType` property'); - // Test Mime Type detection on a local file. - 'local file' => [$this->getData(self::HOST . '/' . self::FILE_PATH)], -// $referencer->reference($data); -// $this->assertEquals(self::MIME_TYPE, $container_chain->getStoredInput('resource')[0]->getMimeType(), 'Unable to fetch MIME type for local file'); - // Test Mime Type detection on a remote file. - 'remote file' => [$this->getData('https://dkan-default-content-files.s3.amazonaws.com/phpunit/district_centerpoints_small.csv')], -// $referencer->reference($data); -// $this->assertEquals(self::MIME_TYPE, $container_chain->getStoredInput('resource')[0]->getMimeType(), 'Unable to fetch MIME type for remote file'); - // Test Mime Type detection on a invalid remote file path. - 'bad remote file' => [$this->getData('http://invalid')], -// $this->expectException(ConnectException::class); - ]; - } - - /** - * Test the remote/local file mime type detection logic. - * - * @dataProvider provideData - */ - public function testMimeTypeDetection(object $data): void { - /** @var Referencer $referencer */ - $referencer = $this->container->get('dkan.metastore.referencer'); - - $referenced = json_decode($referencer->reference($data)); -// $this->assertEquals(self::MIME_TYPE, $container_chain->getStoredInput('resource')[0]->getMimeType(), 'Unable to fetch MIME type for remote file'); - - $this->assertIsObject($referenced); - } - - public function provideDataDictionaryData() { - return [ - [ - (object) ["describedBy" => "http://local-domain.com/api/1/metastore/schemas/data-dictionary/items/111"], - "dkan://metastore/schemas/data-dictionary/items/111", - ], - [ - (object) ["describedBy" => "http://remote-domain.com/dictionary.pdf"], - "http://remote-domain.com/dictionary.pdf", - ], - [ - (object) ["describedBy" => "dkan://metastore/schemas/data-dictionary/items/111"], - "dkan://metastore/schemas/data-dictionary/items/111", - ], - [ - (object) ["describedBy" => "s3://local-domain.com/api/1/metastore/schemas/data-dictionary/items/111"], - "s3://local-domain.com/api/1/metastore/schemas/data-dictionary/items/111", - ], - [ - (object) ["describedBy" => "dkan://metastore/schemas/data-dictionary/items/222"], - new \DomainException("is not a valid data-dictionary URI"), - ], - ]; - } - -} diff --git a/modules/metastore/tests/src/Unit/Reference/ReferencerTest.php b/modules/metastore/tests/src/Unit/Reference/ReferencerTest.php index 88991c526f..4bcf1e71cd 100644 --- a/modules/metastore/tests/src/Unit/Reference/ReferencerTest.php +++ b/modules/metastore/tests/src/Unit/Reference/ReferencerTest.php @@ -26,6 +26,7 @@ use Drupal\node\Entity\Node; use Drupal\node\NodeStorage; +use GuzzleHttp\Client; use GuzzleHttp\Exception\ConnectException; use MockChain\Chain; use MockChain\Options; @@ -36,10 +37,11 @@ use Symfony\Component\HttpFoundation\RequestStack; /** + * @covers \Drupal\metastore\Reference\Referencer + * @coversDefaultClass \Drupal\metastore\Reference\Referencer + * * @group dkan * @group metastore - * - * @see Drupal\Tests\metastore\Kernel\Reference\ReferencerTest */ class ReferencerTest extends TestCase { @@ -116,8 +118,16 @@ private function mockReferencer($existing = TRUE) { ->add(MetastoreUrlGenerator::class, 'uriFromUrl', 'dkan://metastore/schemas/data-dictionary/items/111') ->getMock(); - $referencer = new Referencer($configService, $storageFactory, $urlGenerator); - return $referencer; + $http_client = $this->getMockBuilder(Client::class) + ->disableOriginalConstructor() + ->getMock(); + + return new Referencer( + $configService, + $storageFactory, + $urlGenerator, + $http_client + ); } private function getContainer() { @@ -381,6 +391,10 @@ private function getData(string $downloadUrl, string $mediaType = NULL): object /** * Test the remote/local file mime type detection logic. + * + * @covers ::getLocalMimeType + * @covers ::getMimeType + * @covers ::getRemoteMimeType */ public function testMimeTypeDetection(): void { // Initialize mock node class. @@ -436,7 +450,12 @@ public function getMimeType() { return ReferencerTest::MIME_TYPE; } ->add(MetastoreUrlGenerator::class, 'uriFromUrl', '') ->getMock(); - $referencer = new Referencer($configService, $storageFactory, $urlGenerator); + $referencer = new Referencer( + $configService, + $storageFactory, + $urlGenerator, + new Client() + ); // Test Mime Type detection using the resource `mediaType` property. $data = $this->getData(self::HOST . '/' . self::FILE_PATH, self::MIME_TYPE); @@ -450,10 +469,11 @@ public function getMimeType() { return ReferencerTest::MIME_TYPE; } $data = $this->getData('https://dkan-default-content-files.s3.amazonaws.com/phpunit/district_centerpoints_small.csv'); $referencer->reference($data); $this->assertEquals(self::MIME_TYPE, $container_chain->getStoredInput('resource')[0]->getMimeType(), 'Unable to fetch MIME type for remote file'); - // Test Mime Type detection on a invalid remote file path. + // Test Mime Type detection on a invalid remote file path. Defaults to + // text/plain. $data = $this->getData('http://invalid'); - $this->expectException(ConnectException::class); $referencer->reference($data); + $this->assertEquals(Referencer::DEFAULT_MIME_TYPE, $container_chain->getStoredInput('resource')[0]->getMimeType(), 'Did not use default MIME type for inaccessible remote file.'); } /** @@ -486,7 +506,16 @@ public function testDistributionHandlingDataDict($distribution, $describedBy) { ) ->getMock(); - $referencer = new Referencer($configService, $storageFactory, $urlGenerator); + $http_client = $this->getMockBuilder(Client::class) + ->disableOriginalConstructor() + ->getMock(); + + $referencer = new Referencer( + $configService, + $storageFactory, + $urlGenerator, + $http_client + ); if ($describedBy instanceof \Exception) { $this->expectException(get_class($describedBy)); From 2f6ca1148dddbea5b3dbfe1ebf6f4acfc033e8f5 Mon Sep 17 00:00:00 2001 From: Paul Mitchum Date: Wed, 20 Dec 2023 09:47:10 -0800 Subject: [PATCH 4/4] better mocked referencer in test --- .../metastore/tests/src/Unit/Reference/ReferencerTest.php | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/modules/metastore/tests/src/Unit/Reference/ReferencerTest.php b/modules/metastore/tests/src/Unit/Reference/ReferencerTest.php index 4bcf1e71cd..bbdc469f25 100644 --- a/modules/metastore/tests/src/Unit/Reference/ReferencerTest.php +++ b/modules/metastore/tests/src/Unit/Reference/ReferencerTest.php @@ -118,15 +118,11 @@ private function mockReferencer($existing = TRUE) { ->add(MetastoreUrlGenerator::class, 'uriFromUrl', 'dkan://metastore/schemas/data-dictionary/items/111') ->getMock(); - $http_client = $this->getMockBuilder(Client::class) - ->disableOriginalConstructor() - ->getMock(); - return new Referencer( $configService, $storageFactory, $urlGenerator, - $http_client + new Client() ); }