Skip to content

Commit

Permalink
cascade delete binary
Browse files Browse the repository at this point in the history
  • Loading branch information
dbu committed Sep 19, 2023
1 parent f688316 commit b8e52b9
Show file tree
Hide file tree
Showing 9 changed files with 127 additions and 31 deletions.
2 changes: 0 additions & 2 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,3 @@ phpcr_tests.db-journal

vendor/
composer.lock

/test
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
===
Expand Down
6 changes: 6 additions & 0 deletions UPGRADE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
---

Expand Down
28 changes: 26 additions & 2 deletions src/Jackalope/Transport/DoctrineDBAL/Client.php
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [];

Expand Down Expand Up @@ -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;
Expand All @@ -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
Expand All @@ -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();
Expand Down
13 changes: 10 additions & 3 deletions src/Jackalope/Transport/DoctrineDBAL/RepositorySchema.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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
Expand All @@ -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'));
Expand All @@ -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']);
}
Expand Down
13 changes: 10 additions & 3 deletions tests/Jackalope/Test/Fixture/DBUnitFixtureXML.php
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -63,6 +63,13 @@ public function addDataset()
'data',
]);

parent::save();

Check failure on line 66 in tests/Jackalope/Test/Fixture/DBUnitFixtureXML.php

View workflow job for this annotation

GitHub Actions / PHPStan tests

Method Jackalope\Test\Fixture\DBUnitFixtureXML::save() should return Jackalope\Test\Fixture\XMLDocument but return statement is missing.
}

public function addDataset()
{
$this->dom->appendChild($this->dom->createElement('dataset'));

return $this;
}

Expand Down
36 changes: 17 additions & 19 deletions tests/Jackalope/Test/TestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
57 changes: 57 additions & 0 deletions tests/Jackalope/Transport/DoctrineDBAL/DeleteCascadeTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
<?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();

$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
{
if ($this->conn->getDatabasePlatform() instanceof SqlitePlatform) {
$this->markTestSkipped('Foreign keys are not supported with sqlite');
}
$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());
}
}
2 changes: 0 additions & 2 deletions tests/Jackalope/Transport/DoctrineDBAL/PrefetchTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@

class PrefetchTest extends FunctionalTestCase
{
protected $conn;

public function setUp(): void
{
parent::setUp();
Expand Down

0 comments on commit b8e52b9

Please sign in to comment.