Fix synctoken to int (#287)

Co-authored-by: tchapi <regbasket@gmail.com>

Author: tchapi

migrations/Version20260909100000.php

@@ -0,0 +1,83 @@
+<?php
+
+declare(strict_types=1);
+
+namespace DoctrineMigrations;
+
+use Doctrine\DBAL\Schema\Schema;
+use Doctrine\Migrations\AbstractMigration;
+
+/**
+ * Store DAV sync tokens as integers.
+ *
+ * Adapted from the work of @AnnoyingTechnology in tchapi/davis#277.
+ */
+final class Version20260909100000 extends AbstractMigration
+{
+    public function getDescription(): string
+    {
+        return 'Store DAV sync tokens as integers so that sync-collection range queries compare them numerically';
+    }
+
+    public function up(Schema $schema): void
+    {
+        $engine = $this->connection->getDatabasePlatform()->getName();
+
+        // sabre/dav compares and orders sync tokens numerically:
+        //   WHERE synctoken >= ? AND synctoken < ? ... ORDER BY synctoken
+        // Stored as text, '10' sorts before '9', so a client syncing across a decimal-width
+        // boundary is told the collection advanced but is handed none of the changes.
+        if ('mysql' === $engine) {
+            $this->addSql('ALTER TABLE addressbooks CHANGE synctoken synctoken INT DEFAULT 1 NOT NULL');
+            $this->addSql('ALTER TABLE calendars CHANGE synctoken synctoken INT DEFAULT 1 NOT NULL');
+            $this->addSql('ALTER TABLE addressbookchanges CHANGE synctoken synctoken INT DEFAULT 1 NOT NULL');
+        } elseif ('postgresql' === $engine) {
+            // addressbooks and calendars were already converted by Version20230209142217;
+            // only addressbookchanges is still text here.
+            $this->addSql('ALTER TABLE addressbookchanges ALTER COLUMN synctoken TYPE INT USING synctoken::integer');
+            foreach (['addressbooks', 'calendars', 'addressbookchanges'] as $table) {
+                $this->addSql(sprintf('ALTER TABLE %s ALTER COLUMN synctoken SET DEFAULT 1', $table));
+            }
+        } elseif ('sqlite' === $engine) {
+            // A VARCHAR column has TEXT affinity in SQLite, so the comparison is textual there
+            // too. SQLite cannot alter a column in place: add the replacement, copy, swap, drop.
+            foreach (['addressbooks', 'calendars', 'addressbookchanges'] as $table) {
+                $this->replaceSyncTokenColumn($table, 'INTEGER', 'INTEGER', '1');
+            }
+        }
+    }
+
+    public function down(Schema $schema): void
+    {
+        $engine = $this->connection->getDatabasePlatform()->getName();
+
+        if ('mysql' === $engine) {
+            $this->addSql('ALTER TABLE addressbooks CHANGE synctoken synctoken VARCHAR(255) NOT NULL');
+            $this->addSql('ALTER TABLE calendars CHANGE synctoken synctoken VARCHAR(255) NOT NULL');
+            $this->addSql('ALTER TABLE addressbookchanges CHANGE synctoken synctoken VARCHAR(255) NOT NULL');
+        } elseif ('postgresql' === $engine) {
+            // Only addressbookchanges goes back to text: addressbooks and calendars were
+            // already integers before this migration, and reverting them would reintroduce
+            // the error Version20230209142217 fixed (synctoken + 1 on a text column).
+            foreach (['addressbooks', 'calendars', 'addressbookchanges'] as $table) {
+                $this->addSql(sprintf('ALTER TABLE %s ALTER COLUMN synctoken DROP DEFAULT', $table));
+            }
+            $this->addSql('ALTER TABLE addressbookchanges ALTER COLUMN synctoken TYPE VARCHAR(255) USING synctoken::varchar');
+        } elseif ('sqlite' === $engine) {
+            // NB: SQLite refuses to ADD a NOT NULL column without a default, so the restored
+            // columns keep a harmless DEFAULT '1' that the original schema did not have.
+            foreach (['addressbooks', 'calendars', 'addressbookchanges'] as $table) {
+                $this->replaceSyncTokenColumn($table, 'VARCHAR(255)', 'TEXT', "'1'");
+            }
+        }
+    }
+
+    private function replaceSyncTokenColumn(string $table, string $type, string $cast, string $default): void
+    {
+        $this->addSql(sprintf('ALTER TABLE %s ADD COLUMN new_synctoken %s DEFAULT %s NOT NULL', $table, $type, $default));
+        $this->addSql(sprintf('UPDATE %s SET new_synctoken = CAST(synctoken AS %s)', $table, $cast));
+        $this->addSql(sprintf('ALTER TABLE %s RENAME COLUMN synctoken TO old_synctoken', $table));
+        $this->addSql(sprintf('ALTER TABLE %s RENAME COLUMN new_synctoken TO synctoken', $table));
+        $this->addSql(sprintf('ALTER TABLE %s DROP COLUMN old_synctoken', $table));
+    }
+}

migrations/Version20260910100000.php

@@ -0,0 +1,50 @@
+<?php
+
+declare(strict_types=1);
+
+namespace DoctrineMigrations;
+
+use Doctrine\DBAL\Schema\Schema;
+use Doctrine\Migrations\AbstractMigration;
+
+/**
+ * Index the columns sync-collection reports filter and order on.
+ *
+ * Adapted from the work of @AnnoyingTechnology in tchapi/davis#276.
+ */
+final class Version20260910100000 extends AbstractMigration
+{
+    private const INDEXES = [
+        'idx_calendarchanges_calendar_sync' => ['calendarchanges', 'calendarid, synctoken'],
+        'idx_addressbookchanges_book_sync' => ['addressbookchanges', 'addressbookid, synctoken'],
+    ];
+
+    public function getDescription(): string
+    {
+        return 'Add the (collection, synctoken) indexes that sabre/dav sync-collection reports rely on';
+    }
+
+    public function up(Schema $schema): void
+    {
+        // Every sync-collection REPORT runs
+        //   WHERE synctoken >= ? AND synctoken < ? AND <collection>id = ? ORDER BY synctoken
+        // and only the foreign-key column was indexed, which is not selective enough on its
+        // own: measured on MariaDB with 150k rows, the planner ignored it and did a full scan
+        // with a filesort (150k rows examined, ~14 ms) where the composite index examines 500
+        // rows in under 1 ms.
+        foreach (self::INDEXES as $name => [$table, $columns]) {
+            $this->addSql(sprintf('CREATE INDEX %s ON %s (%s)', $name, $table, $columns));
+        }
+    }
+
+    public function down(Schema $schema): void
+    {
+        $isMysql = 'mysql' === $this->connection->getDatabasePlatform()->getName();
+
+        foreach (self::INDEXES as $name => [$table]) {
+            $this->addSql($isMysql
+                ? sprintf('DROP INDEX %s ON %s', $name, $table)
+                : sprintf('DROP INDEX %s', $name));
+        }
+    }
+}

src/Entity/AddressBook.php

@@ -31,7 +31,7 @@ class AddressBook
     #[ORM\Column(type: 'text', nullable: true)]
     private $description;
 
-    #[ORM\Column(type: 'string', length: 255)]
+    #[ORM\Column(type: 'integer', options: ['default' => 1])]
     private $synctoken;
 
     #[ORM\Column(type: 'boolean', nullable: true, options: ['default' => false])]
@@ -116,12 +116,12 @@ public function setDescription(string $description): self
         return $this;
     }
 
