Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IBX-5502: Added additional tag cleaning to limit down number of orphaned tag en… #477

Open
wants to merge 6 commits into
base: 4.6
Choose a base branch
from
Open

IBX-5502: Added additional tag cleaning to limit down number of orphaned tag en… #477

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3f3b12e
Added additional tag cleaning to limit down number of orphaned tag en…
mateuszbieniek Feb 12, 2025
b8b1d0f
Fixed PHPStan errors
mateuszbieniek Feb 12, 2025
3ab8eab
CS Fix
mateuszbieniek Feb 12, 2025
7055240
Fixing tests
mateuszbieniek Feb 13, 2025
d7fe485
Fixed tests
mateuszbieniek Feb 13, 2025
ed32613
Fixed CS
mateuszbieniek Feb 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 31 additions & 5 deletions src/lib/Persistence/Cache/ContentHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,12 +335,33 @@ public function updateContent($contentId, $versionNo, UpdateStruct $struct)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While at it, given your changes affect method PHPDoc and signature, could you do full improvement on that?
The signature should now be as follows:

    /**
     * @throws \Ibexa\Contracts\Core\Repository\Exceptions\InvalidArgumentException
     * @throws \Psr\Cache\InvalidArgumentException
     */
    public function updateContent($contentId, $versionNo, UpdateStruct $struct): Content

$this->logger->logCall(__METHOD__, ['content' => $contentId, 'version' => $versionNo, 'struct' => $struct]);
$content = $this->persistenceHandler->contentHandler()->updateContent($contentId, $versionNo, $struct);
$this->cache->invalidateTags([
$this->cacheIdentifierGenerator->generateTag(
$locations = $this->persistenceHandler->locationHandler()->loadLocationsByContent($contentId);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you checked the impact of this on performance? Every load inside caching system is a potential risk. I'd like to avoid the situation where we release some memory, but make longer update time as the cost.

$locationTags = array_map(function (Content\Location $location): string {
return $this->cacheIdentifierGenerator->generateTag(self::LOCATION_IDENTIFIER, [$location->id]);
}, $locations);
Comment on lines +339 to +341
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not very readable to me. How about:

Suggested change
$locationTags = array_map(function (Content\Location $location): string {
return $this->cacheIdentifierGenerator->generateTag(self::LOCATION_IDENTIFIER, [$location->id]);
}, $locations);
$locationTags = array_map(
fn (Content\Location $location): string => $this->cacheIdentifierGenerator->generateTag(
self::LOCATION_IDENTIFIER,
[$location->id]
),
$locations
);

$locationPathTags = array_map(function (Content\Location $location): string {
return $this->cacheIdentifierGenerator->generateTag(self::LOCATION_PATH_IDENTIFIER, [$location->id]);
}, $locations);
Comment on lines +342 to +344
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given LOCATION_PATH_IDENTIFIER tag exists only in conjunction with LOCATION_IDENTIFIER tag, are you sure both are necessary?
Moreover, I don't see LOCATION_PATH_IDENTIFIER tags (lp-%s) in the list off affected by the issue.


$versionTags = [];
$versionTags[] = $this->cacheIdentifierGenerator->generateTag(
self::CONTENT_VERSION_IDENTIFIER,
[$contentId, $versionNo]
);

if ($versionNo > 1) {
$versionTags[] = $this->cacheIdentifierGenerator->generateTag(
self::CONTENT_VERSION_IDENTIFIER,
[$contentId, $versionNo]
),
]);
[$contentId, $versionNo - 1]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not invalidate tags for an unrelated versions. The cache performance side effects of that might be difficult to predict.

);
}

$tags = array_merge(
$locationTags,
$locationPathTags,
$versionTags
);
$this->cache->invalidateTags($tags);
Comment on lines +359 to +364
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of all the above comments, maybe...
Have you tried to use:

Suggested change
$tags = array_merge(
$locationTags,
$locationPathTags,
$versionTags
);
$this->cache->invalidateTags($tags);
$this->cache->invalidateTags($this->getContentTags($content));

instead of performing very similar loads to what that callable does?


return $content;
}
Expand All @@ -357,6 +378,7 @@ public function deleteContent($contentId)
$contentId,
APIRelation::FIELD | APIRelation::ASSET
);
$contentLocations = $this->persistenceHandler->locationHandler()->loadLocationsByContent($contentId);

$return = $this->persistenceHandler->contentHandler()->deleteContent($contentId);

Expand All @@ -372,6 +394,10 @@ function ($relation) {
$tags = [];
}
$tags[] = $this->cacheIdentifierGenerator->generateTag(self::CONTENT_IDENTIFIER, [$contentId]);
foreach ($contentLocations as $location) {
$tags[] = $this->cacheIdentifierGenerator->generateTag(self::LOCATION_IDENTIFIER, [$location->id]);
}

