perf(auth): improve check whether the user is registered

Simplify the parameters Improve the readability
leosuncin · Sep 19, 2022 · 072ac1f · 072ac1f
1 parent 7e936e5
commit 072ac1f
Show file tree

Hide file tree

Showing 6 changed files with 109 additions and 103 deletions.
diff --git a/src/auth/dto/register-user.dto.spec.ts b/src/auth/dto/register-user.dto.spec.ts
@@ -22,7 +22,7 @@ describe('Register user validations', () => {
       .useMocker((token) => {
         if (token === AuthenticationService) {
           return createMock<AuthenticationService>({
-            userNotExistWith: jest.fn().mockResolvedValue(true),
+            isRegistered: jest.fn().mockResolvedValue(false),
           });
         }
 

diff --git a/src/auth/dto/update-user.dto.spec.ts b/src/auth/dto/update-user.dto.spec.ts
@@ -32,7 +32,7 @@ describe('Update user validations', () => {
 
         if (token === AuthenticationService) {
           return createMock<AuthenticationService>({
-            userNotExistWith: jest.fn().mockResolvedValue(true),
+            isRegistered: jest.fn().mockResolvedValue(false),
           });
         }
 

diff --git a/src/auth/services/authentication.service.spec.ts b/src/auth/services/authentication.service.spec.ts
@@ -1,7 +1,7 @@
 import { Test } from '@nestjs/testing';
 import { getRepositoryToken } from '@nestjs/typeorm';
 import { createMock } from 'ts-auto-mock';
-import type { Repository } from 'typeorm';
+import { type FindOptionsWhere, type Repository, Equal, Not } from 'typeorm';
 
 import type { UpdateUser } from '~auth/dto/update-user.dto';
 import { User } from '~auth/entities/user.entity';
@@ -102,28 +102,51 @@ describe('AuthenticationService', () => {
   });
 
   it.each([
-    ['email' as const, user.email, undefined],
-    ['username' as const, user.username, undefined],
-    ['email' as const, user.email, user.id],
-    ['username' as const, user.username, user.id],
-  ])('should check if a user exist', async (property, value, id) => {
-    mockedUserRepository.count.mockResolvedValueOnce(1);
-
-    await expect(service.userNotExistWith(property, value, id)).resolves.toBe(
-      false,
-    );
-  });
+    [{ email: user.email }, { email: Equal(user.email) }],
+    [{ id: user.id }, { id: Not(user.id) }],
+    [{ username: user.username }, { username: Equal(user.username) }],
+    [
+      { id: user.id, username: 'nostrud' },
+      { id: Not(user.id), username: Equal('nostrud') },
+    ],
+    [
+      { id: user.id, email: 'magna@pariatur.do' },
+      { id: Not(user.id), email: Equal('magna@pariatur.do') },
+    ],
+    [
+      // eslint-disable-next-line unicorn/no-null
+      { email: user.email, username: null, id: undefined },
+      { email: Equal(user.email) },
+    ],
+  ])(
+    'should check if it is registered with %j',
+    async (partial, where: FindOptionsWhere<User>) => {
+      mockedUserRepository.countBy.mockResolvedValueOnce(1);
+
+      // @ts-expect-error mocked value
+      await expect(service.isRegistered(partial)).resolves.toBe(true);
+      expect(mockedUserRepository.countBy).toHaveBeenCalledWith(where);
+    },
+  );
 
   it.each([
-    ['email' as const, 'johndoe@example.com', undefined],
-    ['username' as const, 'john', undefined],
-    ['email' as const, 'johndoe@example.com', user.id],
-    ['username' as const, 'john', user.id],
-  ])('should check if a user not exist', async (property, value, id) => {
-    mockedUserRepository.count.mockResolvedValueOnce(0);
-
-    await expect(service.userNotExistWith(property, value, id)).resolves.toBe(
-      true,
-    );
-  });
+    [{ email: 'johndoe@example.com' }, { email: Equal('johndoe@example.com') }],
+    [{ username: 'john' }, { username: Equal('john') }],
+    [
+      { email: 'johndoe@example.com', id: user.id },
+      { email: Equal('johndoe@example.com'), id: Not(user.id) },
+    ],
+    [
+      { username: 'john', id: user.id },
+      { username: Equal('john'), id: Not(user.id) },
+    ],
+  ])(
+    'should check if it is not registered with %j',
+    async (partial, where: FindOptionsWhere<User>) => {
+      mockedUserRepository.countBy.mockResolvedValueOnce(0);
+
+      await expect(service.isRegistered(partial)).resolves.toBe(false);
+      expect(mockedUserRepository.countBy).toHaveBeenCalledWith(where);
+    },
+  );
 });
