(core) Clean up and refactor uses of HomeDBManager.getDoc

Summary:
Firstly I just wanted some more consistency and less repetition in places where Documents are retrieved from the DB, so it's more obvious when code differs from the norm. Main changes for that part:

- Let HomeDBManager accept a `Request` directly and convert it to a `Scope`, and use this in a few places.
- `getScope` tries `req.docAuth.docId` if `req.params` doesn't have a docId.

I also refactored how `_createActiveDoc` gets the document URL, separating out getting the document from getting a URL for it. This is because I want to use that document object in a future diff, but I also just find it cleaner. Notable changes for that:

- Extracted a new method `HomeDBManager.getRawDocById` as an alternative to `getDoc` that's explicitly for when you only have a document ID.
- Removed the interface method `GristServer.getDocUrl` and its two implementations because it wasn't used elsewhere and it didn't really add anything on top of getting a doc (now done by `getRawDocById`) and `getResourceUrl`.
- Between `cachedDoc` and `getRawDocById` (which represent previously existing code paths) also try `getDoc(getScope(docSession.req))`, which is new, because it seems better to only `getRawDocById` as a last resort.

Test Plan: Existing tests

Reviewers: georgegevoian

Reviewed By: georgegevoian

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

app/gen-server/ApiServer.ts

@@ -287,7 +287,7 @@ export class ApiServer {
     // GET /api/docs/:did
     // Get information about a document.
     this._app.get('/api/docs/:did', expressWrap(async (req, res) => {
-      const query = await this._dbManager.getDoc(getDocScope(req));
+      const query = await this._dbManager.getDoc(req);
       return sendOkReply(req, res, query);
     }));
 

app/gen-server/lib/HomeDBManager.ts

@@ -7,12 +7,23 @@ import {FullUser, UserProfile} from 'app/common/LoginSessionAPI';
 import {checkSubdomainValidity} from 'app/common/orgNameUtils';
 import {UserOrgPrefs} from 'app/common/Prefs';
 import * as roles from 'app/common/roles';
-// TODO: API should implement UserAPI
+import {StringUnion} from 'app/common/StringUnion';
-import {ANONYMOUS_USER_EMAIL, DocumentProperties, EVERYONE_EMAIL, getRealAccess,
+import {
-        ManagerDelta, NEW_DOCUMENT_CODE, OrganizationProperties,
+  ANONYMOUS_USER_EMAIL,
-        Organization as OrgInfo, PermissionData, PermissionDelta, SUPPORT_EMAIL, UserAccessData,
+  DocumentProperties,
+  EVERYONE_EMAIL,
+  getRealAccess,
+  ManagerDelta,
+  NEW_DOCUMENT_CODE,
+  OrganizationProperties,
+  Organization as OrgInfo,
+  PermissionData,
+  PermissionDelta,
+  SUPPORT_EMAIL,
+  UserAccessData,
   UserOptions,
-        WorkspaceProperties} from "app/common/UserAPI";
+  WorkspaceProperties
+} from "app/common/UserAPI";
 import {AclRule, AclRuleDoc, AclRuleOrg, AclRuleWs} from "app/gen-server/entity/AclRule";
 import {Alias} from "app/gen-server/entity/Alias";
 import {BillingAccount, ExternalBillingOptions} from "app/gen-server/entity/BillingAccount";
@@ -29,19 +40,34 @@ import {Workspace} from "app/gen-server/entity/Workspace";
 import {Permissions} from 'app/gen-server/lib/Permissions';
 import {scrubUserFromOrg} from "app/gen-server/lib/scrubUserFromOrg";
 import {applyPatch} from 'app/gen-server/lib/TypeORMPatches';
-import {bitOr, getRawAndEntities, hasAtLeastOneOfTheseIds, hasOnlyTheseIdsOrNull,
+import {
-        now, readJson} from 'app/gen-server/sqlUtils';
+  bitOr,
+  getRawAndEntities,
+  hasAtLeastOneOfTheseIds,
+  hasOnlyTheseIdsOrNull,
+  now,
+  readJson
+} from 'app/gen-server/sqlUtils';
 import {makeId} from 'app/server/lib/idUtils';
 import * as log from 'app/server/lib/log';
 import {Permit} from 'app/server/lib/Permit';
+import {getScope} from 'app/server/lib/requestUtils';
 import {WebHookSecret} from "app/server/lib/Triggers";
-import {StringUnion} from 'app/common/StringUnion';
 import {EventEmitter} from 'events';
+import {Request} from "express";
+import {
+  Brackets,
+  Connection,
+  createConnection,
+  DatabaseType,
+  EntityManager,
+  getConnection,
+  SelectQueryBuilder,
+  WhereExpression
+} from "typeorm";
+import * as uuidv4 from "uuid/v4";
 import flatten = require('lodash/flatten');
 import pick = require('lodash/pick');
-import {Brackets, Connection, createConnection, DatabaseType, EntityManager,
-        getConnection, SelectQueryBuilder, WhereExpression} from "typeorm";
-import * as uuidv4 from "uuid/v4";
 
 // Support transactions in Sqlite in async code.  This is a monkey patch, affecting
 // the prototypes of various TypeORM classes.
@@ -1038,7 +1064,8 @@ export class HomeDBManager extends EventEmitter {
 
   // Calls getDocImpl() and returns the Document from that, caching a fresh DocAuthResult along
   // the way. Note that we only cache the access level, not Document itself.
-  public async getDoc(scope: Scope): Promise<Document> {
+  public async getDoc(reqOrScope: Request | Scope): Promise<Document> {
+    const scope = "params" in reqOrScope ? getScope(reqOrScope) : reqOrScope;
     const key = getDocAuthKeyFromScope(scope);
     const promise = this.getDocImpl(key);
     await mapSetOrClear(this._docAuthCache, stringifyDocAuthKey(key), makeDocAuthResult(promise));
@@ -1052,6 +1079,10 @@ export class HomeDBManager extends EventEmitter {
     return doc;
   }
 
+  public async getRawDocById(docId: string) {
+    return await this.getDoc({urlId: docId, userId: this.getPreviewerUserId(), showAll: true});
+  }
+
   // Returns access info for the given doc and user, caching the results for DOC_AUTH_CACHE_TTL
   // ms. This helps reduce database load created by liberal authorization requests.
   public async getDocAuthCached(key: DocAuthKey): Promise<DocAuthResult> {

app/server/lib/DocApi.ts

@@ -1,55 +1,70 @@
-import { createEmptyActionSummary } from "app/common/ActionSummary";
+import {createEmptyActionSummary} from "app/common/ActionSummary";
-import { ApiError } from 'app/common/ApiError';
+import {ApiError} from 'app/common/ApiError';
-import { BrowserSettings } from "app/common/BrowserSettings";
+import {BrowserSettings} from "app/common/BrowserSettings";
-import {
+import {BulkColValues, ColValues, fromTableDataAction, TableColValues, TableRecordValue} from 'app/common/DocActions';
-  BulkColValues, ColValues, fromTableDataAction, TableColValues, TableRecordValue,
-} from 'app/common/DocActions';
 import {isRaisedException} from "app/common/gristTypes";
-import { isAffirmative } from "app/common/gutil";
+import {isAffirmative} from "app/common/gutil";
-import { SortFunc } from 'app/common/SortFunc';
+import {SortFunc} from 'app/common/SortFunc';
-import { DocReplacementOptions, DocState, DocStateComparison, DocStates, NEW_DOCUMENT_CODE} from 'app/common/UserAPI';
+import {Sort} from 'app/common/SortSpec';
+import {DocReplacementOptions, DocState, DocStateComparison, DocStates, NEW_DOCUMENT_CODE} from 'app/common/UserAPI';
+import {HomeDBManager, makeDocAuthResult} from 'app/gen-server/lib/HomeDBManager';
+import * as Types from "app/plugin/DocApiTypes";
+import DocApiTypesTI from "app/plugin/DocApiTypes-ti";
 import GristDataTI from 'app/plugin/GristData-ti';
-import { HomeDBManager, makeDocAuthResult } from 'app/gen-server/lib/HomeDBManager';
+import {OpOptions} from "app/plugin/TableOperations";
-import { OpOptions } from "app/plugin/TableOperations";
-import { handleSandboxErrorOnPlatform, TableOperationsImpl,
-         TableOperationsPlatform } from 'app/plugin/TableOperationsImpl';
-import { concatenateSummaries, summarizeAction } from "app/server/lib/ActionSummary";
-import { ActiveDoc, tableIdToRef } from "app/server/lib/ActiveDoc";
-import { assertAccess, getOrSetDocAuth, getTransitiveHeaders, getUserId, isAnonymousUser,
-         RequestWithLogin } from 'app/server/lib/Authorizer';
-import { DocManager } from "app/server/lib/DocManager";
-import { docSessionFromRequest, makeExceptionalDocSession, OptDocSession } from "app/server/lib/DocSession";
-import { DocWorker } from "app/server/lib/DocWorker";
-import { IDocWorkerMap } from "app/server/lib/DocWorkerMap";
-import { parseExportParameters } from "app/server/lib/Export";
-import { downloadCSV, DownloadCSVOptions } from "app/server/lib/ExportCSV";
-import { downloadXLSX, DownloadXLSXOptions } from "app/server/lib/ExportXLSX";
-import { expressWrap } from 'app/server/lib/expressWrap';
-import { filterDocumentInPlace } from "app/server/lib/filterUtils";
-import { googleAuthTokenMiddleware } from "app/server/lib/GoogleAuth";
-import { exportToDrive } from "app/server/lib/GoogleExport";
-import { GristServer } from 'app/server/lib/GristServer';
-import { HashUtil } from 'app/server/lib/HashUtil';
-import { makeForkIds } from "app/server/lib/idUtils";
 import {
-  getDocId, getDocScope, integerParam, isParameterOn, optStringParam,
+  handleSandboxErrorOnPlatform,
-  sendOkReply, sendReply, stringParam } from 'app/server/lib/requestUtils';
+  TableOperationsImpl,
+  TableOperationsPlatform
+} from 'app/plugin/TableOperationsImpl';
+import {concatenateSummaries, summarizeAction} from "app/server/lib/ActionSummary";
+import {ActiveDoc, tableIdToRef} from "app/server/lib/ActiveDoc";
+import {
+  assertAccess,
+  getOrSetDocAuth,
+  getTransitiveHeaders,
+  getUserId,
+  isAnonymousUser,
+  RequestWithLogin
+} from 'app/server/lib/Authorizer';
+import {DocManager} from "app/server/lib/DocManager";
+import {docSessionFromRequest, makeExceptionalDocSession, OptDocSession} from "app/server/lib/DocSession";
+import {DocWorker} from "app/server/lib/DocWorker";
+import {IDocWorkerMap} from "app/server/lib/DocWorkerMap";
+import {parseExportParameters} from "app/server/lib/Export";
+import {downloadCSV, DownloadCSVOptions} from "app/server/lib/ExportCSV";
+import {downloadXLSX, DownloadXLSXOptions} from "app/server/lib/ExportXLSX";
+import {expressWrap} from 'app/server/lib/expressWrap';
+import {filterDocumentInPlace} from "app/server/lib/filterUtils";
+import {googleAuthTokenMiddleware} from "app/server/lib/GoogleAuth";
+import {exportToDrive} from "app/server/lib/GoogleExport";
+import {GristServer} from 'app/server/lib/GristServer';
+import {HashUtil} from 'app/server/lib/HashUtil';
+import {makeForkIds} from "app/server/lib/idUtils";
+import {
+  getDocId,
+  getDocScope,
+  getScope,
+  integerParam,
+  isParameterOn,
+  optStringParam,
+  sendOkReply,
+  sendReply,
+  stringParam
+} from 'app/server/lib/requestUtils';
+import {ServerColumnGetters} from 'app/server/lib/ServerColumnGetters';
 import {localeFromRequest} from "app/server/lib/ServerLocale";
 import {allowedEventTypes, isUrlAllowed, WebhookAction, WebHookSecret} from "app/server/lib/Triggers";
-import { handleOptionalUpload, handleUpload } from "app/server/lib/uploads";
+import {handleOptionalUpload, handleUpload} from "app/server/lib/uploads";
-import DocApiTypesTI from "app/plugin/DocApiTypes-ti";
-import * as Types from "app/plugin/DocApiTypes";
 import * as contentDisposition from 'content-disposition';
-import { Application, NextFunction, Request, RequestHandler, Response } from "express";
+import {Application, NextFunction, Request, RequestHandler, Response} from "express";
 import * as _ from "lodash";
 import * as LRUCache from 'lru-cache';
 import fetch from 'node-fetch';
 import * as path from 'path';
-import * as uuidv4 from "uuid/v4";
 import * as t from "ts-interface-checker";
-import { Checker } from "ts-interface-checker";
+import {Checker} from "ts-interface-checker";
-import { ServerColumnGetters } from 'app/server/lib/ServerColumnGetters';
+import * as uuidv4 from "uuid/v4";
-import { Sort } from 'app/common/SortSpec';
 
 // Cap on the number of requests that can be outstanding on a single document via the
 // rest doc api.  When this limit is exceeded, incoming requests receive an immediate
@@ -631,8 +646,7 @@ export class DocWorkerApi {
 
     this._app.get('/api/docs/:docId/download/csv', canView, withDoc(async (activeDoc, req, res) => {
       // Query DB for doc metadata to get the doc title.
-      const {name: docTitle} =
+      const {name: docTitle} = await this._dbManager.getDoc(req);
-        await this._dbManager.getDoc({userId: getUserId(req), org: req.org, urlId: getDocId(req)});
 
       const params = parseExportParameters(req);
       const filename = docTitle + (params.tableId === docTitle ? '' : '-' + params.tableId);
@@ -647,8 +661,7 @@ export class DocWorkerApi {
 
     this._app.get('/api/docs/:docId/download/xlsx', canView, withDoc(async (activeDoc, req, res) => {
       // Query DB for doc metadata to get the doc title (to use as the filename).
-      const {name: filename} =
+      const {name: filename} = await this._dbManager.getDoc(req);
-        await this._dbManager.getDoc({userId: getUserId(req), org: req.org, urlId: getDocId(req)});
 
       const options: DownloadXLSXOptions = {filename};
 
@@ -707,9 +720,7 @@ export class DocWorkerApi {
    * request.
    */
   private async _confirmDocIdForRead(req: Request, urlId: string): Promise<string> {
-    const userId = getUserId(req);
+    const docAuth = await makeDocAuthResult(this._dbManager.getDoc({...getScope(req), urlId}));
-    const org = (req as RequestWithLogin).org;
-    const docAuth = await makeDocAuthResult(this._dbManager.getDoc({urlId, userId, org}));
     if (docAuth.error) { throw docAuth.error; }
     assertAccess('viewers', docAuth);
     return docAuth.docId!;

app/server/lib/DocManager.ts

@@ -1,4 +1,6 @@
 import * as pidusage from '@gristlabs/pidusage';
+import {Document} from 'app/gen-server/entity/Document';
+import {getScope} from 'app/server/lib/requestUtils';
 import * as bluebird from 'bluebird';
 import {EventEmitter} from 'events';
 import * as path from 'path';
@@ -464,23 +466,50 @@ export class DocManager extends EventEmitter {
     return activeDoc;
   }
 
-  private async _createActiveDoc(docSession: OptDocSession, docName: string, safeMode?: boolean) {
+  private async _getDoc(docSession: OptDocSession, docName: string) {
-    // Get URL for document for use with SELF_HYPERLINK().
     const cachedDoc = getDocSessionCachedDoc(docSession);
-    let docUrl: string|undefined;
-    try {
     if (cachedDoc) {
-        docUrl = await this.gristServer.getResourceUrl(cachedDoc);
+      return cachedDoc;
-      } else {
-        docUrl = await this.gristServer.getDocUrl(docName);
     }
+
+    let db: HomeDBManager;
+    try {
+      // For the sake of existing tests, get the db from gristServer where it may not exist and we should give up,
+      // rather than using this._homeDbManager which may exist and then it turns out the document itself doesn't.
+      db = this.gristServer.getHomeDBManager();
+    } catch (e) {
+      if (e.message === "no db") {
+        return;
+      }
+      throw e;
+    }
+
+    if (docSession.req) {
+      const scope = getScope(docSession.req);
+      if (scope.urlId) {
+        return db.getDoc(scope);
+      }
+    }
+
+    return await db.getRawDocById(docName);
+  }
+
+  private async _getDocUrl(doc: Document) {
+    try {
+      return await this.gristServer.getResourceUrl(doc);
     } catch (e) {
       // If there is no home url, we cannot construct links.  Accept this, for the benefit
       // of legacy tests.
-      if (!String(e).match(/need APP_HOME_URL/)) {
+      if (e.message !== "need APP_HOME_URL") {
         throw e;
       }
     }
+  }
+
+  private async _createActiveDoc(docSession: OptDocSession, docName: string, safeMode?: boolean) {
+    const doc = await this._getDoc(docSession, docName);
+    // Get URL for document for use with SELF_HYPERLINK().
+    const docUrl = doc && await this._getDocUrl(doc);
     return this.gristServer.create.ActiveDoc(this, docName, {docUrl, safeMode});
   }
 

app/server/lib/DocWorker.ts

@@ -4,14 +4,14 @@
  */
 import {HomeDBManager} from 'app/gen-server/lib/HomeDBManager';
 import {ActionHistoryImpl} from 'app/server/lib/ActionHistoryImpl';
-import {assertAccess, getOrSetDocAuth, getUserId, RequestWithLogin} from 'app/server/lib/Authorizer';
+import {assertAccess, getOrSetDocAuth, RequestWithLogin} from 'app/server/lib/Authorizer';
 import {Client} from 'app/server/lib/Client';
 import * as Comm from 'app/server/lib/Comm';
 import {DocSession, docSessionFromRequest} from 'app/server/lib/DocSession';
 import {filterDocumentInPlace} from 'app/server/lib/filterUtils';
 import {IDocStorageManager} from 'app/server/lib/IDocStorageManager';
 import * as log from 'app/server/lib/log';
-import {integerParam, optStringParam, stringParam} from 'app/server/lib/requestUtils';
+import {getDocId, integerParam, optStringParam, stringParam} from 'app/server/lib/requestUtils';
 import {OpenMode, quoteIdent, SQLiteDB} from 'app/server/lib/SQLiteDB';
 import * as contentDisposition from 'content-disposition';
 import * as express from 'express';
@@ -58,11 +58,10 @@ export class DocWorker {
   public async downloadDoc(req: express.Request, res: express.Response,
                            storageManager: IDocStorageManager): Promise<void> {
     const mreq = req as RequestWithLogin;
-    if (!mreq.docAuth || !mreq.docAuth.docId) { throw new Error('Cannot find document'); }
+    const docId = getDocId(mreq);
-    const docId = mreq.docAuth.docId;
 
     // Query DB for doc metadata to get the doc title.
-    const doc = await this._dbManager.getDoc({userId: getUserId(req), org: mreq.org, urlId: docId});
+    const doc = await this._dbManager.getDoc(req);
     const docTitle = doc.name;
 
     // Get a copy of document for downloading.

app/server/lib/FlexServer.ts

@@ -1223,20 +1223,6 @@ export class FlexServer implements GristServer {
     return makeGristConfig(this.getDefaultHomeUrl(), {}, this._defaultBaseDomain);
   }
 
-  /**
-   * Get a url for a document.  The id provided should be a genuine docId, since we query
-   * the db for document details without including organization disambiguation.
-   */
-  public async getDocUrl(docId: string): Promise<string> {
-    if (!this._dbManager) { throw new Error('database missing'); }
-    const doc = await this._dbManager.getDoc({
-      userId: this._dbManager.getPreviewerUserId(),
-      urlId: docId,
-      showAll: true
-    });
-    return this.getResourceUrl(doc);
-  }
-
   /**
    * Get a url for a team site.
    */

app/server/lib/GristServer.ts

@@ -25,7 +25,6 @@ export interface GristServer {
   getHomeUrl(req: express.Request, relPath?: string): string;
   getHomeUrlByDocId(docId: string, relPath?: string): Promise<string>;
   getOwnUrl(): string;
-  getDocUrl(docId: string): Promise<string>;
   getOrgUrl(orgKey: string|number): Promise<string>;
   getMergedOrgUrl(req: RequestWithLogin, pathname?: string): string;
   getResourceUrl(resource: Organization|Workspace|Document): Promise<string>;

app/server/lib/requestUtils.ts

@@ -150,10 +150,10 @@ export function getDocScope(req: Request): DocScope {
  *     is limited to docs/workspaces that have been removed.
  */
 export function getScope(req: Request): Scope {
-  const urlId = req.params.did || req.params.docId;
+  const {specialPermit, docAuth} = (req as RequestWithLogin);
+  const urlId = req.params.did || req.params.docId || docAuth?.docId || undefined;
   const userId = getUserId(req);
   const org = (req as RequestWithOrg).org;
-  const {specialPermit} = (req as RequestWithLogin);
   const includeSupport = isParameterOn(req.query.includeSupport);
   const showRemoved = isParameterOn(req.query.showRemoved);
   return {urlId, userId, org, includeSupport, showRemoved, specialPermit};