-    public function getSynctoken(): ?string
+    public function getSynctoken(): ?int
     {
         return $this->synctoken;
     }
 
-    public function setSynctoken(string $synctoken): self
+    public function setSynctoken(int $synctoken): self
     {
         $this->synctoken = $synctoken;
 

src/Entity/AddressBookChange.php

@@ -6,6 +6,8 @@
 
 #[ORM\Entity()]
 #[ORM\Table(name: 'addressbookchanges')]
+// sync-collection filters on the collection and orders by synctoken
+#[ORM\Index(name: 'idx_addressbookchanges_book_sync', columns: ['addressbookid', 'synctoken'])]
 class AddressBookChange
 {
     #[ORM\Id]
@@ -16,8 +18,8 @@ class AddressBookChange
     #[ORM\Column(type: 'string', length: 255)]
     private $uri;
 
-    #[ORM\Column(type: 'string', length: 255)]
-    private $synctoken;
+    #[ORM\Column(type: 'integer', options: ['default' => 1])]
+    private $synctoken = 1;
 
     #[ORM\ManyToOne(targetEntity: "App\Entity\AddressBook", inversedBy: 'changes')]
     #[ORM\JoinColumn(name: 'addressbookid', nullable: false)]
@@ -43,12 +45,12 @@ public function setUri(string $uri): self
         return $this;
     }
 
-    public function getSynctoken(): ?string
+    public function getSynctoken(): ?int
     {
         return $this->synctoken;
     }
 
-    public function setSynctoken(string $synctoken): self
+    public function setSynctoken(int $synctoken): self
     {
         $this->synctoken = $synctoken;
 

src/Entity/Calendar.php

@@ -19,7 +19,7 @@ class Calendar
     #[ORM\Column(type: 'integer')]
     private $id;
 
-    #[ORM\Column(type: 'string', length: 255)]
+    #[ORM\Column(type: 'integer', options: ['default' => 1])]
     private $synctoken;
 
     #[ORM\Column(type: 'string', length: 255, nullable: true)]
@@ -47,12 +47,12 @@ public function getId(): ?int
         return $this->id;
     }
 
-    public function getSynctoken(): ?string
+    public function getSynctoken(): ?int
     {
         return $this->synctoken;
     }
 
-    public function setSynctoken(string $synctoken): self
+    public function setSynctoken(int $synctoken): self
     {
         $this->synctoken = $synctoken;
 

src/Entity/CalendarChange.php

@@ -6,6 +6,8 @@
 
 #[ORM\Entity()]
 #[ORM\Table(name: 'calendarchanges')]
+// sync-collection filters on the collection and orders by synctoken
+#[ORM\Index(name: 'idx_calendarchanges_calendar_sync', columns: ['calendarid', 'synctoken'])]
 class CalendarChange
 {
     #[ORM\Id]

tests/Functional/Commands/SyncBirthdayCalendarTest.php

@@ -71,7 +71,7 @@ private function createAddressBookWithCard(string $username, string $cardUri, st
             ->setUri('default')
             ->setDisplayName('Default')
             ->setDescription('')
-            ->setSynctoken('1')
+            ->setSynctoken(1)
             ->setIncludedInBirthdayCalendar(true);
         $this->em->persist($addressBook);
 

tests/Functional/Service/BirthdayServiceTest.php

@@ -54,7 +54,7 @@ private function createAddressBook(
             ->setUri('default')
             ->setDisplayName('Default')
             ->setDescription('')
-            ->setSynctoken('1')
+            ->setSynctoken(1)
             ->setIncludedInBirthdayCalendar($includedInBirthdayCalendar);
         $this->em->persist($addressBook);
         $this->em->flush();

tests/Functional/SyncTokenTest.php

@@ -0,0 +1,80 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Functional;
+
+use Doctrine\ORM\EntityManagerInterface;
+use Sabre\CardDAV\Backend\PDO as CardDavBackend;
+use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase;
+
+class SyncTokenTest extends KernelTestCase
+{
+    private const PRINCIPAL = 'principals/test_user';
+
+    private EntityManagerInterface $em;
+    private CardDavBackend $backend;
+
+    protected function setUp(): void
+    {
+        self::bootKernel();
+
+        $this->em = static::getContainer()->get(EntityManagerInterface::class);
+        $this->backend = new CardDavBackend($this->em->getConnection()->getNativeConnection());
+
+        $this->em->getConnection()->beginTransaction();
+    }
+
+    protected function tearDown(): void
+    {
+        $this->em->getConnection()->rollBack();
+        parent::tearDown();
+    }
+
+    /**
+     * Builds an address book that has already seen ten changes: it now sits at sync token 11,
+     * with a contact added at token 9 and another modified at token 10.
+     */
+    private function createAddressBookAtTokenEleven(): int
+    {
+        $pdo = $this->em->getConnection()->getNativeConnection();
+
+        $id = $this->backend->createAddressBook(self::PRINCIPAL, 'sync-token-test', ['{DAV:}displayname' => 'Sync token test']);
+        $pdo->prepare('UPDATE addressbooks SET synctoken = 11 WHERE id = ?')->execute([$id]);
+
+        $insert = $pdo->prepare('INSERT INTO addressbookchanges (addressbookid, uri, synctoken, operation) VALUES (?, ?, ?, ?)');
+        $insert->execute([$id, 'added.vcf', 9, 1]);
+        $insert->execute([$id, 'changed.vcf', 10, 2]);
+
+        return (int) $id;
+    }
+
+    /**
+     * Regression test: sync tokens were stored as text, so `synctoken >= 9 AND synctoken < 11`
+     * was compared lexicographically ('10' sorts before '9'). A client syncing across a
+     * decimal-width boundary was told the collection had advanced but received no changes at
+     * all, silently losing contacts.
+     */
+    public function testChangesAcrossADecimalBoundaryAreReported(): void
+    {
+        $addressBookId = $this->createAddressBookAtTokenEleven();
+
+        $changes = $this->backend->getChangesForAddressBook($addressBookId, '9', 1);
+
+        $this->assertSame(11, (int) $changes['syncToken']);
+        $this->assertSame(['added.vcf'], $changes['added']);
+        $this->assertSame(['changed.vcf'], $changes['modified']);
+        $this->assertSame([], $changes['deleted']);
+    }
+
+    public function testChangesAreOrderedNumerically(): void
+    {
+        $addressBookId = $this->createAddressBookAtTokenEleven();
+
+        // From token 10 the earlier change at 9 must be out of range, the one at 10 in range
+        $changes = $this->backend->getChangesForAddressBook($addressBookId, '10', 1);
+
+        $this->assertSame([], $changes['added']);
+        $this->assertSame(['changed.vcf'], $changes['modified']);
+    }
+}