Improve display and handling of "not found" errors

Both at the top level and at the database level. It's super awkward that NotFoundInDatabaseException is basically a copy of NotFoundException in a different part of the exception hierarchy but I don't really want the db layer to know about http response codes.
PennyDreadfulMTG · Nov 19, 2024 · 1a45ff5 · 1a45ff5
1 parent 18d8f97
commit 1a45ff5
Show file tree

Hide file tree

Showing 13 changed files with 73 additions and 44 deletions.
diff --git a/gatherling/Data/Db.php b/gatherling/Data/Db.php
@@ -7,6 +7,7 @@
 use Gatherling\Exceptions\ConfigurationException;
 use Gatherling\Exceptions\DatabaseException;
 use Gatherling\Exceptions\MarshalException;
+use Gatherling\Exceptions\NotFoundInDatabaseException;
 use PDOException;
 use PDOStatement;
 use PDO;
@@ -180,7 +181,12 @@ public function select(string $sql, string $class, array $params = []): array
     public function selectOnly(string $sql, string $class, array $params = []): Dto
     {
         $result = $this->select($sql, $class, $params);
-        if (count($result) !== 1) {
+        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);
+        }
+        if (count($result) > 1) {
             throw new DatabaseException('Expected 1 row, got ' . count($result) . " for query: $sql with params " . json_encode($params));
         }
         return $result[0];

