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 <[email protected]>
Gerrit-Reviewer: Matthias Mullie <[email protected]>
Gerrit-Reviewer: SG <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
