Refactor hydration pattern in DataResource class (#3930)

GetDKAN · Apr 19, 2023 · 1e56fa4 · 1e56fa4
1 parent 0aadbd0
commit 1e56fa4
Show file tree

Hide file tree

Showing 12 changed files with 131 additions and 107 deletions.
diff --git a/modules/common/src/DataResource.php b/modules/common/src/DataResource.php
@@ -3,7 +3,6 @@
 namespace Drupal\common;
 
 use Drupal\datastore\DatastoreResource;
-use Procrastinator\HydratableTrait;
 use Procrastinator\JsonSerializeTrait;
 
 /**
@@ -28,7 +27,7 @@
  * @todo Refactor as service.
  */
 class DataResource implements \JsonSerializable {
-  use HydratableTrait, JsonSerializeTrait;
+  use JsonSerializeTrait;
 
   const DEFAULT_SOURCE_PERSPECTIVE = 'source';
 
@@ -78,6 +77,13 @@ class DataResource implements \JsonSerializable {
 
   /**
    * Constructor.
+   *
+   * @param string $file_path
+   *   Path to the file.
+   * @param string $mimeType
+   *   File mime type.
+   * @param string $perspective
+   *   Can be one of "local_file", "local_url", or "source".
    */
   public function __construct($file_path, $mimeType, $perspective = self::DEFAULT_SOURCE_PERSPECTIVE) {
     // @todo generate UUID instead.
@@ -91,7 +97,26 @@ public function __construct($file_path, $mimeType, $perspective = self::DEFAULT_
   }
 
   /**
-   * Create a new version.
+   * Create a DataResource object from a database record.
+   *
+   * @param object $record
+   *   Data resource record from the database. Must contain these properties:
+   *   'filePath', 'mimeType', 'perspective', 'version'.
+   *
+   * @return \Drupal\common\DataResource
+   *   DataResource object.
+   */
+  public static function createFromRecord(object $record): DataResource {
+    $resource = new static($record->filePath, $record->mimeType, $record->perspective);
+    // MD5 of record's file path can differ from the MD5 generated in the
+    // constructor, so we have to explicitly set the identifier.
+    $resource->identifier = $record->identifier;
+    $resource->version = $record->version;
+    return $resource;
+  }
+
+  /**
+   * Clone the current resource with a new version identifier.
    *
    * Versions are, simply, a unique "string" used to represent changes in a
    * resource. For example, when new data is added to a file/resource a new
@@ -101,17 +126,18 @@ public function __construct($file_path, $mimeType, $perspective = self::DEFAULT_
    * resources, it simply models the behavior to allow other parts of the
    * system to create new versions of resources when they deem it necessary.
    */
-  public function createNewVersion() {
+  public function createNewVersion(): DataResource {
     $newVersion = time();
     if ($newVersion == $this->version) {
       $newVersion++;
     }
-
-    return $this->createCommon('version', $newVersion);
+    $clone = clone $this;
+    $clone->version = $newVersion;
+    return $clone;
   }
 
   /**
-   * Create a new perspective.
+   * Clone the current resource with a new perspective.
    *
    * Perspectives are useful to represent clusters of connected resources.
    *
@@ -121,11 +147,11 @@ public function createNewVersion() {
    * aware of the new resource, the API endpoint, and maintain the relatioship
    * between the 2 resources.
    */
-  public function createNewPerspective($perspective, $uri) {
-    $new = $this->createCommon('perspective', $perspective);
-    $new->changeFilePath($uri);
-
-    return $new;
+  public function createNewPerspective($perspective, $uri): DataResource {
+    $clone = clone $this;
+    $clone->perspective = $perspective;
+    $clone->changeFilePath($uri);
+    return $clone;
   }
 
   /**
@@ -142,18 +168,6 @@ public function changeMimeType($newMimeType) {
     $this->mimeType = $newMimeType;
   }
 
-  /**
-   * Private.
-   */
-  private function createCommon($property, $value) {
-    $current = $this->{$property};
-    $new = $value;
-    $this->{$property} = $new;
-    $newResource = clone $this;
-    $this->{$property} = $current;
-    return $newResource;
-  }
-
   /**
    * Get object storing datastore specific information about this resource.
    *
@@ -205,6 +219,9 @@ public function getPerspective() {
 
   /**
    * Getter.
+   *
+   * @return string
+   *   The unique identifier.
    */
   public function getUniqueIdentifier() {
     return self::buildUniqueIdentifier($this->identifier, $this->version, $this->perspective);

diff --git a/...es/common/tests/src/Unit/ResourceTest.php → ...ommon/tests/src/Unit/DataResourceTest.php b/...es/common/tests/src/Unit/ResourceTest.php → ...ommon/tests/src/Unit/DataResourceTest.php
@@ -12,8 +12,10 @@
 
 /**
  * Unit tests for Drupal\common\Resource.
+ *
+ * @coversDefaultClass \Drupal\common\DataResource
  */
-class ResourceTest extends TestCase {
+class DataResourceTest extends TestCase {
 
   /**
    * Test getTableName().
@@ -82,4 +84,44 @@ public function testGetIdentifierAndVersionException() {
     $result = DataResource::getIdentifierAndVersion($id);
   }
 
+  /**
+   * @covers ::createNewPerspective
+   */
+  public function testCreateNewPerspective() {
+    $data_resource = new DataResource(
+      '/foo/bar',
+      'txt',
+      DataResource::DEFAULT_SOURCE_PERSPECTIVE
+    );
+
+    $clone_data_resource = $data_resource->createNewPerspective('local_url', 'uri://foo/bar');
+
+    // Not the same object.
+    $this->assertNotSame($data_resource, $clone_data_resource);
+    // Clone contains 'local_url' perspective.
+    $this->assertEquals('local_url', $clone_data_resource->getPerspective());
+    $this->assertEquals('uri://foo/bar', $clone_data_resource->getFilePath());
+  }
+
+  /**
+   * @covers ::createNewVersion
+   */
+  public function testCreateNewVersion() {
+    $data_resource = new DataResource(
+      '/foo/bar',
+      'txt',
+      DataResource::DEFAULT_SOURCE_PERSPECTIVE
+    );
+
+    $clone_data_resource = $data_resource->createNewVersion();
+
+    // Not the same object.
+    $this->assertNotSame($data_resource, $clone_data_resource);
+    // Clone contains new version.
+    $this->assertNotEquals(
+      $data_resource->getVersion(),
+      $clone_data_resource->getVersion()
+    );
+  }
+
 }
diff --git a/modules/datastore/src/DatastoreResource.php b/modules/datastore/src/DatastoreResource.php
@@ -40,21 +40,21 @@ public function __construct($id, $file_path, $mime_type) {
   /**
    * Get the resource ID.
    */
-  public function getId() {
+  public function getId(): string {
     return $this->id;
   }
 
   /**
    * Get the file path.
    */
-  public function getFilePath() {
+  public function getFilePath(): string {
     return $this->filePath;
   }
 
   /**
    * Get the mimeType.
    */
-  public function getMimeType() {
+  public function getMimeType(): string {
     return $this->mimeType;
   }
 
@@ -64,35 +64,10 @@ public function getMimeType() {
   #[\ReturnTypeWillChange]
   public function jsonSerialize() {
     return (object) [
-      'filePath' => $this->filePath,
-      'id' => $this->id,
-      'mimeType' => $this->mimeType,
+      'filePath' => $this->getFilePath(),
+      'id' => $this->getId(),
+      'mimeType' => $this->getMimeType(),
     ];
   }
 
-  /**
-   * {@inheritdoc}
-   */
-  public static function hydrate($json) {
-    $data = json_decode($json);
-    $reflector = new \ReflectionClass(self::class);
-    $object = $reflector->newInstanceWithoutConstructor();
-
-    $reflector = new \ReflectionClass($object);
-
-    $p = $reflector->getProperty('filePath');
-    $p->setAccessible(TRUE);
-    $p->setValue($object, $data->filePath);
-
-    $p = $reflector->getProperty('id');
-    $p->setAccessible(TRUE);
-    $p->setValue($object, $data->id);
-
-    $p = $reflector->getProperty('mimeType');
-    $p->setAccessible(TRUE);
-    $p->setValue($object, $data->mimeType);
-
-    return $object;
-  }
-
 }
diff --git a/modules/datastore/src/Storage/DatabaseTable.php b/modules/datastore/src/Storage/DatabaseTable.php
@@ -70,16 +70,6 @@ public function jsonSerialize() {
     return (object) ['resource' => $this->resource];
   }
 
-  /**
-   * Hydrate.
-   */
-  public static function hydrate(string $json) {
-    $data = json_decode($json);
-    $resource = DatastoreResource::hydrate(json_encode($data->resource));
-
-    return new DatabaseTable(\Drupal::service('database'), $resource);
-  }
-
   /**
    * Get the full name of datastore db table.
    *

diff --git a/modules/datastore/tests/src/Unit/Plugin/QueueWorker/ImportJobTest.php b/modules/datastore/tests/src/Unit/Plugin/QueueWorker/ImportJobTest.php
@@ -13,6 +13,9 @@
 
 /**
  * Unit tests for Importer class.
+ *
+ * @group datastore
+ * @group dkan-core
  */
 class ImportJobTest extends TestCase {
 
@@ -34,7 +37,7 @@ protected function tearDown(): void {
   /**
    *
    */
-  private function getDatastore(DatastoreResource $resource) {
+  private function getDatastore(DatastoreResource $resource): ImportJob {
     $storage = new Memory();
     $config = [
       "resource" => $resource,
@@ -49,7 +52,7 @@ private function getDatastore(DatastoreResource $resource) {
    */
   public function testBasics() {
     $resource = new DatastoreResource(1, __DIR__ . "/../../../../data/countries.csv", "text/csv");
-    $this->assertEquals($resource->getID(), 1);
+    $this->assertEquals(1, $resource->getID());
 
     $datastore = $this->getDatastore($resource);
 
@@ -145,12 +148,16 @@ public function testColumnNameSpaces() {
   }
 
   /**
+   * Test JSON/hydrate round-trip.
    *
+   * This pattern is deprecated.
+   *
+   * @group legacy
    */
   public function testSerialization() {
     $timeLimit = 40;
     $resource = new DatastoreResource(1, __DIR__ . "/../../../../data/countries.csv", "text/csv");
-    $this->assertEquals($resource->getID(), 1);
+    $this->assertEquals(1, $resource->getID());
 
     $datastore = $this->getDatastore($resource);
     $datastore->setTimeLimit($timeLimit);
@@ -216,17 +223,14 @@ public function testMultiplePasses() {
    */
   public function testBadStorage() {
     $storageInterfaceClass = DatabaseTableInterface::class;
-    $this->expectExceptionMessage("Storage must be an instance of {$storageInterfaceClass}");
+    $this->expectExceptionMessage("Storage must be an instance of $storageInterfaceClass");
     $resource = new DatastoreResource(1, __DIR__ . "/../../../../data/countries.csZv", "text/csv");
 
-    $importer = ImportJob::get("1", new Memory(), [
+    ImportJob::get("1", new Memory(), [
       "resource" => $resource,
       "storage" => new TestMemStorageBad(),
       "parser" => Csv::getParser(),
     ]);
-
-    $json = json_encode($importer);
-    ImportJob::hydrate($json);
   }
 
   /**
@@ -235,15 +239,15 @@ public function testBadStorage() {
   public function testNonStorage() {
     $this->expectExceptionMessage("Storage must be an instance of Drupal\common\Storage\DatabaseTableInterface");
     $resource = new DatastoreResource(1, __DIR__ . "/../../../../data/countries.csv", "text/csv");
-    $importer = ImportJob::get("1", new Memory(), [
+    ImportJob::get("1", new Memory(), [
       "resource" => $resource,
       "storage" => new class {
       },
       "parser" => Csv::getParser(),
     ]);
   }
 
-  public function sanitizeDescriptionProvider() {
+  public function sanitizeDescriptionProvider(): array {
     return [
       'multiline' => ["Multi\nLine", 'Multi Line'],
     ];
@@ -272,7 +276,7 @@ public function testSanitizeHeader($column, $expected) {
     $this->assertEquals($expected, ImportJob::sanitizeHeader($column));
   }
 
-  public function truncateHeaderProvider() {
+  public function truncateHeaderProvider(): array {
     $max_length = 64;
     return [
       'max_length' => [

diff --git a/modules/datastore/tests/src/Unit/Plugin/QueueWorker/TestMemStorage.php b/modules/datastore/tests/src/Unit/Plugin/QueueWorker/TestMemStorage.php
@@ -79,18 +79,6 @@ public function jsonSerialize()
         return (object) ['storage' => $this->storage];
     }
 
-    public static function hydrate(string $json)
-    {
-        $class = new \ReflectionClass(self::class);
-        $instance = $class->newInstanceWithoutConstructor();
-
-        $property = $class->getParentClass()->getProperty('storage');
-        $property->setAccessible(true);
-        $property->setValue($instance, (array) (json_decode($json))->storage);
-
-        return $instance;
-    }
-
     /**
      * Clean up and set the schema for SQL storage.
      *

diff --git a/modules/metastore/src/Reference/Referencer.php b/modules/metastore/src/Reference/Referencer.php
@@ -212,9 +212,19 @@ private function registerWithResourceMapper(string $downloadUrl, string $mimeTyp
   }
 
   /**
-   * Private.
+   * Get download URL for existing resource.
+   *
+   * @param array $info
+   *   Info.
+   * @param \Drupal\common\DataResource $stored
+   *   Stored data resource object.
+   * @param string $mimeType
+   *   MIME type.
+   *
+   * @return string
+   *   The download URL.
    */
-  private function handleExistingResource($info, $stored, $mimeType) {
+  private function handleExistingResource(array $info, DataResource $stored, string $mimeType): string {
     if ($info[0]->perspective == DataResource::DEFAULT_SOURCE_PERSPECTIVE &&
       (ResourceMapper::newRevision() == 1 || $stored->getMimeType() != $mimeType)) {
       $new = $stored->createNewVersion();