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 4 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 @@ -16,6 +16,14 @@ interface FeatureFlagsPaginationInfo extends PaginationResponse {
nodes: FeatureFlag[];
}

export interface FeatureFlagFormData {
RidhamShah marked this conversation as resolved.
Show resolved Hide resolved
name: string;
key: string;
description: string;
appContext: string;
tags: string[];
}

/**
* @swagger
* definitions:
Expand Down Expand Up @@ -300,6 +308,38 @@ export class FeatureFlagsController {
return this.featureFlagService.create(flag, request.logger);
}

/**
RidhamShah marked this conversation as resolved.
Show resolved Hide resolved
* @swagger
* /flags/validation:
* post:
* description: Check for duplicate feature flags name or key
* consumes:
* - application/json
* parameters:
* - in: body
* name: flag
* required: true
* schema:
* type: object
* $ref: '#/definitions/FeatureFlag'
* description: Feature flag structure
* tags:
* - Feature Flags
* produces:
* - application/json
* responses:
* '200':
* description: Feature flag name or key is unique
* '409':
* description: Feature flag name or key already exists
*/
@Post('/validation')
public validateFeatureFlags(
@Body({ validate: false }) flag: FeatureFlagFormData,
@Req() request: AppRequest
): Promise<string> {
return this.featureFlagService.validate(flag, request.logger);
}
/**
* @swagger
* /flags/status:
Expand Down
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,11 +1,12 @@
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';
import { FEATURE_FLAG_STATUS, FILTER_MODE } from 'upgrade_types';
import { FeatureFlagSegmentInclusion } from './FeatureFlagSegmentInclusion';
import { FeatureFlagSegmentExclusion } from './FeatureFlagSegmentExclusion';
@Entity()
@Unique(['key', 'context'])
export class FeatureFlag extends BaseModel {
@PrimaryColumn('uuid')
public id: string;
Expand All @@ -14,7 +15,7 @@ export class FeatureFlag extends BaseModel {
@Column()
public name: string;

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

@Column()
Expand Down
18 changes: 18 additions & 0 deletions backend/packages/Upgrade/src/api/services/FeatureFlagService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import { ExperimentUser } from '../models/ExperimentUser';
import { ExperimentAssignmentService } from './ExperimentAssignmentService';
import { SegmentService } from './SegmentService';
import { ErrorWithType } from '../errors/ErrorWithType';
import { FeatureFlagFormData } from '../controllers/FeatureFlagController';

@Service()
export class FeatureFlagService {
Expand Down Expand Up @@ -204,6 +205,23 @@ export class FeatureFlagService {
return this.segmentService.deleteSegment(segmentId, logger);
}

public async validate(flagDTO: FeatureFlagFormData, logger: UpgradeLogger): Promise<string> {
logger.info({ message: `Validating featureFlags` });
const result = await this.featureFlagRepository
RidhamShah marked this conversation as resolved.
Show resolved Hide resolved
.createQueryBuilder('feature_flag')
.where('feature_flag.key = :key', { key: flagDTO.key })
.andWhere('feature_flag.context = :context', { context: [flagDTO.appContext] })
.getOne();

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 '';
}

public async addList(
listInput: FeatureFlagListValidator,
filterType: string,
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 @@ -4,6 +4,7 @@ import { HttpClient } from '@angular/common/http';
import {
AddFeatureFlagRequest,
FeatureFlag,
FeatureFlagFormData,
FeatureFlagsPaginationInfo,
FeatureFlagsPaginationParams,
UpdateFeatureFlagRequest,
Expand Down Expand Up @@ -40,6 +41,11 @@ export class FeatureFlagsDataService {
return this.http.put<FeatureFlag>(url, flag);
}

validateFeatureFlag(flag: FeatureFlagFormData) {
const url = `${this.environment.api.featureFlag}/validation`;
return this.http.post(url, flag);
}

deleteFeatureFlag(id: string) {
const url = `${this.environment.api.featureFlag}/${id}`;
return this.http.delete(url);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,17 @@
<mat-label class="ft-14-400">Key</mat-label>
<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.duplicate-key-error-text' | translate }}
</span>
</ng-template>
</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);
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { ChangeDetectionStrategy, Component, Inject } from '@angular/core';
import { ChangeDetectionStrategy, ChangeDetectorRef, Component, Inject } from '@angular/core';
import {
CommonModalComponent,
CommonTagsInputComponent,
Expand Down Expand Up @@ -36,6 +36,7 @@ import { BehaviorSubject, combineLatestWith, map, Observable, startWith, Subscri
import { TranslateModule } from '@ngx-translate/core';
import { ExperimentService } from '../../../../../core/experiments/experiments.service';
import { CommonTextHelpersService } from '../../../../../shared/services/common-text-helpers.service';
import { FeatureFlagsDataService } from '../../../../../core/feature-flags/feature-flags.data.service';
import isEqual from 'lodash.isequal';

@Component({
Expand Down Expand Up @@ -76,6 +77,7 @@ export class UpsertFeatureFlagModalComponent {
initialFormValues$ = new BehaviorSubject<FeatureFlagFormData>(null);

featureFlagForm: FormGroup;
validationError = false;

constructor(
@Inject(MAT_DIALOG_DATA)
Expand All @@ -85,6 +87,8 @@ export class UpsertFeatureFlagModalComponent {
private featureFlagsService: FeatureFlagsService,
private experimentService: ExperimentService,
private formHelpersService: CommonFormHelpersService,
private featureFlagDataService: FeatureFlagsDataService,
private cdr: ChangeDetectorRef,
public dialogRef: MatDialogRef<UpsertFeatureFlagModalComponent>
) {}

Expand Down Expand Up @@ -154,16 +158,31 @@ export class UpsertFeatureFlagModalComponent {
this.subscriptions.add(this.isSelectedFeatureFlagUpdated$.subscribe(() => this.closeModal()));
}

onPrimaryActionBtnClicked(): void {
onPrimaryActionBtnClicked() {
if (this.featureFlagForm.valid) {
// Handle extra frontend form validation logic here?
this.sendRequest(this.config.params.action, this.config.params.sourceFlag);
this.sendValidateRequest().then((validationError) => {
this.validationError = validationError;
this.cdr.detectChanges();
if (!this.validationError) {
this.sendRequest(this.config.params.action, this.config.params.sourceFlag);
}
});
} else {
// If the form is invalid, manually mark all form controls as touched
this.formHelpersService.triggerTouchedToDisplayErrors(this.featureFlagForm);
}
}

async sendValidateRequest() {
const formData: FeatureFlagFormData = this.featureFlagForm.value;
try {
const result = (await this.featureFlagDataService.validateFeatureFlag(formData).toPromise()) as string;
return !!result;
} catch (error) {
return true;
}
}

sendRequest(action: UPSERT_FEATURE_FLAG_ACTION, sourceFlag?: FeatureFlag): void {
const formData: FeatureFlagFormData = this.featureFlagForm.value;

Expand Down
1 change: 1 addition & 0 deletions frontend/projects/upgrade/src/assets/i18n/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,7 @@
"feature-flags.import-feature-flag.text": "Import Feature Flag",
"feature-flags.import-feature-flag.message.text": "The Feature Flag JSON file should include the required properties for it to be imported.",
"feature-flags.export-all-feature-flags.text": "Export All Feature Flags",
"feature-flags.duplicate-key-error-text": "Feature flag with this key already exists for this app-context.",
"segments.title.text": "Segments",
"segments.subtitle.text": "Define new segments to include or exclude from any experiment",
"segments.no-segments.text": "Welcome! <br> Let's start by creating a new segment!",
Expand Down
1 change: 1 addition & 0 deletions types/src/Experiment/enums.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ export enum SERVER_ERROR {
EXPERIMENT_ID_MISSING_FOR_SHARED_DECISIONPOINT = 'Experiment ID not provided for shared Decision Point',
INVALID_EXPERIMENT_ID_FOR_SHARED_DECISIONPOINT = 'Experiment ID provided is invalid for shared Decision Point',
UNSUPPORTED_CALIPER = 'Caliper profile or event not supported',
DUPLICATE_KEY = 'Feature Flag with same key already exists for this app-context',
}

export enum MARKED_DECISION_POINT_STATUS {
Expand Down
Loading