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

LetItRock · 2023-02-23T20:43:30Z

What change does this PR introduce?

This is the second part, the previous is #2890.
Here I've enabled the strict null check flag in the TS build config for DAL and API, resulting in many null/undefined errors in the API app.
Solved ~300 TS issues.

Some of these errors were visible in the Sentry, ex. here: https://novu-r9.sentry.io/issues/3841226005/?project=6248811&query=is%3Aunresolved&referrer=issue-stream

Why was this change needed?

Other information (Screenshots)

linear · 2023-02-23T20:43:33Z

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-23T20:44:58Z

apps/api/src/app/auth/usecases/switch-environment/switch-environment.usecase.ts

@@ -20,7 +20,11 @@ export class SwitchEnvironment {
    }

    const member = await this.memberRepository.findMemberByUserId(command.organizationId, command.userId);
+    if (!member) throw new NotFoundException('Member is not found');


mainly in a lot of places I've added these kind of checks, because the repository function now returns Entity | null

LetItRock · 2023-02-23T20:48:06Z

apps/api/src/app/auth/usecases/password-reset-request/password-reset-request.usecase.ts

@@ -18,7 +18,7 @@ export class PasswordResetRequest {
  async execute(command: PasswordResetRequestCommand): Promise<{ success: boolean }> {
    const email = normalizeEmail(command.email);
    const foundUser = await this.userRepository.findByEmail(email);
-    if (foundUser) {
+    if (foundUser && foundUser.email) {


this is a reset use-case and user.email should exist

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

apps/api/src/app/events/services/chat-service/handlers/slack.handler.ts

-    const config: {
-      applicationId: string;
-      clientId: string;
-      secretKey: string;
-    } = { applicationId: credentials.applicationId, clientId: credentials.clientId, secretKey: credentials.secretKey };


these were actually not used in the SlackProvider

LetItRock · 2023-02-23T20:50:07Z

apps/api/src/app/events/services/chat-service/interfaces/index.ts

@@ -9,5 +9,5 @@ export interface IChatHandler {
 }

 export interface IChatFactory {
-  getHandler(integration: IntegrationEntity): IChatHandler;
+  getHandler(integration: IntegrationEntity): IChatHandler | null;


email, push, sms, chat factories might not return the handler, so this is the correct return type

LetItRock · 2023-02-23T20:53:19Z

apps/api/src/app/events/usecases/send-message/digest/get-digest-events-regular.usecase.ts

+    if (!currentJob) throw new ApiException('Digest job is not found');
+
    const amount =
-      typeof currentJob?.digest.amount === 'number'
-        ? currentJob?.digest.amount
-        : parseInt(currentJob.digest.amount, 10);
+      typeof currentJob.digest?.amount === 'number'
+        ? currentJob.digest.amount
+        : parseInt(currentJob.digest?.amount ?? '0', 10);
    const earliest = sub(new Date(currentJob.createdAt), {
-      [currentJob.digest.unit]: amount,
+      [currentJob.digest?.unit ?? DigestUnitEnum.MINUTES]: amount,


there were multiple TS errors, I've added default values, hope this solution is correct ;D

We shouldn't do defaults here as would be confusing for the user if for any reason or bug they have selected hours or days and the digest gets executed based in minutes.

@p-fernandez so what's your suggestion? I can add the // @ts-ignore comments...
I'll revert it because it works with NaN and undefined preserving the date:

We should throw an exception to be honest. But it is not responsibility of this PR.
Please add the ignore comment and a TODO, please. 🙏🏻

LetItRock · 2023-02-23T20:54:18Z

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

+    if (!currentTrigger) {
+      this.createExecutionDetails.execute(
+        CreateExecutionDetailsCommand.create({
+          ...CreateExecutionDetailsCommand.getDetailsFromJob(currentJob),
+          detail: DetailEnum.DIGEST_TRIGGERED_EVENTS,
+          source: ExecutionDetailsSourceEnum.INTERNAL,
+          status: ExecutionDetailsStatusEnum.FAILED,
+          isTest: false,
+          isRetry: false,
+        })
+      );
+      throw new ApiException('Trigger job is not found');


this should not be possible, but in the case

LetItRock · 2023-02-23T20:57:23Z

apps/api/src/app/layouts/usecases/update-layout/update-layout.use-case.ts

@@ -90,7 +90,7 @@ export class UpdateLayoutUseCase {
  private mapToEntity(layout: LayoutDto): LayoutEntity {
    return {
      ...layout,
-      _id: layout._id,
+      _id: layout._id as string,


didn't want to fix this issue here because it requires a lot of changes

LetItRock · 2023-02-23T20:57:55Z

...src/app/message-template/usecases/create-message-template/create-message-template.usecase.ts

@@ -37,7 +37,7 @@ export class CreateMessageTemplate {
      actor: command.actor,
    });

-    item = await this.messageTemplateRepository.findById(item._id);
+    item = (await this.messageTemplateRepository.findById(item._id)) as MessageTemplateEntity;


the message template is created above

LetItRock · 2023-02-23T20:59:30Z

apps/api/src/app/shared/services/mail/mail.service.ts

+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    const mailObject: any = {


also wasn't able to fix the TS issues here

LetItRock · 2023-02-23T21:00:12Z

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

@@ -77,7 +77,7 @@ export class BaseRepository<T_DBModel, T_MappedEntity> {
      })
      .batchSize(batchSize)
      .cursor()) {
-      yield this.mapEntities(doc);
+      yield this.mapEntity(doc);


the doc is a single entity

p-fernandez · 2023-02-24T08:11:35Z

apps/api/src/app/events/services/chat-service/chat.factory.ts

@@ -7,7 +7,7 @@ import { MSTeamsHandler } from './handlers/msteams.handler';
 export class ChatFactory implements IChatFactory {
  handlers: IChatHandler[] = [new SlackHandler(), new DiscordHandler(), new MSTeamsHandler()];

-  getHandler(integration: IntegrationEntity): IChatHandler {
+  getHandler(integration: IntegrationEntity) {


What's the reason behind removing some of the return types?

the functions are defined in the interface IChatFactory, so you don't have to redefine them here

p-fernandez · 2023-02-24T08:12:14Z

apps/api/src/app/events/services/chat-service/handlers/slack.handler.ts

-    const config: {
-      applicationId: string;
-      clientId: string;
-      secretKey: string;
-    } = { applicationId: credentials.applicationId, clientId: credentials.clientId, secretKey: credentials.secretKey };


p-fernandez · 2023-02-24T08:13:07Z

apps/api/src/app/events/services/chat-service/handlers/base.handler.ts

@@ -15,7 +15,7 @@ export abstract class BaseChatHandler implements IChatHandler {

  async send(chatContent: IChatOptions) {
    if (process.env.NODE_ENV === 'test') {
-      return null;
+      return {};


I particularly do not like this. Returning an empty object I think it can cause a lot of problems and doesn't make much sense, to be honest.

If not an empty object then I'll have to add null to the return type and do additional checks in the code for that null, and null is only required for the tests, so to me it feels odd.

We are in different hills 😂 .
But having to return anything I'd go for undefined for the same reasons I mentioned in a different comment. Also because return; returns undefined.
Your PR, your choice, though. 👍🏻

I think this empty object is fine for now, it's only returned in testing env...

apps/api/src/app/events/services/push-service/handlers/fcm.handler.ts

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

apps/api/src/app/notifications/activity.controller.ts

apps/api/src/app/shared/interceptors/shared-cache.ts

apps/api/src/app/shared/shared.module.ts

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

providers/burst-sms/src/lib/burst-sms.provider.ts

…-1753-dal-and-api-strict-null-check

p-fernandez

🌟
Big big work. Thank you for that.

p-fernandez · 2023-02-24T14:30:12Z

apps/api/src/app/logs/usecases/create-log/create-log.usecase.ts

@@ -7,7 +7,7 @@ export class CreateLog {
  constructor(private logRepository: LogRepository) {}

  async execute(command: CreateLogCommand): Promise<LogEntity> {
-    let rawData: string = null;
+    let rawData: string | null = null;


I truly thought TS would interpret it as undefined type. Wow, my bad. 😓

I know I am insisting a bit on the issue in a different commands but not keen on using null. It is not directly passing to JSON.stringify but JSON.stringify(null) becomes 'null' while JSON.stringify(undefined) becomes undefined. In this case it would be in the command.raw but if arrives to rawData we can have problems thinking that could get an empty value while it could hold null stringified.
Just wanted to highlight those risks of using null because they can lead to unexpected bugs. So apologies for the repetition. 🙏🏻

p-fernandez · 2023-02-24T14:35:03Z

apps/api/src/app/events/usecases/send-message/digest/get-digest-events-regular.usecase.ts

+    if (!currentJob) throw new ApiException('Digest job is not found');
+
    const amount =
-      typeof currentJob?.digest.amount === 'number'
-        ? currentJob?.digest.amount
-        : parseInt(currentJob.digest.amount, 10);
+      typeof currentJob.digest?.amount === 'number'
+        ? currentJob.digest.amount
+        : parseInt(currentJob.digest?.amount ?? '0', 10);
    const earliest = sub(new Date(currentJob.createdAt), {
-      [currentJob.digest.unit]: amount,
+      [currentJob.digest?.unit ?? DigestUnitEnum.MINUTES]: amount,


We should throw an exception to be honest. But it is not responsibility of this PR.
Please add the ignore comment and a TODO, please. 🙏🏻

p-fernandez · 2023-02-24T14:36:21Z

apps/api/src/app/integrations/usecases/create-integration/create-integration.command.ts

@@ -19,5 +19,5 @@ export class CreateIntegrationCommand extends EnvironmentCommand {
  check: boolean;

  @IsOptional()
-  userId?: string;
+  userId: string;


Then let's remove the isOptional. 💪🏻

…-1753-dal-and-api-strict-null-check

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

cc2ffdb

LetItRock self-assigned this Feb 23, 2023

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

LetItRock commented Feb 23, 2023

View reviewed changes

chore(web): fixing ts issues

f127444

github-actions bot added the @novu/web label Feb 23, 2023

p-fernandez reviewed Feb 24, 2023

View reviewed changes

LetItRock added 2 commits February 24, 2023 10:11

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

5122e00

…-1753-dal-and-api-strict-null-check

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

4305c86

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

p-fernandez approved these changes Feb 24, 2023

View reviewed changes

LetItRock and others added 3 commits February 27, 2023 22:00

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

60e2541

…-1753-dal-and-api-strict-null-check

fix(api): fixed a new ts issue

e0f21b4

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

cbddb75

…-1753-dal-and-api-strict-null-check

Base automatically changed from nv-1753-dal-update-repository-schema-and-model to next March 1, 2023 15:15

LetItRock added 4 commits March 2, 2023 10:17

Merge branch 'next' into nv-1753-dal-and-api-strict-null-check

5ea24fa

chore(api): added suggestion from pr #2893

589267e

chore: increase the pipeline wait timeouts

125426b

chore(ws): fixed the ts issue

0dca08f

github-actions bot added the @novu/ws label Mar 2, 2023

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

Merged via the queue into next with commit 144aa8c Mar 2, 2023

LetItRock deleted the nv-1753-dal-and-api-strict-null-check branch March 2, 2023 11:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

LetItRock commented Feb 23, 2023 •

edited

linear bot commented Feb 23, 2023

Why? (Context)

What?

Definition of Done

LetItRock Feb 23, 2023

LetItRock Feb 23, 2023

LetItRock Feb 23, 2023

p-fernandez Feb 24, 2023

LetItRock Feb 23, 2023

LetItRock Feb 23, 2023

p-fernandez Feb 24, 2023

LetItRock Feb 24, 2023

p-fernandez Feb 24, 2023

LetItRock Feb 23, 2023

LetItRock Feb 23, 2023

LetItRock Feb 23, 2023

LetItRock Feb 23, 2023

LetItRock Feb 23, 2023

p-fernandez Feb 24, 2023

LetItRock Feb 24, 2023

p-fernandez Feb 24, 2023

p-fernandez Feb 24, 2023

LetItRock Feb 24, 2023 •

edited

p-fernandez Feb 24, 2023

LetItRock Mar 2, 2023

p-fernandez left a comment

p-fernandez Feb 24, 2023

p-fernandez Feb 24, 2023

p-fernandez Feb 24, 2023

		// eslint-disable-next-line @typescript-eslint/no-explicit-any
		const mailObject: any = {

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

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

Conversation

LetItRock commented Feb 23, 2023 • edited

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

LetItRock Feb 24, 2023 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LetItRock commented Feb 23, 2023 •

edited

LetItRock Feb 24, 2023 •

edited