feature flag lists backend changes #1719

bcb37 · 2024-07-02T19:51:29Z

With these changes, the feature flag endpoints will not be used to perform CRUD operations on the inclusion and exclusion lists (with the exception of GET by :id, which will return the lists along with the other feature flag data)

Operations on the lists will be performed using the segments endpoints, which have three new optional attributes:

enabled: boolean
includedInFeatureFlag: FeatureFlag
excludedFromFeatureFlag: FeatureFlag

Example body for flag creation (POST to /api/flags endpoint):

{
    "name": "Test Flag",
    "key": "TEST_FLAG",
    "description": "Test Description",
    "status": "enabled",
    "context": [
        "assign-prog"
    ],
    "tags": [
        "tag99",
        "tag96"
    ],
    "filterMode": "includeAll"   
}

Example body to add or edit an inclusion list for the above (POST to /api/flags/addInclusionList endpoint):

{
    "flagId": "78f3293e-c797-44e2-81f3-8c6a6b830003",
    "enabled": true,
    "listType": "segment",
    "list": {
        "id": "2f86e720-d8b9-47b4-88f3-fa9784784902", // if editing
        "name": "seg to seg",
        "context": "assign-prog",
        "userIds": [],
        "groups":[],
        "subSegmentIds":["fe826649-cacf-4c3d-9f79-e96e05307892"]
    }
    }

Deleting lists can be done by hitting the /api/segments/:id DELETE endpoint

This would use the ID of the feature flag; it's for a 'segments' list, which can only have one element.

assuming the above UUIDs refer to entities that exist in the db.

danoswaltCL · 2024-07-03T17:38:28Z

This change is good by me but I'd like @VivekFitkariwala and others to give it a good look. I'm going to proceed with the assumption that this is pretty much how it will be for re-creating the include/exclude modal stories.

danoswaltCL · 2024-07-03T20:47:15Z

Ok, I'm working on the modals for adding/modifying a flag...we have slightly different constraints for add and modify requests that we can enforce with types, so would you say these are accurate representations of our types (FeatureFlag being the full document from the db that we should expect as a response?)

export interface AddFeatureFlagRequest {
  name: string;
  key: string;
  description?: string;
  context: string[];
  tags: string[];
}

export interface ModifyFeatureFlagRequest {
  name?: string;
  key?: string;
  description?: string;
  status?: FEATURE_FLAG_STATUS;
  filterMode?: FILTER_MODE;
  context?: string[];
  tags?: string[];
  enabled?: boolean;
}

export interface FeatureFlag {
  createdAt: string;
  updatedAt: string;
  versionNumber: number;
  id: string;
  name: string;
  key: string;
  description: string;
  status: FEATURE_FLAG_STATUS;
  filterMode: FILTER_MODE;
  context: string[];
  tags: string[];
  enabled: boolean;
}

bcb37 · 2024-07-03T22:30:01Z

Ok, I'm working on the modals for adding/modifying a flag...we have slightly different constraints for add and modify requests that we can enforce with types, so would you say these are accurate representations of our types (FeatureFlag being the full document from the db that we should expect as a response?)
export interface AddFeatureFlagRequest {
  name: string;
  key: string;
  description?: string;
  context: string[];
  tags: string[];
}

export interface ModifyFeatureFlagRequest {
  name?: string;
  key?: string;
  description?: string;
  status?: FEATURE_FLAG_STATUS;
  filterMode?: FILTER_MODE;
  context?: string[];
  tags?: string[];
  enabled?: boolean;
}

export interface FeatureFlag {
  createdAt: string;
  updatedAt: string;
  versionNumber: number;
  id: string;
  name: string;
  key: string;
  description: string;
  status: FEATURE_FLAG_STATUS;
  filterMode: FILTER_MODE;
  context: string[];
  tags: string[];
  enabled: boolean;
}

This looks correct, with the following exceptions:

Add and Modify should be the same: they both should have 'context' and 'filterMode'
Neither Add nor Modify should have 'enabled', which is not a thing (that's handled by 'status)
The response from the GET endpoint for a single Feature Flag would also contain two arrays: 'featureFlagSegmentInclusion' and 'featureFlagSegmentExclusion', which would be arrays of private segments.

VivekFitkariwala · 2024-07-07T20:03:04Z

@bcb37 I am not sure why you have deleted the junction table of Feature Flag Segment Exclusion and Feature Flag Segment Inclusion. Because of this, the relationship data is now moved into the Segment table. If you had added the enabled property in these junction tables and changed the OneToOne relationship to OneToMany in the Feature Flag table you could have achieved the same thing.