$this->cache->invalidateTags($tags);

return $return;
Expand Down
23 changes: 19 additions & 4 deletions src/lib/Persistence/Cache/TrashHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,15 @@

use Ibexa\Contracts\Core\Persistence\Content\Location\Trash\Handler as TrashHandlerInterface;
use Ibexa\Contracts\Core\Persistence\Content\Relation;
use Ibexa\Contracts\Core\Persistence\Content\VersionInfo;
use Ibexa\Contracts\Core\Repository\Values\Content\Query\Criterion;

class TrashHandler extends AbstractHandler implements TrashHandlerInterface
{
private const EMPTY_TRASH_BULK_SIZE = 100;
private const CONTENT_IDENTIFIER = 'content';
private const CONTENT_VERSION_IDENTIFIER = 'content_version';
private const LOCATION_IDENTIFIER = 'location';
private const LOCATION_PATH_IDENTIFIER = 'location_path';
Comment on lines 17 to 20
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh, let's fix it while at it, because adding new consts like that contradicts the purpose of those consts.

What you need to do is make ContentHandler constants used in this block public and either:
a) point const value here to those constants in ContentHandler
b) use directly in this class ContentHandler constants.

b) is preferred, but maybe a bit out of scope for you. a) is easily achievable.

The point of using a const there is usage search. When I'm searching for ContentHandler::CONTENT_IDENTIFIER const, I need to see all places it is used. When done like that that information is lost.


