chore(dal): fixed the mongo schema types and repositories to return the correct mapped types #2890

LetItRock · 2023-02-23T12:32:14Z

What change does this PR introduce?

This is the first part of fixing the dal types. At the end of the second part we would have functions to return the correct types when using repository functions, for ex. findById will return Entity | null. Right now it's just Entity which results in errors in the API app when we forget to add the null check.

The Mongoose docs are suggesting using the ObjectId for the field that is "id", but we do type them with string type. Then our repositories are querying DB and doing a mapping to the entity type just to have strings as ids. But having the wrong types on the schema breaks all the Mongoose types system and model functions.

In this PR I've introduced the DBModel interfaces that the Mongoose Schema uses and the Entity type is returned back from the repositories like it was before, so nothing will have to be changed on the API app side.

Why was this change needed?

Other information (Screenshots)

…he correct mapped types

linear · 2023-02-23T12:32:16Z

NV-1753 The repository function `findById` has wrong return type

Why? (Context)

The repository function findById has wrong return type and is leading to the Cannot read properties of null (reading 'FIELD') error across the API app if there is no check for null.

What?

Revisit all the repositories.

Definition of Done

The return type should be like Entity | null and we should add the missed checks across the API app.

LetItRock · 2023-02-23T12:35:02Z

apps/api/migrations/changes-migration.ts

+import { CreateChange } from '../src/app/change/usecases/create-change/create-change.usecase';
+import { CreateChangeCommand } from '../src/app/change/usecases/create-change/create-change.command';


this is the API migration file, I've fixed the imports and ts issues here

LetItRock · 2023-02-23T12:36:20Z

apps/api/src/app/events/usecases/send-message/send-message-in-app.usecase.ts

+    super(
+      messageRepository,
+      createLogUsecase,
+      createExecutionDetails,
+      subscriberRepository,
+      getDecryptedIntegrationsUsecase
+    );


added GetDecryptedIntegrations as it's required by the super class, check below

LetItRock · 2023-02-23T12:36:58Z

apps/api/src/app/events/usecases/send-message/send-message.base.ts