bcb37 · 2024-07-08T19:13:45Z

@bcb37 I am not sure why you have deleted the junction table of Feature Flag Segment Exclusion and Feature Flag Segment Exclusion. Because of this, the relationship data is now moved into the Segment table. If you had added the enabled property in these junction tables and changed the OneToOne relationship to OneToMany in the Feature Flag table you could have achieved the same thing.

@VivekFitkariwala I was thinking it's better to simplify the database and data models. We can now store the information we need with fewer moving parts, and query with fewer joins. And now we can manage the inclusions and exclusions from the front end just using the segment endpoints that already exist. Is there a compelling reason to use those junction tables for the relationships between feature flags and segments?

VivekFitkariwala · 2024-07-09T13:21:14Z

@bcb37 this change is adding property which doesn't belong to Segment and that is the concern. I don't know if we are facing any issues because of this specific join.
Thinking ahead when we have to refactor the Experiment we will end up adding those specific data also in the Segment table. The present design of table is better I think.

bcb37 · 2024-07-09T19:27:13Z

@bcb37 this change is adding property which doesn't belong to Segment and that is the concern. I don't know if we are facing any issues because of this specific join. Thinking ahead when we have to refactor the Experiment we will end up adding those specific data also in the Segment table. The present design of table is better I think.

@VivekFitkariwala Ok, that makes sense to restore the junction tables. I think we'll still need new endpoints for editing the lists. They should be relatively similar to the /segment endpoints, with the addition of managing the junction table entries. Since we don't create feature flags initially with any exclusion or inclusion lists, I still think it makes sense to edit them separately.

danoswaltCL · 2024-07-15T14:17:17Z

@VivekFitkariwala please consider this the highest priority review...

putting comments from dev sync in writing: it was suggested that PUT api/flags/addInclusionList/:id for edit may be preferred for the edit flow over POST /api/flags/addInclusionList.

also enums for the listType were discussed, but I think we landed on leaving it as is because group listType is not constant, comes from the context metadata.

Zack mentioned that it might be best API practice to only need to send changed fields when updating a known entity, but that is a larger architectural design change, currently we tend to always pass the entire document (as was my understanding of that concern)

…gieLearningWeb/UpGrade into feature/feature-flag-lists-backend

VivekFitkariwala · 2024-07-16T05:05:28Z

backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts

