From 710dad9816993676319324c1fa77ffe4e8e25efb Mon Sep 17 00:00:00 2001 From: Wh1isper <9573586@qq.com> Date: Fri, 29 Dec 2023 12:06:23 +0800 Subject: [PATCH 1/6] Intro hash and hash_algorithm for saving --- .eslintrc.js | 1 + packages/docregistry/src/context.ts | 85 +++++++++++++++++++------ packages/services/src/contents/index.ts | 21 +++++- 3 files changed, 87 insertions(+), 20 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index 90b9f4a48699..c9827bbab47a 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -111,6 +111,7 @@ module.exports = { 'extension_data', 'extension_name', 'file_extension', + 'hash_algorithm', 'help_links', 'hist_access_type', 'ids_only', diff --git a/packages/docregistry/src/context.ts b/packages/docregistry/src/context.ts index 6e85286fa944..9067e6e5d122 100644 --- a/packages/docregistry/src/context.ts +++ b/packages/docregistry/src/context.ts @@ -476,11 +476,19 @@ export class Context< created: model.created, last_modified: model.last_modified, mimetype: model.mimetype, - format: model.format + format: model.format, + hash: model.hash, + hash_algorithm: model.hash_algorithm }; const mod = this._contentsModel?.last_modified ?? null; + const hash = this._contentsModel?.hash ?? null; this._contentsModel = newModel; - if (!mod || newModel.last_modified !== mod) { + if ( + !mod || + newModel.last_modified !== mod || + !hash || + newModel.hash !== hash + ) { this._fileChanged.emit(newModel); } } @@ -622,6 +630,7 @@ export class Context< const opts: Contents.IFetchOptions = { type: this._factory.contentType, content: this._factory.fileFormat !== null, + hash: this._factory.fileFormat !== null, ...(this._factory.fileFormat !== null ? { format: this._factory.fileFormat } : {}) @@ -681,12 +690,27 @@ export class Context< ): Promise { const path = this._path; // Make sure the file has not changed on disk. - const promise = this._manager.contents.get(path, { content: false }); + const promise = this._manager.contents.get(path, { + content: false, + hash: true + }); return promise.then( model => { if (this.isDisposed) { return Promise.reject(new Error('Disposed')); } + // Since jupyter server may provide hash in model, we compare hash first + const hashAvailable = + this.contentsModel?.hash !== undefined && + this.contentsModel?.hash !== null; + const hClient = this.contentsModel?.hash; + const hDisk = model.hash; + if (hashAvailable && hClient !== hDisk) { + console.warn(`Different hash found for ${this.path}`); + return this._raiseConflict(model, options); + } + + // When hash is not provided, we compare last_modified // We want to check last_modified (disk) > last_modified (client) // (our last save) // In some cases the filesystem reports an inconsistent time, so we allow buffer when comparing. @@ -695,16 +719,47 @@ export class Context< const tClient = modified ? new Date(modified) : new Date(); const tDisk = new Date(model.last_modified); if ( + !hashAvailable && modified && tDisk.getTime() - tClient.getTime() > lastModifiedCheckMargin ) { - return this._timeConflict(tClient, model, options); + console.warn( + `Last saving performed ${tClient} ` + + `while the current file seems to have been saved ` + + `${tDisk}` + ); + return this._raiseConflict(model, options); } - return this._manager.contents.save(path, options); + + return this._manager.contents + .save(path, options) + .then(async contentsModel => { + const model = await this._manager.contents.get(path, { + content: false, + hash: true + }); + return { + ...contentsModel, + hash: model.hash, + hash_algorithm: model.hash_algorithm + } as Contents.IModel; + }); }, err => { if (err.response && err.response.status === 404) { - return this._manager.contents.save(path, options); + return this._manager.contents + .save(path, options) + .then(async contentsModel => { + const model = await this._manager.contents.get(path, { + content: false, + hash: true + }); + return { + ...contentsModel, + hash: model.hash, + hash_algorithm: model.hash_algorithm + } as Contents.IModel; + }); } throw err; } @@ -746,22 +801,14 @@ export class Context< } }); } - /** * Handle a time conflict. */ - private _timeConflict( - tClient: Date, + private _raiseConflict( model: Contents.IModel, options: Partial ): Promise { - const tDisk = new Date(model.last_modified); - console.warn( - `Last saving performed ${tClient} ` + - `while the current file seems to have been saved ` + - `${tDisk}` - ); - if (this._timeConflictModalIsOpen) { + if (this._ConflictModalIsOpen) { const error = new Error('Modal is already displayed'); error.name = 'ModalDuplicateError'; return Promise.reject(error); @@ -780,13 +827,13 @@ or load the version on disk (revert)?`, label: this._trans.__('Overwrite'), actions: ['overwrite'] }); - this._timeConflictModalIsOpen = true; + this._ConflictModalIsOpen = true; return showDialog({ title: this._trans.__('File Changed'), body, buttons: [Dialog.cancelButton(), revertBtn, overwriteBtn] }).then(result => { - this._timeConflictModalIsOpen = false; + this._ConflictModalIsOpen = false; if (this.isDisposed) { return Promise.reject(new Error('Disposed')); } @@ -917,7 +964,7 @@ or load the version on disk (revert)?`, private _disposed = new Signal(this); private _dialogs: ISessionContext.IDialogs; private _lastModifiedCheckMargin = 500; - private _timeConflictModalIsOpen = false; + private _ConflictModalIsOpen = false; } /** diff --git a/packages/services/src/contents/index.ts b/packages/services/src/contents/index.ts index 4e68ff5fb94a..1ef17cd1056d 100644 --- a/packages/services/src/contents/index.ts +++ b/packages/services/src/contents/index.ts @@ -112,6 +112,17 @@ export namespace Contents { * The indices of the matched characters in the name. */ indices?: ReadonlyArray | null; + + /** + * The hexdigest hash string of content, if requested (otherwise null). + * It cannot be null if hash_algorithm is defined. + */ + readonly hash?: string; + + /** + * The algorithm used to compute hash value. It cannot be null if hash is defined. + */ + readonly hash_algorithm?: string; } /** @@ -153,6 +164,13 @@ export namespace Contents { * The default is `true`. */ content?: boolean; + + /** + * Whether to include a hash in the response. + * + * The default is `false`. + */ + hash?: boolean; } /** @@ -1122,7 +1140,8 @@ export class Drive implements Contents.IDrive { delete options['format']; } const content = options.content ? '1' : '0'; - const params: PartialJSONObject = { ...options, content }; + const hash = options.hash ? '1' : '0'; + const params: PartialJSONObject = { ...options, content, hash }; url += URLExt.objectToQueryString(params); } From b187bddfbb9b6ff7652ab2fd0ecd5855a9194d7c Mon Sep 17 00:00:00 2001 From: Zhongsheng Ji <9573586@qq.com> Date: Tue, 23 Jan 2024 09:16:50 +0800 Subject: [PATCH 2/6] Rename _ConflictModalIsOpen to _conflictModalIsOpen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com> --- packages/docregistry/src/context.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/docregistry/src/context.ts b/packages/docregistry/src/context.ts index 9067e6e5d122..bd54137438eb 100644 --- a/packages/docregistry/src/context.ts +++ b/packages/docregistry/src/context.ts @@ -808,7 +808,7 @@ export class Context< model: Contents.IModel, options: Partial ): Promise { - if (this._ConflictModalIsOpen) { + if (this._conflictModalIsOpen) { const error = new Error('Modal is already displayed'); error.name = 'ModalDuplicateError'; return Promise.reject(error); @@ -827,13 +827,13 @@ or load the version on disk (revert)?`, label: this._trans.__('Overwrite'), actions: ['overwrite'] }); - this._ConflictModalIsOpen = true; + this._conflictModalIsOpen = true; return showDialog({ title: this._trans.__('File Changed'), body, buttons: [Dialog.cancelButton(), revertBtn, overwriteBtn] }).then(result => { - this._ConflictModalIsOpen = false; + this._conflictModalIsOpen = false; if (this.isDisposed) { return Promise.reject(new Error('Disposed')); } @@ -964,7 +964,7 @@ or load the version on disk (revert)?`, private _disposed = new Signal(this); private _dialogs: ISessionContext.IDialogs; private _lastModifiedCheckMargin = 500; - private _ConflictModalIsOpen = false; + private _conflictModalIsOpen = false; } /** From 0b167481e550626af3aa84372e8997990209ccae Mon Sep 17 00:00:00 2001 From: Wh1isper <9573586@qq.com> Date: Tue, 23 Jan 2024 09:25:27 +0800 Subject: [PATCH 3/6] Code review: Compare hash if available --- packages/docregistry/src/context.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/docregistry/src/context.ts b/packages/docregistry/src/context.ts index bd54137438eb..18ff8699b054 100644 --- a/packages/docregistry/src/context.ts +++ b/packages/docregistry/src/context.ts @@ -486,8 +486,8 @@ export class Context< if ( !mod || newModel.last_modified !== mod || - !hash || - newModel.hash !== hash + // Compare hash if available + (hash && newModel.hash !== hash) ) { this._fileChanged.emit(newModel); } From 600843dc1f3b14ba7912a666ee75bed0d3706abf Mon Sep 17 00:00:00 2001 From: Wh1isper <9573586@qq.com> Date: Tue, 23 Jan 2024 09:47:50 +0800 Subject: [PATCH 4/6] Add hash testing via #fileChanged --- packages/docregistry/test/context.spec.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/docregistry/test/context.spec.ts b/packages/docregistry/test/context.spec.ts index 9fca85c6e422..366a287bdc54 100644 --- a/packages/docregistry/test/context.spec.ts +++ b/packages/docregistry/test/context.spec.ts @@ -85,10 +85,15 @@ describe('docregistry/context', () => { describe('#fileChanged', () => { it('should be emitted when the file is saved', async () => { const path = context.path; + const hash = context.contentsModel?.hash ?? null; let called = false; context.fileChanged.connect((sender, args) => { expect(sender).toBe(context); expect(args.path).toBe(path); + if (hash) { + expect(args.hash).not.toBeNull(); + expect(args.hash).not.toBe(hash); + } called = true; }); await context.initialize(true); From a757d2216bfa7de81ca598f9ddb41d2698e15c4e Mon Sep 17 00:00:00 2001 From: Wh1isper <9573586@qq.com> Date: Fri, 26 Jan 2024 10:20:00 +0800 Subject: [PATCH 5/6] Improve fileChanged and add hash to test mock --- packages/docregistry/src/context.ts | 3 ++- packages/docregistry/test/context.spec.ts | 12 ++++++++++++ packages/services/src/testutils.ts | 4 +++- 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/packages/docregistry/src/context.ts b/packages/docregistry/src/context.ts index 18ff8699b054..8ad70bd056b9 100644 --- a/packages/docregistry/src/context.ts +++ b/packages/docregistry/src/context.ts @@ -485,7 +485,8 @@ export class Context< this._contentsModel = newModel; if ( !mod || - newModel.last_modified !== mod || + // Compare last_modified if no hash + (!hash && newModel.last_modified !== mod) || // Compare hash if available (hash && newModel.hash !== hash) ) { diff --git a/packages/docregistry/test/context.spec.ts b/packages/docregistry/test/context.spec.ts index 366a287bdc54..dc54cfd347ed 100644 --- a/packages/docregistry/test/context.spec.ts +++ b/packages/docregistry/test/context.spec.ts @@ -100,6 +100,18 @@ describe('docregistry/context', () => { expect(called).toBe(true); }); + it('should not emitted when the file hash is not changed', async () => { + let called = false; + + await context.initialize(true); + context.fileChanged.connect(() => { + called = true; + }); + + await context.save(); + expect(called).toBe(false); + }); + it('should not contain the file content attribute', async () => { let called = false; context.fileChanged.connect((sender, args) => { diff --git a/packages/services/src/testutils.ts b/packages/services/src/testutils.ts index ccd582062589..4d7e3638f960 100644 --- a/packages/services/src/testutils.ts +++ b/packages/services/src/testutils.ts @@ -453,7 +453,9 @@ export const ContentsManagerMock = jest.fn(() => { format: 'text', mimetype: 'plain/text', ...options, - last_modified: timeStamp + last_modified: timeStamp, + hash: '123456', + hash_algorithm: 'static' }); } fileChangedSignal.emit({ From dca1ec376c66038b8df7001d32cf058c70fcd717 Mon Sep 17 00:00:00 2001 From: krassowski <5832902+krassowski@users.noreply.github.com> Date: Sat, 27 Jan 2024 01:05:21 +0000 Subject: [PATCH 6/6] Add tests, slight logic adjustments --- packages/docregistry/src/context.ts | 7 +- packages/docregistry/test/context.spec.ts | 85 +++++++++++++++++++++-- packages/services/src/testutils.ts | 22 +++++- 3 files changed, 103 insertions(+), 11 deletions(-) diff --git a/packages/docregistry/src/context.ts b/packages/docregistry/src/context.ts index 8ad70bd056b9..fe55d07bba87 100644 --- a/packages/docregistry/src/context.ts +++ b/packages/docregistry/src/context.ts @@ -484,7 +484,8 @@ export class Context< const hash = this._contentsModel?.hash ?? null; this._contentsModel = newModel; if ( - !mod || + // If neither modification date nor hash available, assume the file has changed + (!mod && !hash) || // Compare last_modified if no hash (!hash && newModel.last_modified !== mod) || // Compare hash if available @@ -703,7 +704,9 @@ export class Context< // Since jupyter server may provide hash in model, we compare hash first const hashAvailable = this.contentsModel?.hash !== undefined && - this.contentsModel?.hash !== null; + this.contentsModel?.hash !== null && + model.hash !== undefined && + model.hash !== null; const hClient = this.contentsModel?.hash; const hDisk = model.hash; if (hashAvailable && hClient !== hDisk) { diff --git a/packages/docregistry/test/context.spec.ts b/packages/docregistry/test/context.spec.ts index dc54cfd347ed..d0bb2f298e11 100644 --- a/packages/docregistry/test/context.spec.ts +++ b/packages/docregistry/test/context.spec.ts @@ -85,25 +85,98 @@ describe('docregistry/context', () => { describe('#fileChanged', () => { it('should be emitted when the file is saved', async () => { const path = context.path; - const hash = context.contentsModel?.hash ?? null; let called = false; context.fileChanged.connect((sender, args) => { expect(sender).toBe(context); expect(args.path).toBe(path); - if (hash) { - expect(args.hash).not.toBeNull(); - expect(args.hash).not.toBe(hash); - } called = true; }); await context.initialize(true); expect(called).toBe(true); }); - it('should not emitted when the file hash is not changed', async () => { + it('should be emitted when the file hash changes', async () => { + let called = false; + context = new Context({ + manager, + factory, + // The path below is a magic string instructing the `get()` + // method of the contents manager mock to return a random hash. + path: 'random-hash.txt' + }); + + await context.initialize(true); + + context.fileChanged.connect((_sender, _args) => { + called = true; + }); + + const promise = context.save(); + + // Expect the correct dialog + await waitForDialog(); + const dialog = document.body.getElementsByClassName( + 'jp-Dialog' + )[0] as HTMLElement; + expect(dialog.innerHTML).toMatch( + 'has changed on disk since the last time it was opened or saved' + ); + + // Accept dialog (overwrite) + await acceptDialog(); + await promise; + + // Expect the signal to have been emitted + expect(called).toBe(true); + }); + + it('should be emitted when the file timestamp changes and there is no hash', async () => { + let called = false; + context = new Context({ + manager, + factory, + // The path below is a magic string instructing the `get()` + // method of the contents manager mock to return a newer timestamp and no hash. + path: 'newer-timestamp-no-hash.txt' + }); + + await context.initialize(true); + + context.fileChanged.connect((_sender, _args) => { + called = true; + }); + + const promise = context.save(); + + // Expect the correct dialog + await waitForDialog(); + const dialog = document.body.getElementsByClassName( + 'jp-Dialog' + )[0] as HTMLElement; + expect(dialog.innerHTML).toMatch( + 'has changed on disk since the last time it was opened or saved' + ); + + // Accept dialog (overwrite) + await acceptDialog(); + await promise; + + // Expect the signal to have been emitted + expect(called).toBe(true); + }); + + it('should not be emitted when the file hash is not changed', async () => { let called = false; + context = new Context({ + manager, + factory, + // The path below is a magic string instructing the `save()` + // method of the contents manager mock to not update time nor hash. + path: 'frozen-time-and-hash.txt' + }); await context.initialize(true); + context.fileChanged.connect(() => { called = true; }); diff --git a/packages/services/src/testutils.ts b/packages/services/src/testutils.ts index 4d7e3638f960..eaa7d9aafcf5 100644 --- a/packages/services/src/testutils.ts +++ b/packages/services/src/testutils.ts @@ -373,6 +373,15 @@ export const ContentsManagerMock = jest.fn(() => { return Private.makeResponseError(404); } const model = files.get(path)!; + const overrides: { hash?: string; last_modified?: string } = {}; + if (path == 'random-hash.txt') { + overrides.hash = Math.random().toString(); + } else if (path == 'newer-timestamp-no-hash.txt') { + overrides.hash = undefined; + const tomorrow = new Date(); + tomorrow.setDate(new Date().getDate() + 1); + overrides.last_modified = tomorrow.toISOString(); + } if (model.type === 'directory') { if (options?.content !== false) { const content: Contents.IModel[] = []; @@ -393,7 +402,7 @@ export const ContentsManagerMock = jest.fn(() => { if (options?.content != false) { return Promise.resolve(model); } - return Promise.resolve({ ...model, content: '' }); + return Promise.resolve({ ...model, content: '', ...overrides }); }), driveName: jest.fn(path => { return dummy.driveName(path); @@ -437,10 +446,17 @@ export const ContentsManagerMock = jest.fn(() => { path = Private.fixSlash(path); const timeStamp = new Date().toISOString(); if (files.has(path)) { + const updates = + path == 'frozen-time-and-hash.txt' + ? {} + : { + last_modified: timeStamp, + hash: timeStamp + }; files.set(path, { ...files.get(path)!, ...options, - last_modified: timeStamp + ...updates }); } else { files.set(path, { @@ -454,7 +470,7 @@ export const ContentsManagerMock = jest.fn(() => { mimetype: 'plain/text', ...options, last_modified: timeStamp, - hash: '123456', + hash: timeStamp, hash_algorithm: 'static' }); }