diff --git a/lib/Cron/JanitorCron.php b/lib/Cron/JanitorCron.php index 35f323870e..4b4f26a959 100644 --- a/lib/Cron/JanitorCron.php +++ b/lib/Cron/JanitorCron.php @@ -81,7 +81,15 @@ protected function run($argument) { $deleted['shares'] = $this->shareMapper->purgeDeletedShares($purgeBefore); // purge orphaned votes; Votes without any corresponding option - $deleted['orphaned votes'] = $this->voteMapper->removeOrphanedVotes(); + // Skipped while hashes are known to be out of sync, otherwise votes of an + // option whose hash could not be written would be deleted as orphaned + if ($this->tableManager->getFailedHashUpdates() === 0) { + $deleted['orphaned votes'] = $this->voteMapper->removeOrphanedVotes(); + } else { + $this->logger->warning('JanitorCron: Skipped removing orphaned votes, {count} hash update(s) failed', [ + 'count' => $this->tableManager->getFailedHashUpdates(), + ]); + } // delete polls after defined days after archiving date $autoDeleteOffset = $this->appSettings->getAutoDeleteOffsetDays(); diff --git a/lib/Db/Option.php b/lib/Db/Option.php index b7e45a2061..b992d8c3e2 100644 --- a/lib/Db/Option.php +++ b/lib/Db/Option.php @@ -74,7 +74,7 @@ class Option extends EntityWithUser implements JsonSerializable { public $id = null; protected int $pollId = 0; protected string $pollOptionText = ''; - protected string $pollOptionHash = ''; + protected ?string $pollOptionHash = ''; protected ?string $description = ''; protected int $timestamp = 0; protected int $duration = 0; @@ -218,7 +218,17 @@ public function getPollOptionHash(): string { } public function getPollOptionHashInDB(): string { - return $this->pollOptionHash; + // Oracle stores an empty string as null + return (string)$this->pollOptionHash; + } + + /** + * Get the raw timestamp column, opposed to getTimestamp(), which derives it from + * the iso timestamp. Needed where the stored value matters, i.e. the unique index + * over poll_id, poll_option_hash and timestamp + */ + public function getTimestampInDB(): int { + return $this->timestamp; } /** * Get the order of the option. If the option has a valid timestamp, diff --git a/lib/Db/V11/TableManager.php b/lib/Db/V11/TableManager.php index 243b3609ef..c0288072de 100644 --- a/lib/Db/V11/TableManager.php +++ b/lib/Db/V11/TableManager.php @@ -11,11 +11,13 @@ use Doctrine\DBAL\Types\Type; use Exception; use OCA\Polls\AppInfo\Application; +use OCA\Polls\Db\Option; use OCA\Polls\Db\OptionMapper; use OCA\Polls\Db\Poll; use OCA\Polls\Db\PollGroup; use OCA\Polls\Db\PollMapper; use OCA\Polls\Db\Share; +use OCA\Polls\Db\Vote; use OCA\Polls\Db\VoteMapper; use OCA\Polls\Db\Watch; use OCA\Polls\Exceptions\PreconditionException; @@ -29,6 +31,7 @@ use Psr\Log\LoggerInterface; class TableManager extends DbManager { + private int $failedHashUpdates = 0; /** @psalm-suppress PossiblyUnusedMethod */ public function __construct( @@ -679,11 +682,143 @@ public function updateHashes(): array { $this->checkPrecondition(OptionMapper::TABLE, ['poll_id', 'poll_option_text', 'poll_option_hash']); $this->checkPrecondition(VoteMapper::TABLE, ['poll_id', 'vote_option_text', 'vote_option_hash']); + $this->failedHashUpdates = 0; + $messages = $this->updateOptionHashes(); $messages = array_merge($messages, $this->updateVoteHashes()); return $messages; } + /** + * Number of rows whose hash could not be written by the last updateHashes() run. + * While this is greater than zero, option and vote hashes are known to be out of + * sync and no caller may delete records based on hash comparison. + */ + public function getFailedHashUpdates(): int { + return $this->failedHashUpdates; + } + + /** + * Split off options which would collide on their recalculated hash + * + * Two rows can hold different stored hashes and still resolve to the same hash, + * i.e. after an option text or duration change that never got the hash rewritten. + * They describe the same option, but the unique index over poll_id, + * poll_option_hash and timestamp rejects the second hash update, which would keep + * the row out of sync with its votes for good. The redundant rows are removed + * instead and updateVoteHashes() moves their votes over to the surviving option. + * + * @param Option[] $options + * @return array{0: Option[], 1: string[]} Remaining options and messages + */ + private function dropDuplicateOptions(array $options): array { + $messages = []; + $grouped = []; + + foreach ($options as $option) { + $key = implode("\0", [ + $option->getPollId(), + Hash::getOptionHash($option->getPollId(), $option->getPollOptionText()), + $option->getTimestampInDB(), + ]); + $grouped[$key][] = $option; + } + + $remaining = []; + + foreach ($grouped as $duplicates) { + // keep a live option over a deleted one and a confirmed one over an + // unconfirmed one, otherwise the lowest id + usort($duplicates, static fn (Option $a, Option $b): int + => [$a->getDeleted() !== 0, $a->getConfirmed() === 0, (int)$a->getId()] + <=> [$b->getDeleted() !== 0, $b->getConfirmed() === 0, (int)$b->getId()]); + + $survivor = array_shift($duplicates); + $remaining[] = $survivor; + + foreach ($duplicates as $duplicate) { + try { + $this->optionMapper->delete($duplicate); + } catch (Exception $e) { + $this->failedHashUpdates++; + $messages[] = 'Skip duplicate removal - Error removing optionId ' . $duplicate->getId(); + $this->logger->error('Error removing duplicate optionId {id}', [ + 'id' => $duplicate->getId(), + 'message' => $e->getMessage() + ]); + continue; + } + + $messages[] = 'Removed duplicate optionId ' . $duplicate->getId() + . ' superseded by optionId ' . $survivor->getId(); + $this->logger->warning('Removed duplicate optionId {id} in poll {pollId}, superseded by optionId {survivorId}', [ + 'id' => $duplicate->getId(), + 'pollId' => $duplicate->getPollId(), + 'survivorId' => $survivor->getId(), + ]); + } + } + + return [$remaining, $messages]; + } + + /** + * Split off votes which would collide on their recalculated hash + * + * Counterpart of dropDuplicateOptions() for the unique index over poll_id, + * user_id and vote_option_hash + * + * @param Vote[] $votes + * @return array{0: Vote[], 1: string[]} Remaining votes and messages + */ + private function dropDuplicateVotes(array $votes): array { + $messages = []; + $grouped = []; + + foreach ($votes as $vote) { + $key = implode("\0", [ + $vote->getPollId(), + $vote->getUserId(), + Hash::getOptionHash($vote->getPollId(), $vote->getVoteOptionText()), + ]); + $grouped[$key][] = $vote; + } + + $remaining = []; + + foreach ($grouped as $duplicates) { + usort($duplicates, static fn (Vote $a, Vote $b): int + => [$a->getDeleted() !== 0, (int)$a->getId()] <=> [$b->getDeleted() !== 0, (int)$b->getId()]); + + $survivor = array_shift($duplicates); + $remaining[] = $survivor; + + foreach ($duplicates as $duplicate) { + try { + $this->voteMapper->delete($duplicate); + } catch (Exception $e) { + $this->failedHashUpdates++; + $messages[] = 'Skip duplicate removal - Error removing voteId ' . $duplicate->getId(); + $this->logger->error('Error removing duplicate voteId {id}', [ + 'id' => $duplicate->getId(), + 'message' => $e->getMessage() + ]); + continue; + } + + $messages[] = 'Removed duplicate voteId ' . $duplicate->getId() + . ' superseded by voteId ' . $survivor->getId(); + $this->logger->warning('Removed duplicate voteId {id} in poll {pollId}, superseded by voteId {survivorId}', [ + 'id' => $duplicate->getId(), + 'pollId' => $duplicate->getPollId(), + 'survivorId' => $survivor->getId(), + ]); + } + } + + return [$remaining, $messages]; + } + /** * Update all vote hashes * Precondition have to be checked before @@ -691,15 +826,15 @@ public function updateHashes(): array { * @return string[] Messages as array */ private function updateVoteHashes(): array { - $messages = []; - $tableName = VoteMapper::TABLE; $prefixedTableName = $this->dbPrefix . $tableName; $count = 0; $updated = 0; - foreach ($this->voteMapper->getAll(includeNull: true) as $vote) { + [$votes, $messages] = $this->dropDuplicateVotes($this->voteMapper->getAll(includeNull: true)); + + foreach ($votes as $vote) { try { // if the hash of the vote differs from calculated hash update the vote hash if ($vote->getVoteOptionHashInDB() !== Hash::getOptionHash($vote->getPollId(), $vote->getVoteOptionText())) { @@ -708,8 +843,7 @@ private function updateVoteHashes(): array { . ':' . Hash::getOptionHash($vote->getPollId(), $vote->getVoteOptionText()) . '\'' . $vote->getVoteOptionText() . '\''; - $vote->setVoteOptionHash(Hash::getOptionHash($vote->getPollId(), $vote->getVoteOptionText())); - $vote = $this->voteMapper->update($vote); + $this->voteMapper->updateHash($vote); $updated++; } @@ -717,6 +851,7 @@ private function updateVoteHashes(): array { $count++; } catch (Exception $e) { + $this->failedHashUpdates++; $messages[] = 'Skip hash update - Error updating option hash for voteId ' . $vote->getId(); $this->logger->error('Error updating option hash for voteId {id}', [ 'id' => $vote->getId(), @@ -752,15 +887,15 @@ private function updateVoteHashes(): array { * @return string[] Messages as array */ private function updateOptionHashes(): array { - $messages = []; - $tableName = OptionMapper::TABLE; $prefixedTableName = $this->dbPrefix . $tableName; $count = 0; $updated = 0; - foreach ($this->optionMapper->getAll(includeNull: true) as $option) { + [$options, $messages] = $this->dropDuplicateOptions($this->optionMapper->getAll(includeNull: true)); + + foreach ($options as $option) { try { // if the option's hash differs from $actualHash update the option if ($option->getPollOptionHashInDB() !== Hash::getOptionHash($option->getPollId(), $option->getPollOptionText())) { @@ -779,6 +914,7 @@ private function updateOptionHashes(): array { $count++; } catch (Exception $e) { + $this->failedHashUpdates++; $messages[] = 'Skip hash update - Error updating option hash for optionId ' . $option->getId(); $this->logger->error('Error updating option hash for optionId {id}', ['id' => $option->getId(), 'message' => $e->getMessage()]); } diff --git a/lib/Db/Vote.php b/lib/Db/Vote.php index 494b357297..abe7c830fa 100644 --- a/lib/Db/Vote.php +++ b/lib/Db/Vote.php @@ -46,7 +46,7 @@ class Vote extends EntityWithUser implements JsonSerializable { protected string $userId = ''; protected int $voteOptionId = 0; protected string $voteOptionText = ''; - protected string $voteOptionHash = ''; + protected ?string $voteOptionHash = ''; protected string $voteAnswer = ''; protected int $deleted = 0; @@ -99,7 +99,8 @@ public function getVoteOptionHash(): string { * @psalm-suppress PossiblyUnusedMethod */ public function getVoteOptionHashInDb(): string { - return $this->voteOptionHash; + // Oracle stores an empty string as null + return (string)$this->voteOptionHash; } /** diff --git a/lib/Db/VoteMapper.php b/lib/Db/VoteMapper.php index 3b3ca7b26e..3f136e6b5e 100644 --- a/lib/Db/VoteMapper.php +++ b/lib/Db/VoteMapper.php @@ -44,6 +44,15 @@ public function insert(Entity $entity): Vote { return $this->find($entity->getId()); } + /** + * Write the vote's hash without reloading the entity, for the maintenance jobs, + * which have no use for the joined attributes + */ + public function updateHash(Vote $vote): void { + $vote->setVoteOptionHash(Hash::getOptionHash($vote->getPollId(), $vote->getVoteOptionText())); + parent::update($vote); + } + /** * @throws \OCP\AppFramework\Db\DoesNotExistException if not found * @return Vote[] diff --git a/tests/Unit/Db/TableManagerTest.php b/tests/Unit/Db/TableManagerTest.php new file mode 100644 index 0000000000..df51714674 --- /dev/null +++ b/tests/Unit/Db/TableManagerTest.php @@ -0,0 +1,312 @@ +connection = Server::get(IDBConnection::class); + $this->pollMapper = Server::get(PollMapper::class); + $this->tableManager = Server::get(TableManager::class); + + /** @var Poll $poll */ + $poll = $this->fm->instance('OCA\Polls\Db\Poll'); + $this->poll = $this->pollMapper->insert($poll); + } + + /** + * A stale hash on an option whose sibling already holds the recalculated hash must + * not stop the hash update. The sibling is deleted here, so the live option has to + * survive together with its vote. + */ + public function testUpdateHashesResolvesCollisionWithDeletedOption(): void { + $text = 'collision ' . bin2hex(random_bytes(8)); + $hash = Hash::getOptionHash($this->poll->getId(), $text); + + $this->insertOptionRow($text, $hash, ['deleted' => time() - 86400]); + $liveId = $this->insertOptionRow($text, self::STALE_HASH); + $this->insertVoteRow('voter', $text, self::STALE_HASH); + + $this->tableManager->updateHashes(); + + $options = $this->fetchOptionRows(); + $this->assertCount(1, $options, 'the duplicate option should be gone'); + $this->assertSame($liveId, (int)$options[0]['id'], 'the live option should have survived'); + $this->assertSame($hash, $options[0]['poll_option_hash']); + + $this->assertSame(0, $this->countOrphanedVotes(), 'the vote should still belong to an option'); + $this->assertSame(0, $this->tableManager->getFailedHashUpdates()); + } + + /** + * Same collision between two live options. Both votes belong to the same voter and + * collide as well, as soon as the two options are merged into one. + */ + public function testUpdateHashesResolvesCollisionWithLiveOption(): void { + $text = 'collision ' . bin2hex(random_bytes(8)); + $hash = Hash::getOptionHash($this->poll->getId(), $text); + + $survivorId = $this->insertOptionRow($text, $hash); + $this->insertOptionRow($text, self::STALE_HASH); + $this->insertVoteRow('voter', $text, $hash); + $this->insertVoteRow('voter', $text, self::STALE_HASH); + + $this->tableManager->updateHashes(); + + $options = $this->fetchOptionRows(); + $this->assertCount(1, $options, 'the duplicate option should be gone'); + $this->assertSame($survivorId, (int)$options[0]['id']); + $this->assertSame($hash, $options[0]['poll_option_hash']); + + $votes = $this->fetchVoteRows(); + $this->assertCount(1, $votes, 'the duplicate vote should be gone'); + $this->assertSame($hash, $votes[0]['vote_option_hash']); + + $this->assertSame(0, $this->countOrphanedVotes()); + $this->assertSame(0, $this->tableManager->getFailedHashUpdates()); + } + + /** + * The surviving option is the one with the lowest id, so it can be the one still + * carrying the stale hash and needing the update after its twin is removed + */ + public function testUpdateHashesResolvesCollisionOnTheSurvivingOption(): void { + $text = 'collision ' . bin2hex(random_bytes(8)); + $hash = Hash::getOptionHash($this->poll->getId(), $text); + + $survivorId = $this->insertOptionRow($text, self::STALE_HASH); + $this->insertOptionRow($text, $hash); + $this->insertVoteRow('voter', $text, $hash); + + $this->tableManager->updateHashes(); + + $options = $this->fetchOptionRows(); + $this->assertCount(1, $options, 'the duplicate option should be gone'); + $this->assertSame($survivorId, (int)$options[0]['id']); + $this->assertSame($hash, $options[0]['poll_option_hash']); + + $this->assertCount(1, $this->fetchVoteRows()); + $this->assertSame(0, $this->countOrphanedVotes(), 'the vote should still belong to an option'); + $this->assertSame(0, $this->tableManager->getFailedHashUpdates()); + } + + /** + * A confirmed option carries the poll's result, so it has to win over its + * duplicate even when the duplicate has the lower id + */ + public function testUpdateHashesKeepsTheConfirmedOption(): void { + $text = 'collision ' . bin2hex(random_bytes(8)); + $hash = Hash::getOptionHash($this->poll->getId(), $text); + + $this->insertOptionRow($text, $hash); + $confirmedId = $this->insertOptionRow($text, self::STALE_HASH, ['confirmed' => time()]); + + $this->tableManager->updateHashes(); + + $options = $this->fetchOptionRows(); + $this->assertCount(1, $options); + $this->assertSame($confirmedId, (int)$options[0]['id'], 'the confirmed option should have survived'); + $this->assertSame($hash, $options[0]['poll_option_hash']); + $this->assertSame(0, $this->tableManager->getFailedHashUpdates()); + } + + /** + * Date options sharing an iso timestamp resolve to the same hash, but the unique + * index keys on the raw timestamp column. Rows whose raw value drifted apart do + * not collide and both have to be kept. + */ + public function testUpdateHashesKeepsDateOptionsDifferingInTheStoredTimestamp(): void { + $iso = '2025-06-01T10:00:00+00:00'; + $timestamp = (new \DateTimeImmutable($iso))->getTimestamp(); + $hash = Hash::getOptionHash($this->poll->getId(), $iso); + + $firstId = $this->insertOptionRow($iso, self::STALE_HASH, ['timestamp' => $timestamp, 'iso_timestamp' => $iso]); + $secondId = $this->insertOptionRow($iso, self::STALE_HASH, ['timestamp' => $timestamp + 1, 'iso_timestamp' => $iso]); + + $this->tableManager->updateHashes(); + + $options = $this->fetchOptionRows(); + $this->assertCount(2, $options, 'both date options should have been kept'); + $this->assertSame([$firstId, $secondId], array_map(static fn (array $row): int => (int)$row['id'], $options)); + $this->assertSame($hash, $options[0]['poll_option_hash']); + $this->assertSame($hash, $options[1]['poll_option_hash']); + $this->assertSame(0, $this->tableManager->getFailedHashUpdates()); + } + + /** + * Oracle reads an empty hash back as null, which must not break the entity + */ + public function testUpdateHashesWritesTheEmptyDefaultHash(): void { + $text = 'empty hash ' . bin2hex(random_bytes(8)); + + $optionId = $this->insertOptionRow($text, ''); + $this->insertVoteRow('voter', $text, ''); + + $this->tableManager->updateHashes(); + + $options = $this->fetchOptionRows(); + $this->assertCount(1, $options); + $this->assertSame($optionId, (int)$options[0]['id']); + $this->assertSame(Hash::getOptionHash($this->poll->getId(), $text), $options[0]['poll_option_hash']); + + $this->assertSame(0, $this->countOrphanedVotes(), 'the vote should still belong to an option'); + $this->assertSame(0, $this->tableManager->getFailedHashUpdates()); + } + + /** + * Options which do not collide keep their row and only get their hash corrected + */ + public function testUpdateHashesKeepsDistinctOptions(): void { + $first = 'distinct ' . bin2hex(random_bytes(8)); + $second = 'distinct ' . bin2hex(random_bytes(8)); + + $firstId = $this->insertOptionRow($first, self::STALE_HASH); + $secondId = $this->insertOptionRow($second, Hash::getOptionHash($this->poll->getId(), $second)); + + $this->tableManager->updateHashes(); + + $options = $this->fetchOptionRows(); + $this->assertCount(2, $options); + $this->assertSame([$firstId, $secondId], array_map(static fn (array $row): int => (int)$row['id'], $options)); + $this->assertSame(Hash::getOptionHash($this->poll->getId(), $first), $options[0]['poll_option_hash']); + $this->assertSame(Hash::getOptionHash($this->poll->getId(), $second), $options[1]['poll_option_hash']); + $this->assertSame(0, $this->tableManager->getFailedHashUpdates()); + } + + /** + * tearDown + */ + public function tearDown(): void { + parent::tearDown(); + + foreach ([OptionMapper::TABLE, VoteMapper::TABLE] as $table) { + $qb = $this->connection->getQueryBuilder(); + $qb->delete($table) + ->where($qb->expr()->eq('poll_id', $qb->createNamedParameter($this->poll->getId(), IQueryBuilder::PARAM_INT))); + $qb->executeStatement(); + } + + $this->pollMapper->delete($this->poll); + } + + /** + * Insert an option bypassing the mapper, which would recalculate the hash + */ + private function insertOptionRow(string $text, string $hash, array $columns = []): int { + $qb = $this->connection->getQueryBuilder(); + $columns = array_merge([ + 'timestamp' => 0, + 'deleted' => 0, + 'confirmed' => 0, + 'iso_timestamp' => null, + ], $columns); + + $qb->insert(OptionMapper::TABLE) + ->values([ + 'poll_id' => $qb->createNamedParameter($this->poll->getId(), IQueryBuilder::PARAM_INT), + 'poll_option_text' => $qb->createNamedParameter($text, IQueryBuilder::PARAM_STR), + 'poll_option_hash' => $qb->createNamedParameter($hash, IQueryBuilder::PARAM_STR), + 'timestamp' => $qb->createNamedParameter($columns['timestamp'], IQueryBuilder::PARAM_INT), + 'deleted' => $qb->createNamedParameter($columns['deleted'], IQueryBuilder::PARAM_INT), + 'confirmed' => $qb->createNamedParameter($columns['confirmed'], IQueryBuilder::PARAM_INT), + 'iso_timestamp' => $qb->createNamedParameter($columns['iso_timestamp'], IQueryBuilder::PARAM_STR), + 'owner' => $qb->createNamedParameter('tester', IQueryBuilder::PARAM_STR), + ]); + $qb->executeStatement(); + + return $qb->getLastInsertId(); + } + + /** + * Insert a vote bypassing the mapper, which would recalculate the hash + */ + private function insertVoteRow(string $userId, string $text, string $hash): int { + $qb = $this->connection->getQueryBuilder(); + $qb->insert(VoteMapper::TABLE) + ->values([ + 'poll_id' => $qb->createNamedParameter($this->poll->getId(), IQueryBuilder::PARAM_INT), + 'user_id' => $qb->createNamedParameter($userId, IQueryBuilder::PARAM_STR), + 'vote_option_text' => $qb->createNamedParameter($text, IQueryBuilder::PARAM_STR), + 'vote_option_hash' => $qb->createNamedParameter($hash, IQueryBuilder::PARAM_STR), + 'vote_answer' => $qb->createNamedParameter('yes', IQueryBuilder::PARAM_STR), + ]); + $qb->executeStatement(); + + return $qb->getLastInsertId(); + } + + /** + * @return list> + */ + private function fetchOptionRows(): array { + $qb = $this->connection->getQueryBuilder(); + $qb->select('id', 'poll_option_text', 'poll_option_hash', 'deleted') + ->from(OptionMapper::TABLE) + ->where($qb->expr()->eq('poll_id', $qb->createNamedParameter($this->poll->getId(), IQueryBuilder::PARAM_INT))) + ->orderBy('id'); + + return $qb->executeQuery()->fetchAll(); + } + + /** + * @return list> + */ + private function fetchVoteRows(): array { + $qb = $this->connection->getQueryBuilder(); + $qb->select('id', 'vote_option_hash') + ->from(VoteMapper::TABLE) + ->where($qb->expr()->eq('poll_id', $qb->createNamedParameter($this->poll->getId(), IQueryBuilder::PARAM_INT))) + ->orderBy('id'); + + return $qb->executeQuery()->fetchAll(); + } + + /** + * Votes of this poll which VoteMapper::removeOrphanedVotes() would delete + */ + private function countOrphanedVotes(): int { + $qb = $this->connection->getQueryBuilder(); + $qb->select('votes.id') + ->from(VoteMapper::TABLE, 'votes') + ->leftJoin( + 'votes', + OptionMapper::TABLE, + 'options', + $qb->expr()->andX( + $qb->expr()->eq('votes.poll_id', 'options.poll_id'), + $qb->expr()->eq('votes.vote_option_hash', 'options.poll_option_hash'), + ) + ) + ->where($qb->expr()->eq('votes.poll_id', $qb->createNamedParameter($this->poll->getId(), IQueryBuilder::PARAM_INT))) + ->andWhere($qb->expr()->isNull('options.poll_id')); + + return count($qb->executeQuery()->fetchAll()); + } +}