Archives/gristlabs_grist-core
1
0
Fork 0
mirror of https://github.com/gristlabs/grist-core.git synced 2025-06-13 20:53:59 +00:00
Code Issues Projects Releases Wiki Activity

(core) Fixing lock issues and reverting back to single connection.

Browse Source 
Summary:
Removing `createNewConnection` method that was used in tests to create a
"scoped" version of `HomeDbManager`. Currently this won't work as there are
many methods (like `Users.findOne`) that are using the default (global) connection.

Additionally `HomeDBManger` had couple of bugs that were causing locks, which
manifested themselves in postgresql tests (that are not serializing transactions).
Repository methods like `Users.findOne` or `user.save()`, even when wrapped in
transaction were using a separate connection from the pool (and a separate
transaction).

Some tests in `UsersManager` are still skipped or refactored, as sinon's `fakeTimers`
doesn't work well with postgresql driver (which is using `setTimout` a lot).

Date mappings in `User` entity were fixed, they were using `SQLite` configuration only,
which caused problems with postgresql database.

Test Plan: Refactored.

Reviewers: paulfitz

Reviewed By: paulfitz

Subscribers: paulfitz

Differential Revision: https://phab.getgrist.com/D4342
This commit is contained in:
Jarosław Sadziński 2024-09-11 19:42:28 +02:00
parent 51acc9a8cc
commit 0ca70e9d43
6 changed files with 130 additions and 168 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
app/gen-server/entity/User.ts
View File

