Issue 1362/total flagged bug

CHANGELOG.md

@@ -15,6 +15,7 @@ All notable changes to this project will be documented in this file. The format
   - Poor TypeORM performance in `/applications` endpoint ([#1131](https://github.com/bloom-housing/bloom/issues/1131)) (Michał Plebański)
   - POST `/users` endpoint response from StatusDTO to UserBasicDto (Michał Plebański)
   - Replaces `toPrecision` function on `units-transformations` to `toFixed` ([#1304](https://github.com/bloom-housing/bloom/pull/1304)) (Marcin Jędras)
+  - "totalFlagged" computation and a race condition on Application insertion ([#1366](https://github.com/bloom-housing/bloom/pull/1366))
 
 - Added:
 

backend/core/package.json

@@ -44,6 +44,7 @@
     "@nestjs/throttler": "^1.1.2",
     "@nestjs/typeorm": "^7.1.0",
     "@types/cache-manager": "^3.4.0",
+    "async-retry": "^1.3.1",
     "axios": "^0.21.0",
     "cache-manager": "^3.4.0",
     "casbin": "^5.1.6",

backend/core/src/application-flagged-sets/application-flagged-sets.service.ts

@@ -3,7 +3,14 @@ import { PaginatedApplicationFlaggedSetQueryParams } from "./application-flagged
 import { AuthzService } from "../auth/authz.service"
 import { ApplicationFlaggedSet } from "./entities/application-flagged-set.entity"
 import { InjectRepository } from "@nestjs/typeorm"
-import { Brackets, DeepPartial, Repository, SelectQueryBuilder } from "typeorm"
+import {
+  Brackets,
+  DeepPartial,
+  Repository,
+  SelectQueryBuilder,
+  getManager,
+  EntityManager,
+} from "typeorm"
 import { paginate } from "nestjs-typeorm-paginate"
 import { Application } from "../applications/entities/application.entity"
 import { ApplicationFlaggedSetResolveDto } from "./dto/application-flagged-set.dto"
@@ -35,7 +42,10 @@ export class ApplicationFlaggedSetsService {
       }
     )
     const countTotalFlagged = await this.afsRepository.count({
-      where: { status: FlaggedSetStatus.flagged },
+      where: {
+        status: FlaggedSetStatus.flagged,
+        ...(queryParams.listingId && { listingId: queryParams.listingId }),
+      },
     })
     return {
       ...results,
@@ -56,63 +66,90 @@ export class ApplicationFlaggedSetsService {
   }
 
   async resolve(dto: ApplicationFlaggedSetResolveDto) {
-    const afs = await this.afsRepository.findOne({
-      where: { id: dto.afsId },
-      relations: ["applications"],
-    })
-    if (!afs) {
-      throw new NotFoundException()
-    }
-    // eslint-disable-next-line @typescript-eslint/no-unnecessary-type-assertion
-    afs.resolvingUser = this.request.user as User
-    afs.resolvedTime = new Date()
-    afs.status = FlaggedSetStatus.resolved
-    const appsToBeResolved = afs.applications.filter((afsApp) =>
-      dto.applications.map((appIdDto) => appIdDto.id).includes(afsApp.id)
-    )
-    const appsNotToBeResolved = afs.applications.filter(
-      (afsApp) => !dto.applications.map((appIdDto) => appIdDto.id).includes(afsApp.id)
-    )
+    return await getManager().transaction("SERIALIZABLE", async (transactionalEntityManager) => {
+      const transAfsRepository = transactionalEntityManager.getRepository(ApplicationFlaggedSet)
+      const transApplicationsRepository = transactionalEntityManager.getRepository(Application)
+      const afs = await transAfsRepository.findOne({
+        where: { id: dto.afsId },
+        relations: ["applications"],
+      })
+      if (!afs) {
+        throw new NotFoundException()
+      }
+      // eslint-disable-next-line @typescript-eslint/no-unnecessary-type-assertion
+      afs.resolvingUser = this.request.user as User
+      afs.resolvedTime = new Date()
+      afs.status = FlaggedSetStatus.resolved
+      const appsToBeResolved = afs.applications.filter((afsApp) =>
+        dto.applications.map((appIdDto) => appIdDto.id).includes(afsApp.id)
+      )
 
-    for (const appToBeResolved of appsToBeResolved) {
-      appToBeResolved.markedAsDuplicate = true
-      await this.applicationsRepository.save(appToBeResolved)
-    }
+      const appsNotToBeResolved = afs.applications.filter(
+        (afsApp) => !dto.applications.map((appIdDto) => appIdDto.id).includes(afsApp.id)
+      )
 
-    for (const appNotToBeResolved of appsNotToBeResolved) {
-      appNotToBeResolved.markedAsDuplicate = false
-      await this.applicationsRepository.save(appNotToBeResolved)
-    }
-    appsToBeResolved.forEach((app) => (app.markedAsDuplicate = true))
-    await this.afsRepository.save(afs)
-    return afs
+      for (const appToBeResolved of appsToBeResolved) {
+        appToBeResolved.markedAsDuplicate = true
+      }
+
+      for (const appNotToBeResolved of appsNotToBeResolved) {
+        appNotToBeResolved.markedAsDuplicate = false
+      }
+
+      await transApplicationsRepository.save([...appsToBeResolved, ...appsNotToBeResolved])
+
+      appsToBeResolved.forEach((app) => (app.markedAsDuplicate = true))
+      await transAfsRepository.save(afs)
+
+      return afs
+    })
   }
 
-  async onApplicationSave(newApplication: Application) {
+  async onApplicationSave(newApplication: Application, transactionalEntityManager: EntityManager) {
     for (const rule of [Rule.email, Rule.nameAndDOB]) {
-      await this.updateApplicationFlaggedSetsForRule(newApplication, rule)
+      await this.updateApplicationFlaggedSetsForRule(
+        transactionalEntityManager,
+        newApplication,
+        rule
+      )
     }
   }
 
-  async fetchDuplicatesMatchingRule(application: Application, rule: Rule) {
+  async fetchDuplicatesMatchingRule(
+    transactionalEntityManager: EntityManager,
+    application: Application,
+    rule: Rule
+  ) {
     switch (rule) {
       case Rule.nameAndDOB:
-        return await this.fetchDuplicatesMatchingNameAndDOBRule(application)
+        return await this.fetchDuplicatesMatchingNameAndDOBRule(
+          transactionalEntityManager,
+          application
+        )
       case Rule.email:
-        return await this.fetchDuplicatesMatchingEmailRule(application)
+        return await this.fetchDuplicatesMatchingEmailRule(transactionalEntityManager, application)
     }
   }
 
-  async updateApplicationFlaggedSetsForRule(newApplication: Application, rule: Rule) {
-    const applicationsMatchingRule = await this.fetchDuplicatesMatchingRule(newApplication, rule)
+  async updateApplicationFlaggedSetsForRule(
+    transactionalEntityManager: EntityManager,
+    newApplication: Application,
+    rule: Rule
+  ) {
+    const applicationsMatchingRule = await this.fetchDuplicatesMatchingRule(
+      transactionalEntityManager,
+      newApplication,
+      rule
+    )
+    const transAfsRepository = transactionalEntityManager.getRepository(ApplicationFlaggedSet)
     const visitedAfses = []
     for (const matchedApplication of applicationsMatchingRule) {
       // NOTE: Optimize it because of N^2 complexity,
       //  for each matched application we create a query returning a list of matching sets
       // TODO: Add filtering into the query, right now all AFSes are fetched for each
       //  matched application which will become a performance problem soon
       const afsesMatchingRule = (
-        await this.afsRepository.find({
+        await transAfsRepository.find({
           join: {
             alias: "afs",
             leftJoinAndSelect: {
@@ -135,15 +172,15 @@ export class ApplicationFlaggedSetsService {
           applications: [newApplication, matchedApplication],
           listing: newApplication.listing,
         }
-        await this.afsRepository.save(newAfs)
+        await transAfsRepository.save(newAfs)
       } else if (afsesMatchingRule.length === 1) {
         for (const afs of afsesMatchingRule) {
           if (visitedAfses.includes(afs.id)) {
             return
           }
           visitedAfses.push(afs.id)
           afs.applications.push(newApplication)
-          await this.afsRepository.save(afs)
+          await transAfsRepository.save(afs)
         }
       } else {
         console.error(
@@ -154,8 +191,12 @@ export class ApplicationFlaggedSetsService {
     }
   }
 
-  private async fetchDuplicatesMatchingEmailRule(newApplication: Application) {
-    return await this.applicationsRepository.find({
+  private async fetchDuplicatesMatchingEmailRule(
+    transactionalEntityManager: EntityManager,
+    newApplication: Application
+  ) {
+    const transApplicationsRepository = transactionalEntityManager.getRepository(Application)
+    return await transApplicationsRepository.find({
       where: (qb: SelectQueryBuilder<Application>) => {
         qb.where("Application.id != :id", {
           id: newApplication.id,
@@ -171,7 +212,11 @@ export class ApplicationFlaggedSetsService {
     })
   }
 
-  private async fetchDuplicatesMatchingNameAndDOBRule(newApplication: Application) {
+  private async fetchDuplicatesMatchingNameAndDOBRule(
+    transactionalEntityManager: EntityManager,
+    newApplication: Application
+  ) {
+    const transApplicationsRepository = transactionalEntityManager.getRepository(Application)
     const firstNames = [
       newApplication.applicant.firstName,
       ...newApplication.householdMembers.map((householdMember) => householdMember.firstName),
@@ -197,7 +242,7 @@ export class ApplicationFlaggedSetsService {
       ...newApplication.householdMembers.map((householdMember) => householdMember.birthYear),
     ]
 
-    return await this.applicationsRepository.find({
+    return await transApplicationsRepository.find({
       where: (qb: SelectQueryBuilder<Application>) => {
         qb.where("Application.id != :id", {
           id: newApplication.id,

backend/core/src/applications/applications.service.ts

@@ -1,8 +1,8 @@
-import { Inject, Injectable, Scope } from "@nestjs/common"
+import { HttpException, HttpStatus, Inject, Injectable, Scope } from "@nestjs/common"
 import { Application } from "./entities/application.entity"
 import { ApplicationCreateDto, ApplicationUpdateDto } from "./dto/application.dto"
 import { InjectRepository } from "@nestjs/typeorm"
-import { Repository } from "typeorm"
+import { getManager, QueryFailedError, Repository } from "typeorm"
 import { paginate, Pagination } from "nestjs-typeorm-paginate"
 import { PaginatedApplicationListQueryParams } from "./applications.controller"
 import { ApplicationFlaggedSetsService } from "../application-flagged-sets/application-flagged-sets.service"
@@ -12,6 +12,7 @@ import { Request as ExpressRequest } from "express"
 import { ListingsService } from "../listings/listings.service"
 import { EmailService } from "../shared/email/email.service"
 import { REQUEST } from "@nestjs/core"
+import retry from "async-retry"
 
 @Injectable({ scope: Scope.REQUEST })
 export class ApplicationsService {
@@ -139,16 +140,62 @@ export class ApplicationsService {
     })
     return qb
   }
-  private async _create(applicationCreateDto: ApplicationUpdateDto) {
-    const application = await this.repository.save({
-      ...applicationCreateDto,
-      user: this.req.user,
+
+  private async _createApplication(applicationCreateDto: ApplicationUpdateDto) {
+    return await getManager().transaction("SERIALIZABLE", async (transactionalEntityManager) => {
+      const applicationsRepository = transactionalEntityManager.getRepository(Application)
+      const application = await applicationsRepository.save({
+        ...applicationCreateDto,
+        user: this.req.user,
+      })
+      await this.applicationFlaggedSetsService.onApplicationSave(
+        application,
+        transactionalEntityManager
+      )
+      return application
     })
+  }
+
+  private async _create(applicationCreateDto: ApplicationUpdateDto) {
+    let application: Application
+
+    try {
+      await retry(
+        async (bail) => {
+          try {
+            application = await this._createApplication(applicationCreateDto)
+          } catch (e) {
+            console.error(e.message)
+            // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+            // @ts-ignore
+            if (!(e instanceof QueryFailedError && e.code === "40001")) {
+              // 40001: could not serialize access due to read/write dependencies among transactions
+              bail(e)
+            }
+            throw e
+          }
+        },
+        { retries: 6, minTimeout: 200 }
+      )
+    } catch (e) {
+      // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+      // @ts-ignore
+      if (e instanceof QueryFailedError && e.code === "40001") {
+        throw new HttpException(
+          {
+            statusCode: HttpStatus.TOO_MANY_REQUESTS,
+            error: "Too Many Requests",
+            message: "Please try again later.",
+          },
+          429
+        )
+      }
+    }
+
     const listing = await this.listingsService.findOne(application.listing.id)
     if (application.applicant.emailAddress) {
       await this.emailService.confirmation(listing, application, applicationCreateDto.appUrl)
     }
-    await this.applicationFlaggedSetsService.onApplicationSave(application)
     return application
   }
 

backend/core/test/afs/afs.e2e-spec.ts

@@ -94,13 +94,15 @@ describe("ApplicationFlaggedSets", () => {
 
     const appContent = getTestAppBody(listing1Id)
     const apps = []
-    for (const payload of [appContent, appContent]) {
-      const appRes = await supertest(app.getHttpServer())
-        .post("/applications/submit")
-        .send(payload)
-        .expect(201)
-      apps.push(appRes)
-    }
+    await Promise.all(
+      [appContent, appContent].map(async (payload) => {
+        const appRes = await supertest(app.getHttpServer())
+          .post("/applications/submit")
+          .send(payload)
+          .expect(201)
+        apps.push(appRes)
+      })
+    )
 
     let afses = await supertest(app.getHttpServer())
       .get(`/applicationFlaggedSets?listingId=${listing1Id}`)
@@ -137,13 +139,15 @@ describe("ApplicationFlaggedSets", () => {
     appContent2.applicant.emailAddress = "another@email.com"
     const apps = []
 
-    for (const payload of [appContent1, appContent2]) {
-      const appRes = await supertest(app.getHttpServer())
-        .post("/applications/submit")
-        .send(payload)
-        .expect(201)
-      apps.push(appRes)
-    }
+    await Promise.all(
+      [appContent1, appContent2].map(async (payload) => {
+        const appRes = await supertest(app.getHttpServer())
+          .post("/applications/submit")
+          .send(payload)
+          .expect(201)
+        apps.push(appRes)
+      })
+    )
 
     let afses = await supertest(app.getHttpServer())
       .get(`/applicationFlaggedSets?listingId=${listing1Id}`)

yarn.lock

@@ -5994,6 +5994,13 @@ async-foreach@^0.1.3:
   resolved "https://registry.yarnpkg.com/async-foreach/-/async-foreach-0.1.3.tgz#36121f845c0578172de419a97dbeb1d16ec34542"
   integrity sha1-NhIfhFwFeBct5Bmpfb6x0W7DRUI=
 
+async-retry@^1.3.1:
+  version "1.3.1"
+  resolved "https://registry.yarnpkg.com/async-retry/-/async-retry-1.3.1.tgz#139f31f8ddce50c0870b0ba558a6079684aaed55"
+  integrity sha512-aiieFW/7h3hY0Bq5d+ktDBejxuwR78vRu9hDUdR8rNhSaQ29VzPL4AoIRG7D/c7tdenwOcKvgPM6tIxB3cB6HA==
+  dependencies:
+    retry "0.12.0"
+
 async@0.9.x:
   version "0.9.2"
   resolved "https://registry.yarnpkg.com/async/-/async-0.9.2.tgz#aea74d5e61c1f899613bf64bda66d4c78f2fd17d"
@@ -17196,6 +17203,11 @@ ret@~0.1.10:
   resolved "https://registry.yarnpkg.com/ret/-/ret-0.1.15.tgz#b8a4825d5bdb1fc3f6f53c2bc33f81388681c7bc"
   integrity sha512-TTlYpa+OL+vMMNG24xSlQGEJ3B/RzEfUlLct7b5G/ytav+wPrplCpVMFuwzXbkecJrb6IYo1iFb0S9v37754mg==
 
+retry@0.12.0:
+  version "0.12.0"
+  resolved "https://registry.yarnpkg.com/retry/-/retry-0.12.0.tgz#1b42a6266a21f07421d1b0b54b7dc167b01c013b"
+  integrity sha1-G0KmJmoh8HQh0bC1S33BZ7AcATs=
+
 retry@^0.10.0:
   version "0.10.1"
   resolved "https://registry.yarnpkg.com/retry/-/retry-0.10.1.tgz#e76388d217992c252750241d3d3956fed98d8ff4"