Feature/http api new approach #348

Jan-Schuppik · 2025-08-22T13:51:53Z

Initial setup for the new approach of the http-api with openapi description.

Resolves:

require:

nilmerg

This review covers only the early parts of routing and dispatching.

run.php

application/controllers/ApiController.php

library/Notifications/Api/V1/ApiV1.php

nilmerg · 2025-08-26T09:14:23Z

Oh, and I got the postgres tests working in my environment. Please get back to me for this.

ICINGA_NOTIFICATIONS_SCHEMA=/path/to/notifications/schema.sql ICINGAWEB_PATH=/icingaweb2 /usr/share/icinga-php/ipl/vendor/bin/phpunit --bootstrap test/php/bootstrap.php Note that you will need a database for this, and propagate this via env as well: * Name | Description * ----------------- | ------------------------ * *_TESTDB | The database to use * *_TESTDB_HOST | The server to connect to * *_TESTDB_PORT | The port to connect to * *_TESTDB_USER | The user to connect with * *_TESTDB_PASSWORD | The password of the user

application/controllers/ApiController.php

library/Notifications/Api/ApiCore.php

application/controllers/ApiController.php

library/Notifications/Api/ApiCore.php

sukhwinder33445 · 2025-09-29T07:26:46Z

library/Notifications/Api/V1/ApiV1.php

+            HttpMethod::POST => $this->post($identifier, $this->getValidRequestBody($request)),
+            HttpMethod::GET => $this->get($identifier, $filterStr),
+            HttpMethod::DELETE => $this->delete($identifier),
+            default => throw new HttpBadRequestException("Invalid request: This case shouldn't be reachable."),


Remove this default case, please.

sukhwinder33445 · 2025-09-29T07:33:15Z

library/Notifications/Api/V1/ApiV1.php

