Fix spurious "File Changed" dialogs using hash from jupyter-server v2.11.1+

.eslintrc.js

@@ -111,6 +111,7 @@ module.exports = {
           'extension_data',
           'extension_name',
           'file_extension',
+          'hash_algorithm',
           'help_links',
           'hist_access_type',
           'ids_only',

packages/docregistry/src/context.ts

@@ -476,11 +476,21 @@ 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 (
+      // 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
+      (hash && newModel.hash !== hash)
+    ) {
       this._fileChanged.emit(newModel);
     }
   }
@@ -622,6 +632,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 +692,29 @@ export class Context<
   ): Promise<Contents.IModel> {
     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 &&
+          model.hash !== undefined &&
+          model.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 +723,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 +805,14 @@ export class Context<
       }
     });
   }
-
   /**
    * Handle a time conflict.
    */
-  private _timeConflict(
-    tClient: Date,
+  private _raiseConflict(
     model: Contents.IModel,
     options: Partial<Contents.IModel>
   ): Promise<Contents.IModel> {
-    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 +831,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 +968,7 @@ or load the version on disk (revert)?`,
   private _disposed = new Signal<this, void>(this);
   private _dialogs: ISessionContext.IDialogs;
   private _lastModifiedCheckMargin = 500;
-  private _timeConflictModalIsOpen = false;
+  private _conflictModalIsOpen = false;
 }
 
 /**

packages/docregistry/test/context.spec.ts

@@ -95,6 +95,96 @@ describe('docregistry/context', () => {
         expect(called).toBe(true);
       });
 
+      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;
+        });
+
+        await context.save();
+        expect(called).toBe(false);
+      });
+
       it('should not contain the file content attribute', async () => {
         let called = false;
         context.fileChanged.connect((sender, args) => {

packages/services/src/contents/index.ts

@@ -112,6 +112,17 @@ export namespace Contents {
      * The indices of the matched characters in the name.
      */
     indices?: ReadonlyArray<number> | 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);
     }
 

packages/services/src/testutils.ts

@@ -373,6 +373,15 @@ export const ContentsManagerMock = jest.fn<Contents.IManager, []>(() => {
         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<Contents.IManager, []>(() => {
       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<Contents.IManager, []>(() => {
       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, {
@@ -453,7 +469,9 @@ export const ContentsManagerMock = jest.fn<Contents.IManager, []>(() => {
           format: 'text',
           mimetype: 'plain/text',
           ...options,
-          last_modified: timeStamp
+          last_modified: timeStamp,
+          hash: timeStamp,
+          hash_algorithm: 'static'
         });
       }
       fileChangedSignal.emit({