Matthias Mullie has submitted this change and it was merged. Change subject: I103: Duplicated topics are returned ......................................................................
I103: Duplicated topics are returned Duplicated topics were being returned when a second query was run but the index did not properly slice the results due to looking for the offset-key options instead of offset-id. I don't think it was ever intended for this to be two options so all offset-key usages were replaced with offset-id and a test was written to verify it slices in the appropriate place. Change-Id: I2f8d69730b8a88e6270b7408922d4775accf7039 --- M autoload.php M includes/Data/Index/FeatureIndex.php M includes/Data/Pager/Pager.php A tests/phpunit/Data/Index/FeatureIndexTest.php 4 files changed, 81 insertions(+), 9 deletions(-) Approvals: Matthias Mullie: Verified; Looks good to me, approved diff --git a/autoload.php b/autoload.php index 836b397..c57e24c 100644 --- a/autoload.php +++ b/autoload.php @@ -227,6 +227,8 @@ $wgAutoloadClasses['Flow\\Tests\\Data\\CachingObjectManagerTest'] = __DIR__ . '/tests/phpunit/Data/CachingObjectMapperTest.php'; $wgAutoloadClasses['Flow\\Tests\\Data\\FlowNothingTest'] = __DIR__ . '/tests/phpunit/Data/NothingTest.php'; $wgAutoloadClasses['Flow\\Tests\\Data\\IndexTest'] = __DIR__ . '/tests/phpunit/Data/IndexTest.php'; +$wgAutoloadClasses['Flow\\Tests\\Data\\Index\\MockFeatureIndex'] = __DIR__ . '/tests/phpunit/Data/Index/FeatureIndexTest.php'; +$wgAutoloadClasses['Flow\\Tests\\Data\\Index\\UniqueFeatureIndexTests'] = __DIR__ . '/tests/phpunit/Data/Index/FeatureIndexTest.php'; $wgAutoloadClasses['Flow\\Tests\\Data\\ObjectLocatorTest'] = __DIR__ . '/tests/phpunit/Data/ObjectLocatorTest.php'; $wgAutoloadClasses['Flow\\Tests\\Data\\Pager\\PagerTest'] = __DIR__ . '/tests/phpunit/Data/Pager/PagerTest.php'; $wgAutoloadClasses['Flow\\Tests\\Data\\RecentChanges\\RecentChangesMock'] = __DIR__ . '/tests/phpunit/Data/RecentChanges/RecentChangesTest.php'; @@ -234,7 +236,6 @@ $wgAutoloadClasses['Flow\\Tests\\Data\\RevisionStorageTest'] = __DIR__ . '/tests/phpunit/Data/RevisionStorageTest.php'; $wgAutoloadClasses['Flow\\Tests\\Data\\UserNameBatchTest'] = __DIR__ . '/tests/phpunit/Data/UserNameBatchTest.php'; $wgAutoloadClasses['Flow\\Tests\\Data\\UserNameListenerTest'] = __DIR__ . '/tests/phpunit/Data/UserNameListenerTest.php'; -$wgAutoloadClasses['Flow\\Tests\\FlowActionsTest'] = __DIR__ . '/tests/phpunit/FlowActionsTest.php'; $wgAutoloadClasses['Flow\\Tests\\FlowTestCase'] = __DIR__ . '/tests/phpunit/FlowTestCase.php'; $wgAutoloadClasses['Flow\\Tests\\Formatter\\FormatterTest'] = __DIR__ . '/tests/phpunit/Formatter/FormatterTest.php'; $wgAutoloadClasses['Flow\\Tests\\HookTest'] = __DIR__ . '/tests/phpunit/HookTest.php'; diff --git a/includes/Data/Index/FeatureIndex.php b/includes/Data/Index/FeatureIndex.php index 15b1d53..14ab5f0 100644 --- a/includes/Data/Index/FeatureIndex.php +++ b/includes/Data/Index/FeatureIndex.php @@ -85,6 +85,14 @@ } /** + * @return string + */ + public function getOrder() { + $order = isset( $this->options['order'] ) ? $this->options['order'] : 'DESC'; + return strtoupper( $order ); + } + + /** * @param array $rows * @param array $options * @return array [offset, limit] @@ -92,14 +100,14 @@ protected function getOffsetLimit( $rows, $options ) { $limit = isset( $options['limit'] ) ? $options['limit'] : $this->getLimit(); - if ( !isset( $options['offset-key'] ) ) { + if ( !isset( $options['offset-id'] ) ) { $offset = isset( $options['offset'] ) ? $options['offset'] : 0; return array( $offset, $limit ); } - $offsetKey = $options['offset-key']; - if ( $offsetKey instanceof UUID ) { - $offsetKey = $offsetKey->getAlphadecimal(); + $offsetId = $options['offset-id']; + if ( $offsetId instanceof UUID ) { + $offsetId = $offsetId->getAlphadecimal(); } $dir = 'fwd'; @@ -110,7 +118,7 @@ $dir = 'rev'; } - $offset = $this->getOffsetFromKey( $rows, $offsetKey ); + $offset = $this->getOffsetFromKey( $rows, $offsetId ); if ( $dir === 'fwd' ) { $startPos = $offset + 1; @@ -143,8 +151,10 @@ protected function getOffsetFromKey( $rows, $offsetKey ) { $rowIndex = 0; foreach ( $rows as $row ) { + $nextInOrder = $this->getOrder() === 'DESC' ? -1 : 1; + $comparisonValue = $this->compareRowToOffset( $row, $offsetKey ); - if ( $comparisonValue <= 0 ) { + if ( $comparisonValue === 0 || $comparisonValue === $nextInOrder ) { return $rowIndex; } $rowIndex++; diff --git a/includes/Data/Pager/Pager.php b/includes/Data/Pager/Pager.php index 94dc3a2..66c9d13 100644 --- a/includes/Data/Pager/Pager.php +++ b/includes/Data/Pager/Pager.php @@ -95,7 +95,7 @@ // We need one item of leeway to determine if there are more items 'limit' => $numRequested, 'offset-dir' => $this->options['pager-dir'], - 'offset-key' => $this->options['pager-offset'], + 'offset-id' => $this->options['pager-offset'], 'offset-elastic' => true, ); $offset = $this->options['pager-offset']; @@ -173,7 +173,7 @@ ); } } elseif ( $this->options['pager-dir'] === 'rev' ) { - if ( count( $results ) == $this->options['pager-limit'] + 1 ) { + if ( count( $results ) > $this->options['pager-limit'] ) { // We got extra, another page exists $results = array_slice( $results, -$this->options['pager-limit'] ); $pagingLinks['rev'] = $this->makePagingLink( diff --git a/tests/phpunit/Data/Index/FeatureIndexTest.php b/tests/phpunit/Data/Index/FeatureIndexTest.php new file mode 100644 index 0000000..911dc16 --- /dev/null +++ b/tests/phpunit/Data/Index/FeatureIndexTest.php @@ -0,0 +1,61 @@ +<?php + +namespace Flow\Tests\Data\Index; + +use Flow\Data\Index\FeatureIndex; + +class UniqueFeatureIndexTests extends \MediaWikiTestCase { + + public function testOffsetIdReturnsCorrectPortionOfIndexedValues() { + global $wgFlowCacheVersion; + $cache = $this->getMockBuilder( 'Flow\Data\BufferedCache' ) + ->disableOriginalConstructor() + ->getMock(); + $storage = $this->getMockBuilder( 'Flow\Data\ObjectStorage' ) + ->disableOriginalConstructor() + ->getMock(); + + $dbId = FeatureIndex::cachedDbId(); + $cache->expects( $this->any() ) + ->method( 'getMulti' ) + ->will( $this->returnValue( array( + "$dbId:foo:5:$wgFlowCacheVersion" => array( + array( 'some_row' => 40 ), + array( 'some_row' => 41 ), + array( 'some_row' => 42 ), + array( 'some_row' => 43 ), + array( 'some_row' => 44 ), + ), + ) ) ); + $storage->expects( $this->never() ) + ->method( 'findMulti' ); + + $index = new MockFeatureIndex( $cache, $storage, 'foo', array( 'bar' ) ); + + $res = $index->find( + array( 'bar' => 5 ), + array( 'offset-id' => 42 ) + ); + + $this->assertEquals( + array( + array( 'some_row' => 43, 'bar' => 5 ), + array( 'some_row' => 44, 'bar' => 5 ), + ), + array_values( $res ), + 'Returns items with some_row > provided offset-id of 42' + ); + } +} + +class MockFeatureIndex extends FeatureIndex { + public function getLimit() { return 42; } + public function queryOptions() { return array(); } + public function limitIndexSize( array $values ) { return $values; } + public function addToIndex( array $indexed, array $row ) {} + public function removeFromIndex( array $indexed, array $row ) {} + + // not abstract, but override for convenience + public function getSort() { return array( 'some_row' ); } + public function getOrder() { return 'ASC'; } +} -- To view, visit https://gerrit.wikimedia.org/r/165380 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: I2f8d69730b8a88e6270b7408922d4775accf7039 Gerrit-PatchSet: 7 Gerrit-Project: mediawiki/extensions/Flow Gerrit-Branch: master Gerrit-Owner: EBernhardson <ebernhard...@wikimedia.org> Gerrit-Reviewer: Matthias Mullie <mmul...@wikimedia.org> Gerrit-Reviewer: SG <shah...@gmail.com> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits