Skip to content

Commit

Permalink
Merge pull request #9071 from nextcloud/bug/5200/mailbox-name-hash
Browse files Browse the repository at this point in the history
fix: allow syncing of mailboxes with a trailing space
  • Loading branch information
ChristophWurst authored Nov 22, 2023
2 parents 61bbb6c + 9783069 commit 64617d1
Show file tree
Hide file tree
Showing 7 changed files with 228 additions and 2 deletions.
2 changes: 1 addition & 1 deletion appinfo/info.xml
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ The rating depends on the installed text processing backend. See [the rating ove
Learn more about the Nextcloud Ethical AI Rating [in our blog](https://nextcloud.com/blog/nextcloud-ethical-ai-rating/).
]]></description>
<version>3.5.0-beta.3</version>
<version>3.5.0-beta.4</version>
<licence>agpl</licence>
<author>Christoph Wurst</author>
<author homepage="https://github.com/nextcloud/groupware">Nextcloud Groupware Team</author>
Expand Down
3 changes: 3 additions & 0 deletions lib/Db/Mailbox.php
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@
* @method void setMyAcls(string|null $acls)
* @method bool|null isShared()
* @method void setShared(bool $shared)
* @method string getNameHash()
* @method void setNameHash(string $nameHash)
*/
class Mailbox extends Entity implements JsonSerializable {
protected $name;
Expand All @@ -89,6 +91,7 @@ class Mailbox extends Entity implements JsonSerializable {
protected $syncInBackground;
protected $myAcls;
protected $shared;
protected $nameHash;

/**
* @var int
Expand Down
2 changes: 1 addition & 1 deletion lib/Db/MailboxMapper.php
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ public function find(Account $account, string $name): Mailbox {
->from($this->getTableName())
->where(
$qb->expr()->eq('account_id', $qb->createNamedParameter($account->getId())),
$qb->expr()->eq('name', $qb->createNamedParameter($name))
$qb->expr()->eq('name_hash', $qb->createNamedParameter(md5($name)))
);

try {
Expand Down
1 change: 1 addition & 0 deletions lib/IMAP/MailboxSync.php
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,7 @@ private function createMailboxFromFolder(Account $account, Folder $folder, ?Hord
$mailbox->setSpecialUse(json_encode($folder->getSpecialUse()));
$mailbox->setMyAcls($folder->getMyAcls());
$mailbox->setShared($this->isMailboxShared($namespaces, $mailbox));
$mailbox->setNameHash(md5($folder->getMailbox()));
return $this->mailboxMapper->insert($mailbox);
}

Expand Down
115 changes: 115 additions & 0 deletions lib/Migration/Version3500Date20231115182612.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
<?php

declare(strict_types=1);

/**
* @copyright Copyright (c) 2023 Daniel Kesselberg <[email protected]>
*
* @author Daniel Kesselberg <[email protected]>
*
* @license GNU AGPL version 3 or any later version
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as
* published by the Free Software Foundation, either version 3 of the
* License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

namespace OCA\Mail\Migration;

use Closure;
use OCP\DB\ISchemaWrapper;
use OCP\DB\Types;
use OCP\IDBConnection;
use OCP\Migration\IOutput;
use OCP\Migration\SimpleMigrationStep;

class Version3500Date20231115182612 extends SimpleMigrationStep {

public function __construct(
private IDBConnection $connection
) {
}

/**
* @param IOutput $output
* @param Closure(): ISchemaWrapper $schemaClosure
* @param array $options
* @return null|ISchemaWrapper
*/
public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper {
/** @var ISchemaWrapper $schema */
$schema = $schemaClosure();

$mailboxesTable = $schema->getTable('mail_mailboxes');
if (!$mailboxesTable->hasColumn('name_hash')) {
$mailboxesTable->addColumn('name_hash', Types::STRING);
}

return $schema;
}

/**
* @param IOutput $output
* @param Closure(): ISchemaWrapper $schemaClosure
* @param array $options
*/
public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options): void {
// Round 1: Hash common mailbox names

$this->connection->beginTransaction();

foreach (['INBOX', 'Drafts', 'Sent', 'Trash', 'Junk', 'Spam', 'Archive', 'Archives'] as $name) {
$qb = $this->connection->getQueryBuilder();
$qb->update('mail_mailboxes')
->set('name_hash', $qb->createNamedParameter(md5($name)))
->where($qb->expr()->like('name', $qb->createNamedParameter($name), Types::STRING))
->executeStatement();
}

$this->connection->commit();

// Round 2: Hash everything else

$qb = $this->connection->getQueryBuilder();
$qb->select(['id', 'name'])
->from('mail_mailboxes')
->where($qb->expr()->emptyString('name_hash'));
$mailboxes = $qb->executeQuery();

$updateQb = $this->connection->getQueryBuilder();
$updateQb->update('mail_mailboxes')
->set('name_hash', $updateQb->createParameter('name_hash'))
->where($updateQb->expr()->eq('id', $updateQb->createParameter('id')));

$this->connection->beginTransaction();

$queryCount = 0;
while (($row = $mailboxes->fetch()) !== false) {
$queryCount++;

$updateQb->setParameter('id', $row['id']);
$updateQb->setParameter('name_hash', md5($row['name']));
$updateQb->executeStatement();

if ($queryCount === 50000) {
$this->connection->commit();
$this->connection->beginTransaction();
$queryCount = 0;
}
}

$mailboxes->closeCursor();

$this->connection->commit();
}
}
61 changes: 61 additions & 0 deletions lib/Migration/Version3500Date20231115184458.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
<?php

