From 0b37e991329dc89c94adb74453cdecfded4c31a5 Mon Sep 17 00:00:00 2001 From: Laurent Cozic Date: Thu, 4 Feb 2021 16:46:01 +0000 Subject: [PATCH] clean up --- packages/lib/file-api-driver-joplinServer.ts | 3 - packages/server/src/models/BaseModel.ts | 14 ++-- packages/server/src/models/FileModel.ts | 68 +++++++++++++------ packages/server/src/routes/api/shares.test.ts | 19 ++++++ packages/server/src/routes/index/shares.ts | 1 - .../server/src/utils/testing/testUtils.ts | 27 ++++++++ 6 files changed, 103 insertions(+), 29 deletions(-) diff --git a/packages/lib/file-api-driver-joplinServer.ts b/packages/lib/file-api-driver-joplinServer.ts index 1e3f607f4..aa45cce7c 100644 --- a/packages/lib/file-api-driver-joplinServer.ts +++ b/packages/lib/file-api-driver-joplinServer.ts @@ -42,9 +42,6 @@ export default class FileApiDriverJoplinServer { isDeleted: isDeleted, }; - // TODO - HANDLE DELETED - // if (md['.tag'] === 'deleted') output.isDeleted = true; - return output; } diff --git a/packages/server/src/models/BaseModel.ts b/packages/server/src/models/BaseModel.ts index c73f99c01..307bf2b25 100644 --- a/packages/server/src/models/BaseModel.ts +++ b/packages/server/src/models/BaseModel.ts @@ -15,6 +15,10 @@ export interface SaveOptions { trackChanges?: boolean; } +export interface LoadOptions { + fields?: string[]; +} + export interface DeleteOptions { validationRules?: any; } @@ -132,7 +136,7 @@ export default abstract class BaseModel { if (debugTransaction) console.info('START', name, txIndex); - let output:T = null; + let output: T = null; try { output = await fn(); @@ -252,15 +256,15 @@ export default abstract class BaseModel { return toSave; } - public async loadByIds(ids: string[]): Promise { + public async loadByIds(ids: string[], options: LoadOptions = {}): Promise { if (!ids.length) return []; - return this.db(this.tableName).select(this.defaultFields).whereIn('id', ids); + return this.db(this.tableName).select(options.fields || this.defaultFields).whereIn('id', ids); } - public async load(id: string): Promise { + public async load(id: string, options: LoadOptions = {}): Promise { if (!id) throw new Error('id cannot be empty'); - return this.db(this.tableName).select(this.defaultFields).where({ id: id }).first(); + return this.db(this.tableName).select(options.fields || this.defaultFields).where({ id: id }).first(); } public async delete(id: string | string[]): Promise { diff --git a/packages/server/src/models/FileModel.ts b/packages/server/src/models/FileModel.ts index d58fe3367..0e30aa8be 100644 --- a/packages/server/src/models/FileModel.ts +++ b/packages/server/src/models/FileModel.ts @@ -396,12 +396,31 @@ export default class FileModel extends BaseModel { return file; } + // private async processFileLink(file: File): Promise { + // if (!('source_file_id' in file)) throw new Error('Cannot process a file without a source_file_id'); + // if (!file.source_file_id) return file; + // const content = await this.loadContent(file.source_file_id, { + // skipPermissionCheck: true, + // // Current we don't follow links more than one level deep. In + // // practice it means that if a user tries to share a note that has + // // been shared with them, it will not work. Instead they'll have to + // // make a copy of that note and share that. Anything else would + // // probably be too complex to make any sense in terms of UI. + // skipFollowLinks: true, + // }); + // return { ...file, content }; + // } + // If the file is a link to another file, the content of the source if - // assigned to the content of the destination. - private async processFileLink(file: File): Promise { - if (!('source_file_id' in file)) throw new Error('Cannot process a file without a source_file_id'); - if (!file.source_file_id) return file; - const content = await this.loadContent(file.source_file_id, { + // assigned to the content of the destination. The updated_time property is + // also set to the most recent one among the source and the dest. + private async processFileLinks(files: File[]): Promise { + const sourceFileIds = files.filter(f => !!f.source_file_id).map(f => f.source_file_id); + if (!sourceFileIds.length) return files; + + const fields = Object.keys(files[0]); + const sourceFiles = await this.loadByIds(sourceFileIds, { + fields, skipPermissionCheck: true, // Current we don't follow links more than one level deep. In // practice it means that if a user tries to share a note that has @@ -410,33 +429,42 @@ export default class FileModel extends BaseModel { // probably be too complex to make any sense in terms of UI. skipFollowLinks: true, }); - return { ...file, content }; - } - private async loadContent(id: string, options: LoadOptions = {}): Promise { - const file: File = await this.loadWithContent(id, { ...options, fields: ['id', 'content'] }); - return file.content; + const modFiles = files.slice(); + for (let i = 0; i < modFiles.length; i++) { + const file = modFiles[i]; + if (!file.source_file_id) continue; + const sourceFile = sourceFiles.find(f => f.id === file.source_file_id); + if (!sourceFile) { + throw new Error(`File is linked to a file that no longer exists: ${file.id} => ${file.source_file_id}`); + } + + const modFile = { ...file }; + + if ('updated_time' in modFile) modFile.updated_time = Math.max(sourceFile.updated_time, file.updated_time); + if ('content' in modFile) modFile.content = sourceFile.content; + + modFiles[i] = modFile; + } + + return modFiles; } public async loadWithContent(id: string, options: LoadOptions = {}): Promise { - const file: File = await this.db(this.tableName).select(options.fields || '*').where({ id: id }).first(); - if (!file) return null; - if (!options.skipPermissionCheck) await this.checkCanReadPermissions(file); - return options.skipFollowLinks ? file : this.processFileLink(file); + const fields = options.fields || this.defaultFields.concat(['content']); + return this.load(id, { ...options, fields }); } public async loadByIds(ids: string[], options: LoadOptions = {}): Promise { - const files: File[] = await super.loadByIds(ids); + const files: File[] = await super.loadByIds(ids, options); if (!files.length) return []; if (!options.skipPermissionCheck) await this.checkCanReadPermissions(files); - return files; + return options.skipFollowLinks ? files : this.processFileLinks(files); } public async load(id: string, options: LoadOptions = {}): Promise { - const file: File = await super.load(id); - if (!file) return null; - if (!options.skipPermissionCheck) await this.checkCanReadPermissions(file); - return file; + const files = await this.loadByIds([id], options); + return files.length ? files[0] : null; } public async save(object: File, options: SaveOptions = {}): Promise { diff --git a/packages/server/src/routes/api/shares.test.ts b/packages/server/src/routes/api/shares.test.ts index de50b7306..7381be6ab 100644 --- a/packages/server/src/routes/api/shares.test.ts +++ b/packages/server/src/routes/api/shares.test.ts @@ -5,6 +5,7 @@ import { postApiC, postApi, getApiC, patchApi, getApi } from '../../utils/testin import { PaginatedFiles } from '../../models/FileModel'; import { PaginatedChanges } from '../../models/ChangeModel'; import { shareWithUserAndAccept } from '../../utils/testing/shareApiUtils'; +import { msleep } from '../../utils/time'; describe('api_shares', function() { @@ -87,6 +88,22 @@ describe('api_shares', function() { expect(fileContent.toString()).toBe('testing share'); }); + test('should get updated time of shared file', async function() { + // If sharer changes the file, sharee should see the updated_time of the sharer file. + const { user: user1, session: session1 } = await createUserAndSession(1); + const { user: user2, session: session2 } = await createUserAndSession(2); + + let { sharerFile, shareeFile } = await shareWithUserAndAccept(session1.id, user1, session2.id, user2); + + await msleep(1); + + await updateFile(user1.id, sharerFile.id, 'content modified'); + + sharerFile = await models().file({ userId: user1.id }).load(sharerFile.id); + shareeFile = await models().file({ userId: user2.id }).load(shareeFile.id); + expect(shareeFile.updated_time).toBe(sharerFile.updated_time); + }); + test('should see delta changes for linked files', async function() { const { user: user1, session: session1 } = await createUserAndSession(1); const { user: user2, session: session2 } = await createUserAndSession(2); @@ -112,6 +129,8 @@ describe('api_shares', function() { cursor2 = page2.cursor; } + await msleep(1); + await updateFile(user1.id, sharerFile.id, 'from sharer'); { diff --git a/packages/server/src/routes/index/shares.ts b/packages/server/src/routes/index/shares.ts index ef8da7242..e7d429e58 100644 --- a/packages/server/src/routes/index/shares.ts +++ b/packages/server/src/routes/index/shares.ts @@ -33,7 +33,6 @@ router.get('shares/:id', async (path: SubPath, ctx: AppContext) => { const file = await fileModel.loadWithContent(share.file_id, { skipPermissionCheck: true }); if (!file) throw new ErrorNotFound(); - const result = await renderFile(ctx, file, share); ctx.response.body = result.body; diff --git a/packages/server/src/utils/testing/testUtils.ts b/packages/server/src/utils/testing/testUtils.ts index 9f2082293..ab85ae034 100644 --- a/packages/server/src/utils/testing/testUtils.ts +++ b/packages/server/src/utils/testing/testUtils.ts @@ -79,6 +79,33 @@ function initGlobalLogger() { Logger.initializeGlobalLogger(globalLogger); } +export function msleep(ms: number) { + // It seems setTimeout can sometimes last less time than the provided + // interval: + // + // https://stackoverflow.com/a/50912029/561309 + // + // This can cause issues in tests where we expect the actual duration to be + // the same as the provided interval or more, but not less. So the code + // below check that the elapsed time is no less than the provided interval, + // and if it is, it waits a bit longer. + const startTime = Date.now(); + return new Promise((resolve) => { + setTimeout(() => { + if (Date.now() - startTime < ms) { + const iid = setInterval(() => { + if (Date.now() - startTime >= ms) { + clearInterval(iid); + resolve(null); + } + }, 2); + } else { + resolve(null); + } + }, ms); + }); +} + export async function koaAppContext(options: AppContextTestOptions = null): Promise { if (!db_) throw new Error('Database must be initialized first');