@ -26,10 +26,10 @@ export class User extends BaseEntity {
@Column({name: 'picture', type: String, nullable: true})
public picture: string | null;
@Column({name: 'first_login_at', type: Date, nullable: true})
@Column({name: 'first_login_at', type: nativeValues.dateTimeType, nullable: true})
public firstLoginAt: Date | null;
@Column({name: 'last_connection_at', type: Date, nullable: true})
@Column({name: 'last_connection_at', type: nativeValues.dateTimeType, nullable: true})
public lastConnectionAt: Date | null;
@OneToOne(type => Organization, organization => organization.owner)

11
app/gen-server/lib/homedb/HomeDBManager.ts
View File

@ -56,7 +56,7 @@ import {
readJson
} from 'app/gen-server/sqlUtils';
import {appSettings} from 'app/server/lib/AppSettings';
import {createNewConnection, getOrCreateConnection} from 'app/server/lib/dbUtils';
import {getOrCreateConnection} from 'app/server/lib/dbUtils';
import {makeId} from 'app/server/lib/idUtils';
import log from 'app/server/lib/log';
import {Permit} from 'app/server/lib/Permit';
@ -70,7 +70,6 @@ import {
Brackets,
Connection,
DatabaseType,
DataSourceOptions,
EntityManager,
ObjectLiteral,
SelectQueryBuilder,
@ -354,8 +353,8 @@ export class HomeDBManager extends EventEmitter {
this._connection = await getOrCreateConnection();
}
public async createNewConnection(overrideConf?: Partial<DataSourceOptions>): Promise<void> {
this._connection = await createNewConnection(overrideConf);
public connectTo(connection: Connection) {
this._connection = connection;
}
// make sure special users and workspaces are available
@ -458,7 +457,7 @@ export class HomeDBManager extends EventEmitter {
* @see UsersManager.prototype.ensureExternalUser
*/
public async ensureExternalUser(profile: UserProfile) {
return this._usersManager.ensureExternalUser(profile);
return await this._usersManager.ensureExternalUser(profile);
}
public async updateUser(userId: number, props: UserProfileChange) {
@ -491,7 +490,7 @@ export class HomeDBManager extends EventEmitter {
* Find a user by email. Don't create the user if it doesn't already exist.
*/
public async getExistingUserByLogin(email: string, manager?: EntityManager): Promise<User|undefined> {
return this._usersManager.getExistingUserByLogin(email, manager);
return await this._usersManager.getExistingUserByLogin(email, manager);
}
/**

16
app/gen-server/lib/homedb/UsersManager.ts
View File

@ -223,7 +223,7 @@ export class UsersManager {
});
// No need to survey this user.
newUser.isFirstTimeUser = false;
await newUser.save();
await manager.save(newUser);
} else {
// Else update profile and login information from external profile.
let updated = false;
@ -277,7 +277,7 @@ export class UsersManager {
if (!props.isFirstTimeUser) { isWelcomed = true; }
}
if (needsSave) {
await user.save();
await manager.save(user);
}
});
return { user, isWelcomed };
@ -285,12 +285,12 @@ export class UsersManager {
// TODO: rather use the updateUser() method, if that makes sense?
public async updateUserOptions(userId: number, props: Partial<UserOptions>) {
const user = await User.findOne({where: {id: userId}});
if (!user) { throw new ApiError("unable to find user", 400); }
const newOptions = {...(user.options ?? {}), ...props};
user.options = newOptions;
await user.save();
await this._runInTransaction(undefined, async manager => {
const user = await manager.findOne(User, {where: {id: userId}});
if (!user) { throw new ApiError("unable to find user", 400); }
user.options = {...(user.options ?? {}), ...props};
await manager.save(user);
});
}
/**

42
app/server/lib/dbUtils.ts
View File

@ -55,43 +55,33 @@ export function getConnectionName() {
*/
const connectionMutex = new Mutex();
async function buildConnection(overrideConf?: Partial<DataSourceOptions>) {
const settings = getTypeORMSettings(overrideConf);
const connection = await createConnection(settings);
// When using Sqlite, set a busy timeout of 3s to tolerate a little
// interference from connections made by tests. Logging doesn't show
// any particularly slow queries, but bad luck is possible.
// This doesn't affect when Postgres is in use. It also doesn't have
// any impact when there is a single connection to the db, as is the
// case when Grist is run as a single process.
if (connection.driver.options.type === 'sqlite') {
await connection.query('PRAGMA busy_timeout = 3000');
}
return connection;
}
export async function getOrCreateConnection(): Promise<Connection> {
return connectionMutex.runExclusive(async () => {
return connectionMutex.runExclusive(async() => {
try {
// If multiple servers are started within the same process, we
// share the database connection. This saves locking trouble
// with Sqlite.
return getConnection(getConnectionName());
const connection = getConnection();
return connection;
} catch (e) {
if (!String(e).match(/ConnectionNotFoundError/)) {
throw e;
}
return buildConnection();
const connection = await createConnection(getTypeORMSettings());
// When using Sqlite, set a busy timeout of 3s to tolerate a little
// interference from connections made by tests. Logging doesn't show
// any particularly slow queries, but bad luck is possible.
// This doesn't affect when Postgres is in use. It also doesn't have
// any impact when there is a single connection to the db, as is the
// case when Grist is run as a single process.
if (connection.driver.options.type === 'sqlite') {
await connection.query('PRAGMA busy_timeout = 3000');
}
return connection;
}
});
}
export async function createNewConnection(overrideConf?: Partial<DataSourceOptions>): Promise<Connection> {
return connectionMutex.runExclusive(async () => {
return buildConnection(overrideConf);
});
}
export async function runMigrations(connection: Connection) {
// on SQLite, migrations fail if we don't temporarily disable foreign key
// constraint checking. This is because for sqlite typeorm copies each
@ -100,9 +90,7 @@ export async function runMigrations(connection: Connection) {
// transaction, or it has no effect.
const sqlite = connection.driver.options.type === 'sqlite';
if (sqlite) { await connection.query("PRAGMA foreign_keys = OFF;"); }
await connection.transaction(async tr => {
await tr.connection.runMigrations();
});
await connection.runMigrations({ transaction: "all" });
if (sqlite) { await connection.query("PRAGMA foreign_keys = ON;"); }
}

202
test/gen-server/lib/homedb/UsersManager.ts
View File

@ -1,4 +1,3 @@
import { isAffirmative } from 'app/common/gutil';
import { FullUser, UserProfile } from 'app/common/LoginSessionAPI';
import { ANONYMOUS_USER_EMAIL, EVERYONE_EMAIL, PREVIEWER_EMAIL, UserOptions } from 'app/common/UserAPI';
import { AclRuleOrg } from 'app/gen-server/entity/AclRule';
@ -13,9 +12,8 @@ import { HomeDBManager } from 'app/gen-server/lib/homedb/HomeDBManager';
import { GetUserOptions, NonGuestGroup, Resource } from 'app/gen-server/lib/homedb/Interfaces';
import { SUPPORT_EMAIL, UsersManager } from 'app/gen-server/lib/homedb/UsersManager';
import { updateDb } from 'app/server/lib/dbUtils';
import { prepareDatabase } from 'test/server/lib/helpers/PrepareDatabase';
import { EnvironmentSnapshot } from 'test/server/testUtils';
import { getDatabase } from 'test/testUtils';
import { createInitialDb, removeConnection, setUpDB } from 'test/gen-server/seed';
import log from 'app/server/lib/log';
import { assert } from 'chai';
@ -23,29 +21,10 @@ import Sinon, { SinonSandbox, SinonSpy } from 'sinon';
import { EntityManager } from 'typeorm';
import winston from 'winston';
import fs from 'fs/promises';
import { tmpdir } from 'os';
import path from 'path';
import { dereferenceConnection } from 'test/gen-server/seed';
const username = process.env.USER || "nobody";
const tmpDirPrefix = path.join(tmpdir(), `grist_test_${username}_userendpoint_`);
// it is sometimes useful in debugging to turn off automatic cleanup of sqlite databases.
const noCleanup = isAffirmative(process.env.NO_CLEANUP);
import {delay} from 'app/common/delay';
describe('UsersManager', function () {
this.timeout('30s');
let tmpDir: string;
before(async function () {
tmpDir = await fs.mkdtemp(tmpDirPrefix);
});
after(async function () {
if (!noCleanup) {
await fs.rm(tmpDir, {recursive: true});
}
});
this.timeout('3m');
describe('static method', function () {
/**
@ -226,15 +205,6 @@ describe('UsersManager', function () {
let sandbox: SinonSandbox;
const uniqueLocalPart = new Set<string>();
/**
* Works around lacks of type narrowing after asserting the value is defined.
* This is fixed in latest versions of @types/chai
*
* FIXME: once upgrading @types/chai to 4.3.17 or higher, remove this function which would not be usefull anymore
*/
function assertExists<T>(value?: T, message?: string): asserts value is T {
assert.exists(value, message);
}
function ensureUnique(localPart: string) {
if (uniqueLocalPart.has(localPart)) {
@ -261,26 +231,6 @@ describe('UsersManager', function () {
return db.getOrg({userId: user.id}, user.personalOrg.id);
}
async function withDataBase(dbName: string, cb: (db: HomeDBManager) => Promise<void>) {
const database = path.join(tmpDir, dbName + '.db');
const localDb = new HomeDBManager();
await localDb.createNewConnection({ name: dbName, database });
await updateDb(localDb.connection);
try {
await cb(localDb);
} finally {
await localDb.connection.destroy();
// HACK: This is a weird case, we have established a different connection
// but the existing connection is also impacted.
// A found workaround consist in destroying and creating again the connection.
//
// TODO: Check whether using DataSource would help and avoid this hack.
await db.connection.destroy();
await db.createNewConnection();
dereferenceConnection(dbName);
}
}
function disableLoggingLevel<T extends keyof winston.LoggerInstance>(method: T) {
return sandbox.stub(log, method);
}
@ -299,18 +249,19 @@ describe('UsersManager', function () {
};
}
before(async function () {
if (process.env.TYPEORM_TYPE === "postgres") {
this.skip();
}
env = new EnvironmentSnapshot();
await prepareDatabase(tmpDir);
db = await getDatabase();
process.env.TEST_CLEAN_DATABASE = 'true';
setUpDB(this);
db = new HomeDBManager();
await createInitialDb();
await db.connect();
await db.initializeSpecialIds();
});
after(async function () {
env?.restore();
await removeConnection();
});
beforeEach(function () {
@ -341,17 +292,6 @@ describe('UsersManager', function () {
it("getSupportUserId() should retrieve 'support' user id", function () {
assert.strictEqual(db.getSupportUserId(), SUPPORT_USER_ID);
});
describe('Without special id initialization', function () {
it('should throw an error', async function () {
await withDataBase('without-special-ids', async function (localDb) {
assert.throws(() => localDb.getAnonymousUserId(), "'Anonymous' user not available");
assert.throws(() => localDb.getPreviewerUserId(), "'Previewer' user not available");
assert.throws(() => localDb.getEveryoneUserId(), "'Everyone' user not available");
assert.throws(() => localDb.getSupportUserId(), "'Support' user not available");
});
});
});
});
describe('getUserByKey()', function () {
@ -523,11 +463,13 @@ describe('UsersManager', function () {
describe('ensureExternalUser()', function () {
let managerSaveSpy: SinonSpy;
let userSaveSpy: SinonSpy;
beforeEach(function () {
managerSaveSpy = sandbox.spy(EntityManager.prototype, 'save');
userSaveSpy = sandbox.spy(User.prototype, 'save');
});
afterEach(function () {
managerSaveSpy.restore();
});
async function checkUserInfo(profile: UserProfile) {
@ -552,14 +494,12 @@ describe('UsersManager', function () {
email: 'chimpy@getgrist.com',
});
assert.isFalse(userSaveSpy.called, 'user.save() should not have been called');
assert.isFalse(managerSaveSpy.called, 'manager.save() should not have been called');
});
it('should save an unknown user', async function () {
const profile = makeProfile('ensureExternalUser-saves-an-unknown-user');
await db.ensureExternalUser(profile);
assert.isTrue(userSaveSpy.called, 'user.save() should have been called');
assert.isTrue(managerSaveSpy.called, 'manager.save() should have been called');
await checkUserInfo(profile);
@ -720,18 +660,18 @@ describe('UsersManager', function () {
it('should create a user when none exist with the corresponding email', async function () {
const localPart = ensureUnique('getuserbylogin-creates-user-when-not-already-exists');
const email = makeEmail(localPart);
sandbox.useFakeTimers(42_000);
assert.notExists(await db.getExistingUserByLogin(email));
const before = Date.now();
// We are storing time without miliseconds, so make sure at least 1 second has passed
await delay(2000);
const user = await db.getUserByLogin(makeEmail(localPart.toUpperCase()));
const after = Date.now();
assert.isTrue(user.isFirstTimeUser, 'should be marked as first time user');
assert.equal(user.loginEmail, email);
assert.equal(user.logins[0].displayEmail, makeEmail(localPart.toUpperCase()));
assert.equal(user.name, '');
// FIXME: why is user.lastConnectionAt actually a string and not a Date?
// FIXME: is it consistent that user.lastConnectionAt updated here and not firstLoginAt?
assert.equal(String(user.lastConnectionAt), '1970-01-01 00:00:42.000');
asssertBetween(before, user.lastConnectionAt?.getTime(), after);
});
it('should create a personnal organization for the new user', async function () {
@ -758,7 +698,7 @@ describe('UsersManager', function () {
assert.deepEqual(userFirstCall, userSecondCall);
});
// FIXME: why using Sinon.useFakeTimers() makes user.lastConnectionAt a string instead of a Date
// FIXME: postgresql doesn't like fake timers.
it.skip('should update lastConnectionAt only for different days', async function () {
const fakeTimer = sandbox.useFakeTimers(0);
const localPart = ensureUnique('getuserbylogin-updates-last_connection_at-for-different-days');
@ -776,15 +716,18 @@ describe('UsersManager', function () {
});
describe('when passing information to update (using `profile`)', function () {
it('should populate the firstTimeLogin and deduce the name from the email', async function () {
sandbox.useFakeTimers(42_000);
// FIXME: postgresql doesn't like fake timers.
it.skip('should populate the firstTimeLogin and deduce the name from the email', async function () {
const timers = sandbox.useFakeTimers(42_000);
const localPart = ensureUnique('getuserbylogin-with-profile-populates-first_time_login-and-name');
const user = await db.getUserByLogin(makeEmail(localPart), {
profile: {name: '', email: makeEmail(localPart)}
});
assert.equal(user.name, localPart);
// FIXME: why using Sinon.useFakeTimers() makes user.firstLoginAt a string instead of a Date
assert.equal(String(user.firstLoginAt), '1970-01-01 00:00:42.000');
assert.equal(user.firstLoginAt?.getTime(), 42_000);
await timers.runAllAsync();
timers.restore();
});
it('should populate user with any passed information', async function () {
@ -954,30 +897,6 @@ describe('UsersManager', function () {
});
});
describe('initializeSpecialIds()', function () {
it('should initialize special ids', async function () {
return withDataBase('test-special-ids', async (localDb) => {
const specialAccounts = [
{name: "Support", email: SUPPORT_EMAIL},
{name: "Anonymous", email: ANONYMOUS_USER_EMAIL},
{name: "Preview", email: PREVIEWER_EMAIL},
{name: "Everyone", email: EVERYONE_EMAIL}
];
for (const {email} of specialAccounts) {
assert.notExists(await localDb.getExistingUserByLogin(email));
}
await localDb.initializeSpecialIds();
for (const {name, email} of specialAccounts) {
const res = await localDb.getExistingUserByLogin(email);
assertExists(res);
assert.equal(res.name, name);
}
});
});
});
describe('completeProfiles()', function () {
it('should return an empty array if no profiles are provided', async function () {
const res = await db.completeProfiles([]);
@ -1030,4 +949,71 @@ describe('UsersManager', function () {
});
});
});
describe('class method without db setup', function () {
let db: HomeDBManager;
let env: EnvironmentSnapshot;
describe('initializeSpecialIds()', function () {
before(async function () {
env = new EnvironmentSnapshot();
process.env.TEST_CLEAN_DATABASE = 'true';
setUpDB(this);
db = new HomeDBManager();
await db.connect();
await createInitialDb(db.connection, false);
await updateDb(db.connection);
});
after(async function () {
await removeConnection();
env?.restore();
});
it('should initialize special ids', async function () {
const specialAccounts = [
{name: "Support", email: SUPPORT_EMAIL},
{name: "Anonymous", email: ANONYMOUS_USER_EMAIL},
{name: "Preview", email: PREVIEWER_EMAIL},
{name: "Everyone", email: EVERYONE_EMAIL}
];
for (const {email} of specialAccounts) {
assert.notExists(await db.getExistingUserByLogin(email));
}
assert.throws(() => db.getAnonymousUserId(), "'Anonymous' user not available");
assert.throws(() => db.getPreviewerUserId(), "'Previewer' user not available");
assert.throws(() => db.getEveryoneUserId(), "'Everyone' user not available");
assert.throws(() => db.getSupportUserId(), "'Support' user not available");
await db.initializeSpecialIds();
for (const {name, email} of specialAccounts) {
const res = await db.getExistingUserByLogin(email);
assertExists(res);
assert.equal(res.name, name);
}
});
});
});
});
/**
* Works around lacks of type narrowing after asserting the value is defined.
* This is fixed in latest versions of @types/chai
*
* FIXME: once upgrading @types/chai to 4.3.17 or higher, remove this function which would not be usefull anymore
*/
function assertExists<T>(value?: T, message?: string): asserts value is T {
assert.exists(value, message);
}
function asssertBetween(min: number, value: number|null|undefined, max: number, message?: string) {
assert.isNotNull(value, message);
assert.isDefined(value, message);
if (value !== null && value !== undefined) {
assert.isAtLeast(value, min, message);
assert.isAtMost(value, max, message);
}
}

23
test/gen-server/seed.ts
View File

@ -43,7 +43,7 @@ import {Workspace} from "app/gen-server/entity/Workspace";
import {EXAMPLE_WORKSPACE_NAME} from 'app/gen-server/lib/homedb/HomeDBManager';
import {Permissions} from 'app/gen-server/lib/Permissions';
import {
getConnectionName, getOrCreateConnection, runMigrations, undoLastMigration, updateDb
getOrCreateConnection, runMigrations, undoLastMigration, updateDb
} from 'app/server/lib/dbUtils';
import {FlexServer} from 'app/server/lib/FlexServer';
import * as fse from 'fs-extra';
@ -529,27 +529,16 @@ class Seed {
// When running mocha on several test files at once, we need to reset our database connection
// if it exists. This is a little ugly since it is stored globally.
export async function removeConnection() {
const connections = getConnectionManager().connections;
if (connections.length > 0) {
if (connections.length > 1) {
if (getConnectionManager().connections.length > 0) {
if (getConnectionManager().connections.length > 1) {
throw new Error("unexpected number of connections");
}
await connections[0].destroy();
dereferenceConnection(getConnectionName());
await getConnectionManager().connections[0].close();
// There is still no official way to delete connections that I've found.
(getConnectionManager() as any).connectionMap = new Map();
}
}
export function dereferenceConnection(name: string) {
// There seem to be no official way to delete connections.
// Also we should probably get rid of the use of connectionManager, which is deprecated
const connectionMgr = getConnectionManager();
const connectionMap = (connectionMgr as any).connectionMap as Map<string, Connection>;
if (!connectionMap.has(name)) {
throw new Error('connection with this name not found: ' + name);
}
connectionMap.delete(name);
}
export async function createInitialDb(connection?: Connection, migrateAndSeedData: boolean = true) {
// In jenkins tests, we may want to reset the database to a clean
// state. If so, TEST_CLEAN_DATABASE will have been set. How to