Merge pull request #426 from jackalope/binary-cascade

delete binary when property is deleted
jackalope · Sep 19, 2023 · 183fd0b · 183fd0b
2 parents f688316 + fa8a880
commit 183fd0b
Show file tree

Hide file tree

Showing 11 changed files with 131 additions and 33 deletions.
diff --git a/.gitignore b/.gitignore
@@ -18,5 +18,3 @@ phpcr_tests.db-journal
 
 vendor/
 composer.lock
-
-/test
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -14,6 +14,7 @@ Changelog
 * Fixed: While it is allowed to call `Repository::login` with `null` credentials, there used to be an error. It now correctly works.
   If you use `jcr:createdBy` or `jcr:lastModifiedBy` in node types, those properties are not set if the credentials are `null`.
 * Improving the performance of `deleteProperties` (#421)
+* Deleting dangling binary references when a property is removed or the whole node with a binary property is deleted (#426) - See UPGRADE.md for the recommended database changes.
 
 1.x
 ===

diff --git a/UPGRADE.md b/UPGRADE.md
@@ -11,6 +11,12 @@ Upgrade
 - `cli-config.php.dist` has been renamed to `cli-config.dist.php` - if you automate usage of the cli
   config file, you will need to adjust.
 
+- We changed the database schema to cascade delete the binary data when a node is deleted. To
+  upgrade the schema, delete all dangling references and add the foreign key to the database:
+
+  `DELETE FROM phpcr_binarydata where node_id NOT IN (SELECT id FROM phpcr_nodes)`
+  `ALTER TABLE phpcr_binarydata ADD CONSTRAINT fk_nodes FOREIGN KEY (node_id) REFERENCES phpcr_nodes(id) ON DELETE CASCADE`
+
 1.5
 ---
 

diff --git a/src/Jackalope/Transport/DoctrineDBAL/Client.php b/src/Jackalope/Transport/DoctrineDBAL/Client.php
@@ -142,6 +142,7 @@ class Client extends BaseTransport implements QueryTransport, WritingInterface,
         PropertyType::REFERENCE => 'phpcr_nodes_references',
         PropertyType::WEAKREFERENCE => 'phpcr_nodes_weakreferences',
     ];
+    private string $binaryTable = 'phpcr_binarydata';
 
     private array $referencesToDelete = [];
 
@@ -1605,6 +1606,7 @@ private function removePropertiesFromNode(string|int $nodeId, array $paths): voi
         foreach ($paths as $path) {
             $propertyName = PathHelper::getNodeName($path);
             $tablesToDeleteReferencesFrom = [];
+            $deleteBinary = false;
             foreach ($xpath->query(sprintf('//*[@sv:name="%s"]', $propertyName)) as $propertyNode) {
                 \assert($propertyNode instanceof \DOMElement);
                 $found = true;
@@ -1615,8 +1617,18 @@ private function removePropertiesFromNode(string|int $nodeId, array $paths): voi
                 }
 
                 $type = strtolower($propertyNode->getAttribute('sv:type'));
-                if (in_array($type, ['reference', 'weakreference'])) {
-                    $tablesToDeleteReferencesFrom[] = $this->referenceTables['reference' === $type ? PropertyType::REFERENCE : PropertyType::WEAKREFERENCE];
+                switch ($type) {
+                    case 'reference':
+                        $tablesToDeleteReferencesFrom[] = $this->referenceTables[PropertyType::REFERENCE];
+                        break;
+                    case 'weakreference':
+                        $tablesToDeleteReferencesFrom[] = $this->referenceTables[PropertyType::WEAKREFERENCE];
+                        break;
+                    case 'binary':
+                        $deleteBinary = true;
+                        break;
+                    default:
+                        // nothing to do
                 }
 
                 // Doing the XML removal
@@ -1641,6 +1653,18 @@ private function removePropertiesFromNode(string|int $nodeId, array $paths): voi
                     );
                 }
             }
+            if ($deleteBinary) {
+                try {
+                    $query = "DELETE FROM {$this->binaryTable} WHERE node_id = ? AND property_name = ?";
+                    $this->getConnection()->executeQuery($query, [$nodeId, $propertyName]);
+                } catch (DBALException $e) {
+                    throw new RepositoryException(
+                        "Can not delete binaries for property $propertyName from `{$this->binaryTable}`",
+                        $e->getCode(),
+                        $e
+                    );
+                }
+            }
         }
 
         $xml = $dom->saveXML();

