From f95e454bb91223b4a13bc143e646467e71709c89 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Tue, 1 Sep 2026 17:14:46 +0400 Subject: [PATCH 01/24] ElasticsearchClient --- .env.dist | 8 ++ .github/workflows/ci.yml | 16 +++ README.md | 1 + composer.json | 3 +- config/parameters.yml | 8 ++ docs/ElasticsearchSearch.md | 111 ++++++++++++++++++ .../Client/ElasticsearchClientAdapter.php | 103 ++++++++++++++++ .../Client/ElasticsearchClientFactory.php | 32 +++++ .../Client/ElasticsearchClientInterface.php | 51 ++++++++ 9 files changed, 332 insertions(+), 1 deletion(-) create mode 100644 docs/ElasticsearchSearch.md create mode 100644 src/Domain/Search/Client/ElasticsearchClientAdapter.php create mode 100644 src/Domain/Search/Client/ElasticsearchClientFactory.php create mode 100644 src/Domain/Search/Client/ElasticsearchClientInterface.php diff --git a/.env.dist b/.env.dist index 03df0e91..e1cdc484 100644 --- a/.env.dist +++ b/.env.dist @@ -53,6 +53,14 @@ BOUNCE_IMAP_PURGE_UNPROCESSED=0 # Messenger configuration for asynchronous processing MESSENGER_TRANSPORT_DSN=doctrine://default?auto_setup=true +# Elasticsearch configuration +ELASTICSEARCH_HOSTS=http://127.0.0.1:9200 +ELASTICSEARCH_USERNAME= +ELASTICSEARCH_PASSWORD= +ELASTICSEARCH_INDEX_PREFIX=phplist_ +ELASTICSEARCH_CONNECT_TIMEOUT=2 +ELASTICSEARCH_REQUEST_TIMEOUT=5 + # A secret key that's used to generate certain security-related tokens PHPLIST_SECRET=%s VERIFY_SSL=1 diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 263488c7..86596523 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -19,6 +19,17 @@ jobs: ports: - 3306/tcp options: --health-cmd="mysqladmin ping" --health-interval=10s --health-timeout=5s --health-retries=3 + elasticsearch: + image: docker.elastic.co/elasticsearch/elasticsearch:8.15.0 + env: + discovery.type: single-node + xpack.security.enabled: false + ES_JAVA_OPTS: -Xms256m -Xmx256m + ports: + - 9200/tcp + options: >- + --health-cmd="curl -sf http://localhost:9200/_cluster/health || exit 1" + --health-interval=10s --health-timeout=5s --health-retries=5 strategy: fail-fast: false matrix: @@ -68,6 +79,10 @@ jobs: php bin/console doctrine:schema:validate --skip-sync - name: Run units tests with phpunit run: vendor/bin/phpunit tests/Unit/ --testdox + - name: Initialize Elasticsearch indices + run: | + export ELASTICSEARCH_HOSTS=http://127.0.0.1:${{ job.services.elasticsearch.ports['9200'] }} + php bin/console phplist:search:init-indices - name: Run integration tests with phpunit run: | export PHPLIST_DATABASE_NAME=${{ env.DB_DATABASE }} @@ -75,6 +90,7 @@ jobs: export PHPLIST_DATABASE_PASSWORD=${{ env.DB_PASSWORD }} export PHPLIST_DATABASE_PORT=${{ job.services.mysql.ports['3306'] }} export PHPLIST_DATABASE_HOST=127.0.0.1 + export ELASTICSEARCH_HOSTS=http://127.0.0.1:${{ job.services.elasticsearch.ports['9200'] }} vendor/bin/phpunit tests/Integration/ - name: Running the system tests run: vendor/bin/phpunit tests/System/ --testdox; diff --git a/README.md b/README.md index 4b929df2..aff4d2ee 100755 --- a/README.md +++ b/README.md @@ -57,6 +57,7 @@ this code. * [Mailer transports](docs/MailerTransports.md) - configuring Gmail, Amazon SES, Mailchimp, and SendGrid * [Asynchronous email sending](docs/AsyncEmailSending.md) - queuing email delivery with Symfony Messenger * [Graylog integration](docs/Graylog.md) - centralized log management +* [Elasticsearch-backed search for big tables](docs/ElasticsearchSearch.md) - dual-write to the database and Elasticsearch, reading from Elasticsearch only * [Generating class API docs](PHPDOC.md) - regenerating the phpDocumentor output ## Running the web server diff --git a/composer.json b/composer.json index d0371f58..5c6f9e05 100644 --- a/composer.json +++ b/composer.json @@ -89,7 +89,8 @@ "phpdocumentor/reflection-docblock": "^5.2", "guzzlehttp/guzzle": "^7.4.5", "symfony/dotenv": "^6.4", - "symfony/doctrine-messenger": "^6.4" + "symfony/doctrine-messenger": "^6.4", + "elasticsearch/elasticsearch": "^8.9" }, "require-dev": { "phpunit/phpunit": "^9.5", diff --git a/config/parameters.yml b/config/parameters.yml index aecc30ec..d413d2b8 100644 --- a/config/parameters.yml +++ b/config/parameters.yml @@ -51,6 +51,14 @@ parameters: # Messenger configuration for asynchronous processing app.messenger_transport_dsn: '%env(MESSENGER_TRANSPORT_DSN)%' + # Elasticsearch configuration + elasticsearch.hosts: '%env(csv:ELASTICSEARCH_HOSTS)%' + elasticsearch.username: '%env(ELASTICSEARCH_USERNAME)%' + elasticsearch.password: '%env(ELASTICSEARCH_PASSWORD)%' + elasticsearch.index_prefix: '%env(ELASTICSEARCH_INDEX_PREFIX)%' + elasticsearch.connect_timeout: '%env(int:ELASTICSEARCH_CONNECT_TIMEOUT)%' + elasticsearch.request_timeout: '%env(int:ELASTICSEARCH_REQUEST_TIMEOUT)%' + # A secret key that's used to generate certain security-related tokens secret: '%env(PHPLIST_SECRET)%' phplist.verify_ssl: '%env(VERIFY_SSL)%' diff --git a/docs/ElasticsearchSearch.md b/docs/ElasticsearchSearch.md new file mode 100644 index 00000000..f4f0bea5 --- /dev/null +++ b/docs/ElasticsearchSearch.md @@ -0,0 +1,111 @@ +# Elasticsearch-backed Search for Big Tables + +This document explains the generic Elasticsearch dual-write/read infrastructure and its first +consumer, `SubscriberHistory` (table `phplist_user_user_history`). + +## Overview + +Some tables grow too large for comfortable ad-hoc filtering/pagination straight off MySQL. For those +tables, phpList Core writes to the database **and** to Elasticsearch, but reads **only** from +Elasticsearch: + +- **Writes** stay exactly as they are today (Doctrine `persist()`/`remove()`). A generic Doctrine + event listener (`PhpList\Core\Core\Doctrine\SearchIndexDoctrineListener`) detects any entity that + implements `SearchIndexableInterface` and asynchronously dispatches an indexing/deletion message via + Symfony Messenger, once the surrounding transaction has actually committed. +- **Reads** for those entities go through a dedicated reader interface (e.g. + `SubscriberHistoryReaderInterface`) that is aliased in DI to an Elasticsearch-backed implementation + instead of the Doctrine repository. + +This is deliberately generic: adding the next big table only requires implementing three small +interfaces (see "Adding a new searchable entity" below) - no changes to the dual-write plumbing. + +## Consistency model + +- Dual-write is **asynchronous**. Between a row being committed to MySQL and the `async_search` worker + processing its queued message, a read from Elasticsearch will not yet reflect that row. +- Reads **hard-fail** if Elasticsearch is unreachable - there is no fallback to the database. Any + Elasticsearch error is raised as `PhpList\Core\Domain\Search\Exception\SearchBackendUnavailableException`. +- If a process crashes between the database transaction committing and the message being dispatched + (a narrow window - see `SearchIndexDoctrineListener`'s docblock for why the dispatch happens in + `postFlush`, not `postPersist`/`postUpdate`/`postRemove`), that one row is missed until the next + `phplist:search:reindex` run. Nothing is ever indexed for a row that was rolled back. + +Consumers of `phplist/core` that build UI on top of these read paths should plan for both of the above +(e.g. a brief "just added" staleness window, and handling a 5xx-equivalent from a search-unavailable +condition) rather than assuming synchronous consistency with the database. + +## Configuration + +Set in `.env` (see `.env.dist`): + +``` +ELASTICSEARCH_HOSTS=http://127.0.0.1:9200 +ELASTICSEARCH_USERNAME= +ELASTICSEARCH_PASSWORD= +ELASTICSEARCH_INDEX_PREFIX=phplist_ +ELASTICSEARCH_CONNECT_TIMEOUT=2 +ELASTICSEARCH_REQUEST_TIMEOUT=5 +``` + +`ELASTICSEARCH_HOSTS` accepts a comma-separated list for multi-node clusters. The index prefix is +applied to every logical index alias (e.g. alias `subscriber_history` becomes index +`phplist_subscriber_history` with the default prefix), the same convention as `DATABASE_PREFIX` for +MySQL tables. + +## Queueing + +Indexing/deletion messages are routed to a dedicated `async_search` Messenger transport +(`config/packages/messenger.yaml`) - the same Doctrine-backed queue table used by `async_email`, but a +distinct `queue_name` so the two workloads don't compete or block each other. Run a worker for it in +addition to the email worker: + +```bash +bin/console messenger:consume async_search +``` + +As with `async_email`, run this as a background service (e.g. via Supervisor) in production. +`auto_setup` is disabled for this transport: the `messenger_messages` table must already exist (it's +created lazily by `async_email` on first use). On a fresh install that enables search before ever +sending an email, run `bin/console messenger:setup-transports` once. + +## Console commands + +```bash +# Create or update Elasticsearch indices (mappings) for every registered searchable entity. +# Safe to re-run - never drops or recreates an existing index. +bin/console phplist:search:init-indices [--index=] + +# Backfill Elasticsearch from the database. Safe to re-run (indexing is an upsert by id). +bin/console phplist:search:reindex [] [--batch-size=500] [--last-id=0] +``` + +Run `phplist:search:init-indices` once per environment before the first `phplist:search:reindex`, and +again after adding a new searchable entity or changing a mapping. + +## Adding a new searchable entity + +1. Implement `PhpList\Core\Domain\Search\Model\Interfaces\SearchIndexableInterface` on the entity + (`getSearchIndexName()`, `getSearchDocumentId()`, `toSearchDocument()`). This is what makes + `SearchIndexDoctrineListener` dual-write it automatically - no other write-path changes needed. +2. Add an index definition (`SearchIndexDefinitionInterface`: alias + mapping + settings) under the + entity's own `Service/Search` folder, following + `PhpList\Core\Domain\Subscription\Service\Search\SubscriberHistoryIndexDefinition` - it's + auto-tagged and picked up by `phplist:search:init-indices` via DI `_instanceof` autoconfiguration + in `config/services/elasticsearch.yml`. +3. Add a reindex provider (`SearchReindexProviderInterface`: alias + `countAll()` + `fetchBatch()`) + following `SubscriberHistoryReindexProvider` - likewise auto-tagged, picked up by + `phplist:search:reindex`. +4. If reads should also move to Elasticsearch, introduce a reader interface for that entity (mirroring + `SubscriberHistoryReaderInterface`) and an Elasticsearch-backed implementation (mirroring + `SubscriberHistoryElasticsearchReader`), then alias the interface to it in DI instead of the + Doctrine repository. + +## Troubleshooting + +- **Reads throwing `SearchBackendUnavailableException`**: check Elasticsearch is reachable at + `ELASTICSEARCH_HOSTS` and that `phplist:search:init-indices` has been run. +- **New/updated rows not appearing in search results**: make sure a `messenger:consume async_search` + worker is running; check `bin/console messenger:failed:show` for stuck messages. +- **Data drifted between MySQL and Elasticsearch**: re-run `bin/console phplist:search:reindex ` + - it's a safe, idempotent full backfill. \ No newline at end of file diff --git a/src/Domain/Search/Client/ElasticsearchClientAdapter.php b/src/Domain/Search/Client/ElasticsearchClientAdapter.php new file mode 100644 index 00000000..d9240b06 --- /dev/null +++ b/src/Domain/Search/Client/ElasticsearchClientAdapter.php @@ -0,0 +1,103 @@ +call(function () use ($indexName, $documentId, $document): void { + $this->client->index([ + 'index' => $indexName, + 'id' => $documentId, + 'body' => $document, + ]); + }); + } + + public function delete(string $indexName, string $documentId): void + { + $this->call(function () use ($indexName, $documentId): void { + try { + $this->client->delete([ + 'index' => $indexName, + 'id' => $documentId, + ]); + } catch (ClientResponseException $exception) { + if ($exception->getCode() !== self::HTTP_NOT_FOUND) { + throw $exception; + } + } + }); + } + + public function indexExists(string $indexName): bool + { + return $this->call(fn (): bool => $this->client->indices()->exists(['index' => $indexName])->asBool()); + } + + public function createIndex(string $indexName, array $mapping, array $settings): void + { + $this->call(function () use ($indexName, $mapping, $settings): void { + $body = ['mappings' => $mapping]; + if ($settings !== []) { + $body['settings'] = $settings; + } + + $this->client->indices()->create([ + 'index' => $indexName, + 'body' => $body, + ]); + }); + } + + public function updateMapping(string $indexName, array $mapping): void + { + $this->call(function () use ($indexName, $mapping): void { + $this->client->indices()->putMapping([ + 'index' => $indexName, + 'body' => $mapping, + ]); + }); + } + + public function search(string $indexName, array $query): array + { + return $this->call(fn (): array => $this->client->search([ + 'index' => $indexName, + 'body' => $query, + ])->asArray()); + } + + /** + * @template T + * @param callable(): T $operation + * @return T + * @throws SearchBackendUnavailableException + */ + private function call(callable $operation): mixed + { + try { + return $operation(); + } catch (Throwable $exception) { + throw new SearchBackendUnavailableException( + 'Elasticsearch operation failed: ' . $exception->getMessage(), + 0, + $exception, + ); + } + } +} diff --git a/src/Domain/Search/Client/ElasticsearchClientFactory.php b/src/Domain/Search/Client/ElasticsearchClientFactory.php new file mode 100644 index 00000000..002f01a1 --- /dev/null +++ b/src/Domain/Search/Client/ElasticsearchClientFactory.php @@ -0,0 +1,32 @@ +setHosts($hosts); + + if (!empty($username)) { + $builder->setBasicAuthentication($username, $password ?? ''); + } + + $builder->setHttpClientOptions([ + 'connect_timeout' => $connectTimeout, + 'timeout' => $requestTimeout, + ]); + + return $builder->build(); + } +} diff --git a/src/Domain/Search/Client/ElasticsearchClientInterface.php b/src/Domain/Search/Client/ElasticsearchClientInterface.php new file mode 100644 index 00000000..ce7fcb13 --- /dev/null +++ b/src/Domain/Search/Client/ElasticsearchClientInterface.php @@ -0,0 +1,51 @@ + $document + * @throws SearchBackendUnavailableException + */ + public function index(string $indexName, string $documentId, array $document): void; + + /** + * Returns quietly (idempotent) if the document does not exist. + * @throws SearchBackendUnavailableException + */ + public function delete(string $indexName, string $documentId): void; + + /** @throws SearchBackendUnavailableException */ + public function indexExists(string $indexName): bool; + + /** + * @param array $mapping + * @param array $settings + * @throws SearchBackendUnavailableException + */ + public function createIndex(string $indexName, array $mapping, array $settings): void; + + /** + * @param array $mapping + * @throws SearchBackendUnavailableException + */ + public function updateMapping(string $indexName, array $mapping): void; + + /** + * @param array $query + * @return array Raw decoded ES response body. + * @throws SearchBackendUnavailableException + */ + public function search(string $indexName, array $query): array; +} From 76d0da86e7a8250992252115b8ead181724fa7d3 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Tue, 1 Sep 2026 17:26:09 +0400 Subject: [PATCH 02/24] IndexDocumentMessageHandler --- config/packages/messenger.yaml | 17 +++ config/services.yml | 2 + config/services/messenger.yml | 6 + .../Doctrine/SearchIndexDoctrineListener.php | 87 ++++++++++++ .../SearchBackendUnavailableException.php | 16 +++ .../Search/Message/IndexDocumentMessage.php | 45 ++++++ .../IndexDocumentMessageHandler.php | 33 +++++ .../Interfaces/SearchIndexableInterface.php | 20 +++ src/Domain/Search/Model/SearchOperation.php | 11 ++ .../Service/ElasticsearchIndexerInterface.php | 27 ++++ .../SearchIndexDoctrineListenerTest.php | 132 ++++++++++++++++++ .../IndexDocumentMessageHandlerTest.php | 51 +++++++ 12 files changed, 447 insertions(+) create mode 100644 src/Core/Doctrine/SearchIndexDoctrineListener.php create mode 100644 src/Domain/Search/Exception/SearchBackendUnavailableException.php create mode 100644 src/Domain/Search/Message/IndexDocumentMessage.php create mode 100644 src/Domain/Search/MessageHandler/IndexDocumentMessageHandler.php create mode 100644 src/Domain/Search/Model/Interfaces/SearchIndexableInterface.php create mode 100644 src/Domain/Search/Model/SearchOperation.php create mode 100644 src/Domain/Search/Service/ElasticsearchIndexerInterface.php create mode 100644 tests/Unit/Core/Doctrine/SearchIndexDoctrineListenerTest.php create mode 100644 tests/Unit/Domain/Search/MessageHandler/IndexDocumentMessageHandlerTest.php diff --git a/config/packages/messenger.yaml b/config/packages/messenger.yaml index 93022618..88d69297 100644 --- a/config/packages/messenger.yaml +++ b/config/packages/messenger.yaml @@ -21,6 +21,22 @@ framework: failed: 'doctrine://default?queue_name=failed' + # Dedicated queue (same Doctrine connection/table, distinct queue_name) for Elasticsearch + # dual-write. auto_setup is deliberately disabled: the messenger_messages table is already + # created by the async_email transport, and no DDL must ever run while a Doctrine ORM + # transaction is open (see SearchIndexDoctrineListener, which dispatches from postFlush). + # On a fresh install that enables search before ever sending email, run + # `bin/console messenger:setup-transports` once. + async_search: + dsn: 'doctrine://default?queue_name=search_index' + options: + auto_setup: false + retry_strategy: + max_retries: 5 + delay: 1000 + multiplier: 2 + max_delay: 30000 + routing: # Route your messages to the transports 'PhpList\Core\Domain\Messaging\Message\AsyncEmailMessage': async_email @@ -30,4 +46,5 @@ framework: 'PhpList\Core\Domain\Messaging\Message\CampaignProcessor\CampaignProcessorMessage': async_email 'PhpList\Core\Domain\Messaging\Message\CampaignProcessor\SyncCampaignProcessorMessage': sync 'PhpList\Core\Domain\Subscription\Message\DynamicTableMessage': sync + 'PhpList\Core\Domain\Search\Message\IndexDocumentMessage': async_search diff --git a/config/services.yml b/config/services.yml index 31a2b6f1..780df8ce 100644 --- a/config/services.yml +++ b/config/services.yml @@ -55,6 +55,8 @@ services: arguments: $tablePrefix: '%database_prefix%' + PhpList\Core\Core\Doctrine\SearchIndexDoctrineListener: ~ + HTMLPurifier_Config: class: HTMLPurifier_Config factory: [ 'HTMLPurifier_Config', 'createDefault' ] diff --git a/config/services/messenger.yml b/config/services/messenger.yml index 38130f6e..16e02ff4 100644 --- a/config/services/messenger.yml +++ b/config/services/messenger.yml @@ -11,6 +11,12 @@ services: resource: '../../src/Domain/Subscription/MessageHandler' tags: [ 'messenger.message_handler' ] + # Register Search message handlers (e.g., IndexDocumentMessageHandler) + PhpList\Core\Domain\Search\MessageHandler\: + autowire: true + resource: '../../src/Domain/Search/MessageHandler' + tags: [ 'messenger.message_handler' ] + PhpList\Core\Domain\Messaging\MessageHandler\CampaignProcessor\CampaignProcessorMessageHandler: autowire: true autoconfigure: true diff --git a/src/Core/Doctrine/SearchIndexDoctrineListener.php b/src/Core/Doctrine/SearchIndexDoctrineListener.php new file mode 100644 index 00000000..1fd394a8 --- /dev/null +++ b/src/Core/Doctrine/SearchIndexDoctrineListener.php @@ -0,0 +1,87 @@ + */ + private array $pending = []; + + public function __construct(private readonly MessageBusInterface $messageBus) + { + } + + public function postPersist(PostPersistEventArgs $args): void + { + $this->queue($args->getObject(), SearchOperation::Index); + } + + public function postUpdate(PostUpdateEventArgs $args): void + { + $this->queue($args->getObject(), SearchOperation::Index); + } + + public function postRemove(PostRemoveEventArgs $args): void + { + $this->queue($args->getObject(), SearchOperation::Delete); + } + + public function postFlush(PostFlushEventArgs $args): void + { + if ($this->pending === []) { + return; + } + + $messages = $this->pending; + $this->pending = []; + + foreach ($messages as $message) { + $this->messageBus->dispatch($message); + } + } + + private function queue(object $entity, SearchOperation $operation): void + { + if (!$entity instanceof SearchIndexableInterface) { + return; + } + + $key = $entity->getSearchIndexName() . '|' . $entity->getSearchDocumentId(); + $document = $operation === SearchOperation::Index ? $entity->toSearchDocument() : []; + + $this->pending[$key] = new IndexDocumentMessage( + $entity->getSearchIndexName(), + $entity->getSearchDocumentId(), + $document, + $operation, + ); + } +} diff --git a/src/Domain/Search/Exception/SearchBackendUnavailableException.php b/src/Domain/Search/Exception/SearchBackendUnavailableException.php new file mode 100644 index 00000000..da3dd01f --- /dev/null +++ b/src/Domain/Search/Exception/SearchBackendUnavailableException.php @@ -0,0 +1,16 @@ + $document */ + public function __construct( + private readonly string $indexName, + private readonly string $documentId, + private readonly array $document, + private readonly SearchOperation $operation, + ) { + } + + public function getIndexName(): string + { + return $this->indexName; + } + + public function getDocumentId(): string + { + return $this->documentId; + } + + /** @return array */ + public function getDocument(): array + { + return $this->document; + } + + public function getOperation(): SearchOperation + { + return $this->operation; + } +} diff --git a/src/Domain/Search/MessageHandler/IndexDocumentMessageHandler.php b/src/Domain/Search/MessageHandler/IndexDocumentMessageHandler.php new file mode 100644 index 00000000..24f85a96 --- /dev/null +++ b/src/Domain/Search/MessageHandler/IndexDocumentMessageHandler.php @@ -0,0 +1,33 @@ +getOperation()) { + SearchOperation::Index => $this->indexer->index( + $message->getIndexName(), + $message->getDocumentId(), + $message->getDocument(), + ), + SearchOperation::Delete => $this->indexer->delete( + $message->getIndexName(), + $message->getDocumentId(), + ), + }; + } +} diff --git a/src/Domain/Search/Model/Interfaces/SearchIndexableInterface.php b/src/Domain/Search/Model/Interfaces/SearchIndexableInterface.php new file mode 100644 index 00000000..275ca8f0 --- /dev/null +++ b/src/Domain/Search/Model/Interfaces/SearchIndexableInterface.php @@ -0,0 +1,20 @@ + */ + public function toSearchDocument(): array; +} diff --git a/src/Domain/Search/Model/SearchOperation.php b/src/Domain/Search/Model/SearchOperation.php new file mode 100644 index 00000000..3a905d26 --- /dev/null +++ b/src/Domain/Search/Model/SearchOperation.php @@ -0,0 +1,11 @@ + $document + * @throws SearchBackendUnavailableException + */ + public function index(string $indexAlias, string $documentId, array $document): void; + + /** @throws SearchBackendUnavailableException */ + public function delete(string $indexAlias, string $documentId): void; + + /** + * Creates the index with its mapping/settings if absent, otherwise applies the mapping + * non-destructively (never drops/recreates an existing index). + * @throws SearchBackendUnavailableException + */ + public function createOrUpdateIndex(SearchIndexDefinitionInterface $definition): void; +} diff --git a/tests/Unit/Core/Doctrine/SearchIndexDoctrineListenerTest.php b/tests/Unit/Core/Doctrine/SearchIndexDoctrineListenerTest.php new file mode 100644 index 00000000..889cb318 --- /dev/null +++ b/tests/Unit/Core/Doctrine/SearchIndexDoctrineListenerTest.php @@ -0,0 +1,132 @@ +messageBus = $this->createMock(MessageBusInterface::class); + $this->objectManager = $this->createMock(EntityManagerInterface::class); + $this->listener = new SearchIndexDoctrineListener($this->messageBus); + } + + private function createIndexable(string $indexName, string $documentId, array $document): SearchIndexableInterface + { + $entity = $this->createMock(SearchIndexableInterface::class); + $entity->method('getSearchIndexName')->willReturn($indexName); + $entity->method('getSearchDocumentId')->willReturn($documentId); + $entity->method('toSearchDocument')->willReturn($document); + + return $entity; + } + + public function testPostPersistDoesNotDispatchBeforePostFlush(): void + { + $entity = $this->createIndexable('subscriber_history', '1', ['id' => 1]); + + $this->messageBus->expects($this->never())->method('dispatch'); + + $this->listener->postPersist(new PostPersistEventArgs($entity, $this->objectManager)); + } + + public function testPostFlushDispatchesBufferedIndexMessage(): void + { + $entity = $this->createIndexable('subscriber_history', '1', ['id' => 1]); + + $this->messageBus + ->expects($this->once()) + ->method('dispatch') + ->with($this->callback(function (IndexDocumentMessage $message): bool { + return $message->getIndexName() === 'subscriber_history' + && $message->getDocumentId() === '1' + && $message->getDocument() === ['id' => 1] + && $message->getOperation() === SearchOperation::Index; + })) + ->willReturn(new Envelope(new stdClass())); + + $this->listener->postPersist(new PostPersistEventArgs($entity, $this->objectManager)); + $this->listener->postFlush(new PostFlushEventArgs($this->objectManager)); + } + + public function testPostRemoveBuffersDeleteOperationWithEmptyDocument(): void + { + $entity = $this->createIndexable('subscriber_history', '1', ['id' => 1]); + + $this->messageBus + ->expects($this->once()) + ->method('dispatch') + ->with($this->callback(function (IndexDocumentMessage $message): bool { + return $message->getOperation() === SearchOperation::Delete + && $message->getDocument() === []; + })) + ->willReturn(new Envelope(new stdClass())); + + $this->listener->postRemove(new PostRemoveEventArgs($entity, $this->objectManager)); + $this->listener->postFlush(new PostFlushEventArgs($this->objectManager)); + } + + public function testMultipleTouchesInOneFlushDedupeToOneDispatch(): void + { + $entity = $this->createIndexable('subscriber_history', '1', ['id' => 1]); + + $this->messageBus->expects($this->once())->method('dispatch') + ->willReturn(new Envelope(new stdClass())); + + $this->listener->postPersist(new PostPersistEventArgs($entity, $this->objectManager)); + $this->listener->postUpdate(new PostUpdateEventArgs($entity, $this->objectManager)); + $this->listener->postFlush(new PostFlushEventArgs($this->objectManager)); + } + + public function testPostFlushWithNothingBufferedDoesNotDispatch(): void + { + $this->messageBus->expects($this->never())->method('dispatch'); + + $this->listener->postFlush(new PostFlushEventArgs($this->objectManager)); + } + + public function testNonSearchIndexableEntityIsIgnored(): void + { + $entity = new stdClass(); + + $this->messageBus->expects($this->never())->method('dispatch'); + + $this->listener->postPersist(new PostPersistEventArgs($entity, $this->objectManager)); + $this->listener->postFlush(new PostFlushEventArgs($this->objectManager)); + } + + public function testPendingBufferIsClearedAfterDispatch(): void + { + $entity = $this->createIndexable('subscriber_history', '1', ['id' => 1]); + + $this->messageBus->expects($this->once())->method('dispatch') + ->willReturn(new Envelope(new stdClass())); + + $this->listener->postPersist(new PostPersistEventArgs($entity, $this->objectManager)); + $this->listener->postFlush(new PostFlushEventArgs($this->objectManager)); + + // A second postFlush with nothing new queued must not re-dispatch the same message. + $this->listener->postFlush(new PostFlushEventArgs($this->objectManager)); + } +} diff --git a/tests/Unit/Domain/Search/MessageHandler/IndexDocumentMessageHandlerTest.php b/tests/Unit/Domain/Search/MessageHandler/IndexDocumentMessageHandlerTest.php new file mode 100644 index 00000000..d9a469da --- /dev/null +++ b/tests/Unit/Domain/Search/MessageHandler/IndexDocumentMessageHandlerTest.php @@ -0,0 +1,51 @@ +indexer = $this->createMock(ElasticsearchIndexerInterface::class); + $this->handler = new IndexDocumentMessageHandler($this->indexer); + } + + public function testInvokeIndexesOnIndexOperation(): void + { + $document = ['id' => 1, 'summary' => 'hello']; + $message = new IndexDocumentMessage('subscriber_history', '1', $document, SearchOperation::Index); + + $this->indexer + ->expects($this->once()) + ->method('index') + ->with('subscriber_history', '1', $document); + $this->indexer->expects($this->never())->method('delete'); + + ($this->handler)($message); + } + + public function testInvokeDeletesOnDeleteOperation(): void + { + $message = new IndexDocumentMessage('subscriber_history', '1', [], SearchOperation::Delete); + + $this->indexer + ->expects($this->once()) + ->method('delete') + ->with('subscriber_history', '1'); + $this->indexer->expects($this->never())->method('index'); + + ($this->handler)($message); + } +} From 18054712e3655691cdf85bf56a0be6f58bd51e87 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Tue, 1 Sep 2026 17:54:11 +0400 Subject: [PATCH 03/24] ElasticsearchIndexer --- config/services/elasticsearch.yml | 47 ++++++++++ .../SearchIndexDefinitionInterface.php | 21 +++++ .../SearchReindexProviderInterface.php | 19 ++++ .../SearchIndexDefinitionRegistry.php | 36 ++++++++ .../SearchReindexProviderRegistry.php | 36 ++++++++ .../Search/Service/ElasticsearchIndexer.php | 45 ++++++++++ .../Service/ElasticsearchIndexerTest.php | 87 +++++++++++++++++++ 7 files changed, 291 insertions(+) create mode 100644 config/services/elasticsearch.yml create mode 100644 src/Domain/Search/Model/Interfaces/SearchIndexDefinitionInterface.php create mode 100644 src/Domain/Search/Model/Interfaces/SearchReindexProviderInterface.php create mode 100644 src/Domain/Search/Registry/SearchIndexDefinitionRegistry.php create mode 100644 src/Domain/Search/Registry/SearchReindexProviderRegistry.php create mode 100644 src/Domain/Search/Service/ElasticsearchIndexer.php create mode 100644 tests/Unit/Domain/Search/Service/ElasticsearchIndexerTest.php diff --git a/config/services/elasticsearch.yml b/config/services/elasticsearch.yml new file mode 100644 index 00000000..9d49aaf1 --- /dev/null +++ b/config/services/elasticsearch.yml @@ -0,0 +1,47 @@ +services: + _defaults: + autowire: true + autoconfigure: true + public: false + + _instanceof: + PhpList\Core\Domain\Search\Model\Interfaces\SearchIndexDefinitionInterface: + tags: ['phplist.search_index_definition'] + PhpList\Core\Domain\Search\Model\Interfaces\SearchReindexProviderInterface: + tags: ['phplist.search_reindex_provider'] + + Elastic\Elasticsearch\Client: + factory: ['PhpList\Core\Domain\Search\Client\ElasticsearchClientFactory', 'create'] + arguments: + $hosts: '%elasticsearch.hosts%' + $username: '%elasticsearch.username%' + $password: '%elasticsearch.password%' + $connectTimeout: '%elasticsearch.connect_timeout%' + $requestTimeout: '%elasticsearch.request_timeout%' + + PhpList\Core\Domain\Search\Client\ElasticsearchClientInterface: + alias: PhpList\Core\Domain\Search\Client\ElasticsearchClientAdapter + + PhpList\Core\Domain\Search\Client\ElasticsearchClientAdapter: ~ + + PhpList\Core\Domain\Search\Service\ElasticsearchIndexerInterface: + alias: PhpList\Core\Domain\Search\Service\ElasticsearchIndexer + + PhpList\Core\Domain\Search\Service\ElasticsearchIndexer: + arguments: + $indexPrefix: '%elasticsearch.index_prefix%' + + PhpList\Core\Domain\Search\Registry\SearchIndexDefinitionRegistry: + arguments: + $definitions: !tagged_iterator 'phplist.search_index_definition' + + PhpList\Core\Domain\Search\Registry\SearchReindexProviderRegistry: + arguments: + $providers: !tagged_iterator 'phplist.search_reindex_provider' + +# PhpList\Core\Domain\Subscription\Repository\SubscriberHistoryElasticsearchReader: +# arguments: +# $indexPrefix: '%elasticsearch.index_prefix%' + +# PhpList\Core\Domain\Subscription\Service\Search\: +# resource: '../../src/Domain/Subscription/Service/Search' diff --git a/src/Domain/Search/Model/Interfaces/SearchIndexDefinitionInterface.php b/src/Domain/Search/Model/Interfaces/SearchIndexDefinitionInterface.php new file mode 100644 index 00000000..4d7df679 --- /dev/null +++ b/src/Domain/Search/Model/Interfaces/SearchIndexDefinitionInterface.php @@ -0,0 +1,21 @@ + */ + public function getMapping(): array; + + /** @return array */ + public function getSettings(): array; +} diff --git a/src/Domain/Search/Model/Interfaces/SearchReindexProviderInterface.php b/src/Domain/Search/Model/Interfaces/SearchReindexProviderInterface.php new file mode 100644 index 00000000..a8bcf955 --- /dev/null +++ b/src/Domain/Search/Model/Interfaces/SearchReindexProviderInterface.php @@ -0,0 +1,19 @@ + */ + public function fetchBatch(int $lastId, int $batchSize): iterable; +} diff --git a/src/Domain/Search/Registry/SearchIndexDefinitionRegistry.php b/src/Domain/Search/Registry/SearchIndexDefinitionRegistry.php new file mode 100644 index 00000000..2c9e85bc --- /dev/null +++ b/src/Domain/Search/Registry/SearchIndexDefinitionRegistry.php @@ -0,0 +1,36 @@ + $definitions */ + public function __construct(iterable $definitions) + { + $this->definitions = $definitions instanceof \Traversable ? iterator_to_array($definitions) : $definitions; + } + + /** @return SearchIndexDefinitionInterface[] */ + public function getAll(): array + { + return $this->definitions; + } + + public function find(string $alias): ?SearchIndexDefinitionInterface + { + foreach ($this->definitions as $definition) { + if ($definition->getIndexAlias() === $alias) { + return $definition; + } + } + + return null; + } +} diff --git a/src/Domain/Search/Registry/SearchReindexProviderRegistry.php b/src/Domain/Search/Registry/SearchReindexProviderRegistry.php new file mode 100644 index 00000000..6f8a1c3c --- /dev/null +++ b/src/Domain/Search/Registry/SearchReindexProviderRegistry.php @@ -0,0 +1,36 @@ + $providers */ + public function __construct(iterable $providers) + { + $this->providers = $providers instanceof \Traversable ? iterator_to_array($providers) : $providers; + } + + /** @return SearchReindexProviderInterface[] */ + public function getAll(): array + { + return $this->providers; + } + + public function find(string $alias): ?SearchReindexProviderInterface + { + foreach ($this->providers as $provider) { + if ($provider->getAlias() === $alias) { + return $provider; + } + } + + return null; + } +} diff --git a/src/Domain/Search/Service/ElasticsearchIndexer.php b/src/Domain/Search/Service/ElasticsearchIndexer.php new file mode 100644 index 00000000..cc33ae89 --- /dev/null +++ b/src/Domain/Search/Service/ElasticsearchIndexer.php @@ -0,0 +1,45 @@ +client->index($this->resolvePhysicalIndexName($indexAlias), $documentId, $document); + } + + public function delete(string $indexAlias, string $documentId): void + { + $this->client->delete($this->resolvePhysicalIndexName($indexAlias), $documentId); + } + + public function createOrUpdateIndex(SearchIndexDefinitionInterface $definition): void + { + $indexName = $this->resolvePhysicalIndexName($definition->getIndexAlias()); + + if ($this->client->indexExists($indexName)) { + $this->client->updateMapping($indexName, $definition->getMapping()); + + return; + } + + $this->client->createIndex($indexName, $definition->getMapping(), $definition->getSettings()); + } + + private function resolvePhysicalIndexName(string $indexAlias): string + { + return $this->indexPrefix . $indexAlias; + } +} diff --git a/tests/Unit/Domain/Search/Service/ElasticsearchIndexerTest.php b/tests/Unit/Domain/Search/Service/ElasticsearchIndexerTest.php new file mode 100644 index 00000000..b293a872 --- /dev/null +++ b/tests/Unit/Domain/Search/Service/ElasticsearchIndexerTest.php @@ -0,0 +1,87 @@ +client = $this->createMock(ElasticsearchClientInterface::class); + $this->indexer = new ElasticsearchIndexer($this->client, 'phplist_'); + } + + public function testIndexAppliesIndexPrefix(): void + { + $this->client + ->expects($this->once()) + ->method('index') + ->with('phplist_subscriber_history', '42', ['id' => 42]); + + $this->indexer->index('subscriber_history', '42', ['id' => 42]); + } + + public function testDeleteAppliesIndexPrefix(): void + { + $this->client + ->expects($this->once()) + ->method('delete') + ->with('phplist_subscriber_history', '42'); + + $this->indexer->delete('subscriber_history', '42'); + } + + public function testCreateOrUpdateIndexCreatesWhenAbsent(): void + { + $definition = $this->createMock(SearchIndexDefinitionInterface::class); + $definition->method('getIndexAlias')->willReturn('subscriber_history'); + $definition->method('getMapping')->willReturn(['properties' => ['id' => ['type' => 'keyword']]]); + $definition->method('getSettings')->willReturn([]); + + $this->client + ->expects($this->once()) + ->method('indexExists') + ->with('phplist_subscriber_history') + ->willReturn(false); + + $this->client + ->expects($this->once()) + ->method('createIndex') + ->with('phplist_subscriber_history', $definition->getMapping(), []); + $this->client->expects($this->never())->method('updateMapping'); + + $this->indexer->createOrUpdateIndex($definition); + } + + public function testCreateOrUpdateIndexUpdatesMappingWhenPresent(): void + { + $definition = $this->createMock(SearchIndexDefinitionInterface::class); + $definition->method('getIndexAlias')->willReturn('subscriber_history'); + $definition->method('getMapping')->willReturn(['properties' => ['id' => ['type' => 'keyword']]]); + $definition->method('getSettings')->willReturn([]); + + $this->client + ->expects($this->once()) + ->method('indexExists') + ->with('phplist_subscriber_history') + ->willReturn(true); + + $this->client + ->expects($this->once()) + ->method('updateMapping') + ->with('phplist_subscriber_history', $definition->getMapping()); + $this->client->expects($this->never())->method('createIndex'); + + $this->indexer->createOrUpdateIndex($definition); + } +} From 09b27a1972f377daddba4bdb5983f077f5ac3004 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Tue, 1 Sep 2026 18:24:54 +0400 Subject: [PATCH 04/24] ReindexSearchCommand --- config/services/commands.yml | 4 + .../Command/InitSearchIndicesCommand.php | 63 +++++++++++ .../Search/Command/ReindexSearchCommand.php | 106 ++++++++++++++++++ 3 files changed, 173 insertions(+) create mode 100644 src/Domain/Search/Command/InitSearchIndicesCommand.php create mode 100644 src/Domain/Search/Command/ReindexSearchCommand.php diff --git a/config/services/commands.yml b/config/services/commands.yml index d9305748..32b431f6 100644 --- a/config/services/commands.yml +++ b/config/services/commands.yml @@ -12,6 +12,10 @@ services: resource: '../../src/Domain/Identity/Command' tags: ['console.command'] + PhpList\Core\Domain\Search\Command\: + resource: '../../src/Domain/Search/Command' + tags: ['console.command'] + PhpList\Core\Domain\Messaging\Command\ProcessBouncesCommand: arguments: $protocolProcessors: !tagged_iterator 'phplist.bounce_protocol_processor' diff --git a/src/Domain/Search/Command/InitSearchIndicesCommand.php b/src/Domain/Search/Command/InitSearchIndicesCommand.php new file mode 100644 index 00000000..a3e5e6dc --- /dev/null +++ b/src/Domain/Search/Command/InitSearchIndicesCommand.php @@ -0,0 +1,63 @@ +addOption( + 'index', + null, + InputOption::VALUE_REQUIRED, + 'Only create/update the index for this alias (e.g. subscriber_history)', + ); + } + + protected function execute(InputInterface $input, OutputInterface $output): int + { + $io = new SymfonyStyle($input, $output); + $alias = $input->getOption('index'); + + $definitions = $alias !== null + ? array_filter([$this->registry->find($alias)]) + : $this->registry->getAll(); + + if ($definitions === []) { + $io->warning($alias !== null + ? sprintf('No index definition registered for alias "%s".', $alias) + : 'No index definitions registered.'); + + return Command::SUCCESS; + } + + foreach ($definitions as $definition) { + $this->indexer->createOrUpdateIndex($definition); + $io->writeln(sprintf('OK %s', $definition->getIndexAlias())); + } + + return Command::SUCCESS; + } +} diff --git a/src/Domain/Search/Command/ReindexSearchCommand.php b/src/Domain/Search/Command/ReindexSearchCommand.php new file mode 100644 index 00000000..f9e6a40c --- /dev/null +++ b/src/Domain/Search/Command/ReindexSearchCommand.php @@ -0,0 +1,106 @@ +addArgument('alias', InputArgument::OPTIONAL, 'Reindex only this alias (default: all registered)') + ->addOption('batch-size', null, InputOption::VALUE_REQUIRED, 'Rows per batch', self::DEFAULT_BATCH_SIZE) + ->addOption( + 'last-id', + null, + InputOption::VALUE_REQUIRED, + 'Resume from this id (only meaningful with a single alias)', + 0, + ); + } + + protected function execute(InputInterface $input, OutputInterface $output): int + { + $io = new SymfonyStyle($input, $output); + $alias = $input->getArgument('alias'); + $batchSize = (int) $input->getOption('batch-size'); + $lastId = (int) $input->getOption('last-id'); + + $providers = $alias !== null + ? array_filter([$this->registry->find($alias)]) + : $this->registry->getAll(); + + if ($providers === []) { + $io->warning($alias !== null + ? sprintf('No reindex provider registered for alias "%s".', $alias) + : 'No reindex providers registered.'); + + return Command::SUCCESS; + } + + foreach ($providers as $provider) { + $this->reindexProvider($provider, $lastId, $batchSize, $io); + } + + return Command::SUCCESS; + } + + private function reindexProvider( + SearchReindexProviderInterface $provider, + int $lastId, + int $batchSize, + SymfonyStyle $io, + ): void { + $total = $provider->countAll(); + $io->writeln(sprintf('%s: %d rows total', $provider->getAlias(), $total)); + $progressBar = $io->createProgressBar($total); + + $indexed = 0; + do { + $batch = $provider->fetchBatch($lastId, $batchSize); + $countInBatch = 0; + + foreach ($batch as $entity) { + $this->indexer->index( + $entity->getSearchIndexName(), + $entity->getSearchDocumentId(), + $entity->toSearchDocument(), + ); + $lastId = (int) $entity->getSearchDocumentId(); + $countInBatch++; + $indexed++; + } + + $progressBar->setProgress($indexed); + } while ($countInBatch >= $batchSize); + + $progressBar->finish(); + $io->newLine(2); + $io->success(sprintf('%s: indexed %d rows.', $provider->getAlias(), $indexed)); + } +} From 2286bc0446de808871b26a240bf87db8c73eecc8 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Wed, 2 Sep 2026 09:57:44 +0400 Subject: [PATCH 05/24] SubscriberHistoryElasticsearch --- config/services/elasticsearch.yml | 10 +- config/services/repositories.yml | 5 + src/Domain/Common/Model/PaginatedResult.php | 2 +- .../SubscriberHistoryRecordInterface.php | 33 +++++ .../ReadModel/SubscriberHistoryReadModel.php | 58 +++++++++ src/Domain/Subscription/Model/Subscriber.php | 7 +- .../Subscription/Model/SubscriberHistory.php | 40 +++++- .../SubscriberHistoryReaderInterface.php | 24 ++++ .../SubscriberHistoryElasticsearchReader.php | 116 ++++++++++++++++++ .../SubscriberHistoryRepository.php | 5 +- .../Manager/SubscriberHistoryManager.php | 13 +- .../Service/Manager/SubscriberManager.php | 6 +- .../SubscriberHistoryIndexDefinition.php | 43 +++++++ .../SubscriberHistoryReindexProvider.php | 39 ++++++ ...bscriberHistoryElasticsearchReaderTest.php | 96 +++++++++++++++ .../Manager/SubscriberHistoryManagerTest.php | 8 +- .../Service/Manager/SubscriberManagerTest.php | 4 +- .../SubscriberHistoryIndexDefinitionTest.php | 37 ++++++ .../SubscriberHistoryReindexProviderTest.php | 19 +++ 19 files changed, 539 insertions(+), 26 deletions(-) create mode 100644 src/Domain/Subscription/Model/Interfaces/SubscriberHistoryRecordInterface.php create mode 100644 src/Domain/Subscription/Model/ReadModel/SubscriberHistoryReadModel.php create mode 100644 src/Domain/Subscription/Repository/Interfaces/SubscriberHistoryReaderInterface.php create mode 100644 src/Domain/Subscription/Repository/SubscriberHistoryElasticsearchReader.php create mode 100644 src/Domain/Subscription/Service/Search/SubscriberHistoryIndexDefinition.php create mode 100644 src/Domain/Subscription/Service/Search/SubscriberHistoryReindexProvider.php create mode 100644 tests/Unit/Domain/Subscription/Repository/SubscriberHistoryElasticsearchReaderTest.php create mode 100644 tests/Unit/Domain/Subscription/Service/Search/SubscriberHistoryIndexDefinitionTest.php create mode 100644 tests/Unit/Domain/Subscription/Service/Search/SubscriberHistoryReindexProviderTest.php diff --git a/config/services/elasticsearch.yml b/config/services/elasticsearch.yml index 9d49aaf1..c59a7463 100644 --- a/config/services/elasticsearch.yml +++ b/config/services/elasticsearch.yml @@ -39,9 +39,9 @@ services: arguments: $providers: !tagged_iterator 'phplist.search_reindex_provider' -# PhpList\Core\Domain\Subscription\Repository\SubscriberHistoryElasticsearchReader: -# arguments: -# $indexPrefix: '%elasticsearch.index_prefix%' + PhpList\Core\Domain\Subscription\Repository\SubscriberHistoryElasticsearchReader: + arguments: + $indexPrefix: '%elasticsearch.index_prefix%' -# PhpList\Core\Domain\Subscription\Service\Search\: -# resource: '../../src/Domain/Subscription/Service/Search' + PhpList\Core\Domain\Subscription\Service\Search\: + resource: '../../src/Domain/Subscription/Service/Search' diff --git a/config/services/repositories.yml b/config/services/repositories.yml index a0650b35..20ed2b79 100644 --- a/config/services/repositories.yml +++ b/config/services/repositories.yml @@ -83,6 +83,11 @@ services: parent: PhpList\Core\Domain\Common\Repository\AbstractRepository arguments: - PhpList\Core\Domain\Subscription\Model\SubscriberHistory + + # Reads for SubscriberHistoryManager/SubscriberManager come only from Elasticsearch - swap this + # alias to SubscriberHistoryRepository to read from the database instead. + PhpList\Core\Domain\Subscription\Repository\Interfaces\SubscriberHistoryReaderInterface: + alias: PhpList\Core\Domain\Subscription\Repository\SubscriberHistoryElasticsearchReader PhpList\Core\Domain\Subscription\Repository\UserBlacklistRepository: parent: PhpList\Core\Domain\Common\Repository\AbstractRepository arguments: diff --git a/src/Domain/Common/Model/PaginatedResult.php b/src/Domain/Common/Model/PaginatedResult.php index 83eec8f2..aa9bb1b7 100644 --- a/src/Domain/Common/Model/PaginatedResult.php +++ b/src/Domain/Common/Model/PaginatedResult.php @@ -6,7 +6,7 @@ use PhpList\Core\Domain\Common\Model\Interfaces\DomainModel; -/** @template T of DomainModel */ +/** @template-covariant T of DomainModel */ class PaginatedResult { /** @var list */ diff --git a/src/Domain/Subscription/Model/Interfaces/SubscriberHistoryRecordInterface.php b/src/Domain/Subscription/Model/Interfaces/SubscriberHistoryRecordInterface.php new file mode 100644 index 00000000..48292f55 --- /dev/null +++ b/src/Domain/Subscription/Model/Interfaces/SubscriberHistoryRecordInterface.php @@ -0,0 +1,33 @@ +id; + } + + public function getSubscriberId(): ?int + { + return $this->subscriberId; + } + + public function getIp(): ?string + { + return $this->ip; + } + + public function getCreatedAt(): ?DateTime + { + return $this->createdAt; + } + + public function getSummary(): ?string + { + return $this->summary; + } + + public function getDetail(): ?string + { + return $this->detail; + } + + public function getSystemInfo(): ?string + { + return $this->systemInfo; + } +} diff --git a/src/Domain/Subscription/Model/Subscriber.php b/src/Domain/Subscription/Model/Subscriber.php index 53299540..84bbb1f8 100644 --- a/src/Domain/Subscription/Model/Subscriber.php +++ b/src/Domain/Subscription/Model/Subscriber.php @@ -12,6 +12,7 @@ use PhpList\Core\Domain\Common\Model\Interfaces\DomainModel; use PhpList\Core\Domain\Common\Model\Interfaces\Identity; use PhpList\Core\Domain\Common\Model\Interfaces\ModificationDate; +use PhpList\Core\Domain\Subscription\Model\Interfaces\SubscriberHistoryRecordInterface; use PhpList\Core\Domain\Subscription\Repository\SubscriberRepository; /** @@ -114,7 +115,7 @@ class Subscriber implements DomainModel, Identity, CreationDate, ModificationDat #[ORM\Column(name: 'foreignkey', type: 'string', length: 100, nullable: true)] private ?string $foreignKey = null; - /** @var SubscriberHistory[] */ + /** @var SubscriberHistoryRecordInterface[] */ private array $history = []; public function __construct(string $email) @@ -378,7 +379,7 @@ public function setForeignKey(?string $foreignKey): void } /** - * @return SubscriberHistory[] + * @return SubscriberHistoryRecordInterface[] */ public function getHistory(): array { @@ -386,7 +387,7 @@ public function getHistory(): array } /** - * @param SubscriberHistory[] $history + * @param SubscriberHistoryRecordInterface[] $history */ public function setHistory(array $history): void { diff --git a/src/Domain/Subscription/Model/SubscriberHistory.php b/src/Domain/Subscription/Model/SubscriberHistory.php index 08f4f974..c4b22084 100644 --- a/src/Domain/Subscription/Model/SubscriberHistory.php +++ b/src/Domain/Subscription/Model/SubscriberHistory.php @@ -5,17 +5,26 @@ namespace PhpList\Core\Domain\Subscription\Model; use DateTime; +use DateTimeInterface; use Doctrine\ORM\Mapping as ORM; use PhpList\Core\Domain\Common\Model\Interfaces\DomainModel; use PhpList\Core\Domain\Common\Model\Interfaces\Identity; +use PhpList\Core\Domain\Search\Model\Interfaces\SearchIndexableInterface; +use PhpList\Core\Domain\Subscription\Model\Interfaces\SubscriberHistoryRecordInterface; use PhpList\Core\Domain\Subscription\Repository\SubscriberHistoryRepository; #[ORM\Entity(repositoryClass: SubscriberHistoryRepository::class)] #[ORM\Table(name: 'user_user_history')] #[ORM\Index(name: 'phplist_user_user_history_dateidx', columns: ['date'])] #[ORM\Index(name: 'phplist_user_user_history_userididx', columns: ['userid'])] -class SubscriberHistory implements DomainModel, Identity +class SubscriberHistory implements + DomainModel, + Identity, + SearchIndexableInterface, + SubscriberHistoryRecordInterface { + private const SEARCH_INDEX_NAME = 'subscriber_history'; + #[ORM\Id] #[ORM\Column(type: 'integer')] #[ORM\GeneratedValue] @@ -56,6 +65,11 @@ public function getSubscriber(): Subscriber return $this->subscriber; } + public function getSubscriberId(): ?int + { + return $this->subscriber->getId(); + } + public function getIp(): ?string { return $this->ip; @@ -110,4 +124,28 @@ public function setSystemInfo(?string $systemInfo): self $this->systemInfo = $systemInfo; return $this; } + + public function getSearchIndexName(): string + { + return self::SEARCH_INDEX_NAME; + } + + public function getSearchDocumentId(): string + { + return (string) $this->id; + } + + public function toSearchDocument(): array + { + return [ + 'id' => $this->id, + 'idSort' => $this->id, + 'subscriberId' => $this->getSubscriberId(), + 'ip' => $this->ip, + 'date' => $this->createdAt?->format(DateTimeInterface::ATOM), + 'summary' => $this->summary, + 'detail' => $this->detail, + 'systemInfo' => $this->systemInfo, + ]; + } } diff --git a/src/Domain/Subscription/Repository/Interfaces/SubscriberHistoryReaderInterface.php b/src/Domain/Subscription/Repository/Interfaces/SubscriberHistoryReaderInterface.php new file mode 100644 index 00000000..3b86e400 --- /dev/null +++ b/src/Domain/Subscription/Repository/Interfaces/SubscriberHistoryReaderInterface.php @@ -0,0 +1,24 @@ + */ + public function getFilteredAfterId(FilterRequestInterface $filter): PaginatedResult; + + /** @return SubscriberHistoryRecordInterface[] */ + public function getBySubscriber(Subscriber $subscriber): array; +} diff --git a/src/Domain/Subscription/Repository/SubscriberHistoryElasticsearchReader.php b/src/Domain/Subscription/Repository/SubscriberHistoryElasticsearchReader.php new file mode 100644 index 00000000..a3fc0da3 --- /dev/null +++ b/src/Domain/Subscription/Repository/SubscriberHistoryElasticsearchReader.php @@ -0,0 +1,116 @@ +getSubscriber() !== null) { + $mustFilters[] = ['term' => ['subscriberId' => $filter->getSubscriber()->getId()]]; + } + + if ($filter->getDateFrom() !== null) { + $mustFilters[] = ['range' => ['date' => ['gte' => $filter->getDateFrom()->format(DATE_ATOM)]]]; + } + + if ($filter->getIp() !== null) { + $mustFilters[] = ['term' => ['ip' => $filter->getIp()]]; + } + + if ($filter->getSummery() !== null) { + $mustFilters[] = ['term' => ['summary.keyword' => $filter->getSummery()]]; + } + + $mustFilters[] = ['range' => ['idSort' => ['gt' => $filter->getLastId()]]]; + + $response = $this->client->search( + indexname: $this->resolvePhysicalIndexName(), + query: [ + 'query' => ['bool' => ['filter' => $mustFilters]], + 'sort' => [['idSort' => 'asc']], + 'size' => $filter->getLimit(), + 'track_total_hits' => true, + ], + ); + + $hits = $response['hits']['hits'] ?? []; + + return new PaginatedResult( + items: array_map($this->hydrate(...), $hits), + total: (int) ($response['hits']['total']['value'] ?? 0), + limit: $filter->getLimit(), + lastId: $filter->getLastId(), + ); + } + + /** @return SubscriberHistoryRecordInterface[] */ + public function getBySubscriber(Subscriber $subscriber): array + { + $response = $this->client->search( + indexname: $this->resolvePhysicalIndexName(), + query: [ + 'query' => ['term' => ['subscriberId' => $subscriber->getId()]], + 'sort' => [['idSort' => 'desc']], + 'size' => 10000, + ], + ); + + $hits = $response['hits']['hits'] ?? []; + + return array_map($this->hydrate(...), $hits); + } + + /** @param array{_source: array} $hit */ + private function hydrate(array $hit): SubscriberHistoryReadModel + { + $source = $hit['_source']; + + return new SubscriberHistoryReadModel( + id: isset($source['id']) ? (int) $source['id'] : null, + subscriberId: isset($source['subscriberId']) ? (int) $source['subscriberId'] : null, + ip: $source['ip'] ?? null, + createdAt: isset($source['date']) ? DateTime::createFromFormat(DATE_ATOM, $source['date']) ?: null : null, + summary: $source['summary'] ?? null, + detail: $source['detail'] ?? null, + systemInfo: $source['systemInfo'] ?? null, + ); + } + + private function resolvePhysicalIndexName(): string + { + return $this->indexPrefix . self::INDEX_ALIAS; + } +} diff --git a/src/Domain/Subscription/Repository/SubscriberHistoryRepository.php b/src/Domain/Subscription/Repository/SubscriberHistoryRepository.php index 137faa84..73c9bc0c 100644 --- a/src/Domain/Subscription/Repository/SubscriberHistoryRepository.php +++ b/src/Domain/Subscription/Repository/SubscriberHistoryRepository.php @@ -13,8 +13,11 @@ use PhpList\Core\Domain\Subscription\Model\Filter\SubscriberHistoryFilter; use PhpList\Core\Domain\Subscription\Model\Subscriber; use PhpList\Core\Domain\Subscription\Model\SubscriberHistory; +use PhpList\Core\Domain\Subscription\Repository\Interfaces\SubscriberHistoryReaderInterface; -class SubscriberHistoryRepository extends AbstractRepository implements PaginatableRepositoryInterface +class SubscriberHistoryRepository extends AbstractRepository implements + PaginatableRepositoryInterface, + SubscriberHistoryReaderInterface { use CursorPaginationTrait; diff --git a/src/Domain/Subscription/Service/Manager/SubscriberHistoryManager.php b/src/Domain/Subscription/Service/Manager/SubscriberHistoryManager.php index bd36422d..5338580b 100644 --- a/src/Domain/Subscription/Service/Manager/SubscriberHistoryManager.php +++ b/src/Domain/Subscription/Service/Manager/SubscriberHistoryManager.php @@ -10,39 +10,40 @@ use PhpList\Core\Domain\Identity\Model\Administrator; use PhpList\Core\Domain\Subscription\Model\Dto\ChangeSetDto; use PhpList\Core\Domain\Subscription\Model\Filter\SubscriberHistoryFilter; +use PhpList\Core\Domain\Subscription\Model\Interfaces\SubscriberHistoryRecordInterface; use PhpList\Core\Domain\Subscription\Model\Subscriber; use PhpList\Core\Domain\Subscription\Model\SubscriberHistory; -use PhpList\Core\Domain\Subscription\Repository\SubscriberHistoryRepository; +use PhpList\Core\Domain\Subscription\Repository\Interfaces\SubscriberHistoryReaderInterface; use Symfony\Contracts\Translation\TranslatorInterface; class SubscriberHistoryManager { - private SubscriberHistoryRepository $repository; + private SubscriberHistoryReaderInterface $reader; private ClientIpResolver $clientIpResolver; private SystemInfoCollector $systemInfoCollector; private TranslatorInterface $translator; private EntityManagerInterface $entityManager; public function __construct( - SubscriberHistoryRepository $repository, + SubscriberHistoryReaderInterface $reader, ClientIpResolver $clientIpResolver, SystemInfoCollector $systemInfoCollector, TranslatorInterface $translator, EntityManagerInterface $entityManager, ) { - $this->repository = $repository; + $this->reader = $reader; $this->clientIpResolver = $clientIpResolver; $this->systemInfoCollector = $systemInfoCollector; $this->translator = $translator; $this->entityManager = $entityManager; } - /** @return SubscriberHistory[] */ + /** @return SubscriberHistoryRecordInterface[] */ public function getHistory(int $lastId, int $limit, SubscriberHistoryFilter $filter): array { $filter->setLastId($lastId)->setLimit($limit); - return $this->repository->getFilteredAfterId($filter)->getItems(); + return $this->reader->getFilteredAfterId($filter)->getItems(); } public function addHistory(Subscriber $subscriber, string $message, ?string $details = null): SubscriberHistory diff --git a/src/Domain/Subscription/Service/Manager/SubscriberManager.php b/src/Domain/Subscription/Service/Manager/SubscriberManager.php index e13980ae..189a2881 100644 --- a/src/Domain/Subscription/Service/Manager/SubscriberManager.php +++ b/src/Domain/Subscription/Service/Manager/SubscriberManager.php @@ -11,7 +11,7 @@ use PhpList\Core\Domain\Subscription\Model\Dto\ImportSubscriberDto; use PhpList\Core\Domain\Subscription\Model\Dto\UpdateSubscriberDto; use PhpList\Core\Domain\Subscription\Model\Subscriber; -use PhpList\Core\Domain\Subscription\Repository\SubscriberHistoryRepository; +use PhpList\Core\Domain\Subscription\Repository\Interfaces\SubscriberHistoryReaderInterface; use PhpList\Core\Domain\Subscription\Repository\SubscriberRepository; use PhpList\Core\Domain\Subscription\Service\SubscriberDeletionService; use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; @@ -25,7 +25,7 @@ public function __construct( private readonly SubscriberDeletionService $subscriberDeletionService, private readonly TranslatorInterface $translator, private readonly SubscriberHistoryManager $subscriberHistoryManager, - private readonly SubscriberHistoryRepository $subscriberHistoryRepository, + private readonly SubscriberHistoryReaderInterface $subscriberHistoryReader, ) { } @@ -55,7 +55,7 @@ public function getSubscriberDetails(int $subscriberId): ?Subscriber return null; } - $history = $this->subscriberHistoryRepository->getBySubscriber($subscriber); + $history = $this->subscriberHistoryReader->getBySubscriber($subscriber); $subscriber->setHistory($history); return $subscriber; diff --git a/src/Domain/Subscription/Service/Search/SubscriberHistoryIndexDefinition.php b/src/Domain/Subscription/Service/Search/SubscriberHistoryIndexDefinition.php new file mode 100644 index 00000000..985508e0 --- /dev/null +++ b/src/Domain/Subscription/Service/Search/SubscriberHistoryIndexDefinition.php @@ -0,0 +1,43 @@ + [ + 'id' => ['type' => 'keyword'], + // Numeric mirror of `id`, used for range/sort in cursor pagination - `id` stays a + // keyword for exact-match filtering. + 'idSort' => ['type' => 'long'], + 'subscriberId' => ['type' => 'keyword'], + 'ip' => ['type' => 'keyword'], + 'date' => ['type' => 'date'], + 'summary' => [ + 'type' => 'text', + 'fields' => [ + 'keyword' => ['type' => 'keyword', 'ignore_above' => 256], + ], + ], + 'detail' => ['type' => 'text'], + 'systemInfo' => ['type' => 'text'], + ], + ]; + } + + public function getSettings(): array + { + return []; + } +} diff --git a/src/Domain/Subscription/Service/Search/SubscriberHistoryReindexProvider.php b/src/Domain/Subscription/Service/Search/SubscriberHistoryReindexProvider.php new file mode 100644 index 00000000..b4a81bd8 --- /dev/null +++ b/src/Domain/Subscription/Service/Search/SubscriberHistoryReindexProvider.php @@ -0,0 +1,39 @@ +repository->createQueryBuilder('sh') + ->select('COUNT(sh.id)') + ->getQuery() + ->getSingleScalarResult(); + } + + public function fetchBatch(int $lastId, int $batchSize): iterable + { + return $this->repository->createQueryBuilder('sh') + ->andWhere('sh.id > :lastId') + ->setParameter('lastId', $lastId) + ->orderBy('sh.id', 'ASC') + ->setMaxResults($batchSize) + ->getQuery() + ->toIterable(); + } +} diff --git a/tests/Unit/Domain/Subscription/Repository/SubscriberHistoryElasticsearchReaderTest.php b/tests/Unit/Domain/Subscription/Repository/SubscriberHistoryElasticsearchReaderTest.php new file mode 100644 index 00000000..a819537d --- /dev/null +++ b/tests/Unit/Domain/Subscription/Repository/SubscriberHistoryElasticsearchReaderTest.php @@ -0,0 +1,96 @@ +client = $this->createMock(ElasticsearchClientInterface::class); + $this->reader = new SubscriberHistoryElasticsearchReader($this->client, 'phplist_'); + } + + public function testGetFilteredAfterIdQueriesPrefixedIndexAndHydratesResults(): void + { + $filter = new SubscriberHistoryFilter(lastId: 5, limit: 10); + + $this->client + ->expects($this->once()) + ->method('search') + ->with( + 'phplist_subscriber_history', + $this->callback(function (array $query): bool { + return $query['size'] === 10 + && $query['query']['bool']['filter'][0] === ['range' => ['idSort' => ['gt' => 5]]]; + }), + ) + ->willReturn([ + 'hits' => [ + 'total' => ['value' => 1], + 'hits' => [ + ['_source' => [ + 'id' => 7, + 'subscriberId' => 3, + 'ip' => '127.0.0.1', + 'date' => '2026-01-01T00:00:00+00:00', + 'summary' => 'Updated', + 'detail' => 'Detail', + 'systemInfo' => 'Info', + ]], + ], + ], + ]); + + $result = $this->reader->getFilteredAfterId($filter); + + $this->assertSame(1, $result->getTotal()); + $this->assertCount(1, $result->getItems()); + $this->assertSame(7, $result->getItems()[0]->getId()); + $this->assertSame(3, $result->getItems()[0]->getSubscriberId()); + $this->assertSame('Updated', $result->getItems()[0]->getSummary()); + } + + public function testGetFilteredAfterIdRejectsWrongFilterType(): void + { + $wrongFilter = $this->createMock(FilterRequestInterface::class); + + $this->expectException(InvalidArgumentException::class); + $this->reader->getFilteredAfterId($wrongFilter); + } + + public function testGetBySubscriberSortsDescending(): void + { + $subscriber = $this->createMock(Subscriber::class); + $subscriber->method('getId')->willReturn(9); + + $this->client + ->expects($this->once()) + ->method('search') + ->with( + 'phplist_subscriber_history', + $this->callback(function (array $query): bool { + return $query['query'] === ['term' => ['subscriberId' => 9]] + && $query['sort'] === [['idSort' => 'desc']]; + }), + ) + ->willReturn(['hits' => ['hits' => []]]); + + $result = $this->reader->getBySubscriber($subscriber); + + $this->assertSame([], $result); + } +} diff --git a/tests/Unit/Domain/Subscription/Service/Manager/SubscriberHistoryManagerTest.php b/tests/Unit/Domain/Subscription/Service/Manager/SubscriberHistoryManagerTest.php index f28dc08a..ce8aa7d1 100644 --- a/tests/Unit/Domain/Subscription/Service/Manager/SubscriberHistoryManagerTest.php +++ b/tests/Unit/Domain/Subscription/Service/Manager/SubscriberHistoryManagerTest.php @@ -10,7 +10,7 @@ use PhpList\Core\Domain\Common\SystemInfoCollector; use PhpList\Core\Domain\Subscription\Model\Filter\SubscriberHistoryFilter; use PhpList\Core\Domain\Subscription\Model\SubscriberHistory; -use PhpList\Core\Domain\Subscription\Repository\SubscriberHistoryRepository; +use PhpList\Core\Domain\Subscription\Repository\Interfaces\SubscriberHistoryReaderInterface; use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberHistoryManager; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; @@ -18,14 +18,14 @@ class SubscriberHistoryManagerTest extends TestCase { - private SubscriberHistoryRepository|MockObject $subscriberHistoryRepository; + private SubscriberHistoryReaderInterface|MockObject $subscriberHistoryRepository; private SubscriberHistoryManager $subscriptionHistoryService; protected function setUp(): void { - $this->subscriberHistoryRepository = $this->createMock(SubscriberHistoryRepository::class); + $this->subscriberHistoryRepository = $this->createMock(SubscriberHistoryReaderInterface::class); $this->subscriptionHistoryService = new SubscriberHistoryManager( - repository: $this->subscriberHistoryRepository, + reader: $this->subscriberHistoryRepository, clientIpResolver: $this->createMock(ClientIpResolver::class), systemInfoCollector: $this->createMock(SystemInfoCollector::class), translator: $this->createMock(TranslatorInterface::class), diff --git a/tests/Unit/Domain/Subscription/Service/Manager/SubscriberManagerTest.php b/tests/Unit/Domain/Subscription/Service/Manager/SubscriberManagerTest.php index d02ca66f..628f1553 100644 --- a/tests/Unit/Domain/Subscription/Service/Manager/SubscriberManagerTest.php +++ b/tests/Unit/Domain/Subscription/Service/Manager/SubscriberManagerTest.php @@ -7,7 +7,7 @@ use Doctrine\ORM\EntityManagerInterface; use PhpList\Core\Domain\Subscription\Model\Dto\CreateSubscriberDto; use PhpList\Core\Domain\Subscription\Model\Subscriber; -use PhpList\Core\Domain\Subscription\Repository\SubscriberHistoryRepository; +use PhpList\Core\Domain\Subscription\Repository\Interfaces\SubscriberHistoryReaderInterface; use PhpList\Core\Domain\Subscription\Repository\SubscriberRepository; use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberHistoryManager; use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberManager; @@ -33,7 +33,7 @@ protected function setUp(): void subscriberDeletionService: $subscriberDeletionService, translator: new Translator('en'), subscriberHistoryManager: $this->createMock(SubscriberHistoryManager::class), - subscriberHistoryRepository: $this->createMock(SubscriberHistoryRepository::class), + subscriberHistoryReader: $this->createMock(SubscriberHistoryReaderInterface::class), ); } diff --git a/tests/Unit/Domain/Subscription/Service/Search/SubscriberHistoryIndexDefinitionTest.php b/tests/Unit/Domain/Subscription/Service/Search/SubscriberHistoryIndexDefinitionTest.php new file mode 100644 index 00000000..065ec627 --- /dev/null +++ b/tests/Unit/Domain/Subscription/Service/Search/SubscriberHistoryIndexDefinitionTest.php @@ -0,0 +1,37 @@ +assertSame('subscriber_history', $definition->getIndexAlias()); + } + + public function testMappingDeclaresExpectedFields(): void + { + $definition = new SubscriberHistoryIndexDefinition(); + $properties = $definition->getMapping()['properties']; + + foreach (['id', 'idSort', 'subscriberId', 'ip', 'date', 'summary', 'detail', 'systemInfo'] as $field) { + $this->assertArrayHasKey($field, $properties); + } + $this->assertSame('long', $properties['idSort']['type']); + $this->assertSame('keyword', $properties['id']['type']); + } + + public function testSettingsAreEmptyByDefault(): void + { + $definition = new SubscriberHistoryIndexDefinition(); + + $this->assertSame([], $definition->getSettings()); + } +} diff --git a/tests/Unit/Domain/Subscription/Service/Search/SubscriberHistoryReindexProviderTest.php b/tests/Unit/Domain/Subscription/Service/Search/SubscriberHistoryReindexProviderTest.php new file mode 100644 index 00000000..28926eb5 --- /dev/null +++ b/tests/Unit/Domain/Subscription/Service/Search/SubscriberHistoryReindexProviderTest.php @@ -0,0 +1,19 @@ +createMock(SubscriberHistoryRepository::class)); + + $this->assertSame('subscriber_history', $provider->getAlias()); + } +} From 1e337614b43c6a8ff3c6ebd9ef7af59a418d0c30 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Wed, 2 Sep 2026 10:29:38 +0400 Subject: [PATCH 06/24] fix errors --- .env.dist | 1 + .env.test | 3 +++ .env.test.local.dist | 9 --------- config/packages/messenger.yaml | 3 ++- config/parameters.yml | 1 + src/Domain/Search/Client/ElasticsearchClientFactory.php | 2 +- .../Repository/SubscriberHistoryElasticsearchReader.php | 8 ++++---- 7 files changed, 12 insertions(+), 15 deletions(-) create mode 100644 .env.test delete mode 100644 .env.test.local.dist diff --git a/.env.dist b/.env.dist index e1cdc484..6419c8b6 100644 --- a/.env.dist +++ b/.env.dist @@ -52,6 +52,7 @@ BOUNCE_IMAP_PURGE_UNPROCESSED=0 # Messenger configuration for asynchronous processing MESSENGER_TRANSPORT_DSN=doctrine://default?auto_setup=true +SEARCH_TRANSPORT_DSN=doctrine://default?queue_name=search_index # Elasticsearch configuration ELASTICSEARCH_HOSTS=http://127.0.0.1:9200 diff --git a/.env.test b/.env.test new file mode 100644 index 00000000..88b6c338 --- /dev/null +++ b/.env.test @@ -0,0 +1,3 @@ +PHPLIST_DATABASE_DRIVER=pdo_sqlite +PHPLIST_DATABASE_PATH=:memory: +SEARCH_TRANSPORT_DSN=sync:// diff --git a/.env.test.local.dist b/.env.test.local.dist deleted file mode 100644 index c9992c34..00000000 --- a/.env.test.local.dist +++ /dev/null @@ -1,9 +0,0 @@ -# Optional: copy this file to ".env.test.local" to run tests against an in-memory SQLite -# database instead of MySQL, so no database server is needed for `vendor/bin/phpunit`. -# -# Note: this file is not loaded automatically by PHPUnit CLI runs (this project's ApplicationKernel -# does not read .env files on its own); either export these as real environment variables before -# running phpunit, or wire them up via your own bootstrap/CI step. - -PHPLIST_DATABASE_DRIVER=pdo_sqlite -PHPLIST_DATABASE_PATH=:memory: \ No newline at end of file diff --git a/config/packages/messenger.yaml b/config/packages/messenger.yaml index 88d69297..c0096ea6 100644 --- a/config/packages/messenger.yaml +++ b/config/packages/messenger.yaml @@ -27,8 +27,9 @@ framework: # transaction is open (see SearchIndexDoctrineListener, which dispatches from postFlush). # On a fresh install that enables search before ever sending email, run # `bin/console messenger:setup-transports` once. + # Configurable so tests can swap in 'sync://' (see .env.test) and index synchronously. async_search: - dsn: 'doctrine://default?queue_name=search_index' + dsn: '%env(SEARCH_TRANSPORT_DSN)%' options: auto_setup: false retry_strategy: diff --git a/config/parameters.yml b/config/parameters.yml index d413d2b8..6e253afc 100644 --- a/config/parameters.yml +++ b/config/parameters.yml @@ -50,6 +50,7 @@ parameters: # Messenger configuration for asynchronous processing app.messenger_transport_dsn: '%env(MESSENGER_TRANSPORT_DSN)%' + app.search_transport_dsn: '%env(SEARCH_TRANSPORT_DSN)%' # Elasticsearch configuration elasticsearch.hosts: '%env(csv:ELASTICSEARCH_HOSTS)%' diff --git a/src/Domain/Search/Client/ElasticsearchClientFactory.php b/src/Domain/Search/Client/ElasticsearchClientFactory.php index 002f01a1..1eff7b86 100644 --- a/src/Domain/Search/Client/ElasticsearchClientFactory.php +++ b/src/Domain/Search/Client/ElasticsearchClientFactory.php @@ -23,7 +23,7 @@ public static function create( } $builder->setHttpClientOptions([ - 'connect_timeout' => $connectTimeout, + 'max_connect_duration' => $connectTimeout, 'timeout' => $requestTimeout, ]); diff --git a/src/Domain/Subscription/Repository/SubscriberHistoryElasticsearchReader.php b/src/Domain/Subscription/Repository/SubscriberHistoryElasticsearchReader.php index a3fc0da3..9e572acd 100644 --- a/src/Domain/Subscription/Repository/SubscriberHistoryElasticsearchReader.php +++ b/src/Domain/Subscription/Repository/SubscriberHistoryElasticsearchReader.php @@ -57,8 +57,8 @@ public function getFilteredAfterId(FilterRequestInterface $filter): PaginatedRes $mustFilters[] = ['range' => ['idSort' => ['gt' => $filter->getLastId()]]]; $response = $this->client->search( - indexname: $this->resolvePhysicalIndexName(), - query: [ + $this->resolvePhysicalIndexName(), + [ 'query' => ['bool' => ['filter' => $mustFilters]], 'sort' => [['idSort' => 'asc']], 'size' => $filter->getLimit(), @@ -80,8 +80,8 @@ public function getFilteredAfterId(FilterRequestInterface $filter): PaginatedRes public function getBySubscriber(Subscriber $subscriber): array { $response = $this->client->search( - indexname: $this->resolvePhysicalIndexName(), - query: [ + $this->resolvePhysicalIndexName(), + [ 'query' => ['term' => ['subscriberId' => $subscriber->getId()]], 'sort' => [['idSort' => 'desc']], 'size' => 10000, From 55a1b916745d15982cc2dd01420458f6e45b4d7b Mon Sep 17 00:00:00 2001 From: Tatevik Date: Wed, 2 Sep 2026 11:34:27 +0400 Subject: [PATCH 07/24] Add lastId handling in SubscriberHistoryElasticsearchReader and tests for pagination --- .../SubscriberHistoryElasticsearchReader.php | 3 +- ...bscriberHistoryElasticsearchReaderTest.php | 61 +++++++++++++++++++ 2 files changed, 63 insertions(+), 1 deletion(-) diff --git a/src/Domain/Subscription/Repository/SubscriberHistoryElasticsearchReader.php b/src/Domain/Subscription/Repository/SubscriberHistoryElasticsearchReader.php index 9e572acd..ecb743a9 100644 --- a/src/Domain/Subscription/Repository/SubscriberHistoryElasticsearchReader.php +++ b/src/Domain/Subscription/Repository/SubscriberHistoryElasticsearchReader.php @@ -67,12 +67,13 @@ public function getFilteredAfterId(FilterRequestInterface $filter): PaginatedRes ); $hits = $response['hits']['hits'] ?? []; + $lastHit = $hits !== [] ? $hits[array_key_last($hits)] : null; return new PaginatedResult( items: array_map($this->hydrate(...), $hits), total: (int) ($response['hits']['total']['value'] ?? 0), limit: $filter->getLimit(), - lastId: $filter->getLastId(), + lastId: $lastHit !== null ? (int) $lastHit['_source']['idSort'] : $filter->getLastId(), ); } diff --git a/tests/Unit/Domain/Subscription/Repository/SubscriberHistoryElasticsearchReaderTest.php b/tests/Unit/Domain/Subscription/Repository/SubscriberHistoryElasticsearchReaderTest.php index a819537d..a1c12bf5 100644 --- a/tests/Unit/Domain/Subscription/Repository/SubscriberHistoryElasticsearchReaderTest.php +++ b/tests/Unit/Domain/Subscription/Repository/SubscriberHistoryElasticsearchReaderTest.php @@ -44,6 +44,7 @@ public function testGetFilteredAfterIdQueriesPrefixedIndexAndHydratesResults(): 'hits' => [ ['_source' => [ 'id' => 7, + 'idSort' => 7, 'subscriberId' => 3, 'ip' => '127.0.0.1', 'date' => '2026-01-01T00:00:00+00:00', @@ -64,6 +65,66 @@ public function testGetFilteredAfterIdQueriesPrefixedIndexAndHydratesResults(): $this->assertSame('Updated', $result->getItems()[0]->getSummary()); } + public function testGetFilteredAfterIdPaginatesAcrossTwoPagesWithoutRepeatingResults(): void + { + $firstFilter = new SubscriberHistoryFilter(lastId: 0, limit: 1); + + $this->client + ->expects($this->exactly(2)) + ->method('search') + ->willReturnOnConsecutiveCalls( + [ + 'hits' => [ + 'total' => ['value' => 2], + 'hits' => [ + ['_source' => [ + 'id' => 5, + 'idSort' => 5, + 'subscriberId' => 1, + 'ip' => '127.0.0.1', + 'date' => '2026-01-01T00:00:00+00:00', + 'summary' => 'First', + 'detail' => 'Detail', + 'systemInfo' => 'Info', + ]], + ], + ], + ], + [ + 'hits' => [ + 'total' => ['value' => 2], + 'hits' => [ + ['_source' => [ + 'id' => 8, + 'idSort' => 8, + 'subscriberId' => 2, + 'ip' => '127.0.0.1', + 'date' => '2026-01-02T00:00:00+00:00', + 'summary' => 'Second', + 'detail' => 'Detail', + 'systemInfo' => 'Info', + ]], + ], + ], + ], + ); + + $firstPage = $this->reader->getFilteredAfterId($firstFilter); + + $this->assertSame(5, $firstPage->getLastId()); + $this->assertSame(5, $firstPage->getItems()[0]->getId()); + + $secondFilter = new SubscriberHistoryFilter(lastId: $firstPage->getLastId(), limit: 1); + $secondPage = $this->reader->getFilteredAfterId($secondFilter); + + $this->assertSame(8, $secondPage->getLastId()); + $this->assertSame(8, $secondPage->getItems()[0]->getId()); + $this->assertNotSame( + $firstPage->getItems()[0]->getId(), + $secondPage->getItems()[0]->getId(), + ); + } + public function testGetFilteredAfterIdRejectsWrongFilterType(): void { $wrongFilter = $this->createMock(FilterRequestInterface::class); From a500bd52e7e8534515a0646e6a0b7ad9dfc7fda2 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Wed, 2 Sep 2026 11:38:12 +0400 Subject: [PATCH 08/24] Clarify dispatch behavior in Elasticsearch indexing documentation --- README.md | 2 ++ config/packages/messenger.yaml | 2 +- docs/ElasticsearchSearch.md | 24 +++++++++++++++++------- 3 files changed, 20 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index aff4d2ee..7b320536 100755 --- a/README.md +++ b/README.md @@ -202,6 +202,8 @@ To extract translation strings from the source into an XLIFF catalog: ```bash php bin/console translation:extract --force en --format=xlf +php bin/console messenger:setup-transports +php bin/console messenger:consume async --limit=1 ``` ## Copyright diff --git a/config/packages/messenger.yaml b/config/packages/messenger.yaml index c0096ea6..4896be58 100644 --- a/config/packages/messenger.yaml +++ b/config/packages/messenger.yaml @@ -14,7 +14,7 @@ framework: check_delayed_interval: 60000 retry_strategy: max_retries: 3 - # milliseconds delay + # millisecond delay delay: 1000 multiplier: 2 max_delay: 0 diff --git a/docs/ElasticsearchSearch.md b/docs/ElasticsearchSearch.md index f4f0bea5..669a59f5 100644 --- a/docs/ElasticsearchSearch.md +++ b/docs/ElasticsearchSearch.md @@ -12,7 +12,10 @@ Elasticsearch: - **Writes** stay exactly as they are today (Doctrine `persist()`/`remove()`). A generic Doctrine event listener (`PhpList\Core\Core\Doctrine\SearchIndexDoctrineListener`) detects any entity that implements `SearchIndexableInterface` and asynchronously dispatches an indexing/deletion message via - Symfony Messenger, once the surrounding transaction has actually committed. + Symfony Messenger from Doctrine's `postFlush` event, i.e. after the ORM flush - not necessarily after + a real commit. Callers that wrap `flush()` in their own explicit transaction (e.g. + `SubscriberCsvImporter`) cause `postFlush` to fire before that outer transaction actually commits; see + "Consistency model" below for why this is still safe. - **Reads** for those entities go through a dedicated reader interface (e.g. `SubscriberHistoryReaderInterface`) that is aliased in DI to an Elasticsearch-backed implementation instead of the Doctrine repository. @@ -26,10 +29,17 @@ interfaces (see "Adding a new searchable entity" below) - no changes to the dual processing its queued message, a read from Elasticsearch will not yet reflect that row. - Reads **hard-fail** if Elasticsearch is unreachable - there is no fallback to the database. Any Elasticsearch error is raised as `PhpList\Core\Domain\Search\Exception\SearchBackendUnavailableException`. -- If a process crashes between the database transaction committing and the message being dispatched - (a narrow window - see `SearchIndexDoctrineListener`'s docblock for why the dispatch happens in - `postFlush`, not `postPersist`/`postUpdate`/`postRemove`), that one row is missed until the next - `phplist:search:reindex` run. Nothing is ever indexed for a row that was rolled back. +- Dispatch happens from `postFlush`, not `postPersist`/`postUpdate`/`postRemove` (those fire while the + flush is still in progress), so a row that ends up rolled back is never indexed. For a plain + `flush()` call with no surrounding transaction, `postFlush`'s own commit is the real commit, so + dispatch does happen after the row is durably committed. When a caller instead wraps `flush()` in + its own explicit transaction (e.g. `SubscriberCsvImporter`), `postFlush` fires before that outer + transaction commits - but the `async_search` transport is a Doctrine queue on the same connection, so + the queued message insert shares that same outer transaction: the row and its message still commit + or roll back together. The remaining crash window is narrower than "commit vs. dispatch" - it's + strictly between the outer transaction's real commit and the `async_search` worker consuming the + message; if a process dies in that window, that one row is missed until the next + `phplist:search:reindex` run. Consumers of `phplist/core` that build UI on top of these read paths should plan for both of the above (e.g. a brief "just added" staleness window, and handling a 5xx-equivalent from a search-unavailable @@ -39,7 +49,7 @@ condition) rather than assuming synchronous consistency with the database. Set in `.env` (see `.env.dist`): -``` +```dotenv ELASTICSEARCH_HOSTS=http://127.0.0.1:9200 ELASTICSEARCH_USERNAME= ELASTICSEARCH_PASSWORD= @@ -108,4 +118,4 @@ again after adding a new searchable entity or changing a mapping. - **New/updated rows not appearing in search results**: make sure a `messenger:consume async_search` worker is running; check `bin/console messenger:failed:show` for stuck messages. - **Data drifted between MySQL and Elasticsearch**: re-run `bin/console phplist:search:reindex ` - - it's a safe, idempotent full backfill. \ No newline at end of file + - it's a safe, idempotent full backfill. From 861b0e67ef4b87c10a51ee8ffb3cfcd2ded3076c Mon Sep 17 00:00:00 2001 From: Tatevik Date: Wed, 2 Sep 2026 11:56:01 +0400 Subject: [PATCH 09/24] Implement versioning for index and delete operations in Elasticsearch client --- .../Doctrine/SearchIndexDoctrineListener.php | 12 ++ .../Client/ElasticsearchClientAdapter.php | 31 +++-- .../Client/ElasticsearchClientInterface.php | 9 +- .../Search/Command/ReindexSearchCommand.php | 1 + .../Search/Message/IndexDocumentMessage.php | 6 + .../IndexDocumentMessageHandler.php | 2 + .../Search/Service/ElasticsearchIndexer.php | 8 +- .../Service/ElasticsearchIndexerInterface.php | 11 +- .../SearchIndexDoctrineListenerTest.php | 28 ++++- .../InMemoryVersionedElasticsearchClient.php | 68 +++++++++++ ...mentMessageHandlerRevisionOrderingTest.php | 113 ++++++++++++++++++ .../IndexDocumentMessageHandlerTest.php | 8 +- .../Service/ElasticsearchIndexerTest.php | 8 +- 13 files changed, 276 insertions(+), 29 deletions(-) create mode 100644 tests/Unit/Domain/Search/Fake/InMemoryVersionedElasticsearchClient.php create mode 100644 tests/Unit/Domain/Search/MessageHandler/IndexDocumentMessageHandlerRevisionOrderingTest.php diff --git a/src/Core/Doctrine/SearchIndexDoctrineListener.php b/src/Core/Doctrine/SearchIndexDoctrineListener.php index 1fd394a8..c18ffa03 100644 --- a/src/Core/Doctrine/SearchIndexDoctrineListener.php +++ b/src/Core/Doctrine/SearchIndexDoctrineListener.php @@ -82,6 +82,18 @@ private function queue(object $entity, SearchOperation $operation): void $entity->getSearchDocumentId(), $document, $operation, + $this->nextRevision(), ); } + + /** + * Wall-clock microseconds, not a per-process counter: a delayed Messenger retry carries the + * revision assigned when it was originally queued, and must stay comparable against revisions + * assigned by other PHP processes/workers for the same document so the indexer (via Elasticsearch + * external versioning) can tell a stale retry apart from a newer write. + */ + private function nextRevision(): int + { + return (int) (microtime(true) * 1_000_000); + } } diff --git a/src/Domain/Search/Client/ElasticsearchClientAdapter.php b/src/Domain/Search/Client/ElasticsearchClientAdapter.php index d9240b06..b1bbefb0 100644 --- a/src/Domain/Search/Client/ElasticsearchClientAdapter.php +++ b/src/Domain/Search/Client/ElasticsearchClientAdapter.php @@ -12,32 +12,43 @@ class ElasticsearchClientAdapter implements ElasticsearchClientInterface { private const HTTP_NOT_FOUND = 404; + private const HTTP_CONFLICT = 409; public function __construct(private readonly Client $client) { } - public function index(string $indexName, string $documentId, array $document): void + public function index(string $indexName, string $documentId, array $document, int $revision): void { - $this->call(function () use ($indexName, $documentId, $document): void { - $this->client->index([ - 'index' => $indexName, - 'id' => $documentId, - 'body' => $document, - ]); + $this->call(function () use ($indexName, $documentId, $document, $revision): void { + try { + $this->client->index([ + 'index' => $indexName, + 'id' => $documentId, + 'body' => $document, + 'version' => $revision, + 'version_type' => 'external_gte', + ]); + } catch (ClientResponseException $exception) { + if ($exception->getCode() !== self::HTTP_CONFLICT) { + throw $exception; + } + } }); } - public function delete(string $indexName, string $documentId): void + public function delete(string $indexName, string $documentId, int $revision): void { - $this->call(function () use ($indexName, $documentId): void { + $this->call(function () use ($indexName, $documentId, $revision): void { try { $this->client->delete([ 'index' => $indexName, 'id' => $documentId, + 'version' => $revision, + 'version_type' => 'external_gte', ]); } catch (ClientResponseException $exception) { - if ($exception->getCode() !== self::HTTP_NOT_FOUND) { + if (!in_array($exception->getCode(), [self::HTTP_NOT_FOUND, self::HTTP_CONFLICT], true)) { throw $exception; } } diff --git a/src/Domain/Search/Client/ElasticsearchClientInterface.php b/src/Domain/Search/Client/ElasticsearchClientInterface.php index ce7fcb13..b7240c1f 100644 --- a/src/Domain/Search/Client/ElasticsearchClientInterface.php +++ b/src/Domain/Search/Client/ElasticsearchClientInterface.php @@ -15,16 +15,19 @@ interface ElasticsearchClientInterface { /** + * Returns quietly (idempotent) if $revision is older than the revision currently stored for this + * document - a delayed retry of a stale write must never resurrect/overwrite newer state. * @param array $document * @throws SearchBackendUnavailableException */ - public function index(string $indexName, string $documentId, array $document): void; + public function index(string $indexName, string $documentId, array $document, int $revision): void; /** - * Returns quietly (idempotent) if the document does not exist. + * Returns quietly (idempotent) if the document does not exist, or if $revision is older than the + * revision currently stored for this document. * @throws SearchBackendUnavailableException */ - public function delete(string $indexName, string $documentId): void; + public function delete(string $indexName, string $documentId, int $revision): void; /** @throws SearchBackendUnavailableException */ public function indexExists(string $indexName): bool; diff --git a/src/Domain/Search/Command/ReindexSearchCommand.php b/src/Domain/Search/Command/ReindexSearchCommand.php index f9e6a40c..350724a2 100644 --- a/src/Domain/Search/Command/ReindexSearchCommand.php +++ b/src/Domain/Search/Command/ReindexSearchCommand.php @@ -90,6 +90,7 @@ private function reindexProvider( $entity->getSearchIndexName(), $entity->getSearchDocumentId(), $entity->toSearchDocument(), + (int) (microtime(true) * 1_000_000), ); $lastId = (int) $entity->getSearchDocumentId(); $countInBatch++; diff --git a/src/Domain/Search/Message/IndexDocumentMessage.php b/src/Domain/Search/Message/IndexDocumentMessage.php index 39871347..48ebc577 100644 --- a/src/Domain/Search/Message/IndexDocumentMessage.php +++ b/src/Domain/Search/Message/IndexDocumentMessage.php @@ -19,6 +19,7 @@ public function __construct( private readonly string $documentId, private readonly array $document, private readonly SearchOperation $operation, + private readonly int $revision, ) { } @@ -42,4 +43,9 @@ public function getOperation(): SearchOperation { return $this->operation; } + + public function getRevision(): int + { + return $this->revision; + } } diff --git a/src/Domain/Search/MessageHandler/IndexDocumentMessageHandler.php b/src/Domain/Search/MessageHandler/IndexDocumentMessageHandler.php index 24f85a96..7896d4cc 100644 --- a/src/Domain/Search/MessageHandler/IndexDocumentMessageHandler.php +++ b/src/Domain/Search/MessageHandler/IndexDocumentMessageHandler.php @@ -23,10 +23,12 @@ public function __invoke(IndexDocumentMessage $message): void $message->getIndexName(), $message->getDocumentId(), $message->getDocument(), + $message->getRevision(), ), SearchOperation::Delete => $this->indexer->delete( $message->getIndexName(), $message->getDocumentId(), + $message->getRevision(), ), }; } diff --git a/src/Domain/Search/Service/ElasticsearchIndexer.php b/src/Domain/Search/Service/ElasticsearchIndexer.php index cc33ae89..317d40c0 100644 --- a/src/Domain/Search/Service/ElasticsearchIndexer.php +++ b/src/Domain/Search/Service/ElasticsearchIndexer.php @@ -15,14 +15,14 @@ public function __construct( ) { } - public function index(string $indexAlias, string $documentId, array $document): void + public function index(string $indexAlias, string $documentId, array $document, int $revision): void { - $this->client->index($this->resolvePhysicalIndexName($indexAlias), $documentId, $document); + $this->client->index($this->resolvePhysicalIndexName($indexAlias), $documentId, $document, $revision); } - public function delete(string $indexAlias, string $documentId): void + public function delete(string $indexAlias, string $documentId, int $revision): void { - $this->client->delete($this->resolvePhysicalIndexName($indexAlias), $documentId); + $this->client->delete($this->resolvePhysicalIndexName($indexAlias), $documentId, $revision); } public function createOrUpdateIndex(SearchIndexDefinitionInterface $definition): void diff --git a/src/Domain/Search/Service/ElasticsearchIndexerInterface.php b/src/Domain/Search/Service/ElasticsearchIndexerInterface.php index e280c977..8e01097d 100644 --- a/src/Domain/Search/Service/ElasticsearchIndexerInterface.php +++ b/src/Domain/Search/Service/ElasticsearchIndexerInterface.php @@ -11,12 +11,17 @@ interface ElasticsearchIndexerInterface { /** * @param array $document + * @param int $revision Monotonic per-document revision; writes older than the last applied + * revision for this document are dropped instead of applied (see ElasticsearchClientAdapter). * @throws SearchBackendUnavailableException */ - public function index(string $indexAlias, string $documentId, array $document): void; + public function index(string $indexAlias, string $documentId, array $document, int $revision): void; - /** @throws SearchBackendUnavailableException */ - public function delete(string $indexAlias, string $documentId): void; + /** + * @param int $revision Monotonic per-document revision; see index(). + * @throws SearchBackendUnavailableException + */ + public function delete(string $indexAlias, string $documentId, int $revision): void; /** * Creates the index with its mapping/settings if absent, otherwise applies the mapping diff --git a/tests/Unit/Core/Doctrine/SearchIndexDoctrineListenerTest.php b/tests/Unit/Core/Doctrine/SearchIndexDoctrineListenerTest.php index 889cb318..afc3fd44 100644 --- a/tests/Unit/Core/Doctrine/SearchIndexDoctrineListenerTest.php +++ b/tests/Unit/Core/Doctrine/SearchIndexDoctrineListenerTest.php @@ -62,7 +62,8 @@ public function testPostFlushDispatchesBufferedIndexMessage(): void return $message->getIndexName() === 'subscriber_history' && $message->getDocumentId() === '1' && $message->getDocument() === ['id' => 1] - && $message->getOperation() === SearchOperation::Index; + && $message->getOperation() === SearchOperation::Index + && $message->getRevision() > 0; })) ->willReturn(new Envelope(new stdClass())); @@ -70,6 +71,31 @@ public function testPostFlushDispatchesBufferedIndexMessage(): void $this->listener->postFlush(new PostFlushEventArgs($this->objectManager)); } + public function testRevisionsAreMonotonicallyIncreasingAcrossFlushes(): void + { + $entity = $this->createIndexable('subscriber_history', '1', ['id' => 1]); + $revisions = []; + + $this->messageBus + ->expects($this->exactly(2)) + ->method('dispatch') + ->with($this->callback(function (IndexDocumentMessage $message) use (&$revisions): bool { + $revisions[] = $message->getRevision(); + + return true; + })) + ->willReturn(new Envelope(new stdClass())); + + $this->listener->postPersist(new PostPersistEventArgs($entity, $this->objectManager)); + $this->listener->postFlush(new PostFlushEventArgs($this->objectManager)); + + $this->listener->postUpdate(new PostUpdateEventArgs($entity, $this->objectManager)); + $this->listener->postFlush(new PostFlushEventArgs($this->objectManager)); + + $this->assertCount(2, $revisions); + $this->assertGreaterThanOrEqual($revisions[0], $revisions[1]); + } + public function testPostRemoveBuffersDeleteOperationWithEmptyDocument(): void { $entity = $this->createIndexable('subscriber_history', '1', ['id' => 1]); diff --git a/tests/Unit/Domain/Search/Fake/InMemoryVersionedElasticsearchClient.php b/tests/Unit/Domain/Search/Fake/InMemoryVersionedElasticsearchClient.php new file mode 100644 index 00000000..81a2e0bb --- /dev/null +++ b/tests/Unit/Domain/Search/Fake/InMemoryVersionedElasticsearchClient.php @@ -0,0 +1,68 @@ + */ + private array $revisions = []; + + /** @var array|null> */ + private array $documents = []; + + public function index(string $indexName, string $documentId, array $document, int $revision): void + { + $key = $indexName . '|' . $documentId; + if (isset($this->revisions[$key]) && $revision < $this->revisions[$key]) { + return; + } + + $this->revisions[$key] = $revision; + $this->documents[$key] = $document; + } + + public function delete(string $indexName, string $documentId, int $revision): void + { + $key = $indexName . '|' . $documentId; + if (isset($this->revisions[$key]) && $revision < $this->revisions[$key]) { + return; + } + + $this->revisions[$key] = $revision; + $this->documents[$key] = null; + } + + /** @return array|null */ + public function getDocument(string $indexName, string $documentId): ?array + { + return $this->documents[$indexName . '|' . $documentId] ?? null; + } + + public function indexExists(string $indexName): bool + { + return true; + } + + public function createIndex(string $indexName, array $mapping, array $settings): void + { + } + + public function updateMapping(string $indexName, array $mapping): void + { + } + + public function search(string $indexName, array $query): array + { + return []; + } +} \ No newline at end of file diff --git a/tests/Unit/Domain/Search/MessageHandler/IndexDocumentMessageHandlerRevisionOrderingTest.php b/tests/Unit/Domain/Search/MessageHandler/IndexDocumentMessageHandlerRevisionOrderingTest.php new file mode 100644 index 00000000..7be8c886 --- /dev/null +++ b/tests/Unit/Domain/Search/MessageHandler/IndexDocumentMessageHandlerRevisionOrderingTest.php @@ -0,0 +1,113 @@ + handler -> indexer -> client) rather than mocking away the + * exact ordering guarantee under test. + */ +class IndexDocumentMessageHandlerRevisionOrderingTest extends TestCase +{ + private InMemoryVersionedElasticsearchClient $client; + private IndexDocumentMessageHandler $handler; + + protected function setUp(): void + { + $this->client = new InMemoryVersionedElasticsearchClient(); + $this->handler = new IndexDocumentMessageHandler(new ElasticsearchIndexer($this->client, 'phplist_')); + } + + public function testDelayedRetryOfOlderUpdateDoesNotOverwriteNewerUpdate(): void + { + // The update at revision 100 is dispatched first but, say, the Messenger transport fails + // to deliver it until a later retry - meanwhile a newer update (revision 200) for the same + // document is dispatched and processed first. + ($this->handler)(new IndexDocumentMessage( + 'subscriber_history', + '1', + ['id' => 1, 'summary' => 'Updated'], + SearchOperation::Index, + 200, + )); + + // The retry of the older message finally lands. + ($this->handler)(new IndexDocumentMessage( + 'subscriber_history', + '1', + ['id' => 1, 'summary' => 'Original'], + SearchOperation::Index, + 100, + )); + + $this->assertSame( + ['id' => 1, 'summary' => 'Updated'], + $this->client->getDocument('phplist_subscriber_history', '1'), + ); + } + + public function testDelayedRetryOfOlderUpdateDoesNotResurrectDeletedDocument(): void + { + ($this->handler)(new IndexDocumentMessage( + 'subscriber_history', + '1', + ['id' => 1, 'summary' => 'Original'], + SearchOperation::Index, + 100, + )); + + // A delete for the same document, at a newer revision, is processed first. + ($this->handler)(new IndexDocumentMessage( + 'subscriber_history', + '1', + [], + SearchOperation::Delete, + 300, + )); + + // The delayed retry of the stale update finally lands and must not resurrect the document. + ($this->handler)(new IndexDocumentMessage( + 'subscriber_history', + '1', + ['id' => 1, 'summary' => 'Original'], + SearchOperation::Index, + 150, + )); + + $this->assertNull($this->client->getDocument('phplist_subscriber_history', '1')); + } + + public function testNewerUpdateAfterADeleteIsStillApplied(): void + { + ($this->handler)(new IndexDocumentMessage( + 'subscriber_history', + '1', + [], + SearchOperation::Delete, + 300, + )); + + ($this->handler)(new IndexDocumentMessage( + 'subscriber_history', + '1', + ['id' => 1, 'summary' => 'Recreated'], + SearchOperation::Index, + 400, + )); + + $this->assertSame( + ['id' => 1, 'summary' => 'Recreated'], + $this->client->getDocument('phplist_subscriber_history', '1'), + ); + } +} \ No newline at end of file diff --git a/tests/Unit/Domain/Search/MessageHandler/IndexDocumentMessageHandlerTest.php b/tests/Unit/Domain/Search/MessageHandler/IndexDocumentMessageHandlerTest.php index d9a469da..661ed121 100644 --- a/tests/Unit/Domain/Search/MessageHandler/IndexDocumentMessageHandlerTest.php +++ b/tests/Unit/Domain/Search/MessageHandler/IndexDocumentMessageHandlerTest.php @@ -25,12 +25,12 @@ protected function setUp(): void public function testInvokeIndexesOnIndexOperation(): void { $document = ['id' => 1, 'summary' => 'hello']; - $message = new IndexDocumentMessage('subscriber_history', '1', $document, SearchOperation::Index); + $message = new IndexDocumentMessage('subscriber_history', '1', $document, SearchOperation::Index, 100); $this->indexer ->expects($this->once()) ->method('index') - ->with('subscriber_history', '1', $document); + ->with('subscriber_history', '1', $document, 100); $this->indexer->expects($this->never())->method('delete'); ($this->handler)($message); @@ -38,12 +38,12 @@ public function testInvokeIndexesOnIndexOperation(): void public function testInvokeDeletesOnDeleteOperation(): void { - $message = new IndexDocumentMessage('subscriber_history', '1', [], SearchOperation::Delete); + $message = new IndexDocumentMessage('subscriber_history', '1', [], SearchOperation::Delete, 100); $this->indexer ->expects($this->once()) ->method('delete') - ->with('subscriber_history', '1'); + ->with('subscriber_history', '1', 100); $this->indexer->expects($this->never())->method('index'); ($this->handler)($message); diff --git a/tests/Unit/Domain/Search/Service/ElasticsearchIndexerTest.php b/tests/Unit/Domain/Search/Service/ElasticsearchIndexerTest.php index b293a872..6d8f300f 100644 --- a/tests/Unit/Domain/Search/Service/ElasticsearchIndexerTest.php +++ b/tests/Unit/Domain/Search/Service/ElasticsearchIndexerTest.php @@ -26,9 +26,9 @@ public function testIndexAppliesIndexPrefix(): void $this->client ->expects($this->once()) ->method('index') - ->with('phplist_subscriber_history', '42', ['id' => 42]); + ->with('phplist_subscriber_history', '42', ['id' => 42], 100); - $this->indexer->index('subscriber_history', '42', ['id' => 42]); + $this->indexer->index('subscriber_history', '42', ['id' => 42], 100); } public function testDeleteAppliesIndexPrefix(): void @@ -36,9 +36,9 @@ public function testDeleteAppliesIndexPrefix(): void $this->client ->expects($this->once()) ->method('delete') - ->with('phplist_subscriber_history', '42'); + ->with('phplist_subscriber_history', '42', 100); - $this->indexer->delete('subscriber_history', '42'); + $this->indexer->delete('subscriber_history', '42', 100); } public function testCreateOrUpdateIndexCreatesWhenAbsent(): void From fe16135803f5422e8fa9d5dbc52a59ae36524c70 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Wed, 2 Sep 2026 11:59:14 +0400 Subject: [PATCH 10/24] Validate batch-size and last-id options in ReindexSearchCommand --- src/Domain/Search/Command/ReindexSearchCommand.php | 10 ++++++++++ .../SubscriberHistoryElasticsearchReader.php | 1 + 2 files changed, 11 insertions(+) diff --git a/src/Domain/Search/Command/ReindexSearchCommand.php b/src/Domain/Search/Command/ReindexSearchCommand.php index 350724a2..55189f57 100644 --- a/src/Domain/Search/Command/ReindexSearchCommand.php +++ b/src/Domain/Search/Command/ReindexSearchCommand.php @@ -51,6 +51,16 @@ protected function execute(InputInterface $input, OutputInterface $output): int $batchSize = (int) $input->getOption('batch-size'); $lastId = (int) $input->getOption('last-id'); + if ($batchSize < 1) { + $io->error('The --batch-size option must be greater than zero.'); + return Command::FAILURE; + } + + if ($alias === null && $lastId !== 0) { + $io->error('The --last-id option requires an index alias.'); + return Command::FAILURE; + } + $providers = $alias !== null ? array_filter([$this->registry->find($alias)]) : $this->registry->getAll(); diff --git a/src/Domain/Subscription/Repository/SubscriberHistoryElasticsearchReader.php b/src/Domain/Subscription/Repository/SubscriberHistoryElasticsearchReader.php index ecb743a9..10f88794 100644 --- a/src/Domain/Subscription/Repository/SubscriberHistoryElasticsearchReader.php +++ b/src/Domain/Subscription/Repository/SubscriberHistoryElasticsearchReader.php @@ -85,6 +85,7 @@ public function getBySubscriber(Subscriber $subscriber): array [ 'query' => ['term' => ['subscriberId' => $subscriber->getId()]], 'sort' => [['idSort' => 'desc']], + // 10000 is enough, I think, but if we ever need more, we can implement pagination here too. 'size' => 10000, ], ); From 89473c61aeb19c50af9ecdb94bff15b40748e457 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Wed, 2 Sep 2026 12:28:34 +0400 Subject: [PATCH 11/24] Refactor SubscriberHistory index name usage to use constant --- src/Domain/Subscription/Model/SubscriberHistory.php | 2 +- .../Repository/SubscriberHistoryElasticsearchReader.php | 5 ++--- .../Service/Search/SubscriberHistoryIndexDefinition.php | 3 ++- .../Service/Search/SubscriberHistoryReindexProvider.php | 3 ++- 4 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/Domain/Subscription/Model/SubscriberHistory.php b/src/Domain/Subscription/Model/SubscriberHistory.php index c4b22084..5526b52d 100644 --- a/src/Domain/Subscription/Model/SubscriberHistory.php +++ b/src/Domain/Subscription/Model/SubscriberHistory.php @@ -23,7 +23,7 @@ class SubscriberHistory implements SearchIndexableInterface, SubscriberHistoryRecordInterface { - private const SEARCH_INDEX_NAME = 'subscriber_history'; + public const SEARCH_INDEX_NAME = 'subscriber_history'; #[ORM\Id] #[ORM\Column(type: 'integer')] diff --git a/src/Domain/Subscription/Repository/SubscriberHistoryElasticsearchReader.php b/src/Domain/Subscription/Repository/SubscriberHistoryElasticsearchReader.php index 10f88794..bca54aa5 100644 --- a/src/Domain/Subscription/Repository/SubscriberHistoryElasticsearchReader.php +++ b/src/Domain/Subscription/Repository/SubscriberHistoryElasticsearchReader.php @@ -13,6 +13,7 @@ use PhpList\Core\Domain\Subscription\Model\Interfaces\SubscriberHistoryRecordInterface; use PhpList\Core\Domain\Subscription\Model\ReadModel\SubscriberHistoryReadModel; use PhpList\Core\Domain\Subscription\Model\Subscriber; +use PhpList\Core\Domain\Subscription\Model\SubscriberHistory; use PhpList\Core\Domain\Subscription\Repository\Interfaces\SubscriberHistoryReaderInterface; /** @@ -22,8 +23,6 @@ */ class SubscriberHistoryElasticsearchReader implements SubscriberHistoryReaderInterface { - private const INDEX_ALIAS = 'subscriber_history'; - public function __construct( private readonly ElasticsearchClientInterface $client, private readonly string $indexPrefix, @@ -113,6 +112,6 @@ private function hydrate(array $hit): SubscriberHistoryReadModel private function resolvePhysicalIndexName(): string { - return $this->indexPrefix . self::INDEX_ALIAS; + return $this->indexPrefix . SubscriberHistory::SEARCH_INDEX_NAME; } } diff --git a/src/Domain/Subscription/Service/Search/SubscriberHistoryIndexDefinition.php b/src/Domain/Subscription/Service/Search/SubscriberHistoryIndexDefinition.php index 985508e0..52171429 100644 --- a/src/Domain/Subscription/Service/Search/SubscriberHistoryIndexDefinition.php +++ b/src/Domain/Subscription/Service/Search/SubscriberHistoryIndexDefinition.php @@ -5,12 +5,13 @@ namespace PhpList\Core\Domain\Subscription\Service\Search; use PhpList\Core\Domain\Search\Model\Interfaces\SearchIndexDefinitionInterface; +use PhpList\Core\Domain\Subscription\Model\SubscriberHistory; class SubscriberHistoryIndexDefinition implements SearchIndexDefinitionInterface { public function getIndexAlias(): string { - return 'subscriber_history'; + return SubscriberHistory::SEARCH_INDEX_NAME; } public function getMapping(): array diff --git a/src/Domain/Subscription/Service/Search/SubscriberHistoryReindexProvider.php b/src/Domain/Subscription/Service/Search/SubscriberHistoryReindexProvider.php index b4a81bd8..e32ced1d 100644 --- a/src/Domain/Subscription/Service/Search/SubscriberHistoryReindexProvider.php +++ b/src/Domain/Subscription/Service/Search/SubscriberHistoryReindexProvider.php @@ -5,6 +5,7 @@ namespace PhpList\Core\Domain\Subscription\Service\Search; use PhpList\Core\Domain\Search\Model\Interfaces\SearchReindexProviderInterface; +use PhpList\Core\Domain\Subscription\Model\SubscriberHistory; use PhpList\Core\Domain\Subscription\Repository\SubscriberHistoryRepository; class SubscriberHistoryReindexProvider implements SearchReindexProviderInterface @@ -15,7 +16,7 @@ public function __construct(private readonly SubscriberHistoryRepository $reposi public function getAlias(): string { - return 'subscriber_history'; + return SubscriberHistory::SEARCH_INDEX_NAME; } public function countAll(): int From 9e1d88d379ff489f82e7a16160c546286be5e11e Mon Sep 17 00:00:00 2001 From: Tatevik Date: Wed, 2 Sep 2026 12:36:05 +0400 Subject: [PATCH 12/24] Remove ELASTICSEARCH_INDEX_PREFIX from configuration files and replace with DATABASE_PREFIX --- .env.dist | 1 - config/parameters.yml | 2 +- docs/ElasticsearchSearch.md | 1 - .../Domain/Search/Fake/InMemoryVersionedElasticsearchClient.php | 2 +- .../IndexDocumentMessageHandlerRevisionOrderingTest.php | 2 +- 5 files changed, 3 insertions(+), 5 deletions(-) diff --git a/.env.dist b/.env.dist index 6419c8b6..16388975 100644 --- a/.env.dist +++ b/.env.dist @@ -58,7 +58,6 @@ SEARCH_TRANSPORT_DSN=doctrine://default?queue_name=search_index ELASTICSEARCH_HOSTS=http://127.0.0.1:9200 ELASTICSEARCH_USERNAME= ELASTICSEARCH_PASSWORD= -ELASTICSEARCH_INDEX_PREFIX=phplist_ ELASTICSEARCH_CONNECT_TIMEOUT=2 ELASTICSEARCH_REQUEST_TIMEOUT=5 diff --git a/config/parameters.yml b/config/parameters.yml index 6e253afc..e0f04067 100644 --- a/config/parameters.yml +++ b/config/parameters.yml @@ -56,7 +56,7 @@ parameters: elasticsearch.hosts: '%env(csv:ELASTICSEARCH_HOSTS)%' elasticsearch.username: '%env(ELASTICSEARCH_USERNAME)%' elasticsearch.password: '%env(ELASTICSEARCH_PASSWORD)%' - elasticsearch.index_prefix: '%env(ELASTICSEARCH_INDEX_PREFIX)%' + elasticsearch.index_prefix: '%env(DATABASE_PREFIX)%' elasticsearch.connect_timeout: '%env(int:ELASTICSEARCH_CONNECT_TIMEOUT)%' elasticsearch.request_timeout: '%env(int:ELASTICSEARCH_REQUEST_TIMEOUT)%' diff --git a/docs/ElasticsearchSearch.md b/docs/ElasticsearchSearch.md index 669a59f5..c7ef84b4 100644 --- a/docs/ElasticsearchSearch.md +++ b/docs/ElasticsearchSearch.md @@ -53,7 +53,6 @@ Set in `.env` (see `.env.dist`): ELASTICSEARCH_HOSTS=http://127.0.0.1:9200 ELASTICSEARCH_USERNAME= ELASTICSEARCH_PASSWORD= -ELASTICSEARCH_INDEX_PREFIX=phplist_ ELASTICSEARCH_CONNECT_TIMEOUT=2 ELASTICSEARCH_REQUEST_TIMEOUT=5 ``` diff --git a/tests/Unit/Domain/Search/Fake/InMemoryVersionedElasticsearchClient.php b/tests/Unit/Domain/Search/Fake/InMemoryVersionedElasticsearchClient.php index 81a2e0bb..7b10e8a4 100644 --- a/tests/Unit/Domain/Search/Fake/InMemoryVersionedElasticsearchClient.php +++ b/tests/Unit/Domain/Search/Fake/InMemoryVersionedElasticsearchClient.php @@ -65,4 +65,4 @@ public function search(string $indexName, array $query): array { return []; } -} \ No newline at end of file +} diff --git a/tests/Unit/Domain/Search/MessageHandler/IndexDocumentMessageHandlerRevisionOrderingTest.php b/tests/Unit/Domain/Search/MessageHandler/IndexDocumentMessageHandlerRevisionOrderingTest.php index 7be8c886..c830e565 100644 --- a/tests/Unit/Domain/Search/MessageHandler/IndexDocumentMessageHandlerRevisionOrderingTest.php +++ b/tests/Unit/Domain/Search/MessageHandler/IndexDocumentMessageHandlerRevisionOrderingTest.php @@ -110,4 +110,4 @@ public function testNewerUpdateAfterADeleteIsStillApplied(): void $this->client->getDocument('phplist_subscriber_history', '1'), ); } -} \ No newline at end of file +} From 464c769339308df80459756cdc61f8722ce773e7 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Thu, 3 Sep 2026 11:20:12 +0400 Subject: [PATCH 13/24] Persist UserMessageBounce entity in linkUserMessageBounce method --- README.md | 1 + src/Domain/Messaging/Service/Manager/BounceManager.php | 2 ++ .../Domain/Messaging/Service/Manager/BounceManagerTest.php | 5 +++++ 3 files changed, 8 insertions(+) diff --git a/README.md b/README.md index 7b320536..1f6a71cd 100755 --- a/README.md +++ b/README.md @@ -204,6 +204,7 @@ To extract translation strings from the source into an XLIFF catalog: php bin/console translation:extract --force en --format=xlf php bin/console messenger:setup-transports php bin/console messenger:consume async --limit=1 +php bin/console phplist:search:init-indices ``` ## Copyright diff --git a/src/Domain/Messaging/Service/Manager/BounceManager.php b/src/Domain/Messaging/Service/Manager/BounceManager.php index bae5e094..e8e7c497 100644 --- a/src/Domain/Messaging/Service/Manager/BounceManager.php +++ b/src/Domain/Messaging/Service/Manager/BounceManager.php @@ -89,6 +89,8 @@ public function linkUserMessageBounce( $userMessageBounce->setUserId($subscriberId); $userMessageBounce->setMessageId($messageId); + $this->userMessageBounceRepo->persist($userMessageBounce); + return $userMessageBounce; } diff --git a/tests/Unit/Domain/Messaging/Service/Manager/BounceManagerTest.php b/tests/Unit/Domain/Messaging/Service/Manager/BounceManagerTest.php index 3a07b0a0..043aa5f4 100644 --- a/tests/Unit/Domain/Messaging/Service/Manager/BounceManagerTest.php +++ b/tests/Unit/Domain/Messaging/Service/Manager/BounceManagerTest.php @@ -130,6 +130,11 @@ public function testLinkUserMessageBounceFlushesAndSetsFields(): void $bounce->method('getId')->willReturn(77); $dt = new DateTimeImmutable('2024-05-01 12:34:56'); + + $this->userMessageBounceRepository->expects($this->once()) + ->method('persist') + ->with($this->isInstanceOf(UserMessageBounce::class)); + $umb = $this->manager->linkUserMessageBounce($bounce, $dt, 123, 456); $this->assertSame(77, $umb->getBounceId()); From 1ccc43ef487fbf4b4abc8a1f0cd23ca523657379 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Thu, 3 Sep 2026 11:36:54 +0400 Subject: [PATCH 14/24] Add Elasticsearch support for UserMessageBounce with reader and filter --- config/services/elasticsearch.yml | 7 + config/services/repositories.yml | 5 + docs/ElasticsearchSearch.md | 5 +- .../Model/Filter/UserMessageBounceFilter.php | 53 +++++++ .../UserMessageBounceRecordInterface.php | 26 +++ .../ReadModel/UserMessageBounceReadModel.php | 46 ++++++ .../Messaging/Model/UserMessageBounce.php | 33 +++- .../UserMessageBounceReaderInterface.php | 23 +++ .../UserMessageBounceElasticsearchReader.php | 116 ++++++++++++++ .../UserMessageBounceRepository.php | 77 ++++++++- .../UserMessageBounceIndexDefinition.php | 37 +++++ .../UserMessageBounceReindexProvider.php | 40 +++++ ...erMessageBounceElasticsearchReaderTest.php | 148 ++++++++++++++++++ .../UserMessageBounceIndexDefinitionTest.php | 37 +++++ .../UserMessageBounceReindexProviderTest.php | 19 +++ 15 files changed, 668 insertions(+), 4 deletions(-) create mode 100644 src/Domain/Messaging/Model/Filter/UserMessageBounceFilter.php create mode 100644 src/Domain/Messaging/Model/Interfaces/UserMessageBounceRecordInterface.php create mode 100644 src/Domain/Messaging/Model/ReadModel/UserMessageBounceReadModel.php create mode 100644 src/Domain/Messaging/Repository/Interfaces/UserMessageBounceReaderInterface.php create mode 100644 src/Domain/Messaging/Repository/UserMessageBounceElasticsearchReader.php create mode 100644 src/Domain/Messaging/Service/Search/UserMessageBounceIndexDefinition.php create mode 100644 src/Domain/Messaging/Service/Search/UserMessageBounceReindexProvider.php create mode 100644 tests/Unit/Domain/Messaging/Repository/UserMessageBounceElasticsearchReaderTest.php create mode 100644 tests/Unit/Domain/Messaging/Service/Search/UserMessageBounceIndexDefinitionTest.php create mode 100644 tests/Unit/Domain/Messaging/Service/Search/UserMessageBounceReindexProviderTest.php diff --git a/config/services/elasticsearch.yml b/config/services/elasticsearch.yml index c59a7463..821025e0 100644 --- a/config/services/elasticsearch.yml +++ b/config/services/elasticsearch.yml @@ -45,3 +45,10 @@ services: PhpList\Core\Domain\Subscription\Service\Search\: resource: '../../src/Domain/Subscription/Service/Search' + + PhpList\Core\Domain\Messaging\Repository\UserMessageBounceElasticsearchReader: + arguments: + $indexPrefix: '%elasticsearch.index_prefix%' + + PhpList\Core\Domain\Messaging\Service\Search\: + resource: '../../src/Domain/Messaging/Service/Search' diff --git a/config/services/repositories.yml b/config/services/repositories.yml index 20ed2b79..1c8bf837 100644 --- a/config/services/repositories.yml +++ b/config/services/repositories.yml @@ -122,6 +122,11 @@ services: parent: PhpList\Core\Domain\Common\Repository\AbstractRepository arguments: - PhpList\Core\Domain\Messaging\Model\UserMessageBounce + + # Reads for UserMessageBounceReaderInterface consumers come only from Elasticsearch - swap this + # alias to UserMessageBounceRepository to read from the database instead. + PhpList\Core\Domain\Messaging\Repository\Interfaces\UserMessageBounceReaderInterface: + alias: PhpList\Core\Domain\Messaging\Repository\UserMessageBounceElasticsearchReader PhpList\Core\Domain\Messaging\Repository\UserMessageForwardRepository: parent: PhpList\Core\Domain\Common\Repository\AbstractRepository arguments: diff --git a/docs/ElasticsearchSearch.md b/docs/ElasticsearchSearch.md index c7ef84b4..95dd15d0 100644 --- a/docs/ElasticsearchSearch.md +++ b/docs/ElasticsearchSearch.md @@ -1,7 +1,8 @@ # Elasticsearch-backed Search for Big Tables -This document explains the generic Elasticsearch dual-write/read infrastructure and its first -consumer, `SubscriberHistory` (table `phplist_user_user_history`). +This document explains the generic Elasticsearch dual-write/read infrastructure and its consumers: +`SubscriberHistory` (table `phplist_user_user_history`) and `UserMessageBounce` +(table `phplist_user_message_bounce`). ## Overview diff --git a/src/Domain/Messaging/Model/Filter/UserMessageBounceFilter.php b/src/Domain/Messaging/Model/Filter/UserMessageBounceFilter.php new file mode 100644 index 00000000..ada2752e --- /dev/null +++ b/src/Domain/Messaging/Model/Filter/UserMessageBounceFilter.php @@ -0,0 +1,53 @@ +userId = $userId; + $this->messageId = $messageId; + $this->bounceId = $bounceId; + $this->dateFrom = $dateFrom; + $this->setLastId($lastId); + $this->setLimit($limit); + } + + public function getUserId(): ?int + { + return $this->userId; + } + + public function getMessageId(): ?int + { + return $this->messageId; + } + + public function getBounceId(): ?int + { + return $this->bounceId; + } + + public function getDateFrom(): ?DateTimeImmutable + { + return $this->dateFrom; + } +} diff --git a/src/Domain/Messaging/Model/Interfaces/UserMessageBounceRecordInterface.php b/src/Domain/Messaging/Model/Interfaces/UserMessageBounceRecordInterface.php new file mode 100644 index 00000000..37e77171 --- /dev/null +++ b/src/Domain/Messaging/Model/Interfaces/UserMessageBounceRecordInterface.php @@ -0,0 +1,26 @@ +id; + } + + public function getUserId(): int + { + return $this->userId; + } + + public function getMessageId(): int + { + return $this->messageId; + } + + public function getBounceId(): int + { + return $this->bounceId; + } + + public function getCreatedAt(): DateTime + { + return $this->createdAt; + } +} diff --git a/src/Domain/Messaging/Model/UserMessageBounce.php b/src/Domain/Messaging/Model/UserMessageBounce.php index 2a7ef519..ff6c6f05 100644 --- a/src/Domain/Messaging/Model/UserMessageBounce.php +++ b/src/Domain/Messaging/Model/UserMessageBounce.php @@ -5,10 +5,13 @@ namespace PhpList\Core\Domain\Messaging\Model; use DateTime; +use DateTimeInterface; use Doctrine\ORM\Mapping as ORM; use PhpList\Core\Domain\Common\Model\Interfaces\DomainModel; use PhpList\Core\Domain\Common\Model\Interfaces\Identity; +use PhpList\Core\Domain\Messaging\Model\Interfaces\UserMessageBounceRecordInterface; use PhpList\Core\Domain\Messaging\Repository\UserMessageBounceRepository; +use PhpList\Core\Domain\Search\Model\Interfaces\SearchIndexableInterface; #[ORM\Entity(repositoryClass: UserMessageBounceRepository::class)] #[ORM\Table(name: 'user_message_bounce')] @@ -17,8 +20,14 @@ #[ORM\Index(name: 'phplist_user_message_bounce_umbindex', columns: ['user', 'message', 'bounce'])] #[ORM\Index(name: 'phplist_user_message_bounce_useridx', columns: ['user'])] // todo: #[ORM\Index(name: 'phplist_user_message_bounce_timeidx', columns: ['time'])] -class UserMessageBounce implements DomainModel, Identity +class UserMessageBounce implements + DomainModel, + Identity, + SearchIndexableInterface, + UserMessageBounceRecordInterface { + public const SEARCH_INDEX_NAME = 'user_message_bounce'; + #[ORM\Id] #[ORM\Column(type: 'integer')] #[ORM\GeneratedValue] @@ -84,4 +93,26 @@ public function setBounceId(int $bounceId): self $this->bounceId = $bounceId; return $this; } + + public function getSearchIndexName(): string + { + return self::SEARCH_INDEX_NAME; + } + + public function getSearchDocumentId(): string + { + return (string) $this->id; + } + + public function toSearchDocument(): array + { + return [ + 'id' => $this->id, + 'idSort' => $this->id, + 'userId' => $this->userId, + 'messageId' => $this->messageId, + 'bounceId' => $this->bounceId, + 'time' => $this->createdAt->format(DateTimeInterface::ATOM), + ]; + } } diff --git a/src/Domain/Messaging/Repository/Interfaces/UserMessageBounceReaderInterface.php b/src/Domain/Messaging/Repository/Interfaces/UserMessageBounceReaderInterface.php new file mode 100644 index 00000000..46e050c5 --- /dev/null +++ b/src/Domain/Messaging/Repository/Interfaces/UserMessageBounceReaderInterface.php @@ -0,0 +1,23 @@ + */ + public function getFilteredAfterId(FilterRequestInterface $filter): PaginatedResult; + + /** @return UserMessageBounceRecordInterface[] */ + public function getByUserId(int $userId): array; +} diff --git a/src/Domain/Messaging/Repository/UserMessageBounceElasticsearchReader.php b/src/Domain/Messaging/Repository/UserMessageBounceElasticsearchReader.php new file mode 100644 index 00000000..78d7c2dc --- /dev/null +++ b/src/Domain/Messaging/Repository/UserMessageBounceElasticsearchReader.php @@ -0,0 +1,116 @@ +getUserId() !== null) { + $mustFilters[] = ['term' => ['userId' => $filter->getUserId()]]; + } + + if ($filter->getMessageId() !== null) { + $mustFilters[] = ['term' => ['messageId' => $filter->getMessageId()]]; + } + + if ($filter->getBounceId() !== null) { + $mustFilters[] = ['term' => ['bounceId' => $filter->getBounceId()]]; + } + + if ($filter->getDateFrom() !== null) { + $mustFilters[] = ['range' => ['time' => ['gte' => $filter->getDateFrom()->format(DATE_ATOM)]]]; + } + + $mustFilters[] = ['range' => ['idSort' => ['gt' => $filter->getLastId()]]]; + + $response = $this->client->search( + $this->resolvePhysicalIndexName(), + [ + 'query' => ['bool' => ['filter' => $mustFilters]], + 'sort' => [['idSort' => 'asc']], + 'size' => $filter->getLimit(), + 'track_total_hits' => true, + ], + ); + + $hits = $response['hits']['hits'] ?? []; + $lastHit = $hits !== [] ? $hits[array_key_last($hits)] : null; + + return new PaginatedResult( + items: array_map($this->hydrate(...), $hits), + total: (int) ($response['hits']['total']['value'] ?? 0), + limit: $filter->getLimit(), + lastId: $lastHit !== null ? (int) $lastHit['_source']['idSort'] : $filter->getLastId(), + ); + } + + /** @return UserMessageBounceRecordInterface[] */ + public function getByUserId(int $userId): array + { + $response = $this->client->search( + $this->resolvePhysicalIndexName(), + [ + 'query' => ['term' => ['userId' => $userId]], + 'sort' => [['idSort' => 'desc']], + // 10000 is enough, I think, but if we ever need more, we can implement pagination here too. + 'size' => 10000, + ], + ); + + $hits = $response['hits']['hits'] ?? []; + + return array_map($this->hydrate(...), $hits); + } + + /** @param array{_source: array} $hit */ + private function hydrate(array $hit): UserMessageBounceReadModel + { + $source = $hit['_source']; + + return new UserMessageBounceReadModel( + id: isset($source['id']) ? (int) $source['id'] : null, + userId: (int) $source['userId'], + messageId: (int) $source['messageId'], + bounceId: (int) $source['bounceId'], + createdAt: isset($source['time']) + ? (DateTime::createFromFormat(DATE_ATOM, $source['time']) ?: new DateTime()) + : new DateTime(), + ); + } + + private function resolvePhysicalIndexName(): string + { + return $this->indexPrefix . UserMessageBounce::SEARCH_INDEX_NAME; + } +} diff --git a/src/Domain/Messaging/Repository/UserMessageBounceRepository.php b/src/Domain/Messaging/Repository/UserMessageBounceRepository.php index c677e5c1..a7ad639b 100644 --- a/src/Domain/Messaging/Repository/UserMessageBounceRepository.php +++ b/src/Domain/Messaging/Repository/UserMessageBounceRepository.php @@ -5,20 +5,95 @@ namespace PhpList\Core\Domain\Messaging\Repository; use DateTimeInterface; +use InvalidArgumentException; +use PhpList\Core\Domain\Common\Model\Filter\FilterRequestInterface; +use PhpList\Core\Domain\Common\Model\PaginatedResult; use PhpList\Core\Domain\Common\Repository\AbstractRepository; use PhpList\Core\Domain\Common\Repository\CursorPaginationTrait; use PhpList\Core\Domain\Common\Repository\Interfaces\PaginatableRepositoryInterface; use PhpList\Core\Domain\Messaging\Model\Bounce; +use PhpList\Core\Domain\Messaging\Model\Filter\UserMessageBounceFilter; use PhpList\Core\Domain\Messaging\Model\Message; use PhpList\Core\Domain\Messaging\Model\UserMessage; use PhpList\Core\Domain\Messaging\Model\UserMessageBounce; +use PhpList\Core\Domain\Messaging\Repository\Interfaces\UserMessageBounceReaderInterface; use PhpList\Core\Domain\Subscription\Model\Subscriber; use PhpList\Core\Domain\Subscription\Model\Subscription; -class UserMessageBounceRepository extends AbstractRepository implements PaginatableRepositoryInterface +class UserMessageBounceRepository extends AbstractRepository implements + PaginatableRepositoryInterface, + UserMessageBounceReaderInterface { use CursorPaginationTrait; + /** + * @return PaginatedResult + * @throws InvalidArgumentException + */ + public function getFilteredAfterId(FilterRequestInterface $filter): PaginatedResult + { + if (!$filter instanceof UserMessageBounceFilter) { + throw new InvalidArgumentException('Expected UserMessageBounceFilter.'); + } + + $lastId = $filter->getLastId(); + $limit = $filter->getLimit(); + $queryBuilder = $this->createQueryBuilder('umb'); + + if ($filter->getUserId() !== null) { + $queryBuilder->andWhere('umb.userId = :userId') + ->setParameter('userId', $filter->getUserId()); + } + + if ($filter->getMessageId() !== null) { + $queryBuilder->andWhere('umb.messageId = :messageId') + ->setParameter('messageId', $filter->getMessageId()); + } + + if ($filter->getBounceId() !== null) { + $queryBuilder->andWhere('umb.bounceId = :bounceId') + ->setParameter('bounceId', $filter->getBounceId()); + } + + if ($filter->getDateFrom() !== null) { + $queryBuilder->andWhere('umb.createdAt >= :dateFrom') + ->setParameter('dateFrom', $filter->getDateFrom()); + } + + $countQb = clone $queryBuilder; + $total = (int) $countQb + ->select('COUNT(DISTINCT umb.id)') + ->getQuery() + ->getSingleScalarResult(); + + /** @var list $items */ + $items = $queryBuilder + ->andWhere('umb.id > :lastId') + ->setParameter('lastId', $lastId) + ->orderBy('umb.id', 'ASC') + ->setMaxResults($limit) + ->getQuery() + ->getResult(); + + return new PaginatedResult( + items: $items, + total: $total, + limit: $limit, + lastId: $lastId, + ); + } + + /** @return UserMessageBounce[] */ + public function getByUserId(int $userId): array + { + return $this->createQueryBuilder('umb') + ->andWhere('umb.userId = :userId') + ->setParameter('userId', $userId) + ->orderBy('umb.id', 'DESC') + ->getQuery() + ->getResult(); + } + public function getCountByMessageId(int $messageId): int { return (int) $this->createQueryBuilder('umb') diff --git a/src/Domain/Messaging/Service/Search/UserMessageBounceIndexDefinition.php b/src/Domain/Messaging/Service/Search/UserMessageBounceIndexDefinition.php new file mode 100644 index 00000000..29292169 --- /dev/null +++ b/src/Domain/Messaging/Service/Search/UserMessageBounceIndexDefinition.php @@ -0,0 +1,37 @@ + [ + 'id' => ['type' => 'keyword'], + // Numeric mirror of `id`, used for range/sort in cursor pagination - `id` stays a + // keyword for exact-match filtering. + 'idSort' => ['type' => 'long'], + 'userId' => ['type' => 'keyword'], + 'messageId' => ['type' => 'keyword'], + 'bounceId' => ['type' => 'keyword'], + 'time' => ['type' => 'date'], + ], + ]; + } + + public function getSettings(): array + { + return []; + } +} diff --git a/src/Domain/Messaging/Service/Search/UserMessageBounceReindexProvider.php b/src/Domain/Messaging/Service/Search/UserMessageBounceReindexProvider.php new file mode 100644 index 00000000..34c9be22 --- /dev/null +++ b/src/Domain/Messaging/Service/Search/UserMessageBounceReindexProvider.php @@ -0,0 +1,40 @@ +repository->createQueryBuilder('umb') + ->select('COUNT(umb.id)') + ->getQuery() + ->getSingleScalarResult(); + } + + public function fetchBatch(int $lastId, int $batchSize): iterable + { + return $this->repository->createQueryBuilder('umb') + ->andWhere('umb.id > :lastId') + ->setParameter('lastId', $lastId) + ->orderBy('umb.id', 'ASC') + ->setMaxResults($batchSize) + ->getQuery() + ->toIterable(); + } +} diff --git a/tests/Unit/Domain/Messaging/Repository/UserMessageBounceElasticsearchReaderTest.php b/tests/Unit/Domain/Messaging/Repository/UserMessageBounceElasticsearchReaderTest.php new file mode 100644 index 00000000..30dd31e2 --- /dev/null +++ b/tests/Unit/Domain/Messaging/Repository/UserMessageBounceElasticsearchReaderTest.php @@ -0,0 +1,148 @@ +client = $this->createMock(ElasticsearchClientInterface::class); + $this->reader = new UserMessageBounceElasticsearchReader($this->client, 'phplist_'); + } + + public function testGetFilteredAfterIdQueriesPrefixedIndexAndHydratesResults(): void + { + $filter = new UserMessageBounceFilter(lastId: 5, limit: 10); + + $this->client + ->expects($this->once()) + ->method('search') + ->with( + 'phplist_user_message_bounce', + $this->callback(function (array $query): bool { + return $query['size'] === 10 + && $query['query']['bool']['filter'][0] === ['range' => ['idSort' => ['gt' => 5]]]; + }), + ) + ->willReturn([ + 'hits' => [ + 'total' => ['value' => 1], + 'hits' => [ + ['_source' => [ + 'id' => 7, + 'idSort' => 7, + 'userId' => 3, + 'messageId' => 42, + 'bounceId' => 99, + 'time' => '2026-01-01T00:00:00+00:00', + ]], + ], + ], + ]); + + $result = $this->reader->getFilteredAfterId($filter); + + $this->assertSame(1, $result->getTotal()); + $this->assertCount(1, $result->getItems()); + $this->assertSame(7, $result->getItems()[0]->getId()); + $this->assertSame(3, $result->getItems()[0]->getUserId()); + $this->assertSame(42, $result->getItems()[0]->getMessageId()); + $this->assertSame(99, $result->getItems()[0]->getBounceId()); + } + + public function testGetFilteredAfterIdPaginatesAcrossTwoPagesWithoutRepeatingResults(): void + { + $firstFilter = new UserMessageBounceFilter(lastId: 0, limit: 1); + + $this->client + ->expects($this->exactly(2)) + ->method('search') + ->willReturnOnConsecutiveCalls( + [ + 'hits' => [ + 'total' => ['value' => 2], + 'hits' => [ + ['_source' => [ + 'id' => 5, + 'idSort' => 5, + 'userId' => 1, + 'messageId' => 10, + 'bounceId' => 20, + 'time' => '2026-01-01T00:00:00+00:00', + ]], + ], + ], + ], + [ + 'hits' => [ + 'total' => ['value' => 2], + 'hits' => [ + ['_source' => [ + 'id' => 8, + 'idSort' => 8, + 'userId' => 2, + 'messageId' => 11, + 'bounceId' => 21, + 'time' => '2026-01-02T00:00:00+00:00', + ]], + ], + ], + ], + ); + + $firstPage = $this->reader->getFilteredAfterId($firstFilter); + + $this->assertSame(5, $firstPage->getLastId()); + $this->assertSame(5, $firstPage->getItems()[0]->getId()); + + $secondFilter = new UserMessageBounceFilter(lastId: $firstPage->getLastId(), limit: 1); + $secondPage = $this->reader->getFilteredAfterId($secondFilter); + + $this->assertSame(8, $secondPage->getLastId()); + $this->assertSame(8, $secondPage->getItems()[0]->getId()); + $this->assertNotSame( + $firstPage->getItems()[0]->getId(), + $secondPage->getItems()[0]->getId(), + ); + } + + public function testGetFilteredAfterIdRejectsWrongFilterType(): void + { + $wrongFilter = $this->createMock(FilterRequestInterface::class); + + $this->expectException(InvalidArgumentException::class); + $this->reader->getFilteredAfterId($wrongFilter); + } + + public function testGetByUserIdSortsDescending(): void + { + $this->client + ->expects($this->once()) + ->method('search') + ->with( + 'phplist_user_message_bounce', + $this->callback(function (array $query): bool { + return $query['query'] === ['term' => ['userId' => 9]] + && $query['sort'] === [['idSort' => 'desc']]; + }), + ) + ->willReturn(['hits' => ['hits' => []]]); + + $result = $this->reader->getByUserId(9); + + $this->assertSame([], $result); + } +} diff --git a/tests/Unit/Domain/Messaging/Service/Search/UserMessageBounceIndexDefinitionTest.php b/tests/Unit/Domain/Messaging/Service/Search/UserMessageBounceIndexDefinitionTest.php new file mode 100644 index 00000000..29214e51 --- /dev/null +++ b/tests/Unit/Domain/Messaging/Service/Search/UserMessageBounceIndexDefinitionTest.php @@ -0,0 +1,37 @@ +assertSame('user_message_bounce', $definition->getIndexAlias()); + } + + public function testMappingDeclaresExpectedFields(): void + { + $definition = new UserMessageBounceIndexDefinition(); + $properties = $definition->getMapping()['properties']; + + foreach (['id', 'idSort', 'userId', 'messageId', 'bounceId', 'time'] as $field) { + $this->assertArrayHasKey($field, $properties); + } + $this->assertSame('long', $properties['idSort']['type']); + $this->assertSame('keyword', $properties['id']['type']); + } + + public function testSettingsAreEmptyByDefault(): void + { + $definition = new UserMessageBounceIndexDefinition(); + + $this->assertSame([], $definition->getSettings()); + } +} diff --git a/tests/Unit/Domain/Messaging/Service/Search/UserMessageBounceReindexProviderTest.php b/tests/Unit/Domain/Messaging/Service/Search/UserMessageBounceReindexProviderTest.php new file mode 100644 index 00000000..8009a0d9 --- /dev/null +++ b/tests/Unit/Domain/Messaging/Service/Search/UserMessageBounceReindexProviderTest.php @@ -0,0 +1,19 @@ +createMock(UserMessageBounceRepository::class)); + + $this->assertSame('user_message_bounce', $provider->getAlias()); + } +} From a109fd2ead0a6e042588358bb8c419f583bbbc2a Mon Sep 17 00:00:00 2001 From: Tatevik Date: Thu, 3 Sep 2026 12:06:00 +0400 Subject: [PATCH 15/24] bugfix --- src/Domain/Messaging/Repository/UserMessageBounceRepository.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Domain/Messaging/Repository/UserMessageBounceRepository.php b/src/Domain/Messaging/Repository/UserMessageBounceRepository.php index a7ad639b..c625636d 100644 --- a/src/Domain/Messaging/Repository/UserMessageBounceRepository.php +++ b/src/Domain/Messaging/Repository/UserMessageBounceRepository.php @@ -263,7 +263,7 @@ public function getUserMessageHistoryWithBounces(Subscriber $subscriber): array ->andWhere('um.status = :status') ->setParameter('userId', $subscriber->getId()) ->setParameter('status', 'sent') - ->orderBy('um.entered', 'DESC') + ->orderBy('um.createdAt', 'DESC') ->getQuery() ->getResult(); } From c81ea203b2b452a66d41b2f6c6bc1b94fadeeb31 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Thu, 3 Sep 2026 12:10:38 +0400 Subject: [PATCH 16/24] Refactor AnalyticsService to use UserMessageBounceReaderInterface for bounce data retrieval --- config/services/elasticsearch.yml | 4 ++ .../Analytics/Service/AnalyticsService.php | 10 +-- .../UserMessageBounceReaderInterface.php | 7 ++ .../UserMessageBounceElasticsearchReader.php | 49 +++++++++++++ .../Service/AnalyticsServiceTest.php | 12 ++-- ...erMessageBounceElasticsearchReaderTest.php | 68 +++++++++++++++++++ 6 files changed, 139 insertions(+), 11 deletions(-) diff --git a/config/services/elasticsearch.yml b/config/services/elasticsearch.yml index 821025e0..5507c857 100644 --- a/config/services/elasticsearch.yml +++ b/config/services/elasticsearch.yml @@ -50,5 +50,9 @@ services: arguments: $indexPrefix: '%elasticsearch.index_prefix%' + PhpList\Core\Domain\Messaging\Repository\UserMessageBounceElasticsearchHybridReader: + arguments: + $indexPrefix: '%elasticsearch.index_prefix%' + PhpList\Core\Domain\Messaging\Service\Search\: resource: '../../src/Domain/Messaging/Service/Search' diff --git a/src/Domain/Analytics/Service/AnalyticsService.php b/src/Domain/Analytics/Service/AnalyticsService.php index e0ff6985..170a24be 100644 --- a/src/Domain/Analytics/Service/AnalyticsService.php +++ b/src/Domain/Analytics/Service/AnalyticsService.php @@ -10,8 +10,8 @@ use PhpList\Core\Domain\Analytics\Service\Manager\LinkTrackManager; use PhpList\Core\Domain\Analytics\Service\Manager\UserMessageViewManager; use PhpList\Core\Domain\Messaging\Model\Filter\MessageFilter; +use PhpList\Core\Domain\Messaging\Repository\Interfaces\UserMessageBounceReaderInterface; use PhpList\Core\Domain\Messaging\Repository\MessageRepository; -use PhpList\Core\Domain\Messaging\Repository\UserMessageBounceRepository; use PhpList\Core\Domain\Messaging\Repository\UserMessageForwardRepository; use PhpList\Core\Domain\Messaging\Repository\UserMessageRepository; use PhpList\Core\Domain\Subscription\Repository\SubscriberRepository; @@ -22,7 +22,7 @@ public function __construct( private readonly LinkTrackManager $linkTrackManager, private readonly UserMessageViewManager $userMessageViewManager, private readonly MessageRepository $messageRepository, - private readonly UserMessageBounceRepository $messageBounceRepository, + private readonly UserMessageBounceReaderInterface $messageBounceReader, private readonly UserMessageForwardRepository $messageForwardRepository, private readonly SubscriberRepository $subscriberRepository, private readonly UserMessageRepository $userMessageRepository, @@ -68,7 +68,7 @@ public function getCampaignStatistics(int $limit = 50, int $lastId = 0): array } $uniqueClicks = count($uniqueClickers); - $bounces = $this->messageBounceRepository->getCountByMessageId($message->getId()); + $bounces = $this->messageBounceReader->getCountByMessageId($message->getId()); $forwards = $this->messageForwardRepository->getCountByMessageId($message->getId()); $sentDate = $message->getMetadata()->getSent(); $sentCount = $message->getMetadata()->getBounceCount() + $views; @@ -178,11 +178,11 @@ public function getSummaryStatistics(): array $sentTotal = $this->userMessageRepository->countSentBetween($thisMonthStart, $now); $openTotal = $this->userMessageViewRepository->countBetween($thisMonthStart, $now); - $bounceTotal = $this->messageBounceRepository->countBetween($thisMonthStart, $now); + $bounceTotal = $this->messageBounceReader->countBetween($thisMonthStart, $now); $sentTotalLastMonth = $this->userMessageRepository->countSentBetween($lastMonthStart, $lastMonthEnd); $openTotalLastMonth = $this->userMessageViewRepository->countBetween($lastMonthStart, $lastMonthEnd); - $bounceTotalLastMonth = $this->messageBounceRepository->countBetween($lastMonthStart, $lastMonthEnd); + $bounceTotalLastMonth = $this->messageBounceReader->countBetween($lastMonthStart, $lastMonthEnd); $openRate = $this->calculateRate($openTotal, $sentTotal); $openRateLastMonth = $this->calculateRate($openTotalLastMonth, $sentTotalLastMonth); diff --git a/src/Domain/Messaging/Repository/Interfaces/UserMessageBounceReaderInterface.php b/src/Domain/Messaging/Repository/Interfaces/UserMessageBounceReaderInterface.php index 46e050c5..083be412 100644 --- a/src/Domain/Messaging/Repository/Interfaces/UserMessageBounceReaderInterface.php +++ b/src/Domain/Messaging/Repository/Interfaces/UserMessageBounceReaderInterface.php @@ -4,6 +4,7 @@ namespace PhpList\Core\Domain\Messaging\Repository\Interfaces; +use DateTimeInterface; use PhpList\Core\Domain\Common\Model\Filter\FilterRequestInterface; use PhpList\Core\Domain\Common\Model\PaginatedResult; use PhpList\Core\Domain\Messaging\Model\Interfaces\UserMessageBounceRecordInterface; @@ -20,4 +21,10 @@ public function getFilteredAfterId(FilterRequestInterface $filter): PaginatedRes /** @return UserMessageBounceRecordInterface[] */ public function getByUserId(int $userId): array; + + public function getCountByMessageId(int $messageId): int; + + public function countBetween(DateTimeInterface $start, DateTimeInterface $end): int; + + public function existsByMessageIdAndUserId(int $messageId, int $subscriberId): bool; } diff --git a/src/Domain/Messaging/Repository/UserMessageBounceElasticsearchReader.php b/src/Domain/Messaging/Repository/UserMessageBounceElasticsearchReader.php index 78d7c2dc..e2de944e 100644 --- a/src/Domain/Messaging/Repository/UserMessageBounceElasticsearchReader.php +++ b/src/Domain/Messaging/Repository/UserMessageBounceElasticsearchReader.php @@ -5,6 +5,7 @@ namespace PhpList\Core\Domain\Messaging\Repository; use DateTime; +use DateTimeInterface; use InvalidArgumentException; use PhpList\Core\Domain\Common\Model\Filter\FilterRequestInterface; use PhpList\Core\Domain\Common\Model\PaginatedResult; @@ -93,6 +94,54 @@ public function getByUserId(int $userId): array return array_map($this->hydrate(...), $hits); } + public function getCountByMessageId(int $messageId): int + { + $response = $this->client->search( + $this->resolvePhysicalIndexName(), + [ + 'query' => ['term' => ['messageId' => $messageId]], + 'size' => 0, + 'track_total_hits' => true, + ], + ); + + return (int) ($response['hits']['total']['value'] ?? 0); + } + + public function countBetween(DateTimeInterface $start, DateTimeInterface $end): int + { + $response = $this->client->search( + $this->resolvePhysicalIndexName(), + [ + 'query' => ['range' => ['time' => [ + 'gte' => $start->format(DateTimeInterface::ATOM), + 'lte' => $end->format(DateTimeInterface::ATOM), + ]]], + 'size' => 0, + 'track_total_hits' => true, + ], + ); + + return (int) ($response['hits']['total']['value'] ?? 0); + } + + public function existsByMessageIdAndUserId(int $messageId, int $subscriberId): bool + { + $response = $this->client->search( + $this->resolvePhysicalIndexName(), + [ + 'query' => ['bool' => ['filter' => [ + ['term' => ['messageId' => $messageId]], + ['term' => ['userId' => $subscriberId]], + ]]], + 'size' => 0, + 'track_total_hits' => true, + ], + ); + + return ((int) ($response['hits']['total']['value'] ?? 0)) > 0; + } + /** @param array{_source: array} $hit */ private function hydrate(array $hit): UserMessageBounceReadModel { diff --git a/tests/Unit/Domain/Analytics/Service/AnalyticsServiceTest.php b/tests/Unit/Domain/Analytics/Service/AnalyticsServiceTest.php index a7558747..3b4cb7f9 100644 --- a/tests/Unit/Domain/Analytics/Service/AnalyticsServiceTest.php +++ b/tests/Unit/Domain/Analytics/Service/AnalyticsServiceTest.php @@ -17,8 +17,8 @@ use PhpList\Core\Domain\Messaging\Model\Message; use PhpList\Core\Domain\Messaging\Model\Message\MessageContent; use PhpList\Core\Domain\Messaging\Model\Message\MessageMetadata; +use PhpList\Core\Domain\Messaging\Repository\Interfaces\UserMessageBounceReaderInterface; use PhpList\Core\Domain\Messaging\Repository\MessageRepository; -use PhpList\Core\Domain\Messaging\Repository\UserMessageBounceRepository; use PhpList\Core\Domain\Messaging\Repository\UserMessageForwardRepository; use PhpList\Core\Domain\Messaging\Repository\UserMessageRepository; use PhpList\Core\Domain\Subscription\Repository\SubscriberRepository; @@ -31,7 +31,7 @@ class AnalyticsServiceTest extends TestCase private LinkTrackManager|MockObject $linkTrackManager; private UserMessageViewManager|MockObject $userMessageViewManager; private MessageRepository|MockObject $messageRepository; - private UserMessageBounceRepository|MockObject $userMessageBounceRepository; + private UserMessageBounceReaderInterface|MockObject $userMessageBounceReader; private UserMessageForwardRepository|MockObject $userMessageForwardRepository; private SubscriberRepository|MockObject $subscriberRepository; private UserMessageRepository|MockObject $userMessageRepository; @@ -42,7 +42,7 @@ protected function setUp(): void $this->linkTrackManager = $this->createMock(LinkTrackManager::class); $this->userMessageViewManager = $this->createMock(UserMessageViewManager::class); $this->messageRepository = $this->createMock(MessageRepository::class); - $this->userMessageBounceRepository = $this->createMock(UserMessageBounceRepository::class); + $this->userMessageBounceReader = $this->createMock(UserMessageBounceReaderInterface::class); $this->userMessageForwardRepository = $this->createMock(UserMessageForwardRepository::class); $this->subscriberRepository = $this->createMock(SubscriberRepository::class); $this->userMessageRepository = $this->createMock(UserMessageRepository::class); @@ -52,7 +52,7 @@ protected function setUp(): void $this->linkTrackManager, $this->userMessageViewManager, $this->messageRepository, - $this->userMessageBounceRepository, + $this->userMessageBounceReader, $this->userMessageForwardRepository, $this->subscriberRepository, $this->userMessageRepository, @@ -109,7 +109,7 @@ public function testGetCampaignStatistics(): void ->with($messageId) ->willReturn([$linkTrack1, $linkTrack2]); - $this->userMessageBounceRepository->expects(self::once()) + $this->userMessageBounceReader->expects(self::once()) ->method('getCountByMessageId') ->with($messageId) ->willReturn(3); @@ -291,7 +291,7 @@ public function testGetSummaryStatistics(): void $this->userMessageRepository->method('countSentBetween')->willReturnOnConsecutiveCalls(500, 400); $this->userMessageViewRepository->method('countBetween')->willReturnOnConsecutiveCalls(250, 160); - $this->userMessageBounceRepository->method('countBetween')->willReturnOnConsecutiveCalls(10, 8); + $this->userMessageBounceReader->method('countBetween')->willReturnOnConsecutiveCalls(10, 8); $result = $this->subject->getSummaryStatistics(); diff --git a/tests/Unit/Domain/Messaging/Repository/UserMessageBounceElasticsearchReaderTest.php b/tests/Unit/Domain/Messaging/Repository/UserMessageBounceElasticsearchReaderTest.php index 30dd31e2..bcbadcc5 100644 --- a/tests/Unit/Domain/Messaging/Repository/UserMessageBounceElasticsearchReaderTest.php +++ b/tests/Unit/Domain/Messaging/Repository/UserMessageBounceElasticsearchReaderTest.php @@ -4,6 +4,7 @@ namespace PhpList\Core\Tests\Unit\Domain\Messaging\Repository; +use DateTime; use InvalidArgumentException; use PhpList\Core\Domain\Common\Model\Filter\FilterRequestInterface; use PhpList\Core\Domain\Messaging\Model\Filter\UserMessageBounceFilter; @@ -145,4 +146,71 @@ public function testGetByUserIdSortsDescending(): void $this->assertSame([], $result); } + + public function testGetCountByMessageIdQueriesTotalHitsForMessage(): void + { + $this->client + ->expects($this->once()) + ->method('search') + ->with( + 'phplist_user_message_bounce', + $this->callback(function (array $query): bool { + return $query['query'] === ['term' => ['messageId' => 42]] + && $query['size'] === 0; + }), + ) + ->willReturn(['hits' => ['total' => ['value' => 7]]]); + + $this->assertSame(7, $this->reader->getCountByMessageId(42)); + } + + public function testCountBetweenQueriesTimeRange(): void + { + $start = new DateTime('2026-01-01 00:00:00'); + $end = new DateTime('2026-01-31 23:59:59'); + + $this->client + ->expects($this->once()) + ->method('search') + ->with( + 'phplist_user_message_bounce', + $this->callback(function (array $query) use ($start, $end): bool { + return $query['query'] === ['range' => ['time' => [ + 'gte' => $start->format(DATE_ATOM), + 'lte' => $end->format(DATE_ATOM), + ]]]; + }), + ) + ->willReturn(['hits' => ['total' => ['value' => 3]]]); + + $this->assertSame(3, $this->reader->countBetween($start, $end)); + } + + public function testExistsByMessageIdAndUserIdReturnsTrueWhenHitsExist(): void + { + $this->client + ->expects($this->once()) + ->method('search') + ->with( + 'phplist_user_message_bounce', + $this->callback(function (array $query): bool { + return $query['query']['bool']['filter'] === [ + ['term' => ['messageId' => 5]], + ['term' => ['userId' => 9]], + ]; + }), + ) + ->willReturn(['hits' => ['total' => ['value' => 1]]]); + + $this->assertTrue($this->reader->existsByMessageIdAndUserId(5, 9)); + } + + public function testExistsByMessageIdAndUserIdReturnsFalseWhenNoHits(): void + { + $this->client + ->method('search') + ->willReturn(['hits' => ['total' => ['value' => 0]]]); + + $this->assertFalse($this->reader->existsByMessageIdAndUserId(5, 9)); + } } From cbe8cf15b31fd5ad429fdfa5883807643a47b6fc Mon Sep 17 00:00:00 2001 From: Tatevik Date: Thu, 3 Sep 2026 12:42:29 +0400 Subject: [PATCH 17/24] UserMessageBounceElasticsearchHybridReader --- .../Doctrine/SearchIndexDoctrineListener.php | 63 +++- ...MessageBounceElasticsearchHybridReader.php | 347 ++++++++++++++++++ .../UserMessageBounceElasticsearchReader.php | 2 + ...ageBounceElasticsearchHybridReaderTest.php | 318 ++++++++++++++++ 4 files changed, 718 insertions(+), 12 deletions(-) create mode 100644 src/Domain/Messaging/Repository/UserMessageBounceElasticsearchHybridReader.php create mode 100644 tests/Integration/Domain/Messaging/Repository/UserMessageBounceElasticsearchHybridReaderTest.php diff --git a/src/Core/Doctrine/SearchIndexDoctrineListener.php b/src/Core/Doctrine/SearchIndexDoctrineListener.php index c18ffa03..71eeae2b 100644 --- a/src/Core/Doctrine/SearchIndexDoctrineListener.php +++ b/src/Core/Doctrine/SearchIndexDoctrineListener.php @@ -9,6 +9,7 @@ use Doctrine\ORM\Event\PostPersistEventArgs; use Doctrine\ORM\Event\PostRemoveEventArgs; use Doctrine\ORM\Event\PostUpdateEventArgs; +use Doctrine\ORM\Event\PreRemoveEventArgs; use Doctrine\ORM\Events; use PhpList\Core\Domain\Search\Message\IndexDocumentMessage; use PhpList\Core\Domain\Search\Model\Interfaces\SearchIndexableInterface; @@ -25,9 +26,14 @@ * postFlush, once the transaction has actually committed. This trades perfect atomicity (a crash * between commit and dispatch loses one message - repaired by `phplist:search:reindex`) for the far * more important guarantee of never indexing/deleting a row that was rolled back. + * + * preRemove captures getSearchIndexName()/getSearchDocumentId() before the delete happens: for + * entities with a Doctrine-generated id, UnitOfWork::executeDeletions() nulls the identifier before + * postRemove fires, so reading it there would queue a delete with an empty document id. */ #[AsDoctrineListener(event: Events::postPersist)] #[AsDoctrineListener(event: Events::postUpdate)] +#[AsDoctrineListener(event: Events::preRemove)] #[AsDoctrineListener(event: Events::postRemove)] #[AsDoctrineListener(event: Events::postFlush)] class SearchIndexDoctrineListener @@ -35,23 +41,56 @@ class SearchIndexDoctrineListener /** @var array */ private array $pending = []; + /** @var array keyed by spl_object_id() */ + private array $removalKeys = []; + public function __construct(private readonly MessageBusInterface $messageBus) { } public function postPersist(PostPersistEventArgs $args): void { - $this->queue($args->getObject(), SearchOperation::Index); + $entity = $args->getObject(); + if (!$entity instanceof SearchIndexableInterface) { + return; + } + + $this->queue($entity, SearchOperation::Index, $entity->getSearchIndexName(), $entity->getSearchDocumentId()); } public function postUpdate(PostUpdateEventArgs $args): void { - $this->queue($args->getObject(), SearchOperation::Index); + $entity = $args->getObject(); + if (!$entity instanceof SearchIndexableInterface) { + return; + } + + $this->queue($entity, SearchOperation::Index, $entity->getSearchIndexName(), $entity->getSearchDocumentId()); + } + + public function preRemove(PreRemoveEventArgs $args): void + { + $entity = $args->getObject(); + if (!$entity instanceof SearchIndexableInterface) { + return; + } + + $this->removalKeys[spl_object_id($entity)] = [$entity->getSearchIndexName(), $entity->getSearchDocumentId()]; } public function postRemove(PostRemoveEventArgs $args): void { - $this->queue($args->getObject(), SearchOperation::Delete); + $entity = $args->getObject(); + if (!$entity instanceof SearchIndexableInterface) { + return; + } + + $objectId = spl_object_id($entity); + [$indexName, $documentId] = $this->removalKeys[$objectId] + ?? [$entity->getSearchIndexName(), $entity->getSearchDocumentId()]; + unset($this->removalKeys[$objectId]); + + $this->queue($entity, SearchOperation::Delete, $indexName, $documentId); } public function postFlush(PostFlushEventArgs $args): void @@ -68,18 +107,18 @@ public function postFlush(PostFlushEventArgs $args): void } } - private function queue(object $entity, SearchOperation $operation): void - { - if (!$entity instanceof SearchIndexableInterface) { - return; - } - - $key = $entity->getSearchIndexName() . '|' . $entity->getSearchDocumentId(); + private function queue( + SearchIndexableInterface $entity, + SearchOperation $operation, + string $indexName, + string $documentId, + ): void { + $key = $indexName . '|' . $documentId; $document = $operation === SearchOperation::Index ? $entity->toSearchDocument() : []; $this->pending[$key] = new IndexDocumentMessage( - $entity->getSearchIndexName(), - $entity->getSearchDocumentId(), + $indexName, + $documentId, $document, $operation, $this->nextRevision(), diff --git a/src/Domain/Messaging/Repository/UserMessageBounceElasticsearchHybridReader.php b/src/Domain/Messaging/Repository/UserMessageBounceElasticsearchHybridReader.php new file mode 100644 index 00000000..7e823c8a --- /dev/null +++ b/src/Domain/Messaging/Repository/UserMessageBounceElasticsearchHybridReader.php @@ -0,0 +1,347 @@ + + */ + public function getListBounceTotals(int $listId): array + { + $rows = $this->entityManager->createQueryBuilder() + ->select( + 'subscriber.id AS subscriberId', + 'subscriber.email AS email', + 'subscriber.confirmed AS confirmed', + 'subscriber.blacklisted AS blacklisted' + ) + ->from(Subscriber::class, 'subscriber') + ->innerJoin(Subscription::class, 'subscription', 'ON', 'subscription.subscriber = subscriber') + ->where('IDENTITY(subscription.subscriberList) = :listId') + ->setParameter('listId', $listId) + ->groupBy('subscriber.id, subscriber.email, subscriber.confirmed, subscriber.blacklisted') + ->orderBy('subscriber.id', 'ASC') + ->getQuery() + ->getArrayResult(); + + if ($rows === []) { + return []; + } + + $subscriberIds = array_map(static fn (array $row): int => (int) $row['subscriberId'], $rows); + $totalsByUserId = $this->countsByTermsField('userId', $subscriberIds); + + $result = []; + foreach ($rows as $row) { + $subscriberId = (int) $row['subscriberId']; + $totalBounces = $totalsByUserId[$subscriberId] ?? 0; + + if ($totalBounces === 0) { + continue; + } + + $result[] = [ + 'subscriber_id' => $subscriberId, + 'email' => (string) $row['email'], + 'confirmed' => (bool) $row['confirmed'], + 'blacklisted' => (bool) $row['blacklisted'], + 'total_bounces' => $totalBounces, + ]; + } + + return $result; + } + + /** @return array */ + public function getCampaignBounceTotals(?int $ownerId = null): array + { + $queryBuilder = $this->entityManager->createQueryBuilder() + ->select('m.id AS messageId', 'm.content.subject AS subject') + ->from(Message::class, 'm') + ->orderBy('m.id', 'ASC'); + + if ($ownerId !== null) { + $queryBuilder + ->andWhere('IDENTITY(m.owner) = :ownerId') + ->setParameter('ownerId', $ownerId); + } + + /** @var array $rows */ + $rows = $queryBuilder->getQuery()->getArrayResult(); + + if ($rows === []) { + return []; + } + + $messageIds = array_map(static fn (array $row): int => (int) $row['messageId'], $rows); + $totalsByMessageId = $this->countsByTermsField('messageId', $messageIds); + + $result = []; + foreach ($rows as $row) { + $messageId = (int) $row['messageId']; + $totalBounces = $totalsByMessageId[$messageId] ?? 0; + + if ($totalBounces === 0) { + continue; + } + + $result[] = [ + 'message_id' => $messageId, + 'subject' => $row['subject'], + 'total_bounces' => $totalBounces, + ]; + } + + return $result; + } + + /** @return array */ + public function getPaginatedWithJoinNoRelation(int $fromId, int $limit): array + { + $response = $this->client->search( + $this->resolvePhysicalIndexName(), + [ + 'query' => ['range' => ['idSort' => ['gt' => $fromId]]], + 'sort' => [['idSort' => 'asc']], + 'size' => $limit, + ], + ); + + $hits = $response['hits']['hits'] ?? []; + if ($hits === []) { + return []; + } + + $records = array_map($this->hydrate(...), $hits); + $bounceIds = array_values(array_unique(array_map( + static fn (UserMessageBounceRecordInterface $record): int => $record->getBounceId(), + $records + ))); + + $bouncesById = $this->findBouncesByIdIndexedById($bounceIds); + + $result = []; + foreach ($records as $record) { + $bounce = $bouncesById[$record->getBounceId()] ?? null; + + if ($bounce === null) { + continue; + } + + $result[] = ['umb' => $record, 'bounce' => $bounce]; + } + + return $result; + } + + /** + * @return array + */ + public function getUserMessageHistoryWithBounces(Subscriber $subscriber): array + { + /** @var UserMessage[] $userMessages */ + $userMessages = $this->entityManager->createQueryBuilder() + ->select('um') + ->from(UserMessage::class, 'um') + ->where('um.user = :userId') + ->andWhere('um.status = :status') + ->setParameter('userId', $subscriber->getId()) + ->setParameter('status', 'sent') + ->orderBy('um.createdAt', 'DESC') + ->getQuery() + ->getResult(); + + if ($userMessages === []) { + return []; + } + + $docsByMessageId = $this->groupByMessageId($this->fetchDocsByUserId((int) $subscriber->getId())); + $bouncesById = $this->findBouncesByIdIndexedById($this->bounceIdsUsedIn($docsByMessageId)); + + $result = []; + foreach ($userMessages as $userMessage) { + $docs = $docsByMessageId[$userMessage->getMessage()->getId()] ?? []; + + if ($docs === []) { + $result[] = ['um' => $userMessage, 'umb' => null, 'b' => null]; + continue; + } + + foreach ($docs as $doc) { + $result[] = [ + 'um' => $userMessage, + 'umb' => $doc, + 'b' => $bouncesById[$doc->getBounceId()] ?? null, + ]; + } + } + + return $result; + } + + /** @return UserMessageBounceRecordInterface[] */ + private function fetchDocsByUserId(int $userId): array + { + $response = $this->client->search( + $this->resolvePhysicalIndexName(), + [ + 'query' => ['term' => ['userId' => $userId]], + 'sort' => [['idSort' => 'desc']], + 'size' => 10000, + ], + ); + + $hits = $response['hits']['hits'] ?? []; + + return array_map($this->hydrate(...), $hits); + } + + /** + * @param UserMessageBounceRecordInterface[] $docs + * @return array + */ + private function groupByMessageId(array $docs): array + { + $docsByMessageId = []; + foreach ($docs as $doc) { + $docsByMessageId[$doc->getMessageId()][] = $doc; + } + + return $docsByMessageId; + } + + /** + * @param array $docsByMessageId + * @return int[] + */ + private function bounceIdsUsedIn(array $docsByMessageId): array + { + $bounceIds = []; + foreach ($docsByMessageId as $docs) { + foreach ($docs as $doc) { + $bounceIds[] = $doc->getBounceId(); + } + } + + return array_values(array_unique($bounceIds)); + } + + /** + * @param int[] $bounceIds + * @return array + */ + private function findBouncesByIdIndexedById(array $bounceIds): array + { + if ($bounceIds === []) { + return []; + } + + $bouncesById = []; + /** @var Bounce $bounce */ + foreach ($this->entityManager->getRepository(Bounce::class)->findBy(['id' => $bounceIds]) as $bounce) { + $bouncesById[$bounce->getId()] = $bounce; + } + + return $bouncesById; + } + + /** @param array{_source: array} $hit */ + private function hydrate(array $hit): UserMessageBounceReadModel + { + $source = $hit['_source']; + + return new UserMessageBounceReadModel( + id: isset($source['id']) ? (int) $source['id'] : null, + userId: (int) $source['userId'], + messageId: (int) $source['messageId'], + bounceId: (int) $source['bounceId'], + createdAt: isset($source['time']) + ? (DateTime::createFromFormat(DATE_ATOM, $source['time']) ?: new DateTime()) + : new DateTime(), + ); + } + + /** + * Aggregates document counts by an exact-match field, restricted to a given set of ids - used to + * correlate bounce counts (Elasticsearch) with rows from a small, non-"big table" DB query + * (subscribers in a list, messages owned by an admin) without joining across data stores. + * + * @param int[] $ids + * @return array counts keyed by id + */ + private function countsByTermsField(string $field, array $ids): array + { + if ($ids === []) { + return []; + } + + $response = $this->client->search( + $this->resolvePhysicalIndexName(), + [ + 'size' => 0, + 'query' => ['bool' => ['filter' => [['terms' => [$field => $ids]]]]], + 'aggs' => [ + 'by_field' => [ + 'terms' => ['field' => $field, 'size' => count($ids)], + ], + ], + ], + ); + + $counts = []; + foreach ($response['aggregations']['by_field']['buckets'] ?? [] as $bucket) { + $counts[(int) $bucket['key']] = (int) $bucket['doc_count']; + } + + return $counts; + } + + private function resolvePhysicalIndexName(): string + { + return $this->indexPrefix . UserMessageBounce::SEARCH_INDEX_NAME; + } +} diff --git a/src/Domain/Messaging/Repository/UserMessageBounceElasticsearchReader.php b/src/Domain/Messaging/Repository/UserMessageBounceElasticsearchReader.php index e2de944e..4507ed6c 100644 --- a/src/Domain/Messaging/Repository/UserMessageBounceElasticsearchReader.php +++ b/src/Domain/Messaging/Repository/UserMessageBounceElasticsearchReader.php @@ -20,6 +20,8 @@ * ES-backed counterpart of UserMessageBounceRepository. Read-only - dual-write is handled entirely * by SearchIndexDoctrineListener, not by this class. Any Elasticsearch failure surfaces as * SearchBackendUnavailableException (via ElasticsearchClientInterface) with no fallback to the database. + * + * Queries that also need to join against MySQL-only data live in UserMessageBounceElasticsearchHybridReader */ class UserMessageBounceElasticsearchReader implements UserMessageBounceReaderInterface { diff --git a/tests/Integration/Domain/Messaging/Repository/UserMessageBounceElasticsearchHybridReaderTest.php b/tests/Integration/Domain/Messaging/Repository/UserMessageBounceElasticsearchHybridReaderTest.php new file mode 100644 index 00000000..4459a4da --- /dev/null +++ b/tests/Integration/Domain/Messaging/Repository/UserMessageBounceElasticsearchHybridReaderTest.php @@ -0,0 +1,318 @@ +loadSchema(); + + $this->client = $this->createMock(ElasticsearchClientInterface::class); + $this->reader = new UserMessageBounceElasticsearchHybridReader($this->client, 'phplist_', $this->entityManager); + } + + protected function tearDown(): void + { + $schemaTool = new SchemaTool($this->entityManager); + $schemaTool->dropDatabase(); + parent::tearDown(); + } + + public function testGetListBounceTotalsMergesElasticsearchCountsWithSubscriberData(): void + { + $admin = (new Administrator()) + ->setLoginName('admin') + ->setEmail('admin@example.com'); + $this->entityManager->persist($admin); + + $targetList = (new SubscriberList())->setName('Target list')->setOwner($admin); + $otherList = (new SubscriberList())->setName('Other list')->setOwner($admin); + $this->entityManager->persist($targetList); + $this->entityManager->persist($otherList); + + $subscriber1 = (new Subscriber('one@example.com'))->setConfirmed(true)->setBlacklisted(false); + $subscriber2 = (new Subscriber('two@example.com'))->setConfirmed(false)->setBlacklisted(true); + $subscriber3 = (new Subscriber('three@example.com'))->setConfirmed(true)->setBlacklisted(false); + $this->entityManager->persist($subscriber1); + $this->entityManager->persist($subscriber2); + $this->entityManager->persist($subscriber3); + $this->entityManager->flush(); + + $subscription1 = (new Subscription())->setSubscriber($subscriber1)->setSubscriberList($targetList); + $subscription2 = (new Subscription())->setSubscriber($subscriber2)->setSubscriberList($targetList); + $subscription3 = (new Subscription())->setSubscriber($subscriber3)->setSubscriberList($otherList); + $this->entityManager->persist($subscription1); + $this->entityManager->persist($subscription2); + $this->entityManager->persist($subscription3); + $this->entityManager->flush(); + + $this->client + ->expects($this->once()) + ->method('search') + ->with( + 'phplist_user_message_bounce', + $this->callback(function (array $query) use ($subscriber1, $subscriber2): bool { + return $query['query']['bool']['filter'][0]['terms']['userId'] === [ + $subscriber1->getId(), + $subscriber2->getId(), + ]; + }), + ) + ->willReturn([ + 'aggregations' => [ + 'by_field' => [ + 'buckets' => [ + ['key' => $subscriber1->getId(), 'doc_count' => 2], + ['key' => $subscriber2->getId(), 'doc_count' => 1], + ], + ], + ], + ]); + + $rows = $this->reader->getListBounceTotals($targetList->getId()); + + self::assertSame( + [ + [ + 'subscriber_id' => $subscriber1->getId(), + 'email' => 'one@example.com', + 'confirmed' => true, + 'blacklisted' => false, + 'total_bounces' => 2, + ], + [ + 'subscriber_id' => $subscriber2->getId(), + 'email' => 'two@example.com', + 'confirmed' => false, + 'blacklisted' => true, + 'total_bounces' => 1, + ], + ], + $rows + ); + } + + public function testGetCampaignBounceTotalsMergesElasticsearchCountsWithMessageData(): void + { + $admin = (new Administrator()) + ->setLoginName('admin') + ->setEmail('admin@example.com'); + $this->entityManager->persist($admin); + $this->entityManager->flush(); + + $message1 = $this->createMessage('Campaign one', $admin); + $message2 = $this->createMessage('Campaign two', $admin); + $this->entityManager->persist($message1); + $this->entityManager->persist($message2); + $this->entityManager->flush(); + + $this->client + ->expects($this->once()) + ->method('search') + ->with( + 'phplist_user_message_bounce', + $this->callback(function (array $query) use ($message1, $message2): bool { + return $query['query']['bool']['filter'][0]['terms']['messageId'] === [ + $message1->getId(), + $message2->getId(), + ]; + }), + ) + ->willReturn([ + 'aggregations' => [ + 'by_field' => [ + 'buckets' => [ + ['key' => $message1->getId(), 'doc_count' => 4], + ], + ], + ], + ]); + + $rows = $this->reader->getCampaignBounceTotals(); + + self::assertSame( + [ + [ + 'message_id' => $message1->getId(), + 'subject' => 'Campaign one', + 'total_bounces' => 4, + ], + ], + $rows + ); + } + + public function testGetPaginatedWithJoinNoRelationHydratesMatchingBounceEntitiesAndSkipsMissingOnes(): void + { + $bounce1 = new Bounce(status: 'new'); + $bounce2 = new Bounce(status: 'processed'); + $this->entityManager->persist($bounce1); + $this->entityManager->persist($bounce2); + $this->entityManager->flush(); + + $this->client + ->expects($this->once()) + ->method('search') + ->with( + 'phplist_user_message_bounce', + $this->callback(function (array $query): bool { + return $query['query'] === ['range' => ['idSort' => ['gt' => 0]]] + && $query['size'] === 10; + }), + ) + ->willReturn([ + 'hits' => [ + 'hits' => [ + ['_source' => [ + 'id' => 1, + 'idSort' => 1, + 'userId' => 5, + 'messageId' => 10, + 'bounceId' => $bounce1->getId(), + 'time' => '2026-01-01T00:00:00+00:00', + ]], + ['_source' => [ + 'id' => 2, + 'idSort' => 2, + 'userId' => 6, + 'messageId' => 11, + // No matching Bounce row - must be skipped, mirroring the SQL inner join. + 'bounceId' => 999999, + 'time' => '2026-01-02T00:00:00+00:00', + ]], + ['_source' => [ + 'id' => 3, + 'idSort' => 3, + 'userId' => 7, + 'messageId' => 12, + 'bounceId' => $bounce2->getId(), + 'time' => '2026-01-03T00:00:00+00:00', + ]], + ], + ], + ]); + + $rows = $this->reader->getPaginatedWithJoinNoRelation(0, 10); + + self::assertCount(2, $rows); + self::assertSame(1, $rows[0]['umb']->getId()); + self::assertSame($bounce1, $rows[0]['bounce']); + self::assertSame(3, $rows[1]['umb']->getId()); + self::assertSame($bounce2, $rows[1]['bounce']); + } + + public function testGetUserMessageHistoryWithBouncesMergesSentMessagesWithBounceDocs(): void + { + $admin = (new Administrator()) + ->setLoginName('admin') + ->setEmail('admin@example.com'); + $this->entityManager->persist($admin); + + $subscriber = new Subscriber('history@example.com'); + $this->entityManager->persist($subscriber); + $this->entityManager->flush(); + + $message1 = $this->createMessage('First', $admin); + $message2 = $this->createMessage('Second', $admin); + $this->entityManager->persist($message1); + $this->entityManager->persist($message2); + $this->entityManager->flush(); + + $bounce = new Bounce(status: 'new'); + $this->entityManager->persist($bounce); + $this->entityManager->flush(); + + $userMessage1 = new UserMessage($subscriber, $message1); + $userMessage1->setStatus(UserMessageStatus::Sent); + $userMessage2 = new UserMessage($subscriber, $message2); + $userMessage2->setStatus(UserMessageStatus::Sent); + $this->entityManager->persist($userMessage1); + $this->entityManager->persist($userMessage2); + $this->entityManager->flush(); + + $this->client + ->expects($this->once()) + ->method('search') + ->with( + 'phplist_user_message_bounce', + $this->callback(function (array $query) use ($subscriber): bool { + return $query['query'] === ['term' => ['userId' => $subscriber->getId()]]; + }), + ) + ->willReturn([ + 'hits' => [ + 'hits' => [ + ['_source' => [ + 'id' => 1, + 'idSort' => 1, + 'userId' => $subscriber->getId(), + 'messageId' => $message1->getId(), + 'bounceId' => $bounce->getId(), + 'time' => '2026-01-01T00:00:00+00:00', + ]], + ], + ], + ]); + + $rows = $this->reader->getUserMessageHistoryWithBounces($subscriber); + + self::assertCount(2, $rows); + + $rowsByMessageId = []; + foreach ($rows as $row) { + $rowsByMessageId[$row['um']->getMessage()->getId()] = $row; + } + + self::assertSame($bounce, $rowsByMessageId[$message1->getId()]['b']); + self::assertSame($bounce->getId(), $rowsByMessageId[$message1->getId()]['umb']->getBounceId()); + self::assertNull($rowsByMessageId[$message2->getId()]['b']); + self::assertNull($rowsByMessageId[$message2->getId()]['umb']); + } + + private function createMessage(string $subject, Administrator $owner): Message + { + return new Message( + new MessageFormat(true, 'text'), + new MessageSchedule(null, null, null, null, null), + new MessageMetadata(), + new MessageContent($subject), + new MessageOptions(), + $owner + ); + } +} From 912e9af3d6ec873b2e03244a2d43639028336652 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Thu, 3 Sep 2026 13:44:46 +0400 Subject: [PATCH 18/24] Add UserMessageBounceReportReaderInterface and implement in UserMessageBounceElasticsearchHybridReader --- config/services/repositories.yml | 6 ++++ ...UserMessageBounceReportReaderInterface.php | 31 +++++++++++++++++++ ...MessageBounceElasticsearchHybridReader.php | 6 +++- .../UserMessageBounceRepository.php | 4 ++- 4 files changed, 45 insertions(+), 2 deletions(-) create mode 100644 src/Domain/Messaging/Repository/Interfaces/UserMessageBounceReportReaderInterface.php diff --git a/config/services/repositories.yml b/config/services/repositories.yml index 1c8bf837..49f5e7e8 100644 --- a/config/services/repositories.yml +++ b/config/services/repositories.yml @@ -127,6 +127,12 @@ services: # alias to UserMessageBounceRepository to read from the database instead. PhpList\Core\Domain\Messaging\Repository\Interfaces\UserMessageBounceReaderInterface: alias: PhpList\Core\Domain\Messaging\Repository\UserMessageBounceElasticsearchReader + + # Same swap point for the reporting queries (getListBounceTotals/getCampaignBounceTotals) that join + # bounce data against Subscriber/Message - swap this alias to UserMessageBounceRepository to read + # from the database instead. + PhpList\Core\Domain\Messaging\Repository\Interfaces\UserMessageBounceReportReaderInterface: + alias: PhpList\Core\Domain\Messaging\Repository\UserMessageBounceElasticsearchHybridReader PhpList\Core\Domain\Messaging\Repository\UserMessageForwardRepository: parent: PhpList\Core\Domain\Common\Repository\AbstractRepository arguments: diff --git a/src/Domain/Messaging/Repository/Interfaces/UserMessageBounceReportReaderInterface.php b/src/Domain/Messaging/Repository/Interfaces/UserMessageBounceReportReaderInterface.php new file mode 100644 index 00000000..4e574628 --- /dev/null +++ b/src/Domain/Messaging/Repository/Interfaces/UserMessageBounceReportReaderInterface.php @@ -0,0 +1,31 @@ + + */ + public function getListBounceTotals(int $listId): array; + + /** @return array */ + public function getCampaignBounceTotals(?int $ownerId = null): array; +} diff --git a/src/Domain/Messaging/Repository/UserMessageBounceElasticsearchHybridReader.php b/src/Domain/Messaging/Repository/UserMessageBounceElasticsearchHybridReader.php index 7e823c8a..9bc94e1f 100644 --- a/src/Domain/Messaging/Repository/UserMessageBounceElasticsearchHybridReader.php +++ b/src/Domain/Messaging/Repository/UserMessageBounceElasticsearchHybridReader.php @@ -12,6 +12,7 @@ use PhpList\Core\Domain\Messaging\Model\ReadModel\UserMessageBounceReadModel; use PhpList\Core\Domain\Messaging\Model\UserMessage; use PhpList\Core\Domain\Messaging\Model\UserMessageBounce; +use PhpList\Core\Domain\Messaging\Repository\Interfaces\UserMessageBounceReportReaderInterface; use PhpList\Core\Domain\Search\Client\ElasticsearchClientInterface; use PhpList\Core\Domain\Subscription\Model\Subscriber; use PhpList\Core\Domain\Subscription\Model\Subscription; @@ -26,8 +27,11 @@ * * Only makes sense when bounce reads are Elasticsearch-backed - UserMessageBounceRepository already * has the plain single-query SQL join versions of all four methods for when they aren't. + * getListBounceTotals/getCampaignBounceTotals are also declared on UserMessageBounceReportReaderInterface, + * which is what external consumers (e.g. phplist/rest-api) should depend on instead of this concrete + * class - see config/services/repositories.yml for the DI alias. */ -class UserMessageBounceElasticsearchHybridReader +class UserMessageBounceElasticsearchHybridReader implements UserMessageBounceReportReaderInterface { // todo: move db queries into repositories and inject them here, rather than using the entity manager directly public function __construct( diff --git a/src/Domain/Messaging/Repository/UserMessageBounceRepository.php b/src/Domain/Messaging/Repository/UserMessageBounceRepository.php index c625636d..8f5f40ea 100644 --- a/src/Domain/Messaging/Repository/UserMessageBounceRepository.php +++ b/src/Domain/Messaging/Repository/UserMessageBounceRepository.php @@ -17,12 +17,14 @@ use PhpList\Core\Domain\Messaging\Model\UserMessage; use PhpList\Core\Domain\Messaging\Model\UserMessageBounce; use PhpList\Core\Domain\Messaging\Repository\Interfaces\UserMessageBounceReaderInterface; +use PhpList\Core\Domain\Messaging\Repository\Interfaces\UserMessageBounceReportReaderInterface; use PhpList\Core\Domain\Subscription\Model\Subscriber; use PhpList\Core\Domain\Subscription\Model\Subscription; class UserMessageBounceRepository extends AbstractRepository implements PaginatableRepositoryInterface, - UserMessageBounceReaderInterface + UserMessageBounceReaderInterface, + UserMessageBounceReportReaderInterface { use CursorPaginationTrait; From b226753226577d95943c0bce8909f1ab26d85641 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Thu, 3 Sep 2026 14:26:36 +0400 Subject: [PATCH 19/24] Make Elasticsearch optional by introducing configurable readers for SubscriberHistory and UserMessageBounce --- .env.dist | 3 + config/parameters.yml | 1 + config/services.yml | 4 +- config/services/repositories.yml | 33 +++++--- docs/ElasticsearchSearch.md | 23 +++++- .../Doctrine/SearchIndexDoctrineListener.php | 27 ++++++- .../UserMessageBounceConfigurableReader.php | 59 ++++++++++++++ ...rMessageBounceReportConfigurableReader.php | 39 ++++++++++ .../SubscriberHistoryConfigurableReader.php | 44 +++++++++++ .../SearchIndexDoctrineListenerTest.php | 15 ++++ ...serMessageBounceConfigurableReaderTest.php | 76 +++++++++++++++++++ ...sageBounceReportConfigurableReaderTest.php | 60 +++++++++++++++ ...ubscriberHistoryConfigurableReaderTest.php | 64 ++++++++++++++++ 13 files changed, 433 insertions(+), 15 deletions(-) create mode 100644 src/Domain/Messaging/Repository/UserMessageBounceConfigurableReader.php create mode 100644 src/Domain/Messaging/Repository/UserMessageBounceReportConfigurableReader.php create mode 100644 src/Domain/Subscription/Repository/SubscriberHistoryConfigurableReader.php create mode 100644 tests/Unit/Domain/Messaging/Repository/UserMessageBounceConfigurableReaderTest.php create mode 100644 tests/Unit/Domain/Messaging/Repository/UserMessageBounceReportConfigurableReaderTest.php create mode 100644 tests/Unit/Domain/Subscription/Repository/SubscriberHistoryConfigurableReaderTest.php diff --git a/.env.dist b/.env.dist index 16388975..966e10cd 100644 --- a/.env.dist +++ b/.env.dist @@ -55,6 +55,9 @@ MESSENGER_TRANSPORT_DSN=doctrine://default?auto_setup=true SEARCH_TRANSPORT_DSN=doctrine://default?queue_name=search_index # Elasticsearch configuration +# Set to false to fall back to reading/writing these tables straight from MySQL - no Elasticsearch +# cluster required. See docs/ElasticsearchSearch.md. +ELASTICSEARCH_ENABLED=false ELASTICSEARCH_HOSTS=http://127.0.0.1:9200 ELASTICSEARCH_USERNAME= ELASTICSEARCH_PASSWORD= diff --git a/config/parameters.yml b/config/parameters.yml index e0f04067..ebb7c6c8 100644 --- a/config/parameters.yml +++ b/config/parameters.yml @@ -53,6 +53,7 @@ parameters: app.search_transport_dsn: '%env(SEARCH_TRANSPORT_DSN)%' # Elasticsearch configuration + elasticsearch.enabled: '%env(bool:ELASTICSEARCH_ENABLED)%' elasticsearch.hosts: '%env(csv:ELASTICSEARCH_HOSTS)%' elasticsearch.username: '%env(ELASTICSEARCH_USERNAME)%' elasticsearch.password: '%env(ELASTICSEARCH_PASSWORD)%' diff --git a/config/services.yml b/config/services.yml index 780df8ce..768a4ca4 100644 --- a/config/services.yml +++ b/config/services.yml @@ -55,7 +55,9 @@ services: arguments: $tablePrefix: '%database_prefix%' - PhpList\Core\Core\Doctrine\SearchIndexDoctrineListener: ~ + PhpList\Core\Core\Doctrine\SearchIndexDoctrineListener: + arguments: + $enabled: '%elasticsearch.enabled%' HTMLPurifier_Config: class: HTMLPurifier_Config diff --git a/config/services/repositories.yml b/config/services/repositories.yml index 49f5e7e8..5ee7eb40 100644 --- a/config/services/repositories.yml +++ b/config/services/repositories.yml @@ -84,10 +84,15 @@ services: arguments: - PhpList\Core\Domain\Subscription\Model\SubscriberHistory - # Reads for SubscriberHistoryManager/SubscriberManager come only from Elasticsearch - swap this - # alias to SubscriberHistoryRepository to read from the database instead. + # Reads for SubscriberHistoryManager/SubscriberManager go through this configurable reader, which + # picks Elasticsearch or the database based on elasticsearch.enabled (ELASTICSEARCH_ENABLED) - see + # SubscriberHistoryConfigurableReader and docs/ElasticsearchSearch.md. + PhpList\Core\Domain\Subscription\Repository\SubscriberHistoryConfigurableReader: + autowire: true + arguments: + $elasticsearchEnabled: '%elasticsearch.enabled%' PhpList\Core\Domain\Subscription\Repository\Interfaces\SubscriberHistoryReaderInterface: - alias: PhpList\Core\Domain\Subscription\Repository\SubscriberHistoryElasticsearchReader + alias: PhpList\Core\Domain\Subscription\Repository\SubscriberHistoryConfigurableReader PhpList\Core\Domain\Subscription\Repository\UserBlacklistRepository: parent: PhpList\Core\Domain\Common\Repository\AbstractRepository arguments: @@ -123,16 +128,24 @@ services: arguments: - PhpList\Core\Domain\Messaging\Model\UserMessageBounce - # Reads for UserMessageBounceReaderInterface consumers come only from Elasticsearch - swap this - # alias to UserMessageBounceRepository to read from the database instead. + # Reads for UserMessageBounceReaderInterface consumers go through this configurable reader, which + # picks Elasticsearch or the database based on elasticsearch.enabled (ELASTICSEARCH_ENABLED) - see + # UserMessageBounceConfigurableReader and docs/ElasticsearchSearch.md. + PhpList\Core\Domain\Messaging\Repository\UserMessageBounceConfigurableReader: + autowire: true + arguments: + $elasticsearchEnabled: '%elasticsearch.enabled%' PhpList\Core\Domain\Messaging\Repository\Interfaces\UserMessageBounceReaderInterface: - alias: PhpList\Core\Domain\Messaging\Repository\UserMessageBounceElasticsearchReader + alias: PhpList\Core\Domain\Messaging\Repository\UserMessageBounceConfigurableReader - # Same swap point for the reporting queries (getListBounceTotals/getCampaignBounceTotals) that join - # bounce data against Subscriber/Message - swap this alias to UserMessageBounceRepository to read - # from the database instead. + # Same configurable swap for the reporting queries (getListBounceTotals/getCampaignBounceTotals) + # that join bounce data against Subscriber/Message. + PhpList\Core\Domain\Messaging\Repository\UserMessageBounceReportConfigurableReader: + autowire: true + arguments: + $elasticsearchEnabled: '%elasticsearch.enabled%' PhpList\Core\Domain\Messaging\Repository\Interfaces\UserMessageBounceReportReaderInterface: - alias: PhpList\Core\Domain\Messaging\Repository\UserMessageBounceElasticsearchHybridReader + alias: PhpList\Core\Domain\Messaging\Repository\UserMessageBounceReportConfigurableReader PhpList\Core\Domain\Messaging\Repository\UserMessageForwardRepository: parent: PhpList\Core\Domain\Common\Repository\AbstractRepository arguments: diff --git a/docs/ElasticsearchSearch.md b/docs/ElasticsearchSearch.md index 95dd15d0..3eb47b89 100644 --- a/docs/ElasticsearchSearch.md +++ b/docs/ElasticsearchSearch.md @@ -63,6 +63,23 @@ applied to every logical index alias (e.g. alias `subscriber_history` becomes in `phplist_subscriber_history` with the default prefix), the same convention as `DATABASE_PREFIX` for MySQL tables. +### Making Elasticsearch optional + +Set `ELASTICSEARCH_ENABLED=false` to run without an Elasticsearch cluster at all - no code changes, +no cluster required, and it's safe even if `ELASTICSEARCH_HOSTS` is unreachable or unset: + +- **Writes**: `SearchIndexDoctrineListener` becomes a no-op - nothing is ever queued to the + `async_search` transport, so no `IndexDocumentMessage` accumulates unconsumed. +- **Reads**: every reader interface (`SubscriberHistoryReaderInterface`, + `UserMessageBounceReaderInterface`, `UserMessageBounceReportReaderInterface`) is aliased to a small + `*ConfigurableReader` that picks the Doctrine repository instead of the Elasticsearch reader - see + `SubscriberHistoryConfigurableReader`/`UserMessageBounceConfigurableReader`/ + `UserMessageBounceReportConfigurableReader`. + +The `elasticsearch/elasticsearch` PHP client package is still a hard Composer dependency of +`phplist/core` either way - disabling the feature at runtime doesn't remove the need to have that +package installed, only the need to have a reachable cluster. + ## Queueing Indexing/deletion messages are routed to a dedicated `async_search` Messenger transport @@ -108,8 +125,10 @@ again after adding a new searchable entity or changing a mapping. `phplist:search:reindex`. 4. If reads should also move to Elasticsearch, introduce a reader interface for that entity (mirroring `SubscriberHistoryReaderInterface`) and an Elasticsearch-backed implementation (mirroring - `SubscriberHistoryElasticsearchReader`), then alias the interface to it in DI instead of the - Doctrine repository. + `SubscriberHistoryElasticsearchReader`). To keep Elasticsearch optional for the new entity too, also + add a `*ConfigurableReader` (mirroring `SubscriberHistoryConfigurableReader`) that picks between the + Doctrine repository and the Elasticsearch reader based on `elasticsearch.enabled`, and alias the + interface to that instead of aliasing directly to either backend. ## Troubleshooting diff --git a/src/Core/Doctrine/SearchIndexDoctrineListener.php b/src/Core/Doctrine/SearchIndexDoctrineListener.php index 71eeae2b..8dc84829 100644 --- a/src/Core/Doctrine/SearchIndexDoctrineListener.php +++ b/src/Core/Doctrine/SearchIndexDoctrineListener.php @@ -30,6 +30,11 @@ * preRemove captures getSearchIndexName()/getSearchDocumentId() before the delete happens: for * entities with a Doctrine-generated id, UnitOfWork::executeDeletions() nulls the identifier before * postRemove fires, so reading it there would queue a delete with an empty document id. + * + * $enabled (elasticsearch.enabled, ELASTICSEARCH_ENABLED) is the write-side half of making + * Elasticsearch fully optional: when false, every event method below is a no-op, so nothing is ever + * queued to the async_search transport - see docs/ElasticsearchSearch.md. The read-side half is each + * entity's *ConfigurableReader falling back to its Doctrine repository. */ #[AsDoctrineListener(event: Events::postPersist)] #[AsDoctrineListener(event: Events::postUpdate)] @@ -44,12 +49,18 @@ class SearchIndexDoctrineListener /** @var array keyed by spl_object_id() */ private array $removalKeys = []; - public function __construct(private readonly MessageBusInterface $messageBus) - { + public function __construct( + private readonly MessageBusInterface $messageBus, + private readonly bool $enabled = true, + ) { } public function postPersist(PostPersistEventArgs $args): void { + if (!$this->enabled) { + return; + } + $entity = $args->getObject(); if (!$entity instanceof SearchIndexableInterface) { return; @@ -60,6 +71,10 @@ public function postPersist(PostPersistEventArgs $args): void public function postUpdate(PostUpdateEventArgs $args): void { + if (!$this->enabled) { + return; + } + $entity = $args->getObject(); if (!$entity instanceof SearchIndexableInterface) { return; @@ -70,6 +85,10 @@ public function postUpdate(PostUpdateEventArgs $args): void public function preRemove(PreRemoveEventArgs $args): void { + if (!$this->enabled) { + return; + } + $entity = $args->getObject(); if (!$entity instanceof SearchIndexableInterface) { return; @@ -80,6 +99,10 @@ public function preRemove(PreRemoveEventArgs $args): void public function postRemove(PostRemoveEventArgs $args): void { + if (!$this->enabled) { + return; + } + $entity = $args->getObject(); if (!$entity instanceof SearchIndexableInterface) { return; diff --git a/src/Domain/Messaging/Repository/UserMessageBounceConfigurableReader.php b/src/Domain/Messaging/Repository/UserMessageBounceConfigurableReader.php new file mode 100644 index 00000000..0f927c4c --- /dev/null +++ b/src/Domain/Messaging/Repository/UserMessageBounceConfigurableReader.php @@ -0,0 +1,59 @@ +activeReader()->getFilteredAfterId($filter); + } + + /** @return UserMessageBounceRecordInterface[] */ + public function getByUserId(int $userId): array + { + return $this->activeReader()->getByUserId($userId); + } + + public function getCountByMessageId(int $messageId): int + { + return $this->activeReader()->getCountByMessageId($messageId); + } + + public function countBetween(DateTimeInterface $start, DateTimeInterface $end): int + { + return $this->activeReader()->countBetween($start, $end); + } + + public function existsByMessageIdAndUserId(int $messageId, int $subscriberId): bool + { + return $this->activeReader()->existsByMessageIdAndUserId($messageId, $subscriberId); + } + + private function activeReader(): UserMessageBounceReaderInterface + { + return $this->elasticsearchEnabled ? $this->elasticsearchReader : $this->databaseReader; + } +} diff --git a/src/Domain/Messaging/Repository/UserMessageBounceReportConfigurableReader.php b/src/Domain/Messaging/Repository/UserMessageBounceReportConfigurableReader.php new file mode 100644 index 00000000..7f154402 --- /dev/null +++ b/src/Domain/Messaging/Repository/UserMessageBounceReportConfigurableReader.php @@ -0,0 +1,39 @@ +activeReader()->getListBounceTotals($listId); + } + + public function getCampaignBounceTotals(?int $ownerId = null): array + { + return $this->activeReader()->getCampaignBounceTotals($ownerId); + } + + private function activeReader(): UserMessageBounceReportReaderInterface + { + return $this->elasticsearchEnabled ? $this->elasticsearchReader : $this->databaseReader; + } +} diff --git a/src/Domain/Subscription/Repository/SubscriberHistoryConfigurableReader.php b/src/Domain/Subscription/Repository/SubscriberHistoryConfigurableReader.php new file mode 100644 index 00000000..c96af077 --- /dev/null +++ b/src/Domain/Subscription/Repository/SubscriberHistoryConfigurableReader.php @@ -0,0 +1,44 @@ +activeReader()->getFilteredAfterId($filter); + } + + /** @return SubscriberHistoryRecordInterface[] */ + public function getBySubscriber(Subscriber $subscriber): array + { + return $this->activeReader()->getBySubscriber($subscriber); + } + + private function activeReader(): SubscriberHistoryReaderInterface + { + return $this->elasticsearchEnabled ? $this->elasticsearchReader : $this->databaseReader; + } +} diff --git a/tests/Unit/Core/Doctrine/SearchIndexDoctrineListenerTest.php b/tests/Unit/Core/Doctrine/SearchIndexDoctrineListenerTest.php index afc3fd44..5be2f4d4 100644 --- a/tests/Unit/Core/Doctrine/SearchIndexDoctrineListenerTest.php +++ b/tests/Unit/Core/Doctrine/SearchIndexDoctrineListenerTest.php @@ -9,6 +9,7 @@ use Doctrine\ORM\Event\PostPersistEventArgs; use Doctrine\ORM\Event\PostRemoveEventArgs; use Doctrine\ORM\Event\PostUpdateEventArgs; +use Doctrine\ORM\Event\PreRemoveEventArgs; use PhpList\Core\Core\Doctrine\SearchIndexDoctrineListener; use PhpList\Core\Domain\Search\Message\IndexDocumentMessage; use PhpList\Core\Domain\Search\Model\Interfaces\SearchIndexableInterface; @@ -155,4 +156,18 @@ public function testPendingBufferIsClearedAfterDispatch(): void // A second postFlush with nothing new queued must not re-dispatch the same message. $this->listener->postFlush(new PostFlushEventArgs($this->objectManager)); } + + public function testDisabledListenerNeverQueuesOrDispatchesAnything(): void + { + $listener = new SearchIndexDoctrineListener($this->messageBus, enabled: false); + $entity = $this->createIndexable('subscriber_history', '1', ['id' => 1]); + + $this->messageBus->expects($this->never())->method('dispatch'); + + $listener->postPersist(new PostPersistEventArgs($entity, $this->objectManager)); + $listener->postUpdate(new PostUpdateEventArgs($entity, $this->objectManager)); + $listener->preRemove(new PreRemoveEventArgs($entity, $this->objectManager)); + $listener->postRemove(new PostRemoveEventArgs($entity, $this->objectManager)); + $listener->postFlush(new PostFlushEventArgs($this->objectManager)); + } } diff --git a/tests/Unit/Domain/Messaging/Repository/UserMessageBounceConfigurableReaderTest.php b/tests/Unit/Domain/Messaging/Repository/UserMessageBounceConfigurableReaderTest.php new file mode 100644 index 00000000..9a2298f0 --- /dev/null +++ b/tests/Unit/Domain/Messaging/Repository/UserMessageBounceConfigurableReaderTest.php @@ -0,0 +1,76 @@ +databaseReader = $this->createMock(UserMessageBounceRepository::class); + $this->elasticsearchReader = $this->createMock(UserMessageBounceElasticsearchReader::class); + } + + public function testDelegatesToElasticsearchWhenEnabled(): void + { + $reader = new UserMessageBounceConfigurableReader($this->databaseReader, $this->elasticsearchReader, true); + + $this->elasticsearchReader->expects($this->once()) + ->method('getCountByMessageId') + ->with(42) + ->willReturn(7); + $this->databaseReader->expects($this->never())->method('getCountByMessageId'); + + $this->assertSame(7, $reader->getCountByMessageId(42)); + } + + public function testDelegatesToDatabaseWhenDisabled(): void + { + $reader = new UserMessageBounceConfigurableReader($this->databaseReader, $this->elasticsearchReader, false); + $start = new DateTime('2026-01-01'); + $end = new DateTime('2026-01-31'); + + $this->databaseReader->expects($this->once()) + ->method('countBetween') + ->with($start, $end) + ->willReturn(3); + $this->elasticsearchReader->expects($this->never())->method('countBetween'); + + $this->assertSame(3, $reader->countBetween($start, $end)); + } + + public function testExistsByMessageIdAndUserIdDelegatesToActiveReader(): void + { + $reader = new UserMessageBounceConfigurableReader($this->databaseReader, $this->elasticsearchReader, true); + + $this->elasticsearchReader->expects($this->once()) + ->method('existsByMessageIdAndUserId') + ->with(5, 9) + ->willReturn(true); + + $this->assertTrue($reader->existsByMessageIdAndUserId(5, 9)); + } + + public function testGetByUserIdDelegatesToActiveReader(): void + { + $reader = new UserMessageBounceConfigurableReader($this->databaseReader, $this->elasticsearchReader, false); + + $this->databaseReader->expects($this->once()) + ->method('getByUserId') + ->with(3) + ->willReturn([]); + + $this->assertSame([], $reader->getByUserId(3)); + } +} diff --git a/tests/Unit/Domain/Messaging/Repository/UserMessageBounceReportConfigurableReaderTest.php b/tests/Unit/Domain/Messaging/Repository/UserMessageBounceReportConfigurableReaderTest.php new file mode 100644 index 00000000..56a16a89 --- /dev/null +++ b/tests/Unit/Domain/Messaging/Repository/UserMessageBounceReportConfigurableReaderTest.php @@ -0,0 +1,60 @@ +databaseReader = $this->createMock(UserMessageBounceRepository::class); + $this->elasticsearchReader = $this->createMock(UserMessageBounceElasticsearchHybridReader::class); + } + + public function testGetListBounceTotalsDelegatesToElasticsearchWhenEnabled(): void + { + $reader = new UserMessageBounceReportConfigurableReader( + $this->databaseReader, + $this->elasticsearchReader, + true, + ); + $expected = [['subscriber_id' => 1, 'email' => 'a@example.com', 'confirmed' => true, + 'blacklisted' => false, 'total_bounces' => 2]]; + + $this->elasticsearchReader->expects($this->once()) + ->method('getListBounceTotals') + ->with(10) + ->willReturn($expected); + $this->databaseReader->expects($this->never())->method('getListBounceTotals'); + + $this->assertSame($expected, $reader->getListBounceTotals(10)); + } + + public function testGetCampaignBounceTotalsDelegatesToDatabaseWhenDisabled(): void + { + $reader = new UserMessageBounceReportConfigurableReader( + $this->databaseReader, + $this->elasticsearchReader, + false, + ); + $expected = [['message_id' => 1, 'subject' => 'Hello', 'total_bounces' => 4]]; + + $this->databaseReader->expects($this->once()) + ->method('getCampaignBounceTotals') + ->with(7) + ->willReturn($expected); + $this->elasticsearchReader->expects($this->never())->method('getCampaignBounceTotals'); + + $this->assertSame($expected, $reader->getCampaignBounceTotals(7)); + } +} diff --git a/tests/Unit/Domain/Subscription/Repository/SubscriberHistoryConfigurableReaderTest.php b/tests/Unit/Domain/Subscription/Repository/SubscriberHistoryConfigurableReaderTest.php new file mode 100644 index 00000000..4af5782c --- /dev/null +++ b/tests/Unit/Domain/Subscription/Repository/SubscriberHistoryConfigurableReaderTest.php @@ -0,0 +1,64 @@ +databaseReader = $this->createMock(SubscriberHistoryRepository::class); + $this->elasticsearchReader = $this->createMock(SubscriberHistoryElasticsearchReader::class); + } + + public function testDelegatesToElasticsearchWhenEnabled(): void + { + $reader = new SubscriberHistoryConfigurableReader( + $this->databaseReader, + $this->elasticsearchReader, + true, + ); + $filter = $this->createMock(FilterRequestInterface::class); + $expected = new PaginatedResult([], 0, 50, 0); + + $this->elasticsearchReader->expects($this->once()) + ->method('getFilteredAfterId') + ->with($filter) + ->willReturn($expected); + $this->databaseReader->expects($this->never())->method('getFilteredAfterId'); + + $this->assertSame($expected, $reader->getFilteredAfterId($filter)); + } + + public function testDelegatesToDatabaseWhenDisabled(): void + { + $reader = new SubscriberHistoryConfigurableReader( + $this->databaseReader, + $this->elasticsearchReader, + false, + ); + $subscriber = $this->createMock(Subscriber::class); + $expected = []; + + $this->databaseReader->expects($this->once()) + ->method('getBySubscriber') + ->with($subscriber) + ->willReturn($expected); + $this->elasticsearchReader->expects($this->never())->method('getBySubscriber'); + + $this->assertSame($expected, $reader->getBySubscriber($subscriber)); + } +} From 054bab1f00820ce3cf4f06f7ea361490d2fcfaf4 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Thu, 3 Sep 2026 16:49:14 +0400 Subject: [PATCH 20/24] add MAX_RESULTS_BY_USER --- src/Domain/Messaging/Model/UserMessageBounce.php | 2 ++ .../Repository/UserMessageBounceElasticsearchReader.php | 3 +-- .../Messaging/Repository/UserMessageBounceRepository.php | 1 + src/Domain/Subscription/Model/SubscriberHistory.php | 1 + .../Repository/SubscriberHistoryElasticsearchReader.php | 3 +-- .../Subscription/Repository/SubscriberHistoryRepository.php | 1 + 6 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/Domain/Messaging/Model/UserMessageBounce.php b/src/Domain/Messaging/Model/UserMessageBounce.php index ff6c6f05..9a350f88 100644 --- a/src/Domain/Messaging/Model/UserMessageBounce.php +++ b/src/Domain/Messaging/Model/UserMessageBounce.php @@ -27,6 +27,8 @@ class UserMessageBounce implements UserMessageBounceRecordInterface { public const SEARCH_INDEX_NAME = 'user_message_bounce'; + // 1000 is enough, I think, but if we ever need more, we can implement pagination. + public const MAX_RESULTS_BY_USER = 1000; #[ORM\Id] #[ORM\Column(type: 'integer')] diff --git a/src/Domain/Messaging/Repository/UserMessageBounceElasticsearchReader.php b/src/Domain/Messaging/Repository/UserMessageBounceElasticsearchReader.php index 4507ed6c..af8c7e1b 100644 --- a/src/Domain/Messaging/Repository/UserMessageBounceElasticsearchReader.php +++ b/src/Domain/Messaging/Repository/UserMessageBounceElasticsearchReader.php @@ -86,8 +86,7 @@ public function getByUserId(int $userId): array [ 'query' => ['term' => ['userId' => $userId]], 'sort' => [['idSort' => 'desc']], - // 10000 is enough, I think, but if we ever need more, we can implement pagination here too. - 'size' => 10000, + 'size' => UserMessageBounce::MAX_RESULTS_BY_USER, ], ); diff --git a/src/Domain/Messaging/Repository/UserMessageBounceRepository.php b/src/Domain/Messaging/Repository/UserMessageBounceRepository.php index 8f5f40ea..08edfb4c 100644 --- a/src/Domain/Messaging/Repository/UserMessageBounceRepository.php +++ b/src/Domain/Messaging/Repository/UserMessageBounceRepository.php @@ -92,6 +92,7 @@ public function getByUserId(int $userId): array ->andWhere('umb.userId = :userId') ->setParameter('userId', $userId) ->orderBy('umb.id', 'DESC') + ->setMaxResults(UserMessageBounce::MAX_RESULTS_BY_USER) ->getQuery() ->getResult(); } diff --git a/src/Domain/Subscription/Model/SubscriberHistory.php b/src/Domain/Subscription/Model/SubscriberHistory.php index 5526b52d..cf1b08e9 100644 --- a/src/Domain/Subscription/Model/SubscriberHistory.php +++ b/src/Domain/Subscription/Model/SubscriberHistory.php @@ -24,6 +24,7 @@ class SubscriberHistory implements SubscriberHistoryRecordInterface { public const SEARCH_INDEX_NAME = 'subscriber_history'; + public const MAX_RESULTS_BY_USER = 1000; #[ORM\Id] #[ORM\Column(type: 'integer')] diff --git a/src/Domain/Subscription/Repository/SubscriberHistoryElasticsearchReader.php b/src/Domain/Subscription/Repository/SubscriberHistoryElasticsearchReader.php index bca54aa5..deb109fb 100644 --- a/src/Domain/Subscription/Repository/SubscriberHistoryElasticsearchReader.php +++ b/src/Domain/Subscription/Repository/SubscriberHistoryElasticsearchReader.php @@ -84,8 +84,7 @@ public function getBySubscriber(Subscriber $subscriber): array [ 'query' => ['term' => ['subscriberId' => $subscriber->getId()]], 'sort' => [['idSort' => 'desc']], - // 10000 is enough, I think, but if we ever need more, we can implement pagination here too. - 'size' => 10000, + 'size' => SubscriberHistory::MAX_RESULTS_BY_USER, ], ); diff --git a/src/Domain/Subscription/Repository/SubscriberHistoryRepository.php b/src/Domain/Subscription/Repository/SubscriberHistoryRepository.php index 73c9bc0c..4cda1805 100644 --- a/src/Domain/Subscription/Repository/SubscriberHistoryRepository.php +++ b/src/Domain/Subscription/Repository/SubscriberHistoryRepository.php @@ -85,6 +85,7 @@ public function getBySubscriber(Subscriber $subscriber): array ->andWhere('sh.subscriber = :subscriberId') ->setParameter('subscriberId', $subscriber->getId()) ->orderBy('sh.id', 'DESC') + ->setMaxResults(SubscriberHistory::MAX_RESULTS_BY_USER) ->getQuery() ->getResult(); } From 2b4daa82db01fb5f557e3bcdaef0a47903ec5a68 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Thu, 3 Sep 2026 16:55:45 +0400 Subject: [PATCH 21/24] Update lastId calculation in UserMessageBounceRepository for improved accuracy --- .../UserMessageBounceRepository.php | 2 +- .../UserMessageBounceRepositoryTest.php | 39 +++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/src/Domain/Messaging/Repository/UserMessageBounceRepository.php b/src/Domain/Messaging/Repository/UserMessageBounceRepository.php index 08edfb4c..4d99217d 100644 --- a/src/Domain/Messaging/Repository/UserMessageBounceRepository.php +++ b/src/Domain/Messaging/Repository/UserMessageBounceRepository.php @@ -81,7 +81,7 @@ public function getFilteredAfterId(FilterRequestInterface $filter): PaginatedRes items: $items, total: $total, limit: $limit, - lastId: $lastId, + lastId: $items !== [] ? $items[array_key_last($items)]->getId() : $lastId, ); } diff --git a/tests/Integration/Domain/Messaging/Repository/UserMessageBounceRepositoryTest.php b/tests/Integration/Domain/Messaging/Repository/UserMessageBounceRepositoryTest.php index ce3a0e9f..d7c82326 100644 --- a/tests/Integration/Domain/Messaging/Repository/UserMessageBounceRepositoryTest.php +++ b/tests/Integration/Domain/Messaging/Repository/UserMessageBounceRepositoryTest.php @@ -8,6 +8,7 @@ use Doctrine\ORM\Tools\SchemaTool; use PhpList\Core\Domain\Identity\Model\Administrator; use PhpList\Core\Domain\Messaging\Model\Bounce; +use PhpList\Core\Domain\Messaging\Model\Filter\UserMessageBounceFilter; use PhpList\Core\Domain\Messaging\Model\UserMessageBounce; use PhpList\Core\Domain\Messaging\Repository\UserMessageBounceRepository; use PhpList\Core\Domain\Subscription\Model\Subscriber; @@ -136,4 +137,42 @@ public function testGetListBounceTotalsReturnsAggregatedBouncesPerSubscriberForL $rows ); } + + public function testGetFilteredAfterIdAdvancesCursorAcrossConsecutivePages(): void + { + $bounce = new Bounce(status: 'new'); + $this->entityManager->persist($bounce); + $this->entityManager->flush(); + + $umb1 = (new UserMessageBounce($bounce->getId(), new DateTime()))->setUserId(1)->setMessageId(10); + $umb2 = (new UserMessageBounce($bounce->getId(), new DateTime()))->setUserId(2)->setMessageId(11); + $this->entityManager->persist($umb1); + $this->entityManager->persist($umb2); + $this->entityManager->flush(); + + $firstPage = $this->repository->getFilteredAfterId(new UserMessageBounceFilter(lastId: 0, limit: 1)); + + self::assertCount(1, $firstPage->getItems()); + self::assertSame($umb1->getId(), $firstPage->getItems()[0]->getId()); + self::assertSame($umb1->getId(), $firstPage->getLastId()); + + $secondPage = $this->repository->getFilteredAfterId( + new UserMessageBounceFilter(lastId: $firstPage->getLastId(), limit: 1) + ); + + self::assertCount(1, $secondPage->getItems()); + self::assertSame($umb2->getId(), $secondPage->getItems()[0]->getId()); + self::assertNotSame( + $firstPage->getItems()[0]->getId(), + $secondPage->getItems()[0]->getId(), + ); + } + + public function testGetFilteredAfterIdKeepsInputLastIdWhenPageIsEmpty(): void + { + $result = $this->repository->getFilteredAfterId(new UserMessageBounceFilter(lastId: 999, limit: 10)); + + self::assertSame([], $result->getItems()); + self::assertSame(999, $result->getLastId()); + } } From 768c07b9fd50b643c1d3081b2c5a8cf4152ca056 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Mon, 7 Sep 2026 10:59:46 +0400 Subject: [PATCH 22/24] Implement purge functionality for SubscriberHistory with configurable retention period --- .env.dist | 3 + config/parameters.yml | 1 + config/services/elasticsearch.yml | 11 ++ docs/ElasticsearchSearch.md | 18 ++ .../Command/PurgeSearchIndexedRowsCommand.php | 183 ++++++++++++++++++ .../SearchPurgeProviderInterface.php | 32 +++ .../Registry/SearchPurgeProviderRegistry.php | 37 ++++ .../SubscriberHistoryRepository.php | 40 ++++ .../Search/SubscriberHistoryPurgeProvider.php | 51 +++++ .../SubscriberHistoryRepositoryTest.php | 111 +++++++++++ .../PurgeSearchIndexedRowsCommandTest.php | 123 ++++++++++++ .../SubscriberHistoryPurgeProviderTest.php | 57 ++++++ 12 files changed, 667 insertions(+) create mode 100644 src/Domain/Search/Command/PurgeSearchIndexedRowsCommand.php create mode 100644 src/Domain/Search/Model/Interfaces/SearchPurgeProviderInterface.php create mode 100644 src/Domain/Search/Registry/SearchPurgeProviderRegistry.php create mode 100644 src/Domain/Subscription/Service/Search/SubscriberHistoryPurgeProvider.php create mode 100644 tests/Integration/Domain/Subscription/Repository/SubscriberHistoryRepositoryTest.php create mode 100644 tests/Unit/Domain/Search/Command/PurgeSearchIndexedRowsCommandTest.php create mode 100644 tests/Unit/Domain/Subscription/Service/Search/SubscriberHistoryPurgeProviderTest.php diff --git a/.env.dist b/.env.dist index 966e10cd..e66a76e4 100644 --- a/.env.dist +++ b/.env.dist @@ -63,6 +63,9 @@ ELASTICSEARCH_USERNAME= ELASTICSEARCH_PASSWORD= ELASTICSEARCH_CONNECT_TIMEOUT=2 ELASTICSEARCH_REQUEST_TIMEOUT=5 +# ISO-8601 duration (e.g. P1M) for how long SubscriberHistory rows are kept in MySQL after being +# confirmed in Elasticsearch. Empty disables purging. See docs/ElasticsearchSearch.md. +ELASTICSEARCH_PURGE_SUBSCRIBER_HISTORY_RETENTION= # A secret key that's used to generate certain security-related tokens PHPLIST_SECRET=%s diff --git a/config/parameters.yml b/config/parameters.yml index ebb7c6c8..1edd106d 100644 --- a/config/parameters.yml +++ b/config/parameters.yml @@ -60,6 +60,7 @@ parameters: elasticsearch.index_prefix: '%env(DATABASE_PREFIX)%' elasticsearch.connect_timeout: '%env(int:ELASTICSEARCH_CONNECT_TIMEOUT)%' elasticsearch.request_timeout: '%env(int:ELASTICSEARCH_REQUEST_TIMEOUT)%' + elasticsearch.purge.subscriber_history_retention: '%env(ELASTICSEARCH_PURGE_SUBSCRIBER_HISTORY_RETENTION)%' # A secret key that's used to generate certain security-related tokens secret: '%env(PHPLIST_SECRET)%' diff --git a/config/services/elasticsearch.yml b/config/services/elasticsearch.yml index 5507c857..e6da2c57 100644 --- a/config/services/elasticsearch.yml +++ b/config/services/elasticsearch.yml @@ -9,6 +9,8 @@ services: tags: ['phplist.search_index_definition'] PhpList\Core\Domain\Search\Model\Interfaces\SearchReindexProviderInterface: tags: ['phplist.search_reindex_provider'] + PhpList\Core\Domain\Search\Model\Interfaces\SearchPurgeProviderInterface: + tags: ['phplist.search_purge_provider'] Elastic\Elasticsearch\Client: factory: ['PhpList\Core\Domain\Search\Client\ElasticsearchClientFactory', 'create'] @@ -39,6 +41,10 @@ services: arguments: $providers: !tagged_iterator 'phplist.search_reindex_provider' + PhpList\Core\Domain\Search\Registry\SearchPurgeProviderRegistry: + arguments: + $providers: !tagged_iterator 'phplist.search_purge_provider' + PhpList\Core\Domain\Subscription\Repository\SubscriberHistoryElasticsearchReader: arguments: $indexPrefix: '%elasticsearch.index_prefix%' @@ -46,6 +52,11 @@ services: PhpList\Core\Domain\Subscription\Service\Search\: resource: '../../src/Domain/Subscription/Service/Search' + PhpList\Core\Domain\Subscription\Service\Search\SubscriberHistoryPurgeProvider: + arguments: + $indexPrefix: '%elasticsearch.index_prefix%' + $retentionPeriod: '%elasticsearch.purge.subscriber_history_retention%' + PhpList\Core\Domain\Messaging\Repository\UserMessageBounceElasticsearchReader: arguments: $indexPrefix: '%elasticsearch.index_prefix%' diff --git a/docs/ElasticsearchSearch.md b/docs/ElasticsearchSearch.md index 3eb47b89..97f9af81 100644 --- a/docs/ElasticsearchSearch.md +++ b/docs/ElasticsearchSearch.md @@ -105,11 +105,29 @@ bin/console phplist:search:init-indices [--index=] # Backfill Elasticsearch from the database. Safe to re-run (indexing is an upsert by id). bin/console phplist:search:reindex [] [--batch-size=500] [--last-id=0] + +# Delete DB rows older than a configured retention period, once confirmed to exist in +# Elasticsearch. Skips (and warns about, without deleting) any row not yet found in ES. +bin/console phplist:search:purge [] [--batch-size=500] [--dry-run] ``` Run `phplist:search:init-indices` once per environment before the first `phplist:search:reindex`, and again after adding a new searchable entity or changing a mapping. +### Purging old rows (`phplist:search:purge`) + +Only entities with an opted-in purge provider and a non-empty retention period are eligible. Today +that's `SubscriberHistory`, controlled by `ELASTICSEARCH_PURGE_SUBSCRIBER_HISTORY_RETENTION` (an +ISO-8601 duration, e.g. `P1M`; empty/unset disables purging for it). Adding another entity means +implementing `SearchPurgeProviderInterface` (mirroring `SubscriberHistoryPurgeProvider`) and adding its +own retention parameter - it's auto-tagged and picked up the same way reindex providers are. + +**Before scheduling this command in production**, make sure Elasticsearch snapshots/backups are +configured. Once a row is purged from MySQL, `phplist:search:reindex` can no longer recover it if the +ES index is ever lost - the verify-before-delete step only protects against rows that *haven't* made it +into ES yet, not against losing the ES index itself afterwards. This command is not currently wired +into any cron/Supervisor schedule; that should happen only after the snapshot policy is in place. + ## Adding a new searchable entity 1. Implement `PhpList\Core\Domain\Search\Model\Interfaces\SearchIndexableInterface` on the entity diff --git a/src/Domain/Search/Command/PurgeSearchIndexedRowsCommand.php b/src/Domain/Search/Command/PurgeSearchIndexedRowsCommand.php new file mode 100644 index 00000000..621de3a0 --- /dev/null +++ b/src/Domain/Search/Command/PurgeSearchIndexedRowsCommand.php @@ -0,0 +1,183 @@ +addArgument('alias', InputArgument::OPTIONAL, 'Purge only this alias (default: all configured)') + ->addOption('batch-size', null, InputOption::VALUE_REQUIRED, 'Rows per batch', self::DEFAULT_BATCH_SIZE) + ->addOption('dry-run', null, InputOption::VALUE_NONE, 'Report counts without deleting anything'); + } + + protected function execute(InputInterface $input, OutputInterface $output): int + { + $io = new SymfonyStyle($input, $output); + $alias = $input->getArgument('alias'); + $batchSize = (int) $input->getOption('batch-size'); + $dryRun = (bool) $input->getOption('dry-run'); + + if ($batchSize < 1) { + $io->error('The --batch-size option must be greater than zero.'); + return Command::FAILURE; + } + + if ($alias !== null) { + $provider = $this->registry->find($alias); + + if ($provider === null) { + $io->error(sprintf('No purge provider registered for alias "%s".', $alias)); + return Command::FAILURE; + } + + if ($provider->getRetentionPeriod() === null) { + $io->error(sprintf('No retention period configured for alias "%s".', $alias)); + return Command::FAILURE; + } + + $providers = [$provider]; + } else { + $providers = array_filter( + $this->registry->getAll(), + static fn (SearchPurgeProviderInterface $provider): bool => $provider->getRetentionPeriod() !== null, + ); + } + + if ($providers === []) { + $io->warning('No purge providers with a configured retention period.'); + return Command::SUCCESS; + } + + foreach ($providers as $provider) { + $this->purgeProvider($provider, $batchSize, $dryRun, $io); + } + + return Command::SUCCESS; + } + + private function purgeProvider( + SearchPurgeProviderInterface $provider, + int $batchSize, + bool $dryRun, + SymfonyStyle $io, + ): void { + $cutoff = (new DateTimeImmutable())->sub($provider->getRetentionPeriod()); + $io->writeln(sprintf( + '%s: purging rows older than %s', + $provider->getAlias(), + $cutoff->format(DateTimeImmutable::ATOM), + )); + + $lastId = 0; + $scanned = 0; + $deleted = 0; + $skipped = []; + + do { + $batch = [...$provider->fetchBatchOlderThan($cutoff, $lastId, $batchSize)]; + $countInBatch = count($batch); + + if ($countInBatch === 0) { + break; + } + + $docIds = array_map( + static fn (SearchIndexableInterface $entity): string => $entity->getSearchDocumentId(), + $batch, + ); + $confirmedIds = $this->confirmedInElasticsearch($provider->getSearchIndexName(), $docIds); + + $confirmedEntityIds = []; + foreach ($batch as $entity) { + $docId = $entity->getSearchDocumentId(); + if (in_array($docId, $confirmedIds, true)) { + $confirmedEntityIds[] = (int) $docId; + } else { + $skipped[] = (int) $docId; + } + } + + if (!$dryRun && $confirmedEntityIds !== []) { + $deleted += $provider->deleteByIds($confirmedEntityIds); + } else { + $deleted += count($confirmedEntityIds); + } + + $scanned += $countInBatch; + $lastId = (int) $docIds[array_key_last($docIds)]; + } while ($countInBatch >= $batchSize); + + if ($skipped !== []) { + $io->warning(sprintf( + '%s: %d row(s) older than cutoff were not found in Elasticsearch and were left in place: %s', + $provider->getAlias(), + count($skipped), + implode(', ', array_slice($skipped, 0, 10)) . (count($skipped) > 10 ? ', ...' : ''), + )); + } + + $io->success(sprintf( + '%s: scanned %d row(s), %s %d row(s)%s.', + $provider->getAlias(), + $scanned, + $dryRun ? 'would delete' : 'deleted', + $deleted, + $skipped !== [] ? sprintf(', skipped %d unconfirmed', count($skipped)) : '', + )); + } + + /** @param string[] $docIds @return string[] */ + private function confirmedInElasticsearch(string $indexName, array $docIds): array + { + if ($docIds === []) { + return []; + } + + $response = $this->client->search($indexName, [ + 'size' => count($docIds), + '_source' => false, + 'query' => [ + 'bool' => [ + 'filter' => [ + ['terms' => ['id' => array_map('intval', $docIds)]], + ], + ], + ], + ]); + + return array_map( + static fn (array $hit): string => (string) $hit['_id'], + $response['hits']['hits'] ?? [], + ); + } +} diff --git a/src/Domain/Search/Model/Interfaces/SearchPurgeProviderInterface.php b/src/Domain/Search/Model/Interfaces/SearchPurgeProviderInterface.php new file mode 100644 index 00000000..69b51cd5 --- /dev/null +++ b/src/Domain/Search/Model/Interfaces/SearchPurgeProviderInterface.php @@ -0,0 +1,32 @@ + */ + public function fetchBatchOlderThan(DateTimeInterface $cutoff, int $lastId, int $batchSize): iterable; + + /** @param int[] $ids @return int number of rows deleted */ + public function deleteByIds(array $ids): int; +} \ No newline at end of file diff --git a/src/Domain/Search/Registry/SearchPurgeProviderRegistry.php b/src/Domain/Search/Registry/SearchPurgeProviderRegistry.php new file mode 100644 index 00000000..d6eef94b --- /dev/null +++ b/src/Domain/Search/Registry/SearchPurgeProviderRegistry.php @@ -0,0 +1,37 @@ + $providers */ + public function __construct(iterable $providers) + { + $this->providers = $providers instanceof Traversable ? iterator_to_array($providers) : $providers; + } + + /** @return SearchPurgeProviderInterface[] */ + public function getAll(): array + { + return $this->providers; + } + + public function find(string $alias): ?SearchPurgeProviderInterface + { + foreach ($this->providers as $provider) { + if ($provider->getAlias() === $alias) { + return $provider; + } + } + + return null; + } +} \ No newline at end of file diff --git a/src/Domain/Subscription/Repository/SubscriberHistoryRepository.php b/src/Domain/Subscription/Repository/SubscriberHistoryRepository.php index 4cda1805..107e5d84 100644 --- a/src/Domain/Subscription/Repository/SubscriberHistoryRepository.php +++ b/src/Domain/Subscription/Repository/SubscriberHistoryRepository.php @@ -4,6 +4,7 @@ namespace PhpList\Core\Domain\Subscription\Repository; +use DateTimeInterface; use InvalidArgumentException; use PhpList\Core\Domain\Common\Model\Filter\FilterRequestInterface; use PhpList\Core\Domain\Common\Model\PaginatedResult; @@ -89,4 +90,43 @@ public function getBySubscriber(Subscriber $subscriber): array ->getQuery() ->getResult(); } + + public function countOlderThan(DateTimeInterface $cutoff): int + { + return (int) $this->createQueryBuilder('sh') + ->select('COUNT(sh.id)') + ->andWhere('sh.createdAt < :cutoff') + ->setParameter('cutoff', $cutoff) + ->getQuery() + ->getSingleScalarResult(); + } + + /** @return iterable */ + public function fetchBatchOlderThan(DateTimeInterface $cutoff, int $lastId, int $batchSize): iterable + { + return $this->createQueryBuilder('sh') + ->andWhere('sh.id > :lastId') + ->andWhere('sh.createdAt < :cutoff') + ->setParameter('lastId', $lastId) + ->setParameter('cutoff', $cutoff) + ->orderBy('sh.id', 'ASC') + ->setMaxResults($batchSize) + ->getQuery() + ->toIterable(); + } + + /** @param int[] $ids */ + public function deleteByIds(array $ids): int + { + if ($ids === []) { + return 0; + } + + return $this->createQueryBuilder('sh') + ->delete() + ->andWhere('sh.id IN (:ids)') + ->setParameter('ids', $ids) + ->getQuery() + ->execute(); + } } diff --git a/src/Domain/Subscription/Service/Search/SubscriberHistoryPurgeProvider.php b/src/Domain/Subscription/Service/Search/SubscriberHistoryPurgeProvider.php new file mode 100644 index 00000000..1a71ed1d --- /dev/null +++ b/src/Domain/Subscription/Service/Search/SubscriberHistoryPurgeProvider.php @@ -0,0 +1,51 @@ +retentionPeriod !== '' ? new DateInterval($this->retentionPeriod) : null; + } + + public function getSearchIndexName(): string + { + return $this->indexPrefix . SubscriberHistory::SEARCH_INDEX_NAME; + } + + public function countOlderThan(DateTimeInterface $cutoff): int + { + return $this->repository->countOlderThan($cutoff); + } + + public function fetchBatchOlderThan(DateTimeInterface $cutoff, int $lastId, int $batchSize): iterable + { + return $this->repository->fetchBatchOlderThan($cutoff, $lastId, $batchSize); + } + + public function deleteByIds(array $ids): int + { + return $this->repository->deleteByIds($ids); + } +} \ No newline at end of file diff --git a/tests/Integration/Domain/Subscription/Repository/SubscriberHistoryRepositoryTest.php b/tests/Integration/Domain/Subscription/Repository/SubscriberHistoryRepositoryTest.php new file mode 100644 index 00000000..840530ec --- /dev/null +++ b/tests/Integration/Domain/Subscription/Repository/SubscriberHistoryRepositoryTest.php @@ -0,0 +1,111 @@ +loadSchema(); + + $this->repository = self::getContainer()->get(SubscriberHistoryRepository::class); + } + + protected function tearDown(): void + { + $schemaTool = new SchemaTool($this->entityManager); + $schemaTool->dropDatabase(); + parent::tearDown(); + } + + private function persistHistoryRow(DateTime $date): SubscriberHistory + { + $subscriber = new Subscriber('subscriber-' . uniqid('', true) . '@example.com'); + $this->entityManager->persist($subscriber); + $this->entityManager->flush(); + + $history = new SubscriberHistory($subscriber); + $reflection = new ReflectionProperty(SubscriberHistory::class, 'createdAt'); + $reflection->setAccessible(true); + $reflection->setValue($history, $date); + + $this->entityManager->persist($history); + $this->entityManager->flush(); + + return $history; + } + + public function testCountOlderThanOnlyCountsRowsBeforeCutoff(): void + { + $old = $this->persistHistoryRow(new DateTime('2020-01-01')); + $this->persistHistoryRow(new DateTime('2030-01-01')); + + $count = $this->repository->countOlderThan(new DateTime('2025-01-01')); + + self::assertSame(1, $count); + self::assertNotNull($old->getId()); + } + + public function testFetchBatchOlderThanReturnsOnlyMatchingRowsInIdOrder(): void + { + $old1 = $this->persistHistoryRow(new DateTime('2020-01-01')); + $old2 = $this->persistHistoryRow(new DateTime('2020-06-01')); + $this->persistHistoryRow(new DateTime('2030-01-01')); + + $batch = [...$this->repository->fetchBatchOlderThan(new DateTime('2025-01-01'), 0, 10)]; + + self::assertCount(2, $batch); + self::assertSame($old1->getId(), $batch[0]->getId()); + self::assertSame($old2->getId(), $batch[1]->getId()); + } + + public function testFetchBatchOlderThanRespectsLastIdCursor(): void + { + $old1 = $this->persistHistoryRow(new DateTime('2020-01-01')); + $old2 = $this->persistHistoryRow(new DateTime('2020-06-01')); + + $batch = [...$this->repository->fetchBatchOlderThan(new DateTime('2025-01-01'), $old1->getId(), 10)]; + + self::assertCount(1, $batch); + self::assertSame($old2->getId(), $batch[0]->getId()); + } + + public function testDeleteByIdsRemovesOnlyGivenRows(): void + { + $toDelete = $this->persistHistoryRow(new DateTime('2020-01-01')); + $toKeep = $this->persistHistoryRow(new DateTime('2020-06-01')); + + $deleted = $this->repository->deleteByIds([$toDelete->getId()]); + $this->entityManager->clear(); // bulk DQL delete bypasses the identity map + + self::assertSame(1, $deleted); + self::assertSame(1, $this->repository->countOlderThan(new DateTime('2025-01-01'))); + self::assertNotNull($this->repository->find($toKeep->getId())); + self::assertNull($this->repository->find($toDelete->getId())); + } + + public function testDeleteByIdsWithEmptyArrayDeletesNothing(): void + { + $this->persistHistoryRow(new DateTime('2020-01-01')); + + $deleted = $this->repository->deleteByIds([]); + + self::assertSame(0, $deleted); + } +} \ No newline at end of file diff --git a/tests/Unit/Domain/Search/Command/PurgeSearchIndexedRowsCommandTest.php b/tests/Unit/Domain/Search/Command/PurgeSearchIndexedRowsCommandTest.php new file mode 100644 index 00000000..a53c8b69 --- /dev/null +++ b/tests/Unit/Domain/Search/Command/PurgeSearchIndexedRowsCommandTest.php @@ -0,0 +1,123 @@ +createMock(SearchIndexableInterface::class); + $row->method('getSearchDocumentId')->willReturn((string) $id); + + return $row; + } + + private function commandTesterWithProviders(SearchPurgeProviderInterface ...$providers): CommandTester + { + $this->client = $this->createMock(ElasticsearchClientInterface::class); + $registry = new SearchPurgeProviderRegistry($providers); + $command = new PurgeSearchIndexedRowsCommand($registry, $this->client); + + $application = new Application(); + $application->add($command); + + return new CommandTester($command); + } + + public function testDeletesOnlyRowsConfirmedInElasticsearch(): void + { + $confirmedRow = $this->makeFakeRow(1); + $unconfirmedRow = $this->makeFakeRow(2); + + $provider = $this->createMock(SearchPurgeProviderInterface::class); + $provider->method('getAlias')->willReturn('some_alias'); + $provider->method('getRetentionPeriod')->willReturn(new DateInterval('P1M')); + $provider->method('getSearchIndexName')->willReturn('phplist_some_alias'); + $provider->method('fetchBatchOlderThan')->willReturnOnConsecutiveCalls( + [$confirmedRow, $unconfirmedRow], + [], + ); + + $tester = $this->commandTesterWithProviders($provider); + $this->client->method('search')->willReturn([ + 'hits' => ['hits' => [['_id' => '1']]], + ]); + + $provider->expects($this->once())->method('deleteByIds')->with([1])->willReturn(1); + + $tester->execute([]); + + $output = $tester->getDisplay(); + $this->assertStringContainsString('not found in', $output); + $this->assertStringContainsString('deleted 1 row(s), skipped 1 unconfirmed', $output); + $this->assertSame(0, $tester->getStatusCode()); + } + + public function testDryRunDoesNotDeleteAnything(): void + { + $row = $this->makeFakeRow(1); + + $provider = $this->createMock(SearchPurgeProviderInterface::class); + $provider->method('getAlias')->willReturn('some_alias'); + $provider->method('getRetentionPeriod')->willReturn(new DateInterval('P1M')); + $provider->method('getSearchIndexName')->willReturn('phplist_some_alias'); + $provider->method('fetchBatchOlderThan')->willReturnOnConsecutiveCalls([$row], []); + + $tester = $this->commandTesterWithProviders($provider); + $this->client->method('search')->willReturn(['hits' => ['hits' => [['_id' => '1']]]]); + + $provider->expects($this->never())->method('deleteByIds'); + + $tester->execute(['--dry-run' => true]); + + $this->assertStringContainsString('would delete', $tester->getDisplay()); + $this->assertSame(0, $tester->getStatusCode()); + } + + public function testSkipsProvidersWithoutARetentionPeriod(): void + { + $provider = $this->createMock(SearchPurgeProviderInterface::class); + $provider->method('getAlias')->willReturn('some_alias'); + $provider->method('getRetentionPeriod')->willReturn(null); + + $tester = $this->commandTesterWithProviders($provider); + $provider->expects($this->never())->method('fetchBatchOlderThan'); + + $tester->execute([]); + + $this->assertStringContainsString( + 'No purge providers with a configured retention period', + $tester->getDisplay(), + ); + $this->assertSame(0, $tester->getStatusCode()); + } + + public function testFailsWhenAliasHasNoRetentionPeriodConfigured(): void + { + $provider = $this->createMock(SearchPurgeProviderInterface::class); + $provider->method('getAlias')->willReturn('some_alias'); + $provider->method('getRetentionPeriod')->willReturn(null); + + $tester = $this->commandTesterWithProviders($provider); + + $tester->execute(['alias' => 'some_alias']); + + $this->assertStringContainsString('No retention period configured', $tester->getDisplay()); + $this->assertSame(1, $tester->getStatusCode()); + } +} \ No newline at end of file diff --git a/tests/Unit/Domain/Subscription/Service/Search/SubscriberHistoryPurgeProviderTest.php b/tests/Unit/Domain/Subscription/Service/Search/SubscriberHistoryPurgeProviderTest.php new file mode 100644 index 00000000..a734d72f --- /dev/null +++ b/tests/Unit/Domain/Subscription/Service/Search/SubscriberHistoryPurgeProviderTest.php @@ -0,0 +1,57 @@ +createMock(SubscriberHistoryRepository::class), + 'phplist_', + 'P1M', + ); + + $this->assertSame('subscriber_history', $provider->getAlias()); + } + + public function testGetRetentionPeriodReturnsNullWhenNotConfigured(): void + { + $provider = new SubscriberHistoryPurgeProvider( + $this->createMock(SubscriberHistoryRepository::class), + 'phplist_', + '', + ); + + $this->assertNull($provider->getRetentionPeriod()); + } + + public function testGetRetentionPeriodParsesConfiguredIsoDuration(): void + { + $provider = new SubscriberHistoryPurgeProvider( + $this->createMock(SubscriberHistoryRepository::class), + 'phplist_', + 'P1M', + ); + + $this->assertEquals(new DateInterval('P1M'), $provider->getRetentionPeriod()); + } + + public function testGetSearchIndexNameIncludesPrefix(): void + { + $provider = new SubscriberHistoryPurgeProvider( + $this->createMock(SubscriberHistoryRepository::class), + 'phplist_', + 'P1M', + ); + + $this->assertSame('phplist_subscriber_history', $provider->getSearchIndexName()); + } +} \ No newline at end of file From c421dfd044c29710c8ff1d1589e5a92c3af81c1c Mon Sep 17 00:00:00 2001 From: Tatevik Date: Mon, 7 Sep 2026 11:31:38 +0400 Subject: [PATCH 23/24] Ensure SubscriberHistory rows are cascaded on Subscriber removal to prevent orphaned Elasticsearch documents --- .coderabbit.yaml | 48 +++++++++++++ .../Command/PurgeSearchIndexedRowsCommand.php | 69 +++++++++++++------ .../SearchPurgeProviderInterface.php | 2 +- .../Registry/SearchPurgeProviderRegistry.php | 2 +- src/Domain/Subscription/Model/Subscriber.php | 13 ++++ .../Subscription/Model/SubscriberHistory.php | 2 +- .../Search/SubscriberHistoryPurgeProvider.php | 2 +- .../SubscriberHistoryRepositoryTest.php | 31 ++++++++- .../PurgeSearchIndexedRowsCommandTest.php | 2 +- .../SubscriberHistoryPurgeProviderTest.php | 2 +- 10 files changed, 144 insertions(+), 29 deletions(-) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 6305f1b4..c343b8fc 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -64,6 +64,54 @@ reviews: - Ensure domain-layer code invoked by the job (outside the DynamicListAttr exception) remains free of persistence calls. - Batch flush operations where practical. + - path: "src/Domain/Search/**" + instructions: &elasticsearch_consistency | + You are reviewing code for the MySQL/Elasticsearch dual-write system (write-through DB + + Elasticsearch via `SearchIndexDoctrineListener`, with `*ConfigurableReader`/`*HybridReader` + classes able to read from either backend, and a retention/purge command that deletes DB rows + once confirmed in Elasticsearch). Flag anything that can let the two stores drift apart: + + - ❌ Bulk/raw deletes or updates on a `SearchIndexableInterface` entity's table that bypass + Doctrine's entity lifecycle - DQL `->delete()`/`->update()` queries, or DBAL + `executeStatement()`/raw SQL - never trigger `SearchIndexDoctrineListener::preRemove/postRemove` + (those only fire for `EntityManager::remove()`/`persist()` + `flush()`). Any such bulk + operation silently skips the Elasticsearch side. This is legitimate ONLY when it is an + explicit, reviewed "delete from MySQL but deliberately keep the Elasticsearch document" + retention/purge path (and even then, it must verify the row exists in Elasticsearch via a + real ES query *before* deleting - never delete on the assumption that dual-write "should + have" succeeded). + - ❌ A `SearchIndexableInterface` entity with a foreign key that has a DB-level + `onDelete: 'CASCADE'` (`#[ORM\JoinColumn(..., onDelete: 'CASCADE')]`) but no matching + Doctrine-level `cascade: ['remove']` on the owning/parent side's association. Without the + Doctrine-level cascade, deleting the parent lets the database silently drop child rows that + Doctrine never loads/removes individually, so the listener never fires for them and their + Elasticsearch documents are orphaned. + - ❌ Calls to `ElasticsearchClientInterface::index()`/`delete()`, or construction of + `IndexDocumentMessage`, using a hardcoded, reused, or non-monotonic revision instead of the + established wall-clock-microseconds pattern (see `SearchIndexDoctrineListener::nextRevision()`). + External versioning is what stops a delayed/retried write or delete from clobbering newer + state - a wrong revision can let a stale message win. + - ❌ New/changed `*ConfigurableReader` implementations that don't correctly gate on + `elasticsearch.enabled` and fall back to the Doctrine repository, or that silently mix data + from both backends for the same logical query/response without that being the explicit intent. + - ⚠️ Any consumer that needs a *complete*/unbounded history or aggregate (not a bounded recent + window) reading directly from the Doctrine repository for an entity that has a retention/purge + policy configured elsewhere - it will silently see only the retained window once older rows + are purged. Point out that it should read through the ES-backed reader/interface instead. + - ⚠️ Missing or incomplete tests around the above: a repository method that deletes/updates rows + for a `SearchIndexableInterface` entity should have a test proving it does (or intentionally + does not) go through the dual-write path, and any new cascade relationship should have a test + that actually removes the parent and asserts the child is gone too. + + - path: "src/**/*Elasticsearch*.php" + instructions: *elasticsearch_consistency + + - path: "src/**/Service/Search/**" + instructions: *elasticsearch_consistency + + - path: "src/Core/Doctrine/SearchIndexDoctrineListener.php" + instructions: *elasticsearch_consistency + auto_review: enabled: true base_branches: diff --git a/src/Domain/Search/Command/PurgeSearchIndexedRowsCommand.php b/src/Domain/Search/Command/PurgeSearchIndexedRowsCommand.php index 621de3a0..02baf293 100644 --- a/src/Domain/Search/Command/PurgeSearchIndexedRowsCommand.php +++ b/src/Domain/Search/Command/PurgeSearchIndexedRowsCommand.php @@ -111,32 +111,59 @@ private function purgeProvider( break; } - $docIds = array_map( - static fn (SearchIndexableInterface $entity): string => $entity->getSearchDocumentId(), - $batch, - ); - $confirmedIds = $this->confirmedInElasticsearch($provider->getSearchIndexName(), $docIds); - - $confirmedEntityIds = []; - foreach ($batch as $entity) { - $docId = $entity->getSearchDocumentId(); - if (in_array($docId, $confirmedIds, true)) { - $confirmedEntityIds[] = (int) $docId; - } else { - $skipped[] = (int) $docId; - } - } + $result = $this->purgeBatch($provider, $batch, $dryRun); + $deleted += $result['deleted']; + array_push($skipped, ...$result['skipped']); + $scanned += $countInBatch; + $lastId = $result['lastId']; + } while ($countInBatch >= $batchSize); + + $this->reportResults($provider, $scanned, $deleted, $skipped, $dryRun, $io); + } - if (!$dryRun && $confirmedEntityIds !== []) { - $deleted += $provider->deleteByIds($confirmedEntityIds); + /** + * @param SearchIndexableInterface[] $batch + * @return array{deleted: int, skipped: int[], lastId: int} + */ + private function purgeBatch(SearchPurgeProviderInterface $provider, array $batch, bool $dryRun): array + { + $docIds = array_map( + static fn (SearchIndexableInterface $entity): string => $entity->getSearchDocumentId(), + $batch, + ); + $confirmedIds = $this->confirmedInElasticsearch($provider->getSearchIndexName(), $docIds); + + $confirmedEntityIds = []; + $skipped = []; + foreach ($docIds as $docId) { + if (in_array($docId, $confirmedIds, true)) { + $confirmedEntityIds[] = (int) $docId; } else { - $deleted += count($confirmedEntityIds); + $skipped[] = (int) $docId; } + } - $scanned += $countInBatch; - $lastId = (int) $docIds[array_key_last($docIds)]; - } while ($countInBatch >= $batchSize); + $deleted = count($confirmedEntityIds); + if (!$dryRun && $confirmedEntityIds !== []) { + $deleted = $provider->deleteByIds($confirmedEntityIds); + } + + return [ + 'deleted' => $deleted, + 'skipped' => $skipped, + 'lastId' => (int) $docIds[array_key_last($docIds)], + ]; + } + /** @param int[] $skipped */ + private function reportResults( + SearchPurgeProviderInterface $provider, + int $scanned, + int $deleted, + array $skipped, + bool $dryRun, + SymfonyStyle $io, + ): void { if ($skipped !== []) { $io->warning(sprintf( '%s: %d row(s) older than cutoff were not found in Elasticsearch and were left in place: %s', diff --git a/src/Domain/Search/Model/Interfaces/SearchPurgeProviderInterface.php b/src/Domain/Search/Model/Interfaces/SearchPurgeProviderInterface.php index 69b51cd5..35e95763 100644 --- a/src/Domain/Search/Model/Interfaces/SearchPurgeProviderInterface.php +++ b/src/Domain/Search/Model/Interfaces/SearchPurgeProviderInterface.php @@ -29,4 +29,4 @@ public function fetchBatchOlderThan(DateTimeInterface $cutoff, int $lastId, int /** @param int[] $ids @return int number of rows deleted */ public function deleteByIds(array $ids): int; -} \ No newline at end of file +} diff --git a/src/Domain/Search/Registry/SearchPurgeProviderRegistry.php b/src/Domain/Search/Registry/SearchPurgeProviderRegistry.php index d6eef94b..3cd3236e 100644 --- a/src/Domain/Search/Registry/SearchPurgeProviderRegistry.php +++ b/src/Domain/Search/Registry/SearchPurgeProviderRegistry.php @@ -34,4 +34,4 @@ public function find(string $alias): ?SearchPurgeProviderInterface return null; } -} \ No newline at end of file +} diff --git a/src/Domain/Subscription/Model/Subscriber.php b/src/Domain/Subscription/Model/Subscriber.php index 84bbb1f8..e29a9b7d 100644 --- a/src/Domain/Subscription/Model/Subscriber.php +++ b/src/Domain/Subscription/Model/Subscriber.php @@ -94,6 +94,18 @@ class Subscriber implements DomainModel, Identity, CreationDate, ModificationDat )] private Collection $attributes; + /** + * Doctrine-only bookkeeping, not part of the public API (see getHistory()/setHistory() for that). + * SubscriberHistory's FK has only a DB-level ON DELETE CASCADE - without this mapped association, + * removing a Subscriber straight through the EntityManager would let the database silently drop its + * SubscriberHistory rows without Doctrine ever loading/removing them individually, so + * SearchIndexDoctrineListener would never fire for those rows and their Elasticsearch documents + * would be orphaned. This cascade makes Doctrine remove them itself instead. + * @var Collection + */ + #[ORM\OneToMany(targetEntity: SubscriberHistory::class, mappedBy: 'subscriber', cascade: ['remove'])] + private Collection $historyRecords; + #[ORM\Column(name: 'optedin', type: 'boolean')] private bool $optedIn = false; @@ -123,6 +135,7 @@ public function __construct(string $email) $this->email = $email; $this->subscriptions = new ArrayCollection(); $this->attributes = new ArrayCollection(); + $this->historyRecords = new ArrayCollection(); $this->extraData = ''; $this->createdAt = new DateTime(); $this->updatedAt = new DateTime(); diff --git a/src/Domain/Subscription/Model/SubscriberHistory.php b/src/Domain/Subscription/Model/SubscriberHistory.php index cf1b08e9..91991c44 100644 --- a/src/Domain/Subscription/Model/SubscriberHistory.php +++ b/src/Domain/Subscription/Model/SubscriberHistory.php @@ -31,7 +31,7 @@ class SubscriberHistory implements #[ORM\GeneratedValue] private ?int $id = null; - #[ORM\ManyToOne(targetEntity: Subscriber::class)] + #[ORM\ManyToOne(targetEntity: Subscriber::class, inversedBy: 'historyRecords')] #[ORM\JoinColumn(name: 'userid', referencedColumnName: 'id', nullable: false, onDelete: 'CASCADE')] private Subscriber $subscriber; diff --git a/src/Domain/Subscription/Service/Search/SubscriberHistoryPurgeProvider.php b/src/Domain/Subscription/Service/Search/SubscriberHistoryPurgeProvider.php index 1a71ed1d..20efdd83 100644 --- a/src/Domain/Subscription/Service/Search/SubscriberHistoryPurgeProvider.php +++ b/src/Domain/Subscription/Service/Search/SubscriberHistoryPurgeProvider.php @@ -48,4 +48,4 @@ public function deleteByIds(array $ids): int { return $this->repository->deleteByIds($ids); } -} \ No newline at end of file +} diff --git a/tests/Integration/Domain/Subscription/Repository/SubscriberHistoryRepositoryTest.php b/tests/Integration/Domain/Subscription/Repository/SubscriberHistoryRepositoryTest.php index 840530ec..c576618f 100644 --- a/tests/Integration/Domain/Subscription/Repository/SubscriberHistoryRepositoryTest.php +++ b/tests/Integration/Domain/Subscription/Repository/SubscriberHistoryRepositoryTest.php @@ -92,7 +92,8 @@ public function testDeleteByIdsRemovesOnlyGivenRows(): void $toKeep = $this->persistHistoryRow(new DateTime('2020-06-01')); $deleted = $this->repository->deleteByIds([$toDelete->getId()]); - $this->entityManager->clear(); // bulk DQL delete bypasses the identity map + // bulk DQL delete bypasses the identity map + $this->entityManager->clear(); self::assertSame(1, $deleted); self::assertSame(1, $this->repository->countOlderThan(new DateTime('2025-01-01'))); @@ -108,4 +109,30 @@ public function testDeleteByIdsWithEmptyArrayDeletesNothing(): void self::assertSame(0, $deleted); } -} \ No newline at end of file + + public function testRemovingSubscriberCascadeDeletesItsHistoryRecords(): void + { + $subscriber = new Subscriber('cascade-' . uniqid('', true) . '@example.com'); + $this->entityManager->persist($subscriber); + $this->entityManager->flush(); + + $history = new SubscriberHistory($subscriber); + $this->entityManager->persist($history); + $this->entityManager->flush(); + $historyId = $history->getId(); + $subscriberId = $subscriber->getId(); + + // Removing the Subscriber directly (not via SubscriberDeletionService) must still cascade to + // SubscriberHistory through Doctrine, not rely solely on the DB-level ON DELETE CASCADE, so + // SearchIndexDoctrineListener::preRemove/postRemove fires for the history row too. Cascade + // remove only walks a *loaded* collection, so re-fetch the Subscriber fresh from the DB first, + // as any real caller doing this outside of SubscriberDeletionService's manual loop would. + $this->entityManager->clear(); + $fetchedSubscriber = $this->entityManager->find(Subscriber::class, $subscriberId); + $this->entityManager->remove($fetchedSubscriber); + $this->entityManager->flush(); + $this->entityManager->clear(); + + self::assertNull($this->repository->find($historyId)); + } +} diff --git a/tests/Unit/Domain/Search/Command/PurgeSearchIndexedRowsCommandTest.php b/tests/Unit/Domain/Search/Command/PurgeSearchIndexedRowsCommandTest.php index a53c8b69..a3046cd0 100644 --- a/tests/Unit/Domain/Search/Command/PurgeSearchIndexedRowsCommandTest.php +++ b/tests/Unit/Domain/Search/Command/PurgeSearchIndexedRowsCommandTest.php @@ -120,4 +120,4 @@ public function testFailsWhenAliasHasNoRetentionPeriodConfigured(): void $this->assertStringContainsString('No retention period configured', $tester->getDisplay()); $this->assertSame(1, $tester->getStatusCode()); } -} \ No newline at end of file +} diff --git a/tests/Unit/Domain/Subscription/Service/Search/SubscriberHistoryPurgeProviderTest.php b/tests/Unit/Domain/Subscription/Service/Search/SubscriberHistoryPurgeProviderTest.php index a734d72f..fb49fca8 100644 --- a/tests/Unit/Domain/Subscription/Service/Search/SubscriberHistoryPurgeProviderTest.php +++ b/tests/Unit/Domain/Subscription/Service/Search/SubscriberHistoryPurgeProviderTest.php @@ -54,4 +54,4 @@ public function testGetSearchIndexNameIncludesPrefix(): void $this->assertSame('phplist_subscriber_history', $provider->getSearchIndexName()); } -} \ No newline at end of file +} From 50c01ec173a7b56a2ab85f95bf2c13ac93016b36 Mon Sep 17 00:00:00 2001 From: Tatevik Date: Mon, 7 Sep 2026 11:54:58 +0400 Subject: [PATCH 24/24] After review 0 --- .../Search/Command/ReindexSearchCommand.php | 7 ++++++- .../UserMessageBounceElasticsearchReaderTest.php | 15 +++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/Domain/Search/Command/ReindexSearchCommand.php b/src/Domain/Search/Command/ReindexSearchCommand.php index 55189f57..c87543cc 100644 --- a/src/Domain/Search/Command/ReindexSearchCommand.php +++ b/src/Domain/Search/Command/ReindexSearchCommand.php @@ -94,13 +94,18 @@ private function reindexProvider( do { $batch = $provider->fetchBatch($lastId, $batchSize); $countInBatch = 0; + // Captured once per batch, before any of this batch's ES writes, rather than per document + // at send time: a batch can take a while to send (progress bar, ES round trips), and a + // per-document revision taken at send time could end up newer than a concurrent delete's + // revision for a row this batch already read earlier, resurrecting it in Elasticsearch. + $revision = (int) (microtime(true) * 1_000_000); foreach ($batch as $entity) { $this->indexer->index( $entity->getSearchIndexName(), $entity->getSearchDocumentId(), $entity->toSearchDocument(), - (int) (microtime(true) * 1_000_000), + $revision, ); $lastId = (int) $entity->getSearchDocumentId(); $countInBatch++; diff --git a/tests/Unit/Domain/Messaging/Repository/UserMessageBounceElasticsearchReaderTest.php b/tests/Unit/Domain/Messaging/Repository/UserMessageBounceElasticsearchReaderTest.php index bcbadcc5..f195ada3 100644 --- a/tests/Unit/Domain/Messaging/Repository/UserMessageBounceElasticsearchReaderTest.php +++ b/tests/Unit/Domain/Messaging/Repository/UserMessageBounceElasticsearchReaderTest.php @@ -67,10 +67,25 @@ public function testGetFilteredAfterIdQueriesPrefixedIndexAndHydratesResults(): public function testGetFilteredAfterIdPaginatesAcrossTwoPagesWithoutRepeatingResults(): void { $firstFilter = new UserMessageBounceFilter(lastId: 0, limit: 1); + $expectedCursors = [0, 5]; + $call = 0; $this->client ->expects($this->exactly(2)) ->method('search') + ->with( + 'phplist_user_message_bounce', + $this->callback(function (array $query) use (&$call, $expectedCursors): bool { + $expectedCursor = $expectedCursors[$call]; + $call++; + + return $query['query']['bool']['filter'][0] === [ + 'range' => [ + 'idSort' => ['gt' => $expectedCursor] + ] + ]; + }), + ) ->willReturnOnConsecutiveCalls( [ 'hits' => [