From 18074d3594c82788cfef0ed4670146a90cae0402 Mon Sep 17 00:00:00 2001 From: Thomas David Baker Date: Tue, 3 Dec 2024 15:29:17 +0000 Subject: [PATCH 1/4] Make DatabaseException more structured, taking sql and params as arguments --- gatherling/Data/Db.php | 57 +++++++++---------- gatherling/Exceptions/DatabaseException.php | 18 ++++++ .../NotFoundInDatabaseException.php | 10 +++- gatherling/Helpers/Request.php | 2 +- gatherling/Logger.php | 18 +++--- gatherling/Models/Entry.php | 2 +- gatherling/cardscp.php | 5 +- 7 files changed, 67 insertions(+), 45 deletions(-) diff --git a/gatherling/Data/Db.php b/gatherling/Data/Db.php index ce896f29d..59396d12d 100644 --- a/gatherling/Data/Db.php +++ b/gatherling/Data/Db.php @@ -17,7 +17,6 @@ use function Gatherling\Helpers\config; use function Gatherling\Helpers\logger; use function Gatherling\Helpers\marshal; -use function Safe\json_encode; use function Safe\preg_match; use function Safe\preg_replace; @@ -66,7 +65,7 @@ private function connect(bool $connectToDatabase = false): void $this->pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); $this->pdo->exec("SET time_zone = 'America/New_York'"); } catch (PDOException $e) { - throw new DatabaseException('Failed to connect to database: ' . $e->getMessage(), 0, $e); + throw new DatabaseException('Failed to connect to database: ' . $e->getMessage(), $e); } } @@ -80,7 +79,7 @@ private function use(): void try { $this->pdo->exec('USE ' . $this->quoteIdentifier($database)); } catch (PDOException $e) { - throw new DatabaseException('Failed to connect to database: ' . $e->getMessage(), 0, $e); + throw new DatabaseException('Failed to connect to database: ' . $e->getMessage(), $e); } $this->connected = true; return; @@ -117,7 +116,7 @@ public function insert(string $sql, array $params = []): int { $ids = $this->insertMany($sql, $params); if (count($ids) !== 1) { - throw new DatabaseException("Expected 1 id, got " . count($ids) . " for query: $sql with params " . json_encode($params)); + throw new DatabaseException("Expected 1 id, got " . count($ids), null, $sql, $params); } return $ids[0]; } @@ -166,7 +165,7 @@ public function select(string $sql, string $class, array $params = []): array try { $rows = $stmt->fetchAll(PDO::FETCH_CLASS, $class); } catch (TypeError $e) { - throw new DatabaseException("Failed to fetch class $class for query: $sql with params " . json_encode($params), 0, $e); + throw new DatabaseException("Failed to fetch class $class", $e, $sql, $params); } return $rows; }); @@ -184,10 +183,10 @@ public function selectOnly(string $sql, string $class, array $params = []): Dto if (count($result) === 0) { $type = str_replace('Dto', '', basename(str_replace('\\', '/', $class))); $ids = array_values(array_filter($params, fn($v) => is_int($v) || is_string($v))); - throw new NotFoundInDatabaseException("Expected 1 row, got 0 for query: $sql with params " . json_encode($params), 0, null, $type, $ids); + throw new NotFoundInDatabaseException('Expected 1 row, got 0', null, $sql, $params, $type, $ids); } if (count($result) > 1) { - throw new DatabaseException('Expected 1 row, got ' . count($result) . " for query: $sql with params " . json_encode($params)); + throw new DatabaseException('Expected 1 row, got ' . count($result), null, $sql, $params); } return $result[0]; } @@ -202,7 +201,7 @@ public function selectOnlyOrNull(string $sql, string $class, array $params = []) { $result = $this->select($sql, $class, $params); if (count($result) > 1) { - throw new DatabaseException('Expected 1 row, got ' . count($result) . " for query: $sql with params " . json_encode($params)); + throw new DatabaseException('Expected 1 row, got ' . count($result), null, $sql, $params); } return $result[0] ?? null; } @@ -213,7 +212,7 @@ public function int(string $sql, array $params = []): int try { return marshal($this->value($sql, $params))->int(); } catch (MarshalException $e) { - throw new DatabaseException("Expected int value for query: $sql with params " . json_encode($params) . ", got " . $e->getMessage(), 0, $e); + throw new DatabaseException('Expected int value for query, got ' . $e->getMessage(), $e, $sql, $params); } } @@ -223,7 +222,7 @@ public function optionalInt(string $sql, array $params = []): ?int try { return marshal($this->value($sql, $params))->optionalInt(); } catch (MarshalException $e) { - throw new DatabaseException("Expected int value for query: $sql with params " . json_encode($params) . ", got " . $e->getMessage(), 0, $e); + throw new DatabaseException('Expected int value for query, got ' . $e->getMessage(), $e, $sql, $params); } } @@ -233,7 +232,7 @@ public function string(string $sql, array $params = []): string try { return marshal($this->value($sql, $params))->string(); } catch (MarshalException $e) { - throw new DatabaseException("Expected string value for query: $sql with params " . json_encode($params) . ", got " . $e->getMessage(), 0, $e); + throw new DatabaseException('Expected string value for query, got ' . $e->getMessage(), $e, $sql, $params); } } @@ -243,7 +242,7 @@ public function optionalString(string $sql, array $params = []): ?string try { return marshal($this->value($sql, $params))->optionalString(); } catch (MarshalException $e) { - throw new DatabaseException("Expected string value for query: $sql with params " . json_encode($params) . ", got " . $e->getMessage(), 0, $e); + throw new DatabaseException('Expected string value, got ' . $e->getMessage(), $e, $sql, $params); } } @@ -253,7 +252,7 @@ public function float(string $sql, array $params = []): float try { $v = marshal($this->value($sql, $params))->float(); } catch (MarshalException $e) { - throw new DatabaseException("Expected float value for query: $sql with params " . json_encode($params) . ", got " . $e->getMessage(), 0, $e); + throw new DatabaseException('Expected float value for query, got ' . $e->getMessage(), $e, $sql, $params); } return $v; } @@ -264,7 +263,7 @@ public function optionalFloat(string $sql, array $params = []): ?float try { return marshal($this->value($sql, $params))->optionalFloat(); } catch (MarshalException $e) { - throw new DatabaseException("Expected float value for query: $sql with params " . json_encode($params) . ", got " . $e->getMessage(), 0, $e); + throw new DatabaseException('Expected float value for query, got ' . $e->getMessage(), $e, $sql, $params); } } @@ -273,7 +272,7 @@ public function bool(string $sql, array $params = []): bool { $v = $this->optionalInt($sql, $params); if ($v === null) { - throw new DatabaseException("Expected non-null bool value for query: $sql with params " . json_encode($params)); + throw new DatabaseException('Expected non-null bool value for query', null, $sql, $params); } return (bool) $v; } @@ -293,7 +292,7 @@ private function value(string $sql, array $params = []): mixed { $values = $this->values($sql, $params); if (count($values) > 1) { - throw new DatabaseException("Expected 1 value, got " . count($values) . " for query: $sql with params " . json_encode($params)); + throw new DatabaseException('Expected 1 value, got ' . count($values), null, $sql, $params); } if (count($values) === 0) { return null; @@ -310,7 +309,7 @@ public function strings(string $sql, array $params = []): array try { return marshal($this->values($sql, $params))->strings(); } catch (MarshalException $e) { - throw new DatabaseException("Expected strings value for query: $sql with params " . json_encode($params) . ", got " . $e->getMessage(), 0, $e); + throw new DatabaseException('Expected strings value for query, got ' . $e->getMessage(), $e, $sql, $params); } } @@ -323,7 +322,7 @@ public function ints(string $sql, array $params = []): array try { return marshal($this->values($sql, $params))->ints(); } catch (MarshalException $e) { - throw new DatabaseException("Expected ints value for query: $sql with params " . json_encode($params) . ", got " . $e->getMessage(), 0, $e); + throw new DatabaseException('Expected ints value for query, got ' . $e->getMessage(), $e, $sql, $params); } } @@ -334,7 +333,7 @@ public function ints(string $sql, array $params = []): array private function values(string $sql, array $params = []): array { /** @var list */ - return $this->executeInternal($sql, $params, function ($sql, $params) { + return $this->executeInternal($sql, $params, /** @param array $params */ function (string $sql, array $params) { $stmt = $this->pdo->prepare($sql); $this->bindParams($stmt, $params); $stmt->execute(); @@ -353,7 +352,7 @@ public function begin(string $rawName): void try { $this->pdo->beginTransaction(); } catch (PDOException $e) { - throw new DatabaseException("Failed to begin transaction $rawName", 0, $e); + throw new DatabaseException("Failed to begin transaction $rawName", $e); } } else { $this->execute("SAVEPOINT $name"); @@ -373,7 +372,7 @@ public function commit(string $rawName): void try { $this->pdo->rollback(); } catch (PDOException $e) { - throw new DatabaseException("Failed to rollback $latestTransaction while handling mismatch", 0, $e); + throw new DatabaseException("Failed to rollback $latestTransaction while handling mismatch", $e); } throw new DatabaseException("Asked to commit $name, but $latestTransaction is open. ROLLBACK issued."); } @@ -381,7 +380,7 @@ public function commit(string $rawName): void try { $this->pdo->commit(); } catch (PDOException $e) { - throw new DatabaseException("Failed to commit $name", 0, $e); + throw new DatabaseException("Failed to commit $name", $e); } } array_pop($this->transactions); @@ -396,7 +395,7 @@ public function rollback(string $rawName): void try { $this->pdo->rollback(); } catch (PDOException $e) { - throw new DatabaseException("Failed to rollback $name while handling faulty rollback call", 0, $e); + throw new DatabaseException("Failed to rollback $name while handling faulty rollback call", $e); } throw new DatabaseException("Asked to rollback $name, but no transaction is open. ROLLBACK issued."); } @@ -405,7 +404,7 @@ public function rollback(string $rawName): void try { $this->pdo->rollback(); } catch (PDOException $e) { - throw new DatabaseException("Failed to rollback while handling incorrect rollback", 0, $e); + throw new DatabaseException("Failed to rollback while handling incorrect rollback", $e); } throw new DatabaseException("Asked to rollback $name, but $latestTransaction is open. ROLLBACK issued."); } @@ -414,7 +413,7 @@ public function rollback(string $rawName): void try { $this->pdo->rollback(); } catch (PDOException $e) { - throw new DatabaseException("Failed to rollback $name", 0, $e); + throw new DatabaseException("Failed to rollback $name", $e); } } else { $this->execute("ROLLBACK TO SAVEPOINT $name"); // Rollback to the savepoint @@ -441,7 +440,7 @@ public function likeEscape(string $value): string // Debugging code, don't use for anything else, probably bad. // See: https://stackoverflow.com/questions/210564/getting-raw-sql-query-string-from-pdo-prepared-statements, from - /** @param array $params */ + /** @param array $params */ public function interpolateQuery(string $query, array $params): string { $s = chr(2); // Escape sequence for start of placeholder @@ -491,7 +490,7 @@ private function bindParams(PDOStatement $stmt, array $params): void } } } catch (PDOException $e) { - throw new DatabaseException("Failed to bind params " . json_encode($params), 0, $e); + throw new DatabaseException("Failed to bind params", $e, '', $params); } } @@ -556,13 +555,13 @@ private function executeInternal(string $sql, array $params, callable $operation $stmt = $this->pdo->prepare($sql); return $stmt->execute($params); } catch (PDOException $e) { - throw new DatabaseException("Failed to reconnect and execute query: $sql with params " . json_encode($params), 0, $e); + throw new DatabaseException('Failed to reconnect and execute query', $e, $sql, $params); } } $msg = "Failed to execute query: " . $this->interpolateQuery($sql, $params); logger()->error($msg, $context); - throw new DatabaseException($msg, 0, $e); + throw new DatabaseException($msg, $e, $sql, $params); } } diff --git a/gatherling/Exceptions/DatabaseException.php b/gatherling/Exceptions/DatabaseException.php index 11fa966b5..e79b98a31 100644 --- a/gatherling/Exceptions/DatabaseException.php +++ b/gatherling/Exceptions/DatabaseException.php @@ -4,6 +4,24 @@ namespace Gatherling\Exceptions; +use Throwable; + +use function Gatherling\Helpers\db; + class DatabaseException extends GatherlingException { + /** @param array $params */ + public function __construct( + string $baseMessage, + ?Throwable $previous = null, + string $sql = '', + array $params = [], + ) + { + $msg = $baseMessage; + if ($sql) { + $msg .= ' (' . db()->interpolateQuery($sql, $params) . ')'; + } + parent::__construct($msg, 0, $previous); + } } diff --git a/gatherling/Exceptions/NotFoundInDatabaseException.php b/gatherling/Exceptions/NotFoundInDatabaseException.php index 6d874c9dc..4ee4728b8 100644 --- a/gatherling/Exceptions/NotFoundInDatabaseException.php +++ b/gatherling/Exceptions/NotFoundInDatabaseException.php @@ -8,14 +8,18 @@ class NotFoundInDatabaseException extends DatabaseException { - /** @param list $ids */ + /** + * @param list $ids + * @param array $params + */ public function __construct( string $message, - int $code, ?Throwable $previous, + string $sql, + array $params, public string $type, public array $ids ) { - parent::__construct($message, $code, $previous); + parent::__construct($message, $previous, $sql, $params); } } diff --git a/gatherling/Helpers/Request.php b/gatherling/Helpers/Request.php index 98e7081b0..fb5f14d60 100644 --- a/gatherling/Helpers/Request.php +++ b/gatherling/Helpers/Request.php @@ -12,7 +12,7 @@ class Request { private static ?string $requestId = null; - /** @param array $vars */ + /** @param array $vars */ public function __construct(private array $vars) { } diff --git a/gatherling/Logger.php b/gatherling/Logger.php index 82345dc29..4d86d45a5 100644 --- a/gatherling/Logger.php +++ b/gatherling/Logger.php @@ -51,7 +51,7 @@ public function __construct() } /** - * @param array $context + * @param array $context */ public function log(mixed $level, Stringable|string $message, array $context = []): void { @@ -79,7 +79,7 @@ private function normalizeLogLevel(mixed $level): Level } /** - * @param array $context + * @param array $context */ public function emergency(string|Stringable $message, array $context = []): void { @@ -87,7 +87,7 @@ public function emergency(string|Stringable $message, array $context = []): void } /** - * @param array $context + * @param array $context */ public function alert(string|Stringable $message, array $context = []): void { @@ -95,7 +95,7 @@ public function alert(string|Stringable $message, array $context = []): void } /** - * @param array $context + * @param array $context */ public function critical(string|Stringable $message, array $context = []): void { @@ -103,7 +103,7 @@ public function critical(string|Stringable $message, array $context = []): void } /** - * @param array $context + * @param array $context */ public function error(string|Stringable $message, array $context = []): void { @@ -111,7 +111,7 @@ public function error(string|Stringable $message, array $context = []): void } /** - * @param array $context + * @param array $context */ public function warning(string|Stringable $message, array $context = []): void { @@ -119,7 +119,7 @@ public function warning(string|Stringable $message, array $context = []): void } /** - * @param array $context + * @param array $context */ public function notice(string|Stringable $message, array $context = []): void { @@ -127,7 +127,7 @@ public function notice(string|Stringable $message, array $context = []): void } /** - * @param array $context + * @param array $context */ public function info(string|Stringable $message, array $context = []): void { @@ -135,7 +135,7 @@ public function info(string|Stringable $message, array $context = []): void } /** - * @param array $context + * @param array $context */ public function debug(string|Stringable $message, array $context = []): void { diff --git a/gatherling/Models/Entry.php b/gatherling/Models/Entry.php index e45cefe7a..29c564f62 100644 --- a/gatherling/Models/Entry.php +++ b/gatherling/Models/Entry.php @@ -51,7 +51,7 @@ public static function findByEventAndPlayer(int $event_id, string $playername): $sql = 'SELECT player FROM entries WHERE event_id = :event_id AND player = :player'; $params = ['event_id' => $event_id, 'player' => $playername]; $player = db()->optionalString($sql, $params); - if (!$player) { + if ($player === null) { return null; } return new self($event_id, $playername); diff --git a/gatherling/cardscp.php b/gatherling/cardscp.php index f5254ba26..9eef2e645 100644 --- a/gatherling/cardscp.php +++ b/gatherling/cardscp.php @@ -69,9 +69,10 @@ function deleteCards(array $cardIds): void function updateCard(int $cardId, string $name, string $type, string $rarity, string $sfId, bool $isChangeling): void { $db = Database::getConnection(); - $stmt = $db->prepare('UPDATE `cards` SET `name` = ?, `type` = ?, `rarity` = ?, `scryfallId` = ?, `is_changeling` = ? WHERE `id` = ?'); + $sql = 'UPDATE `cards` SET `name` = ?, `type` = ?, `rarity` = ?, `scryfallId` = ?, `is_changeling` = ? WHERE `id` = ?'; + $stmt = $db->prepare($sql); if (!$stmt) { - throw new DatabaseException($db->error); + throw new DatabaseException($db->error, null, $sql); } $stmt->bind_param('ssssdi', $name, $type, $rarity, $sfId, $isChangeling, $cardId); $stmt->execute(); From 369325c0d5bf1cc1470f3e612b19ebcd356cd2b1 Mon Sep 17 00:00:00 2001 From: Thomas David Baker Date: Wed, 4 Dec 2024 11:45:09 +0000 Subject: [PATCH 2/4] Complete conversion of updateCard to new db style This gets rid of the need for exception throwing, improves logging, etc. --- gatherling/cardscp.php | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/gatherling/cardscp.php b/gatherling/cardscp.php index 9eef2e645..0362cb473 100644 --- a/gatherling/cardscp.php +++ b/gatherling/cardscp.php @@ -68,14 +68,16 @@ function deleteCards(array $cardIds): void function updateCard(int $cardId, string $name, string $type, string $rarity, string $sfId, bool $isChangeling): void { - $db = Database::getConnection(); - $sql = 'UPDATE `cards` SET `name` = ?, `type` = ?, `rarity` = ?, `scryfallId` = ?, `is_changeling` = ? WHERE `id` = ?'; - $stmt = $db->prepare($sql); - if (!$stmt) { - throw new DatabaseException($db->error, null, $sql); - } - $stmt->bind_param('ssssdi', $name, $type, $rarity, $sfId, $isChangeling, $cardId); - $stmt->execute(); + $sql = 'UPDATE `cards` SET `name` = :name, `type` = :type, `rarity` = :rarity, `scryfallId` = :sfId, `is_changeling` = :isChangeling WHERE `id` = :id'; + $params = [ + 'name' => $name, + 'type' => $type, + 'rarity' => $rarity, + 'sfId' => $sfId, + 'isChangeling' => $isChangeling, + 'id' => $cardId, + ]; + db()->execute($sql, $params); } if (basename(__FILE__) == basename(server()->string('PHP_SELF'))) { From a748cd7e7fe6098febd74e797f1c8e086889f4af Mon Sep 17 00:00:00 2001 From: Thomas David Baker Date: Wed, 4 Dec 2024 22:22:41 +0000 Subject: [PATCH 3/4] Fix typo in password change code --- gatherling/player.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gatherling/player.php b/gatherling/player.php index 05206d298..3c943f606 100644 --- a/gatherling/player.php +++ b/gatherling/player.php @@ -38,7 +38,7 @@ function main(): never // Handle actions if ($action == 'changePassword') { - $result = changePassword($player, post()->string('oldPasssword', ''), post()->string('newPassword'), post()->string('newPassword2')); + $result = changePassword($player, post()->string('oldPassword', ''), post()->string('newPassword'), post()->string('newPassword2')); } elseif ($action == 'editEmail') { $result = editEmail($player, post()->string('newEmail'), post()->string('newEmail2'), post()->int('emailStatus')); } elseif ($action == 'editAccounts') { From aeaa88b5a86ed4eaf759cd565a1bcef8d40fdc7d Mon Sep 17 00:00:00 2001 From: Thomas David Baker Date: Wed, 4 Dec 2024 22:24:05 +0000 Subject: [PATCH 4/4] Update psalm baseline for recent changes --- psalm-baseline.xml | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 6bd6f100e..5d1a2eedd 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -24,8 +24,6 @@ - - @@ -38,7 +36,6 @@ - @@ -356,11 +353,6 @@ - - - - - @@ -1381,7 +1373,6 @@ - @@ -2238,6 +2229,7 @@ +