diff --git a/src/auth/services/authentication.service.ts b/src/auth/services/authentication.service.ts
@@ -1,6 +1,6 @@
 import { Injectable } from '@nestjs/common';
 import { InjectRepository } from '@nestjs/typeorm';
-import { Equal, Not, Repository } from 'typeorm';
+import { type FindOptionsWhere, type Repository, Equal, Not } from 'typeorm';
 
 import type { LoginUser } from '~auth/dto/login-user.dto';
 import type { RegisterUser } from '~auth/dto/register-user.dto';
@@ -41,20 +41,20 @@ export class AuthenticationService {
     return this.userRepository.save(user);
   }
 
-  async userNotExistWith<
-    Property extends keyof Pick<User, 'username' | 'email'>,
-  >(
-    property: Property,
-    value: User[Property],
-    id?: User['id'],
+  async isRegistered(
+    partial: Partial<Pick<User, 'email' | 'id' | 'username'>>,
   ): Promise<boolean> {
-    const count = await this.userRepository.count({
-      where: {
-        [property]: Equal(value),
-        ...(id ? { id: Not(id) } : {}),
-      },
-    });
+    const where: FindOptionsWhere<User> = {};
+
+    for (const [property, value] of Object.entries(partial)) {
+      if (value) {
+        where[property as keyof typeof partial] =
+          property === 'id' ? Not(value) : Equal(value);
+      }
+    }
+
+    const count = await this.userRepository.countBy(where);
 
-    return count === 0;
+    return count >= 1;
   }
 }
diff --git a/src/auth/validators/is-already-register.validator.spec.ts b/src/auth/validators/is-already-register.validator.spec.ts
@@ -2,14 +2,13 @@ import { Test } from '@nestjs/testing';
 import { useContainer, validate } from 'class-validator';
 import { createMock } from 'ts-auto-mock';
 
+import { jane, john } from '~auth/fixtures/users';
 import { AuthenticationService } from '~auth/services/authentication.service';
 import {
   IsAlreadyRegister,
   IsAlreadyRegisterConstraint,
 } from '~auth/validators/is-already-register.validator';
 