diff --git a/src/Jackalope/Transport/DoctrineDBAL/RepositorySchema.php b/src/Jackalope/Transport/DoctrineDBAL/RepositorySchema.php
@@ -26,7 +26,11 @@ class RepositorySchema extends Schema
     public function __construct(array $options = [], Connection $connection = null)
     {
         $this->connection = $connection;
-        $schemaConfig = $connection?->getSchemaManager()->createSchemaConfig();
+        $schemaConfig = null;
+        if ($connection) {
+            $schemaManager = method_exists($connection, 'createSchemaManager') ? $connection->createSchemaManager() : $connection->getSchemaManager();
+            $schemaConfig = $schemaManager->createSchemaConfig();
+        }
 
         parent::__construct([], [], $schemaConfig);
 
@@ -118,6 +122,9 @@ protected function addBinaryDataTable(): void
         $binary->addColumn('data', 'blob');
         $binary->setPrimaryKey(['id']);
         $binary->addUniqueIndex(['node_id', 'property_name', 'workspace_name', 'idx']);
+        if (!array_key_exists('disable_fk', $this->options) || !$this->options['disable_fk']) {
+            $binary->addForeignKeyConstraint('phpcr_nodes', ['node_id'], ['id'], ['onDelete' => 'CASCADE']);
+        }
     }
 
     protected function addNodesReferencesTable(Table $nodes): void
@@ -128,7 +135,7 @@ protected function addNodesReferencesTable(Table $nodes): void
         $references->addColumn('target_id', 'integer');
         $references->setPrimaryKey(['source_id', 'source_property_name', 'target_id']);
         $references->addIndex(['target_id']);
