From 60cba34680fede5fc94864c6ceb56a881220d4ce Mon Sep 17 00:00:00 2001 From: "Caogang (Marcelo) Shen" Date: Wed, 28 Feb 2024 17:00:00 -0800 Subject: [PATCH] Bug Fix for Transaction Read/Write Error (#401) * added packages * sample implementation test * lint fix * testing base functionalities * lint fix * more testing * omfg i got it * clean up testing related variables and functions * lint fix * added comments * added concurrent tests * bumped version number --- package.json | 4 +- repositories/index.ts | 31 +++++- tests/Seeds.ts | 5 - tests/data/EventFactory.ts | 4 +- tests/data/FactoryUtils.ts | 4 + tests/data/MerchFactory.ts | 2 +- tests/userSocialMedia.test.ts | 189 +++++++++++++++++++++++++++++++++- yarn.lock | 24 +++++ 8 files changed, 248 insertions(+), 15 deletions(-) diff --git a/package.json b/package.json index a92c3ce52..9d4a142a1 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@acmucsd/membership-portal", - "version": "3.3.0", + "version": "3.4.0", "description": "REST API for ACM UCSD's membership portal.", "main": "index.d.ts", "files": [ @@ -39,6 +39,7 @@ "dependencies": { "@sendgrid/mail": "^7.4.4", "amazon-s3-uri": "^0.1.1", + "async-retry": "^1.3.3", "aws-sdk": "^2.906.0", "bcrypt": "^5.0.0", "body-parser": "^1.19.0", @@ -71,6 +72,7 @@ "winston-daily-rotate-file": "^4.5.0" }, "devDependencies": { + "@types/async-retry": "^1.4.8", "@types/bcrypt": "^3.0.0", "@types/body-parser": "^1.19.0", "@types/datadog-metrics": "^0.6.1", diff --git a/repositories/index.ts b/repositories/index.ts index e08de61e8..9ede2b5c5 100644 --- a/repositories/index.ts +++ b/repositories/index.ts @@ -1,4 +1,5 @@ import { EntityManager } from 'typeorm'; +import AsyncRetry = require('async-retry'); import { UserRepository } from './UserRepository'; import { FeedbackRepository } from './FeedbackRepository'; import { AttendanceRepository, ExpressCheckinRepository } from './AttendanceRepository'; @@ -16,6 +17,9 @@ import { LeaderboardRepository } from './LeaderboardRepository'; import { ResumeRepository } from './ResumeRepository'; import { UserSocialMediaRepository } from './UserSocialMediaRepository'; +const HINT_FOR_RETRIABLE_TRANSACTIONS : string = 'The transaction might succeed if retried.'; +const AMOUNT_OF_RETRIES : number = 10; + export default class Repositories { public static activity(transactionalEntityManager: EntityManager): ActivityRepository { return transactionalEntityManager.getCustomRepository(ActivityRepository); @@ -93,11 +97,34 @@ export class TransactionsManager { this.transactionalEntityManager = transactionalEntityManager; } + // used async-retry library to handle automatic retries under transaction failure due to concurrent transactions public readOnly(fn: (transactionalEntityManager: EntityManager) => Promise): Promise { - return this.transactionalEntityManager.transaction('REPEATABLE READ', fn); + return AsyncRetry(async (bail, attemptNum) => { + try { + const res = await this.transactionalEntityManager.transaction('REPEATABLE READ', fn); + return res; + } catch (e) { + if (e.hint !== HINT_FOR_RETRIABLE_TRANSACTIONS) bail(e); + else throw e; + } + }, + { + retries: AMOUNT_OF_RETRIES, + }); } public readWrite(fn: (transactionalEntityManager: EntityManager) => Promise): Promise { - return this.transactionalEntityManager.transaction('SERIALIZABLE', fn); + return AsyncRetry(async (bail, attemptNum) => { + try { + const res = await this.transactionalEntityManager.transaction('SERIALIZABLE', fn); + return res; + } catch (e) { + if (e.hint !== HINT_FOR_RETRIABLE_TRANSACTIONS) bail(e); + else throw e; + } + }, + { + retries: AMOUNT_OF_RETRIES, + }); } } diff --git a/tests/Seeds.ts b/tests/Seeds.ts index 7954f1973..a3626228a 100644 --- a/tests/Seeds.ts +++ b/tests/Seeds.ts @@ -479,7 +479,6 @@ async function seed(): Promise { }); const MERCH_ITEM_1_PHOTO_1 = MerchFactory.fakePhoto({ merchItem: MERCH_ITEM_1, - uploadedPhoto: 'https://www.fakepicture.com/', position: 1, }); const MERCH_ITEM_1_PHOTO_2 = MerchFactory.fakePhoto({ @@ -542,7 +541,6 @@ async function seed(): Promise { ]; const MERCH_ITEM_2_PHOTO_0 = MerchFactory.fakePhoto({ merchItem: MERCH_ITEM_2, - uploadedPhoto: 'https://www.fakepicture.com/', position: 0, }); const MERCH_ITEM_2_PHOTO_1 = MerchFactory.fakePhoto({ @@ -591,7 +589,6 @@ async function seed(): Promise { }); const MERCH_ITEM_3_PHOTO_1 = MerchFactory.fakePhoto({ merchItem: MERCH_ITEM_3, - uploadedPhoto: 'https://www.fakepicture.com/', position: 1, }); const MERCH_ITEM_3_PHOTO_2 = MerchFactory.fakePhoto({ @@ -601,12 +598,10 @@ async function seed(): Promise { }); const MERCH_ITEM_3_PHOTO_3 = MerchFactory.fakePhoto({ merchItem: MERCH_ITEM_3, - uploadedPhoto: 'https://www.fakepicture.com/', position: 3, }); const MERCH_ITEM_3_PHOTO_4 = MerchFactory.fakePhoto({ merchItem: MERCH_ITEM_3, - uploadedPhoto: 'https://www.fakepicture.com/', position: 4, }); MERCH_ITEM_3.merchPhotos = [ diff --git a/tests/data/EventFactory.ts b/tests/data/EventFactory.ts index c6dccd2ed..d07f523b3 100644 --- a/tests/data/EventFactory.ts +++ b/tests/data/EventFactory.ts @@ -37,10 +37,10 @@ export class EventFactory { requiresStaff: FactoryUtils.getRandomBoolean(), staffPointBonus: EventFactory.randomPointValue(), committee: 'ACM', - cover: faker.internet.url(), + cover: FactoryUtils.getRandomImageUrl(), deleted: false, eventLink: faker.internet.url(), - thumbnail: faker.internet.url(), + thumbnail: FactoryUtils.getRandomImageUrl(), }); return EventModel.merge(fake, substitute); } diff --git a/tests/data/FactoryUtils.ts b/tests/data/FactoryUtils.ts index 9bd9f9cdd..567990cff 100644 --- a/tests/data/FactoryUtils.ts +++ b/tests/data/FactoryUtils.ts @@ -40,4 +40,8 @@ export default class FactoryUtils { public static randomHexString(): string { return faker.datatype.hexaDecimal(10); } + + public static getRandomImageUrl(): string { + return `http://i.imgur.com/${faker.random.alphaNumeric(10)}.jpeg`; + } } diff --git a/tests/data/MerchFactory.ts b/tests/data/MerchFactory.ts index b28349329..5885fde04 100644 --- a/tests/data/MerchFactory.ts +++ b/tests/data/MerchFactory.ts @@ -72,7 +72,7 @@ export class MerchFactory { const fake = MerchandiseItemPhotoModel.create({ uuid: uuid(), position: 0, - uploadedPhoto: 'https://www.fakepicture.com/', + uploadedPhoto: FactoryUtils.getRandomImageUrl(), uploadedAt: faker.date.recent(), }); return MerchandiseItemPhotoModel.merge(fake, substitute); diff --git a/tests/userSocialMedia.test.ts b/tests/userSocialMedia.test.ts index 98938edc6..f2b39778b 100644 --- a/tests/userSocialMedia.test.ts +++ b/tests/userSocialMedia.test.ts @@ -1,24 +1,26 @@ import faker = require('faker'); +import { Connection } from 'typeorm'; import { UserModel } from '../models/UserModel'; import { SocialMediaType } from '../types'; import { ControllerFactory } from './controllers'; import { DatabaseConnection, PortalState, UserFactory } from './data'; import { UserSocialMediaFactory } from './data/UserSocialMediaFactory'; +import { UserController } from '../api/controllers/UserController'; beforeAll(async () => { await DatabaseConnection.connect(); }); -beforeEach(async () => { - await DatabaseConnection.clear(); -}); - afterAll(async () => { await DatabaseConnection.clear(); await DatabaseConnection.close(); }); describe('social media URL submission', () => { + beforeEach(async () => { + await DatabaseConnection.clear(); + }); + test('properly persists on successful submission', async () => { const conn = await DatabaseConnection.get(); let member = UserFactory.fake(); @@ -60,6 +62,10 @@ describe('social media URL submission', () => { }); describe('social media URL update', () => { + beforeEach(async () => { + await DatabaseConnection.clear(); + }); + test('is invalidated when updating social media URL of another user', async () => { const conn = await DatabaseConnection.get(); let member = UserFactory.fake(); @@ -100,6 +106,10 @@ describe('social media URL update', () => { }); describe('social media URL delete', () => { + beforeEach(async () => { + await DatabaseConnection.clear(); + }); + test('is invalidated when deleting social media URL of another user', async () => { const conn = await DatabaseConnection.get(); let member = UserFactory.fake(); @@ -119,3 +129,174 @@ describe('social media URL delete', () => { await expect(userController.deleteSocialMediaForUser(uuidParams, unauthorizedMember)).rejects.toThrow(errorMessage); }); }); + +describe('social media URL update', () => { + let conn : Connection; + let member : UserModel; + let userController : UserController; + + let flag : boolean = false; + function waitForFlag(interval = 500, timeout = 5000) { + let acc = 0; // time accumulation + return new Promise((resolve, reject) => { + const i = setInterval(() => { + acc += interval; + if (flag) { + clearInterval(i); + resolve(flag); + } + if (acc > timeout) { + clearInterval(i); + reject(); + } + }, interval); + }); + } + + // beforeAll does not behave properly when concurrent is being used: + // https://stackoverflow.com/questions/42633635/how-to-run-concurrent-tests-in-jest-with-multiple-tests-per-request + beforeAll(async () => { + await DatabaseConnection.clear(); + conn = await DatabaseConnection.get(); + member = UserFactory.fake(); + + const userSocialMedia0 = UserSocialMediaFactory.fake({ user: member, type: SocialMediaType.FACEBOOK }); + const userSocialMedia1 = UserSocialMediaFactory.fake({ user: member, type: SocialMediaType.GITHUB }); + const userSocialMedia2 = UserSocialMediaFactory.fake({ user: member, type: SocialMediaType.DEVPOST }); + const userSocialMedia3 = UserSocialMediaFactory.fake({ user: member, type: SocialMediaType.EMAIL }); + const userSocialMedia4 = UserSocialMediaFactory.fake({ user: member, type: SocialMediaType.INSTAGRAM }); + + await new PortalState() + .createUsers(member) + .createUserSocialMedia( + member, userSocialMedia0, userSocialMedia1, userSocialMedia2, userSocialMedia3, userSocialMedia4, + ).write(); + + userController = ControllerFactory.user(conn); + + // refreshes member to have the userSocialMedia field + member = await conn.manager.findOne(UserModel, { uuid: member.uuid }, { relations: ['userSocialMedia'] }); + + flag = true; + }); + + test.concurrent('concurrent updates properly persist on successful submission 0', async () => { + await waitForFlag(); + const index = 0; + const uuidParams = { uuid: member.userSocialMedia[index].uuid }; + const socialMediaParams = { socialMedia: { url: faker.internet.url() } }; + await userController.updateSocialMediaForUser(uuidParams, socialMediaParams, member); + + const expectedUserSocialMedia0 = { + url: socialMediaParams.socialMedia.url, + type: member.userSocialMedia[index].type, + uuid: uuidParams.uuid, + }; + const updatedMember = await conn.manager.findOne( + UserModel, { uuid: member.uuid }, { relations: ['userSocialMedia'] }, + ); + + expect(updatedMember.userSocialMedia).toHaveLength(5); + const targetUserSocialMedia = updatedMember.userSocialMedia.find( + (socialMedia) => socialMedia.uuid === expectedUserSocialMedia0.uuid, + ); + expect(targetUserSocialMedia.url).toEqual(expectedUserSocialMedia0.url); + expect(targetUserSocialMedia.type).toEqual(expectedUserSocialMedia0.type); + }); + + test.concurrent('concurrent updates properly persist on successful submission 1', async () => { + await waitForFlag(); + const index = 1; + const uuidParams = { uuid: member.userSocialMedia[index].uuid }; + const socialMediaParams = { socialMedia: { url: faker.internet.url() } }; + await userController.updateSocialMediaForUser(uuidParams, socialMediaParams, member); + + const expectedUserSocialMedia1 = { + url: socialMediaParams.socialMedia.url, + type: member.userSocialMedia[index].type, + uuid: uuidParams.uuid, + }; + const updatedMember = await conn.manager.findOne( + UserModel, { uuid: member.uuid }, { relations: ['userSocialMedia'] }, + ); + + expect(updatedMember.userSocialMedia).toHaveLength(5); + const targetUserSocialMedia = updatedMember.userSocialMedia.find( + (socialMedia) => socialMedia.uuid === expectedUserSocialMedia1.uuid, + ); + expect(targetUserSocialMedia.url).toEqual(expectedUserSocialMedia1.url); + expect(targetUserSocialMedia.type).toEqual(expectedUserSocialMedia1.type); + }); + + test.concurrent('concurrent updates properly persist on successful submission 2', async () => { + await waitForFlag(); + const index = 2; + const uuidParams = { uuid: member.userSocialMedia[index].uuid }; + const socialMediaParams = { socialMedia: { url: faker.internet.url() } }; + await userController.updateSocialMediaForUser(uuidParams, socialMediaParams, member); + + const expectedUserSocialMedia2 = { + url: socialMediaParams.socialMedia.url, + type: member.userSocialMedia[index].type, + uuid: uuidParams.uuid, + }; + const updatedMember = await conn.manager.findOne( + UserModel, { uuid: member.uuid }, { relations: ['userSocialMedia'] }, + ); + + expect(updatedMember.userSocialMedia).toHaveLength(5); + const targetUserSocialMedia = updatedMember.userSocialMedia.find( + (socialMedia) => socialMedia.uuid === expectedUserSocialMedia2.uuid, + ); + expect(targetUserSocialMedia.url).toEqual(expectedUserSocialMedia2.url); + expect(targetUserSocialMedia.type).toEqual(expectedUserSocialMedia2.type); + }); + + test.concurrent('concurrent updates properly persist on successful submission 3', async () => { + await waitForFlag(); + const index = 3; + const uuidParams = { uuid: member.userSocialMedia[index].uuid }; + const socialMediaParams = { socialMedia: { url: faker.internet.url() } }; + await userController.updateSocialMediaForUser(uuidParams, socialMediaParams, member); + + const expectedUserSocialMedia3 = { + url: socialMediaParams.socialMedia.url, + type: member.userSocialMedia[index].type, + uuid: uuidParams.uuid, + }; + const updatedMember = await conn.manager.findOne( + UserModel, { uuid: member.uuid }, { relations: ['userSocialMedia'] }, + ); + + expect(updatedMember.userSocialMedia).toHaveLength(5); + const targetUserSocialMedia = updatedMember.userSocialMedia.find( + (socialMedia) => socialMedia.uuid === expectedUserSocialMedia3.uuid, + ); + expect(targetUserSocialMedia.url).toEqual(expectedUserSocialMedia3.url); + expect(targetUserSocialMedia.type).toEqual(expectedUserSocialMedia3.type); + }); + + test.concurrent('concurrent updates properly persist on successful submission 4', async () => { + await waitForFlag(); + const index = 4; + const uuidParams = { uuid: member.userSocialMedia[index].uuid }; + const socialMediaParams = { socialMedia: { url: faker.internet.url() } }; + await userController.updateSocialMediaForUser(uuidParams, socialMediaParams, member); + + const expectedUserSocialMedia4 = { + url: socialMediaParams.socialMedia.url, + type: member.userSocialMedia[index].type, + uuid: uuidParams.uuid, + }; + const updatedMember = await conn.manager.findOne( + UserModel, { uuid: member.uuid }, { relations: ['userSocialMedia'] }, + ); + + expect(updatedMember.userSocialMedia).toHaveLength(5); + const targetUserSocialMedia = updatedMember.userSocialMedia.find( + (socialMedia) => socialMedia.uuid === expectedUserSocialMedia4.uuid, + ); + expect(targetUserSocialMedia.url).toEqual(expectedUserSocialMedia4.url); + expect(targetUserSocialMedia.type).toEqual(expectedUserSocialMedia4.type); + }); +}); diff --git a/yarn.lock b/yarn.lock index f53205a52..c84255721 100644 --- a/yarn.lock +++ b/yarn.lock @@ -641,6 +641,13 @@ resolved "https://registry.yarnpkg.com/@tootallnate/once/-/once-1.1.2.tgz#ccb91445360179a04e7fe6aff78c00ffc1eeaf82" integrity sha512-RbzJvlNzmRq5c3O09UipeuXno4tA1FE6ikOjxZK0tuxVv3412l64l5t1W5pj4+rJq9vpkm/kwiR07aZXnsKPxw== +"@types/async-retry@^1.4.8": + version "1.4.8" + resolved "https://registry.yarnpkg.com/@types/async-retry/-/async-retry-1.4.8.tgz#eb32df13aceb9ba1a8a80e7fe518ff4e3fe46bb3" + integrity sha512-Qup/B5PWLe86yI5I3av6ePGaeQrIHNKCwbsQotD6aHQ6YkHsMUxVZkZsmx/Ry3VZQ6uysHwTjQ7666+k6UjVJA== + dependencies: + "@types/retry" "*" + "@types/babel__core@^7.0.0", "@types/babel__core@^7.1.14": version "7.1.18" resolved "https://registry.yarnpkg.com/@types/babel__core/-/babel__core-7.1.18.tgz#1a29abcc411a9c05e2094c98f9a1b7da6cdf49f8" @@ -823,6 +830,11 @@ resolved "https://registry.yarnpkg.com/@types/range-parser/-/range-parser-1.2.4.tgz#cd667bcfdd025213aafb7ca5915a932590acdcdc" integrity sha512-EEhsLsD6UsDM1yFhAvy0Cjr6VwmpMWqFBCb9w07wVugF7w9nfajxLuVmngTIpgS6svCnm6Vaw+MZhoDCKnOfsw== +"@types/retry@*": + version "0.12.5" + resolved "https://registry.yarnpkg.com/@types/retry/-/retry-0.12.5.tgz#f090ff4bd8d2e5b940ff270ab39fd5ca1834a07e" + integrity sha512-3xSjTp3v03X/lSQLkczaN9UIEwJMoMCA1+Nb5HfbJEQWogdeQIyVtTvxPXDQjZ5zws8rFQfVfRdz03ARihPJgw== + "@types/rfdc@^1.1.0": version "1.2.0" resolved "https://registry.yarnpkg.com/@types/rfdc/-/rfdc-1.2.0.tgz#ff0aedd2d33589442f3a1285d37bdbb54d18dfbc" @@ -1169,6 +1181,13 @@ astral-regex@^2.0.0: resolved "https://registry.yarnpkg.com/astral-regex/-/astral-regex-2.0.0.tgz#483143c567aeed4785759c0865786dc77d7d2e31" integrity sha512-Z7tMw1ytTXt5jqMcOP+OQteU1VuNK9Y02uuJtKQ1Sv69jXQKKg5cibLwGJow8yzZP+eAc18EmLGPal0bp36rvQ== +async-retry@^1.3.3: + version "1.3.3" + resolved "https://registry.yarnpkg.com/async-retry/-/async-retry-1.3.3.tgz#0e7f36c04d8478e7a58bdbed80cedf977785f280" + integrity sha512-wfr/jstw9xNi/0teMHrRW7dsz3Lt5ARhYNZ2ewpadnhaIp5mbALhOAP+EAdsC7t4Z6wqsDVv9+W6gm1Dk9mEyw== + dependencies: + retry "0.13.1" + async@0.9.x: version "0.9.2" resolved "https://registry.yarnpkg.com/async/-/async-0.9.2.tgz#aea74d5e61c1f899613bf64bda66d4c78f2fd17d" @@ -4918,6 +4937,11 @@ responselike@^1.0.2: dependencies: lowercase-keys "^1.0.0" +retry@0.13.1: + version "0.13.1" + resolved "https://registry.yarnpkg.com/retry/-/retry-0.13.1.tgz#185b1587acf67919d63b357349e03537b2484658" + integrity sha512-XQBQ3I8W1Cge0Seh+6gjj03LbmRFWuoszgK9ooCpwYIrhhoO80pfq4cUkU5DkknwfOfFteRwlZ56PYOGYyFWdg== + reusify@^1.0.4: version "1.0.4" resolved "https://registry.yarnpkg.com/reusify/-/reusify-1.0.4.tgz#90da382b1e126efc02146e90845a88db12925d76"