From 3b2c8e896cf7b9c38be7eb17eceaba9d648880ef Mon Sep 17 00:00:00 2001 From: Paul Mitchum Date: Thu, 6 Jul 2023 11:38:22 -0700 Subject: [PATCH] 15750 Jobstore allow existing sites to use deprecated table name (#3987) --- modules/common/src/Storage/JobStore.php | 68 ++++++-- .../tests/src/Kernel/Storage/JobStoreTest.php | 105 +++++++++++ .../tests/src/Unit/Storage/JobStoreTest.php | 3 +- .../datastore/src/Service/Info/ImportInfo.php | 2 +- .../src/Service/Info/ImportInfoList.php | 2 +- .../tests/src/Unit/Storage/JobStoreTest.php | 165 ------------------ 6 files changed, 165 insertions(+), 180 deletions(-) create mode 100644 modules/common/tests/src/Kernel/Storage/JobStoreTest.php delete mode 100644 modules/datastore/tests/src/Unit/Storage/JobStoreTest.php diff --git a/modules/common/src/Storage/JobStore.php b/modules/common/src/Storage/JobStore.php index e1bc0c005a..1c86d331e5 100644 --- a/modules/common/src/Storage/JobStore.php +++ b/modules/common/src/Storage/JobStore.php @@ -15,21 +15,30 @@ class JobStore extends AbstractDatabaseTable { * * @var string */ - private $jobClass; + private string $jobClass; /** - * Store the name of the table so we do not have to recompute. + * Store the name of the table so that we do not have to recompute. * * @var string */ - private $tableName; + private string $tableName; /** * Constructor. + * + * @param string $jobClass + * Class name of the job object which is using this storage table. Must be + * a subclass of \Procrastinator\Job\Job. + * @param \Drupal\Core\Database\Connection $connection + * Database connection. + * + * @throws \Exception + * Thrown if the job class is not valid. */ public function __construct(string $jobClass, Connection $connection) { if (!$this->validateJobClass($jobClass)) { - throw new \Exception("Invalid jobType provided: $jobClass"); + throw new \Exception('Invalid jobType provided: ' . $jobClass); } $this->jobClass = $jobClass; $this->setOurSchema(); @@ -48,21 +57,46 @@ public function retrieve(string $id) { } /** - * Protected. + * Get the table name, preferring the deprecated one if it exists. + * + * Since we have two table names (one deprecated), we should try to find out + * if the deprecated one exists. If it does, we use its name. Otherwise, we + * use the new table name. + * + * @todo Phase out the use of the deprecated table name. */ protected function getTableName() { if (empty($this->tableName)) { - // Avoid table-name-too-long errors by hashing the FQN of the class. - $exploded_class = explode("\\", $this->jobClass); - $this->tableName = strtolower(implode('_', [ - 'jobstore', - crc32($this->jobClass), - array_pop($exploded_class), - ])); + if ($this->tableExist($table = $this->getDeprecatedTableName())) { + $this->tableName = $table; + } + else { + // Avoid table-name-too-long errors by hashing the FQN of the class. + $exploded_class = explode('\\', $this->jobClass); + $this->tableName = strtolower(implode('_', [ + 'jobstore', + crc32($this->jobClass), + array_pop($exploded_class), + ])); + } } return $this->tableName; } + /** + * Get the deprecated table name. + * + * @return string + * Deprecated table name. + */ + protected function getDeprecatedTableName(): string { + $safeClassName = strtolower(preg_replace( + '/\\\\/', '_', + $this->jobClass + )); + return 'jobstore_' . $safeClassName; + } + /** * Private. */ @@ -112,4 +146,14 @@ public function primaryKey() { return 'ref_uuid'; } + /** + * Drop the table if it exists. + * + * Will also drop the deprecated table if it exists. + */ + public function destruct() { + parent::destruct(); + $this->connection->schema()->dropTable($this->getDeprecatedTableName()); + } + } diff --git a/modules/common/tests/src/Kernel/Storage/JobStoreTest.php b/modules/common/tests/src/Kernel/Storage/JobStoreTest.php new file mode 100644 index 0000000000..bb5bcd270a --- /dev/null +++ b/modules/common/tests/src/Kernel/Storage/JobStoreTest.php @@ -0,0 +1,105 @@ +container->get('database'); + // Make a JobStore object. + /** @var \Drupal\common\Storage\JobStoreFactory $job_store_factory */ + $job_store_factory = $this->container->get('dkan.common.job_store'); + $job_store = $job_store_factory->getInstance(\DkanTestJobSubclass::class); + + $this->assertInstanceOf(JobStore::class, $job_store); + + // First, get the table name without deprecation. + $ref_get_table_name = new \ReflectionMethod($job_store, 'getTableName'); + $ref_get_table_name->setAccessible(TRUE); + $table_name = $ref_get_table_name->invoke($job_store); + + // This table does not exist. + $this->assertFalse( + $db->schema()->tableExists($table_name) + ); + + // Use the deprecated table name. + $ref_get_deprecated_table_name = new \ReflectionMethod($job_store, 'getDeprecatedTableName'); + $ref_get_deprecated_table_name->setAccessible(TRUE); + $deprecated_table_name = $ref_get_deprecated_table_name->invoke($job_store); + $ref_table_name = new \ReflectionProperty($job_store, 'tableName'); + $ref_table_name->setAccessible(TRUE); + $ref_table_name->setValue($job_store, $deprecated_table_name); + + // Write the table. Count() will create the table before counting rows. + $this->assertEquals(0, $job_store->count()); + // Assert that the deprecated table exists. + $this->assertTrue( + $db->schema()->tableExists($deprecated_table_name) + ); + // Assert that the non-deprecated table does not exist. + $this->assertFalse( + $db->schema()->tableExists($table_name) + ); + + // Unset the table name, which should cause JobStore to decide to use the + // deprecated table name on its own, since we already have a table for it. + $ref_table_name->setValue($job_store, ''); + $this->assertEquals( + $deprecated_table_name, + $ref_get_table_name->invoke($job_store) + ); + + // Finally, remove the table, unset the job's table name, and it should + // create a new table with the non-deprecated name. + $job_store->destruct(); + $this->assertFalse( + $db->schema()->tableExists($deprecated_table_name) + ); + $this->assertFalse( + $db->schema()->tableExists($table_name) + ); + $ref_table_name->setValue($job_store, ''); + $this->assertEquals(0, $job_store->count()); + $this->assertTrue( + $db->schema()->tableExists($table_name) + ); + } + + } +} + +/** + * We must manage namespaces so that we don't end up with a too-long table name. + */ + +namespace { + + use Procrastinator\Job\Job; + + class DkanTestJobSubclass extends Job { + + protected function runIt() { + } + + } +} diff --git a/modules/common/tests/src/Unit/Storage/JobStoreTest.php b/modules/common/tests/src/Unit/Storage/JobStoreTest.php index 38d20e8622..e6d961536f 100644 --- a/modules/common/tests/src/Unit/Storage/JobStoreTest.php +++ b/modules/common/tests/src/Unit/Storage/JobStoreTest.php @@ -17,8 +17,9 @@ use PHPUnit\Framework\TestCase; /** + * @covers \Drupal\common\Storage\JobStore * @coversDefaultClass \Drupal\common\Storage\JobStore - * @group datastore + * @group common */ class JobStoreTest extends TestCase { diff --git a/modules/datastore/src/Service/Info/ImportInfo.php b/modules/datastore/src/Service/Info/ImportInfo.php index cd81587361..fde891bc1b 100644 --- a/modules/datastore/src/Service/Info/ImportInfo.php +++ b/modules/datastore/src/Service/Info/ImportInfo.php @@ -17,7 +17,7 @@ class ImportInfo { /** * A JobStore object. * - * @var \Drupal\common\Storage\JobStore + * @var \Drupal\common\Storage\JobStoreFactory */ private $jobStoreFactory; diff --git a/modules/datastore/src/Service/Info/ImportInfoList.php b/modules/datastore/src/Service/Info/ImportInfoList.php index d17aaef238..9ddec0c7ce 100644 --- a/modules/datastore/src/Service/Info/ImportInfoList.php +++ b/modules/datastore/src/Service/Info/ImportInfoList.php @@ -15,7 +15,7 @@ class ImportInfoList implements ContainerInjectionInterface { /** * A JobStore object. * - * @var \Drupal\common\Storage\JobStore + * @var \Drupal\common\Storage\JobStoreFactory */ private $jobStoreFactory; diff --git a/modules/datastore/tests/src/Unit/Storage/JobStoreTest.php b/modules/datastore/tests/src/Unit/Storage/JobStoreTest.php deleted file mode 100644 index d0b02d4699..0000000000 --- a/modules/datastore/tests/src/Unit/Storage/JobStoreTest.php +++ /dev/null @@ -1,165 +0,0 @@ -add(Connection::class, "schema", Schema::class) - ->add(Schema::class, "tableExists", FALSE); - - $jobStore = new JobStore(FileFetcher::class, $chain->getMock()); - $this->assertTrue(is_object($jobStore)); - } - - /** - * - */ - public function testRetrieve() { - $job_data = json_encode($this->getFileFetcher()); - $job = (object) []; - $job->ref_uuid = "1"; - $job->job_data = $job_data; - - $fieldInfo = [ - (object) ['Field' => "ref_uuid"], - (object) ['Field' => "job_data"], - ]; - - $chain = (new Chain($this)) - ->add(Connection::class, "schema", Schema::class) - ->add(Schema::class, "tableExists", TRUE) - ->add(Connection::class, 'select', Select::class, 'select_1') - ->add(Select::class, 'fields', Select::class) - ->add(Select::class, 'condition', Select::class) - ->add(Select::class, 'execute', StatementWrapper::class) - ->add(StatementWrapper::class, 'fetch', $job) - ->add(Connection::class, 'query', StatementWrapper::class) - ->add(StatementWrapper::class, 'fetchAll', $fieldInfo); - - $jobStore = new JobStore(FileFetcher::class, $chain->getMock()); - $this->assertEquals($job_data, $jobStore->retrieve("1", FileFetcher::class)); - } - - /** - * - */ - public function testRetrieveAll() { - $job_data = json_encode($this->getFileFetcher()); - $job = (object) []; - $job->ref_uuid = "1"; - $job->job_data = $job_data; - - $fieldInfo = [ - (object) ['Field' => "ref_uuid"], - (object) ['Field' => "job_data"], - ]; - - $sequence = (new Sequence()) - ->add($fieldInfo) - ->add([$job]); - - $chain = (new Chain($this)) - ->add(Connection::class, "schema", Schema::class) - ->add(Schema::class, "tableExists", TRUE) - ->add(Connection::class, 'select', Select::class, 'select_1') - ->add(Select::class, 'fields', Select::class) - ->add(Select::class, 'execute', StatementWrapper::class) - ->add(Connection::class, 'query', StatementWrapper::class) - ->add(StatementWrapper::class, 'fetchAll', $sequence); - - $jobStore = new JobStore(FileFetcher::class, $chain->getMock()); - $this->assertTrue(is_array($jobStore->retrieveAll())); - } - - /** - * - */ - public function testStore() { - $jobObject = $this->getFileFetcher(); - - $job_data = json_encode($jobObject); - $job = (object) []; - $job->jid = "1"; - $job->ref_uuid = "1"; - $job->job_data = $job_data; - - $fieldInfo = [ - (object) ['Field' => "ref_uuid"], - (object) ['Field' => "job_data"], - ]; - - $connection = (new Chain($this)) - ->add(Connection::class, "schema", Schema::class) - ->add(Schema::class, "tableExists", TRUE) - ->add(Connection::class, 'select', Select::class, 'select_1') - ->add(Select::class, 'fields', Select::class) - ->add(Select::class, 'condition', Select::class) - ->add(Select::class, 'execute', StatementWrapper::class) - ->add(StatementWrapper::class, 'fetch', $job) - ->add(Connection::class, 'update', Update::class) - ->add(Update::class, "fields", Update::class) - ->add(Update::class, "condition", Update::class) - ->add(Update::class, "execute", NULL) - ->add(Connection::class, 'query', StatementWrapper::class) - ->add(StatementWrapper::class, 'fetchAll', $fieldInfo) - ->getMock(); - - $jobStore = new JobStore(FileFetcher::class, $connection); - - $this->assertEquals("1", $jobStore->store(json_encode($jobObject), "1")); - } - - /** - * - */ - public function testRemove() { - $fieldInfo = [ - (object) ['Field' => "ref_uuid"], - (object) ['Field' => "job_data"], - ]; - - $connection = (new Chain($this)) - ->add(Connection::class, "schema", Schema::class) - ->add(Schema::class, "tableExists", TRUE) - ->add(Connection::class, "delete", Delete::class) - ->add(Delete::class, "condition", Delete::class) - ->add(Delete::class, "execute", NULL) - ->add(Connection::class, 'query', StatementWrapper::class) - ->add(StatementWrapper::class, 'fetchAll', $fieldInfo) - ->getMock(); - - $jobStore = new JobStore(FileFetcher::class, $connection); - - $this->assertEquals("", $jobStore->remove("1", FileFetcher::class)); - } - - /** - * Private. - */ - private function getFileFetcher() { - return FileFetcher::get("1", new Memory(), ["filePath" => "file://" . __DIR__ . "/../../data/countries.csv"]); - } - -}