Server: Added tests for logout and fixed transaction deadlock

packages/server/src/models/BaseModel.ts

@@ -102,16 +102,57 @@ export default abstract class BaseModel {
 		return false;
 	}
 
-	protected async withTransaction(fn: Function): Promise<void> {
+	// When using withTransaction, make sure any database call uses an instance
+	// of `this.db()` that was accessed within the `fn` callback, otherwise the
+	// transaction will be stuck!
+	//
+	// This for example, would result in a stuck transaction:
+	//
+	// const query = this.db(this.tableName).where('id', '=', id);
+	//
+	// this.withTransaction(async () => {
+	//     await query.delete();
+	// });
+	//
+	// This is because withTransaction is going to swap the value of "this.db()"
+	// for as long as the transaction is active. So if the query is started
+	// outside the transaction, it will use the regular db connection and wait
+	// for the newly created transaction to finish, which will never happen.
+	//
+	// This is a bit of a leaky abstraction, which ideally should be improved
+	// but for now one just has to be aware of the caveat.
+	//
+	// The `name` argument is only for debugging, so that any stuck transaction
+	// can be more easily identified.
+	protected async withTransaction(fn: Function, name: string = null): Promise<void> {
+		const debugTransaction = false;
+
+		const debugTimerId = debugTransaction ? setTimeout(() => {
+			console.info('Transaction did not complete:', name, txIndex);
+		}, 5000) : null;
+
 		const txIndex = await this.transactionHandler_.start();
 
+		if (debugTransaction) console.info('START', name, txIndex);
+
 		try {
 			await fn();
 		} catch (error) {
 			await this.transactionHandler_.rollback(txIndex);
+
+			if (debugTransaction) {
+				console.info('ROLLBACK', name, txIndex);
+				clearTimeout(debugTimerId);
+			}
+
 			throw error;
 		}
 
+		if (debugTransaction) {
+			console.info('COMMIT', name, txIndex);
+			clearTimeout(debugTimerId);
+		}
+
 		await this.transactionHandler_.commit(txIndex);
 	}
 
@@ -197,7 +238,7 @@ export default abstract class BaseModel {
 				// Sanity check:
 				if (updatedCount !== 1) throw new ErrorBadRequest(`one row should have been updated, but ${updatedCount} row(s) were updated`);
 			}
-		});
+		}, 'BaseModel::save');
 
 		return toSave;
 	}
@@ -220,11 +261,6 @@ export default abstract class BaseModel {
 
 		if (!ids.length) throw new Error('no id provided');
 
-		const query = this.db(this.tableName).where({ id: ids[0] });
-		for (let i = 1; i < ids.length; i++) {
-			await query.orWhere({ id: ids[i] });
-		}
-
 		const trackChanges = this.trackChanges;
 
 		let itemsWithParentIds: AnyItemType[] = null;
@@ -233,13 +269,18 @@ export default abstract class BaseModel {
 		}
 
 		await this.withTransaction(async () => {
+			const query = this.db(this.tableName).where({ id: ids[0] });
+			for (let i = 1; i < ids.length; i++) {
+				await query.orWhere({ id: ids[i] });
+			}
+
 			const deletedCount = await query.del();
 			if (deletedCount !== ids.length) throw new Error(`${ids.length} row(s) should have been deleted by ${deletedCount} row(s) were deleted`);
 
 			if (trackChanges) {
 				for (const item of itemsWithParentIds) await this.handleChangeTracking({}, item, ChangeType.Delete);
 			}
-		});
+		}, 'BaseModel::delete');
 	}
 
 }

packages/server/src/models/FileModel.ts

@@ -87,7 +87,7 @@ export default class FileModel extends BaseModel {
 
 				output[item.id] = segments.length ? (`root:/${segments.join('/')}:`) : 'root';
 			}
-		});
+		}, 'FileModel::itemFullPaths');
 
 		return output;
 	}
@@ -404,7 +404,7 @@ export default class FileModel extends BaseModel {
 			for (const childId of childrenIds) {
 				await this.delete(childId);
 			}
-		});
+		}, 'FileModel::deleteChildren');
 	}
 
 	public async delete(id: string, options: DeleteOptions = {}): Promise<void> {
@@ -427,7 +427,7 @@ export default class FileModel extends BaseModel {
 			}
 
 			await super.delete(id);
-		});
+		}, 'FileModel::delete');
 	}
 
 }

packages/server/src/models/SessionModel.ts

@@ -29,4 +29,9 @@ export default class SessionModel extends BaseModel {
 		return this.createUserSession(user.id);
 	}
 
+	public async logout(sessionId: string) {
+		if (!sessionId) return;
+		await this.delete(sessionId);
+	}
+
 }

packages/server/src/models/UserModel.ts

@@ -97,7 +97,7 @@ export default class UserModel extends BaseModel {
 			const rootFile = await fileModel.userRootFile();
 			await fileModel.delete(rootFile.id, { validationRules: { canDeleteRoot: true } });
 			await super.delete(id);
-		});
+		}, 'UserModel::delete');
 	}
 
 	public async save(object: User, options: SaveOptions = {}): Promise<User> {
@@ -114,7 +114,7 @@ export default class UserModel extends BaseModel {
 				const fileModel = this.models().file({ userId: newUser.id });
 				await fileModel.createRootFile();
 			}
-		});
+		}, 'UserModel::save');
 
 		return newUser;
 	}

packages/server/src/routes/index/logout.test.ts

@@ -0,0 +1,37 @@
+import routeHandler from '../../middleware/routeHandler';
+import { beforeAllDb, afterAllTests, beforeEachDb, koaAppContext, models, createUserAndSession } from '../../utils/testing/testUtils';
+
+describe('index_logout', function() {
+
+	beforeAll(async () => {
+		await beforeAllDb('index_logout');
+	});
+
+	afterAll(async () => {
+		await afterAllTests();
+	});
+
+	beforeEach(async () => {
+		await beforeEachDb();
+	});
+
+	test('should logout', async function() {
+		const { session } = await createUserAndSession();
+
+		const context = await koaAppContext({
+			sessionId: session.id,
+			request: {
+				method: 'POST',
+				url: '/logout',
+			},
+		});
+
+		expect(context.cookies.get('sessionId')).toBe(session.id);
+		expect(!!(await models().session().load(session.id))).toBe(true);
+		await routeHandler(context);
+
+		expect(!context.cookies.get('sessionId')).toBe(true);
+		expect(!!(await models().session().load(session.id))).toBe(false);
+	});
+
+});

packages/server/src/routes/index/logout.ts

@@ -2,13 +2,15 @@ import { SubPath, Route, redirect } from '../../utils/routeUtils';
 import { ErrorMethodNotAllowed } from '../../utils/errors';
 import { AppContext } from '../../utils/types';
 import { baseUrl } from '../../config';
+import { contextSessionId } from '../../utils/requestUtils';
 
 const route: Route = {
 
 	exec: async function(_path: SubPath, ctx: AppContext) {
 		if (ctx.method === 'POST') {
-			// TODO: also delete the session from the database
+			const sessionId = contextSessionId(ctx, false);
 			ctx.cookies.set('sessionId', '');
+			await ctx.models.session().logout(sessionId);
 			return redirect(ctx, `${baseUrl()}/login`);
 		}