Changed how exceptions are handled in InstantBook #260
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Take a look at using Exception to status
This can potentially save some code in the controller. |
turegjorup
requested changes
Aug 20, 2025
| 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?
src/InteractiveSlide/InstantBook.php
Outdated
| @@ -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.
Suggested change
| 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
src/InteractiveSlide/InstantBook.php
Outdated
| @@ -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.
Suggested change
| throw new InteractiveSlideException('tenantId, clientId, username, password must all be set.', 400); | |
| throw new InteractiveSlideException('tenantId, clientId, username, password must all be set.'); |
src/InteractiveSlide/InstantBook.php
Outdated
| 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.
Suggested change
| throw new InteractiveSlideException('interval not set.', 400); | |
| throw new InteractiveSlideException('interval not set.'); |
src/InteractiveSlide/InstantBook.php
Outdated
| } | ||
|
|
||
| $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.
Suggested change
| throw new InteractiveSlideException("interval.'.$key.' not set.", 400); | |
| throw new InteractiveSlideException("interval.'.$key.' not set."); |
src/InteractiveSlide/InstantBook.php
Outdated
| @@ -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.
Suggested change
| throw new InteractiveSlideException('resourceEndpoint not set', 400); | |
| throw new InteractiveSlideException('resourceEndpoint not set'); |
src/InteractiveSlide/InstantBook.php
Outdated
| } | ||
|
|
||
| $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.
Suggested change
| throw new InteractiveSlideException('resourceEndpoint value not set', 400); | |
| throw new InteractiveSlideException('resourceEndpoint value not set'); |
src/InteractiveSlide/InstantBook.php
Outdated
| @@ -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.
Suggested change
| throw new InteractiveSlideException('Not allowed', 403); | |
| throw new InteractiveSlidePermissionException('Not allowed'); |
Then create InteractiveSlidePermissionException and map it to 403 Forbidden
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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