@@ -21,7 +21,7 @@ export abstract class SendMessageBase extends SendMessageType {
    protected createLogUsecase: CreateLog,
    protected createExecutionDetails: CreateExecutionDetails,
    protected subscriberRepository: SubscriberRepository,
-    protected getDecryptedIntegrationsUsecase?: GetDecryptedIntegrations
+    protected getDecryptedIntegrationsUsecase: GetDecryptedIntegrations


this should not be optional as we have the code that uses this use-case

CC. @djabarovgeorge

That's true, I made him optional only because at the moment in-app does not require integration.

LetItRock · 2023-02-23T12:40:48Z

libs/dal/src/repositories/base-repository.ts


-export class BaseRepository<T_Query, T_Response> {
-  public _model: Model<any & Document>;
+export class BaseRepository<T_DBModel, T_MappedEntity> {


Did a lot of type fixes here (no logic changes), but basically, the BaseRepository requires now to pass the T_DBModel which is a DB model, and T_MappedEntity which is the type that we map the DB object to.
So like the Mongoose model return DBModel objects and we map them to Entity, just to have ids as strings, not the ObjectIds.

I really like to differentiate the type of what the repository is expecting and what is returning. Was a pattern I used a lot in the past. I am happy you went that direction.

LetItRock · 2023-02-23T12:43:08Z

libs/dal/src/repositories/change/change.entity.ts

+export type ChangeDBModel = Omit<
+  ChangeEntity,
+  '_creatorId' | '_environmentId' | '_organizationId' | '_entityId' | '_parentId'
+> & {
+  _creatorId: Types.ObjectId;
+
+  _environmentId: Types.ObjectId;
+
+  _organizationId: Types.ObjectId;
+
+  _entityId: Types.ObjectId;
+
+  _parentId?: Types.ObjectId;
+};


This is a DBModel that we pass to the Mongoose schema, please note that id fields have types as Types.ObjectId. So we will have here just to override types for the ids.

Is this the replacement of the query enforcement that @djabarovgeorge implemented?

the query enforcement was to force using _organizationId or _environmentId when querying the model... but as I commented somewhere else this strict rule breaks the types, like the Mongoose model raises TS errors because these fields should be optional

LetItRock · 2023-02-23T12:43:28Z

libs/dal/src/repositories/change/change.entity.ts

 import { ChangeEntityTypeEnum } from '@novu/shared';

+import type { EnvironmentId } from '../environment';
+import type { OrganizationId } from '../organization';
+
 export class ChangeEntity {


this type we do return from the repositories to the outside world

LetItRock · 2023-02-23T12:45:53Z

libs/dal/src/repositories/change/change.schema.ts

+export const Change =
+  (mongoose.models.Change as mongoose.Model<ChangeDBModel>) || mongoose.model<ChangeDBModel>('Change', changeSchema);


I'm not sure why we have here "or", but I guess it's for the case when the model already is created.
So when the first part is true it means that the model is created so we cast it to the correct type.
The second part creates the model passing the correct type and schema.

It is mainly for e2e testin for the exact reason you have mentioned 🙏

LetItRock · 2023-02-23T12:49:59Z

libs/dal/src/repositories/execution-details/execution-details.repository.ts

-class PartialExecutionDetailsEntity extends Omit(ExecutionDetailsEntity, ['_environmentId', '_organizationId']) {}

-type EnforceEnvironmentQuery = FilterQuery<PartialExecutionDetailsEntity & Document> &
-  ({ _environmentId: string } | { _organizationId: string });


Removed these hacks to force the _environmentId or _organizationId, because it breaks all the Mongoose types.
When the query is passed to the model function, the query fields are optional, but we force them to be required and that breaks types.

This was implemented in purpose by @djabarovgeorge. @scopsy already flagged it but what's the replacement for this functionality?

I've answered here: #2890 (comment)
There is no replacement, when you do code the queries you should know which fields you have to use to get the correct DB result...
So the recommendation is when the document has both fields _environmentId, _organizationId we should query by _environmentId as it's org specific. In other cases by the fields that are accessible.

We added it mainly to help us (and the community) from missing those fields. While working on the implementation I encountered a couple of those issues in the project that was missing environment fields.

LetItRock · 2023-02-23T13:20:45Z

libs/dal/src/repositories/job/job.repository.ts

      .populate('template', selectTemplate)
      .populate('notification', selectNotification)
      .populate('subscriber', selectSubscriber)
      .populate('environment', selectEnvironment)
      .lean()
      .exec();
+
+    return job as unknown as JobEntityPopulated;


here we don't do mapping, I don't know why, but I see its failing when calling this.mapEntity(job) and returns {} empty object

Try to replace this.MongooseModel for this._model in line 157 and see if that's the reason.

@p-fernandez that is not the reason, base repository sets the this._model in the constructor, the same as it does for this.MongooseModel. The problem might be the JSON.stringify(entity) call that does the this.mapEntity

scopsy · 2023-02-23T14:09:06Z

apps/api/src/app/events/usecases/send-message/send-message-in-app.usecase.ts

@@ -32,6 +32,7 @@ import { CacheKeyPrefixEnum, InvalidateCacheService } from '../../../shared/serv
 import { SendMessageBase } from './send-message.base';
 import { ApiException } from '../../../shared/exceptions/api.exception';
 import { OrganizationEntity } from '../../../../../../../libs/dal/src/repositories/organization/organization.entity';


I know not related but this import is bad, need to be refactored to be from @novu/dal, don't we have a lint rule for banned imports 🤔

I think we don't. And this was probably and IDE playing around with a fix with an auto import. It is the only occurrence I could find in a quick search in the codebase.

I'll fix this one, but agree we should enable that rule.

scopsy · 2023-02-23T14:10:38Z

...ation-template/usecases/update-notification-template/update-notification-template.usecase.ts

@@ -1,5 +1,6 @@
 // eslint-ignore max-len
 import { BadRequestException, Inject, Injectable, NotFoundException } from '@nestjs/common';
+import { AnyKeys } from 'mongoose';


Unfortuently we should not use mongoose related things in API. Can we re export this thing from the dal package? Ideally we would want the application to know as less as possible about mongoose or mongo

Was going to highlight this. Also what's the difference between AnyKeys type and Partial the one implemented before?

There is a small difference, Partial still requires the correct types for values to be passed, but the AnyKeys allows either for the correct value type or for any other type. So for this file it might work, but I've used it in another place when we do assign the _feedId: null but the type is string | undefined, so it raises the issue there, but yeah I'll fix that place too.

scopsy · 2023-02-23T14:12:59Z

libs/dal/src/repositories/log/log.repository.ts


-class PartialLogEntity extends Omit(LogEntity, ['_environmentId', '_organizationId']) {}
-
-type EnforceEnvironmentQuery = FilterQuery<PartialLogEntity & Document> &


How the enforcement is happening right now? Maybe just missed it

answered here: #2890 (comment)

p-fernandez · 2023-02-24T07:47:24Z

apps/api/src/app/change/usecases/get-changes/get-changes.usecase.ts

@@ -181,7 +182,7 @@ export class GetChanges {
    entityId: string,
    environmentId: string
  ): Promise<IViewEntity | Record<string, unknown>> {
-    let item = await this.layoutRepository.findOne({
+    let item: LayoutEntity | undefined = await this.layoutRepository.findOne({


Do we need to be explicit with the type? Is not infered from the method?

yes, the findOne return type is LayoutEntity which is the issue I aim to solve with both PRs.
But in the second PR these types are removed ;)

p-fernandez · 2023-02-24T07:49:29Z

apps/api/src/app/events/usecases/send-message/send-message-in-app.usecase.ts

@@ -32,6 +32,7 @@ import { CacheKeyPrefixEnum, InvalidateCacheService } from '../../../shared/serv
 import { SendMessageBase } from './send-message.base';
 import { ApiException } from '../../../shared/exceptions/api.exception';
 import { OrganizationEntity } from '../../../../../../../libs/dal/src/repositories/organization/organization.entity';


I think we don't. And this was probably and IDE playing around with a fix with an auto import. It is the only occurrence I could find in a quick search in the codebase.

p-fernandez · 2023-02-24T07:51:21Z

...ation-template/usecases/update-notification-template/update-notification-template.usecase.ts

@@ -1,5 +1,6 @@
 // eslint-ignore max-len
 import { BadRequestException, Inject, Injectable, NotFoundException } from '@nestjs/common';
+import { AnyKeys } from 'mongoose';


Was going to highlight this. Also what's the difference between AnyKeys type and Partial the one implemented before?

p-fernandez · 2023-02-24T07:52:56Z

libs/dal/src/repositories/base-repository.ts


-export class BaseRepository<T_Query, T_Response> {
-  public _model: Model<any & Document>;
+export class BaseRepository<T_DBModel, T_MappedEntity> {


I really like to differentiate the type of what the repository is expecting and what is returning. Was a pattern I used a lot in the past. I am happy you went that direction.

p-fernandez · 2023-02-24T07:54:58Z

libs/dal/src/repositories/change/change.entity.ts

 export class ChangeEntity {
  _id: string;

  _creatorId: string;

-  _environmentId: string;
+  _environmentId: EnvironmentId;


I am pending to implement the way that this really enforces the types to be of this kind (or flavour) as right now will still allow any string to be passed, so still open to get mixed.

p-fernandez · 2023-02-24T07:55:38Z

libs/dal/src/repositories/change/change.entity.ts

+export type ChangeDBModel = Omit<
+  ChangeEntity,
+  '_creatorId' | '_environmentId' | '_organizationId' | '_entityId' | '_parentId'
+> & {
+  _creatorId: Types.ObjectId;
+
+  _environmentId: Types.ObjectId;
+
+  _organizationId: Types.ObjectId;
+
+  _entityId: Types.ObjectId;
+
+  _parentId?: Types.ObjectId;
+};


Is this the replacement of the query enforcement that @djabarovgeorge implemented?

p-fernandez · 2023-02-24T07:59:28Z

libs/dal/src/repositories/execution-details/execution-details.repository.ts

-class PartialExecutionDetailsEntity extends Omit(ExecutionDetailsEntity, ['_environmentId', '_organizationId']) {}

-type EnforceEnvironmentQuery = FilterQuery<PartialExecutionDetailsEntity & Document> &
-  ({ _environmentId: string } | { _organizationId: string });


This was implemented in purpose by @djabarovgeorge. @scopsy already flagged it but what's the replacement for this functionality?

p-fernandez · 2023-02-24T08:01:40Z

libs/dal/src/repositories/job/job.repository.ts

      .populate('template', selectTemplate)
      .populate('notification', selectNotification)
      .populate('subscriber', selectSubscriber)
      .populate('environment', selectEnvironment)
      .lean()
      .exec();
+
+    return job as unknown as JobEntityPopulated;


Try to replace this.MongooseModel for this._model in line 157 and see if that's the reason.

libs/dal/src/repositories/job/job.schema.ts

p-fernandez · 2023-02-24T08:06:18Z

libs/dal/src/repositories/organization/organization.entity.ts

@@ -17,6 +17,8 @@ export class OrganizationEntity {
  partnerConfigurations?: IPartnerConfiguration[];
 }

+export type OrganizationDBModel = OrganizationEntity;


I find weird that the DBModel and the Entity are the same in this case. Shouldn't the entity return with the OrganizationId for the _id?
Also DbModel._id should be of type Types.ObjectId for coherence with the rest of the changes done.

The DbModel._id type is not required by the schema, it's created automatically, so the OrganizationEntity just has to have it defined as the _id: string as it does, and because of that the OrganizationDBModel = OrganizationEntity for this case.

p-fernandez

Great big work again.
For me is approved, but because the query enforcement might be sensitive I'd like to have full agreement with @scopsy and @djabarovgeorge. 👍🏻

LetItRock · 2023-02-27T20:45:02Z

@djabarovgeorge @scopsy @p-fernandez I've found the solution here for the env or org ids enforcement so it will stay ;) please check the latest commit

djabarovgeorge · 2023-02-27T20:53:10Z

Amazing! Looks great to me will review it again tomorrow 🙃, did you had the chance to test it locally it behaves as expected?

LetItRock · 2023-02-27T20:55:49Z

Amazing! Looks great to me will review it again tomorrow 🙃, did you had the chance to test it locally it behaves as expected?

yes, I've tested it and it works the exact same way ;)

p-fernandez

🌟

So for future reference the use will be:
Mongoose -> FooEntity -> FooDBModel -> [MongoDB] -> FooDBModel -> FooEntity -> Mongoose?
Keeping all the transformations inside of the repositories?

p-fernandez · 2023-02-28T09:11:19Z

libs/dal/src/types/enforce.ts

+import type { EnvironmentId } from '../repositories/environment';
+import type { OrganizationId } from '../repositories/organization';
+
+export type EnforceOrgId = { _organizationId: OrganizationId };
+export type EnforceEnvOrOrgIds = { _environmentId: EnvironmentId } | EnforceOrgId;


Great! Only still missing thing (related to what I have pending) is that you could pass any string of any type and it will pass the validation, but for the enforcement query right now is enough. We can make it stricter in the future. 💪🏻

p-fernandez · 2023-02-28T09:13:16Z

libs/dal/src/repositories/subscriber/subscriber.schema.ts

-interface ISubscriberDocument extends SubscriberEntity, Document {
-  _id: never;
-  __v: never;
-}


p-fernandez · 2023-02-28T09:21:29Z

libs/dal/src/repositories/topic/topic-subscribers.entity.ts

+export type TopicSubscribersDBModel = Omit<
+  TopicSubscribersEntity,
+  '_environmentId' | '_organizationId' | '_subscriberId' | '_topicId'
+> & {
+  _environmentId: Types.ObjectId;
+
+  _organizationId: Types.ObjectId;
+
+  _subscriberId: Types.ObjectId;
+
+  _topicId: Types.ObjectId;
+};


Would be so cool if we could implement a generic that could do this automatic type (pseudocode):

type CustomOmit<T, <A extends ReadonlyArray<string>, V = Types.ObjectId>> = Omit<T, A> & { [K in (A extends ReadonlyArray<infer A> ? A : never)]: V }

So we could do something like this:

type TopicSubscriberDBModel = CustomOmit<TopicSubscribersEntity, <'_environmentId' | '_organizationId' | '_subscriberId' | '_topicId', Types.ObjectId>>;

your wish is my order :D

type ChangePropsValueType<T, K extends keyof T, V = Types.ObjectId> = Omit<T, K> & { [P in K]: V; }; type TopicSubscribersDBModel = ChangePropsValueType< TopicSubscribersEntity, '_environmentId' | '_organizationId' | '_subscriberId' | '_topicId' >;

LetItRock · 2023-02-28T21:12:11Z

🌟

So for future reference the use will be: Mongoose -> FooEntity -> FooDBModel -> [MongoDB] -> FooDBModel -> FooEntity -> Mongoose? Keeping all the transformations inside of the repositories?

exactly!

chore(dal): fixed the mongo schema types and repositories to return t…

0ccd903

…he correct mapped types

LetItRock self-assigned this Feb 23, 2023

github-actions bot added @novu/api @novu/dal labels Feb 23, 2023

chore(api): fix migration types

4b98139

LetItRock commented Feb 23, 2023

View reviewed changes

chore(api): fixing failing test

cb25a1d

LetItRock commented Feb 23, 2023

View reviewed changes

scopsy reviewed Feb 23, 2023

View reviewed changes

LetItRock mentioned this pull request Feb 23, 2023

feat(dal,api): strict null check and type fixes #2893

Merged

p-fernandez reviewed Feb 24, 2023

View reviewed changes

LetItRock and others added 2 commits February 24, 2023 09:11

Merge branch 'next' into nv-1753-dal-update-repository-schema-and-model

1ace80b

chore(api): added suggestions from the pr #2890

cad5325

LetItRock requested review from p-fernandez and scopsy February 24, 2023 10:49

p-fernandez reviewed Feb 24, 2023

View reviewed changes

LetItRock and others added 4 commits February 24, 2023 18:07

Merge branch 'next' into nv-1753-dal-update-repository-schema-and-model

f25ffde

Merge branch 'next' into nv-1753-dal-update-repository-schema-and-model

fbb5162

Merge branch 'next' into nv-1753-dal-update-repository-schema-and-model

e36a1ff

chore(api): env or org id enforcement on the repository

4401af6

LetItRock requested review from p-fernandez and djabarovgeorge February 27, 2023 20:45

Merge branch 'next' into nv-1753-dal-update-repository-schema-and-model

1e7c35d

p-fernandez approved these changes Feb 28, 2023

View reviewed changes

LetItRock added 2 commits February 28, 2023 22:30

chore(dal): added helper allowing to change fields to objectid type

bf2e1fc

Merge branch 'next' into nv-1753-dal-update-repository-schema-and-model

081e93c

LetItRock added this pull request to the merge queue Mar 1, 2023

Merged via the queue into next with commit c814bc3 Mar 1, 2023

LetItRock deleted the nv-1753-dal-update-repository-schema-and-model branch March 1, 2023 15:15

		import { CreateChange } from '../src/app/change/usecases/create-change/create-change.usecase';
		import { CreateChangeCommand } from '../src/app/change/usecases/create-change/create-change.command';

		export const Change =
		(mongoose.models.Change as mongoose.Model<ChangeDBModel>) \|\| mongoose.model<ChangeDBModel>('Change', changeSchema);


		class PartialLogEntity extends Omit(LogEntity, ['_environmentId', '_organizationId']) {}

		type EnforceEnvironmentQuery = FilterQuery<PartialLogEntity & Document> &

chore(dal): fixed the mongo schema types and repositories to return the correct mapped types #2890

chore(dal): fixed the mongo schema types and repositories to return the correct mapped types #2890

Conversation

LetItRock commented Feb 23, 2023

What change does this PR introduce?

Why was this change needed?

Other information (Screenshots)

linear bot commented Feb 23, 2023

Why? (Context)

What?

Definition of Done

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

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

p-fernandez left a comment

Choose a reason for hiding this comment

LetItRock commented Feb 27, 2023

djabarovgeorge commented Feb 27, 2023

LetItRock commented Feb 27, 2023

p-fernandez left a 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

LetItRock commented Feb 28, 2023