Typeorm updation #1606

RidhamShah · 2024-06-04T12:40:53Z

This PR covers below changes:

Update typeORM and all related depreciated code to new code.
Now, the export replica hostname would also be an array as a separate env variable TYPEORM_HOSTNAME_EXPORT_REPLICA and rest replicas (if any) would be in the TYPEORM_HOSTNAME_REPLICAS. For now you need to move the value in TYPEORM_HOSTNAME_REPLICAS to TYPEORM_HOSTNAME_EXPORT_REPLICA.
We updated the typeormLoader file to have the mainDB connection use the 2nd read replica as slaves which will be automatically used for any read operation that uses the main DB connection by default. Only the export queries would use the export connection which will hit the 1st read replica. In case we dont have a 2nd read replica, all the reads apart from export queries would hit the main RDS instance.

…ry unit test case

…/UpGrade into upgrade/1282-typeorm

…into upgrade/1282-typeorm

…002084)

(cherry picked from commit 4e7a6ee)

…into upgrade/1282-typeorm

backend/packages/Upgrade/src/loaders/typeormLoader.ts

danoswaltCL · 2024-07-02T18:31:41Z

Currently all of our CL client lib usage is on 5.0, which is fine right now, 5.1 is not required. When this code gets promoted, since still synced with dev, it will carry everything currently in dev also as 5.2.

5.2 shouldn't be required either. Are we sure that this 5.2 TypeOrm update version won't break compatibility with CL apps using 5.0? I ask because a change like this should not be merged in yet, that would break all of our clients on staging and we would need to get them to update their libraries / usage. Can we maybe cease pulling dev and "freeze" into branch this so we don't pull in accidental surprises? Then we can pip dev to like 5.3? @VivekFitkariwala @RidhamShah @bcb37

danoswaltCL · 2024-07-12T19:35:40Z

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