declare(strict_types=1);

/**
* @copyright Copyright (c) 2023 Daniel Kesselberg <[email protected]>
*
* @author Daniel Kesselberg <[email protected]>
*
* @license GNU AGPL version 3 or any later version
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as
* published by the Free Software Foundation, either version 3 of the
* License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

namespace OCA\Mail\Migration;

use Closure;
use OCP\DB\ISchemaWrapper;
use OCP\Migration\IOutput;
use OCP\Migration\SimpleMigrationStep;

class Version3500Date20231115184458 extends SimpleMigrationStep {

/**
* @param IOutput $output
* @param Closure(): ISchemaWrapper $schemaClosure
* @param array $options
* @return null|ISchemaWrapper
*/
public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper {
/** @var ISchemaWrapper $schema */
$schema = $schemaClosure();

$mailboxesTable = $schema->getTable('mail_mailboxes');

$indexOld = 'UNIQ_22DEBD839B6B5FBA5E237E06';
$indexNew = 'mail_mb_account_id_name_hash';

if ($mailboxesTable->hasIndex($indexOld)) {
$mailboxesTable->dropIndex($indexOld);
}

if (!$mailboxesTable->hasIndex($indexNew)) {
$mailboxesTable->addUniqueIndex(['account_id', 'name_hash'], $indexNew);
}

return $schema;
}
}
46 changes: 46 additions & 0 deletions tests/Integration/Db/MailboxMapperTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ public function testFindAll() {
'messages' => $qb->createNamedParameter($i * 100, IQueryBuilder::PARAM_INT),
'unseen' => $qb->createNamedParameter($i, IQueryBuilder::PARAM_INT),
'selectable' => $qb->createNamedParameter(true, IQueryBuilder::PARAM_BOOL),
'name_hash' => $qb->createNamedParameter(md5("folder$i")),
]);
$insert->executeStatement();
}
Expand Down Expand Up @@ -122,11 +123,56 @@ public function testFindInbox() {
'messages' => $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT),
'unseen' => $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT),
'selectable' => $qb->createNamedParameter(true, IQueryBuilder::PARAM_BOOL),
'name_hash' => $qb->createNamedParameter(md5('INBOX')),
]);
$insert->executeStatement();

$result = $this->mapper->find($account, 'INBOX');

$this->assertSame('INBOX', $result->getName());
}

public function testMailboxesWithTrailingSpace() {
/** @var Account|MockObject $account */
$account = $this->createMock(Account::class);
$account->method('getId')->willReturn(13);

$qb = $this->db->getQueryBuilder();
$insert = $qb->insert($this->mapper->getTableName())
->values([
'name' => $qb->createNamedParameter('Test'),
'account_id' => $qb->createNamedParameter(13, IQueryBuilder::PARAM_INT),
'sync_new_token' => $qb->createNamedParameter('VTEsVjE0Mjg1OTkxNDk='),
'sync_changed_token' => $qb->createNamedParameter('VTEsVjE0Mjg1OTkxNDk='),
'sync_vanished_token' => $qb->createNamedParameter('VTEsVjE0Mjg1OTkxNDk='),
'delimiter' => $qb->createNamedParameter('.'),
'messages' => $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT),
'unseen' => $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT),
'selectable' => $qb->createNamedParameter(true, IQueryBuilder::PARAM_BOOL),
'name_hash' => $qb->createNamedParameter(md5('Test')),
]);
$insert->executeStatement();

$qb = $this->db->getQueryBuilder();
$insert = $qb->insert($this->mapper->getTableName())
->values([
'name' => $qb->createNamedParameter('Test '),
'account_id' => $qb->createNamedParameter(13, IQueryBuilder::PARAM_INT),
'sync_new_token' => $qb->createNamedParameter('VTEsVjE0Mjg1OTkxNDk='),
'sync_changed_token' => $qb->createNamedParameter('VTEsVjE0Mjg1OTkxNDk='),
'sync_vanished_token' => $qb->createNamedParameter('VTEsVjE0Mjg1OTkxNDk='),
'delimiter' => $qb->createNamedParameter('.'),
'messages' => $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT),
'unseen' => $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT),
'selectable' => $qb->createNamedParameter(true, IQueryBuilder::PARAM_BOOL),
'name_hash' => $qb->createNamedParameter(md5('Test ')),
]);
$insert->executeStatement();

$resultA = $this->mapper->find($account, 'Test');
$this->assertSame('Test', $resultA->getName());

$resultB = $this->mapper->find($account, 'Test ');
$this->assertSame('Test ', $resultB->getName());
}
}

0 comments on commit 64617d1

Please sign in to comment.