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.

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-9379: Added Grace Period for archived versions #515

Merged
merged 28 commits into from
Apr 7, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
72aa20f
Added Grace Period for achived versions
mateuszbieniek Mar 17, 2025
3acd147
Changes after CR#1
mateuszbieniek Mar 18, 2025
48b0db3
Fixed CS
mateuszbieniek Mar 18, 2025
337df43
Update src/lib/Persistence/Legacy/Content/Gateway/DoctrineDatabase.php
mateuszbieniek Mar 20, 2025
9a166df
Exposed grace_period_in_seconds in configuration
ViniTou Mar 26, 2025
2f88274
moved tag invalidating to kernel:terminate listener
ViniTou Mar 27, 2025
d51676d
added rough integration test for grace period content load
ViniTou Mar 28, 2025
d41399c
fixed phpstan issues
ViniTou Mar 28, 2025
f1f89fb
pass same id
ViniTou Mar 28, 2025
01f457b
Moved to mockable time in integration test
ViniTou Apr 1, 2025
7d895a2
Moved registering mocked time services to setUp method, as there is a…
ViniTou Apr 1, 2025
b14af0f
Moved registering mocked time services to bootstrap, as now it have s…
ViniTou Apr 1, 2025
13bcc0e
I am having such a good time
ViniTou Apr 1, 2025
9966027
I'm having a ball
ViniTou Apr 1, 2025
6efedc8
dont stop me now
ViniTou Apr 1, 2025
12feb97
just give ma a call
ViniTou Apr 1, 2025
8e5548b
extracted collector from subscriber
ViniTou Apr 2, 2025
6875253
cs fix
ViniTou Apr 2, 2025
7ebad66
marked Subscriber as final
ViniTou Apr 3, 2025
d979a0a
restored final keyword
ViniTou Apr 3, 2025
0c35b84
implement Reset interface
ViniTou Apr 3, 2025
9efaa4b
Update tests/integration/Core/Repository/ContentServiceTest.php
ViniTou Apr 3, 2025
56fa8f1
Update tests/integration/Core/Repository/UserServiceTest.php
ViniTou Apr 3, 2025
7df510b
Update tests/integration/Core/Repository/ContentServiceTest.php
ViniTou Apr 3, 2025
42b39cf
Update tests/integration/Core/Repository/ContentServiceTest.php
ViniTou Apr 3, 2025
bec0cb4
Update src/lib/Persistence/Legacy/Content/Gateway/DoctrineDatabase.php
ViniTou Apr 3, 2025
32d3da4
dropped shared on ContentCollector
ViniTou Apr 3, 2025
811e55c
Typehinted against TagAwareAdapterInterface to match @ibexa.cache_pool
ViniTou Apr 3, 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
9 changes: 9 additions & 0 deletions bootstrap.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,20 @@
* @copyright Copyright (C) Ibexa AS. All rights reserved.
* @license For full copyright and license information view LICENSE file distributed with this source code.
*/

use Ibexa\Core\Persistence\Legacy\Content\Gateway\DoctrineDatabase;
use Ibexa\Core\Persistence\Legacy\Content\Gateway\DoctrineDatabase\QueryBuilder;
use Ibexa\Core\Repository\ContentService;
use Symfony\Bridge\PhpUnit\ClockMock;
use Symfony\Component\HttpFoundation\Request;

// Register ClockMock for Request class before any tests are run
// https://github.com/symfony/symfony/issues/28259
ClockMock::register(Request::class);

// Register ClockMock, as otherwise they are mocked until first method call.
ClockMock::register(DoctrineDatabase::class);
ClockMock::register(ContentService::class);
ClockMock::register(QueryBuilder::class);

require_once __DIR__ . '/vendor/autoload.php';
12 changes: 0 additions & 12 deletions phpstan-baseline.neon
Original file line number Diff line number Diff line change
Expand Up @@ -70662,12 +70662,6 @@ parameters:
count: 1
path: tests/lib/Repository/Service/Mock/ContentTest.php

-
message: '#^Call to new Ibexa\\Core\\Repository\\ContentService\(\) on a separate line has no effect\.$#'
identifier: new.resultUnused
count: 1
path: tests/lib/Repository/Service/Mock/ContentTest.php

-
message: '#^Cannot access offset mixed on Ibexa\\Contracts\\Core\\Repository\\Values\\Content\\Field\.$#'
identifier: offsetAccess.nonOffsetAccessible
Expand Down Expand Up @@ -70836,12 +70830,6 @@ parameters:
count: 1
path: tests/lib/Repository/Service/Mock/ContentTest.php

