Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding validation and 409 error to flags #1786

Merged
merged 25 commits into from
Sep 26, 2024
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
8c1e678
showing duplicate key error on UI
Jul 25, 2024
84f9d1e
error display for duplicate key and added cdr
Yagnik56 Jul 25, 2024
7e1dd7e
added 409 error in backend for unique-pair error
Jul 26, 2024
2161f0d
Merge branch 'dev' into feature/-1762-validation-and-409-error
Jul 26, 2024
edc4668
remove validate endpoint and return 409 on create/update
Jul 29, 2024
f08f723
fix test-cases
Jul 29, 2024
3f86004
Merge branch 'dev' into feature/-1762-validation-and-409-error
Sep 18, 2024
ea4ef83
solve git comments
Sep 18, 2024
1bca616
Added duplicateKey related changes in the store
Sep 20, 2024
db2b029
moved validationKey function to repository
Sep 20, 2024
a3d1408
show duplicate key error message and addressed UI bugs related to it
Yagnik56 Sep 20, 2024
f0f19b4
solve failing testcases
Sep 20, 2024
7f2980c
Merge branch 'dev' into feature/-1762-validation-and-409-error
danoswaltCL Sep 20, 2024
5a71ea8
moved error throwing code from repo to service
Sep 23, 2024
77b157a
update import for key & context unique pair
Sep 23, 2024
e53ca46
Merge branch 'dev' into feature/-1762-validation-and-409-error
zackcl Sep 24, 2024
f153093
Merge branch 'dev' into feature/-1762-validation-and-409-error
danoswaltCL Sep 24, 2024
71b0500
Key modal outline turns red on duplicate error
Sep 24, 2024
88f00b2
Merge branch 'dev' into feature/-1762-validation-and-409-error
danoswaltCL Sep 24, 2024
dc9a823
fix red box not showing around key
Sep 25, 2024
8a394f4
Merge branch 'dev' into feature/-1762-validation-and-409-error
zackcl Sep 25, 2024
17e4f90
addressed review comment about not updating ff with same key
Yagnik56 Sep 26, 2024
4ee532d
Merge branch 'dev' into feature/-1762-validation-and-409-error
zackcl Sep 26, 2024
8924eea
addressed bug not allowing to create featureFlag
Yagnik56 Sep 26, 2024
a981c15
Merge branch 'dev' into feature/-1762-validation-and-409-error
Yagnik56 Sep 26, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,10 @@ export class ErrorHandlerMiddleware implements ExpressErrorMiddlewareInterface {
message = error.message;
type = SERVER_ERROR.QUERY_FAILED;
break;
case 409:
message = error.message;
type = SERVER_ERROR.DUPLICATE_KEY;
danoswaltCL marked this conversation as resolved.
Show resolved Hide resolved
break;
case 422:
message = error.message;
type = SERVER_ERROR.UNSUPPORTED_CALIPER;
Expand Down
5 changes: 3 additions & 2 deletions backend/packages/Upgrade/src/api/models/FeatureFlag.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Column, Entity, PrimaryColumn, OneToMany } from 'typeorm';
import { Column, Entity, PrimaryColumn, OneToMany, Unique } from 'typeorm';
import { IsNotEmpty } from 'class-validator';
import { BaseModel } from './base/BaseModel';
import { Type } from 'class-transformer';
Expand All @@ -7,6 +7,7 @@ import { FeatureFlagSegmentInclusion } from './FeatureFlagSegmentInclusion';
import { FeatureFlagSegmentExclusion } from './FeatureFlagSegmentExclusion';
import { FeatureFlagExposure } from './FeatureFlagExposure';
@Entity()
@Unique(['key', 'context'])
export class FeatureFlag extends BaseModel {
@PrimaryColumn('uuid')
public id: string;
Expand All @@ -15,7 +16,7 @@ export class FeatureFlag extends BaseModel {
@Column()
public name: string;

@Column('text', { unique: true })
@Column('text')
public key: string;

@Column()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { EntityRepository } from '../../typeorm-typedi-extensions';
import { FeatureFlag } from '../models/FeatureFlag';
import repositoryError from './utils/repositoryError';
import { FEATURE_FLAG_STATUS, FILTER_MODE } from 'upgrade_types';
import { FeatureFlagValidation } from '../controllers/validators/FeatureFlagValidator';

@EntityRepository(FeatureFlag)
export class FeatureFlagRepository extends Repository<FeatureFlag> {
Expand Down Expand Up @@ -111,4 +112,13 @@ export class FeatureFlagRepository extends Repository<FeatureFlag> {

return result;
}

public async validateUniqueKey(flagDTO: FeatureFlagValidation) {
const result = await this.createQueryBuilder('feature_flag')
.where('feature_flag.key = :key', { key: flagDTO.key })
.andWhere('feature_flag.context = :context', { context: flagDTO.context })
.getOne();

return result;
}
}
25 changes: 22 additions & 3 deletions backend/packages/Upgrade/src/api/services/FeatureFlagService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,17 @@ export class FeatureFlagService {
return featureFlag;
}

public create(flagDTO: FeatureFlagValidation, currentUser: User, logger: UpgradeLogger): Promise<FeatureFlag> {
public async create(flagDTO: FeatureFlagValidation, currentUser: User, logger: UpgradeLogger): Promise<FeatureFlag> {
logger.info({ message: 'Create a new feature flag', details: flagDTO });
const result = await this.featureFlagRepository.validateUniqueKey(flagDTO);

if (result) {
const error = new Error(`A flag with this key already exists for this app-context`);
(error as any).type = SERVER_ERROR.DUPLICATE_KEY;
(error as any).httpCode = 409;
throw error;
}

return this.addFeatureFlagInDB(this.featureFlagValidatorToFlag(flagDTO), currentUser, logger);
}

Expand Down Expand Up @@ -286,8 +295,16 @@ export class FeatureFlagService {
return featureFlag;
}

public update(flagDTO: FeatureFlagValidation, currentUser: User, logger: UpgradeLogger): Promise<FeatureFlag> {
public async update(flagDTO: FeatureFlagValidation, currentUser: User, logger: UpgradeLogger): Promise<FeatureFlag> {
logger.info({ message: `Update a Feature Flag => ${flagDTO.toString()}` });
const result = await this.featureFlagRepository.validateUniqueKey(flagDTO);

if (result) {
const error = new Error(`A flag with this key already exists for this app-context`);
(error as any).type = SERVER_ERROR.DUPLICATE_KEY;
(error as any).httpCode = 409;
throw error;
}
// TODO add entry in log of updating feature flag
return this.updateFeatureFlagInDB(this.featureFlagValidatorToFlag(flagDTO), currentUser, logger);
}
Expand Down Expand Up @@ -893,7 +910,9 @@ export class FeatureFlagService {
}

if (compatibilityType === FF_COMPATIBILITY_TYPE.COMPATIBLE) {
const keyExists = existingFeatureFlags.find((existingFlag) => existingFlag.key === flag.key);
const keyExists = existingFeatureFlags?.find(
(existingFlag) => existingFlag.key === flag.key && existingFlag.context === flag.context
);

if (keyExists) {
compatibilityType = FF_COMPATIBILITY_TYPE.INCOMPATIBLE;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
import { MigrationInterface, QueryRunner } from 'typeorm';

export class UniquePairFlag1721969267641 implements MigrationInterface {
name = 'UniquePairFlag1721969267641';

public async up(queryRunner: QueryRunner): Promise<void> {
await queryRunner.query(`ALTER TABLE "feature_flag_segment_exclusion" ALTER COLUMN "enabled" SET DEFAULT false`);
await queryRunner.query(`ALTER TABLE "feature_flag" DROP CONSTRAINT "UQ_960310efa932f7a29eec57350b3"`);
await queryRunner.query(`ALTER TABLE "feature_flag_segment_inclusion" ALTER COLUMN "enabled" SET DEFAULT false`);
await queryRunner.query(
`ALTER TYPE "public"."experiment_error_type_enum" RENAME TO "experiment_error_type_enum_old"`
);
await queryRunner.query(
`CREATE TYPE "public"."experiment_error_type_enum" AS ENUM('Database not reachable', 'Database auth fail', 'Error in the assignment algorithm', 'Parameter missing in the client request', 'Parameter not in the correct format', 'User ID not found', 'Query Failed', 'Error reported from client', 'Experiment user not defined', 'Experiment user group not defined', 'Working group is not a subset of user group', 'Invalid token', 'Token is not present in request', 'JWT Token validation failed', 'Error in migration', 'Email send error', 'Condition not found', 'Experiment ID not provided for shared Decision Point', 'Experiment ID provided is invalid for shared Decision Point', 'Caliper profile or event not supported', 'Feature Flag with same key already exists for this app-context')`
);
await queryRunner.query(
`ALTER TABLE "experiment_error" ALTER COLUMN "type" TYPE "public"."experiment_error_type_enum" USING "type"::"text"::"public"."experiment_error_type_enum"`
);
await queryRunner.query(`DROP TYPE "public"."experiment_error_type_enum_old"`);
await queryRunner.query(
`ALTER TABLE "feature_flag" ADD CONSTRAINT "UQ_653d98ebae725596b4052f80a65" UNIQUE ("key", "context")`
);
}

public async down(queryRunner: QueryRunner): Promise<void> {
await queryRunner.query(`ALTER TABLE "feature_flag" DROP CONSTRAINT "UQ_653d98ebae725596b4052f80a65"`);
await queryRunner.query(
`CREATE TYPE "public"."experiment_error_type_enum_old" AS ENUM('Caliper profile or event not supported', 'Condition not found', 'Database auth fail', 'Database not reachable', 'Email send error', 'Error in migration', 'Error in the assignment algorithm', 'Error reported from client', 'Experiment ID not provided for shared Decision Point', 'Experiment ID provided is invalid for shared Decision Point', 'Experiment user group not defined', 'Experiment user not defined', 'Invalid token', 'JWT Token validation failed', 'Parameter missing in the client request', 'Parameter not in the correct format', 'Query Failed', 'Token is not present in request', 'User ID not found', 'Working group is not a subset of user group')`
);
await queryRunner.query(
`ALTER TABLE "experiment_error" ALTER COLUMN "type" TYPE "public"."experiment_error_type_enum_old" USING "type"::"text"::"public"."experiment_error_type_enum_old"`
);
await queryRunner.query(`DROP TYPE "public"."experiment_error_type_enum"`);
await queryRunner.query(
`ALTER TYPE "public"."experiment_error_type_enum_old" RENAME TO "experiment_error_type_enum"`
);
await queryRunner.query(`ALTER TABLE "feature_flag_segment_inclusion" ALTER COLUMN "enabled" DROP DEFAULT`);
await queryRunner.query(
`ALTER TABLE "feature_flag" ADD CONSTRAINT "UQ_960310efa932f7a29eec57350b3" UNIQUE ("key")`
);
await queryRunner.query(`ALTER TABLE "feature_flag_segment_exclusion" ALTER COLUMN "enabled" DROP DEFAULT`);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@ describe('Feature Flag Service Testing', () => {
addOrderBy: addOrderBySpy,
setParameter: setParameterSpy,
where: jest.fn().mockReturnThis(),
andWhere: jest.fn().mockReturnThis(),
offset: offsetSpy,
limit: limitSpy,
innerJoinAndSelect: jest.fn().mockReturnThis(),
Expand All @@ -201,6 +202,7 @@ describe('Feature Flag Service Testing', () => {
getMany: jest.fn().mockResolvedValue(mockFlagArr),
getOne: jest.fn().mockResolvedValue(mockFlag1),
})),
validateUniqueKey: jest.fn().mockResolvedValue(null),
},
},
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
selectFeatureFlagIds,
selectShouldShowWarningForSelectedFlag,
selectWarningStatusForAllFlags,
selectDuplicateKeyFound,
} from './store/feature-flags.selectors';
import * as FeatureFlagsActions from './store/feature-flags.actions';
import { actionFetchContextMetaData } from '../experiments/store/experiments.actions';
Expand All @@ -51,6 +52,7 @@ export class FeatureFlagsService {
isLoadingFeatureFlags$ = this.store$.pipe(select(selectIsLoadingFeatureFlags));
isLoadingSelectedFeatureFlag$ = this.store$.pipe(select(selectIsLoadingSelectedFeatureFlag));
isLoadingUpsertFeatureFlag$ = this.store$.pipe(select(selectIsLoadingUpsertFeatureFlag));
isDuplicateKeyFound$ = this.store$.pipe(select(selectDuplicateKeyFound));
isLoadingFeatureFlagDelete$ = this.store$.pipe(select(selectIsLoadingFeatureFlagDelete));
isLoadingImportFeatureFlag$ = this.store$.pipe(select(selectIsLoadingImportFeatureFlag));
isLoadingUpdateFeatureFlagStatus$ = this.store$.pipe(select(selectIsLoadingUpdateFeatureFlagStatus));
Expand Down Expand Up @@ -140,6 +142,10 @@ export class FeatureFlagsService {
this.store$.dispatch(FeatureFlagsActions.actionUpdateFilterMode({ updateFilterModeRequest }));
}

setIsDuplicateKey(duplicateKeyFound: boolean) {
this.store$.dispatch(FeatureFlagsActions.actionSetIsDuplicateKey({ duplicateKeyFound }));
}

deleteFeatureFlag(flagId: string) {
this.store$.dispatch(FeatureFlagsActions.actionDeleteFeatureFlag({ flagId }));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,11 @@ export const actionAddFeatureFlagSuccess = createAction(

export const actionAddFeatureFlagFailure = createAction('[Feature Flags] Add Feature Flag Failure');

export const actionSetIsDuplicateKey = createAction(
'[Feature Flags] Upsert Feature Flag Failure, Duplicate Key found',
props<{ duplicateKeyFound: boolean }>()
);

export const actionDeleteFeatureFlag = createAction('[Feature Flags] Delete Feature Flag', props<{ flagId: string }>());

export const actionDeleteFeatureFlagSuccess = createAction(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,11 @@ import {
selectSortAs,
selectSearchString,
selectIsAllFlagsFetched,
selectSelectedFeatureFlag,
} from './feature-flags.selectors';
import { selectCurrentUser } from '../../auth/store/auth.selectors';
import { CommonExportHelpersService } from '../../../shared/services/common-export-helpers.service';
import { of } from 'rxjs';

import { SERVER_ERROR } from 'upgrade_types';
@Injectable()
export class FeatureFlagsEffects {
constructor(
Expand Down Expand Up @@ -103,7 +102,12 @@ export class FeatureFlagsEffects {
tap(({ response }) => {
this.router.navigate(['/featureflags', 'detail', response.id]);
}),
catchError(() => [FeatureFlagsActions.actionAddFeatureFlagFailure()])
catchError((res) => {
if (res.error.type == SERVER_ERROR.DUPLICATE_KEY) {
return [FeatureFlagsActions.actionSetIsDuplicateKey({ duplicateKeyFound: true })];
}
return [FeatureFlagsActions.actionAddFeatureFlagFailure()];
})
);
})
)
Expand All @@ -117,7 +121,12 @@ export class FeatureFlagsEffects {
map((response) => {
return FeatureFlagsActions.actionUpdateFeatureFlagSuccess({ response });
}),
catchError(() => [FeatureFlagsActions.actionUpdateFeatureFlagFailure()])
catchError((res) => {
if (res.error.type == SERVER_ERROR.DUPLICATE_KEY) {
return [FeatureFlagsActions.actionSetIsDuplicateKey({ duplicateKeyFound: true })];
}
return [FeatureFlagsActions.actionAddFeatureFlagFailure()];
})
);
})
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,7 @@ export interface FeatureFlagState extends EntityState<FeatureFlag> {
isLoadingFeatureFlagDelete: boolean;
isLoadingUpsertPrivateSegmentList: boolean;
hasInitialFeatureFlagsDataLoaded: boolean;
duplicateKeyFound: boolean;
activeDetailsTabIndex: number;
skipFlags: number;
totalFlags: number;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export const initialState: FeatureFlagState = adapter.getInitialState({
isLoadingSelectedFeatureFlag: false,
isLoadingUpsertPrivateSegmentList: false,
hasInitialFeatureFlagsDataLoaded: false,
duplicateKeyFound: false,
activeDetailsTabIndex: 0,
skipFlags: 0,
totalFlags: null,
Expand Down Expand Up @@ -75,6 +76,11 @@ const reducer = createReducer(
...state,
isLoadingUpsertFeatureFlag: false,
})),
on(FeatureFlagsActions.actionSetIsDuplicateKey, (state, { duplicateKeyFound }) => ({
...state,
duplicateKeyFound,
isLoadingUpsertFeatureFlag: false,
})),

// Feature Flag Delete Actions
on(FeatureFlagsActions.actionDeleteFeatureFlag, (state) => ({ ...state, isLoadingFeatureFlagDelete: true })),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ export const selectIsLoadingUpsertFeatureFlag = createSelector(
(state) => state.isLoadingUpsertFeatureFlag
);

export const selectDuplicateKeyFound = createSelector(selectFeatureFlagsState, (state) => state.duplicateKeyFound);

export const selectSelectedFeatureFlag = createSelector(
selectRouterState,
selectFeatureFlagsState,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { Observable, throwError } from 'rxjs';
import { catchError } from 'rxjs/operators';
import { ENV, Environment } from '../../../environments/environment-types';
import { AuthService } from '../auth/auth.service';
import { SERVER_ERROR } from 'upgrade_types';

@Injectable()
export class HttpErrorInterceptor implements HttpInterceptor {
Expand All @@ -21,7 +22,9 @@ export class HttpErrorInterceptor implements HttpInterceptor {
content: error.url,
animate: 'fromRight',
};
this._notifications.create(temp.title, temp.content, temp.type, temp);
if (error.error?.type !== SERVER_ERROR.DUPLICATE_KEY) {
this._notifications.create(temp.title, temp.content, temp.type, temp);
}
}

intercept(request: HttpRequest<any>, next: HttpHandler): Observable<HttpEvent<any>> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,38 +5,53 @@
[primaryActionBtnColor]="config.primaryActionBtnColor"
[primaryActionBtnDisabled]="isPrimaryButtonDisabled$ | async"
(primaryActionBtnClicked)="onPrimaryActionBtnClicked()"
>
>
<form [formGroup]="featureFlagForm" class="form-standard dense-3">
<mat-form-field appearance="outline">
<mat-label class="ft-14-400">Name</mat-label>
<input matInput formControlName="name" placeholder="e.g., My feature flag" class="ft-14-400"/>
<input matInput formControlName="name" placeholder="e.g., My feature flag" class="ft-14-400" />
<mat-hint class="form-hint ft-12-400">
{{'feature-flags.upsert-flag-modal.name-hint.text' | translate}}
{{ 'feature-flags.upsert-flag-modal.name-hint.text' | translate }}
</mat-hint>
</mat-form-field>

<mat-form-field appearance="outline">
<mat-label class="ft-14-400">Key</mat-label>
<input matInput formControlName="key" placeholder="Key for this feature flag" class="ft-14-400"/>
<input matInput formControlName="key" placeholder="Key for this feature flag" class="ft-14-400" />
<mat-hint class="form-hint ft-12-400">
{{'feature-flags.upsert-flag-modal.key-hint.text' | translate}}
<a class="learn-more-link" href="https://www.upgradeplatform.org/" target="_blank" rel="noopener noreferrer">Learn more</a>
<ng-container *ngIf="!validationError; else duplicateError">
danoswaltCL marked this conversation as resolved.
Show resolved Hide resolved
<span class="key-hint">
{{ 'feature-flags.upsert-flag-modal.key-hint.text' | translate }}
<a class="learn-more-link" href="https://www.upgradeplatform.org/" target="_blank" rel="noopener noreferrer"
>Learn more</a
>
</span>
</ng-container>
<ng-template #duplicateError>
<span class="ft-14-500 duplicate-error-message">
{{ 'feature-flags.upsert-flag-modal.duplicate-key-error.text' | translate }}
</span>
</ng-template>
</mat-hint>
</mat-form-field>

<mat-form-field appearance="outline">
<mat-label class="ft-14-400">Description (optional)</mat-label>
<input matInput formControlName="description" placeholder="Describe your feature flag" class="ft-14-400"/>
<input matInput formControlName="description" placeholder="Describe your feature flag" class="ft-14-400" />
</mat-form-field>

<mat-form-field appearance="outline">
<mat-label class="ft-14-400">App Context</mat-label>
<mat-select formControlName="appContext" class="ft-14-400">
<mat-option *ngFor="let context of appContexts$ | async" [value]="context" class="ft-14-400">{{ context }}</mat-option>
<mat-option *ngFor="let context of appContexts$ | async" [value]="context" class="ft-14-400">{{
context
}}</mat-option>
</mat-select>
<mat-hint class="form-hint ft-12-400">
{{ 'feature-flags.upsert-flag-modal.app-context-hint.text' | translate}}
<a class="learn-more-link" href="https://www.upgradeplatform.org/" target="_blank" rel="noopener noreferrer">Learn more</a>
{{ 'feature-flags.upsert-flag-modal.app-context-hint.text' | translate }}
<a class="learn-more-link" href="https://www.upgradeplatform.org/" target="_blank" rel="noopener noreferrer"
>Learn more</a
>
</mat-hint>
</mat-form-field>

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
.duplicate-error-message {
danoswaltCL marked this conversation as resolved.
Show resolved Hide resolved
color: var(--red);
}
Loading
Loading