From fe6f089163a36c35da9d7333f95969c00ad761d8 Mon Sep 17 00:00:00 2001 From: Troels Ugilt Jensen <6103205+tuj@users.noreply.github.com> Date: Wed, 5 Aug 2026 07:04:10 +0000 Subject: [PATCH 1/2] feat: own the sync watermark in an itk_data_api_modified column MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Leantime core does not maintain its `modified` column on every write path: timesheets saved through ON DUPLICATE KEY UPDATE leave it untouched, and tickets and milestones were filtered on `ticket.date` — the creation date — so edits to existing entities never reached consumers at all. The plugin now owns `itk_data_api_modified` on zp_projects, zp_tickets, zp_timesheets and zp_user, maintained by database triggers so no write path can bypass it. All modifiedAfter filtering and every `modified` value in a response come from that column; core's own column is left alone. Installing stamps existing rows, so the first sync after install returns everything once. The DDL moves out of the service into a SchemaRepository that executes one statement at a time, so a failure past the first is reported instead of swallowed, and is idempotent — reinstalling no longer throws on the delete triggers it left behind. Timestamps are written with UTC_TIMESTAMP() rather than NOW(), which would record the session timezone's clock; the delete triggers now stamp dateDeleted the same way. Users gain a `modified` field, and WorkerData accepts a null name — CONCAT of the first and last name is NULL for a user without a surname, which used to fail the whole /users response. --- CHANGELOG.md | 6 + Model/WorkerData.php | 15 +- README.md | 23 +- Repositories/ApiDataRepository.php | 52 +++-- Repositories/SchemaRepository.php | 270 ++++++++++++++++++++++ Services/APIData.php | 100 ++------ phpunit.xml.dist | 1 + tests/Model/WorkerDataTest.php | 26 +++ tests/Repository/SchemaRepositoryTest.php | 242 +++++++++++++++++++ tests/Service/APIDataTest.php | 78 ++++++- 10 files changed, 702 insertions(+), 111 deletions(-) create mode 100644 Repositories/SchemaRepository.php create mode 100644 tests/Model/WorkerDataTest.php create mode 100644 tests/Repository/SchemaRepositoryTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d0ae4b..8e42df6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ ## [Unreleased] +* [PR-20](https://github.com/ITK-Leantime/data-api/pull/20) + * Added a plugin owned `itk_data_api_modified` column, maintained by database triggers, on projects, tickets, timesheets and users, so no write path can leave the sync watermark behind. + * Changed `modifiedAfter` to filter on that column, so edits to existing tickets and milestones are no longer missed and time logged from the weekly grid is picked up. + * Added `modified` to the users endpoint, and allowed null name and email for users without a surname. + * Changed the deletion triggers to stamp `dateDeleted` in UTC, so `deleted` filters against the same clock the responses are read in. + * Moved the schema handling into a SchemaRepository, executing one statement at a time so installation reports failures instead of swallowing them, and made installing idempotent. * [PR-18](https://github.com/ITK-Leantime/data-api/pull/18) * Allowed null values in API models, so entries referencing deleted users or deleted tickets no longer fail the whole request. * Added userId to timesheets, so hours logged by a deleted user stay attributable. diff --git a/Model/WorkerData.php b/Model/WorkerData.php index 41f47f4..92e29c5 100644 --- a/Model/WorkerData.php +++ b/Model/WorkerData.php @@ -1,16 +1,17 @@ query() - ->select(["id", "name", "modified"]) + ->select(["project.id", "project.name", $this->modifiedSelect("project")]) ->from("zp_projects", "project") ->where("project.id", ">=", $startId) - ->when($modifiedAfter !== null, fn ($query) => $query->where("project.modified", ">=", CarbonImmutable::createFromTimestamp($modifiedAfter)->format(APIData::DATE_FORMAT))) + ->when($modifiedAfter !== null, fn ($query) => $query->where($this->modified("project"), ">=", $this->cutoff($modifiedAfter))) ->when($ids !== null, fn ($query) => $query->whereIn("project.id", $ids)) ->orderBy("id", "ASC") ->limit($limit) @@ -28,14 +28,14 @@ public function getProjects(int $startId, int $limit, ?int $modifiedAfter = null ->toArray(); } - public function getMilestones(int $startId, int $limit, int $modifiedAfter = null, ?array $ids = null, ?array $projectIds = null): array + public function getMilestones(int $startId, int $limit, ?int $modifiedAfter = null, ?array $ids = null, ?array $projectIds = null): array { return $this->query() - ->select(["id", "headline", "projectId", "modified"]) + ->select(["ticket.id", "ticket.headline", "ticket.projectId", $this->modifiedSelect("ticket")]) ->from("zp_tickets", "ticket") ->where("ticket.id", ">=", $startId) ->where("ticket.type", "=", "milestone") - ->when($modifiedAfter !== null, fn ($query) => $query->where("ticket.date", ">=", CarbonImmutable::createFromTimestamp($modifiedAfter)->format(APIData::DATE_FORMAT))) + ->when($modifiedAfter !== null, fn ($query) => $query->where($this->modified("ticket"), ">=", $this->cutoff($modifiedAfter))) ->when($ids !== null, fn ($query) => $query->whereIn("ticket.id", $ids)) ->when($projectIds != null, fn ($query) => $query->whereIn("ticket.projectId", $projectIds)) ->orderBy("id", "ASC") @@ -44,15 +44,15 @@ public function getMilestones(int $startId, int $limit, int $modifiedAfter = nul ->toArray(); } - public function getTickets(int $startId, int $limit, int $modifiedAfter = null, array $ids = null, ?array $projectIds = null): array + public function getTickets(int $startId, int $limit, ?int $modifiedAfter = null, ?array $ids = null, ?array $projectIds = null): array { return $this->query() - ->select(["ticket.id", "ticket.headline", "ticket.projectId", "ticket.status", "ticket.planHours", "ticket.hourRemaining", "ticket.tags", "ticket.dateToFinish", "ticket.editTo", "ticket.milestoneid", "ticket.modified", "user.username"]) + ->select(["ticket.id", "ticket.headline", "ticket.projectId", "ticket.status", "ticket.planHours", "ticket.hourRemaining", "ticket.tags", "ticket.dateToFinish", "ticket.editTo", "ticket.milestoneid", $this->modifiedSelect("ticket"), "user.username"]) ->from("zp_tickets", "ticket") ->where("ticket.id", ">=", $startId) ->where("ticket.type", "<>", "milestone") ->leftJoin('zp_user as user', "user.id", "=", "ticket.editorId") - ->when($modifiedAfter !== null, fn ($query) => $query->where("ticket.date", ">=", CarbonImmutable::createFromTimestamp($modifiedAfter)->format(APIData::DATE_FORMAT))) + ->when($modifiedAfter !== null, fn ($query) => $query->where($this->modified("ticket"), ">=", $this->cutoff($modifiedAfter))) ->when($ids !== null, fn ($query) => $query->whereIn("ticket.id", $ids)) ->when($projectIds != null, fn ($query) => $query->whereIn("ticket.projectId", $projectIds)) ->orderBy("id", "ASC") @@ -65,12 +65,12 @@ public function getTimesheets(int $startId, int $limit, ?int $modifiedAfter = nu { return $this->query() ->from("zp_timesheets", "timesheet") - ->select(["timesheet.id", "timesheet.description", "timesheet.hours", "timesheet.workDate", "timesheet.modified", "timesheet.ticketId", "timesheet.userId", "timesheet.kind", "user.username", "ticket.projectId"]) + ->select(["timesheet.id", "timesheet.description", "timesheet.hours", "timesheet.workDate", $this->modifiedSelect("timesheet"), "timesheet.ticketId", "timesheet.userId", "timesheet.kind", "user.username", "ticket.projectId"]) ->where("timesheet.id", ">=", $startId) ->whereNotNull("timesheet.hours") ->leftJoin('zp_user as user', "user.id", "=", "timesheet.userId") ->leftJoin('zp_tickets as ticket', "ticket.id", "=", "timesheet.ticketId") - ->when($modifiedAfter !== null, fn ($query) => $query->where("timesheet.modified", ">=", CarbonImmutable::createFromTimestamp($modifiedAfter)->format(APIData::DATE_FORMAT))) + ->when($modifiedAfter !== null, fn ($query) => $query->where($this->modified("timesheet"), ">=", $this->cutoff($modifiedAfter))) ->when($ids !== null, fn ($query) => $query->whereIn("timesheet.id", $ids)) ->when($projectIds != null, fn ($query) => $query->whereIn("ticket.projectId", $projectIds)) ->orderBy("timesheet.id", "ASC") @@ -83,10 +83,10 @@ public function getWorkers(int $startId, int $limit, ?int $modifiedAfter = null, { return $this->query() ->from("zp_user", "worker") - ->select(["worker.id", "worker.username", DB::raw("CONCAT(worker.firstname, ' ', worker.lastname) as name")]) + ->select(["worker.id", "worker.username", DB::raw("CONCAT(worker.firstname, ' ', worker.lastname) as name"), $this->modifiedSelect("worker")]) ->where("worker.id", ">=", $startId) ->where("worker.source", "<>", "api") - ->when($modifiedAfter !== null, fn ($query) => $query->where("worker.modified", ">=", CarbonImmutable::createFromTimestamp($modifiedAfter)->format(APIData::DATE_FORMAT))) + ->when($modifiedAfter !== null, fn ($query) => $query->where($this->modified("worker"), ">=", $this->cutoff($modifiedAfter))) ->when($ids !== null, fn ($query) => $query->whereIn("worker.id", $ids)) ->orderBy("worker.id", "ASC") ->limit($limit) @@ -108,8 +108,34 @@ public function getDeleted(string $type, ?int $deletedAfter = null): array ->select(["entryId", "dateDeleted"]) ->when($type === APIData::TYPE_MILESTONES, fn ($query) => $query->where('type', '=', 'milestone')) ->when($type === APIData::TYPE_TICKETS, fn ($query) => $query->where('type', '<>', 'milestone')) - ->when($deletedAfter !== null, fn ($query) => $query->where("entry.dateDeleted", ">=", CarbonImmutable::createFromTimestamp($deletedAfter)->format(APIData::DATE_FORMAT))) + ->when($deletedAfter !== null, fn ($query) => $query->where("entry.dateDeleted", ">=", $this->cutoff($deletedAfter))) ->get() ->toArray(); } + + /** + * The plugin-owned timestamp column, qualified by the query's table alias. + * Core's own `modified` is not maintained on every write path, so it cannot + * carry the modifiedAfter contract — see SchemaRepository. + */ + private function modified(string $alias): string + { + return sprintf('%s.%s', $alias, SchemaRepository::COLUMN); + } + + /** + * Exposed to consumers as plain `modified`, so the column swap is invisible + * to them and to the mapping in APIData. + */ + private function modifiedSelect(string $alias): string + { + return sprintf('%s as modified', $this->modified($alias)); + } + + private function cutoff(int $timestamp): string + { + // Explicit UTC: Carbon 3 defaults to it, but Carbon comes from the host + // Leantime install, and the triggers write UTC_TIMESTAMP(). + return CarbonImmutable::createFromTimestamp($timestamp, 'UTC')->format(APIData::DATE_FORMAT); + } } diff --git a/Repositories/SchemaRepository.php b/Repositories/SchemaRepository.php new file mode 100644 index 0000000..01eadcb --- /dev/null +++ b/Repositories/SchemaRepository.php @@ -0,0 +1,270 @@ + 'BEFORE INSERT', 'update' => 'BEFORE UPDATE']; + + /** Deletion tracking, keyed by trigger name: [source table, target table, columns, values]. */ + private const DELETE_TRIGGERS = [ + 'itk_projects_deleted_trigger' => ['zp_projects', 'itk_projects_deleted', 'entryId', 'OLD.id'], + 'itk_tickets_deleted_trigger' => ['zp_tickets', 'itk_tickets_deleted', 'entryId, type', 'OLD.id, OLD.type'], + 'itk_timesheets_deleted_trigger' => ['zp_timesheets', 'itk_timesheets_deleted', 'entryId', 'OLD.id'], + ]; + + public function install(): void + { + $this->execute(...self::deletedTableStatements()); + $this->execute(...self::deleteTriggerStatements()); + + foreach (self::TRACKED_TABLES as $table) { + if (!$this->hasColumn($table, self::COLUMN)) { + $this->execute(self::addColumnStatement($table)); + } + + if (!$this->hasIndex($table, self::INDEX)) { + $this->execute(self::addIndexStatement($table)); + } + + // Triggers first, so rows written between here and the backfill are + // stamped by the trigger rather than left behind. + $this->execute(...self::triggerStatements($table)); + $this->execute(self::backfillStatement($table)); + } + } + + public function uninstall(): void + { + // Columns, indexes and the itk_*_deleted tables are deliberately kept: + // a reinstall then preserves timestamp history instead of forcing every + // consumer through another full resync. + $this->execute(...self::uninstallStatements()); + } + + /** + * `zp_timesheets` + `insert` → `itk_data_api_timesheets_modified_insert`. + */ + public static function triggerName(string $table, string $event): string + { + $shortName = str_starts_with($table, 'zp_') ? substr($table, 3) : $table; + + return sprintf('itk_data_api_%s_modified_%s', $shortName, $event); + } + + public static function addColumnStatement(string $table): string + { + return sprintf('ALTER TABLE `%s` ADD COLUMN `%s` DATETIME NULL DEFAULT NULL', $table, self::COLUMN); + } + + public static function addIndexStatement(string $table): string + { + return sprintf('ALTER TABLE `%s` ADD INDEX `%s` (`%s`)', $table, self::INDEX, self::COLUMN); + } + + /** + * Runs on every install, not just the first. On an existing install it is a + * no-op except for rows written while the plugin was uninstalled and the + * triggers were gone — which it heals. + */ + public static function backfillStatement(string $table): string + { + return sprintf( + 'UPDATE `%s` SET `%s` = UTC_TIMESTAMP() WHERE `%s` IS NULL', + $table, + self::COLUMN, + self::COLUMN, + ); + } + + /** + * @return list A DROP IF EXISTS immediately followed by its CREATE, per event. + */ + public static function triggerStatements(string $table): array + { + $statements = []; + + foreach (self::EVENTS as $event => $timing) { + $name = self::triggerName($table, $event); + + $statements[] = self::dropTriggerStatement($name); + // A single-statement body, so it needs no BEGIN … END and contains + // no semicolon — which is what lets the statements be executed one + // at a time instead of as one multi-statement string. + $statements[] = sprintf( + 'CREATE TRIGGER `%s` %s ON `%s` FOR EACH ROW SET NEW.%s = UTC_TIMESTAMP()', + $name, + $timing, + $table, + self::COLUMN, + ); + } + + return $statements; + } + + /** + * @return list + */ + public static function dropTriggerStatements(string $table): array + { + return array_map( + static fn (string $event) => self::dropTriggerStatement(self::triggerName($table, $event)), + array_keys(self::EVENTS), + ); + } + + /** + * Every trigger the plugin owns, modified and deleted alike. + * + * @return list + */ + public static function uninstallStatements(): array + { + $statements = array_map(self::dropTriggerStatement(...), array_keys(self::DELETE_TRIGGERS)); + + foreach (self::TRACKED_TABLES as $table) { + array_push($statements, ...self::dropTriggerStatements($table)); + } + + return $statements; + } + + /** + * Left exactly as they were first shipped: IF NOT EXISTS makes these no-ops + * on every existing install, so changing them here would only make fresh + * databases differ. The `DEFAULT NOW()` on `dateDeleted` is unreachable now + * that the delete triggers set the column themselves. + * + * @return list + */ + public static function deletedTableStatements(): array + { + return [ + 'CREATE TABLE IF NOT EXISTS `itk_projects_deleted` ( + `id` int(11) NOT NULL AUTO_INCREMENT, + `entryId` int(11) DEFAULT NULL, + `dateDeleted` datetime DEFAULT NOW(), + PRIMARY KEY (`id`) + ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci', + + 'CREATE TABLE IF NOT EXISTS `itk_tickets_deleted` ( + `id` int(11) NOT NULL AUTO_INCREMENT, + `entryId` int(11) DEFAULT NULL, + `type` varchar(255) DEFAULT NULL, + `dateDeleted` datetime DEFAULT NOW(), + PRIMARY KEY (`id`) + ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci', + + 'CREATE TABLE IF NOT EXISTS `itk_timesheets_deleted` ( + `id` int(11) NOT NULL AUTO_INCREMENT, + `entryId` int(11) DEFAULT NULL, + `dateDeleted` datetime DEFAULT NOW(), + PRIMARY KEY (`id`) + ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci', + ]; + } + + /** + * Unchanged in name, but re-expressed as single-statement bodies behind a + * DROP IF EXISTS. Reinstalling used to throw here because CREATE TRIGGER hit + * the triggers the previous install left behind. + * + * `dateDeleted` is now set explicitly rather than left to the column's + * `DEFAULT NOW()`, which wrote the session timezone's clock while the read + * side parses the value as UTC. + * + * @return list + */ + public static function deleteTriggerStatements(): array + { + $statements = []; + + foreach (self::DELETE_TRIGGERS as $name => [$source, $target, $columns, $values]) { + $statements[] = self::dropTriggerStatement($name); + $statements[] = sprintf( + 'CREATE TRIGGER `%s` AFTER DELETE ON `%s` FOR EACH ROW' + . ' INSERT INTO `%s`(%s, dateDeleted) VALUES (%s, UTC_TIMESTAMP())', + $name, + $source, + $target, + $columns, + $values, + ); + } + + return $statements; + } + + private static function dropTriggerStatement(string $name): string + { + return sprintf('DROP TRIGGER IF EXISTS `%s`', $name); + } + + private function execute(string ...$statements): void + { + $pdo = app('db')->connection()->getPdo(); + + foreach ($statements as $statement) { + // Laravel sets ERRMODE_EXCEPTION so exec() throws, but guard the + // false return in case the connection is configured differently. + if ($pdo->exec($statement) === false) { + [, $code, $message] = $pdo->errorInfo(); + + throw new \RuntimeException(sprintf('Schema statement failed (%s): %s', $code, $message)); + } + } + } + + private function hasColumn(string $table, string $column): bool + { + return $this->query() + ->from('information_schema.COLUMNS') + ->whereRaw('TABLE_SCHEMA = DATABASE()') + ->where('TABLE_NAME', '=', $table) + ->where('COLUMN_NAME', '=', $column) + ->exists(); + } + + private function hasIndex(string $table, string $index): bool + { + return $this->query() + ->from('information_schema.STATISTICS') + ->whereRaw('TABLE_SCHEMA = DATABASE()') + ->where('TABLE_NAME', '=', $table) + ->where('INDEX_NAME', '=', $index) + ->exists(); + } + + private function query(): Builder + { + return app('db')->connection()->query(); + } +} diff --git a/Services/APIData.php b/Services/APIData.php index af28091..e5132a9 100644 --- a/Services/APIData.php +++ b/Services/APIData.php @@ -12,6 +12,7 @@ use Leantime\Plugins\APIData\Model\TimesheetData; use Leantime\Plugins\APIData\Model\WorkerData; use Leantime\Plugins\APIData\Repositories\ApiDataRepository; +use Leantime\Plugins\APIData\Repositories\SchemaRepository; class APIData { @@ -25,91 +26,21 @@ class APIData public function __construct( private readonly TicketRepository $ticketRepository, private readonly ApiDataRepository $apiDataRepository, + private readonly SchemaRepository $schemaRepository, ) {} + /** + * Leantime calls this on every install, and offers no separate upgrade hook, + * so SchemaRepository::install() has to be idempotent. + */ public function install(): void { - $sql = " - CREATE TABLE IF NOT EXISTS `itk_projects_deleted` ( - `id` int(11) NOT NULL AUTO_INCREMENT, - `entryId` int(11) DEFAULT NULL, - `dateDeleted` datetime DEFAULT NOW(), - PRIMARY KEY (`id`) - ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci; - - CREATE TABLE IF NOT EXISTS `itk_tickets_deleted` ( - `id` int(11) NOT NULL AUTO_INCREMENT, - `entryId` int(11) DEFAULT NULL, - `type` varchar(255) DEFAULT NULL, - `dateDeleted` datetime DEFAULT NOW(), - PRIMARY KEY (`id`) - ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci; - - CREATE TABLE IF NOT EXISTS `itk_timesheets_deleted` ( - `id` int(11) NOT NULL AUTO_INCREMENT, - `entryId` int(11) DEFAULT NULL, - `dateDeleted` datetime DEFAULT NOW(), - PRIMARY KEY (`id`) - ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci; - - CREATE TRIGGER itk_projects_deleted_trigger - AFTER DELETE ON zp_projects - FOR EACH ROW - BEGIN - INSERT INTO itk_projects_deleted(entryId) - VALUES (OLD.id); - END; - - CREATE TRIGGER itk_tickets_deleted_trigger - AFTER DELETE ON zp_tickets - FOR EACH ROW - BEGIN - INSERT INTO itk_tickets_deleted(entryId, type) - VALUES (OLD.id, OLD.type); - END; - - CREATE TRIGGER itk_timesheets_deleted_trigger - AFTER DELETE ON zp_timesheets - FOR EACH ROW - BEGIN - INSERT INTO itk_timesheets_deleted(entryId) - VALUES (OLD.id); - END; - "; - - // Use PDO for multi-statement SQL with parameter binding - // We need to use PDO directly because Laravel's statement() method - // may not handle multi-statement SQL properly - $pdo = app('db')->connection()->getPdo(); - $stmn = $pdo->prepare($sql); - - $stmn->execute(); - - $stmn->closeCursor(); + $this->schemaRepository->install(); } public function uninstall(): void { - $sql = " - DROP TRIGGER itk_projects_deleted_trigger; - DROP TRIGGER itk_tickets_deleted_trigger; - DROP TRIGGER itk_timesheets_deleted_trigger; - "; - - // Tables are not remove, to preserve data through install/uninstalls. - // DROP TABLE `itk_projects_deleted`; - // DROP TABLE `itk_tickets_deleted`; - // DROP TABLE `itk_timesheets_deleted`; - - // Use PDO for multi-statement SQL with parameter binding - // We need to use PDO directly because Laravel's statement() method - // may not handle multi-statement SQL properly - $pdo = app('db')->connection()->getPdo(); - $stmn = $pdo->prepare($sql); - - $stmn->execute(); - - $stmn->closeCursor(); + $this->schemaRepository->uninstall(); } public function getProjects(int $startId, int $limit, ?int $modifiedAfter = null, ?array $ids = null): array @@ -125,7 +56,7 @@ public function getProjects(int $startId, int $limit, ?int $modifiedAfter = null }, $values); } - public function getMilestones(int $startId, int $limit, int $modifiedAfter = null, ?array $ids = null, ?array $projectIds = null): array + public function getMilestones(int $startId, int $limit, ?int $modifiedAfter = null, ?array $ids = null, ?array $projectIds = null): array { $values = $this->apiDataRepository->getMilestones($startId, $limit, $modifiedAfter, $ids, $projectIds); @@ -139,7 +70,7 @@ public function getMilestones(int $startId, int $limit, int $modifiedAfter = nul }, $values); } - public function getTickets(int $startId, int $limit, int $modifiedAfter = null, array $ids = null, ?array $projectIds = null): array + public function getTickets(int $startId, int $limit, ?int $modifiedAfter = null, ?array $ids = null, ?array $projectIds = null): array { $values = $this->apiDataRepository->getTickets($startId, $limit, $modifiedAfter, $ids, $projectIds); @@ -191,15 +122,16 @@ public function getTimesheets(int $startId, int $limit, ?int $modifiedAfter = nu }, $values); } - public function getWorkers(int $startId, int $limit, ?int $modifiedAfter = null, ?array $ids = null, ?array $projectIds = null): array + public function getWorkers(int $startId, int $limit, ?int $modifiedAfter = null, ?array $ids = null): array { - $values = $this->apiDataRepository->getWorkers($startId, $limit, $modifiedAfter, $ids, $projectIds); + $values = $this->apiDataRepository->getWorkers($startId, $limit, $modifiedAfter, $ids); return array_map(function ($value) { return new WorkerData( - $value->id, - $value->username, - $value->name, + id: $value->id, + email: $value->username, + name: $value->name, + modified: $this->getCarbonFromDatabaseValue($value->modified), ); }, $values); } diff --git a/phpunit.xml.dist b/phpunit.xml.dist index 2a306e4..7b73098 100644 --- a/phpunit.xml.dist +++ b/phpunit.xml.dist @@ -15,6 +15,7 @@ Model + Repositories Services diff --git a/tests/Model/WorkerDataTest.php b/tests/Model/WorkerDataTest.php new file mode 100644 index 0000000..2fe6cd5 --- /dev/null +++ b/tests/Model/WorkerDataTest.php @@ -0,0 +1,26 @@ +assertSame(57, $worker->id); + $this->assertNull($worker->email); + $this->assertNull($worker->name); + $this->assertNull($worker->modified); + } +} diff --git a/tests/Repository/SchemaRepositoryTest.php b/tests/Repository/SchemaRepositoryTest.php new file mode 100644 index 0000000..b19add7 --- /dev/null +++ b/tests/Repository/SchemaRepositoryTest.php @@ -0,0 +1,242 @@ +assertSame( + 'itk_data_api_timesheets_modified_insert', + SchemaRepository::triggerName('zp_timesheets', 'insert'), + ); + $this->assertSame( + 'itk_data_api_timesheets_modified_update', + SchemaRepository::triggerName('zp_timesheets', 'update'), + ); + } + + public function testATableGetsBothAnInsertAndAnUpdateTrigger(): void + { + $statements = SchemaRepository::triggerStatements('zp_timesheets'); + + $this->assertCount(4, $statements); + $this->assertStringStartsWith( + 'CREATE TRIGGER `itk_data_api_timesheets_modified_insert` BEFORE INSERT ON `zp_timesheets`', + $statements[1], + ); + $this->assertStringStartsWith( + 'CREATE TRIGGER `itk_data_api_timesheets_modified_update` BEFORE UPDATE ON `zp_timesheets`', + $statements[3], + ); + } + + /** + * Installation is also migration — Leantime has no upgrade hook — so every + * CREATE has to be preceded by the DROP that makes re-running it safe. + */ + public function testEveryCreateTriggerIsPrecededByItsOwnDrop(): void + { + $statements = $this->allTriggerStatements(); + + foreach ($statements as $index => $statement) { + if (!str_starts_with($statement, 'CREATE TRIGGER')) { + continue; + } + + $this->assertSame( + sprintf('DROP TRIGGER IF EXISTS `%s`', $this->triggerNameIn($statement)), + $statements[$index - 1] ?? null, + ); + } + } + + /** + * The whole point of owning the column is that its values have one + * unambiguous meaning. NOW() would write the session timezone's clock. + */ + public function testTimestampsAreWrittenInUtc(): void + { + $statements = $this->allTriggerStatements(); + + foreach (SchemaRepository::TRACKED_TABLES as $table) { + $statements[] = SchemaRepository::backfillStatement($table); + } + + foreach ($statements as $statement) { + $this->assertStringNotContainsStringIgnoringCase('NOW()', $statement); + } + + foreach ($statements as $statement) { + if (str_starts_with($statement, 'DROP TRIGGER')) { + continue; + } + + $this->assertStringContainsString('UTC_TIMESTAMP()', $statement); + } + } + + /** + * The deletion tables' `DEFAULT NOW()` writes the session timezone's clock, + * but `/deleted` parses `dateDeleted` as UTC. The trigger sets the column + * itself so the two agree. + */ + public function testDeletionsAreStampedInUtcRatherThanLeftToTheColumnDefault(): void + { + $creates = array_values(array_filter( + SchemaRepository::deleteTriggerStatements(), + static fn (string $statement) => str_starts_with($statement, 'CREATE TRIGGER'), + )); + + $this->assertCount(3, $creates); + + foreach ($creates as $statement) { + $this->assertStringContainsString('dateDeleted', $statement); + $this->assertStringContainsString('UTC_TIMESTAMP()', $statement); + } + + $this->assertStringContainsString( + 'INSERT INTO `itk_tickets_deleted`(entryId, type, dateDeleted) VALUES (OLD.id, OLD.type, UTC_TIMESTAMP())', + $creates[1], + ); + } + + /** + * Production is MySQL 8.4. `CREATE OR REPLACE TRIGGER` and + * `ADD COLUMN IF NOT EXISTS` are MariaDB-only — an earlier attempt at this + * feature was rejected for using them. + */ + public function testNoStatementUsesMariaDbOnlySyntax(): void + { + foreach ($this->allInstallStatements() as $statement) { + $this->assertStringNotContainsStringIgnoringCase('CREATE OR REPLACE', $statement); + $this->assertStringNotContainsStringIgnoringCase('ADD COLUMN IF NOT EXISTS', $statement); + } + } + + /** + * Statements are executed one at a time, so a semicolon inside a trigger + * body would be read as the end of the CREATE. + */ + public function testNoTriggerBodyContainsASemicolon(): void + { + foreach ($this->allTriggerStatements() as $statement) { + $this->assertStringNotContainsString(';', $statement); + } + } + + #[DataProvider('trackedTables')] + public function testEveryTrackedTableGetsTheFullSchema(string $table): void + { + $this->assertStringContainsString( + sprintf('ALTER TABLE `%s` ADD COLUMN `%s`', $table, SchemaRepository::COLUMN), + SchemaRepository::addColumnStatement($table), + ); + $this->assertStringContainsString( + sprintf('ADD INDEX `%s` (`%s`)', SchemaRepository::INDEX, SchemaRepository::COLUMN), + SchemaRepository::addIndexStatement($table), + ); + $this->assertStringContainsString( + sprintf('UPDATE `%s`', $table), + SchemaRepository::backfillStatement($table), + ); + $this->assertCount(4, SchemaRepository::triggerStatements($table)); + } + + /** + * Only rows the triggers never saw are stamped, so re-installing does not + * rewrite timestamps and force consumers through a second full resync. + */ + public function testTheBackfillOnlyTouchesRowsWithoutATimestamp(): void + { + $this->assertStringEndsWith( + sprintf('WHERE `%s` IS NULL', SchemaRepository::COLUMN), + SchemaRepository::backfillStatement('zp_timesheets'), + ); + } + + /** + * Eight modified triggers plus the three deletion triggers. + */ + public function testUninstallDropsEveryTriggerThePluginOwns(): void + { + $statements = SchemaRepository::uninstallStatements(); + + $this->assertCount(11, $statements); + $this->assertSame($statements, array_unique($statements)); + + foreach ($statements as $statement) { + $this->assertStringStartsWith('DROP TRIGGER IF EXISTS ', $statement); + } + + foreach ($this->allTriggerStatements() as $statement) { + if (!str_starts_with($statement, 'CREATE TRIGGER')) { + continue; + } + + $this->assertContains( + sprintf('DROP TRIGGER IF EXISTS `%s`', $this->triggerNameIn($statement)), + $statements, + ); + } + } + + /** + * @return list + */ + public static function trackedTables(): array + { + return array_map(static fn (string $table) => [$table], SchemaRepository::TRACKED_TABLES); + } + + /** + * @return list + */ + private function allTriggerStatements(): array + { + $statements = SchemaRepository::deleteTriggerStatements(); + + foreach (SchemaRepository::TRACKED_TABLES as $table) { + array_push($statements, ...SchemaRepository::triggerStatements($table)); + } + + return $statements; + } + + /** + * @return list + */ + private function allInstallStatements(): array + { + $statements = array_merge( + SchemaRepository::deletedTableStatements(), + $this->allTriggerStatements(), + ); + + foreach (SchemaRepository::TRACKED_TABLES as $table) { + $statements[] = SchemaRepository::addColumnStatement($table); + $statements[] = SchemaRepository::addIndexStatement($table); + $statements[] = SchemaRepository::backfillStatement($table); + } + + return $statements; + } + + private function triggerNameIn(string $statement): string + { + preg_match('/^CREATE TRIGGER `([^`]+)`/', $statement, $matches); + + return $matches[1] ?? ''; + } +} diff --git a/tests/Service/APIDataTest.php b/tests/Service/APIDataTest.php index 6d1435d..5649010 100644 --- a/tests/Service/APIDataTest.php +++ b/tests/Service/APIDataTest.php @@ -5,6 +5,7 @@ use Carbon\CarbonInterface; use Leantime\Domain\Tickets\Repositories\Tickets as TicketRepository; use Leantime\Plugins\APIData\Repositories\ApiDataRepository; +use Leantime\Plugins\APIData\Repositories\SchemaRepository; use Leantime\Plugins\APIData\Services\APIData; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\TestCase; @@ -91,7 +92,7 @@ public function testGetTicketsDoesNotLookUpStatusLabelsForATicketWithoutAProject $repository = $this->createMock(ApiDataRepository::class); $repository->method('getTickets')->willReturn([$this->ticketRow(['projectId' => null])]); - $tickets = (new APIData($ticketRepository, $repository))->getTickets(0, 100); + $tickets = $this->makeService($repository, $ticketRepository)->getTickets(0, 100); $this->assertNull($tickets[0]->projectId); $this->assertNull($tickets[0]->status); @@ -114,12 +115,47 @@ public function testGetTicketsResolvesStatusFromTheProjectsOwnLabels(): void $this->ticketRow(['projectId' => 92, 'status' => 3]), ]); - $tickets = (new APIData($ticketRepository, $repository))->getTickets(0, 100); + $tickets = $this->makeService($repository, $ticketRepository)->getTickets(0, 100); $this->assertSame(92, $tickets[0]->projectId); $this->assertSame('NEW', $tickets[0]->status); } + /** + * Users are the fourth entity type to carry a sync watermark. It comes from + * the plugin's own column, so it has to survive the mapping as UTC. + */ + public function testGetWorkersMapsTheModifiedTimestampAsUtc(): void + { + $service = $this->makeServiceReturningWorkers([$this->workerRow()]); + + $worker = $service->getWorkers(0, 100)[0]; + + $this->assertSame(57, $worker->id); + $this->assertSame('anne@aarhus.dk', $worker->email); + $this->assertSame('Anne Andersen', $worker->name); + + $this->assertInstanceOf(CarbonInterface::class, $worker->modified); + $this->assertSame('2026-03-03 11:30:00', $worker->modified->format('Y-m-d H:i:s')); + $this->assertSame('UTC', $worker->modified->timezoneName); + } + + /** + * `CONCAT(firstname, ' ', lastname)` is NULL when either column is, and one + * such user used to fail the whole /users response with a TypeError. + */ + public function testGetWorkersMapsAUserWithoutANameToNull(): void + { + $service = $this->makeServiceReturningWorkers([ + $this->workerRow(['name' => null, 'modified' => '0000-00-00 00:00:00']), + ]); + + $worker = $service->getWorkers(0, 100)[0]; + + $this->assertNull($worker->name); + $this->assertNull($worker->modified); + } + /** * @param list $rows */ @@ -128,7 +164,27 @@ private function makeServiceReturningTimesheets(array $rows): APIData $repository = $this->createMock(ApiDataRepository::class); $repository->method('getTimesheets')->willReturn($rows); - return new APIData($this->createMock(TicketRepository::class), $repository); + return $this->makeService($repository); + } + + /** + * @param list $rows + */ + private function makeServiceReturningWorkers(array $rows): APIData + { + $repository = $this->createMock(ApiDataRepository::class); + $repository->method('getWorkers')->willReturn($rows); + + return $this->makeService($repository); + } + + private function makeService(ApiDataRepository $repository, ?TicketRepository $ticketRepository = null): APIData + { + return new APIData( + $ticketRepository ?? $this->createMock(TicketRepository::class), + $repository, + $this->createMock(SchemaRepository::class), + ); } /** @@ -176,6 +232,22 @@ private function ticketRow(array $overrides = []): object ], $overrides); } + /** + * A row as `ApiDataRepository::getWorkers()` returns it — `modified` is the + * aliased `itk_data_api_modified` column. + * + * @param array $overrides + */ + private function workerRow(array $overrides = []): object + { + return (object) array_merge([ + 'id' => 57, + 'username' => 'anne@aarhus.dk', + 'name' => 'Anne Andersen', + 'modified' => '2026-03-03 11:30:00', + ], $overrides); + } + /** * Mirrors the shape of Leantime v3.9.7's `$statusListSeed`: keyed by the * integer status id, each entry carrying a `statusType`. From 1f1f3c306a8a836585ddbe456df6848971617109 Mon Sep 17 00:00:00 2001 From: Troels Ugilt Jensen <6103205+tuj@users.noreply.github.com> Date: Fri, 14 Aug 2026 13:38:43 +0000 Subject: [PATCH 2/2] fix: addressed review comments on PR-20 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extracted the per-table install sequence into installStatements() so the order it depends on is pinned by a test rather than by a comment, and moved the index after the backfill — it was being built over an all-NULL column and then rewritten entry by entry. Gave the deletion tables a migration path. CREATE TABLE IF NOT EXISTS never reaches a database that already has the table, so changes now go in deletedTableAlterStatements(), which both populations run. First of them drops the dateDeleted default: the triggers make it unreachable, but the tables outlive an uninstall and the triggers do not. Wrote down the precondition the design rests on — install and update happen with the site down — and corrected the backfill docblock, which claimed to heal rows written while the triggers were absent when it only recovers inserts. --- CHANGELOG.md | 1 + README.md | 8 ++ Repositories/SchemaRepository.php | 118 +++++++++++++++---- tests/Repository/SchemaRepositoryTest.php | 136 +++++++++++++++++++++- 4 files changed, 234 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dc50e4e..a524e8f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ * Added `modified` to the users endpoint. * Changed the deletion triggers to stamp `dateDeleted` in UTC, so `deleted` filters against the same clock the responses are read in. * Moved the schema handling into a SchemaRepository, executing one statement at a time so installation reports failures instead of swallowing them, and made installing idempotent. + * Dropped the `dateDeleted` default on the deletion tables, so a row inserted without a trigger in place is left null rather than stamped with the server's local time. * [PR-18](https://github.com/ITK-Leantime/data-api/pull/18) * Allowed null values in API models, so entries referencing deleted users or deleted tickets no longer fail the whole request. * Added userId to timesheets, so hours logged by a deleted user stay attributable. diff --git a/README.md b/README.md index dcced30..5d147c0 100644 --- a/README.md +++ b/README.md @@ -22,6 +22,14 @@ database, no write path can bypass them. The column is written as UTC, and `modifiedAfter` filters on it. +The Leantime database user needs `ALTER` on `zp_projects`, `zp_tickets`, `zp_timesheets` and +`zp_user`, on top of the `CREATE` and `TRIGGER` the plugin already needed. Installation fails, and +says so, if the grant is missing. + +NB! Install and update the plugin with the site down. The triggers are absent while the plugin is +being replaced, and an edit made in that window is not recoverable — installing only stamps rows that +have no timestamp at all, which covers new rows and nothing else. + NB! Installing stamps every existing row with the install time, so **the first sync after installing returns everything once**. diff --git a/Repositories/SchemaRepository.php b/Repositories/SchemaRepository.php index 01eadcb..549479f 100644 --- a/Repositories/SchemaRepository.php +++ b/Repositories/SchemaRepository.php @@ -21,6 +21,11 @@ * `ADD COLUMN IF NOT EXISTS` — both are MariaDB-only. * - Every timestamp is `UTC_TIMESTAMP()`, never `NOW()`, which would write the * session timezone's clock and reintroduce the ambiguity we are fixing. + * + * Installing and updating the plugin happens with the site down. That precondition + * is load-bearing: while the plugin is being replaced its triggers are gone, and + * an edit landing in that window is lost for good. The backfill only stamps rows + * that have no timestamp at all, so it recovers inserts and nothing else. */ class SchemaRepository { @@ -43,21 +48,15 @@ class SchemaRepository public function install(): void { $this->execute(...self::deletedTableStatements()); + $this->execute(...self::deletedTableAlterStatements()); $this->execute(...self::deleteTriggerStatements()); foreach (self::TRACKED_TABLES as $table) { - if (!$this->hasColumn($table, self::COLUMN)) { - $this->execute(self::addColumnStatement($table)); - } - - if (!$this->hasIndex($table, self::INDEX)) { - $this->execute(self::addIndexStatement($table)); - } - - // Triggers first, so rows written between here and the backfill are - // stamped by the trigger rather than left behind. - $this->execute(...self::triggerStatements($table)); - $this->execute(self::backfillStatement($table)); + $this->execute(...self::installStatements( + $table, + $this->hasColumn($table, self::COLUMN), + $this->hasIndex($table, self::INDEX), + )); } } @@ -69,6 +68,37 @@ public function uninstall(): void $this->execute(...self::uninstallStatements()); } + /** + * The per-table install sequence, in the order it has to run. + * + * The triggers precede the backfill so a row written between the two is + * stamped by the trigger rather than missed by an UPDATE that has already + * passed it. The index goes last: built any earlier it would index an + * all-NULL column and then have every entry rewritten by the backfill. + * + * The column and index are guarded because installing is also migrating — + * Leantime has no upgrade hook, so this runs again on every install. + * + * @return list + */ + public static function installStatements(string $table, bool $hasColumn, bool $hasIndex): array + { + $statements = []; + + if (!$hasColumn) { + $statements[] = self::addColumnStatement($table); + } + + array_push($statements, ...self::triggerStatements($table)); + $statements[] = self::backfillStatement($table); + + if (!$hasIndex) { + $statements[] = self::addIndexStatement($table); + } + + return $statements; + } + /** * `zp_timesheets` + `insert` → `itk_data_api_timesheets_modified_insert`. */ @@ -90,9 +120,9 @@ public static function addIndexStatement(string $table): string } /** - * Runs on every install, not just the first. On an existing install it is a - * no-op except for rows written while the plugin was uninstalled and the - * triggers were gone — which it heals. + * Stamps the rows the triggers never saw. Runs on every install, not just the + * first, but `IS NULL` keeps it from rewriting timestamps a previous install + * already set and forcing consumers through a second full resync. */ public static function backfillStatement(string $table): string { @@ -158,10 +188,15 @@ public static function uninstallStatements(): array } /** - * Left exactly as they were first shipped: IF NOT EXISTS makes these no-ops - * on every existing install, so changing them here would only make fresh - * databases differ. The `DEFAULT NOW()` on `dateDeleted` is unreachable now - * that the delete triggers set the column themselves. + * The historical baseline, frozen. `CREATE TABLE IF NOT EXISTS` is a bootstrap + * rather than a declaration: a database that already has the table ignores it + * forever, so editing a body here only changes what fresh installs get and lets + * the two populations drift apart silently. Every later change belongs in + * `deletedTableAlterStatements()`, which both populations run. + * + * The one exception is a change that produces an identical column either way — + * `int(11)` lost its display width here, since `int(11)` and `int` are the same + * column and only the former emits an 8.0.17 deprecation. * * @return list */ @@ -169,29 +204,62 @@ public static function deletedTableStatements(): array { return [ 'CREATE TABLE IF NOT EXISTS `itk_projects_deleted` ( - `id` int(11) NOT NULL AUTO_INCREMENT, - `entryId` int(11) DEFAULT NULL, + `id` int NOT NULL AUTO_INCREMENT, + `entryId` int DEFAULT NULL, `dateDeleted` datetime DEFAULT NOW(), PRIMARY KEY (`id`) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci', 'CREATE TABLE IF NOT EXISTS `itk_tickets_deleted` ( - `id` int(11) NOT NULL AUTO_INCREMENT, - `entryId` int(11) DEFAULT NULL, + `id` int NOT NULL AUTO_INCREMENT, + `entryId` int DEFAULT NULL, `type` varchar(255) DEFAULT NULL, `dateDeleted` datetime DEFAULT NOW(), PRIMARY KEY (`id`) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci', 'CREATE TABLE IF NOT EXISTS `itk_timesheets_deleted` ( - `id` int(11) NOT NULL AUTO_INCREMENT, - `entryId` int(11) DEFAULT NULL, + `id` int NOT NULL AUTO_INCREMENT, + `entryId` int DEFAULT NULL, `dateDeleted` datetime DEFAULT NOW(), PRIMARY KEY (`id`) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci', ]; } + /** + * Changes to the deleted tables made after their original CREATE, expressed so + * that existing databases converge on what a fresh install gets. Re-running one + * has to be safe; `hasColumn()`/`hasIndex()` are there for the changes that + * cannot express that in SQL alone. + * + * Dropping `dateDeleted`'s default is the first of them. The delete triggers set + * the column themselves, so `DEFAULT NOW()` is unreachable while they exist — + * but the tables outlive an uninstall and the triggers do not. Without the + * default, a row inserted with no trigger in place gets a NULL instead of the + * session timezone's clock in a column the read side parses as UTC. + * + * @return list + */ + public static function deletedTableAlterStatements(): array + { + return array_map( + static fn (string $table) => sprintf( + 'ALTER TABLE `%s` ALTER COLUMN `dateDeleted` DROP DEFAULT', + $table, + ), + self::deletedTables(), + ); + } + + /** + * @return list + */ + public static function deletedTables(): array + { + return array_values(array_unique(array_column(self::DELETE_TRIGGERS, 1))); + } + /** * Unchanged in name, but re-expressed as single-statement bodies behind a * DROP IF EXISTS. Reinstalling used to throw here because CREATE TRIGGER hit diff --git a/tests/Repository/SchemaRepositoryTest.php b/tests/Repository/SchemaRepositoryTest.php index b19add7..1a190bf 100644 --- a/tests/Repository/SchemaRepositoryTest.php +++ b/tests/Repository/SchemaRepositoryTest.php @@ -154,6 +154,116 @@ public function testEveryTrackedTableGetsTheFullSchema(string $table): void $this->assertCount(4, SchemaRepository::triggerStatements($table)); } + /** + * The order the install sequence runs in is the whole argument for it: the + * triggers have to be in place before the backfill, or a row written between + * the two is missed by an UPDATE that has already passed it, and the index has + * to come after it, or it is built over an all-NULL column and then rewritten + * entry by entry. + */ + public function testTheInstallSequenceStampsBeforeItIndexes(): void + { + $statements = SchemaRepository::installStatements('zp_timesheets', false, false); + + $this->assertSame( + [ + 'ALTER TABLE `zp_timesheets` ADD COLUMN', + 'DROP TRIGGER IF EXISTS', + 'CREATE TRIGGER', + 'DROP TRIGGER IF EXISTS', + 'CREATE TRIGGER', + 'UPDATE `zp_timesheets`', + 'ALTER TABLE `zp_timesheets` ADD INDEX', + ], + array_map($this->kindOf(...), $statements), + ); + } + + /** + * Installation is also migration, so the second install has to skip what the + * first one created — but still re-create the triggers and re-run the backfill, + * which are the parts that heal a partial install. + */ + #[DataProvider('columnAndIndexStates')] + public function testAlreadyInstalledSchemaIsNotAddedTwice(bool $hasColumn, bool $hasIndex): void + { + $statements = SchemaRepository::installStatements('zp_timesheets', $hasColumn, $hasIndex); + + $this->assertSame(!$hasColumn, in_array('ALTER TABLE `zp_timesheets` ADD COLUMN', array_map($this->kindOf(...), $statements), true)); + $this->assertSame(!$hasIndex, in_array('ALTER TABLE `zp_timesheets` ADD INDEX', array_map($this->kindOf(...), $statements), true)); + $this->assertContains(SchemaRepository::backfillStatement('zp_timesheets'), $statements); + + foreach (SchemaRepository::triggerStatements('zp_timesheets') as $statement) { + $this->assertContains($statement, $statements); + } + } + + /** + * @return list + */ + public static function columnAndIndexStates(): array + { + return [[false, false], [true, false], [false, true], [true, true]]; + } + + /** + * `CREATE TABLE IF NOT EXISTS` never reaches a database that already has the + * table, so a change expressed there alone would only ever apply to fresh + * installs. Every deleted table has to be carried by a statement both + * populations run. + */ + public function testEveryDeletedTableColumnDefaultIsDroppedOnExistingInstallsToo(): void + { + $alters = SchemaRepository::deletedTableAlterStatements(); + + $this->assertCount(3, SchemaRepository::deletedTables()); + + foreach (SchemaRepository::deletedTables() as $table) { + $this->assertContains( + sprintf('ALTER TABLE `%s` ALTER COLUMN `dateDeleted` DROP DEFAULT', $table), + $alters, + ); + + $this->assertStringContainsString( + sprintf('CREATE TABLE IF NOT EXISTS `%s`', $table), + implode("\n", SchemaRepository::deletedTableStatements()), + ); + } + } + + /** + * The default writes the session timezone's clock into a column `/deleted` + * parses as UTC. The triggers make it unreachable, but the tables outlive an + * uninstall and the triggers do not. + */ + public function testNoDeletedTableKeepsALocalTimeDefault(): void + { + foreach (SchemaRepository::deletedTableStatements() as $statement) { + if (!str_contains($statement, 'DEFAULT NOW()')) { + continue; + } + + preg_match('/CREATE TABLE IF NOT EXISTS `([^`]+)`/', $statement, $matches); + + $this->assertContains( + sprintf('ALTER TABLE `%s` ALTER COLUMN `dateDeleted` DROP DEFAULT', $matches[1] ?? ''), + SchemaRepository::deletedTableAlterStatements(), + ); + } + } + + /** + * Display width, deprecated since MySQL 8.0.17. Editing it in place is safe + * precisely because `int(11)` and `int` are the same column — the one kind of + * change to the frozen CREATEs that cannot make the two populations differ. + */ + public function testTheDeletedTablesDoNotDeclareADisplayWidth(): void + { + foreach (SchemaRepository::deletedTableStatements() as $statement) { + $this->assertStringNotContainsString('int(11)', $statement); + } + } + /** * Only rows the triggers never saw are stamped, so re-installing does not * rewrite timestamps and force consumers through a second full resync. @@ -221,13 +331,12 @@ private function allInstallStatements(): array { $statements = array_merge( SchemaRepository::deletedTableStatements(), - $this->allTriggerStatements(), + SchemaRepository::deletedTableAlterStatements(), + SchemaRepository::deleteTriggerStatements(), ); foreach (SchemaRepository::TRACKED_TABLES as $table) { - $statements[] = SchemaRepository::addColumnStatement($table); - $statements[] = SchemaRepository::addIndexStatement($table); - $statements[] = SchemaRepository::backfillStatement($table); + array_push($statements, ...SchemaRepository::installStatements($table, false, false)); } return $statements; @@ -239,4 +348,23 @@ private function triggerNameIn(string $statement): string return $matches[1] ?? ''; } + + /** + * A statement reduced to its opening clause, so an ordering assertion reads as + * a sequence rather than as five statements the other tests already pin. + */ + private function kindOf(string $statement): string + { + if (str_starts_with($statement, 'DROP TRIGGER IF EXISTS')) { + return 'DROP TRIGGER IF EXISTS'; + } + + if (str_starts_with($statement, 'CREATE TRIGGER')) { + return 'CREATE TRIGGER'; + } + + preg_match('/^(ALTER TABLE `[^`]+` ADD (?:COLUMN|INDEX)|UPDATE `[^`]+`)/', $statement, $matches); + + return $matches[1] ?? $statement; + } }