diff --git a/phpunit.xml.dist b/phpunit.xml.dist index 745ac44..956c8ef 100644 --- a/phpunit.xml.dist +++ b/phpunit.xml.dist @@ -1,13 +1,17 @@ - - - tests/ - - - - src/ - - tests/ - - - + + + + + src/ + + + tests/ + + + + tests/ + + + + diff --git a/src/DataObject/DataObjectBatchProcessor.php b/src/DataObject/DataObjectBatchProcessor.php index c2d1399..db0b12e 100644 --- a/src/DataObject/DataObjectBatchProcessor.php +++ b/src/DataObject/DataObjectBatchProcessor.php @@ -31,6 +31,8 @@ public function removeDocuments(array $documents): array $this->run($job); foreach ($documents as $doc) { + // Indexer::METHOD_ADD as default parameter make sure we check first its related documents + // and decide whether we should delete or update them automatically. $childJob = RemoveDataObjectJob::create($doc, $timestamp); $this->run($childJob); } diff --git a/src/Jobs/RemoveDataObjectJob.php b/src/Jobs/RemoveDataObjectJob.php index 5314339..8a1d825 100644 --- a/src/Jobs/RemoveDataObjectJob.php +++ b/src/Jobs/RemoveDataObjectJob.php @@ -11,6 +11,10 @@ use SilverStripe\Versioned\Versioned; /** + * By virture of the default parameter, Index::METHOD_ADD, this does not remove the documents straight away. + * It checks first the status of the underlaying DataObjects and decides whether to remove or add them to the index. + * Then pass on to the parent's process() method to handle the job. + * * @property DataObjectDocument|null $document * @property int|null $timestamp */ @@ -19,6 +23,8 @@ class RemoveDataObjectJob extends IndexJob public function __construct(?DataObjectDocument $document = null, ?int $timestamp = null, ?int $batchSize = null) { + // Indexer::METHOD_ADD as default parameter make sure we check first its related documents + // whether we should delete or update them automatically. parent::__construct([], Indexer::METHOD_ADD, $batchSize); if ($document !== null) { @@ -46,7 +52,7 @@ public function getTitle(): string */ public function setup(): void { - // Set the documents in setup to ensure async + /** @var DBDatetime $datetime - set the documents in setup to ensure async */ $datetime = DBField::create_field('Datetime', $this->getTimestamp()); $archiveDate = $datetime->format($datetime->getISOFormat()); $documents = Versioned::withVersionedMode(function () use ($archiveDate) { diff --git a/src/Service/Indexer.php b/src/Service/Indexer.php index b46f3a3..9ea719e 100644 --- a/src/Service/Indexer.php +++ b/src/Service/Indexer.php @@ -124,6 +124,8 @@ function (DocumentInterface $dependentDocument) use ($document) { ); if ($dependentDocs) { + // Indexer::METHOD_ADD as default parameter make sure we check first its related documents + // and decide whether we should delete or update them automatically. $child = Indexer::create($dependentDocs, self::METHOD_ADD, $this->getBatchSize()); while (!$child->finished()) { diff --git a/tests/DataObject/DataObjectDocumentTest.php b/tests/DataObject/DataObjectDocumentTest.php index 980e83b..d674def 100644 --- a/tests/DataObject/DataObjectDocumentTest.php +++ b/tests/DataObject/DataObjectDocumentTest.php @@ -17,6 +17,7 @@ use SilverStripe\SearchService\Tests\Fake\DataObjectFakeVersioned; use SilverStripe\SearchService\Tests\Fake\DataObjectSubclassFake; use SilverStripe\SearchService\Tests\Fake\ImageFake; +use SilverStripe\SearchService\Tests\Fake\PageFake; use SilverStripe\SearchService\Tests\Fake\TagFake; use SilverStripe\SearchService\Tests\SearchServiceTest; use SilverStripe\Security\Member; @@ -45,6 +46,7 @@ class DataObjectDocumentTest extends SearchServiceTest DataObjectSubclassFake::class, DataObjectFakeVersioned::class, DataObjectFakePrivate::class, + PageFake::class, ]; public function testGetIdentifier(): void diff --git a/tests/Fake/ImageFake.php b/tests/Fake/ImageFake.php index f117a95..df1c697 100644 --- a/tests/Fake/ImageFake.php +++ b/tests/Fake/ImageFake.php @@ -10,11 +10,13 @@ class ImageFake extends DataObject implements TestOnly { private static array $db = [ + 'Title' => 'Varchar', 'URL' => 'Varchar', ]; private static array $has_one = [ 'Parent' => DataObjectFake::class, + 'PageFake' => PageFake::class, ]; private static array $many_many = [ @@ -27,4 +29,10 @@ class ImageFake extends DataObject implements TestOnly private static string $table_name = 'ImageFake'; + // For DataObjects that are not versioned, canView is needed and must return true + public function canView($member = null) // phpcs:ignore SlevomatCodingStandard.TypeHints + { + return true; + } + } diff --git a/tests/Fake/PageFake.php b/tests/Fake/PageFake.php new file mode 100644 index 0000000..6d27d24 --- /dev/null +++ b/tests/Fake/PageFake.php @@ -0,0 +1,31 @@ + TagFake::class, + ]; + + private static array $has_many = [ + 'Images' => ImageFake::class, + ]; + + private static array $owns = [ + 'Tags', + 'Images', + ]; + + private static string $table_name = 'PageFake'; + + private static array $extensions = [ + SearchServiceExtension::class, + ]; + +} diff --git a/tests/Jobs/RemoveDataObjectJobTest.php b/tests/Jobs/RemoveDataObjectJobTest.php index 3f98d0a..8660c86 100644 --- a/tests/Jobs/RemoveDataObjectJobTest.php +++ b/tests/Jobs/RemoveDataObjectJobTest.php @@ -2,10 +2,10 @@ namespace SilverStripe\SearchService\Tests\Jobs; -use Page; use SilverStripe\SearchService\DataObject\DataObjectDocument; use SilverStripe\SearchService\Jobs\RemoveDataObjectJob; use SilverStripe\SearchService\Schema\Field; +use SilverStripe\SearchService\Service\Indexer; use SilverStripe\SearchService\Tests\Fake\DataObjectFake; use SilverStripe\SearchService\Tests\Fake\DataObjectFakePrivate; use SilverStripe\SearchService\Tests\Fake\DataObjectFakeVersioned; @@ -17,10 +17,7 @@ class RemoveDataObjectJobTest extends SearchServiceTest { - protected static $fixture_file = [ // @phpcs:ignore - '../fixtures.yml', - '../pages.yml', - ]; + protected static $fixture_file = '../fixtures.yml'; // @phpcs:ignore /** * @phpcsSuppress SlevomatCodingStandard.TypeHints.PropertyTypeHint.MissingNativeTypeHint @@ -57,6 +54,23 @@ public function testJob(): void ] ); + $index = [ + 'main' => [ + 'includeClasses' => [ + DataObjectFake::class => ['title' => true], + TagFake::class => ['title' => true], + ], + ], + ]; + + $config->set( + 'getIndexesForClassName', + [ + DataObjectFake::class => $index, + TagFake::class => $index, + ] + ); + // Select tag one from our fixture $tag = $this->objFromFixture(TagFake::class, 'one'); // Queue up a job to remove our Tag, the result should be that any related DataObject (DOs that have this Tag @@ -66,6 +80,9 @@ public function testJob(): void ); $job->setup(); + // Creating this job does not necessarily mean to delete documents from index + $this->assertEquals(Indexer::METHOD_ADD, $job->getMethod()); + // Grab what Documents the Job determined it needed to action /** @var DataObjectDocument[] $documents */ $documents = $job->getDocuments(); @@ -80,66 +97,25 @@ public function testJob(): void $resultTitles = []; + // This determines whether the document should be added or removed from from the index foreach ($documents as $document) { $resultTitles[] = $document->getDataObject()?->Title; - } - - $this->assertEqualsCanonicalizing($expectedTitles, $resultTitles); - } - - public function testUnpublishedParentPage(): void - { - $config = $this->mockConfig(); - - $config->set( - 'getSearchableClasses', - [ - Page::class, - ] - ); - $config->set( - 'getFieldsForClass', - [ - Page::class => [ - new Field('title'), - new Field('content'), - ], - ] - ); - - // Publish all pages in fixtures since the internal dependency checks looks for live version - for ($i=1; $i<=8; $i++) { - $this->objFromFixture(Page::class, 'page' . $i)->publishRecursive(); + // The document should be added to index + $this->assertTrue($document->shouldIndex()); } - // Queue up a job to remove a page with child pages are added as related documents - $pageOne = $this->objFromFixture(Page::class, 'page1'); - $pageDoc = DataObjectDocument::create($pageOne); - $job = RemoveDataObjectJob::create($pageDoc); - $job->setup(); - - // Grab what Documents the Job determined it needed to action - /** @var DataObjectDocument[] $documents */ - $documents = $job->getDocuments(); - - // There should be two Pages with this Tag assigned - $this->assertCount(4, $documents); - - $expectedTitles = [ - 'Child Page 1', - 'Child Page 2', - 'Grandchild Page 1', - 'Grandchild Page 2', - ]; + $this->assertEqualsCanonicalizing($expectedTitles, $resultTitles); - $resultTitles = []; + // Deleting related documents so that they will be removed from index as well + $this->objFromFixture(DataObjectFake::class, 'one')->delete(); + $this->objFromFixture(DataObjectFake::class, 'three')->delete(); + // This determines whether the document should be added or removed from from the index foreach ($documents as $document) { - $resultTitles[] = $document->getDataObject()?->Title; + // The document should be removed from index + $this->assertFalse($document->shouldIndex()); } - - $this->assertEqualsCanonicalizing($expectedTitles, $resultTitles); } } diff --git a/tests/Jobs/RemoveRelatedDataObjectJobTest.php b/tests/Jobs/RemoveRelatedDataObjectJobTest.php new file mode 100644 index 0000000..7860be4 --- /dev/null +++ b/tests/Jobs/RemoveRelatedDataObjectJobTest.php @@ -0,0 +1,402 @@ +objFromFixture(Page::class, 'page' . $i)->publishRecursive(); + } + + $this->objFromFixture(PageFake::class, 'page9')->publishRecursive(); + $this->objFromFixture(PageFake::class, 'page10')->publishRecursive(); + $this->objFromFixture(PageFake::class, 'page11')->publishRecursive(); + $this->objFromFixture(TagFake::class, 'four')->publishRecursive(); + $this->objFromFixture(TagFake::class, 'five')->publishRecursive(); + $this->objFromFixture(TagFake::class, 'six')->publishRecursive(); + + $config = $this->mockConfig(); + + $config->set( + 'getSearchableClasses', + [ + DataObjectFake::class, + TagFake::class, + ImageFake::class, + PageFake::class, + Page::class, + ] + ); + + $config->set( + 'getFieldsForClass', + [ + DataObjectFake::class => [ + new Field('title'), + new Field('tagtitles', 'Tags.Title'), + ], + PageFake::class => [ + new Field('title'), + new Field('tagtitles', 'Tags.Title'), + ], + ImageFake::class => [ + new Field('title'), + new Field('url'), + new Field('tagtitles', 'Tags.Title'), + ], + ] + ); + + $index = [ + 'main' => [ + 'includeClasses' => [ + DataObjectFake::class => ['title' => true], + TagFake::class => ['title' => true], + ImageFake::class => ['title' => true], + PageFake::class => ['title' => true], + Page::class => ['title' => true], + ], + ], + ]; + + $config->set( + 'getIndexesForClassName', + [ + DataObjectFake::class => $index, + TagFake::class => $index, + ImageFake::class => $index, + PageFake::class => $index, + Page::class => $index, + ] + ); + } + + public function testUnpublishParentPage(): void + { + $childA = $this->objFromFixture(Page::class, 'page2'); + $childB = $this->objFromFixture(Page::class, 'page3'); + $grandChildA1 = $this->objFromFixture(Page::class, 'page7'); + $grandChildA2 = $this->objFromFixture(Page::class, 'page8'); + + $this->assertTrue($childA->isPublished()); + $this->assertTrue($childB->isPublished()); + $this->assertTrue($grandChildA1->isPublished()); + $this->assertTrue($grandChildA2->isPublished()); + + $pageOne = $this->objFromFixture(Page::class, 'page1'); + $pageOne->doUnpublish(); + + // Default behaviour when unpublishng the parent page + $this->assertTrue(SiteTree::config()->get('enforce_strict_hierarchy')); + $this->assertFalse($childA->isPublished()); + $this->assertFalse($childB->isPublished()); + $this->assertFalse($grandChildA1->isPublished()); + $this->assertFalse($grandChildA2->isPublished()); + + // Queue up a job to remove a page with child pages are added as related documents + $pageDoc = DataObjectDocument::create($pageOne); + $job = RemoveDataObjectJob::create($pageDoc); + $job->setup(); + + // Creating this job does not necessarily mean to delete documents from index + $this->assertEquals(Indexer::METHOD_ADD, $job->getMethod()); + + // Grab what Documents the Job determined it needed to action + /** @var DataObjectDocument[] $documents */ + $documents = $job->getDocuments(); + + // There should be two Pages with this Tag assigned + $this->assertCount(4, $documents); + + $expectedTitles = [ + 'Child of Parent Page 1 - A', + 'Child of Parent Page 1 - B', + 'Grandchild of Parent Page 1 - A1', + 'Grandchild of Parent Page 1 - A2', + ]; + + $resultTitles = []; + + // This determines whether the document should be added or removed from from the index + foreach ($documents as $document) { + $resultTitles[] = $document->getDataObject()?->Title; + + // The document should be removed from index + $this->assertFalse($document->shouldIndex()); + } + + $this->assertEqualsCanonicalizing($expectedTitles, $resultTitles); + } + + public function testUnpublishRelatedTagsObject(): void + { + $tagFour = $this->objFromFixture(TagFake::class, 'four'); + $tagFour->doUnpublish(); + + // Queue up a job to remove our Tag, the result should be that any related DataObject (DOs that have this Tag + // assigned to them) are added as related Documents + $job = RemoveDataObjectJob::create( + DataObjectDocument::create($tagFour) + ); + $job->setup(); + + // Creating this job does not necessarily mean to delete documents from index + $this->assertEquals(Indexer::METHOD_ADD, $job->getMethod()); + + // Grab what Documents the Job determined it needed to action + /** @var DataObjectDocument[] $documents */ + $documents = $job->getDocuments(); + + // There should be two Pages with this Tag assigned + $this->assertCount(3, $documents); + + $expectedTitles = [ + 'Child of Parent Page 2 - B', + 'Great Grandchild of Parent Page 2 - B1 - One', + 'Image Fake Three', + ]; + + $pageChild = $this->objFromFixture(PageFake::class, 'page9'); + $grandChild = $this->objFromFixture(PageFake::class, 'page10'); + $greatGrandChild = $this->objFromFixture(PageFake::class, 'page11'); + $imageThree = $this->objFromFixture(ImageFake::class, 'three'); + $imageFour = $this->objFromFixture(ImageFake::class, 'four'); + + $resultTitles = []; + + // This determines whether the document should be added or removed from from the index + foreach ($documents as $document) { + $resultTitles[] = $document->getDataObject()?->Title; + + // The document should be added to index + $this->assertTrue($document->shouldIndex()); + } + + $this->assertEqualsCanonicalizing( + $expectedTitles, + $resultTitles + ); + + $this->assertEqualsCanonicalizing( + $expectedTitles, + [ + $pageChild->Title, + $greatGrandChild->Title, + $imageThree->Title, + ], + ); + + Versioned::withVersionedMode(function () use ( + $tagFour, + $pageChild, + $greatGrandChild + ): void { + Versioned::set_stage(Versioned::LIVE); + + $this->assertNotContains($tagFour->ID, $pageChild->Tags()->column('ID')); + $this->assertNotContains($tagFour->ID, $greatGrandChild->Tags()->column('ID')); + }); + + + $tagFive = $this->objFromFixture(TagFake::class, 'five'); + $tagFive->doUnpublish(); + // Queue up a job to remove our Tag, the result should be that any related DataObject (DOs that have this Tag + // assigned to them) are added as related Documents + $job2 = RemoveDataObjectJob::create( + DataObjectDocument::create($tagFive) + ); + $job2->setup(); + + // Creating this job does not necessarily mean to delete documents from index + $this->assertEquals(Indexer::METHOD_ADD, $job->getMethod()); + + // Grab what Documents the Job determined it needed to action + /** @var DataObjectDocument[] $documents */ + $documents = $job2->getDocuments(); + + $resultTitles = []; + + foreach ($documents as $document) { + $resultTitles[] = $document->getDataObject()?->Title; + + // The document should be added to index + $this->assertTrue($document->shouldIndex()); + } + + // There should be two Pages with this Tag assigned + $this->assertCount(4, $documents); + + $expectedTitles = [ + 'Child of Parent Page 2 - B', + 'Grandchild of Parent Page 2 - B1', + 'Image Fake Three', + 'Image Fake Four', + ]; + + $this->assertEqualsCanonicalizing($expectedTitles, $resultTitles); + $this->assertEqualsCanonicalizing( + $expectedTitles, + [ + $pageChild->Title, + $grandChild->Title, + $imageThree->Title, + $imageFour->Title, + ], + ); + + Versioned::withVersionedMode(function () use ( + $tagFive, + $pageChild, + $grandChild, + $imageThree, + $imageFour, + ): void { + Versioned::set_stage(Versioned::LIVE); + + $this->assertNotContains($tagFive->ID, $pageChild->Tags()->column('ID')); + $this->assertNotContains($tagFive->ID, $grandChild->Tags()->column('ID')); + $this->assertNotContains($tagFive->ID, $imageThree->Tags()->column('ID')); + $this->assertNotContains($tagFive->ID, $imageFour->Tags()->column('ID')); + }); + + // Then unpublish these related pages to include them to the documents to be removed + $pageChild->doUnpublish(); + $grandChild->doUnpublish(); + $imageThree->delete(); + $imageFour->delete(); + + /** @var DataObjectDocument[] $documents */ + $documents = $job2->getDocuments(); + $resultTitles = []; + + foreach ($documents as $document) { + $resultTitles[] = $document->getDataObject()?->Title; + + // The document should be removed from index + $this->assertFalse($document->shouldIndex()); + } + + // There should be two Pages with this Tag assigned + $this->assertCount(4, $documents); + + $expectedTitles = [ + 'Child of Parent Page 2 - B', + 'Grandchild of Parent Page 2 - B1', + 'Image Fake Three', + 'Image Fake Four', + ]; + + $this->assertEqualsCanonicalizing($expectedTitles, $resultTitles); + $this->assertEqualsCanonicalizing( + $expectedTitles, + [ + $pageChild->Title, + $grandChild->Title, + $imageThree->Title, + $imageFour->Title, + ], + ); + } + + public function testUnpublishImageRelatedtoPages(): void + { + $grandChild = $this->objFromFixture(PageFake::class, 'page10'); + $greatGrandChild = $this->objFromFixture(PageFake::class, 'page11'); + + $this->assertTrue($grandChild->isPublished()); + $this->assertTrue($greatGrandChild->isPublished()); + + $imageFour = $this->objFromFixture(ImageFake::class, 'four'); + $imageFive = $this->objFromFixture(ImageFake::class, 'five'); + $tagSix = $this->objFromFixture(TagFake::class, 'six'); + // Queue up a job to remove our Image, the result should be that any related DataObject + $job = RemoveDataObjectJob::create( + DataObjectDocument::create($tagSix) + ); + $job->setup(); + + // Creating this job does not necessarily mean to delete documents from index + $this->assertEquals(Indexer::METHOD_ADD, $job->getMethod()); + + // Grab what Documents the Job determined it needed to action + /** @var DataObjectDocument[] $documents */ + $documents = $job->getDocuments(); + + // There should be two Pages with this Tag assigned + $this->assertCount(4, $documents); + + $expectedTitles = [ + 'Grandchild of Parent Page 2 - B1', + 'Great Grandchild of Parent Page 2 - B1 - One', + 'Image Fake Four', + 'Image Fake Five', + ]; + + $resultTitles = []; + + // This determines whether the document should be added or removed from from the index + foreach ($documents as $document) { + $resultTitles[] = $document->getDataObject()?->Title; + + // The document should be added to index + $this->assertTrue($document->shouldIndex()); + } + + $this->assertEqualsCanonicalizing( + $expectedTitles, + $resultTitles + ); + + $this->assertEqualsCanonicalizing( + $expectedTitles, + [ + $grandChild->Title, + $greatGrandChild->Title, + $imageFour->Title, + $imageFive->Title, + ], + ); + } + +} diff --git a/tests/pages.yml b/tests/pages.yml index 616ff25..6f75cab 100644 --- a/tests/pages.yml +++ b/tests/pages.yml @@ -1,20 +1,42 @@ +SilverStripe\SearchService\Tests\Fake\TagFake: + four: + Title: Tag four + five: + Title: Tag five + six: + Title: Tag six + +SilverStripe\SearchService\Tests\Fake\ImageFake: + three: + Title: Image Fake Three + URL: "/image-three/" + Tags: =>SilverStripe\SearchService\Tests\Fake\TagFake.four,=>SilverStripe\SearchService\Tests\Fake\TagFake.five + four: + Title: Image Fake Four + URL: "/image-four/" + Tags: =>SilverStripe\SearchService\Tests\Fake\TagFake.five,=>SilverStripe\SearchService\Tests\Fake\TagFake.six + five: + Title: Image Fake Five + URL: "/image-five/" + Tags: =>SilverStripe\SearchService\Tests\Fake\TagFake.six + Page: page1: - Title: Parent Page + Title: Parent Page 1 ShowInSearch: 1 page2: - Title: Child Page 1 + Title: Child of Parent Page 1 - A Parent: =>Page.page1 ShowInSearch: 1 page3: - Title: Child Page 2 + Title: Child of Parent Page 1 - B Parent: =>Page.page1 ShowInSearch: 1 page4: Title: Parent Page 2 ShowInSearch: 1 page5: - Title: Child Page 3 + Title: Child of Parent Page 2 - A Parent: =>Page.page4 ShowInSearch: 1 page6: @@ -22,10 +44,32 @@ Page: Subsite: =>SilverStripe\Subsites\Model\Subsite.subsite1 ShowInSearch: 1 page7: - Title: Grandchild Page 1 + Title: Grandchild of Parent Page 1 - A1 Parent: =>Page.page2 ShowInSearch: 1 page8: - Title: Grandchild Page 2 + Title: Grandchild of Parent Page 1 - A2 Parent: =>Page.page2 ShowInSearch: 1 + +SilverStripe\SearchService\Tests\Fake\PageFake: + page9: + Title: Child of Parent Page 2 - B + Parent: =>Page.page4 + Banner: =>SilverStripe\SearchService\Tests\Fake\ImageFake.five + Tags: =>SilverStripe\SearchService\Tests\Fake\TagFake.four,=>SilverStripe\SearchService\Tests\Fake\TagFake.five + Images: =>SilverStripe\SearchService\Tests\Fake\ImageFake.three + ShowInSearch: 1 + page10: + Title: Grandchild of Parent Page 2 - B1 + Parent: =>SilverStripe\SearchService\Tests\Fake\PageFake.page9 + Banner: =>SilverStripe\SearchService\Tests\Fake\ImageFake.five + Tags: =>SilverStripe\SearchService\Tests\Fake\TagFake.five,=>SilverStripe\SearchService\Tests\Fake\TagFake.six + Images: =>SilverStripe\SearchService\Tests\Fake\ImageFake.four + ShowInSearch: 1 + page11: + Title: Great Grandchild of Parent Page 2 - B1 - One + Parent: =>SilverStripe\SearchService\Tests\Fake\PageFake.page10 + Banner: =>SilverStripe\SearchService\Tests\Fake\ImageFake.five + Tags: =>SilverStripe\SearchService\Tests\Fake\TagFake.four,=>SilverStripe\SearchService\Tests\Fake\TagFake.six + ShowInSearch: 1