diff --git a/Classes/Command/ContentReleaseValidationCommandController.php b/Classes/Command/ContentReleaseValidationCommandController.php index 69f5687..edec92a 100644 --- a/Classes/Command/ContentReleaseValidationCommandController.php +++ b/Classes/Command/ContentReleaseValidationCommandController.php @@ -8,7 +8,6 @@ use Flowpack\DecoupledContentStore\Core\Domain\ValueObject\RedisInstanceIdentifier; use Flowpack\DecoupledContentStore\Core\Infrastructure\ContentReleaseLogger; use Flowpack\DecoupledContentStore\Exception; -use Flowpack\DecoupledContentStore\NodeEnumeration\Domain\Repository\RedisEnumerationRepository; use Flowpack\DecoupledContentStore\NodeRendering\Infrastructure\RedisRenderingErrorManager; use Flowpack\DecoupledContentStore\QuickPublish\ContentReleaseScope; use Flowpack\DecoupledContentStore\ReleaseSwitch\Infrastructure\RedisReleaseSwitchService; @@ -32,12 +31,6 @@ class ContentReleaseValidationCommandController extends CommandController */ protected $redisReleaseSwitchService; - /** - * @Flow\Inject - * @var RedisEnumerationRepository - */ - protected $redisEnumerationRepository; - #[Flow\Inject] protected ContentReleaseScope $contentReleaseScope; @@ -68,8 +61,12 @@ public function validateCommand(string $contentReleaseIdentifier) } $logger->info('Previous Content Release: ' . $currentlyLiveReleaseIdentifier->getIdentifier()); - $currentUrlsCount = $this->countUrls($currentlyLiveReleaseIdentifier, $logger, 'Currently live release'); - $newUrlsCount = $this->countUrls($contentReleaseIdentifier, $logger, 'Content release'); + // Both sides are counted by the URLs they publish, never by their enumeration: the enumeration holds one + // entry per document *and renderer*, so with a second configured document renderer it is a multiple of the + // URL count - and the enumeration of a quick release covers only the documents it re-rendered, while it + // publishes everything it copied along. `meta:urls` is the one measure which means the same on both sides. + $currentUrlsCount = $this->contentReleaseScope->countPublishedUrls($currentlyLiveReleaseIdentifier); + $newUrlsCount = $this->contentReleaseScope->countPublishedUrls($contentReleaseIdentifier); $minimumUrlsCount = (int) ceil($this->validReleaseUrlCountThreshold * $currentUrlsCount); $logger->info('Previous URL Count: ' . $currentUrlsCount); @@ -112,29 +109,6 @@ public function validateCommand(string $contentReleaseIdentifier) $this->logCompletion($logger, $startedAt); } - /** - * How many URLs a content release covers, measured so that two releases are comparable. - * - * A quick release enumerates only the handful of documents it re-renders and copies the rest, so its enumeration - * describes neither what it publishes nor what a later release has to live up to - taken as the baseline it would - * put the threshold at a handful of URLs and wave through any release which lost most of the site. Its published - * URLs are the comparable number: after the copy they equal the release it was built on. Each release is - * therefore measured on its own terms, whichever side of the comparison it is on. - */ - private function countUrls( - ContentReleaseIdentifier $contentReleaseIdentifier, - ContentReleaseLogger $logger, - string $label, - ): int { - if ($this->contentReleaseScope->getChangedUrls($contentReleaseIdentifier) === null) { - return $this->redisEnumerationRepository->count($contentReleaseIdentifier); - } - - $logger->info($label . ' is a quick release, so its published URLs are counted instead of its enumeration.'); - - return $this->contentReleaseScope->countPublishedUrls($contentReleaseIdentifier); - } - /** * Final log line of the command. If it is the last line you see while the task is still marked as * "running" in the UI, the remaining time is NOT spent in this command, but in another script line diff --git a/Classes/QuickPublish/ContentReleaseScope.php b/Classes/QuickPublish/ContentReleaseScope.php index 96aad8e..74d9643 100644 --- a/Classes/QuickPublish/ContentReleaseScope.php +++ b/Classes/QuickPublish/ContentReleaseScope.php @@ -70,8 +70,9 @@ public function setChangedUrls(ContentReleaseIdentifier $contentReleaseIdentifie /** * How many URLs the release holds. * - * This is what makes two releases comparable in size: the enumeration of a quick release only covers what it - * re-rendered, while every release - copied or rendered - carries the full list of URLs it publishes. + * This is what makes two releases comparable in size, and the only measure which does: every release - copied or + * rendered - carries the full list of URLs it publishes, while the enumeration of a quick release covers only + * what it re-rendered, and the enumeration of any release counts documents per renderer rather than URLs. */ public function countPublishedUrls(ContentReleaseIdentifier $contentReleaseIdentifier): int { diff --git a/Documentation/Concepts/QuickContentReleases.md b/Documentation/Concepts/QuickContentReleases.md index 5102aed..145b65a 100644 --- a/Documentation/Concepts/QuickContentReleases.md +++ b/Documentation/Concepts/QuickContentReleases.md @@ -247,15 +247,21 @@ every ordinary release through, so the two cases have to be handled explicitly. ### `contentReleaseValidation:validate` had to be adapted, not just scoped -This is a trap rather than an optimisation. The validator compares the enumeration count of the new release against -the live one and aborts below 70%. A quick release deliberately enumerates a handful of documents instead of all of -them, so it is counted by its number of *published* URLs instead, which after a copy-forward equals the release it -was built on. Both sides of the comparison are measured that way, each release on its own terms: as the new release a -quick one would fail the check every single time, and as the currently live release it would put the threshold at a -handful of URLs and let the next full release pass no matter how much of the site that one lost. +This is a trap rather than an optimisation. The validator compared the enumeration count of the new release against +the live one and aborted below 70%. A quick release deliberately enumerates a handful of documents instead of all of +them, so as the new release it would fail that check every single time, and as the currently live release it would put +the threshold at a handful of URLs and let the next full release pass no matter how much of the site that one lost. + +It therefore counts what a release *publishes* — the `meta:urls` cardinality — on **both** sides. Measuring each +release on its own terms, the enumeration for a full one and the published URLs for a quick one, is not enough: the +two are different units. The enumeration holds one entry per document **and renderer**, so an installation with a +second document renderer (Louis renders every page as HTML and as headless JSON) enumerates twice as many entries as +it has URLs. A quick release compared against a full one that way reports 50% and is refused, and a full release +compared against a quick one reports 200% and is waved through whatever it lost — the check disabled in the direction +where it matters. Any project validator which reasons about the size of the enumeration has the same problem, and the failure mode is -the good one — the release is refused rather than published wrongly — but it needs the same treatment. +not always the good one — see above for the direction in which the comparison silently passes. ## 7. The pause switch diff --git a/README.md b/README.md index f5d9337..4e32616 100644 --- a/README.md +++ b/README.md @@ -447,12 +447,16 @@ if ($changedUrls === null) { everything through, so treat the two cases explicitly. The typical win is turning an `hGetAll` over the whole document hash into an `hMGet` for the changed URLs. -The package's own `contentReleaseValidation:validate` already does this, and it had to: it compares the enumeration -of the new release against the live one and aborts below 70%, while a quick release deliberately enumerates a handful -of documents instead of all of them. A quick release is therefore counted by its number of published URLs — which -after a copy-forward equals the release it was built on — whichever side of the comparison it stands on. As the new -release its enumeration would fail the check every single time; as the currently live one it would put the threshold -at a handful of URLs and wave the next full release through however much of the site that one lost. +The package's own `contentReleaseValidation:validate` had to be adapted as well: it aborts a release below 70% of the +size of the live one, while a quick release deliberately enumerates a handful of documents instead of all of them. As +the new release its enumeration would fail that check every single time; as the currently live one it would put the +threshold at a handful of URLs and wave the next full release through however much of the site that one lost. + +It therefore counts the URLs a release *publishes* (`ContentReleaseScope::countPublishedUrls()`, the `meta:urls` +cardinality) on **both** sides, which after a copy-forward equals the release the quick one was built on. Do the same +in a size check of your own, and do not mix the two measures: the enumeration holds one entry per document **and +renderer**, so with a second document renderer configured it is a multiple of the URL count, and comparing one against +the other refuses every quick release while letting a full release which lost half the site pass. ### The commands diff --git a/Tests/Behavior/Features/Bootstrap/FeatureContext.php b/Tests/Behavior/Features/Bootstrap/FeatureContext.php index 10fd495..8ab87b2 100644 --- a/Tests/Behavior/Features/Bootstrap/FeatureContext.php +++ b/Tests/Behavior/Features/Bootstrap/FeatureContext.php @@ -16,6 +16,7 @@ use Flowpack\DecoupledContentStore\Core\RedisKeyService; use Flowpack\DecoupledContentStore\Exception as DecoupledContentStoreException; use Flowpack\DecoupledContentStore\IncrementalContentReleaseHandler; +use Flowpack\DecoupledContentStore\NodeEnumeration\Domain\Dto\EnumeratedNode; use Flowpack\DecoupledContentStore\NodeEnumeration\Domain\Repository\RedisEnumerationRepository; use Flowpack\DecoupledContentStore\NodeEnumeration\Domain\Service\NodeContextCombinator; use Flowpack\DecoupledContentStore\NodeEnumeration\NodeEnumerator; @@ -409,6 +410,33 @@ public function theEnumerationContainsNode($contentReleaseIdentifier, $expectedC Assert::assertCount((int) $expectedCount, $enumerationAsArray); } + /** + * The enumeration is written per document *and renderer*, so a second document renderer doubles it while the + * release still publishes one URL per document. That is the one setup in which the length of the enumeration and + * the number of published URLs differ for an ordinary release, and rendering a page twice - as HTML and as JSON, + * say - is a common enough reason to configure one. + * + * @Given the enumeration of content release :contentReleaseIdentifier is duplicated for a second document renderer + */ + public function theEnumerationIsDuplicatedForASecondDocumentRenderer($contentReleaseIdentifier) + { + $contentReleaseIdentifier = ContentReleaseIdentifier::fromString($contentReleaseIdentifier); + $redisEnumerationRepository = $this->getObjectManager()->get(RedisEnumerationRepository::class); + + $enumerationOfSecondRenderer = []; + foreach ($redisEnumerationRepository->findAll($contentReleaseIdentifier) as $enumeratedNode) { + $enumerationOfSecondRenderer[] = EnumeratedNode::fromJsonString((string) json_encode(array_merge( + $enumeratedNode->jsonSerialize(), + ['rendererId' => 'secondRenderer'], + ))); + } + + $redisEnumerationRepository->addDocumentNodesToEnumeration( + $contentReleaseIdentifier, + ...$enumerationOfSecondRenderer, + ); + } + /** * @When I run the render-orchestrator control loop once for content release :contentReleaseIdentifier */ diff --git a/Tests/Behavior/Features/ContentStore/QuickRelease.feature b/Tests/Behavior/Features/ContentStore/QuickRelease.feature index 80df845..e059b80 100644 --- a/Tests/Behavior/Features/ContentStore/QuickRelease.feature +++ b/Tests/Behavior/Features/ContentStore/QuickRelease.feature @@ -66,13 +66,24 @@ Feature: Quick Release Scenario: A quick release is not rejected for enumerating only what it changed # the URL count check compares the new release against the live one, and the enumeration of a quick release is - # smaller than that by design - it has to be measured by the URLs it publishes instead + # smaller than that by design - both sides are therefore measured by the URLs they publish Given the currently live content release is "5" When I create a content release "6" And I copy the content release "5" to the content release "6" And I enumerate the node at path "/sites/test/sub" for content release "6" Then validating content release "6" succeeds + Scenario: A quick release is not rejected for the size of a release rendered by two renderers + # the enumeration counts documents per renderer, the released URLs count documents - so the two measures must not + # be compared against each other. A live release enumerated by two renderers is twice its own URL count, which + # puts the threshold above what any quick release publishes, and it refuses one which holds the whole site. + Given the currently live content release is "5" + And the enumeration of content release "5" is duplicated for a second document renderer + When I create a content release "6" + And I copy the content release "5" to the content release "6" + And I enumerate the node at path "/sites/test/sub" for content release "6" + Then validating content release "6" succeeds + Scenario: A node which cannot be found is not published # a quick release which renders nothing would publish the release it was copied from, and look successful When I create a content release "6"