Orm #1942

PooyaRaki · 2024-09-23T15:44:17Z

Summary

This pull request introduces TypeORM as the Object-Relational Mapping (ORM) solution for the project. TypeORM simplifies database interactions and enhances code maintainability by mapping database tables to application entities.

Benefits

Reduces boilerplate code and simplifies database operations.
Enhances code readability and maintainability.
Provides built-in support for migrations and schema management.
Improves error handling and validation through TypeORM features.

Changes

Integrated TypeORM into the project.
Updated database connection configuration to support TypeORM.
Created entity models corresponding to database tables.
Updated unit tests to ensure compatibility with TypeORM.

Next Steps

Explore additional features like eager loading and query optimizations in future iterations.

coveralls · 2024-09-27T10:56:50Z

Pull Request Test Coverage Report for Build 11070730341

Details

29 of 116 (25.0%) changed or added relevant lines in 23 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.7%) to 90.21%

Changes Missing Coverage	Changed/Added Lines	%
src/domain/accounts/counterfactual-safes/entities/counterfactual-safe.entity.ts	1	0.0%
migrations/1726754508107-notification_types.ts	3	0.0%
src/datasources/db/v2/postgres-database.module.ts	3	0.0%
src/config/entities/orm.config.ts	4	0.0%
src/datasources/notifications/entities/notification-type.entity.ts	8	0.0%
src/datasources/notifications/entities/notification-subscription-notification-type.entity.ts	10	0.0%
src/datasources/db/v2/postgres-database.service.ts	13	0.0%
src/datasources/notifications/entities/notification-devices.entity.ts	13	0.0%
migrations/1726752966034-notification.ts	16	0.0%
src/datasources/notifications/entities/notification-subscription.entity.ts	16	0.0%

Totals
Change from base Build 11055227805:	-0.7%
Covered Lines:	8439
Relevant Lines:	9001

💛 - Coveralls

iamacook · 2024-09-27T10:58:46Z

src/app.module.ts

+          imports: [ConfigModule],
+          useFactory: (configService: ConfigService) => {
+            return {
+              ...{ autoLoadEntities: true, manualInitialization: true },


Would there be any benefit in moving these to the configuration, e.g. should we be able to change these on the fly?

I considered it as well, but for now, I think it's too much. Since the app heavily relies on manual initialization, it seems better to hardcode it here. For both cases, since they shouldn't be changed, hardcoding seems like the best approach IMO. What do you think?

I like the idea of it being centralised, e.g. if we hardcoded these values in our configuration file but I am equally impartial. If it makes the most sense to have them here, we can keep it as is.

I like the idea of it being centralised
💯
Done ✅
adf7bf4

src/config/entities/postgres.config.ts

iamacook · 2024-09-27T11:01:39Z

src/datasources/notifications/entities/notification-devices.entity.ts

+
+@Entity('push_notification_devices')
+@Unique('device_uuid', ['device_uuid'])
+@Check('device_type', 'device_type IN ("ANDROID", "IOS", "WEB")')


Can we set this to the values of the enum via TypeORM?

Do you mean we should dynamically create the string? TypeOrm Check only accepts string. I wouldn't dynamically create the string as that kills the readability here.

It would be nice to somehow have them linked. We aren't otherwise certain that our enum and this are the same.

How about changing the field type in the database to ENUM? We can handle that in a separate PR.
P.S. I didn't modify the database fields, as it was beyond the scope of this PR.

iamacook · 2024-09-27T11:02:06Z

src/datasources/notifications/entities/notification-devices.entity.ts

+    type: 'varchar',
+    length: 255,
+  })
+  device_type!: string;


If we can use the enum in the Check decorator, this would then be typed that of the enum.

IMO, The best solution is to define this field as an enum in the database. But, that's outside the scope of this PR.

That would be preferable. If it's any benefit, we already have the DeviceType enum in the project.

Do you agree with me that it should be refactored in another PR?

Yes, that's fine with me.

