From 18af2bd9fdaddad8c2a243694338f6f13ec83a95 Mon Sep 17 00:00:00 2001 From: Mark Anthony Adriano Date: Fri, 13 May 2022 11:08:38 +1200 Subject: [PATCH] FIX Index configuration overrides DO subsite field if NULL value --- .travis.yml | 35 ++++++++ docs/en/configuration.md | 15 ++-- .../Subsites/IndexConfigurationExtension.php | 23 +++++- .../Subsites/SearchAdminExtension.php | 7 +- tests/DataObject/DataObjectDocumentTest.php | 79 +++++++++++++++++++ tests/Fake/SubsiteDataObjectFake.php | 35 ++++++++ 6 files changed, 183 insertions(+), 11 deletions(-) create mode 100644 .travis.yml create mode 100644 tests/Fake/SubsiteDataObjectFake.php diff --git a/.travis.yml b/.travis.yml new file mode 100644 index 0000000..98c218b --- /dev/null +++ b/.travis.yml @@ -0,0 +1,35 @@ +language: php + +dist: trusty + +env: + global: + - COMPOSER_ROOT_VERSION=2.x-dev + - APP_SEARCH_API_KEY='test' + - APP_SEARCH_ENDPOINT='test' + +matrix: + include: + - php: 7.2 + env: DB=MYSQL PHPUNIT_TEST=1 PHPCS_TEST=1 + - php: 7.3 + env: DB=MYSQL PHPUNIT_COVERAGE_TEST=1 + +before_script: + # Init PHP + - phpenv rehash + - phpenv config-rm xdebug.ini + - composer validate + - composer require silverstripe/recipe-cms 4.5.x-dev --no-update + - composer require silverstripe/subsites 2.4.x-dev --no-update + - if [[ $DB == PGSQL ]]; then composer require silverstripe/postgresql:2.0.x-dev --no-update; fi + - if [[ $PHPCS_TEST ]]; then composer global require squizlabs/php_codesniffer:^3 --prefer-dist --no-interaction --no-progress --no-suggest -o; fi + - composer install --prefer-dist --no-interaction --no-progress --no-suggest --optimize-autoloader --verbose --profile + +script: + - if [[ $PHPUNIT_TEST ]]; then vendor/bin/phpunit tests/; fi + - if [[ $PHPUNIT_COVERAGE_TEST ]]; then phpdbg -qrr vendor/bin/phpunit --coverage-clover=coverage.xml; fi + - if [[ $PHPCS_TEST ]]; then vendor/bin/phpcs src/ tests/ ; fi + +after_success: + - if [[ $PHPUNIT_COVERAGE_TEST ]]; then bash <(curl -s https://codecov.io/bash) -f coverage.xml; fi diff --git a/docs/en/configuration.md b/docs/en/configuration.md index e6cf482..c6d4d9a 100644 --- a/docs/en/configuration.md +++ b/docs/en/configuration.md @@ -27,7 +27,7 @@ on your service provider. For EnterpriseSearch, it should only contain lowercase and hyphens. * `includedClasses`: A list of content classes to index. These are just the _source_ of the -content, so they have no contractual bind to the module. If they are dataobjects, they +content, so they have no contractual bind to the module. If they are dataobjects, they should have the `SearchServiceExtension` applied, however. This is discussed further below. * `SilverStripe\CMS\Model\SiteTree`: This class already has the necessary extension applied @@ -81,8 +81,8 @@ SilverStripe\SearchService\Service\IndexConfiguration: property: 'Comments.Author.Name' ``` -For DataObject content, the dot syntax allows traversal of relationships. If the final -property in the notation is on a list, it will use the `->column()` function to derive +For DataObject content, the dot syntax allows traversal of relationships. If the final +property in the notation is on a list, it will use the `->column()` function to derive the values as an array. This will roughly get indexed as a structure like this: @@ -193,7 +193,7 @@ SilverStripe\Core\Injector\Injector: constructor: index_variant: '`MY_CUSTOM_VAR`' -``` +``` This is useful if you have multiple staging environments and you don't want to overcrowd your search instance with distinct indexes for each one. @@ -201,7 +201,7 @@ your search instance with distinct indexes for each one. ## Full page indexing Page and DataObject content is eligible for full-page indexing of its content. This is -predicated upon the object having a `Link()` method defined that can be rendered in a +predicated upon the object having a `Link()` method defined that can be rendered in a controller. The content is extracted using an XPath selector. By default, this is `//main`, but it @@ -248,6 +248,9 @@ SilverStripe\SearchService\Service\IndexConfiguration: Note the syntax to reduce the need for copy-paste if you want to duplicate the same configuration across. +__Additional note__: +> In the sample above, if the data object (My\Other\Class) does not have a subsite ID, then it will be included in the indexing as it is explicitly defined in the index configuration + This is handled via `SubsiteIndexConfigurationExtension` - this logic could be replicated for other scenarios like languages if required. @@ -255,5 +258,5 @@ replicated for other scenarios like languages if required. * [Usage](usage.md) * [Implementations](implementations.md) -* [Customising and extending](customising.md) +* [Customising and extending](customising.md) * [Overview and Rationale](overview.md) diff --git a/src/Extensions/Subsites/IndexConfigurationExtension.php b/src/Extensions/Subsites/IndexConfigurationExtension.php index eeefa27..a45a869 100644 --- a/src/Extensions/Subsites/IndexConfigurationExtension.php +++ b/src/Extensions/Subsites/IndexConfigurationExtension.php @@ -17,11 +17,26 @@ public function updateIndexesForDocument(DocumentInterface $doc, array &$indexes $docSubsiteId = $doc->getDataObject()->SubsiteID ?? null; } - foreach ($indexes as $indexName => $data) { - $subsiteId = $data['subsite_id'] ?? 'all'; + if ($docSubsiteId === null) { + // DataObject does not have a defined SubsiteID + // So if the developer explicitly defined the dataObject to be + // included in the Subsite Index configuration then + // allow the dataObject to be added in. + foreach ($indexes as $indexName => $data) { + // DataObject explicitly defined on Subsite index definition + $explicitClasses = $data['includeClasses'] ?? []; + if (!isset($explicitClasses[$doc->getDataObject()->ClassName])) { + unset($indexes[$indexName]); + break; + } + } + } else { + foreach ($indexes as $indexName => $data) { + $subsiteId = $data['subsite_id'] ?? 'all'; - if ($subsiteId !== 'all' && $docSubsiteId !== $subsiteId) { - unset($indexes[$indexName]); + if ($subsiteId !== 'all' && $docSubsiteId !== $subsiteId) { + unset($indexes[$indexName]); + } } } } diff --git a/src/Extensions/Subsites/SearchAdminExtension.php b/src/Extensions/Subsites/SearchAdminExtension.php index 7f342b6..4e1d546 100644 --- a/src/Extensions/Subsites/SearchAdminExtension.php +++ b/src/Extensions/Subsites/SearchAdminExtension.php @@ -2,6 +2,7 @@ namespace SilverStripe\SearchService\Extensions\Subsites; +use SilverStripe\Core\Config\Config; use SilverStripe\Core\Extension; use SilverStripe\ORM\ArrayList; use SilverStripe\ORM\DataQuery; @@ -20,7 +21,11 @@ public function updateQuery(DataQuery $query, array $data): void } Subsite::disable_subsite_filter(true); - $query->where(sprintf('SubsiteID = %s', $data['subsite_id'])); + + // If the DataObject has a Subsite relation, then apply a SubsiteID filter + if (in_array(Subsite::class, Config::inst()->get($query->dataClass(), 'has_one'))) { + $query->where("SubsiteID IS NULL OR SubsiteID = {$data['subsite_id']}"); + } } } diff --git a/tests/DataObject/DataObjectDocumentTest.php b/tests/DataObject/DataObjectDocumentTest.php index 350c3df..2db2f6e 100644 --- a/tests/DataObject/DataObjectDocumentTest.php +++ b/tests/DataObject/DataObjectDocumentTest.php @@ -15,6 +15,7 @@ use SilverStripe\SearchService\Tests\Fake\DataObjectFakeVersioned; use SilverStripe\SearchService\Tests\Fake\DataObjectSubclassFake; use SilverStripe\SearchService\Tests\Fake\ImageFake; +use SilverStripe\SearchService\Tests\Fake\SubsiteDataObjectFake; use SilverStripe\SearchService\Tests\Fake\TagFake; use SilverStripe\SearchService\Tests\SearchServiceTest; use SilverStripe\Security\Member; @@ -29,6 +30,7 @@ class DataObjectDocumentTest extends SearchServiceTest * @var array */ protected static $extra_dataobjects = [ + SubsiteDataObjectFake::class, DataObjectFake::class, TagFake::class, ImageFake::class, @@ -95,6 +97,83 @@ public function testShouldIndex(): void $this->assertFalse($docThree->shouldIndex()); } + public function testSubsiteDataObjectShouldIndex(): void + { + $config = $this->mockConfig(); + + $config->set( + 'indexes', + [ + 'index0' => [ + 'subsite_id' => 0, + 'includeClasses' => [ + SubsiteDataObjectFake::class => true + ] + ], + 'index1' => [ + 'subsite_id' => 1, + 'includeClasses' => [ + DataObjectFake::class => true + ] + ], + ] + ); + + $dataobject1 = new SubsiteDataObjectFake([ + 'ID' => 6, + 'ShowInSearch' => true, + 'SubsiteID' => 2, + ]); + $dataobject1->write(); + $dataobject1->can_view = true; + + $doc1 = DataObjectDocument::create($dataobject1); + + // Incorrect SubsiteID for DO so should fail + $this->assertFalse($doc1->shouldIndex()); + + $dataobject2 = new SubsiteDataObjectFake([ + 'ID' => 7, + 'ShowInSearch' => true, + 'SubsiteID' => null, + ]); + $dataobject2->write(); + $dataobject2->can_view = true; + + $doc2 = DataObjectDocument::create($dataobject2); + + // SubsiteID is NULL for DO but explicitly defined in config so should be true. + $this->assertNull($dataobject2->SubsiteID); + $this->assertTrue($doc2->shouldIndex()); + + $dataobject3 = new SubsiteDataObjectFake([ + 'ID' => 7, + 'ShowInSearch' => true, + 'SubsiteID' => 0, + ]); + $dataobject3->write(); + $dataobject3->can_view = true; + + $doc3 = DataObjectDocument::create($dataobject3); + + // SubsiteID is correct on DO and as per config so should be true. + $this->assertEquals(0, $dataobject3->SubsiteID); + $this->assertTrue($doc3->shouldIndex()); + + $dataobject4 = new DataObjectFake([ + 'ID' => 8, + 'ShowInSearch' => true, + ]); + $dataobject4->write(); + $dataobject4->can_view = true; + + $doc4 = DataObjectDocument::create($dataobject3); + + // Check DataObjects which don't have a Subsite relation will still be indexed + $this->assertNull($dataobject4->SubsiteID); + $this->assertTrue($doc4->shouldIndex()); + } + public function testMarkIndexed(): void { $dataobject = new DataObjectFake(['ShowInSearch' => true]); diff --git a/tests/Fake/SubsiteDataObjectFake.php b/tests/Fake/SubsiteDataObjectFake.php new file mode 100644 index 0000000..c8dcd48 --- /dev/null +++ b/tests/Fake/SubsiteDataObjectFake.php @@ -0,0 +1,35 @@ + 'Subsite' + ); + + private static $db = [ + 'ShowInSearch' => 'Boolean', + ]; + + public function canView($member = null) + { + if (is_callable($this->can_view)) { + $func = $this->can_view; + return $func(); + } + return $this->can_view; + } +}