Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Force live object when adding to the index #91

Merged
merged 2 commits into from
Apr 17, 2024
Merged

Force live object when adding to the index #91

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8666f3d
Force live object on unserialise
blueo Apr 7, 2024
e95d234
Force live on adding to index
blueo Apr 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 37 additions & 14 deletions src/DataObject/DataObjectDocument.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
use SilverStripe\ORM\RelationList;
use SilverStripe\ORM\UnsavedRelationList;
use SilverStripe\SearchService\Exception\IndexConfigurationException;
use SilverStripe\SearchService\Exception\IndexingServiceException;
use SilverStripe\SearchService\Extensions\DBFieldExtension;
use SilverStripe\SearchService\Extensions\SearchServiceExtension;
use SilverStripe\SearchService\Interfaces\DataObjectDocumentInterface;
Expand Down Expand Up @@ -147,8 +148,10 @@ public function shouldIndex(): bool
return false;
}

// Dataobject is only in draft
if ($dataObject->hasExtension(Versioned::class) && !$dataObject->isLiveVersion()) {
// DataObject has no published version (or draft changes could cause a doc to be removed)
if ($dataObject->hasExtension(Versioned::class) && !$dataObject->isPublished()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have a feeling that your fix below will solve this edge case. The check for isLiveVersion does check the _Live table, so now that you've made sure the DataObject is consistently the LIVE record, I think this would work.

That said, it makes more practical sense to me for us to be checking isPublished(), as I think even if you have a different LIVE version, you would still want to reindex (and you certainly wouldn't want to remove it from the index), so I'm more than happy to make this switch.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The change to the Before Add event will actually run after this check. So its possible that you can add a DO that is the latest version, but it is draft (eg you published and then saved some draft changes). In this case the most recent (read draft) DO will be fetched by the _unserialize function because dev tasks run in 'draft' reading mode. Then the isLiveVersion will fail (as its not the live version, it is a version after) and the indexer will remove it.

I don't think we can always make the attached dataobject Live because we need a record for deleted versions (or we'd need to rethink how deletions are done)

// note even if we pass a draft object to the indexer onAddToSearchIndexes will
// set the version to live before adding
return false;
}

Expand Down Expand Up @@ -204,24 +207,20 @@ public function getIndexes(): array
/**
* Generates a map of all the fields and values which will be sent
*
* This will always use the current DataObject so you must ensure
* it is in the correct state (eg Live) prior to calling toArray.
* For example the onAddToSearchIndexes method will set the data
* object to LIVE when adding to the index
*
* @see DataObjectDocument::onAddToSearchIndexes()
* @throws IndexConfigurationException
*/
public function toArray(): array
{
$pageContentField = $this->config()->get('page_content_field');

if ($this->getDataObject()->hasExtension(Versioned::class)) {
$dataObject = Versioned::withVersionedMode(function () {
Versioned::set_stage(Versioned::LIVE);

return DataObject::get_by_id($this->getSourceClass(), $this->getDataObject()->ID);
});
} else {
$dataObject = DataObject::get_by_id(
$this->getSourceClass(),
$this->getDataObject()->ID
);
}
// assume shouldIndex is called before this
$dataObject = $this->getDataObject();

if (!$dataObject || !$dataObject->exists()) {
throw new IndexConfigurationException(
Expand Down Expand Up @@ -628,8 +627,32 @@ public function __unserialize(array $data): void
}
}

/**
* Add to index event handler
*
* @throws IndexingServiceException
* @return void
*/
public function onAddToSearchIndexes(string $event): void
{
if ($event === DocumentAddHandler::BEFORE_ADD) {
// make sure DataObject is always live on adding to the index
Versioned::withVersionedMode(function (): void {
Versioned::set_stage(Versioned::LIVE);

$currentDataObject = $this->getDataObject();

$liveDataObject = DataObject::get($currentDataObject->ClassName)->byID($currentDataObject->ID);

if (!$liveDataObject) {
// unlikely case as indexer calls 'shouldIndex' immediately prior to this
throw new IndexingServiceException('Only published DataObjects may be added to the index');
}

$this->setDataObject($liveDataObject);
});
}

if ($event === DocumentAddHandler::AFTER_ADD) {
$this->markIndexed();
}
Expand Down
104 changes: 101 additions & 3 deletions tests/DataObject/DataObjectDocumentTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,20 @@
use SilverStripe\SearchService\Interfaces\DocumentAddHandler;
use SilverStripe\SearchService\Interfaces\DocumentRemoveHandler;
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;
use SilverStripe\SearchService\Tests\Fake\DataObjectSubclassFake;
use SilverStripe\SearchService\Tests\Fake\ImageFake;
use SilverStripe\SearchService\Tests\Fake\PageFake;
use SilverStripe\SearchService\Tests\Fake\ServiceFake;
use SilverStripe\SearchService\Tests\Fake\TagFake;
use SilverStripe\SearchService\Tests\SearchServiceTest;
use SilverStripe\Security\Member;
use SilverStripe\Subsites\Model\Subsite;
use SilverStripe\Versioned\ReadingMode;
use SilverStripe\Versioned\Versioned;

class DataObjectDocumentTest extends SearchServiceTest
{
Expand Down Expand Up @@ -623,21 +627,58 @@ public function testExtensionRequired(): void
$this->assertEquals($fake, $doc->getDataObject());
}

public function testEvents(): void
public function testMarkIndexedOnEvents(): void
{
$dataObject = $this->objFromFixture(DataObjectFakeVersioned::class, 'one');
$mock = $this->getMockBuilder(DataObjectDocument::class)
->onlyMethods(['markIndexed'])
->onlyMethods(['markIndexed', 'getDataObject'])
->disableOriginalConstructor()
->getMock();
$mock->expects($this->exactly(2))
->method('markIndexed');

$mock->onAddToSearchIndexes(DocumentAddHandler::BEFORE_ADD);
$mock->method('getDataObject')
->willReturn($dataObject);

$mock->onAddToSearchIndexes(DocumentAddHandler::AFTER_ADD);
// currenlty BEFORE_REMOVE is a noop
$mock->onRemoveFromSearchIndexes(DocumentRemoveHandler::BEFORE_REMOVE);
$mock->onRemoveFromSearchIndexes(DocumentRemoveHandler::AFTER_REMOVE);
}

public function testOnAddToSearchIndexes(): void
{
Versioned::withVersionedMode(function (): void {
// set DRAFT to match state of queue runners which generally
// run through DevelopmentAdmin controller
Versioned::set_stage(Versioned::DRAFT);

$dataObject = $this->objFromFixture(DataObjectFakeVersioned::class, 'one');
$dataObject->publishRecursive();
$document = DataObjectDocument::create($dataObject);

$queryParams = $document->getDataObject()->getSourceQueryParams();
$readingMode = ReadingMode::fromDataQueryParams($queryParams);

$this->assertEquals('Stage.' . Versioned::DRAFT, $readingMode);

$document->onAddToSearchIndexes(DocumentAddHandler::BEFORE_ADD);

$queryParams = $document->getDataObject()->getSourceQueryParams();
$readingMode = ReadingMode::fromDataQueryParams($queryParams);

$this->assertEquals('Stage.' . Versioned::LIVE, $readingMode);

$dataObject->doArchive();

$this->expectExceptionMessage(
sprintf('Only published DataObjects may be added to the index')
);

$document->onAddToSearchIndexes(DocumentAddHandler::BEFORE_ADD);
});
}

public function testDeletedDataObject(): void
{
$dataObject = $this->objFromFixture(DataObjectFakeVersioned::class, 'one');
Expand All @@ -660,4 +701,61 @@ public function testDeletedDataObject(): void
unserialize(serialize($doc));
}

public function testIndexDataObjectDocument(): void
{
$config = $this->mockConfig();
$config->set('getSearchableClasses', [
PageFake::class,
]);
$config->set('getFieldsForClass', [
PageFake::class => [
new Field('title'),
new Field('page_link', 'Link'),
],
]);

Versioned::withVersionedMode(function () use ($config): void {
// Reading mode as if run by job - development Admin sets Draft reading mode
// usually via /dev/tasks/ProcessJobQueueTask
// @see SilverStripe\Dev\DevelopmentAdmin
Versioned::set_stage(Versioned::DRAFT);

$dataObject = PageFake::create();
$dataObject->Title = 'Draft';
$dataObject->write();

$doc = DataObjectDocument::create($dataObject);

$config->set(
'getIndexesForDocument',
[
$doc->getIdentifier() => [
'index' => 'data',
],
]
);

$indexer = new Indexer([$doc]);
$indexer->setIndexService($service = new ServiceFake());
$indexer->setBatchSize(1);
$indexer->processNode();

$this->assertCount(0, $service->documents, 'Draft documents should not be indexed');

$dataObject->Title = 'Published';
$dataObject->write();
$published = $dataObject->publishRecursive();
$this->assertTrue($published);
$dataObject->flushCache();
$doc = DataObjectDocument::create($dataObject);

$indexer = new Indexer([$doc]);
$indexer->setIndexService($service = new ServiceFake());
$indexer->setBatchSize(1);
$indexer->processNode();

$this->assertCount(1, $service->documents, 'Published documents should be indexed');
});
}

}
Loading