diff --git a/app/gen-server/entity/User.ts b/app/gen-server/entity/User.ts index c93837cb..b4a99090 100644 --- a/app/gen-server/entity/User.ts +++ b/app/gen-server/entity/User.ts @@ -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) diff --git a/app/gen-server/lib/homedb/HomeDBManager.ts b/app/gen-server/lib/homedb/HomeDBManager.ts index b510c21b..111f370d 100644 --- a/app/gen-server/lib/homedb/HomeDBManager.ts +++ b/app/gen-server/lib/homedb/HomeDBManager.ts @@ -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): Promise { - 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 { - return this._usersManager.getExistingUserByLogin(email, manager); + return await this._usersManager.getExistingUserByLogin(email, manager); } /** diff --git a/app/gen-server/lib/homedb/UsersManager.ts b/app/gen-server/lib/homedb/UsersManager.ts index a219fa23..295e4de1 100644 --- a/app/gen-server/lib/homedb/UsersManager.ts +++ b/app/gen-server/lib/homedb/UsersManager.ts @@ -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) { - 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); + }); } /** diff --git a/app/server/lib/dbUtils.ts b/app/server/lib/dbUtils.ts index 754d774e..c540954a 100644 --- a/app/server/lib/dbUtils.ts +++ b/app/server/lib/dbUtils.ts @@ -55,43 +55,33 @@ export function getConnectionName() { */ const connectionMutex = new Mutex(); -async function buildConnection(overrideConf?: Partial) { - 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 { - 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): Promise { - 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;"); } } diff --git a/test/gen-server/lib/homedb/UsersManager.ts b/test/gen-server/lib/homedb/UsersManager.ts index 736f1710..d2306ead 100644 --- a/test/gen-server/lib/homedb/UsersManager.ts +++ b/test/gen-server/lib/homedb/UsersManager.ts @@ -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(); - /** - * 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(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) { - 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(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(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); + } +} diff --git a/test/gen-server/seed.ts b/test/gen-server/seed.ts index 0e230660..c3e7073f 100644 --- a/test/gen-server/seed.ts +++ b/test/gen-server/seed.ts @@ -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; - 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