/**
Expand All @@ -34,8 +37,11 @@ public function trashSubtree($locationId)
$this->logger->logCall(__METHOD__, ['locationId' => $locationId]);

$location = $this->persistenceHandler->locationHandler()->load($locationId);
$reverseRelations = $this->persistenceHandler->contentHandler()->loadRelations($location->contentId);
$contentId = $location->contentId;

$contentHandler = $this->persistenceHandler->contentHandler();
$reverseRelations = $contentHandler->loadRelations($contentId);
$versions = $contentHandler->listVersions($contentId);
$return = $this->persistenceHandler->trashHandler()->trashSubtree($locationId);

$relationTags = [];
Expand All @@ -48,12 +54,21 @@ public function trashSubtree($locationId)
}, $reverseRelations);
}

$versionTags = array_map(function (VersionInfo $versionInfo) use ($contentId): string {
return $this->cacheIdentifierGenerator->generateTag(
self::CONTENT_VERSION_IDENTIFIER,
[$contentId, $versionInfo->versionNo]
);
}, $versions);

$tags = array_merge(
$versionTags,
$relationTags,
[
$this->cacheIdentifierGenerator->generateTag(self::CONTENT_IDENTIFIER, [$location->contentId]),
$this->cacheIdentifierGenerator->generateTag(self::CONTENT_IDENTIFIER, [$contentId]),
$this->cacheIdentifierGenerator->generateTag(self::LOCATION_PATH_IDENTIFIER, [$locationId]),
],
$relationTags
$this->cacheIdentifierGenerator->generateTag(self::LOCATION_IDENTIFIER, [$locationId]),
]
);
$this->cache->invalidateTags(array_values(array_unique($tags)));

Expand Down
122 changes: 103 additions & 19 deletions tests/lib/Persistence/Cache/ContentHandlerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
use Ibexa\Contracts\Core\Persistence\Content\ContentInfo;
use Ibexa\Contracts\Core\Persistence\Content\CreateStruct;
use Ibexa\Contracts\Core\Persistence\Content\Handler as SPIContentHandler;
use Ibexa\Contracts\Core\Persistence\Content\Location\Handler as SPILocationHandler;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technical debt: PersistenceLocationHandler. There's no SPI guarantees on persistence layer, so let's not use that name to avoid further confusion. Yeah it's fine to me that's inconsistent. I prefer that to appearing to promise sth we can't.

use Ibexa\Contracts\Core\Persistence\Content\MetadataUpdateStruct;
use Ibexa\Contracts\Core\Persistence\Content\Relation;
use Ibexa\Contracts\Core\Persistence\Content\Relation as SPIRelation;
Expand Down Expand Up @@ -48,7 +49,8 @@ public function providerForUnCachedMethods(): array
['setStatus', [2, 0, 1], [['content_version', [2, 1], false]], null, ['c-2-v-1']],
['setStatus', [2, 1, 1], [['content', [2], false]], null, ['c-2']],
['updateMetadata', [2, new MetadataUpdateStruct()], [['content', [2], false]], null, ['c-2']],
['updateContent', [2, 1, new UpdateStruct()], [['content_version', [2, 1], false]], null, ['c-2-v-1']],
//updateContent has its own test now due to relations complexity
//['updateContent', [2, 1, new UpdateStruct()], [['content_version', [2, 1], false]], null, ['c-2-v-1']],
//['deleteContent', [2]], own tests for relations complexity
['deleteVersion', [2, 1], [['content_version', [2, 1], false]], null, ['c-2-v-1']],
['addRelation', [new RelationCreateStruct(['destinationContentId' => 2, 'sourceContentId' => 4])], [['content', [2], false], ['content', [4], false]], null, ['c-2', 'c-4']],
Expand Down Expand Up @@ -437,27 +439,63 @@ public function providerForCachedLoadMethodsMiss(): array
];
}

public function testDeleteContent()
public function testUpdateContent(): void
{
$this->loggerMock->expects($this->once())->method('logCall');

$innerHandlerMock = $this->createMock(SPIContentHandler::class);
$this->persistenceHandlerMock
->expects($this->exactly(2))
->method('contentHandler')
->willReturn($innerHandlerMock);
$innerContentHandlerMock = $this->createMock(SPIContentHandler::class);
$innerLocationHandlerMock = $this->createMock(SPILocationHandler::class);

$innerHandlerMock
$this->prepareHandlerMocks(
$innerContentHandlerMock,
$innerLocationHandlerMock,
1,
1,
0
);

$innerContentHandlerMock
->expects($this->once())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
->expects($this->once())
->expects(self::once())

->method('loadReverseRelations')
->with(2, APIRelation::FIELD | APIRelation::ASSET)
->willReturn(
[
new SPIRelation(['sourceContentId' => 42]),
]
->method('updateContent')
->with(2, 1, new UpdateStruct())
->willReturn(new Content());

$this->cacheIdentifierGeneratorMock
->expects($this->exactly(5))
->method('generateTag')
->will(
self::returnValueMap([
['location', [3], false, 'l-3'],
['location', [4], false, 'l-4'],
['location_path', [3], false, 'lp-3'],
['location_path', [4], false, 'lp-4'],
['content_version', [2, 1], false, 'c-2-v-1'],
])
);
Comment on lines +463 to 474
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That returnValueMap syntax is obsolete.

Suggested change
$this->cacheIdentifierGeneratorMock
->expects($this->exactly(5))
->method('generateTag')
->will(
self::returnValueMap([
['location', [3], false, 'l-3'],
['location', [4], false, 'l-4'],
['location_path', [3], false, 'lp-3'],
['location_path', [4], false, 'lp-4'],
['content_version', [2, 1], false, 'c-2-v-1'],
])
);
$this->cacheIdentifierGeneratorMock
->expects(self::exactly(5))
->method('generateTag')
->willReturnMap(
[
['location', [3], false, 'l-3'],
['location', [4], false, 'l-4'],
['location_path', [3], false, 'lp-3'],
['location_path', [4], false, 'lp-4'],
['content_version', [2, 1], false, 'c-2-v-1'],
]
);


$innerHandlerMock
$this->cacheMock
->expects($this->once())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
->expects($this->once())
->expects(self::once())

->method('invalidateTags')
->with(['l-3', 'l-4', 'lp-3', 'lp-4', 'c-2-v-1']);

$handler = $this->persistenceCacheHandler->contentHandler();
$handler->updateContent(2, 1, new UpdateStruct());
}

public function testDeleteContent()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public function testDeleteContent()
public function testDeleteContent(): void

{
$this->loggerMock->expects($this->once())->method('logCall');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$this->loggerMock->expects($this->once())->method('logCall');
$this->loggerMock->expects(self::once())->method('logCall');


$innerContentHandlerMock = $this->createMock(SPIContentHandler::class);
$innerLocationHandlerMock = $this->createMock(SPILocationHandler::class);

$this->prepareHandlerMocks(
$innerContentHandlerMock,
$innerLocationHandlerMock,
2
);

$innerContentHandlerMock
->expects($this->once())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
->expects($this->once())
->expects(self::once())

->method('deleteContent')
->with(2)
Expand All @@ -468,22 +506,68 @@ public function testDeleteContent()
->method('deleteItem');

$this->cacheIdentifierGeneratorMock
->expects($this->exactly(2))
->expects($this->exactly(4))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
->expects($this->exactly(4))
->expects(self::exactly(4))

->method('generateTag')
->withConsecutive(
['content', [42], false],
['content', [2], false]
['content', [2], false],
['location', [3], false],
['location', [4], false]
)
->willReturnOnConsecutiveCalls('c-42', 'c-2');
->willReturnOnConsecutiveCalls('c-42', 'c-2', 'l-3', 'l-4');

$this->cacheMock
->expects($this->once())
->method('invalidateTags')
->with(['c-42', 'c-2']);
->with(['c-42', 'c-2', 'l-3', 'l-4']);

$handler = $this->persistenceCacheHandler->contentHandler();
$handler->deleteContent(2);
}

/**
* @param \Ibexa\Contracts\Core\Persistence\Content\Handler|\PHPUnit\Framework\MockObject\MockObject $innerContentHandlerMock
* @param \Ibexa\Contracts\Core\Persistence\Content\Location\Handler|\PHPUnit\Framework\MockObject\MockObject $innerLocationHandlerMock
*/
private function prepareHandlerMocks(
SPIContentHandler $innerContentHandlerMock,
SPILocationHandler $innerLocationHandlerMock,
int $contentHandlerCount = 1,
int $locationHandlerCount = 1,
int $loadReverseRelationsCount = 1,
int $loadLocationsByContentCount = 1
): void {
$this->persistenceHandlerMock
->expects($this->exactly($contentHandlerCount))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
->expects($this->exactly($contentHandlerCount))
->expects(self::exactly($contentHandlerCount))

->method('contentHandler')
->willReturn($innerContentHandlerMock);

$innerContentHandlerMock
->expects($this->exactly($loadReverseRelationsCount))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
->expects($this->exactly($loadReverseRelationsCount))
->expects(self::exactly($loadReverseRelationsCount))

->method('loadReverseRelations')
->with(2, APIRelation::FIELD | APIRelation::ASSET)
->willReturn(
[
new SPIRelation(['sourceContentId' => 42]),
]
);

$this->persistenceHandlerMock
->expects($this->exactly($locationHandlerCount))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
->expects($this->exactly($locationHandlerCount))
->expects(self::exactly($locationHandlerCount))

->method('locationHandler')
->willReturn($innerLocationHandlerMock);

$innerLocationHandlerMock
->expects($this->exactly($loadLocationsByContentCount))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
->expects($this->exactly($loadLocationsByContentCount))
->expects(self::exactly($loadLocationsByContentCount))

->method('loadLocationsByContent')
->with(2)
->willReturn(
[
new Content\Location(['id' => 3]),
new Content\Location(['id' => 4]),
]
);
}
}