-
message: '#^Method Ibexa\\Tests\\Core\\Repository\\Service\\Mock\\ContentTest\:\:getPartlyMockedContentService\(\) invoked with 2 parameters, 0\-1 required\.$#'
identifier: arguments.count
count: 1
path: tests/lib/Repository/Service/Mock/ContentTest.php

-
message: '#^Method Ibexa\\Tests\\Core\\Repository\\Service\\Mock\\ContentTest\:\:getPartlyMockedContentService\(\) should return Ibexa\\Core\\Repository\\ContentService&PHPUnit\\Framework\\MockObject\\MockObject but returns Ibexa\\Core\\Repository\\ContentService\.$#'
identifier: return.type
Expand Down
6 changes: 5 additions & 1 deletion src/bundle/Core/ApiLoader/RepositoryFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
use Ibexa\Contracts\Core\Search\Handler as SearchHandler;
use Ibexa\Contracts\Core\SiteAccess\ConfigResolverInterface;
use Ibexa\Core\FieldType\FieldTypeRegistry;
use Ibexa\Core\Repository\Collector\ContentCollector;
use Ibexa\Core\Repository\Helper\RelationProcessor;
use Ibexa\Core\Repository\Mapper;
use Ibexa\Core\Repository\Permission\LimitationService;
Expand Down Expand Up @@ -93,7 +94,8 @@ public function buildRepository(
LocationFilteringHandler $locationFilteringHandler,
PasswordValidatorInterface $passwordValidator,
ConfigResolverInterface $configResolver,
NameSchemaServiceInterface $nameSchemaService
NameSchemaServiceInterface $nameSchemaService,
ContentCollector $contentCollector
): Repository {
$config = $this->container->get(RepositoryConfigurationProvider::class)->getRepositoryConfig();

Expand All @@ -119,6 +121,7 @@ public function buildRepository(
$passwordValidator,
$configResolver,
$nameSchemaService,
$contentCollector,
[
'role' => [
'policyMap' => $this->policyMap,
Expand All @@ -127,6 +130,7 @@ public function buildRepository(
'content' => [
'default_version_archive_limit' => $config['options']['default_version_archive_limit'],
'remove_archived_versions_on_publish' => $config['options']['remove_archived_versions_on_publish'],
'grace_period_in_seconds' => $config['options']['grace_period_in_seconds'] ?? (int) ini_get('max_execution_time'),
],
],
$this->logger
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ public function addSemanticConfig(NodeBuilder $nodeBuilder): void
->defaultTrue()
->info('Enables automatic removal of archived versions when publishing, at the cost of performance. "ezplatform:content:cleanup-versions" command should be used to perform this task instead if this option is set to false.')
->end()
->integerNode('grace_period_in_seconds')
->info('Provide a value in seconds, when archived content is still accessible for users with access to current version. Prevents 500 error when accessed content is updated during request. Defaults to php max execution time.')
->end()
->end()
->end();
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
<?php

/**
* @copyright Copyright (C) Ibexa AS. All rights reserved.
* @license For full copyright and license information view LICENSE file distributed with this source code.
*/
declare(strict_types=1);

namespace Ibexa\Bundle\Core\EventSubscriber;

use Ibexa\Core\Persistence\Cache\Identifier\CacheIdentifierGeneratorInterface;
use Ibexa\Core\Repository\Collector\ContentCollector;
use Symfony\Component\Cache\Adapter\TagAwareAdapterInterface;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpKernel\Event\TerminateEvent;
use Symfony\Component\HttpKernel\KernelEvents;

final class ClearCollectedContentCacheSubscriber implements EventSubscriberInterface
{
private TagAwareAdapterInterface $cache;

private CacheIdentifierGeneratorInterface $identifierGenerator;

private ContentCollector $contentCollector;

public function __construct(
ContentCollector $contentCollector,
Copy link
Member

Choose a reason for hiding this comment

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

AFAICS, this service is not shared, so won't that make it always new instance when injecting? Meaning are the items that you've collected in ContentService be available here in this instance?
Just asking, never used non-shared services in practice.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought this works in a little bit different, with making sure that it is not shared between requests in for example long living proceses, but I think you might be right (or we both are - and potential gains outweights problems) so I will remove shared config from service.

Copy link
Member

Choose a reason for hiding this comment

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

so I will remove shared config from service.

That would be my expectation, but then maybe - given its generic name and namespace location - it should be a named service with some interface? Someone could be tempted to reuse this for different purpose.

Meaning defining it like:

    ibexa.core.content.collector.grace_period:
         class: ...\ContentCollectorInterface

so sb can create his own instance:

    app.content.collector.my_other_purpose:
         class: ...\ContentCollectorInterface

TagAwareAdapterInterface $cache,
CacheIdentifierGeneratorInterface $identifierGenerator
) {
$this->cache = $cache;
$this->identifierGenerator = $identifierGenerator;
$this->contentCollector = $contentCollector;
}

public static function getSubscribedEvents(): array
{
return [KernelEvents::TERMINATE => 'clearCache'];
}

public function clearCache(TerminateEvent $event): void
{
foreach ($this->contentCollector->getCollectedContentIds() as $contentId) {
$this->cache->invalidateTags([
$this->identifierGenerator->generateTag('content', [$contentId]),
]);
}

$this->contentCollector->reset();
}
}
6 changes: 6 additions & 0 deletions src/bundle/Core/Resources/config/services.yml
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,12 @@ services:
tags:
- {name: kernel.event_subscriber}

Ibexa\Bundle\Core\EventSubscriber\ClearCollectedContentCacheSubscriber:
autowire: true
autoconfigure: true
arguments:
$cache: '@ibexa.cache_pool'
Copy link
Member

Choose a reason for hiding this comment

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

Seems like ibexa.cache_pool is of Symfony\Component\Cache\Adapter\TagAwareAdapterInterface type, while you're expecting \Ibexa\Core\Persistence\Cache\Adapter\TransactionAwareAdapterInterface in the constructor. Maybe we should decorate something?
ibexa.cache_pool can be theoretically replaced by different cache implementing/extending TagAwareAdapterInterface, so this might be important, if someone did so.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can just change type to Symfony\Component\Cache\Adapter\TagAwareAdapterInterface and see how that works, it should be fine due to method used.


Ibexa\Bundle\Core\EventSubscriber\TrustedHeaderClientIpEventSubscriber:
arguments:
$trustedHeaderName: '%ibexa.trusted_header_client_ip_name%'
Expand Down
7 changes: 7 additions & 0 deletions src/contracts/Persistence/Content/Handler.php
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,13 @@ public function loadContentInfoByRemoteId($remoteId);
*/
public function loadVersionInfo($contentId, $versionNo = null);

/**
* @throws \Ibexa\Contracts\Core\Repository\Exceptions\NotFoundException
*
* @return int[]
*/
public function loadVersionNoArchivedWithin(int $contentId, int $seconds): array;

/**
* Returns the number of versions with draft status created by the given $userId.
*
Expand Down
3 changes: 3 additions & 0 deletions src/lib/Base/Container/ApiLoader/RepositoryFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
use Ibexa\Contracts\Core\SiteAccess\ConfigResolverInterface;
use Ibexa\Core\Base\Exceptions\InvalidArgumentException;
use Ibexa\Core\FieldType\FieldTypeRegistry;
use Ibexa\Core\Repository\Collector\ContentCollector;
use Ibexa\Core\Repository\Helper\RelationProcessor;
use Ibexa\Core\Repository\Mapper;
use Ibexa\Core\Repository\Permission\LimitationService;
Expand Down Expand Up @@ -85,6 +86,7 @@ public function buildRepository(
PasswordValidatorInterface $passwordValidator,
ConfigResolverInterface $configResolver,
NameSchemaServiceInterface $nameSchemaService,
ContentCollector $contentCollector,
array $languages
): Repository {
return new $this->repositoryClass(
Expand All @@ -109,6 +111,7 @@ public function buildRepository(
$passwordValidator,
$configResolver,
$nameSchemaService,
$contentCollector,
[
'role' => [
'policyMap' => $this->policyMap,
Expand Down
8 changes: 8 additions & 0 deletions src/lib/Persistence/Cache/ContentHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,14 @@ public function loadVersionInfo($contentId, $versionNo = null)
return $versionInfo;
}

/**
* @return int[]
*/
public function loadVersionNoArchivedWithin(int $contentId, int $seconds): array
{
return $this->persistenceHandler->contentHandler()->loadVersionNoArchivedWithin($contentId, $seconds);
}

public function countDraftsForUser(int $userId): int
{
$this->logger->logCall(__METHOD__, ['user' => $userId]);
Expand Down
5 changes: 5 additions & 0 deletions src/lib/Persistence/Legacy/Content/Gateway.php
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,11 @@ abstract public function loadContentInfoList(array $contentIds): array;
*/
abstract public function loadVersionInfo(int $contentId, ?int $versionNo = null): array;

/**
* @return array<mixed>
*/
abstract public function loadVersionNoArchivedWithin(int $contentId, int $seconds): array;

/**
* Return the number of all versions with given status created by the given $userId.
*/
Expand Down
45 changes: 45 additions & 0 deletions src/lib/Persistence/Legacy/Content/Gateway/DoctrineDatabase.php
Original file line number Diff line number Diff line change
Expand Up @@ -910,6 +910,51 @@ public function loadVersionInfo(int $contentId, ?int $versionNo = null): array
return $queryBuilder->execute()->fetchAll(FetchMode::ASSOCIATIVE);
}

/**
* @return array<int,array<string,mixed>>
*/
public function loadVersionNoArchivedWithin(int $contentId, int $seconds): array
{
$cutoffTimestamp = time() - $seconds;
if ($cutoffTimestamp < 0) {
return [];
}
$queryBuilder = $this->queryBuilder->createVersionInfoFindQueryBuilder();
$expr = $queryBuilder->expr();

$queryBuilder
->andWhere(
$expr->eq(
'v.contentobject_id',
$queryBuilder->createNamedParameter(
$contentId,
ParameterType::INTEGER,
':content_id'
)
)
)->andWhere(
$expr->eq(
'v.status',
$queryBuilder->createNamedParameter(
VersionInfo::STATUS_ARCHIVED,
ParameterType::INTEGER,
':status'
)
)
)->andWhere(
$expr->gt(
'v.modified',
$queryBuilder->createNamedParameter(
$cutoffTimestamp,
ParameterType::INTEGER,
':modified'
)
)
)->orderBy('v.modified', 'DESC');

return $queryBuilder->execute()->fetchAllAssociative();
}

public function countVersionsForUser(int $userId, int $status = VersionInfo::STATUS_DRAFT): int
{
$query = $this->connection->createQueryBuilder();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
use Doctrine\DBAL\ParameterType;
use Doctrine\DBAL\Query\QueryBuilder as DoctrineQueryBuilder;
use Ibexa\Core\Persistence\Legacy\Content\Gateway;
use function time;

/**
* @internal For internal use by the Content gateway.
Expand Down
12 changes: 12 additions & 0 deletions src/lib/Persistence/Legacy/Content/Gateway/ExceptionConversion.php
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,18 @@ public function loadVersionInfo(int $contentId, ?int $versionNo = null): array
}
}

/**
* @return int[]
*/
public function loadVersionNoArchivedWithin(int $contentId, int $seconds): array
{
try {
return $this->innerGateway->loadVersionNoArchivedWithin($contentId, $seconds);
} catch (DBALException | PDOException $e) {
throw DatabaseException::wrap($e);
}
}

public function countVersionsForUser(int $userId, int $status = VersionInfo::STATUS_DRAFT): int
{
try {
Expand Down
15 changes: 15 additions & 0 deletions src/lib/Persistence/Legacy/Content/Handler.php
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,21 @@ public function loadVersionInfo($contentId, $versionNo = null)
return reset($versionInfo);
}

public function loadVersionNoArchivedWithin(int $contentId, int $seconds): array
{
$rows = $this->contentGateway->loadVersionNoArchivedWithin($contentId, $seconds);
if (empty($rows)) {
throw new NotFound('content', $contentId);
}

$archivedVersionNos = [];
foreach ($rows as $row) {
$archivedVersionNos[] = (int) $row['ezcontentobject_version_version'];
}

return $archivedVersionNos;
}

public function countDraftsForUser(int $userId): int
{
return $this->contentGateway->countVersionsForUser($userId, VersionInfo::STATUS_DRAFT);
Expand Down
36 changes: 36 additions & 0 deletions src/lib/Repository/Collector/ContentCollector.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
<?php

/**
* @copyright Copyright (C) Ibexa AS. All rights reserved.
* @license For full copyright and license information view LICENSE file distributed with this source code.
*/
declare(strict_types=1);

namespace Ibexa\Core\Repository\Collector;

use Ibexa\Contracts\Core\Repository\Values\Content\Content;
use Symfony\Contracts\Service\ResetInterface;

final class ContentCollector implements ResetInterface
{
/** @var array<int, bool> */
private array $contentMap = [];

public function collectContent(Content $content): void
{
$this->contentMap[$content->getId()] = false;
}

/**
* @return int[]
*/
public function getCollectedContentIds(): array
{
return array_keys($this->contentMap);
}

public function reset(): void
{
$this->contentMap = [];
}
}
Loading
Loading