Fixed merge

Author: tuj

CHANGELOG.md

@@ -9,6 +9,14 @@
   * 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.
+  * Accepted comma separated ids, projectIds and types, since the endpoints are documented as GET with query parameters.
+  * Required types on the deleted endpoint, so a bare request answers 400 instead of returning every deleted id ever recorded, and stopped an unknown type reaching the error page.
+  * 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.

Controllers/API.php

@@ -3,6 +3,9 @@
 namespace Leantime\Plugins\APIData\Controllers;
 
 use Leantime\Core\Controller\Controller;
+use Leantime\Plugins\APIData\Model\BadRequestException;
+use Leantime\Plugins\APIData\Model\DeletedRequestParameters;
+use Leantime\Plugins\APIData\Model\RequestParameters;
 use Leantime\Plugins\APIData\Model\ResponseData;
 use Leantime\Plugins\APIData\Services\APIData;
 use Symfony\Component\HttpFoundation\JsonResponse;
@@ -21,78 +24,80 @@ public function init(APIData $dataAPIService): void
 
     public function deleted(array $input): JsonResponse
     {
-        return new JsonResponse($this->getDeleted($input));
+        return $this->respond(fn () => $this->getDeleted($input));
     }
 
     public function projects(array $input): JsonResponse
     {
-        return new JsonResponse($this->getResults($input, APIData::TYPE_PROJECTS));
+        return $this->respond(fn () => $this->getResults($input, APIData::TYPE_PROJECTS));
     }
 
     public function milestones(array $input): JsonResponse
     {
-        return new JsonResponse($this->getResults($input, APIData::TYPE_MILESTONES));
+        return $this->respond(fn () => $this->getResults($input, APIData::TYPE_MILESTONES));
     }
 
     public function tickets(array $input): JsonResponse
     {
-        return new JsonResponse($this->getResults($input, APIData::TYPE_TICKETS));
+        return $this->respond(fn () => $this->getResults($input, APIData::TYPE_TICKETS));
     }
 
     public function timesheets(array $input): JsonResponse
     {
-        return new JsonResponse($this->getResults($input, APIData::TYPE_TIMESHEETS));
+        return $this->respond(fn () => $this->getResults($input, APIData::TYPE_TIMESHEETS));
     }
 
     public function workers(array $input): JsonResponse
     {
-        return new JsonResponse($this->getResults($input, APIData::TYPE_WORKERS));
+        return $this->respond(fn () => $this->getResults($input, APIData::TYPE_WORKERS));
+    }
+
+    /**
+     * A parameter the caller got wrong is their error, not ours, so it answers
+     * 400 with the reason instead of Leantime's 500 error page.
+     */
+    private function respond(callable $resolve): JsonResponse
+    {
+        try {
+            return new JsonResponse($resolve());
+        } catch (BadRequestException $exception) {
+            return new JsonResponse(['error' => $exception->getMessage()], JsonResponse::HTTP_BAD_REQUEST);
+        }
     }
 
     private function getDeleted(array $input): array
     {
-        $types = $input['types'];
-        $deleted = $input['deleted'] ?? null;
+        $parameters = DeletedRequestParameters::fromInput($input);
 
         $deletedEntries = [];
         $count = 0;
 
-        foreach ($types as $type) {
-            $deletedEntries[$type] = $this->dataAPIService->getDeleted($type, $deleted);
+        foreach ($parameters->types as $type) {
+            $deletedEntries[$type] = $this->dataAPIService->getDeleted($type, $parameters->deleted);
             $count = $count + count($deletedEntries[$type]);
         }
 
         return (new ResponseData(
-            ['types' => $types],
+            $parameters->toArray(),
             $count,
             $deletedEntries,
         ))->toArray();
     }
 
     private function getResults(array $input, string $type): array
     {
-        $start = (int) ($input['start'] ?? 0);
-        $limit = (int) ($input['limit'] ?? 100);
-        $modifiedAfter = $input['modifiedAfter'] ?? null;
-        $ids = $input['ids'] ?? null;
-        $projectIds = $input['projectIds'] ?? null;
+        $parameters = RequestParameters::fromInput($input);
 
         $results = match ($type) {
-            APIData::TYPE_PROJECTS => $this->dataAPIService->getProjects($start, $limit, $modifiedAfter, $ids),
-            APIData::TYPE_MILESTONES => $this->dataAPIService->getMilestones($start, $limit, $modifiedAfter, $ids, $projectIds),
-            APIData::TYPE_TICKETS => $this->dataAPIService->getTickets($start, $limit, $modifiedAfter, $ids, $projectIds),
-            APIData::TYPE_TIMESHEETS => $this->dataAPIService->getTimesheets($start, $limit, $modifiedAfter, $ids, $projectIds),
-            APIData::TYPE_WORKERS => $this->dataAPIService->getWorkers($start, $limit, $modifiedAfter, $ids),
+            APIData::TYPE_PROJECTS => $this->dataAPIService->getProjects($parameters->start, $parameters->limit, $parameters->modifiedAfter, $parameters->ids),
+            APIData::TYPE_MILESTONES => $this->dataAPIService->getMilestones($parameters->start, $parameters->limit, $parameters->modifiedAfter, $parameters->ids, $parameters->projectIds),
+            APIData::TYPE_TICKETS => $this->dataAPIService->getTickets($parameters->start, $parameters->limit, $parameters->modifiedAfter, $parameters->ids, $parameters->projectIds),
+            APIData::TYPE_TIMESHEETS => $this->dataAPIService->getTimesheets($parameters->start, $parameters->limit, $parameters->modifiedAfter, $parameters->ids, $parameters->projectIds),
+            APIData::TYPE_WORKERS => $this->dataAPIService->getWorkers($parameters->start, $parameters->limit, $parameters->modifiedAfter, $parameters->ids),
         };
 
         return (new ResponseData(
-            [
-                'start' => $start,
-                'limit' => $limit,
-                'modifiedAfter' => $modifiedAfter,
-                'ids' => $ids,
-                'projectIds' => $projectIds,
-            ],
+            $parameters->toArray(),
             count($results),
             $results,
         ))->toArray();

Model/BadRequestException.php

@@ -0,0 +1,10 @@
+<?php
+
+namespace Leantime\Plugins\APIData\Model;
+
+/**
+ * A request parameter the caller can fix. The controller turns this into a 400,
+ * so the message reaches the client — name the parameter and the expected shape,
+ * never the value that was sent.
+ */
+class BadRequestException extends \InvalidArgumentException {}

Model/CoercesRequestInput.php

@@ -0,0 +1,81 @@
+<?php
+
+namespace Leantime\Plugins\APIData\Model;
+
+/**
+ * Shared coercions for the request parameter objects. Everything arrives as a
+ * string when the endpoints are called with query parameters, so each value has
+ * to be narrowed explicitly rather than cast — a plain (int) cast turns "abc"
+ * into 0, which silently means "from the beginning of time" for a timestamp.
+ */
+trait CoercesRequestInput
+{
+    private static function toNonNegativeInt(mixed $value, string $name): int
+    {
+        if (!is_int($value) && !(is_string($value) && is_numeric($value))) {
+            throw new BadRequestException(sprintf('%s must be a whole number.', $name));
+        }
+
+        if ((float) $value !== (float) (int) $value) {
+            throw new BadRequestException(sprintf('%s must be a whole number.', $name));
+        }
+
+        if ((int) $value < 0) {
+            throw new BadRequestException(sprintf('%s cannot be negative.', $name));
+        }
+
+        return (int) $value;
+    }
+
+    private static function toTimestamp(mixed $value, string $name): ?int
+    {
+        if ($value === null || $value === '') {
+            return null;
+        }
+
+        return self::toNonNegativeInt($value, $name);
+    }
+
+    /**
+     * The README documents GET with query parameters, so a list commonly arrives
+     * as "1,2,3" rather than ids[]=1&ids[]=2. An empty array is kept as an empty
+     * list — the caller asked for a list containing nothing — while an empty
+     * string means the parameter was never really sent.
+     *
+     * @return list<mixed>|null
+     */
+    private static function toList(mixed $value, string $name): ?array
+    {
+        if ($value === null) {
+            return null;
+        }
+
+        if (is_array($value)) {
+            $elements = [];
+
+            foreach ($value as $element) {
+                if (!is_scalar($element)) {
+                    throw new BadRequestException(sprintf('%s must be a list of values.', $name));
+                }
+
+                // Trimmed like the comma separated form, so ?types[]=tickets%20
+                // is not a 400 while ?types=tickets,%20timesheets works. Only
+                // strings, to leave a JSON body's integers as integers.
+                $elements[] = is_string($element) ? trim($element) : $element;
+            }
+
+            return $elements;
+        }
+
+        if (!is_scalar($value)) {
+            throw new BadRequestException(sprintf('%s must be a list of values.', $name));
+        }
+
+        $elements = array_filter(
+            array_map('trim', explode(',', (string) $value)),
+            fn ($element) => $element !== '',
+        );
+
+        return [] === $elements ? null : array_values($elements);
+    }
+}

Model/DeletedRequestParameters.php

@@ -0,0 +1,88 @@
+<?php
+
+namespace Leantime\Plugins\APIData\Model;
+
+use Leantime\Plugins\APIData\Services\APIData;
+
+/**
+ * The parameters for the deleted-entities endpoint. `types` is required: the
+ * endpoint has no limit, so each type returns its whole deletion history, and
+ * defaulting it would let a bare request scan every table. An unknown value used
+ * to reach the repository's match arm, which reflected the raw input into the
+ * error page.
+ */
+readonly class DeletedRequestParameters
+{
+    use CoercesRequestInput;
+
+    /**
+     * @param list<string> $types
+     */
+    public function __construct(
+        public array $types,
+        public ?int $deleted,
+    ) {}
+
+    /**
+     * @param array<string, mixed> $input
+     */
+    public static function fromInput(array $input): self
+    {
+        return new self(
+            types: self::toTypes($input['types'] ?? null),
+            deleted: self::toTimestamp($input['deleted'] ?? null, 'deleted'),
+        );
+    }
+
+    /**
+     * @return array<string, mixed>
+     */
+    public function toArray(): array
+    {
+        return [
+            'types' => $this->types,
+            'deleted' => $this->deleted,
+        ];
+    }
+
+    /**
+     * @return list<string>
+     */
+    public static function supportedTypes(): array
+    {
+        return [
+            APIData::TYPE_PROJECTS,
+            APIData::TYPE_MILESTONES,
+            APIData::TYPE_TICKETS,
+            APIData::TYPE_TIMESHEETS,
+        ];
+    }
+
+    /**
+     * @return list<string>
+     */
+    private static function toTypes(mixed $value): array
+    {
+        $types = self::toList($value, 'types');
+
+        // An empty list is rejected along with a missing one: it would otherwise
+        // answer 200 with nothing, which a caller reads as "nothing was deleted".
+        if ($types === null || $types === []) {
+            throw new BadRequestException(sprintf(
+                'types is required and must contain at least one of: %s.',
+                implode(', ', self::supportedTypes()),
+            ));
+        }
+
+        foreach ($types as $type) {
+            if (!in_array($type, self::supportedTypes(), true)) {
+                throw new BadRequestException(sprintf(
+                    'types must only contain: %s.',
+                    implode(', ', self::supportedTypes()),
+                ));
+            }
+        }
+
+        return array_values(array_unique($types));
+    }
+}