iamacook · 2024-09-27T11:04:30Z

src/datasources/notifications/entities/notification-type.entity.ts

+@Unique('name', ['name'])
+export class NotificationType {
+  @PrimaryGeneratedColumn()
+  id!: number;
+
+  @Column({
+    type: 'varchar',
+    length: 255,
+  })
+  name!: string;


As with the device, could we ensure that the name is of the NotificationType enum?

Yes, we can make it an ENUM in the database. I think it should be fixed in another PR. What do you think?

Agreed. We already have a reference enum in the project - NotificationType.

iamacook · 2024-09-27T11:08:10Z

src/datasources/notifications/entities/notification-devices.entity.ts

+@Entity('push_notification_devices')
+@Unique('device_uuid', ['device_uuid'])
+@Check('device_type', 'device_type IN ("ANDROID", "IOS", "WEB")')
+export class NotificationDevice {


It can be done in a follow up, but I would be in favour of (on the domain level) validating these entities and, as such, we could then implement the type here, e.g.

// Datasource export class NotificationDevice implements z.infer<NotificationDeviceSchema> { ... } // Domain function example() { const deviceFromDatabase = // ... return NotificationDeviceSchema.parse(device) }

I like the idea, but let's do it in the notification PR.

iamacook · 2024-09-27T11:10:05Z

src/datasources/notifications/entities/notification-subscription.entity.ts

+  @Column({
+    type: 'varchar',
+    length: 42,
+  })
+  safe_address!: string;
+
+  @Column({
+    type: 'varchar',
+    nullable: true,
+    length: 42,
+  })
+  signer_address!: string | null;


I think we should use the refinement functionality to call viem's getAddress, ensuring that these addresses are checksummed. We can then set the type to 0x${string} as well.

This will be addressed in a separate PR focused on notifications.

iamacook · 2024-09-30T07:38:45Z

src/config/entities/configuration.ts

@@ -131,11 +131,13 @@ export default () => ({
      apiKey: process.env.INFURA_API_KEY,
    },
  },
+  typeorm: { autoLoadEntities: true, manualInitialization: true },


Nit: I've move these under the db property.

In my opinion, a better approach than the current one would be to use db.orm and Array<db.databases>, allowing us the flexibility to add multiple databases in the future and easily iterate over them. Do you believe we should move Typeorm under db?

db.orm sounds good to me. I wouldn't create and array out of them for now though as we don't have more than one. What do you think?

db.orm also sounds good to me. Regarding the array, no strong opinion, but I'd slightly prefer not having it until it's needed. It can be easily added later if we need to add another db.

@iamacook @hectorgomezv What do you think about moving orm under db and placing postgres under db.connection? My concern is that having orm and postgres under the same parent could lead to ambiguity. For now, db.connection will remain an object, but in the future, we can convert it to an array and rename it to db.connections(add an s to make it plural).

Sounds good to me @PooyaRaki, thanks! 👍

iamacook · 2024-09-30T07:39:27Z

src/config/entities/postgres.config.ts

+    password: postgresEnvConfig.password,
+    database: postgresEnvConfig.database,
+    migrations: ['dist/migrations/*.js'],
+    // logging: !isCIContext,


If we can't have CI logging, I'd remove this.

I would say let's keep it, it's like a TODO. I should have been more clear by adding a TODO.

src/datasources/db/v2/postgres-database.service.ts

iamacook · 2024-09-30T11:56:23Z

To keep track of our discussion, we need to consider automatic migrations across multiple instances.

src/app.module.ts

PooyaRaki · 2024-10-17T14:10:31Z

To keep track of our discussion, we need to consider automatic migrations across multiple instances.

@iamacook
This has been implemented in #1988

- Introduces a new query logger to TypeORM. - Ensures TypeORM migrations run after locking the database during startup.

PooyaRaki force-pushed the orm branch from 6910e1a to 58de588 Compare September 27, 2024 10:50

PooyaRaki marked this pull request as ready for review September 27, 2024 10:55

PooyaRaki requested a review from a team as a code owner September 27, 2024 10:55

iamacook reviewed Sep 27, 2024

View reviewed changes

PooyaRaki force-pushed the orm branch from 58de588 to 24011fc Compare September 27, 2024 11:11

PooyaRaki requested a review from iamacook September 27, 2024 12:41

iamacook reviewed Sep 30, 2024

View reviewed changes

hectorgomezv approved these changes Sep 30, 2024

View reviewed changes

PooyaRaki force-pushed the orm branch from adf7bf4 to 242dd53 Compare October 15, 2024 15:44

iamacook reviewed Oct 16, 2024

View reviewed changes

src/app.module.ts Outdated Show resolved Hide resolved

PooyaRaki force-pushed the orm branch from 29fc38b to 450edc6 Compare October 18, 2024 08:59

PooyaRaki requested review from iamacook and hectorgomezv October 18, 2024 13:46

iamacook approved these changes Oct 22, 2024

View reviewed changes

PooyaRaki added 12 commits October 22, 2024 16:26

add typeorm dependencies and scripts

4138cd9

update migrations

7920b2a

add orm related configurations

6d2bfaf

register typeorm

940b13f

add new notification entities

160715b

registers typeorm manually, restructure datasources

9ad5806

add nestjs typeorm dependency

504e6aa

move typeorm config values to the configuration file

8885c12

Move new v1 eigration to deprecated folder

c6bd76e

Fix linting issues

de4e99b

OrmMigrationAndLogger (#1988)

4e6bed0

- Introduces a new query logger to TypeORM. - Ensures TypeORM migrations run after locking the database during startup.

move the new v1 migration to deprecated folder

1757c22

PooyaRaki force-pushed the orm branch from 715d3c2 to 1757c22 Compare October 22, 2024 14:28

iamacook approved these changes Oct 22, 2024

View reviewed changes

PooyaRaki merged commit c95a038 into main Oct 22, 2024
20 checks passed

PooyaRaki deleted the orm branch October 22, 2024 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Orm #1942

Orm #1942

PooyaRaki commented Sep 23, 2024 •

edited

Loading

coveralls commented Sep 27, 2024 •

edited

Loading

iamacook Sep 27, 2024

PooyaRaki Sep 27, 2024

iamacook Sep 27, 2024

PooyaRaki Sep 27, 2024

iamacook Sep 27, 2024

PooyaRaki Sep 27, 2024

iamacook Sep 27, 2024

PooyaRaki Sep 27, 2024

iamacook Sep 27, 2024

PooyaRaki Sep 27, 2024

iamacook Sep 27, 2024

PooyaRaki Sep 27, 2024

iamacook Sep 27, 2024

iamacook Sep 27, 2024

PooyaRaki Sep 27, 2024

iamacook Sep 27, 2024

iamacook Sep 27, 2024

PooyaRaki Sep 27, 2024

iamacook Sep 27, 2024

PooyaRaki Sep 27, 2024

iamacook Sep 30, 2024

PooyaRaki Sep 30, 2024

iamacook Sep 30, 2024

hectorgomezv Sep 30, 2024

PooyaRaki Sep 30, 2024

hectorgomezv Oct 1, 2024

iamacook Sep 30, 2024

PooyaRaki Sep 30, 2024

iamacook commented Sep 30, 2024

PooyaRaki commented Oct 17, 2024 •

edited

Loading

Orm #1942

Orm #1942

Conversation

PooyaRaki commented Sep 23, 2024 • edited Loading

Summary

Benefits

Changes

Next Steps

coveralls commented Sep 27, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11070730341

Details

💛 - Coveralls

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

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

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

iamacook commented Sep 30, 2024

PooyaRaki commented Oct 17, 2024 • edited Loading

PooyaRaki commented Sep 23, 2024 •

edited

Loading

coveralls commented Sep 27, 2024 •

edited

Loading

PooyaRaki commented Oct 17, 2024 •

edited

Loading