-
Notifications
You must be signed in to change notification settings - Fork 0
Feature/http api new approach #348
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
6132474
9a622d8
f67dc8c
efce1f9
be015b6
e6fac6a
47dcb8e
bed7f05
a1e1d4f
ed824d1
5c15181
f3999ce
6bd5cbc
0dc1def
5ee193a
9116e17
fe816f8
64912d8
7a86078
3510235
282485d
fece5c1
a256cdc
115207b
e5b8b3d
4e0a22c
e68863b
f00c61b
46619f3
3a9fc09
ae926b4
acb091c
5d0d954
4d6dedc
4582a73
27ad56a
b90041f
d1d9368
5a49d86
16df14d
0f39baf
d36ec70
e2db760
e3a53d6
41f869e
f650381
da1bc68
9723cba
b230a89
65c8c27
2d14d67
357a563
838587d
466f740
b5f4b7e
c7d30aa
d301bbe
4b5d251
6ac90bd
38cc01a
a712bc9
e1a51f3
637fa1a
29160aa
101eac3
8ba9bb6
6d39ea0
cb846ba
2bc6826
3f349d0
76eb05b
da665ca
22cc2c2
439c88b
dc36e1c
1792eb3
5b1dda2
bf626f2
b602287
a4a8b93
93b1f96
8b0757b
efaebe2
6a7294b
f5978f7
bd5722e
7033717
3edeb19
8b95b09
fa3c98e
bb49065
e4dd073
98f37f7
2de692e
5a419f5
c381c9f
0548fbf
af2ba99
bcee5a0
58fb45d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,136 @@ | ||||||||||
<?php | ||||||||||
|
||||||||||
namespace Icinga\Module\Notifications\Controllers; | ||||||||||
|
||||||||||
use Exception; | ||||||||||
use GuzzleHttp\Psr7\Response; | ||||||||||
use GuzzleHttp\Psr7\ServerRequest; | ||||||||||
use Icinga\Exception\Http\HttpBadRequestException; | ||||||||||
use Icinga\Exception\Http\HttpExceptionInterface; | ||||||||||
use Icinga\Exception\Json\JsonEncodeException; | ||||||||||
use Icinga\Module\Notifications\Api\V1\ApiV1; | ||||||||||
use Icinga\Module\Notifications\Api\V1\OpenApi; | ||||||||||
use Icinga\Util\Json; | ||||||||||
use Icinga\Web\Request; | ||||||||||
use ipl\Stdlib\Str; | ||||||||||
use ipl\Web\Compat\CompatController; | ||||||||||
use Psr\Http\Message\ResponseInterface; | ||||||||||
use Throwable; | ||||||||||
use Zend_Controller_Request_Exception; | ||||||||||
|
||||||||||
class ApiController extends CompatController | ||||||||||
{ | ||||||||||
/** | ||||||||||
* Handle API requests and route them to the appropriate endpoint class. | ||||||||||
* | ||||||||||
* Processes API requests for the Notifications module, serving as the main entry point for all API interactions. | ||||||||||
* | ||||||||||
* @return never | ||||||||||
* @throws JsonEncodeException | ||||||||||
*/ | ||||||||||
public function indexAction(): never | ||||||||||
{ | ||||||||||
try { | ||||||||||
$this->assertPermission('notifications/api'); | ||||||||||
|
||||||||||
$request = $this->getRequest(); | ||||||||||
if ( | ||||||||||
! $request->isApiRequest() | ||||||||||
&& strtolower($request->getParam('endpoint')) !== (new OpenApi())->getEndpoint() // for browser query | ||||||||||
Check failure on line 39 in application/controllers/ApiController.php
|
||||||||||
) { | ||||||||||
$this->httpBadRequest('No API request'); | ||||||||||
} | ||||||||||
|
||||||||||
$params = $request->getParams(); | ||||||||||
$version = ucfirst($params['version']); | ||||||||||
Check failure on line 45 in application/controllers/ApiController.php
|
||||||||||
$endpoint = ucfirst(Str::camel($params['endpoint'])); | ||||||||||
Check failure on line 46 in application/controllers/ApiController.php
|
||||||||||
$identifier = $params['identifier'] ?? null; | ||||||||||
|
||||||||||
$module = (($moduleName = $request->getModuleName()) !== null) | ||||||||||
? 'Module\\' . ucfirst($moduleName) . '\\' | ||||||||||
: ''; | ||||||||||
$className = sprintf('Icinga\\%sApi\\%s\\%s', $module, $version, $endpoint); | ||||||||||
|
||||||||||
// TODO: works only for V1 right now | ||||||||||
if (! class_exists($className) || ! is_subclass_of($className, ApiV1::class)) { | ||||||||||
Comment on lines
+54
to
+55
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then I'd suggest to check for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||||||
$this->httpNotFound("Endpoint $endpoint does not exist."); | ||||||||||
} | ||||||||||
|
||||||||||
$serverRequest = (new ServerRequest( | ||||||||||
$request->getMethod(), | ||||||||||
$request->getRequestUri(), | ||||||||||
['Content-Type' => $request->getHeader('Content-Type')], | ||||||||||
Check failure on line 62 in application/controllers/ApiController.php
|
||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. phpstan is on point here. It's not guaranteed at this stage that this header is part of the request, is it? |
||||||||||
serverParams: $request->getServer(), | ||||||||||
)) | ||||||||||
->withParsedBody($this->getRequestBody($request)) | ||||||||||
->withAttribute('identifier', $identifier); | ||||||||||
|
||||||||||
$response = (new $className())->handle($serverRequest); | ||||||||||
} catch (HttpExceptionInterface $e) { | ||||||||||
$response = new Response( | ||||||||||
$e->getStatusCode(), | ||||||||||
array_merge($e->getHeaders(), ['Content-Type' => 'application/json']), | ||||||||||
Check failure on line 72 in application/controllers/ApiController.php
|
||||||||||
Json::sanitize(['message' => $e->getMessage()]) | ||||||||||
); | ||||||||||
} catch (Throwable $e) { | ||||||||||
$response = new Response( | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Log the error:
Suggested change
|
||||||||||
500, | ||||||||||
['Content-Type' => 'application/json'], | ||||||||||
Json::sanitize(['message' => $e->getMessage()]) | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do not use the exception message. Instead, An error occurred. Please check the log |
||||||||||
); | ||||||||||
} finally { | ||||||||||
$this->emitResponse($response); | ||||||||||
Check failure on line 82 in application/controllers/ApiController.php
|
||||||||||
} | ||||||||||
|
||||||||||
exit; | ||||||||||
} | ||||||||||
|
||||||||||
/** | ||||||||||
* Validate that the request has an appropriate body. | ||||||||||
* | ||||||||||
* @param Request $request The request object to validate. | ||||||||||
* | ||||||||||
* @return ?array The validated JSON content as an associative array. | ||||||||||
* | ||||||||||
* @throws HttpBadRequestException | ||||||||||
* @throws Zend_Controller_Request_Exception | ||||||||||
*/ | ||||||||||
private function getRequestBody(Request $request): ?array | ||||||||||
Check failure on line 98 in application/controllers/ApiController.php
|
||||||||||
{ | ||||||||||
if ( | ||||||||||
! preg_match('/([^;]*);?/', $request->getHeader('Content-Type'), $matches) | ||||||||||
Check failure on line 101 in application/controllers/ApiController.php
|
||||||||||
|| $matches[1] !== 'application/json' | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I must ask: Why does this not throw already? Or why does it check for JSON? |
||||||||||
) { | ||||||||||
return null; | ||||||||||
} | ||||||||||
nilmerg marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
|
||||||||||
try { | ||||||||||
$data = $request->getPost(); | ||||||||||
} catch (Exception) { | ||||||||||
$this->httpBadRequest('Invalid request body: given content is not a valid JSON'); | ||||||||||
} | ||||||||||
|
||||||||||
return $data; | ||||||||||
} | ||||||||||
|
||||||||||
/** | ||||||||||
* Emit the HTTP response to the client. | ||||||||||
* | ||||||||||
* @param ResponseInterface $response The response object to emit. | ||||||||||
* | ||||||||||
* @return void | ||||||||||
*/ | ||||||||||
protected function emitResponse(ResponseInterface $response): void | ||||||||||
{ | ||||||||||
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(); | ||||||||||
Comment on lines
+125
to
+134
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prior to this, stop output buffering:
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 |
||||||||||
} | ||||||||||
} |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,123 @@ | ||
<?php | ||
|
||
namespace Icinga\Module\Notifications\Api; | ||
|
||
use GuzzleHttp\Psr7\Response; | ||
use Icinga\Exception\Http\HttpException; | ||
use Icinga\Module\Notifications\Api\Elements\HttpMethod; | ||
use Psr\Http\Message\ResponseInterface; | ||
use Psr\Http\Message\ServerRequestInterface; | ||
use Psr\Http\Message\StreamInterface; | ||
use Psr\Http\Server\RequestHandlerInterface; | ||
use ValueError; | ||
|
||
/** | ||
* Abstract base class for API endpoints. | ||
* | ||
* This class provides the base functionality for handling API requests, | ||
*/ | ||
abstract class ApiCore implements RequestHandlerInterface | ||
{ | ||
/** | ||
* Endpoint based request handling. | ||
* | ||
* @param ServerRequestInterface $request | ||
* | ||
* @return ResponseInterface | ||
*/ | ||
abstract protected function handleRequest(ServerRequestInterface $request): ResponseInterface; | ||
|
||
/** | ||
* Get the name of the API endpoint. | ||
* | ||
* @return string | ||
*/ | ||
abstract public function getEndpoint(): string; | ||
|
||
/** | ||
* The main entry point for processing API requests. | ||
* | ||
* @param ServerRequestInterface $request The incoming server request. | ||
* | ||
* @return ResponseInterface The response generated by the invoked method. | ||
* | ||
* @throws HttpException If the requested method does not exist. | ||
*/ | ||
public function handle(ServerRequestInterface $request): ResponseInterface | ||
{ | ||
try { | ||
$httpMethod = HttpMethod::from(strtolower($request->getMethod())); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎉 (GitHub, allow to react on changed lines!) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Though, you could go a step further: |
||
} catch (ValueError) { | ||
throw (new HttpException(405, sprintf('HTTP method %s is not supported', $request->getMethod()))) | ||
->setHeader('Allow', $this->getAllowedMethods()); | ||
} | ||
|
||
$request = $request->withAttribute('httpMethod', $httpMethod); | ||
nilmerg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
if (! method_exists($this, $httpMethod->lowercase())) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. May I suggest to use |
||
throw (new HttpException( | ||
405, | ||
sprintf('Method %s is not supported for endpoint %s', $httpMethod->uppercase(), $this->getEndpoint()) | ||
)) | ||
->setHeader('Allow', $this->getAllowedMethods()); | ||
} | ||
|
||
$this->assertValidRequest($request); | ||
|
||
return $this->handleRequest($request); | ||
} | ||
|
||
/** | ||
* Validate the incoming request. | ||
* | ||
* Override to implement specific request validation logic. | ||
* | ||
* @param ServerRequestInterface $request The incoming server request to validate. | ||
* | ||
* @return void | ||
*/ | ||
protected function assertValidRequest(ServerRequestInterface $request): void | ||
sukhwinder33445 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
} | ||
sukhwinder33445 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/** | ||
* Get allowed HTTP methods for the API. | ||
* | ||
* @return string | ||
*/ | ||
protected function getAllowedMethods(): string | ||
{ | ||
$methods = []; | ||
|
||
foreach (HttpMethod::cases() as $method) { | ||
if (method_exists($this, $method->lowercase())) { | ||
$methods[] = $method->uppercase(); | ||
} | ||
} | ||
|
||
return implode(', ', $methods); | ||
} | ||
|
||
/** | ||
* Create a Response object. | ||
* | ||
* @param int $status The HTTP status code. | ||
* @param array $headers An associative array of HTTP headers. | ||
* @param ?(StreamInterface|resource|string) $body The response body. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't this be part of the native type? |
||
* @param string $version The HTTP version. | ||
* @param ?string $reason The reason phrase (optional). | ||
* | ||
* @return ResponseInterface | ||
*/ | ||
protected function createResponse( | ||
int $status = 200, | ||
array $headers = [], | ||
$body = null, | ||
string $version = '1.1', | ||
?string $reason = null | ||
): ResponseInterface { | ||
$headers['Content-Type'] = 'application/json'; | ||
|
||
return new Response($status, $headers, $body, $version, $reason); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
<?php | ||
|
||
namespace Icinga\Module\Notifications\Api\Elements; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Elements? Why not inside |
||
|
||
enum HttpMethod: string | ||
{ | ||
case GET = 'get'; | ||
case POST = 'post'; | ||
case PUT = 'put'; | ||
case DELETE = 'delete'; | ||
|
||
|
||
Comment on lines
+11
to
+12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. single empyt lines |
||
/** | ||
* Returns the current enum case as string in uppercase. | ||
* | ||
* @return string | ||
*/ | ||
public function uppercase(): string | ||
{ | ||
return $this->name; | ||
} | ||
|
||
/** | ||
* Returns the current enum case as string in lowercase. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. return type missing |
||
*/ | ||
public function lowercase(): string | ||
{ | ||
return $this->value; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing license header. It seems, not only this file is missing it, please make sure all PHP files have one.