(core) Fix for linking issue.

Summary:
If linking state changes multiple times frequently the code that simulates async operation is
wrongly debounced, which causes inverted order of execution. This fix makes sure that only the last
call to filter function is used.

Test Plan: Adding new test

Reviewers: alexmojaki

Reviewed By: alexmojaki

Subscribers: alexmojaki

Differential Revision: https://phab.getgrist.com/D4139

app/client/models/QuerySet.ts

@@ -117,28 +117,16 @@ export class QuerySetManager extends Disposable {
  * RowSource.
  */
 export class DynamicQuerySet extends RowSource {
-  /**
-   * Replace the query represented by this DynamicQuerySet. If multiple makeQuery() calls are made
-   * quickly (while waiting for the server), cb() may only be called for the latest one.
-   *
-   * If there is an error fetching data, cb(err) will be called with that error. The second
-   * argument to cb() is true if any data was changed, and false if not. Note that for a series of
-   * makeQuery() calls, cb() is always called at least once, and always asynchronously.
-   *
-   * It's possible for this to be called very quickly in succession,
-   * e.g. when selecting another row of a linked summary table grouped by multiple columns,
-   * as an observable gets triggered for each one.
-   * We only want to keep the last call, and _updateQuerySetDebounced may not be called
-   * in the correct order, so we first debounce here.
-   */
-  public readonly makeQuery = debounce(tbind(this._makeQuery, this), 0);
-
   // Holds a reference to the currently active QuerySet.
   private _holder = Holder.create<IRefCountSub<QuerySet>>(this);
 
   // Shortcut to _holder.get().get().
   private _querySet?: QuerySet;
 
+  // A ticket number for the latest makeQuery() call. We use it to avoid calling cb() for
+  // superseded queries.
+  private _lastTicket = 0;
+
   // We could switch between several different queries quickly. If several queries are done
   // fetching at the same time (e.g. were already ready), debounce lets us only update the
   // query-set once to the last query.
@@ -163,19 +151,26 @@ export class DynamicQuerySet extends RowSource {
     return this._querySet ? this._querySet.isTruncated : false;
   }
 
-  private _makeQuery(filters: { [colId: string]: any[] },
+  /**
-                     operations: {[colId: string]: QueryOperation},
+   * Replace the query represented by this DynamicQuerySet. If multiple makeQuery() calls are made
-                     cb: (err: Error|null, changed: boolean) => void): void {
+   * quickly (while waiting for the server), cb() may only be called for the latest one.
-    if (this.isDisposed()) {
+   *
-      cb(new Error('Disposed'), false);
+   * If there is an error fetching data, cb(err) will be called with that error. The second
-      return;
+   * argument to cb() is true if any data was changed, and false if not. Note that for a series of
-    }
+   * makeQuery() calls, cb() is always called at least once, and always asynchronously.
+   */
+  public makeQuery(filters: {[colId: string]: any[]},
+                   operations: {[colId: string]: QueryOperation},
+                   cb: (err: Error|null, changed: boolean) => void): void {
     const query: ClientQuery = {tableId: this._tableModel.tableData.tableId, filters, operations};
     const newQuerySet = this._querySetManager.useQuerySet(this._holder, query);
+    const ticket = this._getTicket();
 
     // CB should be called asynchronously, since surprising hard-to-debug interactions can happen
     // if it's sometimes synchronous and sometimes not.
     newQuerySet.fetchPromise.then(() => {
+      // Only if we weren't superseded by another query.
+      if (!ticket.isValid()) { return; }
       this._updateQuerySetDebounced(newQuerySet, cb);
     })
     .catch((err) => { cb(err, false); });
@@ -201,6 +196,13 @@ export class DynamicQuerySet extends RowSource {
       cb(err, true);
     }
   }
+
+  private _getTicket() {
+    const myTicket = ++this._lastTicket;
+    return {
+      isValid: () => this._lastTicket === myTicket
+    };
+  }
 }
 
 /**

test/nbrowser/LinkingErrors.ts

@@ -1,7 +1,7 @@
 /**
  * Test various error situations with linking page widgets.
  */
-import {assert, driver, stackWrapFunc} from 'mocha-webdriver';
+import {assert, driver} from 'mocha-webdriver';
 import * as gu from 'test/nbrowser/gristUtils';
 import {setupTestSuite} from 'test/nbrowser/testUtils';
 
@@ -19,9 +19,79 @@ describe("LinkingErrors", function() {
 
   afterEach(() => gu.checkForErrors());
 
-  it("should link correctly in the normal case", async function() {
+  before(async function() {
     session = await gu.session().teamSite.login();
     api = session.createHomeApi();
+  });
+
+  it("should maintain links after reload", async function() {
+    await session.tempNewDoc(cleanup);
+
+    // Recreate the bug.
+    await gu.sendActions([
+      ['AddEmptyTable', 'Table2'], // NOTICE: The order here matters, the bug was all about ordering.
+      ['AddEmptyTable', 'Table0'],
+      ['ModifyColumn', 'Table1', 'B', {type: 'Ref:Table0'}],
+      ['ModifyColumn', 'Table2', 'A', {type: 'Ref:Table1'}],
+
+      ['AddRecord', 'Table0', null, {A: 'A'}],
+      ['AddRecord', 'Table0', null, {A: 'B'}],
+      ['AddRecord', 'Table1', null, {A: 'a', B: 1}],
+      ['AddRecord', 'Table1', null, {A: 'b', B: 1}],
+      ['AddRecord', 'Table1', null, {A: 'c', B: 1}],
+      ['AddRecord', 'Table1', null, {A: 'd', B: 1}],
+      ['AddRecord', 'Table2', null, {A: 2, B: 1}],
+      ['AddRecord', 'Table2', null, {A: 2, B: 2}],
+      ['AddRecord', 'Table2', null, {A: 4, B: 3}],
+      ['AddRecord', 'Table2', null, {A: 4, B: 4}],
+      ['AddRecord', 'Table2', null, {A: 1, B: 5}],
+      ['AddRecord', 'Table2', null, {A: 1, B: 6}],
+      ['AddRecord', 'Table2', null, {A: 3, B: 7}],
+      ['AddRecord', 'Table2', null, {A: 3, B: 8}],
+    ]);
+
+    // Now add those two tables to a page, and link them.
+    // Pay attention to order.
+    await gu.addNewPage('Table', 'Table1');
+    await gu.getCell('B', 1).click();
+    await gu.openColumnPanel();
+    await gu.setRefShowColumn('A');
+
+    await gu.addNewSection('Table', 'Table2', {selectBy: 'TABLE1'});
+    await gu.getCell('A', 1).click();
+    await gu.openColumnPanel();
+    await gu.setRefShowColumn('A');
+
+    await gu.addNewSection('Table', 'Table0');
+    await gu.selectSectionByTitle('TABLE1');
+    await gu.openWidgetPanel('data');
+    await gu.selectBy('TABLE0');
+
+    // Place cursor on the first row of Table0
+    await gu.getCell({section: 'Table1', rowNum: 1, col: 'A'}).click();
+
+    const checkLink = async () => {
+      // This should show the linked rows in Table1
+      assert.deepEqual(await gu.getVisibleGridCells({section: 'Table1', col: 'A', rowNums: [1, 2, 3, 4]}),
+        ['a', 'b', 'c', 'd']);
+
+      // This should show the linked rows in Table2
+      assert.deepEqual(await gu.getVisibleGridCells({section: 'Table2', col: 'B', rowNums: [1, 2]}),
+        ['5', '6']);
+    };
+
+    await checkLink();
+
+    // After reloading, we should see the same thing.
+    await gu.reloadDoc();
+    await gu.waitToPass(async () => {
+      // Linking is done asynchronously, so make sure it is loaded first.
+      assert.equal(await gu.selectedBy(), 'TABLE0');
+      await checkLink();
+    });
+  });
+
+  it("should link correctly in the normal case", async function() {
     docId = await session.tempNewDoc(cleanup, 'LinkingErrors1', {load: false});
 
     // Make a table with some data.
@@ -46,20 +116,9 @@ describe("LinkingErrors", function() {
     await checkLinking();
   });
 
-  // Check that normal linking works.
-  const checkLinking = stackWrapFunc(async function() {
-    await gu.getCell({section: 'PLANETS', rowNum: 1, col: 'PlanetName'}).click();
-    assert.deepEqual(await gu.getVisibleGridCells({section: 'MOONS', col: 'MoonName', rowNums: [1, 2]}),
-      ['Moon', '']);
-    await gu.getCell({section: 'PLANETS', rowNum: 2, col: 'PlanetName'}).click();
-    assert.deepEqual(await gu.getVisibleGridCells({section: 'MOONS', col: 'MoonName', rowNums: [1, 2, 3]}),
-      ['Phobos', 'Deimos', '']);
-  });
-
   it("should unset linking setting when changing the data table for a widget", async function() {
     await gu.getCell({section: 'Moons', rowNum: 1, col: 'MoonName'}).click();
-    await gu.toggleSidePanel('right', 'open');
+    await gu.openWidgetPanel();
-    await driver.find('.test-right-tab-pagewidget').click();
     await driver.findContent('.test-right-panel button', /Change Widget/).click();
 
     assert.equal(await driver.find('.test-wselect-table-label[class*=-selected]').getText(), 'Moons');
@@ -173,6 +232,7 @@ describe("LinkingErrors", function() {
     await revert();
     await gu.checkForErrors();
   });
+
 });
 
 async function getTableData(docApi: DocAPI, tableId: string): Promise<TableData> {
@@ -180,3 +240,13 @@ async function getTableData(docApi: DocAPI, tableId: string): Promise<TableData>
   const tableAction = toTableDataAction(tableId, colValues);
   return new TableData(tableId, tableAction, (schema as any)[tableId]);
 }
+
+// Check that normal linking works.
+async function checkLinking() {
+  await gu.getCell({section: 'PLANETS', rowNum: 1, col: 'PlanetName'}).click();
+  assert.deepEqual(await gu.getVisibleGridCells({section: 'MOONS', col: 'MoonName', rowNums: [1, 2]}),
+    ['Moon', '']);
+  await gu.getCell({section: 'PLANETS', rowNum: 2, col: 'PlanetName'}).click();
+  assert.deepEqual(await gu.getVisibleGridCells({section: 'MOONS', col: 'MoonName', rowNums: [1, 2, 3]}),
+    ['Phobos', 'Deimos', '']);
+}

test/nbrowser/gristUtils.ts

@@ -2705,6 +2705,16 @@ export async function selectBy(table: string|RegExp) {
   await waitForServer();
 }
 
+/**
+ * Returns "Select by" of the current section.
+ */
+export async function selectedBy() {
+  await toggleSidePanel('right', 'open');
+  await driver.find('.test-right-tab-pagewidget').click();
+  await driver.find('.test-config-data').click();
+  return await driver.find('.test-right-select-by').getText();
+}
+
 // Add column to sort.
 export async function addColumnToSort(colName: RegExp|string) {
   await driver.find(".test-sort-config-add").click();