@@ -1014,7 +1014,7 @@ export class ExperimentClientController {
  @Delete('clearDB')
  public async clearDB(@Req() request: AppRequest): Promise<string> {
    // if DEMO mode is enabled, then clear the database:
-    if (env.app.demo) {
+    if (!env.app.demo) {


why this change?

please remove. i have to assume someone was doing this in debugging and accidentally committed it, if this change snuck into prod, anyone with a token would be able to delete our database by doing DELETE /clearDB. It makes me nervous even having this endpoint exist in the first place for this exact reason.

code updated

danoswaltCL · 2024-07-12T19:38:24Z

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

-              return { userId: individual.userId, segment: segment.id };
-            });
-            await transactionalEntityManager.getRepository(IndividualForSegment).delete(usersToDelete as any);
+            // const usersToDelete = segmentDoc.individualForSegment.map((individual) => {


if the commented code isn't needed anymore, let's get rid of it, we can see the old implementation in git history

danoswaltCL · 2024-07-12T19:40:34Z

backend/packages/Upgrade/.env.example

@@ -22,6 +22,7 @@ LOG_OUTPUT=dev
 #
 TYPEORM_CONNECTION=postgres
 TYPEORM_HOST=localhost
+TYPEORM_HOSTNAME_EXPORT_REPLICA=[]


is the separate new env var needed for the typeorm update? does it need to be an array if it's just one thing?

No, it was part of improvement we found while updating replica connection syntax during this PR. Its kept as an array for consistency with the other replica hostnames var. We can make it a string if you want.

ok, this complicates promoting this to production as it's an unexpected configuration change that I don't remember being talked about. @bcb37 @VivekFitkariwala what do you think

danoswaltCL · 2024-07-12T22:14:03Z

Seeing these errors when doing /assign to a context with any experiments enrolling (not just group). I see same thing in local as when I deployed this branch into our staging environment.

{
"client_session_id": null,
"endpoint": "/v5/assign",
"level": "error",
"message": {
"className": "IndividualExclusionRepository",
"code": "42703",
"details": "Error in assignment",
"driverError": {
"code": "42703",
"file": "parse_relation.c",
"length": 126,
"line": "3648",
"name": "error",
"position": "282",
"routine": "errorMissingColumn",
"severity": "ERROR"
},
"file": "parse_relation.c",
"functionName": "findExcluded",
"length": 126,
"line": "3648",
"parameters": {
"experimentIds": [
"83ae5cc5-1e78-4c00-8306-20499058941e",
"abcd6602-f8c6-4779-adf6-127c16af8cd9",
"b39586e4-275d-435a-afda-6e9bbbb72ad9",
"c643eb90-046f-458e-8420-9d9495282ccb"
],
"userId": "locust12"
},
"position": "282",
"query": "SELECT "individualExclusion"."createdAt" AS "individualExclusion_createdAt", "individualExclusion"."updatedAt" AS "individualExclusion_updatedAt", "individualExclusion"."versionNumber" AS "individualExclusion_versionNumber", "individualExclusion"."id" AS "individualExclusion_id", "individualExclusion"."groupId" AS "individualExclusion_groupId", "individualExclusion"."exclusionCode" AS "individualExclusion_exclusionCode", "individualExclusion"."experimentId" AS "individualExclusion_experimentId", "individualExclusion"."userId" AS "individualExclusion_userId", "experiment"."createdAt" AS "experiment_createdAt", "experiment"."updatedAt" AS "experiment_updatedAt", "experiment"."versionNumber" AS "experiment_versionNumber", "experiment"."id" AS "experiment_id", "experiment"."name" AS "experiment_name", "experiment"."description" AS "experiment_description", "experiment"."context" AS "experiment_context", "experiment"."state" AS "experiment_state", "experiment"."startOn" AS "experiment_startOn", "experiment"."consistencyRule" AS "experiment_consistencyRule", "experiment"."assignmentUnit" AS "experiment_assignmentUnit", "experiment"."postExperimentRule" AS "experiment_postExperimentRule", "experiment"."enrollmentCompleteCondition" AS "experiment_enrollmentCompleteCondition", "experiment"."endOn" AS "experiment_endOn", "experiment"."revertTo" AS "experiment_revertTo", "experiment"."tags" AS "experiment_tags", "experiment"."group" AS "experiment_group", "experiment"."conditionOrder" AS "experiment_conditionOrder", "experiment"."assignmentAlgorithm" AS "experiment_assignmentAlgorithm", "experiment"."logging" AS "experiment_logging", "experiment"."filterMode" AS "experiment_filterMode", "experiment"."backendVersion" AS "experiment_backendVersion", "experiment"."type" AS "experiment_type", "experiment"."stratificationFactorStratificationFactorName" AS "experiment_stratificationFactorStratificationFactorName", "user"."createdAt" AS "user_createdAt", "user"."updatedAt" AS "user_updatedAt", "user"."versionNumber" AS "user_versionNumber", "user"."id" AS "user_id", "user"."group" AS "user_group", "user"."workingGroup" AS "user_workingGroup", "user"."originalUserId" AS "user_originalUserId" FROM "individual_exclusion" "individualExclusion" LEFT JOIN "experiment" "experiment" ON "experiment"."id"="individualExclusion"."experimentId" LEFT JOIN "experiment_user" "user" ON "user"."id"="individualExclusion"."userId" WHERE "individualExclusion"."id" IN ($1, $2, $3, $4)",
"routine": "errorMissingColumn",
"severity": "ERROR",
"type": "Error in the assignment algorithm"
},
"request_id": "8fb2d683-de24-45ba-9d55-5d95fe9fd842",
"request_method_type": "POST"
}
{
"client_session_id": null,
"endpoint": "/v5/assign",
"level": "error",
"message": {
"endPoint": "/api/v5/assign",
"message": "column individualExclusion.groupId does not exist",
"name": "QueryFailedError",
"request": {
"context": "assign-prog",
"userId": "locust12"
},
"type": "Error in the assignment algorithm"
},
"request_id": "8fb2d683-de24-45ba-9d55-5d95fe9fd842",
"request_method_type": "POST"
}

danoswaltCL · 2024-07-15T14:36:56Z

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

@@ -1014,7 +1014,7 @@ export class ExperimentClientController {
  @Delete('clearDB')
  public async clearDB(@Req() request: AppRequest): Promise<string> {
    // if DEMO mode is enabled, then clear the database:
-    if (env.app.demo) {
+    if (!env.app.demo) {


please remove. i have to assume someone was doing this in debugging and accidentally committed it, if this change snuck into prod, anyone with a token would be able to delete our database by doing DELETE /clearDB. It makes me nervous even having this endpoint exist in the first place for this exact reason.

danoswaltCL · 2024-07-15T14:53:55Z

Can someone please report out exactly what the breaking changes are in this code that we should be aware of? This branch will essentially be promoted directly to staging and then to prod as its own release apart from dev.

There are definitely some commits that exist in 5.1 that I recall being asked to not merge down into dev, please check that also.

…into upgrade/1282-typeorm

danoswaltCL · 2024-07-16T23:13:38Z

Tested the changes in local and in staging with some quick running of locust tests just to get a sense of the API working, looks good no errors, times looked good also.

Here are my notes on things that need changed for deploying to production. Please help me to note other things to check with this update so we are sure prod's version (release/v5.1) is not going to break when we deploy this branch.

notes:
METRICS var will need to be modified in prod. In staging I removed the METRICS var in order to get it to run, I have not tested correctly the new format.

TYPEORM_HOSTNAME_EXPORT_REPLICA is a new var, what is the correct usage? Is does this replace the other replica array?

Please also notice this.

@bcb37 we should find an hour or so to swap in the db one more time and test it together

…rningWeb/UpGrade into upgrade/1282-typeorm

ppratikcr7 · 2024-07-17T13:41:53Z

Tested the changes in local and in staging with some quick running of locust tests just to get a sense of the API working, looks good no errors, times looked good also.

Here are my notes on things that need changed for deploying to production. Please help me to note other things to check with this update so we are sure prod's version (release/v5.1) is not going to break when we deploy this branch.

notes: METRICS var will need to be modified in prod. In staging I removed the METRICS var in order to get it to run, I have not tested correctly the new format.

TYPEORM_HOSTNAME_EXPORT_REPLICA is a new var, what is the correct usage? Is does this replace the other replica array?

Please also notice this.

@bcb37 we should find an hour or so to swap in the db one more time and test it together

Yeah METRICS needs to be updated.

Now, the export replica hostname would also be an array as a separate env variable TYPEORM_HOSTNAME_EXPORT_REPLICA and rest replicas (if any) would be in the TYPEORM_HOSTNAME_REPLICAS. For now you need to move the value in TYPEORM_HOSTNAME_REPLICAS to TYPEORM_HOSTNAME_EXPORT_REPLICA.

Ridham has reverted that delete api change.

updated

…pdates

VivekFitkariwala and others added 28 commits May 12, 2024 15:01

Removing typedi-typeorm-extensions and updating typeorm

7892463

Updating syntax in the code and fixing integration test cases

8c62634

Reverting typedi 0.10.0 -> 0.8.0

e478009

Fixing Controller and Service unit test cases. Adding sample Reposito…

d95ffc7

…ry unit test case

updated lint errors

442680d

update repository depricated code

5a2c73f

updated all test cases

c441ab4

Solved clearData testcases

35e8709

solve multiple repository called testcases

2ecf865

solved remaining logRepo testcases

3fcd3d0

Update orUpdate method parameters in repository files

cb163d0

Merge branch 'dev' into upgrade/1282-typeorm

c6b15e7

solve global.ts errors

f254123

solved segment failing delete function

9ea7d2d

Merge branch 'dev' into upgrade/1282-typeorm

ea21f5c

update deprecated function in analytics

53f1771

Solved import exp not working

dcc9fee

Fixing type

e1f733f

Merge branch 'upgrade/1282-typeorm' of github.com:CarnegieLearningWeb…

6da36d4

…/UpGrade into upgrade/1282-typeorm

Merge branch 'dev' of https://github.com/CarnegieLearningWeb/UpGrade …

37e5414

…into upgrade/1282-typeorm

solve typerorm changes for repo and export(cherry picked from commit f…

e5566ec

…002084)

resolve test cases

f678bf0

solve failing feature-flag testcases

c969037

Updating model

851a2c8

Adding export connection

18d0244

(cherry picked from commit 4e7a6ee)

Merge branch 'dev' of https://github.com/CarnegieLearningWeb/UpGrade …

b2d4d95

…into upgrade/1282-typeorm

adding separate env variable for export replica and generic replicas

3595a59

added entities and migration imports

c275654

ppratikcr7 reviewed Jun 26, 2024

View reviewed changes

backend/packages/Upgrade/src/loaders/typeormLoader.ts Outdated Show resolved Hide resolved

making common array for entities and migrations

d5e650c

Yagnik56 assigned RidhamShah Jun 28, 2024

solve migration errors

521ffac

RidhamShah marked this pull request as ready for review June 30, 2024 09:47

danoswaltCL mentioned this pull request Jul 2, 2024

Throw error when user not found in ExperimentController #1692

Closed

ppratikcr7 requested review from bcb37 and danoswaltCL July 3, 2024 06:28

ppratikcr7 assigned VivekFitkariwala and ppratikcr7 Jul 3, 2024

ppratikcr7 requested review from ppratikcr7 and removed request for ppratikcr7 July 3, 2024 06:30

danoswaltCL reviewed Jul 12, 2024

View reviewed changes

danoswaltCL previously requested changes Jul 15, 2024

View reviewed changes

danoswaltCL added the priority: high High priority issue label Jul 15, 2024

Merge branch 'dev' of https://github.com/CarnegieLearningWeb/UpGrade …

8d1d8c5

…into upgrade/1282-typeorm

RidhamShah added 2 commits July 17, 2024 14:32

removed unwanted code

8b32f28

Merge branch 'upgrade/1282-typeorm' of https://github.com/CarnegieLea…

ce16d2f

…rningWeb/UpGrade into upgrade/1282-typeorm

ppratikcr7 added 2 commits July 18, 2024 14:46

fix for export csv not working due to uuid type match

56e8bb4

fix for experiments with metrics not getting created with test case u…

92d7893

…pdates

ppratikcr7 marked this pull request as draft August 1, 2024 07:37

ppratikcr7 closed this Aug 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Typeorm updation #1606

Typeorm updation #1606

RidhamShah commented Jun 4, 2024 •

edited by ppratikcr7

Loading

danoswaltCL commented Jul 2, 2024

danoswaltCL Jul 12, 2024

danoswaltCL Jul 15, 2024

RidhamShah Jul 17, 2024

danoswaltCL Jul 12, 2024

danoswaltCL Jul 12, 2024

ppratikcr7 Jul 15, 2024

danoswaltCL Jul 15, 2024

danoswaltCL commented Jul 12, 2024 •

edited

Loading

danoswaltCL Jul 15, 2024

danoswaltCL commented Jul 15, 2024

danoswaltCL commented Jul 16, 2024 •

edited

Loading

ppratikcr7 commented Jul 17, 2024 •

edited

Loading

Typeorm updation #1606

Typeorm updation #1606

Conversation

RidhamShah commented Jun 4, 2024 • edited by ppratikcr7 Loading

danoswaltCL commented Jul 2, 2024

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

danoswaltCL commented Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

danoswaltCL commented Jul 15, 2024

danoswaltCL commented Jul 16, 2024 • edited Loading

ppratikcr7 commented Jul 17, 2024 • edited Loading

RidhamShah commented Jun 4, 2024 •

edited by ppratikcr7

Loading

danoswaltCL commented Jul 12, 2024 •

edited

Loading

danoswaltCL commented Jul 16, 2024 •

edited

Loading

ppratikcr7 commented Jul 17, 2024 •

edited

Loading