diff --git a/gatherling/Exceptions/NotFoundException.php b/gatherling/Exceptions/NotFoundException.php
@@ -8,13 +8,27 @@
 
 class NotFoundException extends BadRequestException
 {
-    public function __construct(string $message = "", int $code = 0, ?Throwable $previous = null)
-    {
+    /** @var list<int|string> */
+    public array $ids;
+
+    /** @param list<int|string|null> $ids */
+    public function __construct(
+        string $message,
+        int $code,
+        ?Throwable $previous,
+        public string $type,
+        array $ids
+    ) {
+        $this->ids = array_values(array_filter($ids, fn($id) => $id !== null));
         parent::__construct($message, $code, $previous, 404);
     }
 
     public function getUserMessage(): string
     {
-        return 'The requested resource was not found';
+        $msg = 'The requested ' . strtolower($this->type);
+        if ($this->ids !== []) {
+            $msg .= " (" . implode(', ', $this->ids) . ")";
+        }
+        return $msg . " was not found";
     }
 }
diff --git a/gatherling/Exceptions/NotFoundInDatabaseException.php b/gatherling/Exceptions/NotFoundInDatabaseException.php
@@ -0,0 +1,21 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Gatherling\Exceptions;
+
+use Throwable;
+
+class NotFoundInDatabaseException extends DatabaseException
+{
+    /** @param list<int|string> $ids */
+    public function __construct(
+        string $message,
+        int $code,
+        ?Throwable $previous,
+        public string $type,
+        public array $ids
+    ) {
+        parent::__construct($message, $code, $previous);
+    }
+}
diff --git a/gatherling/Helpers/ErrorHandler.php b/gatherling/Helpers/ErrorHandler.php
@@ -6,6 +6,8 @@
 
 use Gatherling\Exceptions\BadRequestException;
 use Gatherling\Exceptions\GatherlingException;
+use Gatherling\Exceptions\NotFoundException;
+use Gatherling\Exceptions\NotFoundInDatabaseException;
 use Gatherling\Views\Pages\Error;
 use Throwable;
 
@@ -15,6 +17,11 @@ public function handle(Throwable $e): never
     {
         $requestId = Request::getRequestId();
 
+        if ($e instanceof NotFoundInDatabaseException) {
+            // BAKERT this is weak but it's a start
+            $e = new NotFoundException($e->getMessage(), $e->getCode(), $e, $e->type, $e->ids);
+        }
+
         $message = (string)$e;
 
         if ($e instanceof BadRequestException) {

diff --git a/gatherling/Models/Deck.php b/gatherling/Models/Deck.php
@@ -424,7 +424,7 @@ public function save(): void
             $affectedRows = db()->modify($sql, $params);
             if ($affectedRows != 1) {
                 db()->rollback($transactionName);
-                throw new NotFoundException('Entry for ' . $this->playername . ' in ' . $this->eventname . ' not found');
+                throw new NotFoundException('Entry for ' . $this->playername . ' in ' . $this->eventname . ' not found', 0, null, 'Entry', [$this->playername, $this->eventname]);
             }
         } else {
             $sql = 'UPDATE decks SET archetype = :archetype, name = :name, format = :format, tribe = :tribe, deck_colors = :deck_colors, notes = :notes WHERE id = :id';

diff --git a/gatherling/Models/Entry.php b/gatherling/Models/Entry.php
@@ -29,7 +29,7 @@ public function __construct(int $event_id, string $playername)
         $params = ['event_id' => $event_id, 'player' => $playername];
         $entry = db()->selectOnlyOrNull($sql, EntryDto::class, $params);
         if ($entry == null) {
-            throw new NotFoundException('Entry for ' . $playername . ' in ' . $event_id . ' not found');
+            throw new NotFoundException('Entry for ' . $playername . ' in ' . $event_id . ' not found', 0, null, 'Entry', [$playername, $event_id]);
         }
         $this->medal = $entry->medal;
         $this->drop_round = $entry->drop_round;

diff --git a/gatherling/Models/Player.php b/gatherling/Models/Player.php
@@ -53,11 +53,7 @@ public function __construct(string $name)
               FROM players
              WHERE name = :name';
         $params = ['name' => $name];
-        try {
-            $result = db()->selectOnly($sql, PlayerDto::class, $params);
-        } catch (DatabaseException $e) {
-            throw new NotFoundException("Player $name is not found.", 0, $e);
-        }
+        $result = db()->selectOnly($sql, PlayerDto::class, $params);
         $this->name = $result->name;
         $this->password = $result->password;
         $this->host = $result->host;
@@ -240,7 +236,7 @@ public static function createByName(string $playername): self
 
         $newPlayer = self::findByName($playername);
         if (!$newPlayer) {
-            throw new NotFoundException("Failed to retrieve player we just created: {$playername}");
+            throw new NotFoundException("Failed to retrieve player we just created: {$playername}", 0, null, 'Player', [$playername]);
         }
         return $newPlayer;
     }

diff --git a/gatherling/Views/Components/MatchTable.php b/gatherling/Views/Components/MatchTable.php
@@ -29,8 +29,8 @@ public function __construct(Player $player, string $selectedFormat, string $sele
             }
 
             $opp = $match->otherPlayer($player->name);
-            if (!$opp) {
-                throw new NotFoundException("Opponent not found for match {$match->id}");
+            if ($opp === null) {
+                throw new NotFoundException("Opponent not found for match {$match->id}", 0, null, 'Opponent', [$player->name]);
             }
             $res = 'D';
             if ($match->playerWon($player->name)) {
@@ -43,7 +43,7 @@ public function __construct(Player $player, string $selectedFormat, string $sele
 
             $event = $match->getEvent();
             if (!$event->id) {
-                throw new NotFoundException("Event not found for match {$match->id}");
+                throw new NotFoundException("Event not found for match {$match->id}", 0, null, 'Event', [$match->id]);
             }
             $oppRating = $opponent->getRating('Composite', $event->start);
             $oppDeck = $opponent->getDeckEvent($event->id);

diff --git a/gatherling/Views/Components/Preregistration.php b/gatherling/Views/Components/Preregistration.php
@@ -8,6 +8,7 @@
 use Gatherling\Models\Entry;
 use Gatherling\Models\Event;
 use Gatherling\Models\Player;
+use InvalidArgumentException;
 
 use function Safe\strtotime;
 
@@ -24,7 +25,7 @@ class Preregistration extends Component
     public function __construct(Player $player)
     {
         if (!$player->name) {
-            throw new NotFoundException("Tried to display preregistration for a player with no name");
+            throw new NotFoundException("Tried to display preregistration for a player with no name", 0, null, 'Player', []);
         }
 
         $upcomingEvents = Event::getUpcomingEvents($player->name);
@@ -61,13 +62,14 @@ public function __construct(Player $player)
             }
             $eventLink = $targetUrl . '.php?event=' . rawurlencode($event->name);
             $eventName = $event->name;
-            if (!$event->start || !strtotime($event->start)) {
-                throw new NotFoundException("Event start time not found for event {$event->name}");
+            $eventStart = strtotime($event->start);
+            if (!$eventStart) {
+                throw new InvalidArgumentException("Event start time not found for event {$event->name}");
             }
-            $startingSoon = time() >= strtotime($event->start);
-            $startTime = new Time(strtotime($event->start), $now);
+            $startingSoon = time() >= $eventStart;
+            $startTime = new Time($eventStart, $now);
             if (!$event->id) {
-                throw new NotFoundException("Event ID not found for event {$event->name}");
+                throw new InvalidArgumentException("Event ID not found for event {$event->name}");
             }
             $entry = new Entry($event->id, $player->name);
 
@@ -99,10 +101,11 @@ public function __construct(Player $player)
         foreach ($availableEvents as $event) {
             $eventReportLink = 'eventreport.php?event=' . rawurlencode($event->name);
             $eventName = $event->name;
-            if (!$event->start || !strtotime($event->start)) {
-                throw new NotFoundException("Event start time not found for event {$event->name}");
+            $eventStart = strtotime($event->start);
+            if (!$eventStart) {
+                throw new InvalidArgumentException("Event start time not found for event {$event->name}");
             }
-            $startTime = new Time(strtotime($event->start), time());
+            $startTime = new Time($eventStart, time());
             $isFull = $event->isFull();
             $requiresMtgo = $event->client == 1 && empty($player->mtgo_username);
             $requiresMtga = $event->client == 2 && empty($player->mtga_username);

diff --git a/gatherling/Views/Components/RecentDecksTable.php b/gatherling/Views/Components/RecentDecksTable.php
@@ -7,7 +7,6 @@
 use Gatherling\Models\Entry;
 use Gatherling\Models\Event;
 use Gatherling\Models\Player;
-use Gatherling\Exceptions\NotFoundException;
 
 use function Gatherling\Helpers\logger;
 
@@ -22,9 +21,6 @@ public function __construct(Player $player)
         if (is_null($event)) {
             return;
         }
-        if (!$event->id || !$player->name || !$event->name) {
-            throw new NotFoundException("Seeming invalid event data for ({$event->id}|{$event->name}|{$player->name}");
-        }
         $entry = new Entry($event->id, $player->name);
         if ($entry->deck) {
             $decks = $player->getRecentDecks(6);

diff --git a/gatherling/deck.php b/gatherling/deck.php
@@ -96,7 +96,7 @@ function main(): never
             } elseif ($postMode === 'Update Deck') {
                 $deck = updateDeck($deck, post()->string('archetype'), post()->string('name'), post()->string('notes'), post()->string('contents', ''), post()->string('sideboard', ''));
                 if ($deck->id === null) {
-                    throw new NotFoundException('Trying to update a deck with null id, which is not possible');
+                    throw new InvalidArgumentException('Trying to update a deck with null id, which is not possible');
                 }
                 $deck = new Deck($deck->id); // had to do this to get the constructor to run, otherwise errors weren't loading
                 if ($deck->isValid()) {

diff --git a/gatherling/eventreport.php b/gatherling/eventreport.php
@@ -16,7 +16,7 @@
 function main(): never
 {
     $eventName = get()->optionalString('event') ?? get()->optionalString('name');
-    if ($eventName && Event::exists($eventName)) {
+    if ($eventName !== null && Event::exists($eventName)) {
         $event = new Event($eventName);
         $notYetStarted = db()->bool('SELECT `start` > NOW() AS okay FROM events WHERE `name` = :name', ['name' => $event->name]);
         $canPrereg = $event->prereg_allowed && $notYetStarted;

diff --git a/psalm-baseline.xml b/psalm-baseline.xml
@@ -1767,7 +1767,6 @@
       <code><![CDATA[$rows]]></code>
     </PropertyNotSetInConstructor>
     <RiskyTruthyFalsyComparison>
-      <code><![CDATA[!$opp]]></code>
       <code><![CDATA[$losses]]></code>
       <code><![CDATA[$wins]]></code>
     </RiskyTruthyFalsyComparison>
@@ -1961,14 +1960,6 @@
       <code><![CDATA[!$opp]]></code>
     </RiskyTruthyFalsyComparison>
   </file>
-  <file src="gatherling/Views/Components/SymbolTable.php">
-    <PropertyNotSetInConstructor>
-      <code><![CDATA[$symbols]]></code>
-    </PropertyNotSetInConstructor>
-    <UninitializedProperty>
-      <code><![CDATA[$this->symbols]]></code>
-    </UninitializedProperty>
-  </file>
   <file src="gatherling/Views/Components/TimeZoneDropMenu.php">
     <RiskyTruthyFalsyComparison>
       <code><![CDATA[$selected]]></code>
@@ -2399,11 +2390,6 @@
       <code><![CDATA[Player::loginName()]]></code>
     </RiskyTruthyFalsyComparison>
   </file>
-  <file src="gatherling/eventreport.php">
-    <RiskyTruthyFalsyComparison>
-      <code><![CDATA[$eventName]]></code>
-    </RiskyTruthyFalsyComparison>
-  </file>
   <file src="gatherling/forgot.php">
     <MixedArgument>
       <code><![CDATA[$payload->name]]></code>