+        }
+        if ($httpMethod === HttpMethod::GET && ! empty($identifier) && ! empty($filterStr)) {
+            throw new HttpBadRequestException(
+                'Invalid request: ' . $httpMethod->uppercase() . ' with identifier and query parameters,'


I would recommend using sprintf() instead.

sukhwinder33445 · 2025-09-29T07:36:16Z

library/Notifications/Api/V1/Channels.php

+                'channel_id' => 'ch.id',
+                'id' => 'ch.external_uuid',


Please correct the indentation.

sukhwinder33445 · 2025-09-29T07:40:54Z

library/Notifications/Api/V1/Channels.php

+     * @throws HttpNotFoundException
+     * @throws JsonEncodeException
+     */
+    public function get(?string $identifier, string $filterStr): array


Please rename the parameter to $queryFilter or $filter. The data type already specifies that it must be a string.

sukhwinder33445 · 2025-09-29T07:41:53Z

library/Notifications/Api/V1/ApiV1.php

+            HttpMethod::PUT => $this->put($identifier, $this->getValidRequestBody($request)),
+            HttpMethod::POST => $this->post($identifier, $this->getValidRequestBody($request)),
+            HttpMethod::GET => $this->get($identifier, $filterStr),
+            HttpMethod::DELETE => $this->delete($identifier),


Please correct the indentation.

sukhwinder33445 · 2025-09-29T08:14:05Z

library/Notifications/Api/V1/Contacts.php

+                'contact_id' => 'co.id',
+                'id' => 'co.external_uuid',
+                'full_name',
+                'username',
+                'default_channel' => 'ch.external_uuid',


Please correct the indentation.

sukhwinder33445 · 2025-09-29T08:14:23Z

library/Notifications/Api/V1/Contacts.php

+            ->joinLeft('contact_address ca', 'ca.contact_id = co.id')
+            ->joinLeft('channel ch', 'ch.id = co.default_channel_id')
+            ->where(['co.deleted = ?' => 'n']);
+        if ($identifier === null) {


Add a new line please.

sukhwinder33445 · 2025-09-29T08:23:36Z

library/Notifications/Api/V1/Contacts.php

+                $this->assertUniqueUsername($requestBody['username'], $contactId);
+            }
+
+            if (! $channelID = Channels::getChannelId($requestBody['default_channel'])) {


Please initialize the variable before the condition, as it is used outside the scope of the condition.

sukhwinder33445 · 2025-09-29T08:25:13Z

library/Notifications/Api/V1/Contacts.php

+                'contact_id'    => $contactId,
+                'type'          => $type,
+                'address'       => $address,
+                'changed_at'            => (int) (new DateTime())->format("Uv"),


Fix the indentation please.

sukhwinder33445 · 2025-09-29T08:41:46Z

library/Notifications/Api/V1/ApiV1.php

+        $identifier = $request->getAttribute('identifier');
+        $filterStr = $request->getUri()->getQuery();
+
+        $responseData = match ($request->getAttribute('httpMethod')) {


I think it's a good idea for the methods to return a ResponseInterface instead of an array.

This reverts commit e4dd073.

nilmerg

Let's discuss how we handle the openapi description and the usage of Swagger, as there are three possible solutions for me:

Caching (properly, not the way it's right now ;) )
CI (as your todo suggests)
Static (it's a versioned api after all)

nilmerg · 2025-10-08T11:02:46Z

application/controllers/ApiController.php

+            // TODO: works only for V1 right now
+            if (! class_exists($className) || ! is_subclass_of($className, ApiV1::class)) {


Then I'd suggest to check for Psr\Http\Server\RequestHandlerInterface instead, given that this is the only assumption this actions makes for this class: being able to call handle and pass it a request.

But I see that we're going in circles here. Earlier, an abstract check for reflection was used instead. Now, ApiV1 implements the interface I mentioned above. Let me propose something in addition: Do not let the base class implement the interface, the actual endpoint classes should implement it.

nilmerg · 2025-10-08T11:18:25Z

application/controllers/ApiController.php

+                Json::sanitize(['message' => $e->getMessage()])
+            );
+        } catch (Throwable $e) {
+            $response = new Response(


Log the error:

Suggested change

$response = new Response(

Icinga\Application\Logger::error($e);

Icinga\Application\Logger::debug(IcingaException::getConfidentialTraceAsString($e));

$response = new Response(

nilmerg · 2025-10-08T11:19:37Z

application/controllers/ApiController.php

+            $response = new Response(
+                500,
+                ['Content-Type' => 'application/json'],
+                Json::sanitize(['message' => $e->getMessage()])


Do not use the exception message. Instead, An error occurred. Please check the log

nilmerg · 2025-10-08T11:25:37Z

application/controllers/ApiController.php

+            $serverRequest = (new ServerRequest(
+                $request->getMethod(),
+                $request->getRequestUri(),
+                ['Content-Type' => $request->getHeader('Content-Type')],


phpstan is on point here. It's not guaranteed at this stage that this header is part of the request, is it?

nilmerg · 2025-10-08T11:32:01Z

application/controllers/ApiController.php

+    {
+        if (
+            ! preg_match('/([^;]*);?/', $request->getHeader('Content-Type'), $matches)
+            || $matches[1] !== 'application/json'


ApiV1::getValidRequestBody() throws in this case

This returns null, effectively preventing any handling of non-JSON

I must ask: Why does this not throw already? Or why does it check for JSON?

nilmerg · 2025-10-08T11:36:27Z

application/controllers/ApiController.php

+        http_response_code($response->getStatusCode());
+
+        foreach ($response->getHeaders() as $name => $values) {
+            foreach ($values as $value) {
+                header(sprintf('%s: %s', $name, $value), false);
+            }
+        }
+        header('Content-Type: application/json');
+
+        echo $response->getBody();


Prior to this, stop output buffering:

do { ob_end_clean(); } while (ob_get_level() > 0);

We're dealing with potentially large responses here and I don't want to really buffer everything twice.

Which brings me to the next suggestion: Don't cast the body to string.

You get a StreamInterface, handle it as such. Didn't look at the endpoints yet, but they have to produce streams, at least those that return large result sets (i.e. plural indexes).

nilmerg · 2025-10-08T11:46:24Z

application/controllers/ApiController.php

Missing license header. It seems, not only this file is missing it, please make sure all PHP files have one.

nilmerg · 2025-10-08T11:52:52Z

application/forms/ContactGroupForm.php

The ContactForm and the ChannelForm also require this change.

nilmerg · 2025-10-08T12:04:37Z

library/Notifications/Common/PsrLogger.php

+     * @param string $message The log message
+     * @param array  $context Additional context variables to interpolate in the message
+     */
+    public function log($level, $message, array $context = []): void


Icinga/icinga-php-thirdparty#47 will be merged soon, please ensure compatibility

nilmerg · 2025-10-08T12:04:57Z

library/Notifications/Common/PsrLogger.php

+        }
+        array_unshift($parsedContext, (string) $message);


empty lines after closing braces

nilmerg · 2025-10-08T12:06:35Z

library/Notifications/Common/PsrLogger.php

+        $icingaMethod = self::MAP[$level] ?? 'debug';
+
+        $parsedContext = [];
+        if (isset($context['exception']) && $context['exception'] instanceof \Throwable) {


Why this? The IcingaLogger handles this as well?

nilmerg · 2025-10-08T12:11:25Z

library/Notifications/Test/BaseApiV1TestCase.php

+
+    public function jsonEncodeResults(array $data): string
+    {
+        return Json::sanitize(['data' => (! empty($data) && ! isset($data[0])) ? [$data] : $data]);


Instead of ! isset maybe ! is_array?

nilmerg · 2025-10-08T12:22:49Z

library/Notifications/Api/ApiCore.php

+
+        $request = $request->withAttribute('httpMethod', $httpMethod);
+
+        if (! method_exists($this, $httpMethod->lowercase())) {


May I suggest to use $this->getAllowedMethods() here instead? Of course, it must return an array for this, which isn't a bad idea either, the string join is only necessary in case of the header definition and doing it then on demand seems fine to me.

nilmerg · 2025-10-08T12:26:13Z

library/Notifications/Api/ApiCore.php

+     *
+     * @param int $status The HTTP status code.
+     * @param array $headers An associative array of HTTP headers.
+     * @param ?(StreamInterface|resource|string) $body The response body.


Can't this be part of the native type?

nilmerg · 2025-10-08T12:34:03Z

library/Notifications/Api/Middlewares/GeneralValidatorMiddleware.php

+     * @throws HttpException
+     * @throws HttpBadRequestException


I already wondered why ApiController catches http exceptions. Here we have a middleware responsible for validation, why isn't it also responsible for error handling? Given the fact that it throws them by itself, instead of producing error responses, it can easily be made responsible to catch errors further down the chain and to respond with fatal errors as well. Removing the need for the try…catch handler in ApiController.

nilmerg · 2025-10-08T12:43:52Z

library/Notifications/Api/Middlewares/GeneralValidatorMiddleware.php

+ * If all validations pass, it adds the validated identifier and parsed request body to the request attributes
+ * and passes the request to the next handler in the middleware chain.
+ */
+class GeneralValidatorMiddleware implements MiddlewareInterface


Just noticed, this isn't used at all. I suppose that's a result of a discussion between me and @sukhwinder33445. Please let's discuss it once more. 🙄

nilmerg · 2025-10-08T12:55:45Z

library/Notifications/Api/V1/ApiV1.php

+     * Create a filter from the filter string.
+     *
+     * @param string $queryFilter
+     * @param array $allowedColumns


Not quite a fan of this to be honest. At the moment it's hardcoded, sure that's not much of a deal due to the versioining, but the schema isn't versioned so that's a moot point. I just wonder where this comes from. Does the OpenAPI specification make any requirements here? It surely becomes easily messy to list all available columns for larger objects. If Icinga DB Web ever has its own API like this, an error message will never ever include all available host columns! What is allowed should of course be described in the OpenAPI document, but the error message must not, IMHO. Please drop this.

nilmerg · 2025-10-09T09:39:05Z

Let's discuss how we handle the openapi description and the usage of Swagger, as there are three possible solutions for me:

Caching (properly, not the way it's right now ;) )

CI (as your todo suggests)

Static (it's a versioned api after all)

As discussed: We should serve static files that need to be prepared each time the description changes. (Read: never, as description changes mean a new version is required)

So please drop the description handling from the ApiController. The description is only available in the repository as part of the documentation. Please add doc/20-REST-API.md, give a short introduction that we have an API, use versioning and that the details are available in api/v1.md. (We will probably make use https://github.com/bharel/mkdocs-render-swagger-plugin in our docs, please refer to this example on how to embed the description.)

Jan-Schuppik requested a review from nilmerg August 22, 2025 13:51

Jan-Schuppik self-assigned this Aug 22, 2025

cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Aug 22, 2025

Jan-Schuppik force-pushed the feature/http-api-new-approach branch from 9a3a6aa to aa2b625 Compare August 25, 2025 12:48

nilmerg requested changes Aug 26, 2025

View reviewed changes

This was referenced Sep 3, 2025

Add column external_uuid to contact/contactgroup table Icinga/icinga-notifications#216

Open

HTTP API to configure contacts and contactgroups #199

Closed

Jan-Schuppik force-pushed the feature/http-api-new-approach branch from f53b374 to 1705125 Compare September 3, 2025 10:49

Jan-Schuppik and others added 17 commits September 3, 2025 12:51

Add: column external_uuid & missing column changed_at

6132474

Introduce: PSR Logger

f67dc8c

Fix: test-setup for MySQL compatibility (PostgreSQL tests still failing)

efce1f9

Adjust: unit tests

be015b6

Add: route for new api controller

e6fac6a

Introduce: new API srtucture (starting with Contact GET)

47dcb8e

Add: initial openapi docs

bed7f05

Adjust: give the request object to ApiCore

a1e1d4f

Fix: lint-errors & moduleName fetching

ed824d1

Remove: Icinga Web 2 BaseTestCase dependency

5c15181

Adjust: add small review suggestions

f3999ce

Rework: implements psr-15 RequestHandlerInterface

6bd5cbc

Adjust: api-version handling

0dc1def

Fix: request validation & plural-GET result name & test permissions

5ee193a

Adjust: openapi documentation creating & storing

9116e17

Fix: Psr-Logger exception-handling

fe816f8

Jan-Schuppik force-pushed the feature/http-api-new-approach branch from 1705125 to fe816f8 Compare September 3, 2025 10:58

Jan-Schuppik added 3 commits September 3, 2025 13:22

Introduce: PUT method handling

64912d8

Adjust: Openapi Attributes (+ fix strings)

7a86078

Introduce: POST method handling

3510235

Fix: Code-Sniffer Errors

22cc2c2

sukhwinder33445 reviewed Sep 26, 2025

View reviewed changes

Jan-Schuppik added 3 commits September 26, 2025 13:15

Adjust: cleanup row preparation for output

439c88b

Adjust: improve the php-docs

dc36e1c

Adjust: cleanup code & set response-header if not provided

1792eb3

sukhwinder33445 requested changes Sep 29, 2025

View reviewed changes

Jan-Schuppik and others added 21 commits September 29, 2025 12:23

Adjust: return response instead of assoc array in endpoint-methods

5b1dda2

Fix: change unsuitable names

bf626f2

Fix: correct indentations

b602287

Adjust: use sprintf() to cleanup strings

a4a8b93

Remove: TODOs

93b1f96

Adjust: identifier-check in post-methods

8b0757b

Fix: scope of variables initialized in if-condition

efaebe2

Fix: add spacing to improve readability

6a7294b

Fix: IDE warnings

f5978f7

Adjust: use database via singleton instead of initialize variables

bd5722e

Fix: exceeding line

7033717

Fix: unit-tests for new database-trait

3edeb19

Fix: scope of a variable

8b95b09

Adjust: make unit-tests independent

fa3c98e

Fix: use getConnection() (need ipl-sql shared-test-databases updated)

bb49065

Fix: unit-test errors caused by databases

e4dd073

Revert "Fix: unit-test errors caused by databases"

98f37f7

This reverts commit e4dd073.

Introduce and use new ApiTestBackends data provider

2de692e

Fix: unit-tests according to new data provider

5a419f5

Fix: Code-Sniffer Errors

c381c9f

Fix: softdelete-blocks-uuid problem

0548fbf

nilmerg requested changes Oct 8, 2025

View reviewed changes

		// TODO: works only for V1 right now
		if (! class_exists($className) \|\| ! is_subclass_of($className, ApiV1::class)) {


		$request = $request->withAttribute('httpMethod', $httpMethod);

		if (! method_exists($this, $httpMethod->lowercase())) {

Feature/http api new approach #348

Are you sure you want to change the base?

Feature/http api new approach #348

Uh oh!

Conversation

Jan-Schuppik commented Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Resolves:

require:

Uh oh!

nilmerg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nilmerg commented Aug 26, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nilmerg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Jan-Schuppik commented Aug 22, 2025 •

edited

Loading