Feat/improve error management in core module #8933
Conversation
Add custom class validator to restrict certain subdomains. Integrate the "ForbiddenWords" constraint to prevent illegal subdomain choices like 'demo'. Update workspace service to check default subdomains, enhancing domain management and validation in the application.
AMoreaux
force-pushed
the
feat/improve-error-management-in-core-module
branch
from
2ec5542 to
2821bef
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR introduces comprehensive error handling improvements in the workspace core module, focusing on consistent exception handling and subdomain validation.
- Added
workspaceGraphqlApiExceptionHandler.tsto map workspace exceptions to appropriate GraphQL errors (NotFound, Conflict, Internal) - Implemented new
ForbiddenWordsvalidator to prevent reserved words (e.g. 'demo') in workspace subdomains - Enhanced
UpdateWorkspaceInputwith stricter subdomain validation using regex pattern and forbidden words check - Added environment-aware validation in
WorkspaceServiceto prevent using default subdomains - Standardized error handling in
WorkspaceResolverby wrapping mutations with the new exception handler
8 file(s) reviewed, 8 comment(s)
Edit PR Review Bot Settings | Greptile
| @Matches(/^[a-z0-9][a-z0-9-]{1,28}[a-z0-9]$/) | ||
| @ForbiddenWords(['demo']) |
There was a problem hiding this comment.
style: regex allows 3-30 char subdomains, but error message from ForbiddenWords doesn't mention length restrictions
| expect(() => workspaceGraphqlApiExceptionHandler(error)).toThrow( | ||
| NotFoundError, | ||
| ); |
There was a problem hiding this comment.
style: Consider testing the error message is preserved by checking error.message matches 'Subdomain not found'
| try { | ||
| return this.workspaceService.updateWorkspaceById({ | ||
| ...data, | ||
| id: workspace.id, | ||
| }); | ||
| } catch (error) { | ||
| workspaceGraphqlApiExceptionHandler(error); | ||
| } |
There was a problem hiding this comment.
logic: missing return statement after error handler - could cause undefined to be returned instead of throwing
| import { ForbiddenWords } from 'src/engine/utils/custom-class-validator/ForbiddenWords'; | ||
|
|
||
| describe('ForbiddenWordsConstraint', () => { | ||
| test('should throw error when word is forbidden', async () => { |
There was a problem hiding this comment.
logic: test case missing validation for case sensitivity - 'FORBIDDEN' or 'Forbidden' could potentially pass validation
| test('should pass validation word is not in the list', async () => { | ||
| class Test { | ||
| @ForbiddenWords(['forbidden', 'restricted']) | ||
| subdomain: string; | ||
| } | ||
|
|
||
| const instance = new Test(); | ||
|
|
||
| instance.subdomain = 'valid'; | ||
|
|
||
| const errors = await validate(instance); | ||
|
|
||
| expect(errors.length).toEqual(0); | ||
| }); |
There was a problem hiding this comment.
logic: test case missing validation for partial matches - 'forbidden-word' or 'restricted_name' could contain forbidden words
|
|
||
| @ValidatorConstraint({ async: false }) | ||
| export class ForbiddenWordsConstraint implements ValidatorConstraintInterface { | ||
| private forbiddenWords: Set<string>; |
There was a problem hiding this comment.
style: forbiddenWords Set is recreated on every validation call instead of being initialized once in constructor
|
|
||
| constructor() {} | ||
|
|
||
| validate(value: string, validationArguments: ValidationArguments) { |
There was a problem hiding this comment.
style: value parameter should be typed as unknown since input type cannot be guaranteed
| defaultMessage() { | ||
| return `${Array.from(this.forbiddenWords).join(', ')} are not allowed`; | ||
| } |
There was a problem hiding this comment.
logic: error message includes all forbidden words rather than just the specific word that failed validation - could expose sensitive information
| @@ -13,6 +15,7 @@ export class UpdateWorkspaceInput { | |||
| @IsString() | |||
| @IsOptional() | |||
| @Matches(/^[a-z0-9][a-z0-9-]{1,28}[a-z0-9]$/) | |||
| @ForbiddenWords(['demo']) | |||
There was a problem hiding this comment.
LGTM but I feel this validation should happen at the model level not in the dto which is the graphql input (we could actually justify that they happen in both points)
| if (!subdomainAvailable) { | ||
| throw new ConflictError('Subdomain already taken'); | ||
| if ( | ||
| !subdomainAvailable || |
There was a problem hiding this comment.
here we are adding the check updateWorkspaceById method which is closer to the model layer. This is fine as long as we say that we will always use this method to update a workspace and not use the repository directly.
I feel our model layer is typeorm and an approach like this https://orkhan.gitbook.io/typeorm/docs/validation would be more robust
charlesBochet
deleted the
feat/improve-error-management-in-core-module
branch
|
Thanks @AMoreaux for your contribution! |
Summary
This Pull Request introduces a custom validator for checking forbidden words in workspaces and refines how exceptions are handled within the workspace module.
ForbiddenWordscustom class validator for validating forbidden words against specific fields inUpdateWorkspaceInput.EnvironmentServiceusage inWorkspaceServiceto check default subdomains.workspaceGraphqlApiExceptionHandlerto handle GraphQL API exceptions with specific error mappings.WorkspaceExceptionCodewithSUBDOMAIN_ALREADY_TAKEN.