diff --git a/CHANGELOG.md b/CHANGELOG.md index cfba38c..4a20b26 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,13 @@ ## [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. + * 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-19](https://github.com/ITK-Leantime/data-api/pull/19) * Validated request parameters, so malformed input answers 400 with a reason instead of failing with a 500. * Rejected a limit below 1, which previously dropped the LIMIT clause and returned every row, and capped limit at 1000. @@ -10,7 +17,6 @@ * Fixed an empty projectIds list dropping the filter, which answered with every row instead of none. * Trimmed whitespace around ids, projectIds and types elements sent in array form. * Renamed InvalidRequestException to BadRequestException, matching the 400 it turns into. - * [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 ef1f96f..c562520 100644 --- a/Model/WorkerData.php +++ b/Model/WorkerData.php @@ -2,11 +2,16 @@ namespace Leantime\Plugins\APIData\Model; +use Carbon\CarbonInterface; + readonly class WorkerData { public function __construct( public int $id, public ?string $email, + // getWorkers() maps an all-blank name to null rather than to a string + // of whitespace, so a user with no name at all has none here. public ?string $name, + public ?CarbonInterface $modified, ) {} } diff --git a/README.md b/README.md index 9fccad4..bcd8be2 100644 --- a/README.md +++ b/README.md @@ -4,15 +4,37 @@ An API plugin for exposing data to external applications. Copy the plugin to the folder app/Plugins/APIData, install and enable. -During installation the following tables will be created to tracked deleted entities: +## What installation changes in the database + +The following tables are created to track deleted entities: * itk_projects_deleted * itk_tickets_deleted * itk_timesheets_deleted -3 triggers will also be installed that populate the tables when entities are deleted. +3 triggers populate those tables when entities are deleted. + +An `itk_data_api_modified` column, with an index, is added to `zp_projects`, `zp_tickets`, +`zp_timesheets` and `zp_user`, and 8 more triggers (insert and update, one pair per table) keep it +current. This column exists because Leantime does not maintain its own `modified` column on every write +path — time logged from the weekly grid, for instance, leaves it untouched. Since the triggers sit in the +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**. -NB! The triggers are removed on uninstall, but the tables are left alone to avoid data loss through install/uninstalls. +NB! All 11 triggers are removed on uninstall, but the tables, the column and its data are left alone to +avoid data loss through install/uninstalls. ## Endpoints @@ -25,7 +47,7 @@ The API consists of the following endpoints: GET/POST: `https://{{YOUR_DOMAIN}}/apidata/api/{{TYPE}}` -TYPE: projects, milestones, tickets, timesheets +TYPE: projects, milestones, tickets, timesheets, users Attach query/body parameters to the request: @@ -33,6 +55,7 @@ Attach query/body parameters to the request: * limit: Maximum number of results to get from start id in ascending order. Must be at least 1, and is capped at 1000. The limit that was actually applied is echoed in `parameters`. * modifiedAfter: Only retrieve entries that have a modified later than modifiedAfter (unix timestamp). + All five types, users included, carry a `modified` timestamp in the response. * ids: Array of ids to retrieve. A comma separated string is also accepted, e.g. `?ids=1,2,3`. * projectIds: Array of projectIds. Limits the entities to those attached to projects in projectIds. Only applies for types: milestone, tickets, timesheets. diff --git a/Repositories/ApiDataRepository.php b/Repositories/ApiDataRepository.php index 4510c17..fdfa029 100644 --- a/Repositories/ApiDataRepository.php +++ b/Repositories/ApiDataRepository.php @@ -17,10 +17,10 @@ private function query(): Builder public function getProjects(int $startId, int $limit, ?int $modifiedAfter = null, ?array $ids = null): array { return $this->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) @@ -31,11 +31,11 @@ public function getProjects(int $startId, int $limit, ?int $modifiedAfter = null 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") @@ -47,12 +47,12 @@ public function getMilestones(int $startId, int $limit, ?int $modifiedAfter = nu 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") @@ -86,10 +86,10 @@ public function getWorkers(int $startId, int $limit, ?int $modifiedAfter = null, // CONCAT_WS skips a missing name part, so a worker with only a // firstname keeps a usable name. NULLIF turns an all-blank name into // null rather than a string of whitespace. - ->select(["worker.id", "worker.username", DB::raw("NULLIF(TRIM(CONCAT_WS(' ', worker.firstname, worker.lastname)), '') as name")]) + ->select(["worker.id", "worker.username", DB::raw("NULLIF(TRIM(CONCAT_WS(' ', 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) @@ -111,8 +111,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..549479f --- /dev/null +++ b/Repositories/SchemaRepository.php @@ -0,0 +1,338 @@ + '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::deletedTableAlterStatements()); + $this->execute(...self::deleteTriggerStatements()); + + foreach (self::TRACKED_TABLES as $table) { + $this->execute(...self::installStatements( + $table, + $this->hasColumn($table, self::COLUMN), + $this->hasIndex($table, self::INDEX), + )); + } + } + + 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()); + } + + /** + * 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`. + */ + 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); + } + + /** + * 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 + { + 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; + } + + /** + * 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 + */ + public static function deletedTableStatements(): array + { + return [ + 'CREATE TABLE IF NOT EXISTS `itk_projects_deleted` ( + `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 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 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 + * 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 17b9116..c4f54d4 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 @@ -196,15 +127,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 748b90e..698758f 100644 --- a/phpunit.xml.dist +++ b/phpunit.xml.dist @@ -20,6 +20,7 @@ Model + Repositories Services diff --git a/tests/Model/WorkerDataTest.php b/tests/Model/WorkerDataTest.php index bc41797..b76171e 100644 --- a/tests/Model/WorkerDataTest.php +++ b/tests/Model/WorkerDataTest.php @@ -18,10 +18,11 @@ final class WorkerDataTest extends TestCase */ public function testAcceptsNullNameAndEmail(): void { - $worker = new WorkerData(id: 57, email: null, name: null); + $worker = new WorkerData(id: 57, email: null, name: null, modified: null); $this->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..1a190bf --- /dev/null +++ b/tests/Repository/SchemaRepositoryTest.php @@ -0,0 +1,370 @@ +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)); + } + + /** + * 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. + */ + 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(), + SchemaRepository::deletedTableAlterStatements(), + SchemaRepository::deleteTriggerStatements(), + ); + + foreach (SchemaRepository::TRACKED_TABLES as $table) { + array_push($statements, ...SchemaRepository::installStatements($table, false, false)); + } + + return $statements; + } + + private function triggerNameIn(string $statement): string + { + preg_match('/^CREATE TRIGGER `([^`]+)`/', $statement, $matches); + + 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; + } +} diff --git a/tests/Service/APIDataTest.php b/tests/Service/APIDataTest.php index b301712..72b195c 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->createStub(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,7 +115,7 @@ 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); @@ -138,13 +139,17 @@ public function testGetTicketsLooksUpStatusLabelsOncePerProject(): void $this->ticketRow(['id' => 3, 'projectId' => 93]), ]); - $tickets = (new APIData($ticketRepository, $repository))->getTickets(0, 100); + $tickets = $this->makeService($repository, $ticketRepository)->getTickets(0, 100); $this->assertCount(3, $tickets); $this->assertSame('NEW', $tickets[0]->status); $this->assertSame('NEW', $tickets[1]->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 testGetWorkersMapsEveryFieldToItsOwnProperty(): void { $worker = $this->makeServiceReturningWorkers([$this->workerRow()])->getWorkers(0, 100)[0]; @@ -152,6 +157,10 @@ public function testGetWorkersMapsEveryFieldToItsOwnProperty(): void $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); } /** @@ -161,11 +170,12 @@ public function testGetWorkersMapsEveryFieldToItsOwnProperty(): void public function testGetWorkersMapsABlankNameToNull(): void { $worker = $this->makeServiceReturningWorkers([ - $this->workerRow(['name' => null]), + $this->workerRow(['name' => null, 'modified' => '0000-00-00 00:00:00']), ])->getWorkers(0, 100)[0]; $this->assertNull($worker->name); $this->assertSame('anne@aarhus.dk', $worker->email); + $this->assertNull($worker->modified); } public function testGetDeletedMapsEntryIdAndDeletedDate(): void @@ -201,7 +211,7 @@ private function makeServiceReturningTimesheets(array $rows): APIData $repository = $this->createStub(ApiDataRepository::class); $repository->method('getTimesheets')->willReturn($rows); - return new APIData($this->createStub(TicketRepository::class), $repository); + return $this->makeService($repository); } /** @@ -212,7 +222,7 @@ private function makeServiceReturningWorkers(array $rows): APIData $repository = $this->createStub(ApiDataRepository::class); $repository->method('getWorkers')->willReturn($rows); - return new APIData($this->createStub(TicketRepository::class), $repository); + return $this->makeService($repository); } /** @@ -223,7 +233,16 @@ private function makeServiceReturningDeleted(array $rows): APIData $repository = $this->createStub(ApiDataRepository::class); $repository->method('getDeleted')->willReturn($rows); - return new APIData($this->createStub(TicketRepository::class), $repository); + return $this->makeService($repository); + } + + private function makeService(ApiDataRepository $repository, ?TicketRepository $ticketRepository = null): APIData + { + return new APIData( + $ticketRepository ?? $this->createStub(TicketRepository::class), + $repository, + $this->createStub(SchemaRepository::class), + ); } /** @@ -273,7 +292,8 @@ private function ticketRow(array $overrides = []): object /** * A row as `ApiDataRepository::getWorkers()` returns it. `name` is the - * `CONCAT_WS` expression from the select list, not a column. + * `CONCAT_WS` expression from the select list, not a column, and `modified` + * is the aliased `itk_data_api_modified` column. * * @param array $overrides */ @@ -283,6 +303,7 @@ private function workerRow(array $overrides = []): object 'id' => 57, 'username' => 'anne@aarhus.dk', 'name' => 'Anne Andersen', + 'modified' => '2026-03-03 11:30:00', ], $overrides); }