class_alias(ContentHandlerTest::class, 'eZ\Publish\Core\Persistence\Cache\Tests\ContentHandlerTest');
19 changes: 17 additions & 2 deletions tests/lib/Persistence/Cache/TrashHandlerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
use Ibexa\Contracts\Core\Persistence\Content\Location\Trash\Handler as TrashHandler;
use Ibexa\Contracts\Core\Persistence\Content\Location\Trashed;
use Ibexa\Contracts\Core\Persistence\Content\Relation;
use Ibexa\Contracts\Core\Persistence\Content\VersionInfo;
use Ibexa\Contracts\Core\Repository\Values\Content\Trash\TrashItemDeleteResult;
use Ibexa\Core\Persistence\Cache\ContentHandler;
use Ibexa\Core\Persistence\Cache\LocationHandler;
Expand Down Expand Up @@ -118,10 +119,13 @@ public function testTrashSubtree()
{
$locationId = 6;
$contentId = 42;
$versionNo = 1;

$tags = [
'c-' . $contentId . '-v-' . $versionNo,
'c-' . $contentId,
'lp-' . $locationId,
'l-' . $locationId,
];

$handlerMethodName = $this->getHandlerMethodName();
Expand Down Expand Up @@ -149,18 +153,29 @@ public function testTrashSubtree()
->method($handlerMethodName)
->willReturn($innerHandler);

$contentHandlerMock
->expects($this->once())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
->expects($this->once())
->expects(self::once())

->method('listVersions')
->with($contentId)
->willReturn(
[
new VersionInfo(['versionNo' => $versionNo]),
]
);
$innerHandler
->expects($this->once())
->method('trashSubtree')
->with($locationId)
->willReturn(null);

$this->cacheIdentifierGeneratorMock
->expects($this->exactly(2))
->expects($this->exactly(4))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
->expects($this->exactly(4))
->expects(self::exactly(4))

->method('generateTag')
->withConsecutive(
['content_version', [$contentId, $versionNo], false],
['content', [$contentId], false],
['location_path', [$locationId], false]
['location_path', [$locationId], false],
['location', [$locationId], false],
)
->willReturnOnConsecutiveCalls(...$tags);

Expand Down
Loading