-        if (!empty($this->options['disable_fk'])) {
+        if (!array_key_exists('disable_fk', $this->options) || !$this->options['disable_fk']) {
             $references->addForeignKeyConstraint($nodes, ['source_id'], ['id'], ['onDelete' => 'CASCADE']);
             // TODO: this should be reenabled on RDBMS with deferred FK support
             // $references->addForeignKeyConstraint($nodes, array('target_id'), array('id'));
@@ -143,7 +150,7 @@ protected function addNodesWeakreferencesTable(Table $nodes): void
         $weakreferences->addColumn('target_id', 'integer');
         $weakreferences->setPrimaryKey(['source_id', 'source_property_name', 'target_id']);
         $weakreferences->addIndex(['target_id']);
-        if (!empty($this->options['disable_fk'])) {
+        if (!array_key_exists('disable_fk', $this->options) || !$this->options['disable_fk']) {
             $weakreferences->addForeignKeyConstraint($nodes, ['source_id'], ['id'], ['onDelete' => 'CASCADE']);
             $weakreferences->addForeignKeyConstraint($nodes, ['target_id'], ['id'], ['onDelete' => 'CASCADE']);
         }

diff --git a/tests/Jackalope/Test/Fixture/DBUnitFixtureXML.php b/tests/Jackalope/Test/Fixture/DBUnitFixtureXML.php
@@ -43,18 +43,18 @@ class DBUnitFixtureXML extends XMLDocument
     public function __construct(string $file)
     {
         parent::__construct($file);
+        $this->dom->appendChild($this->dom->createComment(' This fixture is generated from the PHPCR fixtures. Do not edit manually '));
 
         $this->tables = [];
         $this->ids = [];
         $this->references = [];
         $this->expectedNodes = [];
     }
 
-    public function addDataset()
+    public function save()
     {
-        $this->dom->appendChild($this->dom->createElement('dataset'));
-
         // purge binary in case no binary properties are in fixture
+        // do this at the very end to make sure the binary table comes after the nodes table to avoid referencial integrity issues.
         $this->ensureTableExists('phpcr_binarydata', [
             'node_id',
             'property_name',
@@ -63,6 +63,13 @@ public function addDataset()
             'data',
         ]);
 
+        return parent::save();
+    }
+
+    public function addDataset()
+    {
+        $this->dom->appendChild($this->dom->createElement('dataset'));
+
         return $this;
     }
 

diff --git a/tests/Jackalope/Test/FunctionalTestCase.php b/tests/Jackalope/Test/FunctionalTestCase.php
@@ -46,7 +46,7 @@ public function setUp(): void
 
     protected function loadFixtures(Connection $conn): void
     {
-        $options = ['disable_fks' => $conn->getDatabasePlatform() instanceof SqlitePlatform];
+        $options = ['disable_fk' => $conn->getDatabasePlatform() instanceof SqlitePlatform];
         $schema = new RepositorySchema($options, $conn);
         $tables = $schema->getTables();
 

diff --git a/tests/Jackalope/Test/TestCase.php b/tests/Jackalope/Test/TestCase.php
@@ -8,30 +8,28 @@
 
 abstract class TestCase extends BaseTestCase
 {
-    /**
-     * @var Connection
-     */
-    protected $conn;
+    protected ?Connection $conn;
 
     protected function getConnection(): Connection
     {
-        if (null === $this->conn) {
-            // @TODO see https://github.com/jackalope/jackalope-doctrine-dbal/issues/48
-            global $dbConn;
-            $this->conn = $dbConn;
-
-            if (null === $this->conn) {
-                $this->conn = DriverManager::getConnection([
-                    'driver' => @$GLOBALS['phpcr.doctrine.dbal.driver'],
-                    'path' => @$GLOBALS['phpcr.doctrine.dbal.path'],
-                    'host' => @$GLOBALS['phpcr.doctrine.dbal.host'],
-                    'user' => @$GLOBALS['phpcr.doctrine.dbal.username'],
-                    'password' => @$GLOBALS['phpcr.doctrine.dbal.password'],
-                    'dbname' => @$GLOBALS['phpcr.doctrine.dbal.dbname'],
-                ]);
-            }
+        if (isset($this->conn)) {
+            return $this->conn;
+        }
+        // see https://github.com/jackalope/jackalope-doctrine-dbal/issues/48
+        global $dbConn;
+        if ($this->conn = $dbConn) {
+            return $this->conn;
         }
 
+        $this->conn = DriverManager::getConnection([
+            'driver' => @$GLOBALS['phpcr.doctrine.dbal.driver'],
+            'path' => @$GLOBALS['phpcr.doctrine.dbal.path'],
+            'host' => @$GLOBALS['phpcr.doctrine.dbal.host'],
+            'user' => @$GLOBALS['phpcr.doctrine.dbal.username'],
+            'password' => @$GLOBALS['phpcr.doctrine.dbal.password'],
+            'dbname' => @$GLOBALS['phpcr.doctrine.dbal.dbname'],
+        ]);
+
         return $this->conn;
     }
 }
diff --git a/tests/Jackalope/Transport/DoctrineDBAL/DeleteCascadeTest.php b/tests/Jackalope/Transport/DoctrineDBAL/DeleteCascadeTest.php
@@ -0,0 +1,58 @@
+<?php
+
+namespace Jackalope\Transport\DoctrineDBAL;
+
+use Doctrine\DBAL\Platforms\SqlitePlatform;
+use Jackalope\Test\FunctionalTestCase;
+use PHPCR\PropertyType;
+
+class DeleteCascadeTest extends FunctionalTestCase
+{
+    public function setUp(): void
+    {
+        parent::setUp();
+
+        if ($this->conn->getDatabasePlatform() instanceof SqlitePlatform) {
+            $this->markTestSkipped('Foreign keys are not supported with sqlite');
+        }
+
+        $a = $this->session->getNode('/')->addNode('node-a');
+        $a->setProperty('data', 'foo', PropertyType::BINARY);
+        $this->session->save();
+    }
+
+    public function testRemoveProperty(): void
+    {
+        $binaryRows = $this->conn->executeQuery('SELECT * FROM phpcr_binarydata');
+        $this->assertSame(1, $binaryRows->rowCount());
+
+        $this->session->removeItem('/node-a/data');
+        $this->session->save();
+        $binaryRows = $this->conn->executeQuery('SELECT * FROM phpcr_binarydata');
+        $this->assertSame(0, $binaryRows->rowCount());
+    }
+
+    public function testRemovePropertyFromNode(): void
+    {
+        $a = $this->session->getNode('/node-a');
+        $binaryRows = $this->conn->executeQuery('SELECT * FROM phpcr_binarydata');
+        $this->assertSame(1, $binaryRows->rowCount());
+
+        $a->getProperty('data')->remove();
+        $this->session->save();
+        $binaryRows = $this->conn->executeQuery('SELECT * FROM phpcr_binarydata');
+        $this->assertSame(0, $binaryRows->rowCount());
+    }
+
+    public function testRemoveNode(): void
+    {
+        $a = $this->session->getNode('/node-a');
+        $binaryRows = $this->conn->executeQuery('SELECT * FROM phpcr_binarydata');
+        $this->assertSame(1, $binaryRows->rowCount());
+
+        $a->remove();
+        $this->session->save();
+        $binaryRows = $this->conn->executeQuery('SELECT * FROM phpcr_binarydata');
+        $this->assertSame(0, $binaryRows->rowCount());
+    }
+}
diff --git a/tests/Jackalope/Transport/DoctrineDBAL/PrefetchTest.php b/tests/Jackalope/Transport/DoctrineDBAL/PrefetchTest.php
@@ -6,8 +6,6 @@
 
 class PrefetchTest extends FunctionalTestCase
 {
-    protected $conn;
-
     public function setUp(): void
     {
         parent::setUp();

diff --git a/tests/bootstrap.php b/tests/bootstrap.php
@@ -1,6 +1,7 @@
 <?php
 
 use Doctrine\DBAL\DriverManager;
+use Doctrine\DBAL\Platforms\SqlitePlatform;
 use Jackalope\Transport\DoctrineDBAL\RepositorySchema;
 
 /* Make sure we get ALL infos from php */
@@ -43,7 +44,7 @@
 
 /* Recreate database schema */
 if (!getenv('JACKALOPE_NO_TEST_DB_INIT')) {
-    $options = ['disable_fks' => $dbConn->getDatabasePlatform() instanceof \Doctrine\DBAL\Platforms\SqlitePlatform];
+    $options = ['disable_fk' => $dbConn->getDatabasePlatform() instanceof SqlitePlatform];
     $repositorySchema = new RepositorySchema($options, $dbConn);
     $repositorySchema->reset();
 }
Original file line number	Diff line number	Diff line change
Expand Up		@@ -18,5 +18,3 @@ phpcr_tests.db-journal

		vendor/
		composer.lock

		/test