Skip to content
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

Docblock Updates and General Improvements #4

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
/vendor/
composer.lock
8 changes: 7 additions & 1 deletion src/Auth/ApiAuthenticator.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,18 @@ class ApiAuthenticator implements ContextAware, WithContext
use WithContextTrait;
use ContextAwareTrait;

/** @return bool */
public function isAuthenticated(): bool
{
return false;
}

public function hasPermission(ApiPermission ...$permission)
/**
* @param ApiPermission ...$permission
*
* @return bool
*/
public function hasPermission(ApiPermission ...$permission): bool
{
return false;
}
Expand Down
27 changes: 21 additions & 6 deletions src/Controller/ApiController.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,23 +4,39 @@
use Cubex\ApiFoundation\Auth\ApiAuthenticator;
use Cubex\ApiFoundation\Module\Module;
use Cubex\ApiFoundation\Routing\ModuleRoute;
use Generator;
use Packaged\Context\Context;
use Packaged\Http\Responses\JsonResponse;
use Packaged\Routing\Handler\FuncHandler;
use Packaged\Routing\Handler\Handler;
use Packaged\Routing\Route;

abstract class ApiController extends ApiFoundationController
{
const VERSION = '1.0.0';

public function getVersion()
/** @return JsonResponse */
public function getVersion(): JsonResponse
{
return JsonResponse::create(static::VERSION);
}

public function getAuthenticator(Context $context)
/**
* @param Context $context
*
* @return ApiAuthenticator
*/
public function getAuthenticator(Context $context): ApiAuthenticator
{
return ApiAuthenticator::withContext($context);
}

/**
* @param Context $c
* @param $handler
*
* @return array|callable|\Closure|mixed|FuncHandler|Handler|string
*/
protected function _prepareHandler(Context $c, $handler)
{
if($handler instanceof ApiModuleController)
Expand All @@ -30,16 +46,15 @@ protected function _prepareHandler(Context $c, $handler)
return parent::_prepareHandler($c, $handler);
}

/**
* @return \Generator|Module[]
*/
/** @return Generator<Module>|Array<Module> */
abstract protected function _yieldModules();

/** @return Generator<Route> */
protected function _generateRoutes()
{
foreach($this->_yieldModules() as $module)
{
yield ModuleRoute::withModule($module);
yield new ModuleRoute($module);
}
yield self::_route('version', 'version');
}
Expand Down
8 changes: 8 additions & 0 deletions src/Controller/ApiFoundationController.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,18 @@

use Cubex\Controller\Controller;
use Packaged\Context\Context;
use Packaged\Http\Response;
use Packaged\Http\Responses\JsonResponse;

abstract class ApiFoundationController extends Controller
{
/**
* @param Context $c
* @param $result
* @param $buffer
*
* @return mixed|Response
*/
protected function _prepareResponse(Context $c, $result, $buffer = null)
{
if(is_array($result) || is_scalar($result))
Expand Down
21 changes: 10 additions & 11 deletions src/Controller/ApiModuleController.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,19 @@
namespace Cubex\ApiFoundation\Controller;

use Cubex\ApiFoundation\Auth\ApiAuthenticator;
use Cubex\ApiFoundation\Module\Module;
use Cubex\ApiFoundation\Module\Procedures\ProcedureRoute;
use Generator;

class ApiModuleController extends ApiFoundationController
{
/**
* @var Module
*/
protected $_module;
/** @var ApiAuthenticator */
protected ApiAuthenticator $_authenticator;

public function __construct(Module $module)
/** @param Array<ProcedureRoute>|Generator<ProcedureRoute> $_routes */
public function __construct(protected array|Generator $_routes)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should stay as module, otherwise its a breaking change.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the code base this looks like an object that is used internally and I've already updated the reference(https://github.com/cubex/api-foundation/pull/4/files#diff-d08ab29dc7d96cd6c5d5f15f1c75366b9f4167b82a94951330adda2d7bac1ec6R30)?

{
$this->_module = $module;
}

protected $_authenticator;

/**
* @param ApiAuthenticator $authenticator
*
Expand All @@ -29,12 +26,14 @@ public function setAuthenticator(ApiAuthenticator $authenticator)
return $this;
}

/**
* @return Generator<ProcedureRoute>
*/
protected function _generateRoutes()
{
foreach($this->_module->getRoutes() as $route)
foreach($this->_routes as $route)
{
yield $route->setAuthenticator($this->_authenticator);
}
}

}
29 changes: 25 additions & 4 deletions src/Exceptions/InvalidPayloadException.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,36 @@

namespace Cubex\ApiFoundation\Exceptions;

use Throwable;

class InvalidPayloadException extends \Exception
{
/** @var string */
protected $message = 'Invalid payload';

/** @var int */
protected $code = 400;

public static function withType($type): static
/**
* @param string $type
* @param string $message
* @param int $code
* @param Throwable|null $previous
*/
public function __construct(string $type, string $message = "", int $code = 0, ?Throwable $previous = null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't follow the constructor for Exception

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we are looking to add a type I don't see why we want it to follow the exception constructor.

{
parent::__construct($message, $code, $previous);
$this->message .= ' ' . $type;
}

/**
* @deprecated Use the constructor instead
* @param string $type
*
* @return static
*/
public static function withType(string $type)
{
$ex = new static();
$ex->message .= ' ' . $type;
return $ex;
return new static($type);
}
}
74 changes: 40 additions & 34 deletions src/Module/Procedures/ProcedureRoute.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,27 +5,29 @@
use Cubex\ApiFoundation\Exceptions\InvalidPayloadException;
use Cubex\ApiTransport\Endpoints\ApiEndpoint;
use Cubex\ApiTransport\Payloads\AbstractPayload;
use Exception;
use Packaged\Context\Context;
use Packaged\Context\ContextAware;
use Packaged\Http\Responses\JsonResponse;
use Packaged\Routing\Handler\Handler;
use Packaged\Routing\RequestCondition;
use Packaged\Routing\Route;
use ReflectionClass;
use ReflectionException;
use Symfony\Component\HttpFoundation\Response;

class ProcedureRoute extends Route implements Handler
{
/** @var */
protected $_endpoint;
/** @var string */
protected $_procedureClass;

public function __construct(ApiEndpoint $endpoint, string $procedureClass)
/**
* @param ApiEndpoint $_endpoint
* @param class-string<AbstractProcedure> $_procedureClass
*/
public function __construct(protected ApiEndpoint $_endpoint, protected string $_procedureClass)
{
$this->_endpoint = $endpoint;
$this->_procedureClass = $procedureClass;
$this->add(
RequestCondition::i()->path($endpoint->getPath(), RequestCondition::TYPE_EXACT)->method($endpoint->getVerb())
RequestCondition::i()
->path($this->_endpoint->getPath(), RequestCondition::TYPE_EXACT)
->method($this->_endpoint->getVerb())
);
}

Expand All @@ -45,6 +47,11 @@ public function setAuthenticator(ApiAuthenticator $authenticator)
return $this;
}

/**
* @param Context $context
*
* @return bool
*/
public function match(Context $context): bool
{
if(parent::match($context))
Expand All @@ -68,53 +75,52 @@ public function match(Context $context): bool
return false;
}

/** @return $this */
public function getHandler()
{
return $this;
}

/**
* @throws InvalidPayloadException
* @throws InvalidPayloadException|ReflectionException|Exception
*/
public function handle(Context $c): Response
{
$procedure = new $this->_procedureClass();
if(!($procedure instanceof Procedure))
{
throw new \Exception("Invalid procedure class given");
throw new Exception("Invalid procedure class given");
}

if($procedure instanceof ContextAware)
{
$procedure->setContext($c);
}

if(method_exists($procedure, 'execute'))
if(!method_exists($procedure, 'execute'))
{
$plClass = $this->_endpoint->getPayloadClass();
if($plClass !== null)
{
$payload = new $plClass();
if($payload instanceof AbstractPayload)
{
$payload->fromContext($c);
}
return \Packaged\Http\Response::create("Unable to handle procedure: execute missing", 404);
}

if(!$payload->isValid())
{
throw InvalidPayloadException::withType((new \ReflectionClass($payload))->getShortName());
}
$plClass = $this->_endpoint->getPayloadClass();

//Execute with payload
$response = $procedure->execute($payload);
}
else
{
//Execute without payload
$response = $procedure->execute();
}
return JsonResponse::create($response);
if (!$plClass)
{
return JsonResponse::create($procedure->execute());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should avoid creating the JsonResponse twice here.

Its preferred to create the final response in a single line for improved refactoring or modification in the future.

Another option to would be to have a method to convert the response to the JsonResponse. This would give us the option to return as text/xml for different clients, or endpoints.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having an else statement increases code complexity. An early return does look like the better option here.

If we need to modify any of this logic we can override the method.

}
return \Packaged\Http\Response::create("Unable to handle procedure: execute missing", 404);

$payload = new $plClass();
if($payload instanceof AbstractPayload)
{
$payload->fromContext($c);
}

if(!$payload->isValid())
{
throw new InvalidPayloadException((new ReflectionClass($payload))->getShortName());
}

//Execute with payload
return JsonResponse::create($procedure->execute($payload));
}
}
28 changes: 21 additions & 7 deletions src/Routing/ModuleCondition.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,33 @@

class ModuleCondition extends RequestCondition
{
private $_module;
/**
* @param class-string<Module> $_moduleClassName
* @param string $moduleBasePath
*/
public function __construct(private string $_moduleClassName, string $moduleBasePath)
{
$this->path($moduleBasePath);
}

/**
* @deprecated Use the constructor instead
* @param Module $module
*
* @return static
*/
public static function withModule(Module $module)
{
$condition = new static();
$condition->_module = $module;
$condition->path($module::getBasePath());
return $condition;
return new static(get_class($module), $module::getBasePath());
}

/**
* @param Context $context
*
* @return void
*/
public function complete(Context $context)
{
$context->meta()->set('api.module', get_class($this->_module));
$context->meta()->set('api.module', $this->_moduleClassName);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As class names are not case sensitive, sticking with get_class here is safer

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

}

}
20 changes: 14 additions & 6 deletions src/Routing/ModuleRoute.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,26 @@

class ModuleRoute extends Route
{
protected $_module;
/** @param Module $_module */
public function __construct(protected Module $_module)
{
$this->add(new ModuleCondition(get_class($this->_module), $this->_module::getBasePath()));
}

/**
* @deprecated Use the constructor instead
* @param Module $module
*
* @return static
*/
public static function withModule(Module $module)
{
$route = new static();
$route->_module = $module;
$route->add(ModuleCondition::withModule($module));
return $route;
return new static($module);
}

/** @return ApiModuleController */
public function getHandler()
{
return new ApiModuleController($this->_module);
return new ApiModuleController($this->_module->getRoutes());
}
}