-import { jane, john } from '../fixtures/users';
-
 class WithEmail {
   @IsAlreadyRegister()
   readonly email!: string;
@@ -50,122 +49,114 @@ describe('IsAlreadyRegister', () => {
   it('should fail when an user already exists with the same email', async () => {
     const dto = new WithEmail(john.email);
 
-    mockedAuthenticationService.userNotExistWith.mockResolvedValueOnce(false);
+    mockedAuthenticationService.isRegistered.mockResolvedValueOnce(true);
 
     const errors = await validate(dto);
 
     expect(errors).toHaveLength(1);
     expect(errors[0]).toHaveProperty('property', 'email');
-    expect(mockedAuthenticationService.userNotExistWith).toHaveBeenCalledWith(
-      'email',
-      john.email,
-      undefined,
-    );
+    expect(mockedAuthenticationService.isRegistered).toHaveBeenCalledWith({
+      email: john.email,
+      id: undefined,
+    });
   });
 
   it('should fail when an user already exists with the same username', async () => {
     const dto = new WithUsername(john.username);
 
-    mockedAuthenticationService.userNotExistWith.mockResolvedValueOnce(false);
+    mockedAuthenticationService.isRegistered.mockResolvedValueOnce(true);
 
     const errors = await validate(dto);
 
     expect(errors).toHaveLength(1);
     expect(errors[0]).toHaveProperty('property', 'username');
-    expect(mockedAuthenticationService.userNotExistWith).toHaveBeenCalledWith(
-      'username',
-      john.username,
+    expect(mockedAuthenticationService.isRegistered).toHaveBeenCalledWith({
+      username: john.username,
       undefined,
-    );
+    });
   });
 
   it('should pass when no user exists with the email', async () => {
     const dto = new WithEmail(jane.email);
 
-    mockedAuthenticationService.userNotExistWith.mockResolvedValueOnce(true);
+    mockedAuthenticationService.isRegistered.mockResolvedValueOnce(false);
 
     const errors = await validate(dto);
 
     expect(errors).toHaveLength(0);
-    expect(mockedAuthenticationService.userNotExistWith).toHaveBeenCalledWith(
-      'email',
-      jane.email,
-      undefined,
-    );
+    expect(mockedAuthenticationService.isRegistered).toHaveBeenCalledWith({
+      email: jane.email,
+      id: undefined,
+    });
   });
 
   it('should pass when no user exists with the username', async () => {
     const dto = new WithUsername(jane.username);
 
-    mockedAuthenticationService.userNotExistWith.mockResolvedValueOnce(true);
+    mockedAuthenticationService.isRegistered.mockResolvedValueOnce(false);
 
     const errors = await validate(dto);
 
     expect(errors).toHaveLength(0);
-    expect(mockedAuthenticationService.userNotExistWith).toHaveBeenCalledWith(
-      'username',
-      jane.username,
-      undefined,
-    );
+    expect(mockedAuthenticationService.isRegistered).toHaveBeenCalledWith({
+      username: jane.username,
+      id: undefined,
+    });
   });
 
   it('should pass when the email is not used by another user', async () => {
     const dto = new WithEmail('johndoe@example.com', john.id);
 
-    mockedAuthenticationService.userNotExistWith.mockResolvedValueOnce(true);
+    mockedAuthenticationService.isRegistered.mockResolvedValueOnce(false);
 
     const errors = await validate(dto);
 
     expect(errors).toHaveLength(0);
-    expect(mockedAuthenticationService.userNotExistWith).toHaveBeenCalledWith(
-      'email',
-      'johndoe@example.com',
-      dto.id,
-    );
+    expect(mockedAuthenticationService.isRegistered).toHaveBeenCalledWith({
+      email: 'johndoe@example.com',
+      id: dto.id,
+    });
   });
 
   it('should fail when the email is already used by another user', async () => {
     const dto = new WithEmail(jane.email, john.id);
 
-    mockedAuthenticationService.userNotExistWith.mockResolvedValueOnce(false);
+    mockedAuthenticationService.isRegistered.mockResolvedValueOnce(true);
 
     const errors = await validate(dto);
 
     expect(errors).toHaveLength(1);
-    expect(mockedAuthenticationService.userNotExistWith).toHaveBeenCalledWith(
-      'email',
-      jane.email,
-      dto.id,
-    );
+    expect(mockedAuthenticationService.isRegistered).toHaveBeenCalledWith({
+      email: jane.email,
+      id: dto.id,
+    });
   });
 
   it('should pass when the username is not used by another user', async () => {
     const dto = new WithUsername('johndoe', john.id);
 
-    mockedAuthenticationService.userNotExistWith.mockResolvedValueOnce(true);
+    mockedAuthenticationService.isRegistered.mockResolvedValueOnce(false);
 
     const errors = await validate(dto);
 
     expect(errors).toHaveLength(0);
-    expect(mockedAuthenticationService.userNotExistWith).toHaveBeenCalledWith(
-      'username',
-      'johndoe',
-      dto.id,
-    );
+    expect(mockedAuthenticationService.isRegistered).toHaveBeenCalledWith({
+      username: 'johndoe',
+      id: dto.id,
+    });
   });
 
   it('should fail when the username is already used by another user', async () => {
     const dto = new WithUsername(jane.username, john.id);
 
-    mockedAuthenticationService.userNotExistWith.mockResolvedValueOnce(false);
+    mockedAuthenticationService.isRegistered.mockResolvedValueOnce(true);
 
     const errors = await validate(dto);
 
     expect(errors).toHaveLength(1);
-    expect(mockedAuthenticationService.userNotExistWith).toHaveBeenCalledWith(
-      'username',
-      jane.username,
-      dto.id,
-    );
+    expect(mockedAuthenticationService.isRegistered).toHaveBeenCalledWith({
+      username: jane.username,
+      id: dto.id,
+    });
   });
 });
diff --git a/src/auth/validators/is-already-register.validator.ts b/src/auth/validators/is-already-register.validator.ts
@@ -7,7 +7,6 @@ import {
   ValidatorConstraintInterface,
 } from 'class-validator';
 
-import type { User } from '~auth/entities/user.entity';
 import { AuthenticationService } from '~auth/services/authentication.service';
 
 @Injectable()
@@ -17,26 +16,19 @@ export class IsAlreadyRegisterConstraint
 {
   constructor(private readonly authenticationService: AuthenticationService) {}
 
-  validate(value: string, { object, property }: ValidationArguments) {
-    if (!this.isValidProperty(property)) return false;
-
-    return this.authenticationService.userNotExistWith(
-      property,
-      value,
+  async validate(value: string, { object, property }: ValidationArguments) {
+    const userExist = await this.authenticationService.isRegistered({
+      [property]: value,
       // @ts-expect-error object has an id property and it's defined
-      object.id as unknown,
-    );
+      id: object.id as unknown,
+    });
+
+    return !userExist;
   }
 
   defaultMessage(): string {
     return '$property «$value» is already registered';
   }
-
-  private isValidProperty(
-    property: string,
-  ): property is keyof Pick<User, 'email' | 'username'> {
-    return property === 'email' || property === 'username';
-  }
 }
 
 export function IsAlreadyRegister(options: ValidationOptions = {}) {