+   */
+  @Delete('/inclusionList/:id')
+  public async deleteInclusionList(
+    @Param('id') segmentId: string,


You can modify this and use @Params . This way you can create a class validator to validate id and remove the if condition from the line below.

This seems to be how single UUID parameters are verified throughout the codebase. If we create a validator that checks whether something's a UUID, we should probably start using it everywhere. Maybe a separate task for that.

VivekFitkariwala · 2024-07-16T05:06:53Z

backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts

+   */
+  @Delete('/exclusionList/:id')
+  public async deleteExclusionList(
+    @Param('id') segmentId: string,


Same as above

VivekFitkariwala · 2024-07-16T05:07:35Z

backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts

+  @Delete('/exclusionList/:id')
+  public async deleteExclusionList(
+    @Param('id') segmentId: string,
+    @CurrentUser() currentUser: User,


currentUser can be removed

VivekFitkariwala · 2024-07-16T05:21:09Z

backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts

+   */
+  @Put('/exclusionList/:id')
+  public async updateExclusionList(
+    @Param('id') segmentId: string,


Same as below

VivekFitkariwala · 2024-07-16T06:26:17Z

backend/packages/Upgrade/src/api/controllers/SegmentController.ts

@@ -53,6 +53,12 @@ export interface getSegmentData {
 *         type: array
 *         items:
 *           type: string
+ *       enabled:


Do we need this?

VivekFitkariwala · 2024-07-16T06:33:07Z

backend/packages/Upgrade/src/api/models/FeatureFlagSegmentExclusion.ts

    onDelete: 'CASCADE',
    primary: true,
  })
  @JoinColumn()
  public featureFlag: FeatureFlag;
+
+  @Column()


We can add a default value as false.

VivekFitkariwala · 2024-07-16T06:33:42Z

backend/packages/Upgrade/src/api/models/FeatureFlagSegmentInclusion.ts

    onDelete: 'CASCADE',
    primary: true,
  })
  @JoinColumn()
  public featureFlag: FeatureFlag;
+


Same as in Segment Exclusion

VivekFitkariwala · 2024-07-16T06:43:56Z

backend/packages/Upgrade/src/api/services/FeatureFlagService.ts

+      if (featureFlag) {
+        const deletedFlag = await this.featureFlagRepository.deleteById(featureFlagId, transactionalEntityManager);
+
+        featureFlag.featureFlagSegmentInclusion.forEach(async (segmentInclusion) => {


Do we need this? The cascade delete should automatically delete this.

In that case the transaction is not required

VivekFitkariwala · 2024-07-16T06:52:13Z

@danoswaltCL Sorry for the delay. I have added my review comments.

feature flag lists backend changes

49904dd

bcb37 requested review from VivekFitkariwala, ppratikcr7, danoswaltCL and zackcl July 2, 2024 19:53

update swagger

5b3fdfe

bcb37 added 2 commits July 3, 2024 15:00

Merge branch 'dev' into feature/feature-flag-lists-backend

9782fb7

Merge branch 'dev' into feature/feature-flag-lists-backend

10cfb89

Yagnik56 assigned bcb37 Jul 4, 2024

restore junction tables, support public segments

c12fa3d

bcb37 marked this pull request as draft July 11, 2024 22:47

simplify endpoints

c11eecc

bcb37 marked this pull request as ready for review July 12, 2024 21:59

Merge branch 'dev' into feature/feature-flag-lists-backend

6ffd573

rationalize endpoints

fe6b67c

danoswaltCL added priority: high High priority issue epic: feature flags labels Jul 15, 2024

bcb37 added 4 commits July 15, 2024 20:36

fix test

70cc65a

Merge branch 'dev' into feature/feature-flag-lists-backend

95017ea

Merge branch 'dev' into feature/feature-flag-lists-backend

c96eab5

Merge branch 'feature/feature-flag-lists-backend' of github.com:Carne…

ee49985

…gieLearningWeb/UpGrade into feature/feature-flag-lists-backend

VivekFitkariwala requested changes Jul 16, 2024

View reviewed changes

bcb37 added 2 commits July 16, 2024 15:48

review changes

7422546

Merge branch 'dev' into feature/feature-flag-lists-backend

c693800

bcb37 requested a review from VivekFitkariwala July 16, 2024 19:56

VivekFitkariwala approved these changes Jul 18, 2024

View reviewed changes

bcb37 merged commit 825ce38 into dev Jul 18, 2024
8 checks passed

bcb37 deleted the feature/feature-flag-lists-backend branch July 18, 2024 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature flag lists backend changes #1719

feature flag lists backend changes #1719

bcb37 commented Jul 2, 2024 •

edited

Loading

danoswaltCL commented Jul 3, 2024

danoswaltCL commented Jul 3, 2024

bcb37 commented Jul 3, 2024

VivekFitkariwala commented Jul 7, 2024 •

edited

Loading

bcb37 commented Jul 8, 2024 •

edited

Loading

VivekFitkariwala commented Jul 9, 2024

bcb37 commented Jul 9, 2024 •

edited

Loading

danoswaltCL commented Jul 15, 2024 •

edited

Loading

VivekFitkariwala Jul 16, 2024

bcb37 Jul 16, 2024

VivekFitkariwala Jul 16, 2024

VivekFitkariwala Jul 16, 2024

VivekFitkariwala Jul 16, 2024

VivekFitkariwala Jul 16, 2024

VivekFitkariwala Jul 16, 2024

VivekFitkariwala Jul 16, 2024

VivekFitkariwala Jul 16, 2024

VivekFitkariwala Jul 16, 2024

VivekFitkariwala commented Jul 16, 2024

feature flag lists backend changes #1719

feature flag lists backend changes #1719

Conversation

bcb37 commented Jul 2, 2024 • edited Loading

danoswaltCL commented Jul 3, 2024

danoswaltCL commented Jul 3, 2024

bcb37 commented Jul 3, 2024

VivekFitkariwala commented Jul 7, 2024 • edited Loading

bcb37 commented Jul 8, 2024 • edited Loading

VivekFitkariwala commented Jul 9, 2024

bcb37 commented Jul 9, 2024 • edited Loading

danoswaltCL commented Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VivekFitkariwala commented Jul 16, 2024

bcb37 commented Jul 2, 2024 •

edited

Loading

VivekFitkariwala commented Jul 7, 2024 •

edited

Loading

bcb37 commented Jul 8, 2024 •

edited

Loading

bcb37 commented Jul 9, 2024 •

edited

Loading

danoswaltCL commented Jul 15, 2024 •

edited

Loading