Changed how exceptions are handled in InstantBook #260
Conversation
|
Take a look at using Exception to status
This can potentially save some code in the controller. |
| try { | ||
| $interactionRequest = $this->interactiveSlideService->parseRequestBody($requestBody); | ||
| } catch (InteractiveSlideException $e) { | ||
| throw new HttpException($e->getCode(), $e->getMessage()); |
There was a problem hiding this comment.
We should use API Platforms Exception to status. That way we have one line of config instead of having to catch the domain exceptions multiple places.
| try { | ||
| $actionResult = $this->interactiveSlideService->performAction($user, $slide, $interactionRequest); | ||
| } catch (InteractiveSlideException $e) { | ||
| throw new HttpException($e->getCode(), $e->getMessage()); |
There was a problem hiding this comment.
Use "Exception to status" as above
|
|
||
| $user = $this->security->getUser(); | ||
|
|
||
| if (!($user instanceof User || $user instanceof ScreenUser)) { | ||
| throw new NotFoundException('User not found'); | ||
| throw new NotFoundHttpException('User not found'); |
There was a problem hiding this comment.
Questions:
- We only have these two user types, so how is this even relevant?
- If it is relevant it seems "Access denied" is the proper exception to throw
- Should this only be allowed for ScreenUsers or are there legitimate use cases for normal users or service accounts?
| @@ -90,7 +88,7 @@ public function performAction(UserInterface $user, Slide $slide, InteractionSlid | |||
| return match ($interactionRequest->action) { | |||
| self::ACTION_GET_QUICK_BOOK_OPTIONS => $this->getQuickBookOptions($slide, $interactionRequest), | |||
| self::ACTION_QUICK_BOOK => $this->quickBook($slide, $interactionRequest), | |||
| default => throw new BadRequestHttpException('Action not allowed'), | |||
| default => throw new InteractiveSlideException('Action not supported', 400), | |||
There was a problem hiding this comment.
| default => throw new InteractiveSlideException('Action not supported', 400), | |
| default => throw new InteractiveSlideException('Action not supported'), |
I don't think domain services should ever implement HTTP specifics. So it's good that we switch from BadRequestHttpException to a custom domain exception. But bad that we retain 400 as the error code given that it's HTTP specific. So without the HTTP context it's just a magic number.
At a minimum use Response::HTTP_BAD_REQUEST constant to indicate context.
Better to not use code and just let the domain exception bubble up, then handle it in config with "exception to status" as described in comment on controller
| @@ -105,7 +103,7 @@ private function authenticate(array $configuration): array | |||
| $password = $this->keyValueService->getValue($configuration['password']); | |||
|
|
|||
| if (4 !== count(array_filter([$tenantId, $clientId, $username, $password]))) { | |||
| throw new BadRequestHttpException('tenantId, clientId, username, password must all be set.'); | |||
| throw new InteractiveSlideException('tenantId, clientId, username, password must all be set.', 400); | |||
There was a problem hiding this comment.
| throw new InteractiveSlideException('tenantId, clientId, username, password must all be set.', 400); | |
| throw new InteractiveSlideException('tenantId, clientId, username, password must all be set.'); |
| private function getValueFromInterval(string $key, InteractionSlideRequest $interactionRequest): string|int | ||
| { | ||
| $interval = $interactionRequest->data['interval'] ?? null; | ||
|
|
||
| if (null === $interval) { | ||
| throw new BadRequestHttpException('interval not set.'); | ||
| throw new InteractiveSlideException('interval not set.', 400); |
There was a problem hiding this comment.
| throw new InteractiveSlideException('interval not set.', 400); | |
| throw new InteractiveSlideException('interval not set.'); |
| } | ||
|
|
||
| $value = $interval[$key] ?? null; | ||
|
|
||
| if (null === $value) { | ||
| throw new BadRequestHttpException("interval.'.$key.' not set."); | ||
| throw new InteractiveSlideException("interval.'.$key.' not set.", 400); |
There was a problem hiding this comment.
| throw new InteractiveSlideException("interval.'.$key.' not set.", 400); | |
| throw new InteractiveSlideException("interval.'.$key.' not set."); |
| @@ -468,13 +501,13 @@ private function getAllowedResources(InteractiveSlide $interactive): array | |||
| $key = $configuration['resourceEndpoint'] ?? null; | |||
|
|
|||
| if (null === $key) { | |||
| throw new \Exception('resourceEndpoint not set'); | |||
| throw new InteractiveSlideException('resourceEndpoint not set', 400); | |||
There was a problem hiding this comment.
| throw new InteractiveSlideException('resourceEndpoint not set', 400); | |
| throw new InteractiveSlideException('resourceEndpoint not set'); |
| } | ||
|
|
||
| $resourceEndpoint = $this->keyValueService->getValue($key); | ||
|
|
||
| if (null === $resourceEndpoint) { | ||
| throw new \Exception('resourceEndpoint value not set'); | ||
| throw new InteractiveSlideException('resourceEndpoint value not set', 400); |
There was a problem hiding this comment.
| throw new InteractiveSlideException('resourceEndpoint value not set', 400); | |
| throw new InteractiveSlideException('resourceEndpoint value not set'); |
| @@ -453,7 +486,7 @@ private function checkPermission(InteractiveSlide $interactive, string $resource | |||
| $allowedResources = $this->getAllowedResources($interactive); | |||
|
|
|||
| if (!in_array($resource, $allowedResources)) { | |||
| throw new \Exception('Not allowed'); | |||
| throw new InteractiveSlideException('Not allowed', 403); | |||
There was a problem hiding this comment.
| throw new InteractiveSlideException('Not allowed', 403); | |
| throw new InteractiveSlidePermissionException('Not allowed'); |
Then create InteractiveSlidePermissionException and map it to 403 Forbidden
Link to issue
#257
Link to ticket
https://leantime.itkdev.dk/#/tickets/showTicket/4992
Description
Exceptions are not handled correctly for the InstantBook feature. This results in 500 errors.
This PR aims to handle the exceptions in a better way.
If an error is encountered when requesting booking options, an empty array of options is returned instead of an exception.
Checklist