Adjust: review suggestions

Author: Jan-Schuppik

application/controllers/ApiController.php

@@ -19,23 +19,23 @@
 class ApiController extends CompatController
 {
     /**
-     * Index action for the API controller.
+     * Handle API requests and route them to the appropriate endpoint class.
      *
-     * This method handles API requests, validates that the request has a valid format,
-     * and dispatches the appropriate endpoint based on the request method and parameters.
+     * This method checks for the required permissions, validates the request,
+     * and routes the request to the appropriate API endpoint class based on the
+     * version and endpoint parameters. It handles exceptions and emits the response.
      *
      * @return never
      */
     public function indexAction(): never
     {
-        // TODO: temporary workaround until we have proper middleware support!!!
         try {
             $this->assertPermission('notifications/api');
 
             $request = $this->getRequest();
             if (
                 ! $request->isApiRequest()
-                && strtolower($request->getParam('endpoint')) !== OpenApi::OPENAPI_ENDPOINT
+                && strtolower($request->getParam('endpoint')) !== (new OpenApi())->getEndpoint()
             ) {
                 $this->httpBadRequest('No API request');
             }
@@ -57,11 +57,11 @@ public function indexAction(): never
             $serverRequest = (new ServerRequest(
                 method: $request->getMethod(),
                 uri: $request->getRequestUri(),
-                serverParams: $request->getServer()
+                headers: ['Content-Type' => $request->getHeader('Content-Type')],
+                serverParams: $request->getServer(),
             ))
                 ->withParsedBody($this->getRequestBody($request))
-                ->withAttribute('identifier', $identifier)
-                ->withHeader('Content-Type', $request->getHeader('Content-Type'));
+                ->withAttribute('identifier', $identifier);
 
             $response = (new $className())->handle($serverRequest);
         } catch (HttpExceptionInterface $e) {
@@ -91,8 +91,10 @@ public function indexAction(): never
      * Validate that the request has a JSON content type and return the parsed JSON content.
      *
      * @param Request $request The request object to validate.
-     * @return ?array The validated JSON content as an associative array, or null if not applicable.
-     * @throws HttpBadRequestException If the request content is not valid JSON.
+     *
+     * @return ?array The validated JSON content as an associative array.
+     *
+     * @throws HttpBadRequestException
      * @throws Zend_Controller_Request_Exception
      */
     private function getRequestBody(Request $request): ?array
@@ -118,6 +120,7 @@ private function getRequestBody(Request $request): ?array
      * Sends the status code, headers, and body of the response to the client.
      *
      * @param ResponseInterface $response The response object to emit.
+     *
      * @return void
      */
     protected function emitResponse(ResponseInterface $response): void

library/Notifications/Api/ApiCore.php

@@ -3,59 +3,43 @@
 namespace Icinga\Module\Notifications\Api;
 
 use GuzzleHttp\Psr7\Response;
-use Icinga\Exception\Http\HttpBadRequestException;
 use Icinga\Exception\Http\HttpException;
 use Icinga\Module\Notifications\Api\Elements\HttpMethod;
-use LogicException;
 use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\ServerRequestInterface;
+use Psr\Http\Message\StreamInterface;
 use Psr\Http\Server\RequestHandlerInterface;
-use Ramsey\Uuid\Uuid;
 use ValueError;
 
 abstract class ApiCore implements RequestHandlerInterface
 {
     /**
-     * Suffix for plural method names.
+     * Handle the API request and return a ResponseInterface object.
      *
-     * This constant is used to differentiate between singular and plural method names
+     * @param ServerRequestInterface $request
      *
-     * @var string
-     */
-    protected const PLURAL_SUFFIX = 'Plural';
-
-    /**
-     * Handle the API request and return a Response object.
-     *
-     * This method should be implemented by subclasses to handle the specific API request
-     * by calling the appropriate methods of the endpoint based on the HTTP method
-     * and return a Response object.
-     *
-     * @return Response
+     * @return ResponseInterface
      */
     abstract protected function handleRequest(ServerRequestInterface $request): ResponseInterface;
 
     /**
-     * Get the endpoint path for the API.
-     *
-     * This method returns the endpoint path for the API, which is defined by the ENDPOINT constant.
-     * If the ENDPOINT constant is not defined in the subclass, a LogicException is thrown.
+     * Get the name of the API endpoint.
      *
      * @return string
-     * @throws LogicException
      */
     abstract public function getEndpoint(): string;
 
     /**
      * Handle the incoming server request and return a response.
      *
-     * This method processes the incoming request, determines the appropriate method to call
-     * based on the HTTP method and presence of an identifier, and invokes that method.
-     * It also handles validation of the identifier and request body for POST and PUT requests.
+     * This method determines the HTTP method of the request and checks if the corresponding
+     * method exists in the subclass. If the method does not exist,
+     * it throws an HttpException with a 405 status code.
      *
      * @param ServerRequestInterface $request The incoming server request.
+     *
      * @return ResponseInterface The response generated by the invoked method.
-     * @throws HttpBadRequestException If the request is not valid.
+     *
      * @throws HttpException If the requested method does not exist.
      */
     public function handle(ServerRequestInterface $request): ResponseInterface
@@ -85,19 +69,16 @@ public function handle(ServerRequestInterface $request): ResponseInterface
      * Validate the incoming request.
      *
      * This method can be overridden in subclasses to implement API- or module-specific
-     * request validation logic. The default implementation checks if the 'identifier'
-     * attribute in the request is a valid UUID, if it is present.
+     * request validation logic. By default, it does nothing.
      *
      * @param ServerRequestInterface $request The incoming server request to validate.
+     *
      * @return void
-     * @throws HttpBadRequestException If the identifier is not a valid UUID.
      */
     protected function assertValidRequest(ServerRequestInterface $request): void
     {
-        // API- / Module-specific request validation can be implemented in this method in child-classes
     }
 
-
     /**
      * Get allowed HTTP methods for the API.
      *
@@ -116,18 +97,14 @@ protected function getAllowedMethods(): string
     }
 
     /**
-     * Create a Response object from the given data array.
+     * Create a Response object.
      *
-     * The data array can contain the following keys:
-     * - 'status': The HTTP status code (default: 200)
-     * - 'headers': An associative array of HTTP headers (default: empty array)
-     * - 'body': The response body as null|StreamInterface|resource|string (default: null)
+     * @param int $status The HTTP status code.
+     * @param array $headers An associative array of HTTP headers.
+     * @param ?(StreamInterface|resource|string) $body The response body.
+     * @param string $version The HTTP version.
+     * @param ?string $reason The reason phrase (optional).
      *
-     * @param int $status
-     * @param array $headers
-     * @param null $body
-     * @param string $version
-     * @param string|null $reason
      * @return Response
      */
     protected function createResponse(

library/Notifications/Api/V1/ApiV1.php

@@ -4,14 +4,12 @@
 
 use Exception;
 use Generator;
-use GuzzleHttp\Psr7\Response;
 use Icinga\Exception\Http\HttpBadRequestException;
 use Icinga\Exception\Http\HttpException;
 use Icinga\Exception\Json\JsonDecodeException;
 use Icinga\Exception\Json\JsonEncodeException;
 use Icinga\Module\Notifications\Api\ApiCore;
 use Icinga\Module\Notifications\Api\Elements\HttpMethod;
-use Icinga\Module\Notifications\Common\Database;
 use Icinga\Util\Json;
 use ipl\Sql\Compat\FilterProcessor;
 use ipl\Sql\Connection;
@@ -22,9 +20,7 @@
 use Psr\Http\Message\ServerRequestInterface;
 use OpenApi\Attributes as OA;
 use Ramsey\Uuid\Uuid;
-use Psr\Http\Server\RequestHandlerInterface;
 use stdClass;
-use ValueError;
 
 /**
  * Base class for API version 1.
@@ -71,8 +67,6 @@
 abstract class ApiV1 extends ApiCore
 {
     /**
-     * API version.
-     *
      * This constant defines the version of the API.
      *
      * @var string
@@ -84,10 +78,12 @@ abstract class ApiV1 extends ApiCore
      *
      * This method processes the incoming request, determines the appropriate method to call
      * based on the HTTP method and presence of an identifier, and invokes that method.
-     * It also handles validation of the identifier and request body for POST and PUT requests.
+     * It also handles validation of the request body for POST and PUT requests.
      *
      * @param ServerRequestInterface $request The incoming server request.
+     *
      * @return ResponseInterface The response generated by the invoked method.
+     *
      * @throws HttpBadRequestException If the request is not valid.
      * @throws HttpException If the requested method does not exist.
      */
@@ -107,6 +103,17 @@ public function handleRequest(ServerRequestInterface $request): ResponseInterfac
         return $this->createResponse(...$responseData);
     }
 
+    /**
+     * Validate the incoming request.
+     *
+     * This method checks the validity of the request based on the HTTP method,
+     * presence of an identifier, and query parameters. It throws an HttpBadRequestException
+     * if any validation fails.
+     *
+     * @param ServerRequestInterface $request The request object to validate.
+     *
+     * @throws HttpBadRequestException If the request is not valid.
+     */
     protected function assertValidRequest(ServerRequestInterface $request): void
     {
         $httpMethod = $request->getAttribute('httpMethod');
@@ -139,61 +146,61 @@ protected function assertValidRequest(ServerRequestInterface $request): void
         }
     }
 
-
-    //TODO: decide if these following functions should be versioned or moved to ApiCore
-
     /**
      * Create a filter from the filter string.
      *
-     * This method parses the filter string and returns an array of filter rules.
-     * If the filter string is empty, it returns false.
+     * @param string $filterStr
+     * @param array $allowedColumns
+     * @param string $idColumnName
      *
-     * @param callable $listener A listener function to handle conditions in the query string.
      * @return array|bool Returns an array of filter rules or false if no filter string is provided.
+     *
      * @throws HttpBadRequestException If the filter string cannot be parsed.
      */
     protected function assembleFilter(string $filterStr, array $allowedColumns, string $idColumnName): array|bool
     {
-        if (! empty($filterStr)) {
-            try {
-                $filterRule = QueryString::fromString($filterStr)
-                    ->on(
-                        QueryString::ON_CONDITION,
-                        function (Condition $condition) use ($allowedColumns, $idColumnName) {
-                            $column = $condition->getColumn();
-                            if (! in_array($column, $allowedColumns)) {
-                                throw new HttpBadRequestException(
-                                    sprintf(
-                                        'Invalid request parameter: Filter column %s given, only %s are allowed',
-                                        $column,
-                                        preg_replace('/,([^,]*)$/', ' and$1', implode(', ', $allowedColumns))
-                                    )
-                                );
-                            }
-
-                            if ($column === 'id') {
-                                if (! Uuid::isValid($condition->getValue())) {
-                                    throw new HttpBadRequestException('The given filter id is not a valid UUID');
-                                }
+        if (empty($filterStr)) {
+            return false;
+        }
+        try {
+            $filterRule = QueryString::fromString($filterStr)
+                ->on(
+                    QueryString::ON_CONDITION,
+                    function (Condition $condition) use ($allowedColumns, $idColumnName) {
+                        $column = $condition->getColumn();
+                        if (! in_array($column, $allowedColumns)) {
+                            throw new HttpBadRequestException(
+                                sprintf(
+                                    'Invalid request parameter: Filter column %s given, only %s are allowed',
+                                    $column,
+                                    preg_replace('/,([^,]*)$/', ' and$1', implode(', ', $allowedColumns))
+                                )
+                            );
+                        }
 
-                                $condition->setColumn($idColumnName);
+                        if ($column === 'id') {
+                            if (! Uuid::isValid($condition->getValue())) {
+                                throw new HttpBadRequestException('The given filter id is not a valid UUID');
                             }
+
+                            $condition->setColumn($idColumnName);
                         }
-                    )->parse();
+                    }
+                )->parse();
 
-                return FilterProcessor::assembleFilter($filterRule);
-            } catch (Exception $e) {
-                throw new HttpBadRequestException($e->getMessage());
-            }
+            return FilterProcessor::assembleFilter($filterRule);
+        } catch (Exception $e) {
+            throw new HttpBadRequestException($e->getMessage());
         }
-        return false;
     }
 
     /**
      * Validate that the request has a JSON content type and return the parsed JSON content.
      *
      * @param ServerRequestInterface $request The request object to validate.
+     *
      * @return array The validated JSON content as an associative array.
+     *
      * @throws HttpBadRequestException If the content type is not application/json.
      */
     private function getValidRequestBody(ServerRequestInterface $request): array
@@ -233,6 +240,7 @@ private function getValidRequestBody(ServerRequestInterface $request): array
      * @param int $batchSize The number of rows to fetch in each batch (default is 500).
      *
      * @return Generator Yields JSON-encoded strings representing the content.
+     *
      * @throws JsonEncodeException
      */
     protected function createContentGenerator(