Skip to content

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

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

Open
wants to merge 10 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
10 commits
Select commit Hold shift + click to select a range
6e60c00
Added additional tag cleaning to limit down number of orphaned tag en…
mateuszbieniek Feb 12, 2025
06b7bce
Fixed PHPStan errors
mateuszbieniek Feb 12, 2025
e6ce8b7
CS Fix
mateuszbieniek Feb 12, 2025
6536532
Fixing tests
mateuszbieniek Feb 13, 2025
b501ee2
Fixed tests
mateuszbieniek Feb 13, 2025
d597ba7
Fixed CS
mateuszbieniek Feb 13, 2025
a71ea65
Changes after CR#1
mateuszbieniek Mar 31, 2025
d46a8f6
Fixed cs styling and first stan issue
mateuszbieniek Mar 31, 2025
e8b66fc
Fixed phpstan conflict
mateuszbieniek Mar 31, 2025
c6eb694
Fixed baseline for 7.4
mateuszbieniek Mar 31, 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
6 changes: 6 additions & 0 deletions phpstan-baseline-7.4.neon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,12 @@ parameters:
count: 1
path: src/lib/MVC/Symfony/Matcher/ContentBased/UrlAlias.php

-
message: '#^PHPDoc tag @throws with type Ibexa\\Contracts\\Core\\Repository\\Exceptions\\InvalidArgumentException\|Psr\\Cache\\InvalidArgumentException is not subtype of Throwable$#'
identifier: throws.notThrowable
count: 1
path: src/lib/Persistence/Cache/ContentHandler.php

-
message: '#^PHPDoc tag @throws with type Psr\\Cache\\InvalidArgumentException is not subtype of Throwable$#'
identifier: throws.notThrowable
Expand Down
6 changes: 0 additions & 6 deletions phpstan-baseline.neon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60516,12 +60516,6 @@ parameters:
count: 1
path: tests/lib/Persistence/Cache/ContentHandlerTest.php

-
message: '#^Method Ibexa\\Tests\\Core\\Persistence\\Cache\\ContentHandlerTest\:\:testDeleteContent\(\) has no return type specified\.$#'
identifier: missingType.return
count: 1
path: tests/lib/Persistence/Cache/ContentHandlerTest.php

-
message: '#^Method Ibexa\\Tests\\Core\\Persistence\\Cache\\ContentLanguageHandlerTest\:\:providerForCachedLoadMethodsHit\(\) return type has no value type specified in iterable type array\.$#'
identifier: missingType.iterableValue
Expand Down
41 changes: 34 additions & 7 deletions src/lib/Persistence/Cache/ContentHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,18 +329,40 @@ public function updateMetadata($contentId, MetadataUpdateStruct $struct)
}

/**
* {@inheritdoc}
* @throws \Ibexa\Contracts\Core\Repository\Exceptions\InvalidArgumentException
* @throws \Psr\Cache\InvalidArgumentException
*/
public function updateContent($contentId, $versionNo, UpdateStruct $struct)
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.

Copy link
Contributor Author

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?

Only in the empirical way. If you have dozens or hundreds of locations, ofc this can have an impact, but then, probably this one is less of your worry then.

Every load inside caching system is a potential risk.

Any explanation behind this one? The change is made in a handler, not in the caching system, which to be frank main purpose is to do "loads".

I'd like to avoid the situation where we release some memory, but make longer update time as the cost.

That's, in my opinion is a wrong approach. This helps to clear unevictable memory from Redis - not doing this always results in eventually reaching a point when the whole app crashes. So this is not memory vs longer updates, but making the project less prone to failure due to OOM errors.

$locationTags = array_map(function (Content\Location $location): string {
return $this->cacheIdentifierGenerator->generateTag(self::LOCATION_IDENTIFIER, [$location->id]);
}, $locations);
Comment on lines +340 to +342
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
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, to be honest in my opinion thi is even less readable ;)

But no strong opinion here.

$locationPathTags = array_map(function (Content\Location $location): string {
return $this->cacheIdentifierGenerator->generateTag(self::LOCATION_PATH_IDENTIFIER, [$location->id]);
}, $locations);
Comment on lines +343 to +345
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned in the PR's description, the list presented only has an example, and the number of orphaned tags is not limited to it. But if you look really closely, you will also find mention of the lp- tag ;)


$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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true, and I agree. Any suggestionson how we can handle c-<contentId>-v-<prevVersionNo> tags?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ping @alongosz

);
}

$tags = array_merge(
$locationTags,
$locationPathTags,
$versionTags
);
$this->cache->invalidateTags($tags);
Comment on lines +360 to +365
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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not agree, getContentTags should not return location tags.


return $content;
}
Expand All @@ -357,6 +379,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 +395,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
126 changes: 105 additions & 21 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 PersistenceLocationHandler;
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,53 +439,135 @@ 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(PersistenceLocationHandler::class);

$innerHandlerMock
->expects($this->once())
->method('loadReverseRelations')
->with(2, APIRelation::FIELD | APIRelation::ASSET)
->willReturn(
$this->prepareHandlerMocks(
$innerContentHandlerMock,
$innerLocationHandlerMock,
1,
1,
0
);

$innerContentHandlerMock
->expects(self::once())
->method('updateContent')
->with(2, 1, new UpdateStruct())
->willReturn(new Content());

$this->cacheIdentifierGeneratorMock
->expects(self::exactly(5))
->method('generateTag')
->willReturnMap(
[
new SPIRelation(['sourceContentId' => 42]),
['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
->expects($this->once())
$this->cacheMock
->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(): void
{
$this->loggerMock->expects(self::once())->method('logCall');

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

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

$innerContentHandlerMock
->expects(self::once())
->method('deleteContent')
->with(2)
->willReturn(true);

$this->cacheMock
->expects($this->never())
->expects(self::never())
->method('deleteItem');

$this->cacheIdentifierGeneratorMock
->expects($this->exactly(2))
->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())
->expects(self::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,
PersistenceLocationHandler $innerLocationHandlerMock,
int $contentHandlerCount = 1,
int $locationHandlerCount = 1,
int $loadReverseRelationsCount = 1,
int $loadLocationsByContentCount = 1
): void {
$this->persistenceHandlerMock
->expects(self::exactly($contentHandlerCount))
->method('contentHandler')
->willReturn($innerContentHandlerMock);

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

$this->persistenceHandlerMock
->expects(self::exactly($locationHandlerCount))
->method('locationHandler')
->willReturn($innerLocationHandlerMock);

$innerLocationHandlerMock
->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');
25 changes: 20 additions & 5 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 All @@ -145,27 +149,38 @@ public function testTrashSubtree()
->willReturn($locationHandlerMock);

$this->persistenceHandlerMock
->expects($this->once())
->expects(self::once())
->method($handlerMethodName)
->willReturn($innerHandler);

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

$this->cacheIdentifierGeneratorMock
->expects($this->exactly(2))
->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);

$this->cacheMock
->expects($this->once())
->expects(self::once())
->method('invalidateTags')
->with($tags);

Expand Down
Loading