Skip to content

Commit

Permalink
Merge pull request #2065 from CarnegieLearningWeb/hotfix/resolve-404-…
Browse files Browse the repository at this point in the history 
…init-call-httperror

hotfix to throw http 404 error for api calls other than init for new users
  • Loading branch information
@ppratikcr7
ppratikcr7 authored Oct 22, 2024
2 parents 8bef480 + 46ffb84 commit 49c0b7d
Show file tree
Hide file tree
Showing 8 changed files with 34 additions and 180 deletions.
38 changes: 28 additions & 10 deletions backend/packages/Upgrade/src/api/controllers/ExperimentClientController.v5.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@ import { AppRequest } from '../../types';
import { MonitoredDecisionPointLog } from '../models/MonitoredDecisionPointLog';
import { MarkExperimentValidatorv5 } from './validators/MarkExperimentValidator.v5';
import { Log } from '../models/Log';
import { ExperimentUserValidator } from './validators/ExperimentUserValidator';
import { ExperimentUserValidator, RequestedExperimentUser } from './validators/ExperimentUserValidator';
import { HttpError } from '../errors';
import { UpgradeLogger } from 'src/lib/logger/UpgradeLogger';

interface IMonitoredDecisionPoint {
id: string;
Expand Down Expand Up @@ -104,6 +106,22 @@ export class ExperimentClientController {
public metricService: MetricService
) {}

private async checkIfUserExist(
userId: string,
logger: UpgradeLogger,
api?: string
): Promise<RequestedExperimentUser | null> {
const experimentUserDoc = await this.experimentUserService.getUserDoc(userId, logger);
if (experimentUserDoc === null) {
if (api === 'init') {
return null;
} else {
throw new HttpError(404, `Experiment User not found: ${userId}`);
}
} else {
return experimentUserDoc;
}
}
/**
* @swagger
* /v5/init:
Expand Down Expand Up @@ -179,7 +197,7 @@ export class ExperimentClientController {
): Promise<Pick<ExperimentUser, 'id' | 'group' | 'workingGroup'>> {
request.logger.info({ message: 'Starting the init call for user' });
// getOriginalUserDoc call for alias
const experimentUserDoc = await this.experimentUserService.getUserDoc(experimentUser.id, request.logger);
const experimentUserDoc = await this.checkIfUserExist(experimentUser.id, request.logger, 'init');
// if reinit call is made with any of the below fields not included in the call,
// then we will fetch the stored values of the field and return them in the response
// for consistent init response with 3 fields ['userId', 'group', 'workingGroup']
Expand Down Expand Up @@ -267,7 +285,7 @@ export class ExperimentClientController {
): Promise<IGroupMembership> {
request.logger.info({ message: 'Starting the groupmembership call for user' });
// getOriginalUserDoc call for alias
const experimentUserDoc = await this.experimentUserService.getUserDoc(experimentUser.id, request.logger);
const experimentUserDoc = await this.checkIfUserExist(experimentUser.id, request.logger);
// append userDoc in logger
request.logger.child({ userDoc: experimentUserDoc });
request.logger.info({ message: 'Got the original user doc' });
Expand Down Expand Up @@ -339,7 +357,7 @@ export class ExperimentClientController {
): Promise<IWorkingGroup> {
request.logger.info({ message: 'Starting the workinggroup call for user' });
// getOriginalUserDoc call for alias
const experimentUserDoc = await this.experimentUserService.getUserDoc(workingGroupParams.id, request.logger);
const experimentUserDoc = await this.checkIfUserExist(workingGroupParams.id, request.logger);
// append userDoc in logger
request.logger.child({ userDoc: experimentUserDoc });
request.logger.info({ message: 'Got the original user doc' });
Expand Down Expand Up @@ -446,7 +464,7 @@ export class ExperimentClientController {
): Promise<IMonitoredDecisionPoint> {
request.logger.info({ message: 'Starting the markExperimentPoint call for user' });
// getOriginalUserDoc call for alias
const experimentUserDoc = await this.experimentUserService.getUserDoc(experiment.userId, request.logger);
const experimentUserDoc = await this.checkIfUserExist(experiment.userId, request.logger);
// append userDoc in logger
request.logger.child({ userDoc: experimentUserDoc });
request.logger.info({ message: 'Got the original user doc' });
Expand Down Expand Up @@ -549,7 +567,7 @@ export class ExperimentClientController {
experiment: ExperimentAssignmentValidator
): Promise<IExperimentAssignmentv5[]> {
request.logger.info({ message: 'Starting the getAllExperimentConditions call for user' });
const experimentUserDoc = await this.experimentUserService.getUserDoc(experiment.userId, request.logger);
const experimentUserDoc = await this.checkIfUserExist(experiment.userId, request.logger);
const assignedData = await this.experimentAssignmentService.getAllExperimentConditions(
experimentUserDoc,
experiment.context,
Expand Down Expand Up @@ -679,7 +697,7 @@ export class ExperimentClientController {
): Promise<Omit<Log, 'createdAt' | 'updatedAt' | 'versionNumber'>[]> {
request.logger.info({ message: 'Starting the log call for user' });
// getOriginalUserDoc call for alias
const experimentUserDoc = await this.experimentUserService.getUserDoc(logData.userId, request.logger);
const experimentUserDoc = await this.checkIfUserExist(logData.userId, request.logger);
// append userDoc in logger
request.logger.child({ userDoc: experimentUserDoc });
request.logger.info({ message: 'Got the original user doc' });
Expand Down Expand Up @@ -733,7 +751,7 @@ export class ExperimentClientController {
const blobData = JSON.parse(request.read());
try {
// The function will throw error if userId doesn't exist
const experimentUserDoc = await this.experimentUserService.getUserDoc(blobData.userId, request.logger);
const experimentUserDoc = await this.checkIfUserExist(blobData.userId, request.logger);
// append userDoc in logger
request.logger.child({ userDoc: experimentUserDoc });
request.logger.info({ message: 'Got the original user doc' });
Expand Down Expand Up @@ -805,7 +823,7 @@ export class ExperimentClientController {
@Body({ validate: true })
experiment: ExperimentAssignmentValidator
): Promise<string[]> {
const experimentUserDoc = await this.experimentUserService.getUserDoc(experiment.userId, request.logger);
const experimentUserDoc = await this.checkIfUserExist(experiment.userId, request.logger);
return this.featureFlagService.getKeys(experimentUserDoc, experiment.context, request.logger);
}

Expand Down Expand Up @@ -871,7 +889,7 @@ export class ExperimentClientController {
@Body({ validate: true })
user: ExperimentUserAliasesValidator
): Promise<IUserAliases> {
const experimentUserDoc = await this.experimentUserService.getUserDoc(user.userId, request.logger);
const experimentUserDoc = await this.checkIfUserExist(user.userId, request.logger);
// append userDoc in logger
request.logger.child({ userDoc: experimentUserDoc });
request.logger.info({ message: 'Got the original user doc' });
Expand Down
3 changes: 1 addition & 2 deletions backend/packages/Upgrade/src/api/middlewares/UserCheckMiddleware.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ export class UserCheckMiddleware {
req.logger.child({ user_id });
req.logger.debug({ message: 'User Id is:', user_id });
}

const experimentUserDoc = await this.experimentUserService.getUserDoc(user_id, req.logger);
if (!req.url.endsWith('/init') && !experimentUserDoc) {
const error = new Error(`User not found: ${user_id}`);
Expand All @@ -39,4 +38,4 @@ export class UserCheckMiddleware {
return next(error);
}
}
}
}
56 changes: 0 additions & 56 deletions backend/packages/Upgrade/src/api/services/ExperimentAssignmentService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,17 +133,7 @@ export class ExperimentAssignmentService {
logger.error(error);
}

// adding experiment error when user is not defined
if (!userDoc || !userDoc.id) {
const error = new Error('User not defined in markExperimentPoint');
(error as any).type = SERVER_ERROR.EXPERIMENT_USER_NOT_DEFINED;
(error as any).httpCode = 404;
logger.error(error);
throw error;
}

const userId = userDoc.id;

const previewUser: PreviewUser = await this.previewUserService.findOne(userId, logger);

// search decision points in experiments cache
Expand Down Expand Up @@ -338,26 +328,8 @@ export class ExperimentAssignmentService {
logger: UpgradeLogger
): Promise<IExperimentAssignmentv5[]> {
logger.info({ message: `getAllExperimentConditions: User: ${experimentUserDoc?.requestedUserId}` });

// throw error if user not defined
if (!experimentUserDoc || !experimentUserDoc.id) {
logger.error({
message: 'User not defined in getAllExperimentConditions',
});
const error = new Error(
JSON.stringify({
type: SERVER_ERROR.EXPERIMENT_USER_NOT_DEFINED,
message: 'User not defined in getAllExperimentConditions',
})
);
(error as any).type = SERVER_ERROR.EXPERIMENT_USER_NOT_DEFINED;
(error as any).httpCode = 404;
throw error;
}
const userId = experimentUserDoc?.id;

const previewUser = await this.previewUserService.findOne(userId, logger);

const experimentUser: ExperimentUser = experimentUserDoc as ExperimentUser;

// query all experiment and sub experiment
Expand Down Expand Up @@ -777,20 +749,6 @@ export class ExperimentAssignmentService {
logger.info({ message: `Add data log userId ${userId}`, details: jsonLog });
const keyUniqueArray: { key: string; uniquifier: string }[] = [];

// throw error if user not defined
if (!userDoc) {
logger.error({ message: `User not found in dataLog, userId => ${userId}`, details: jsonLog });
const error = new Error(
JSON.stringify({
type: SERVER_ERROR.EXPERIMENT_USER_NOT_DEFINED,
message: `User not defined dataLog: ${userId}`,
})
);
(error as any).type = SERVER_ERROR.EXPERIMENT_USER_NOT_DEFINED;
(error as any).httpCode = 404;
throw error;
}

// extract the array value
const promise = jsonLog.map(async (individualMetrics) => {
return this.createLog(individualMetrics, keyUniqueArray, userDoc, logger);
Expand All @@ -811,20 +769,6 @@ export class ExperimentAssignmentService {
const { logger, userDoc } = requestContext;
logger.info({ message: `Failed experiment point for userId ${userId}` });

// throw error if user not defined
if (!userDoc) {
logger.error({ message: `User not found in clientFailedExperimentPoint, userId => ${userId}` });
const error = new Error(
JSON.stringify({
type: SERVER_ERROR.EXPERIMENT_USER_NOT_DEFINED,
message: `User not defined clientFailedExperimentPoint: ${userId}`,
})
);
(error as any).type = SERVER_ERROR.EXPERIMENT_USER_NOT_DEFINED;
(error as any).httpCode = 404;
throw error;
}

error.type = SERVER_ERROR.REPORTED_ERROR;
error.message = JSON.stringify({
site,
Expand Down
44 changes: 2 additions & 42 deletions backend/packages/Upgrade/src/api/services/ExperimentUserService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import { Experiment } from '../models/Experiment';
import isEqual from 'lodash/isEqual';
import { RequestedExperimentUser } from '../controllers/validators/ExperimentUserValidator';
import { env } from '../../env';
import { HttpError } from '../errors';

@Service()
export class ExperimentUserService {
Expand Down Expand Up @@ -105,21 +104,6 @@ export class ExperimentUserService {
const userId = userDoc.id;
const userExist = userDoc;
logger.info({ message: 'Set aliases for experiment user => ' + userId, details: aliases });

// throw error if user not defined
if (!userExist || !userExist.id) {
logger.error({ message: 'User not defined setAliasesForUser' + userId, details: aliases });

const error = new Error(
JSON.stringify({
type: SERVER_ERROR.EXPERIMENT_USER_NOT_DEFINED,
message: `User not defined setAliasesForUser: ${userId}`,
})
);
(error as any).type = SERVER_ERROR.EXPERIMENT_USER_NOT_DEFINED;
(error as any).httpCode = 404;
throw error;
}
const promiseArray = [];
const dedupedArray = [...new Set(aliases)];

Expand Down Expand Up @@ -238,18 +222,6 @@ export class ExperimentUserService {
const { logger, userDoc } = requestContext;
const userExist = userDoc;
logger.info({ message: 'Update working group for user: ' + userId, details: workingGroup });
if (!userExist) {
logger.error({ message: 'User not defined updateWorkingGroup', details: userId });
const error = new Error(
JSON.stringify({
type: SERVER_ERROR.EXPERIMENT_USER_NOT_DEFINED,
message: `User not defined updateWorkingGroup: ${userId}`,
})
);
(error as any).type = SERVER_ERROR.EXPERIMENT_USER_NOT_DEFINED;
(error as any).httpCode = 404;
throw error;
}

// removing enrollments in case working group is changed
if (userExist && userExist.workingGroup && workingGroup) {
Expand Down Expand Up @@ -279,33 +251,21 @@ export class ExperimentUserService {
message: `Set Group Membership for userId: ${userId} with Group membership details as below:`,
details: groupMembership,
});
if (!userExist) {
logger.error({ message: 'User not defined updateGroupMembership', details: userId });
const error = new Error(
JSON.stringify({
type: SERVER_ERROR.EXPERIMENT_USER_NOT_DEFINED,
message: `User not defined updateGroupMembership: ${userId}`,
})
);
(error as any).type = SERVER_ERROR.EXPERIMENT_USER_NOT_DEFINED;
(error as any).httpCode = 404;
throw error;
}

const newDocument = { ...userExist, group: groupMembership };

// update group membership
return this.userRepository.save(newDocument);
}

public async getUserDoc(experimentUserId, logger): Promise<RequestedExperimentUser> {
public async getUserDoc(experimentUserId: string, logger: UpgradeLogger): Promise<RequestedExperimentUser> {
const experimentUserDoc = await this.getOriginalUserDoc(experimentUserId, logger);
if (experimentUserDoc) {
const userDoc = { ...experimentUserDoc, requestedUserId: experimentUserId };
logger.info({ message: 'Got the user doc', details: userDoc });
return userDoc;
} else {
throw new HttpError(404, `Experiment User not found: ${experimentUserId}`);
return null;
}
}

Expand Down
15 changes: 0 additions & 15 deletions backend/packages/Upgrade/src/api/services/FeatureFlagService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,21 +74,6 @@ export class FeatureFlagService {
logger: UpgradeLogger
): Promise<string[]> {
logger.info({ message: `getKeys: User: ${experimentUserDoc?.requestedUserId}` });

// throw error if user not defined
if (!experimentUserDoc || !experimentUserDoc.id) {
logger.error({ message: 'User not defined in getKeys' });
const error = new Error(
JSON.stringify({
type: SERVER_ERROR.EXPERIMENT_USER_NOT_DEFINED,
message: 'User not defined in getKeys',
})
);
(error as any).type = SERVER_ERROR.EXPERIMENT_USER_NOT_DEFINED;
(error as any).httpCode = 404;
throw error;
}

const filteredFeatureFlags = await this.featureFlagRepository.getFlagsFromContext(context);

const [userExcluded, groupExcluded] = await this.experimentAssignmentService.checkUserOrGroupIsGloballyExcluded(
Expand Down
2 changes: 1 addition & 1 deletion backend/packages/Upgrade/test/integration/ExperimentUser/NoExperimentUserOnAssignment.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export default async function testCase(): Promise<void> {
expect(experimentUser.length).toEqual(0);

// get all experiment condition for user 1
await expect(getAllExperimentCondition(experimentUsers[0].id, new UpgradeLogger())).rejects.toThrow();
await expect(getAllExperimentCondition(experimentUsers[0].id, new UpgradeLogger())).toEqual(Promise.resolve({}));

experimentUser = await experimentUserService.find(new UpgradeLogger());
expect(experimentUser.length).toEqual(0);
Expand Down
4 changes: 2 additions & 2 deletions backend/packages/Upgrade/test/integration/UserNotDefined/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export const UserNotDefined = async () => {
null,
new UpgradeLogger()
)
).rejects.toThrow();
).toEqual(Promise.resolve({}));

await expect(
experimentAssignmentService.blobDataLog(
Expand Down Expand Up @@ -57,7 +57,7 @@ export const UserNotDefined = async () => {
null,
new UpgradeLogger()
)
).rejects.toThrow();
).toEqual(Promise.resolve({ "aliases": [], "userId": undefined }));

await expect(
experimentUserService.updateGroupMembership(experimentUsers[0].id, null, {
Expand Down
Loading

0 comments on commit 49c0b7d

Please sign in to comment.