From c77d8705ca68404d28a91094abd443514958a852 Mon Sep 17 00:00:00 2001 From: Amir Sarabadani Date: Tue, 4 Dec 2018 19:49:31 +0100 Subject: [PATCH] Stop updating tag_summary table This table will be dropped in favor of change_tag/change_tag_def Bug: T209525 Change-Id: I9e305dc0f71a126cff57cade37180ad438838030 --- includes/changetags/ChangeTags.php | 118 +++++------------- .../includes/changetags/ChangeTagsTest.php | 96 ++++++++++++++ 2 files changed, 125 insertions(+), 89 deletions(-) diff --git a/includes/changetags/ChangeTags.php b/includes/changetags/ChangeTags.php index bb5122f3ef..32cfd13f58 100644 --- a/includes/changetags/ChangeTags.php +++ b/includes/changetags/ChangeTags.php @@ -332,12 +332,19 @@ class ChangeTags { ); } - // update the tag_summary row - $prevTags = []; - if ( !self::updateTagSummaryRow( $tagsToAdd, $tagsToRemove, $rc_id, $rev_id, - $log_id, $prevTags ) - ) { - // nothing to do + $prevTags = self::getPrevTags( $rc_id, $log_id, $rev_id ); + + // add tags + $tagsToAdd = array_values( array_diff( $tagsToAdd, $prevTags ) ); + $newTags = array_unique( array_merge( $prevTags, $tagsToAdd ) ); + + // remove tags + $tagsToRemove = array_values( array_intersect( $tagsToRemove, $newTags ) ); + $newTags = array_values( array_diff( $newTags, $tagsToRemove ) ); + + sort( $prevTags ); + sort( $newTags ); + if ( $prevTags == $newTags ) { return [ [], [], $prevTags ]; } @@ -421,75 +428,24 @@ class ChangeTags { return [ $tagsToAdd, $tagsToRemove, $prevTags ]; } - /** - * Adds or removes a given set of tags to/from the relevant row of the - * tag_summary table. Modifies the tagsToAdd and tagsToRemove arrays to - * reflect the tags that were actually added and/or removed. - * - * @param array &$tagsToAdd - * @param array &$tagsToRemove If a tag is present in both $tagsToAdd and - * $tagsToRemove, it will be removed - * @param int|null $rc_id Null if not known or not applicable - * @param int|null $rev_id Null if not known or not applicable - * @param int|null $log_id Null if not known or not applicable - * @param array &$prevTags Optionally outputs a list of the tags that were - * in the tag_summary row to begin with - * @return bool True if any modifications were made, otherwise false - * @since 1.25 - */ - protected static function updateTagSummaryRow( &$tagsToAdd, &$tagsToRemove, - $rc_id, $rev_id, $log_id, &$prevTags = [] - ) { - $dbw = wfGetDB( DB_MASTER ); - - $tsConds = array_filter( [ - 'ts_rc_id' => $rc_id, - 'ts_rev_id' => $rev_id, - 'ts_log_id' => $log_id - ] ); - - // Can't both add and remove a tag at the same time... - $tagsToAdd = array_diff( $tagsToAdd, $tagsToRemove ); - - // Update the summary row. - // $prevTags can be out of date on replica DBs, especially when addTags is called consecutively, - // causing loss of tags added recently in tag_summary table. - $prevTags = $dbw->selectField( 'tag_summary', 'ts_tags', $tsConds, __METHOD__ ); - $prevTags = $prevTags ?: ''; - $prevTags = array_filter( explode( ',', $prevTags ) ); - - // add tags - $tagsToAdd = array_values( array_diff( $tagsToAdd, $prevTags ) ); - $newTags = array_unique( array_merge( $prevTags, $tagsToAdd ) ); - - // remove tags - $tagsToRemove = array_values( array_intersect( $tagsToRemove, $newTags ) ); - $newTags = array_values( array_diff( $newTags, $tagsToRemove ) ); - - sort( $prevTags ); - sort( $newTags ); - if ( $prevTags == $newTags ) { - return false; - } + private static function getPrevTags( $rc_id = null, $rev_id = null, $log_id = null ) { + $conds = array_filter( + [ + 'ct_rc_id' => $rc_id, + 'ct_log_id' => $log_id, + 'ct_rev_id' => $rev_id, + ] + ); - if ( !$newTags ) { - // No tags left, so delete the row altogether - $dbw->delete( 'tag_summary', $tsConds, __METHOD__ ); - } else { - // Specify the non-DEFAULT value columns in the INSERT/REPLACE clause - $row = array_filter( [ 'ts_tags' => implode( ',', $newTags ) ] + $tsConds ); - // Check the unique keys for conflicts, ignoring any NULL *_id values - $uniqueKeys = []; - foreach ( [ 'ts_rev_id', 'ts_rc_id', 'ts_log_id' ] as $uniqueColumn ) { - if ( isset( $row[$uniqueColumn] ) ) { - $uniqueKeys[] = [ $uniqueColumn ]; - } - } + $dbw = wfGetDB( DB_MASTER ); + $tagIds = $dbw->selectFieldValues( 'change_tag', 'ct_tag_id', $conds, __METHOD__ ); - $dbw->replace( 'tag_summary', $uniqueKeys, $row, __METHOD__ ); + $tags = []; + foreach ( $tagIds as $tagId ) { + $tags[] = MediaWikiServices::getInstance()->getChangeTagDefStore()->getName( (int)$tagId ); } - return true; + return $tags; } /** @@ -938,7 +894,7 @@ class ChangeTags { } /** - * Set ctd_user_defined = 0 in change_tag_def. + * Update ctd_user_defined = 0 field in change_tag_def. * The tag may remain in use by extensions, and may still show up as 'defined' * if an extension is setting it from the ListDefinedTags hook. * @@ -1152,7 +1108,7 @@ class ChangeTags { return Status::newFatal( 'tags-create-no-name' ); } - // tags cannot contain commas (used as a delimiter in tag_summary table), + // tags cannot contain commas (used to be used as a delimiter in tag_summary table), // pipe (used as a delimiter between multiple tags in // SpecialRecentchanges and friends), or slashes (would break tag description messages in // MediaWiki namespace) @@ -1266,22 +1222,6 @@ class ChangeTags { // set ctd_user_defined = 0 self::undefineTag( $tag ); - $tagId = MediaWikiServices::getInstance()->getChangeTagDefStore()->getId( $tag ); - $conditions = [ 'ct_tag_id' => $tagId ]; - - // find out which revisions use this tag, so we can delete from tag_summary - $result = $dbw->select( 'change_tag', - [ 'ct_rc_id', 'ct_log_id', 'ct_rev_id' ], - $conditions, - __METHOD__ ); - foreach ( $result as $row ) { - // remove the tag from the relevant row of tag_summary - $tagsToAdd = []; - $tagsToRemove = [ $tag ]; - self::updateTagSummaryRow( $tagsToAdd, $tagsToRemove, $row->ct_rc_id, - $row->ct_rev_id, $row->ct_log_id ); - } - // delete from change_tag $tagId = MediaWikiServices::getInstance()->getChangeTagDefStore()->getId( $tag ); $dbw->delete( 'change_tag', [ 'ct_tag_id' => $tagId ], __METHOD__ ); diff --git a/tests/phpunit/includes/changetags/ChangeTagsTest.php b/tests/phpunit/includes/changetags/ChangeTagsTest.php index 8265af801a..f1e82ffc78 100644 --- a/tests/phpunit/includes/changetags/ChangeTagsTest.php +++ b/tests/phpunit/includes/changetags/ChangeTagsTest.php @@ -421,6 +421,102 @@ class ChangeTagsTest extends MediaWikiTestCase { $this->assertEquals( $expected2, iterator_to_array( $res2, false ) ); } + public function testUpdateTagsSkipDuplicates() { + // FIXME: fails under postgres + $this->markTestSkippedIfDbType( 'postgres' ); + + $dbw = wfGetDB( DB_MASTER ); + $dbw->delete( 'change_tag', '*' ); + $dbw->delete( 'change_tag_def', '*' ); + + $rcId = 123; + ChangeTags::updateTags( [ 'tag1', 'tag2' ], [], $rcId ); + ChangeTags::updateTags( [ 'tag2', 'tag3' ], [], $rcId ); + + $dbr = wfGetDB( DB_REPLICA ); + + $expected = [ + (object)[ + 'ctd_name' => 'tag1', + 'ctd_id' => 1, + 'ctd_count' => 1 + ], + (object)[ + 'ctd_name' => 'tag2', + 'ctd_id' => 2, + 'ctd_count' => 1 + ], + (object)[ + 'ctd_name' => 'tag3', + 'ctd_id' => 3, + 'ctd_count' => 1 + ], + ]; + $res = $dbr->select( 'change_tag_def', [ 'ctd_name', 'ctd_id', 'ctd_count' ], '' ); + $this->assertEquals( $expected, iterator_to_array( $res, false ) ); + + $expected2 = [ + (object)[ + 'ct_tag_id' => 1, + 'ct_rc_id' => 123 + ], + (object)[ + 'ct_tag_id' => 2, + 'ct_rc_id' => 123 + ], + (object)[ + 'ct_tag_id' => 3, + 'ct_rc_id' => 123 + ], + ]; + $res2 = $dbr->select( 'change_tag', [ 'ct_tag_id', 'ct_rc_id' ], '' ); + $this->assertEquals( $expected2, iterator_to_array( $res2, false ) ); + } + + public function testUpdateTagsDoNothingOnRepeatedCall() { + // FIXME: fails under postgres + $this->markTestSkippedIfDbType( 'postgres' ); + + $dbw = wfGetDB( DB_MASTER ); + $dbw->delete( 'change_tag', '*' ); + $dbw->delete( 'change_tag_def', '*' ); + + $rcId = 123; + ChangeTags::updateTags( [ 'tag1', 'tag2' ], [], $rcId ); + $res = ChangeTags::updateTags( [ 'tag2', 'tag1' ], [], $rcId ); + $this->assertEquals( [ [], [], [ 'tag1', 'tag2' ] ], $res ); + + $dbr = wfGetDB( DB_REPLICA ); + + $expected = [ + (object)[ + 'ctd_name' => 'tag1', + 'ctd_id' => 1, + 'ctd_count' => 1 + ], + (object)[ + 'ctd_name' => 'tag2', + 'ctd_id' => 2, + 'ctd_count' => 1 + ], + ]; + $res = $dbr->select( 'change_tag_def', [ 'ctd_name', 'ctd_id', 'ctd_count' ], '' ); + $this->assertEquals( $expected, iterator_to_array( $res, false ) ); + + $expected2 = [ + (object)[ + 'ct_tag_id' => 1, + 'ct_rc_id' => 123 + ], + (object)[ + 'ct_tag_id' => 2, + 'ct_rc_id' => 123 + ], + ]; + $res2 = $dbr->select( 'change_tag', [ 'ct_tag_id', 'ct_rc_id' ], '' ); + $this->assertEquals( $expected2, iterator_to_array( $res2, false ) ); + } + public function testDeleteTags() { $dbw = wfGetDB( DB_MASTER ); $dbw->delete( 'change_tag', '*' ); -- 2.20.1