From fc4cf35a7430e6f88d5c15fcf8c8fa23c3e5e834 Mon Sep 17 00:00:00 2001 From: pratik Date: Thu, 24 Oct 2024 18:18:29 +0530 Subject: [PATCH 1/2] enhancement for api path params validators and removing rejected api responses (cherry picked from commit 5e71f84a22f4d3ad3b586605ecff2959b8a095e4) --- .../Upgrade/src/api/DTO/ExperimentDTO.ts | 7 +++ .../ExperimentClientController.v5.ts | 2 +- .../api/controllers/ExperimentController.ts | 35 ++++-------- .../controllers/ExperimentUserController.ts | 50 ++++++++++++----- .../api/controllers/FeatureFlagController.ts | 10 +--- .../src/api/controllers/MetricController.ts | 9 ++-- .../api/controllers/PreviewUserController.ts | 54 ++++++++++--------- .../src/api/controllers/SegmentController.ts | 31 +++-------- .../controllers/StratificationController.ts | 23 ++++---- .../src/api/controllers/UserController.ts | 23 ++------ .../validators/ExperimentUserValidator.ts | 6 +++ .../validators/FeatureFlagValidator.ts | 12 +---- .../controllers/validators/MetricValidator.ts | 6 +++ .../validators/PreviewUserValidator.ts | 8 ++- .../validators/SegmentInputValidator.ts | 6 +++ .../validators/StratificationValidator.ts | 6 +++ .../UserPaginatedParamsValidator.ts | 6 +++ .../src/api/services/ExperimentService.ts | 8 ++- .../src/api/services/FeatureFlagService.ts | 15 +++++- .../ConditionPayloads.ts | 4 +- .../ExperimentUserController.test.ts | 12 ++--- .../controllers/FeatureFlagController.test.ts | 4 +- .../mocks/ExperimentClientControllerMock.ts | 9 ++++ .../unit/services/FeatureFlagService.test.ts | 4 +- 24 files changed, 183 insertions(+), 167 deletions(-) create mode 100644 backend/packages/Upgrade/test/unit/controllers/mocks/ExperimentClientControllerMock.ts diff --git a/backend/packages/Upgrade/src/api/DTO/ExperimentDTO.ts b/backend/packages/Upgrade/src/api/DTO/ExperimentDTO.ts index 5d89189088..ab5d60312f 100644 --- a/backend/packages/Upgrade/src/api/DTO/ExperimentDTO.ts +++ b/backend/packages/Upgrade/src/api/DTO/ExperimentDTO.ts @@ -10,6 +10,7 @@ import { IsObject, IsOptional, IsString, + IsUUID, MaxLength, MinLength, ValidateIf, @@ -458,6 +459,12 @@ export class ExperimentDTO { public type: EXPERIMENT_TYPE; } +export class ExperimentIdValidator { + @IsNotEmpty() + @IsUUID() + public id: string; +} + export interface ExperimentFile { fileName: string; fileContent: string; diff --git a/backend/packages/Upgrade/src/api/controllers/ExperimentClientController.v5.ts b/backend/packages/Upgrade/src/api/controllers/ExperimentClientController.v5.ts index 6a1363a879..d5dd8b2dde 100644 --- a/backend/packages/Upgrade/src/api/controllers/ExperimentClientController.v5.ts +++ b/backend/packages/Upgrade/src/api/controllers/ExperimentClientController.v5.ts @@ -106,7 +106,7 @@ export class ExperimentClientController { public metricService: MetricService ) {} - private async checkIfUserExist( + public async checkIfUserExist( userId: string, logger: UpgradeLogger, api?: string diff --git a/backend/packages/Upgrade/src/api/controllers/ExperimentController.ts b/backend/packages/Upgrade/src/api/controllers/ExperimentController.ts index 8d66e2bd19..20bc1b89ce 100644 --- a/backend/packages/Upgrade/src/api/controllers/ExperimentController.ts +++ b/backend/packages/Upgrade/src/api/controllers/ExperimentController.ts @@ -3,7 +3,6 @@ import { Get, JsonController, OnUndefined, - Param, Post, Put, Delete, @@ -17,8 +16,6 @@ import { Experiment } from '../models/Experiment'; import { ExperimentNotFoundError } from '../errors/ExperimentNotFoundError'; import { ExperimentService } from '../services/ExperimentService'; import { ExperimentAssignmentService } from '../services/ExperimentAssignmentService'; -import { SERVER_ERROR } from 'upgrade_types'; -import { isUUID } from 'class-validator'; import { ExperimentCondition } from '../models/ExperimentCondition'; import { ExperimentPaginatedParamsValidator } from './validators/ExperimentPaginatedParamsValidator'; import { UserDTO } from '../DTO/UserDTO'; @@ -28,7 +25,7 @@ import { AppRequest, PaginationResponse } from '../../types'; import { ExperimentDTO, ExperimentFile, ValidatedExperimentError } from '../DTO/ExperimentDTO'; import { ExperimentIds } from './validators/ExperimentIdsValidator'; import { NotFoundException } from '@nestjs/common/exceptions'; -import { IdValidator } from './validators/FeatureFlagValidator'; +import { ExperimentIdValidator } from '../DTO/ExperimentDTO'; interface ExperimentPaginationInfo extends PaginationResponse { nodes: Experiment[]; @@ -634,7 +631,7 @@ export class ExperimentController { * description: AuthorizationRequiredError */ @Get() - public find(@Req() request: AppRequest): Promise { + public find(@Req() request: AppRequest): Promise { return this.experimentService.find(request.logger); } @@ -743,14 +740,6 @@ export class ExperimentController { @Req() request: AppRequest ): Promise { - if (!paginatedParams) { - return Promise.reject( - new Error( - JSON.stringify({ type: SERVER_ERROR.MISSING_PARAMS, message: ' : paginatedParams should not be null.' }) - ) - ); - } - const [experiments, count] = await Promise.all([ this.experimentService.findPaginated( paginatedParams.skip, @@ -841,7 +830,10 @@ export class ExperimentController { */ @Get('/single/:id') @OnUndefined(ExperimentNotFoundError) - public one(@Params({ validate: true }) { id }: IdValidator, @Req() request: AppRequest): Promise { + public one( + @Params({ validate: true }) { id }: ExperimentIdValidator, + @Req() request: AppRequest + ): Promise { return this.experimentService.getSingleExperiment(id, request.logger); } @@ -920,7 +912,7 @@ export class ExperimentController { @Get('/conditions/:id') @OnUndefined(ExperimentNotFoundError) public async getCondition( - @Params({ validate: true }) { id }: IdValidator, + @Params({ validate: true }) { id }: ExperimentIdValidator, @Req() request: AppRequest ): Promise { return this.experimentService.getExperimentalConditions(id, request.logger); @@ -1042,7 +1034,7 @@ export class ExperimentController { @Delete('/:id') public async delete( - @Params({ validate: true }) { id }: IdValidator, + @Params({ validate: true }) { id }: ExperimentIdValidator, @CurrentUser() currentUser: UserDTO, @Req() request: AppRequest ): Promise { @@ -1140,19 +1132,12 @@ export class ExperimentController { */ @Put('/:id') public update( - @Param('id') id: string, + @Params({ validate: true }) { id }: ExperimentIdValidator, @Body({ validate: true }) experiment: ExperimentDTO, @CurrentUser() currentUser: UserDTO, @Req() request: AppRequest ): Promise { - if (!isUUID(id)) { - return Promise.reject( - new Error( - JSON.stringify({ type: SERVER_ERROR.INCORRECT_PARAM_FORMAT, message: ' : id should be of type UUID.' }) - ) - ); - } request.logger.child({ user: currentUser }); return this.experimentService.update(experiment, currentUser, request.logger); } @@ -1345,7 +1330,7 @@ export class ExperimentController { @Get('/getGroupAssignmentStatus/:id') @OnUndefined(ExperimentNotFoundError) public async getGroupAssignmentStatus( - @Params({ validate: true }) { id }: IdValidator, + @Params({ validate: true }) { id }: ExperimentIdValidator, @Req() request: AppRequest ): Promise | undefined { return this.experimentAssignmentService.getGroupAssignmentStatus(id, request.logger); diff --git a/backend/packages/Upgrade/src/api/controllers/ExperimentUserController.ts b/backend/packages/Upgrade/src/api/controllers/ExperimentUserController.ts index 1a2ccf8d01..962cf804c9 100644 --- a/backend/packages/Upgrade/src/api/controllers/ExperimentUserController.ts +++ b/backend/packages/Upgrade/src/api/controllers/ExperimentUserController.ts @@ -1,12 +1,14 @@ -import { JsonController, Req, Get, OnUndefined, Param, Post, Put, Body, Authorized } from 'routing-controllers'; +import { JsonController, Req, Get, Post, Put, Body, Authorized, Params } from 'routing-controllers'; import { ExperimentUserService } from '../services/ExperimentUserService'; import { ExperimentUser } from '../models/ExperimentUser'; -import { UserNotFoundError } from '../errors/UserNotFoundError'; -import { SERVER_ERROR } from 'upgrade_types'; -import { isUUID } from 'class-validator'; import { AppRequest } from '../../types'; -import { ExperimentUserArrayValidator, ExperimentUserValidator } from './validators/ExperimentUserValidator'; +import { + ExperimentUserArrayValidator, + ExperimentUserValidator, + IdValidator, +} from './validators/ExperimentUserValidator'; import { InsertResult } from 'typeorm'; +import { ExperimentClientController } from './ExperimentClientController.v5'; // TODO delete this from experiment system /** @@ -34,7 +36,10 @@ import { InsertResult } from 'typeorm'; @Authorized() @JsonController('/experimentusers') export class UserController { - constructor(public userService: ExperimentUserService) {} + constructor( + public userService: ExperimentUserService, + public experimentClientController: ExperimentClientController + ) {} /** * @swagger @@ -46,6 +51,8 @@ export class UserController { * responses: * '200': * description: Successful + * '401': + * description: AuthorizationRequiredError */ @Get() public find(@Req() request: AppRequest): Promise { @@ -71,15 +78,19 @@ export class UserController { * responses: * '200': * description: Get user By Id + * '400': + * description: BadRequestError - InvalidParameterValue + * '401': + * description: AuthorizationRequiredError * '404': - * description: user not found + * description: Experiment User not defined */ @Get('/:id') - @OnUndefined(UserNotFoundError) - public one(@Param('id') id: string, @Req() request: AppRequest): Promise { - if (!isUUID(id)) { - return Promise.reject(new Error(SERVER_ERROR.INCORRECT_PARAM_FORMAT + ' : id should be of type UUID.')); - } + public async one( + @Params({ validate: true }) { id }: IdValidator, + @Req() request: AppRequest + ): Promise { + await this.experimentClientController.checkIfUserExist(id, request.logger); return this.userService.findOne(id, request.logger); } @@ -105,6 +116,10 @@ export class UserController { * responses: * '200': * description: New ExperimentUser is created + * '400': + * description: BadRequestError - InvalidParameterValue + * '401': + * description: AuthorizationRequiredError */ @Post() public create( @@ -143,13 +158,20 @@ export class UserController { * responses: * '200': * description: ExperimentUser is updated + * '400': + * description: BadRequestError - InvalidParameterValue + * '401': + * description: AuthorizationRequiredError + * '404': + * description: Experiment User not defined */ @Put('/:id') - public update( - @Param('id') id: string, + public async update( + @Params({ validate: true }) { id }: IdValidator, @Body({ validate: true }) user: ExperimentUserValidator, @Req() request: AppRequest ): Promise { + await this.experimentClientController.checkIfUserExist(id, request.logger); return this.userService.update(id, user, request.logger); } } diff --git a/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts b/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts index 9fcd46cc8c..f75a1dfacd 100644 --- a/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts +++ b/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts @@ -251,14 +251,6 @@ export class FeatureFlagsController { paginatedParams: FeatureFlagPaginatedParamsValidator, @Req() request: AppRequest ): Promise { - if (!paginatedParams) { - return Promise.reject( - new Error( - JSON.stringify({ type: SERVER_ERROR.MISSING_PARAMS, message: ' : paginatedParams should not be null.' }) - ) - ); - } - const [featureFlags, count] = await Promise.all([ this.featureFlagService.findPaginated( paginatedParams.skip, @@ -453,7 +445,7 @@ export class FeatureFlagsController { @CurrentUser() currentUser: UserDTO, @Req() request: AppRequest ): Promise { - return this.featureFlagService.update(flag, currentUser, request.logger); + return this.featureFlagService.update(flag, id, currentUser, request.logger); } /** diff --git a/backend/packages/Upgrade/src/api/controllers/MetricController.ts b/backend/packages/Upgrade/src/api/controllers/MetricController.ts index e5c1db391d..40f7a019e7 100644 --- a/backend/packages/Upgrade/src/api/controllers/MetricController.ts +++ b/backend/packages/Upgrade/src/api/controllers/MetricController.ts @@ -1,8 +1,8 @@ -import { Authorized, JsonController, Get, Delete, Param, Post, Req, Body, Params } from 'routing-controllers'; +import { Authorized, JsonController, Get, Delete, Post, Req, Body, Params } from 'routing-controllers'; import { MetricService } from '../services/MetricService'; import { IMetricUnit } from 'upgrade_types'; import { AppRequest } from '../../types'; -import { MetricKeyValidator, MetricValidator } from './validators/MetricValidator'; +import { ContextValidator, MetricKeyValidator, MetricValidator } from './validators/MetricValidator'; /** * @swagger @@ -56,7 +56,10 @@ export class MetricController { * description: Metrics Context not found */ @Get('/:context') - public getMetricsByContext(@Param('context') context: string, @Req() request: AppRequest): Promise { + public getMetricsByContext( + @Params({ validate: true }) { context }: ContextValidator, + @Req() request: AppRequest + ): Promise { return this.metricService.getMetricsByContext(context, request.logger); } diff --git a/backend/packages/Upgrade/src/api/controllers/PreviewUserController.ts b/backend/packages/Upgrade/src/api/controllers/PreviewUserController.ts index 5ea80fafb2..0251e05456 100644 --- a/backend/packages/Upgrade/src/api/controllers/PreviewUserController.ts +++ b/backend/packages/Upgrade/src/api/controllers/PreviewUserController.ts @@ -1,12 +1,21 @@ -import { JsonController, Get, OnUndefined, Param, Post, Put, Body, Authorized, Delete, Req } from 'routing-controllers'; +import { + JsonController, + Get, + OnUndefined, + Post, + Put, + Body, + Authorized, + Delete, + Req, + Params, +} from 'routing-controllers'; import { UserNotFoundError } from '../errors/UserNotFoundError'; -import { SERVER_ERROR } from 'upgrade_types'; import { PreviewUserService } from '../services/PreviewUserService'; import { PreviewUser } from '../models/PreviewUser'; -import { isString } from 'class-validator'; import { PaginatedParamsValidator } from './validators/PaginatedParamsValidator'; import { AppRequest, PaginationResponse } from '../../types'; -import { PreviewUserValidator } from './validators/PreviewUserValidator'; +import { IdValidator, PreviewUserValidator } from './validators/PreviewUserValidator'; interface PreviewUserPaginationInfo extends PaginationResponse { nodes: PreviewUser[]; @@ -83,14 +92,6 @@ export class PreviewUserController { @Body({ validate: true }) paginatedParams: PaginatedParamsValidator, @Req() request: AppRequest ): Promise { - if (!paginatedParams) { - return Promise.reject( - new Error( - JSON.stringify({ type: SERVER_ERROR.MISSING_PARAMS, message: ' : paginatedParams should not be null.' }) - ) - ); - } - const [previewUsers, count] = await Promise.all([ this.previewUserService.findPaginated(paginatedParams.skip, paginatedParams.take, request.logger), this.previewUserService.getTotalCount(request.logger), @@ -126,14 +127,10 @@ export class PreviewUserController { */ @Get('/:id') @OnUndefined(UserNotFoundError) - public one(@Param('id') id: string, @Req() request: AppRequest): Promise { - if (!isString(id)) { - return Promise.reject( - new Error( - JSON.stringify({ type: SERVER_ERROR.INCORRECT_PARAM_FORMAT, message: ' : id should be of type string.' }) - ) - ); - } + public one( + @Params({ validate: true }) { id }: IdValidator, + @Req() request: AppRequest + ): Promise { return this.previewUserService.findOne(id, request.logger); } @@ -161,7 +158,10 @@ export class PreviewUserController { * description: New ExperimentUser is created */ @Post() - public create(@Body({ validate: true }) users: PreviewUserValidator, @Req() request: AppRequest): Promise { + public create( + @Body({ validate: true }) users: PreviewUserValidator, + @Req() request: AppRequest + ): Promise { return this.previewUserService.create(users, request.logger); } @@ -196,7 +196,7 @@ export class PreviewUserController { */ @Put('/:id') public update( - @Param('id') id: string, + @Params({ validate: true }) { id }: IdValidator, @Body({ validate: true }) user: PreviewUserValidator, @Req() request: AppRequest ): Promise { @@ -224,7 +224,10 @@ export class PreviewUserController { * description: Delete User By Id */ @Delete('/:id') - public delete(@Param('id') id: string, @Req() request: AppRequest): Promise { + public delete( + @Params({ validate: true }) { id }: IdValidator, + @Req() request: AppRequest + ): Promise { return this.previewUserService.delete(id, request.logger); } @@ -252,7 +255,10 @@ export class PreviewUserController { * description: Assignment is created */ @Post('/assign') - public assign(@Body({ validate: true }) user: PreviewUserValidator, @Req() request: AppRequest): Promise { + public assign( + @Body({ validate: true }) user: PreviewUserValidator, + @Req() request: AppRequest + ): Promise { return this.previewUserService.upsertExperimentConditionAssignment(user, request.logger); } } diff --git a/backend/packages/Upgrade/src/api/controllers/SegmentController.ts b/backend/packages/Upgrade/src/api/controllers/SegmentController.ts index 0ed9e31069..d6e47df88c 100644 --- a/backend/packages/Upgrade/src/api/controllers/SegmentController.ts +++ b/backend/packages/Upgrade/src/api/controllers/SegmentController.ts @@ -1,24 +1,12 @@ -import { - JsonController, - Get, - Delete, - Param, - Authorized, - Post, - Req, - Body, - QueryParams, - Params, -} from 'routing-controllers'; +import { JsonController, Get, Delete, Authorized, Post, Req, Body, QueryParams, Params } from 'routing-controllers'; import { SegmentService, SegmentWithStatus } from '../services/SegmentService'; import { Segment } from '../models/Segment'; -import { SERVER_ERROR } from 'upgrade_types'; -import { isUUID } from 'class-validator'; import { AppRequest } from '../../types'; import { IdValidator, SegmentFile, SegmentIds, + SegmentIdValidator, SegmentImportError, SegmentInputValidator, } from './validators/SegmentInputValidator'; @@ -383,17 +371,10 @@ export class SegmentController { * description: Internal Server Error, SegmentId is not valid */ @Delete('/:segmentId') - public deleteSegment(@Param('segmentId') segmentId: string, @Req() request: AppRequest): Promise { - if (!segmentId) { - return Promise.reject(new Error(SERVER_ERROR.MISSING_PARAMS + ' : segmentId should not be null.')); - } - if (!isUUID(segmentId)) { - return Promise.reject( - new Error( - JSON.stringify({ type: SERVER_ERROR.INCORRECT_PARAM_FORMAT, message: ' : segmentId should be of type UUID.' }) - ) - ); - } + public deleteSegment( + @Params({ validate: true }) { segmentId }: SegmentIdValidator, + @Req() request: AppRequest + ): Promise { return this.segmentService.deleteSegment(segmentId, request.logger); } diff --git a/backend/packages/Upgrade/src/api/controllers/StratificationController.ts b/backend/packages/Upgrade/src/api/controllers/StratificationController.ts index 751721976e..4de08ff698 100644 --- a/backend/packages/Upgrade/src/api/controllers/StratificationController.ts +++ b/backend/packages/Upgrade/src/api/controllers/StratificationController.ts @@ -1,9 +1,12 @@ -import { JsonController, Get, Delete, Param, Authorized, Req, Res, Body, Post } from 'routing-controllers'; -import { SERVER_ERROR } from 'upgrade_types'; +import { JsonController, Get, Delete, Authorized, Req, Res, Body, Post, Params } from 'routing-controllers'; import { AppRequest } from '../../types'; import { UserStratificationFactor } from '../models/UserStratificationFactor'; import { StratificationService } from '../services/StratificationService'; -import { FactorStrata, UploadedFilesArrayValidator } from './validators/StratificationValidator'; +import { + FactorStrata, + StratificationFactorValidator, + UploadedFilesArrayValidator, +} from './validators/StratificationValidator'; import { StratificationFactor } from '../models/StratificationFactor'; import * as express from 'express'; @@ -110,16 +113,11 @@ export class StratificationController { */ @Get('/download/:factor') public async getStratificationByFactor( - @Param('factor') factor: string, + @Params({ validate: true }) { factor }: StratificationFactorValidator, @Req() request: AppRequest, @Res() res: express.Response ): Promise { - if (!factor) { - return Promise.reject(new Error(SERVER_ERROR.MISSING_PARAMS + ' : stratification Factor should not be null.')); - } - const csvData = await this.stratificationService.getCSVDataByFactor(factor, request.logger); - // return csv file with appropriate headers to request; res.setHeader('Content-Type', 'text/csv; charset=UTF-8'); res.setHeader('Content-Disposition', `attachment; filename="${factor}.csv"`); @@ -190,12 +188,9 @@ export class StratificationController { */ @Delete('/:factor') public deleteStratification( - @Param('factor') stratificationFactor: string, + @Params({ validate: true }) { factor }: StratificationFactorValidator, @Req() request: AppRequest ): Promise { - if (!stratificationFactor) { - return Promise.reject(new Error(SERVER_ERROR.MISSING_PARAMS + ' : stratification Factor should not be null.')); - } - return this.stratificationService.deleteStratification(stratificationFactor, request.logger); + return this.stratificationService.deleteStratification(factor, request.logger); } } diff --git a/backend/packages/Upgrade/src/api/controllers/UserController.ts b/backend/packages/Upgrade/src/api/controllers/UserController.ts index a14eafe679..8c9046639b 100644 --- a/backend/packages/Upgrade/src/api/controllers/UserController.ts +++ b/backend/packages/Upgrade/src/api/controllers/UserController.ts @@ -1,9 +1,8 @@ -import { JsonController, Post, Body, Get, Param, Authorized, Delete, Req } from 'routing-controllers'; +import { JsonController, Post, Body, Get, Authorized, Delete, Req, Params } from 'routing-controllers'; import { User } from '../models/User'; import { UserService } from '../services/UserService'; import { UserDTO } from '../DTO/UserDTO'; -import { UserPaginatedParamsValidator } from './validators/UserPaginatedParamsValidator'; -import { SERVER_ERROR } from 'upgrade_types'; +import { EmailValidator, UserPaginatedParamsValidator } from './validators/UserPaginatedParamsValidator'; import { AppRequest, PaginationResponse } from '../../types'; interface UserPaginationInfo extends PaginationResponse { @@ -94,17 +93,6 @@ export class UserController { paginatedParams: UserPaginatedParamsValidator, @Req() request: AppRequest ): Promise { - if (!paginatedParams || Object.keys(paginatedParams).length === 0) { - return Promise.reject( - new Error( - JSON.stringify({ - type: SERVER_ERROR.MISSING_PARAMS, - message: ' : paginatedParams should not be null or empty object.', - }) - ) - ); - } - const [users, count] = await Promise.all([ this.userService.findPaginated( paginatedParams.skip, @@ -143,7 +131,7 @@ export class UserController { * description: Get User By Email */ @Get('/:email') - public getUserByEmail(@Param('email') email: string): Promise { + public getUserByEmail(@Params({ validate: true }) { email }: EmailValidator): Promise { return this.userService.getUserByEmail(email); } @@ -240,10 +228,7 @@ export class UserController { * description: Delete User By email */ @Delete('/:email') - public delete(@Param('email') email: string): Promise { - if (!email) { - return Promise.reject(new Error(SERVER_ERROR.MISSING_PARAMS + ' : email should not be null.')); - } + public delete(@Params({ validate: true }) { email }: EmailValidator): Promise { return this.userService.deleteUser(email); } } diff --git a/backend/packages/Upgrade/src/api/controllers/validators/ExperimentUserValidator.ts b/backend/packages/Upgrade/src/api/controllers/validators/ExperimentUserValidator.ts index 7e46109908..853723aa2d 100644 --- a/backend/packages/Upgrade/src/api/controllers/validators/ExperimentUserValidator.ts +++ b/backend/packages/Upgrade/src/api/controllers/validators/ExperimentUserValidator.ts @@ -88,6 +88,12 @@ export class ExperimentUserValidator extends ExperimentUserValidatorv6 { public id: string; } +export class IdValidator { + @IsNotEmpty() + @IsString() + public id: string; +} + export class ExperimentUserArrayValidator { @IsNotEmpty() @IsArray() diff --git a/backend/packages/Upgrade/src/api/controllers/validators/FeatureFlagValidator.ts b/backend/packages/Upgrade/src/api/controllers/validators/FeatureFlagValidator.ts index 15777325cf..ebe4ade568 100644 --- a/backend/packages/Upgrade/src/api/controllers/validators/FeatureFlagValidator.ts +++ b/backend/packages/Upgrade/src/api/controllers/validators/FeatureFlagValidator.ts @@ -6,7 +6,7 @@ import { FeatureFlagListValidator } from './FeatureFlagListValidator'; export class FeatureFlagCoreValidation { @IsOptional() - @IsString() + @IsUUID() public id: string; @IsNotEmpty() @@ -54,16 +54,6 @@ export class FeatureFlagValidation extends FeatureFlagCoreValidation { public featureFlagSegmentExclusion?: FeatureFlagListValidator[]; } -export class UserParamsValidator { - @IsNotEmpty() - @IsString() - public userId: string; - - @IsNotEmpty() - @IsString() - public context: string; -} - export class IdValidator { @IsNotEmpty() @IsUUID() diff --git a/backend/packages/Upgrade/src/api/controllers/validators/MetricValidator.ts b/backend/packages/Upgrade/src/api/controllers/validators/MetricValidator.ts index 77604e6cda..ea3f365dbf 100644 --- a/backend/packages/Upgrade/src/api/controllers/validators/MetricValidator.ts +++ b/backend/packages/Upgrade/src/api/controllers/validators/MetricValidator.ts @@ -110,3 +110,9 @@ export class MetricKeyValidator { @IsString() key: string; } + +export class ContextValidator { + @IsNotEmpty() + @IsString() + context: string; +} diff --git a/backend/packages/Upgrade/src/api/controllers/validators/PreviewUserValidator.ts b/backend/packages/Upgrade/src/api/controllers/validators/PreviewUserValidator.ts index 84e730ab3e..e8092eccf1 100644 --- a/backend/packages/Upgrade/src/api/controllers/validators/PreviewUserValidator.ts +++ b/backend/packages/Upgrade/src/api/controllers/validators/PreviewUserValidator.ts @@ -1,4 +1,4 @@ -import { ValidateNested, IsOptional, IsString, IsArray } from 'class-validator'; +import { ValidateNested, IsOptional, IsString, IsArray, IsNotEmpty } from 'class-validator'; import { Type } from 'class-transformer'; import { Experiment } from '../../../../src/api/models/Experiment'; import { ExperimentCondition } from '../../../../src/api/models/ExperimentCondition'; @@ -26,3 +26,9 @@ export class PreviewUserValidator { @Type(() => ExplicitIndividualAssignment) public assignments?: ExplicitIndividualAssignment[]; } + +export class IdValidator { + @IsNotEmpty() + @IsString() + public id: string; +} diff --git a/backend/packages/Upgrade/src/api/controllers/validators/SegmentInputValidator.ts b/backend/packages/Upgrade/src/api/controllers/validators/SegmentInputValidator.ts index 2ef4584cf7..0cc060847d 100644 --- a/backend/packages/Upgrade/src/api/controllers/validators/SegmentInputValidator.ts +++ b/backend/packages/Upgrade/src/api/controllers/validators/SegmentInputValidator.ts @@ -89,3 +89,9 @@ export class SegmentIds { @IsUUID('all', { each: true }) public ids: string[]; } + +export class SegmentIdValidator { + @IsNotEmpty() + @IsUUID() + public segmentId: string; +} diff --git a/backend/packages/Upgrade/src/api/controllers/validators/StratificationValidator.ts b/backend/packages/Upgrade/src/api/controllers/validators/StratificationValidator.ts index 4021c9877e..b0c0c52736 100644 --- a/backend/packages/Upgrade/src/api/controllers/validators/StratificationValidator.ts +++ b/backend/packages/Upgrade/src/api/controllers/validators/StratificationValidator.ts @@ -41,3 +41,9 @@ export class UploadedFilesValidator { @IsString() public file: string; } + +export class StratificationFactorValidator { + @IsNotEmpty() + @IsString() + public factor: string; +} diff --git a/backend/packages/Upgrade/src/api/controllers/validators/UserPaginatedParamsValidator.ts b/backend/packages/Upgrade/src/api/controllers/validators/UserPaginatedParamsValidator.ts index c9fe00eca9..268cd02ad2 100644 --- a/backend/packages/Upgrade/src/api/controllers/validators/UserPaginatedParamsValidator.ts +++ b/backend/packages/Upgrade/src/api/controllers/validators/UserPaginatedParamsValidator.ts @@ -48,3 +48,9 @@ export class UserPaginatedParamsValidator { @Type(() => UserSortParamsValidator) public sortParams?: UserSortParamsValidator; } + +export class EmailValidator { + @IsNotEmpty() + @IsString() + public email: string; +} diff --git a/backend/packages/Upgrade/src/api/services/ExperimentService.ts b/backend/packages/Upgrade/src/api/services/ExperimentService.ts index 8fb2ef93b6..ab23f1d0da 100644 --- a/backend/packages/Upgrade/src/api/services/ExperimentService.ts +++ b/backend/packages/Upgrade/src/api/services/ExperimentService.ts @@ -127,7 +127,7 @@ export class ExperimentService { public queryService: QueryService ) {} - public async find(logger?: UpgradeLogger): Promise { + public async find(logger?: UpgradeLogger): Promise { if (logger) { logger.info({ message: `Find all experiments` }); } @@ -211,7 +211,7 @@ export class ExperimentService { }); } - public async getSingleExperiment(id: string, logger?: UpgradeLogger): Promise { + public async getSingleExperiment(id: string, logger?: UpgradeLogger): Promise { const experiment = await this.findOne(id, logger); if (experiment) { return this.reducedConditionPayload(this.formatingPayload(experiment)); @@ -367,9 +367,7 @@ export class ExperimentService { if (logger) { logger.info({ message: `Update the experiment`, details: experiment }); } - return this.reducedConditionPayload( - await this.updateExperimentInDB(experiment as ExperimentDTO, currentUser, logger) - ); + return this.reducedConditionPayload(await this.updateExperimentInDB(experiment, currentUser, logger)); } public async getExperimentalConditions(experimentId: string, logger: UpgradeLogger): Promise { diff --git a/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts b/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts index a152f91b45..0bf8e816f6 100644 --- a/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts +++ b/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts @@ -130,7 +130,11 @@ export class FeatureFlagService { return featureFlag; } - public async create(flagDTO: FeatureFlagValidation, currentUser: UserDTO, logger: UpgradeLogger): Promise { + public async create( + flagDTO: FeatureFlagValidation, + currentUser: UserDTO, + logger: UpgradeLogger + ): Promise { logger.info({ message: 'Create a new feature flag', details: flagDTO }); const result = await this.featureFlagRepository.validateUniqueKey(flagDTO); @@ -316,7 +320,14 @@ export class FeatureFlagService { return featureFlag; } - public async update(flagDTO: FeatureFlagValidation, currentUser: UserDTO, logger: UpgradeLogger): Promise { + public async update( + flagDTO: FeatureFlagValidation, + flagId: string, + currentUser: UserDTO, + logger: UpgradeLogger + ): Promise { + // update the flag with the flagId from the path params: + flagDTO.id = flagId; logger.info({ message: `Update a Feature Flag => ${flagDTO.toString()}` }); const result = await this.featureFlagRepository.validateUniqueKey(flagDTO); diff --git a/backend/packages/Upgrade/test/integration/Experiment/conditionAndPartition/ConditionPayloads.ts b/backend/packages/Upgrade/test/integration/Experiment/conditionAndPartition/ConditionPayloads.ts index 9e5f2f7da6..7220034b0b 100644 --- a/backend/packages/Upgrade/test/integration/Experiment/conditionAndPartition/ConditionPayloads.ts +++ b/backend/packages/Upgrade/test/integration/Experiment/conditionAndPartition/ConditionPayloads.ts @@ -103,7 +103,7 @@ export default async function ConditionPayload(): Promise { updatedExperimentDoc = await experimentService.getSingleExperiment( updatedExperimentDoc.id as any, new UpgradeLogger() - ); + ) as any; expect(updatedExperimentDoc.conditionPayloads.length).toEqual(1); expect(updatedExperimentDoc.conditionPayloads).toEqual( @@ -128,6 +128,6 @@ export default async function ConditionPayload(): Promise { updatedExperimentDoc = await experimentService.getSingleExperiment( updatedExperimentDoc.id as any, new UpgradeLogger() - ); + ) as any; expect(updatedExperimentDoc.conditionPayloads.length).toEqual(0); } diff --git a/backend/packages/Upgrade/test/unit/controllers/ExperimentUserController.test.ts b/backend/packages/Upgrade/test/unit/controllers/ExperimentUserController.test.ts index 7672796389..90b3a61bb8 100644 --- a/backend/packages/Upgrade/test/unit/controllers/ExperimentUserController.test.ts +++ b/backend/packages/Upgrade/test/unit/controllers/ExperimentUserController.test.ts @@ -8,6 +8,8 @@ import ExperimentUserServiceMock from './mocks/ExperimentUserServiceMock'; import { useContainer as classValidatorUseContainer } from 'class-validator'; import { v4 as uuid } from 'uuid'; +import { ExperimentClientController } from '../../../src/api/controllers/ExperimentClientController.v5'; +import ExperimentClientControllerMock from './mocks/ExperimentClientControllerMock'; describe('Experiment User Controller Testing', () => { beforeAll(() => { @@ -16,6 +18,8 @@ describe('Experiment User Controller Testing', () => { classValidatorUseContainer(Container); Container.set(ExperimentUserService, new ExperimentUserServiceMock()); + Container.set(ExperimentClientController, new ExperimentClientControllerMock()); + }); afterAll(() => { @@ -38,14 +42,6 @@ describe('Experiment User Controller Testing', () => { .expect(200); }); - test('Get request for /api/experimentusers/id with bad id', () => { - return request(app) - .get('/api/experimentusers/u22') - .set('Accept', 'application/json') - .expect('Content-Type', 'text/html; charset=utf-8') - .expect(500); - }); - test('Post request for /api/experimentusers/', () => { return request(app) .post('/api/experimentusers/') diff --git a/backend/packages/Upgrade/test/unit/controllers/FeatureFlagController.test.ts b/backend/packages/Upgrade/test/unit/controllers/FeatureFlagController.test.ts index 327995db78..403fcebf6a 100644 --- a/backend/packages/Upgrade/test/unit/controllers/FeatureFlagController.test.ts +++ b/backend/packages/Upgrade/test/unit/controllers/FeatureFlagController.test.ts @@ -45,7 +45,7 @@ describe('Feature Flag Controller Testing', () => { return request(app) .post('/api/flags') .send({ - id: 'string', + id: uuid(), name: 'string', key: 'string', description: 'string', @@ -95,7 +95,7 @@ describe('Feature Flag Controller Testing', () => { return request(app) .put('/api/flags/' + uuid()) .send({ - id: 'string', + id: uuid(), name: 'string', key: 'string', description: 'string', diff --git a/backend/packages/Upgrade/test/unit/controllers/mocks/ExperimentClientControllerMock.ts b/backend/packages/Upgrade/test/unit/controllers/mocks/ExperimentClientControllerMock.ts new file mode 100644 index 0000000000..07a29369dc --- /dev/null +++ b/backend/packages/Upgrade/test/unit/controllers/mocks/ExperimentClientControllerMock.ts @@ -0,0 +1,9 @@ +import { Service } from 'typedi'; +import { UpgradeLogger } from '../../../../src/lib/logger/UpgradeLogger'; + +@Service() +export default class ExperimentClientControllerMock { + public checkIfUserExist(userId: string, take: number, logger: UpgradeLogger, api?: string): Promise<[]> { + return Promise.resolve([]); + } +} diff --git a/backend/packages/Upgrade/test/unit/services/FeatureFlagService.test.ts b/backend/packages/Upgrade/test/unit/services/FeatureFlagService.test.ts index 0dd231c210..3dbca1662e 100644 --- a/backend/packages/Upgrade/test/unit/services/FeatureFlagService.test.ts +++ b/backend/packages/Upgrade/test/unit/services/FeatureFlagService.test.ts @@ -370,7 +370,7 @@ describe('Feature Flag Service Testing', () => { }); it('should update the flag', async () => { - const results = await service.update(mockFlag2, mockUser1, logger); + const results = await service.update(mockFlag2, uuid(), mockUser1, logger); expect(isUUID(results.id)).toBeTruthy(); }); @@ -378,7 +378,7 @@ describe('Feature Flag Service Testing', () => { const err = new Error('insert error'); flagRepo.updateFeatureFlag = jest.fn().mockRejectedValue(err); expect(async () => { - await service.update(mockFlag2, mockUser1, logger); + await service.update(mockFlag2, uuid(), mockUser1, logger); }).rejects.toThrow( new Error('Error in updating feature flag document "updateFeatureFlagInDB" Error: insert error') ); From 727438c1cf4407613f8eb8796de0d6b56fa207ec Mon Sep 17 00:00:00 2001 From: pratik Date: Tue, 29 Oct 2024 17:04:37 +0530 Subject: [PATCH 2/2] peer review comments for params updates in api --- .../Upgrade/src/api/controllers/ExperimentController.ts | 4 ++-- .../Upgrade/src/api/controllers/FeatureFlagController.ts | 2 +- .../Upgrade/src/api/controllers/PreviewUserController.ts | 3 ++- .../api/controllers/validators/PreviewUserValidator.ts | 8 +------- .../Upgrade/src/api/services/ExperimentService.ts | 2 +- .../Upgrade/src/api/services/FeatureFlagService.ts | 3 --- .../Experiment/conditionAndPartition/ConditionPayloads.ts | 4 ++-- .../Upgrade/test/unit/services/FeatureFlagService.test.ts | 4 ++-- 8 files changed, 11 insertions(+), 19 deletions(-) diff --git a/backend/packages/Upgrade/src/api/controllers/ExperimentController.ts b/backend/packages/Upgrade/src/api/controllers/ExperimentController.ts index 20bc1b89ce..597bb45207 100644 --- a/backend/packages/Upgrade/src/api/controllers/ExperimentController.ts +++ b/backend/packages/Upgrade/src/api/controllers/ExperimentController.ts @@ -1137,9 +1137,9 @@ export class ExperimentController { experiment: ExperimentDTO, @CurrentUser() currentUser: UserDTO, @Req() request: AppRequest - ): Promise { + ): Promise { request.logger.child({ user: currentUser }); - return this.experimentService.update(experiment, currentUser, request.logger); + return this.experimentService.update({ ...experiment, id }, currentUser, request.logger); } /** diff --git a/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts b/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts index f75a1dfacd..1398cfde4d 100644 --- a/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts +++ b/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts @@ -445,7 +445,7 @@ export class FeatureFlagsController { @CurrentUser() currentUser: UserDTO, @Req() request: AppRequest ): Promise { - return this.featureFlagService.update(flag, id, currentUser, request.logger); + return this.featureFlagService.update({ ...flag, id }, currentUser, request.logger); } /** diff --git a/backend/packages/Upgrade/src/api/controllers/PreviewUserController.ts b/backend/packages/Upgrade/src/api/controllers/PreviewUserController.ts index 0251e05456..0f3cef6491 100644 --- a/backend/packages/Upgrade/src/api/controllers/PreviewUserController.ts +++ b/backend/packages/Upgrade/src/api/controllers/PreviewUserController.ts @@ -15,7 +15,8 @@ import { PreviewUserService } from '../services/PreviewUserService'; import { PreviewUser } from '../models/PreviewUser'; import { PaginatedParamsValidator } from './validators/PaginatedParamsValidator'; import { AppRequest, PaginationResponse } from '../../types'; -import { IdValidator, PreviewUserValidator } from './validators/PreviewUserValidator'; +import { PreviewUserValidator } from './validators/PreviewUserValidator'; +import { IdValidator } from './validators/ExperimentUserValidator'; interface PreviewUserPaginationInfo extends PaginationResponse { nodes: PreviewUser[]; diff --git a/backend/packages/Upgrade/src/api/controllers/validators/PreviewUserValidator.ts b/backend/packages/Upgrade/src/api/controllers/validators/PreviewUserValidator.ts index e8092eccf1..84e730ab3e 100644 --- a/backend/packages/Upgrade/src/api/controllers/validators/PreviewUserValidator.ts +++ b/backend/packages/Upgrade/src/api/controllers/validators/PreviewUserValidator.ts @@ -1,4 +1,4 @@ -import { ValidateNested, IsOptional, IsString, IsArray, IsNotEmpty } from 'class-validator'; +import { ValidateNested, IsOptional, IsString, IsArray } from 'class-validator'; import { Type } from 'class-transformer'; import { Experiment } from '../../../../src/api/models/Experiment'; import { ExperimentCondition } from '../../../../src/api/models/ExperimentCondition'; @@ -26,9 +26,3 @@ export class PreviewUserValidator { @Type(() => ExplicitIndividualAssignment) public assignments?: ExplicitIndividualAssignment[]; } - -export class IdValidator { - @IsNotEmpty() - @IsString() - public id: string; -} diff --git a/backend/packages/Upgrade/src/api/services/ExperimentService.ts b/backend/packages/Upgrade/src/api/services/ExperimentService.ts index ab23f1d0da..be1d6cad01 100644 --- a/backend/packages/Upgrade/src/api/services/ExperimentService.ts +++ b/backend/packages/Upgrade/src/api/services/ExperimentService.ts @@ -363,7 +363,7 @@ export class ExperimentService { }); } - public async update(experiment: ExperimentDTO, currentUser: UserDTO, logger: UpgradeLogger): Promise { + public async update(experiment: ExperimentDTO, currentUser: UserDTO, logger: UpgradeLogger): Promise { if (logger) { logger.info({ message: `Update the experiment`, details: experiment }); } diff --git a/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts b/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts index 0bf8e816f6..561e81f075 100644 --- a/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts +++ b/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts @@ -322,12 +322,9 @@ export class FeatureFlagService { public async update( flagDTO: FeatureFlagValidation, - flagId: string, currentUser: UserDTO, logger: UpgradeLogger ): Promise { - // update the flag with the flagId from the path params: - flagDTO.id = flagId; logger.info({ message: `Update a Feature Flag => ${flagDTO.toString()}` }); const result = await this.featureFlagRepository.validateUniqueKey(flagDTO); diff --git a/backend/packages/Upgrade/test/integration/Experiment/conditionAndPartition/ConditionPayloads.ts b/backend/packages/Upgrade/test/integration/Experiment/conditionAndPartition/ConditionPayloads.ts index 7220034b0b..9e5f2f7da6 100644 --- a/backend/packages/Upgrade/test/integration/Experiment/conditionAndPartition/ConditionPayloads.ts +++ b/backend/packages/Upgrade/test/integration/Experiment/conditionAndPartition/ConditionPayloads.ts @@ -103,7 +103,7 @@ export default async function ConditionPayload(): Promise { updatedExperimentDoc = await experimentService.getSingleExperiment( updatedExperimentDoc.id as any, new UpgradeLogger() - ) as any; + ); expect(updatedExperimentDoc.conditionPayloads.length).toEqual(1); expect(updatedExperimentDoc.conditionPayloads).toEqual( @@ -128,6 +128,6 @@ export default async function ConditionPayload(): Promise { updatedExperimentDoc = await experimentService.getSingleExperiment( updatedExperimentDoc.id as any, new UpgradeLogger() - ) as any; + ); expect(updatedExperimentDoc.conditionPayloads.length).toEqual(0); } diff --git a/backend/packages/Upgrade/test/unit/services/FeatureFlagService.test.ts b/backend/packages/Upgrade/test/unit/services/FeatureFlagService.test.ts index 3dbca1662e..0dd231c210 100644 --- a/backend/packages/Upgrade/test/unit/services/FeatureFlagService.test.ts +++ b/backend/packages/Upgrade/test/unit/services/FeatureFlagService.test.ts @@ -370,7 +370,7 @@ describe('Feature Flag Service Testing', () => { }); it('should update the flag', async () => { - const results = await service.update(mockFlag2, uuid(), mockUser1, logger); + const results = await service.update(mockFlag2, mockUser1, logger); expect(isUUID(results.id)).toBeTruthy(); }); @@ -378,7 +378,7 @@ describe('Feature Flag Service Testing', () => { const err = new Error('insert error'); flagRepo.updateFeatureFlag = jest.fn().mockRejectedValue(err); expect(async () => { - await service.update(mockFlag2, uuid(), mockUser1, logger); + await service.update(mockFlag2, mockUser1, logger); }).rejects.toThrow( new Error('Error in updating feature flag document "updateFeatureFlagInDB" Error: insert error') );