(core) Exposing more descriptive errors from exports

Summary:
Exports used to show generic message on error.
Adding error description to the message.

Test Plan: Updated tests

Reviewers: paulfitz

Reviewed By: paulfitz

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

app/gen-server/ApiServer.ts

@@ -53,7 +53,7 @@ function helpScoutSign(email: string): string|undefined {
  *   - If there is no identifier available, a 400 error is thrown.
  */
 export function getOrgKey(req: Request): string|number {
-  let orgKey: string|null = stringParam(req.params.oid);
+  let orgKey: string|null = stringParam(req.params.oid, 'oid');
   if (orgKey === 'current') {
     orgKey = getOrgFromRequest(req);
   }
@@ -119,7 +119,7 @@ export class ApiServer {
     // GET /api/workspace/:wid
     // Get workspace by id, returning nested documents that user has access to.
     this._app.get('/api/workspaces/:wid', expressWrap(async (req, res) => {
-      const wsId = integerParam(req.params.wid);
+      const wsId = integerParam(req.params.wid, 'wid');
       const query = await this._dbManager.getWorkspace(getScope(req), wsId);
       return sendReply(req, res, query);
     }));
@@ -183,7 +183,7 @@ export class ApiServer {
     // Body params: name
     // Update the specified workspace.
     this._app.patch('/api/workspaces/:wid', expressWrap(async (req, res) => {
-      const wsId = integerParam(req.params.wid);
+      const wsId = integerParam(req.params.wid, 'wid');
       const query = await this._dbManager.updateWorkspace(getScope(req), wsId, req.body);
       return sendReply(req, res, query);
     }));
@@ -191,7 +191,7 @@ export class ApiServer {
     // // DELETE /api/workspaces/:wid
     // Delete the specified workspace and all included docs.
     this._app.delete('/api/workspaces/:wid', expressWrap(async (req, res) => {
-      const wsId = integerParam(req.params.wid);
+      const wsId = integerParam(req.params.wid, 'wid');
       const query = await this._dbManager.deleteWorkspace(getScope(req), wsId);
       return sendReply(req, res, query);
     }));
@@ -200,7 +200,7 @@ export class ApiServer {
     // Soft-delete the specified workspace.  If query parameter "permanent" is set,
     // delete permanently.
     this._app.post('/api/workspaces/:wid/remove', expressWrap(async (req, res) => {
-      const wsId = integerParam(req.params.wid);
+      const wsId = integerParam(req.params.wid, 'wid');
       if (isParameterOn(req.query.permanent)) {
         const query = await this._dbManager.deleteWorkspace(getScope(req), wsId);
         return sendReply(req, res, query);
@@ -214,7 +214,7 @@ export class ApiServer {
     // Recover the specified workspace if it was previously soft-deleted and is
     // still available.
     this._app.post('/api/workspaces/:wid/unremove', expressWrap(async (req, res) => {
-      const wsId = integerParam(req.params.wid);
+      const wsId = integerParam(req.params.wid, 'wid');
       await this._dbManager.undeleteWorkspace(getScope(req), wsId);
       return sendOkReply(req, res);
     }));
@@ -222,7 +222,7 @@ export class ApiServer {
     // POST /api/workspaces/:wid/docs
     // Create a new doc owned by the specific workspace.
     this._app.post('/api/workspaces/:wid/docs', expressWrap(async (req, res) => {
-      const wsId = integerParam(req.params.wid);
+      const wsId = integerParam(req.params.wid, 'wid');
       const query = await this._dbManager.addDocument(getScope(req), wsId, req.body);
       return sendReply(req, res, query);
     }));
@@ -265,7 +265,7 @@ export class ApiServer {
     // PATCH /api/workspaces/:wid/access
     // Update the specified workspace acl rules.
     this._app.patch('/api/workspaces/:wid/access', expressWrap(async (req, res) => {
-      const workspaceId = integerParam(req.params.wid);
+      const workspaceId = integerParam(req.params.wid, 'wid');
       const delta = req.body.delta;
       const query = await this._dbManager.updateWorkspacePermissions(getScope(req), workspaceId, delta);
       return sendReply(req, res, query);
@@ -316,7 +316,7 @@ export class ApiServer {
     // GET /api/workspaces/:wid/access
     // Get user access information regarding a workspace
     this._app.get('/api/workspaces/:wid/access', expressWrap(async (req, res) => {
-      const workspaceId = integerParam(req.params.wid);
+      const workspaceId = integerParam(req.params.wid, 'wid');
       const query = await this._dbManager.getWorkspaceAccess(getScope(req), workspaceId);
       return sendReply(req, res, query);
     }));

app/gen-server/lib/Housekeeper.ts

@@ -228,7 +228,7 @@ export class Housekeeper {
       if (userId !== this._dbManager.getSupportUserId()) {
         throw new ApiError('access denied', 403);
       }
-      const docId = stringParam(req.params.docId);
+      const docId = stringParam(req.params.docId, 'docId');
       const permitKey = await this._permitStore.setPermit({docId});
       try {
         const result = await callback(docId, {

app/server/lib/DocApi.ts

@@ -356,8 +356,8 @@ export class DocWorkerApi {
 
     // Initiate a fork.  Used internally to implement ActiveDoc.fork.  Only usable via a Permit.
     this._app.post('/api/docs/:docId/create-fork', canEdit, throttled(async (req, res) => {
-      const docId = stringParam(req.params.docId);
+      const docId = stringParam(req.params.docId, 'docId');
-      const srcDocId = stringParam(req.body.srcDocId);
+      const srcDocId = stringParam(req.body.srcDocId, 'srcDocId');
       if (srcDocId !== req.specialPermit?.otherDocId) { throw new Error('access denied'); }
       const fname = await this._docManager.storageManager.prepareFork(srcDocId, docId);
       await filterDocumentInPlace(docSessionFromRequest(req), fname);
@@ -629,7 +629,7 @@ export class DocWorkerApi {
 
     this._app.post('/api/docs/:docId/states/remove', isOwner, withDoc(async (activeDoc, req, res) => {
       const docSession = docSessionFromRequest(req);
-      const keep = integerParam(req.body.keep);
+      const keep = integerParam(req.body.keep, 'keep');
       res.json(await activeDoc.deleteActions(docSession, keep));
     }));
 
@@ -683,8 +683,8 @@ export class DocWorkerApi {
     // Give details about what changed between two versions of a document.
     this._app.get('/api/docs/:docId/compare', canView, withDoc(async (activeDoc, req, res) => {
       // This could be a relatively slow operation if actions are large.
-      const left = stringParam(req.query.left || 'HEAD');
+      const left = stringParam(req.query.left || 'HEAD', 'left');
-      const right = stringParam(req.query.right || 'HEAD');
+      const right = stringParam(req.query.right || 'HEAD', 'right');
       const docSession = docSessionFromRequest(req);
       const {states} = await this._getStates(docSession, activeDoc);
       res.json(await this._getChanges(docSession, activeDoc, states, left, right));
@@ -695,8 +695,8 @@ export class DocWorkerApi {
     // actual file uploads, so no worries here about large request bodies.)
     this._app.post('/api/workspaces/:wid/import', expressWrap(async (req, res) => {
       const userId = getUserId(req);
-      const wsId = integerParam(req.params.wid);
+      const wsId = integerParam(req.params.wid, 'wid');
-      const uploadId = integerParam(req.body.uploadId);
+      const uploadId = integerParam(req.body.uploadId, 'uploadId');
       const result = await this._docManager.importDocToWorkspace(userId, uploadId, wsId, req.body.browserSettings);
       res.json(result);
     }));

app/server/lib/DocWorker.ts

@@ -31,10 +31,10 @@ export class DocWorker {
 
   public async getAttachment(req: express.Request, res: express.Response): Promise<void> {
     try {
-      const docSession = this._getDocSession(stringParam(req.query.clientId),
+      const docSession = this._getDocSession(stringParam(req.query.clientId, 'clientId'),
-                                             integerParam(req.query.docFD));
+                                             integerParam(req.query.docFD, 'docFD'));
       const activeDoc = docSession.activeDoc;
-      const ext = path.extname(stringParam(req.query.ident));
+      const ext = path.extname(stringParam(req.query.ident, 'ident'));
       const type = mimeTypes.lookup(ext);
 
       let inline = Boolean(req.query.inline);
@@ -43,8 +43,8 @@ export class DocWorker {
 
       // Construct a content-disposition header of the form 'inline|attachment; filename="NAME"'
       const contentDispType = inline ? "inline" : "attachment";
-      const contentDispHeader = contentDisposition(stringParam(req.query.name), {type: contentDispType});
+      const contentDispHeader = contentDisposition(stringParam(req.query.name, 'name'), {type: contentDispType});
-      const data = await activeDoc.getAttachmentData(docSession, stringParam(req.query.ident));
+      const data = await activeDoc.getAttachmentData(docSession, stringParam(req.query.ident, 'ident'));
       res.status(200)
         .type(ext)
         .set('Content-Disposition', contentDispHeader)
@@ -138,8 +138,8 @@ export class DocWorker {
     let urlId: string|undefined;
     try {
       if (optStringParam(req.query.clientId)) {
-        const activeDoc = this._getDocSession(stringParam(req.query.clientId),
+        const activeDoc = this._getDocSession(stringParam(req.query.clientId, 'clientId'),
-                                              integerParam(req.query.docFD)).activeDoc;
+                                              integerParam(req.query.docFD, 'docFD')).activeDoc;
         // TODO: The docId should be stored in the ActiveDoc class. Currently docName is
         // used instead, which will coincide with the docId for hosted grist but not for
         // standalone grist.
@@ -147,7 +147,7 @@ export class DocWorker {
       } else {
         // Otherwise, if being used without a client, expect the doc query parameter to
         // be the docId.
-        urlId = stringParam(req.query.doc);
+        urlId = stringParam(req.query.doc, 'doc');
       }
       if (!urlId) { return res.status(403).send({error: 'missing document id'}); }
 

app/server/lib/Export.ts

@@ -1,19 +1,20 @@
-import { buildColFilter } from 'app/common/ColumnFilterFunc';
+import {ApiError} from 'app/common/ApiError';
-import { RowRecord } from 'app/common/DocActions';
+import {buildColFilter} from 'app/common/ColumnFilterFunc';
-import { DocData } from 'app/common/DocData';
+import {RowRecord} from 'app/common/DocActions';
-import { DocumentSettings } from 'app/common/DocumentSettings';
+import {DocData} from 'app/common/DocData';
+import {DocumentSettings} from 'app/common/DocumentSettings';
 import * as gristTypes from 'app/common/gristTypes';
 import * as gutil from 'app/common/gutil';
-import { buildRowFilter } from 'app/common/RowFilterFunc';
+import {buildRowFilter} from 'app/common/RowFilterFunc';
-import { SchemaTypes } from 'app/common/schema';
+import {SchemaTypes} from 'app/common/schema';
-import { SortFunc } from 'app/common/SortFunc';
+import {SortFunc} from 'app/common/SortFunc';
-import { Sort } from 'app/common/SortSpec';
+import {Sort} from 'app/common/SortSpec';
-import { TableData } from 'app/common/TableData';
+import {TableData} from 'app/common/TableData';
-import { ActiveDoc } from 'app/server/lib/ActiveDoc';
+import {ActiveDoc} from 'app/server/lib/ActiveDoc';
-import { RequestWithLogin } from 'app/server/lib/Authorizer';
+import {RequestWithLogin} from 'app/server/lib/Authorizer';
-import { docSessionFromRequest } from 'app/server/lib/DocSession';
+import {docSessionFromRequest} from 'app/server/lib/DocSession';
-import { optIntegerParam, optJsonParam, stringParam } from 'app/server/lib/requestUtils';
+import {optIntegerParam, optJsonParam, stringParam} from 'app/server/lib/requestUtils';
-import { ServerColumnGetters } from 'app/server/lib/ServerColumnGetters';
+import {ServerColumnGetters} from 'app/server/lib/ServerColumnGetters';
 import * as express from 'express';
 import * as _ from 'underscore';
 
@@ -82,7 +83,7 @@ export interface ExportParameters {
  * Gets export parameters from a request.
  */
 export function parseExportParameters(req: express.Request): ExportParameters {
-  const tableId = stringParam(req.query.tableId);
+  const tableId = stringParam(req.query.tableId, 'tableId');
   const viewSectionId = optIntegerParam(req.query.viewSection);
   const sortOrder = optJsonParam(req.query.activeSortSpec, []) as number[];
   const filters: Filter[] = optJsonParam(req.query.filters, []);
@@ -97,7 +98,7 @@ export function parseExportParameters(req: express.Request): ExportParameters {
 
 // Makes assertion that value does exists or throws an error
 function safe<T>(value: T, msg: string) {
-  if (!value) { throw new Error(msg); }
+  if (!value) { throw new ApiError(msg, 404); }
   return value as NonNullable<T>;
 }
 

app/server/lib/ExportCSV.ts

@@ -1,11 +1,12 @@
+import {ApiError} from 'app/common/ApiError';
 import {createFormatter} from 'app/common/ValueFormatter';
 import {ActiveDoc} from 'app/server/lib/ActiveDoc';
 import {ExportData, exportSection, exportTable, Filter} from 'app/server/lib/Export';
+import * as log from 'app/server/lib/log';
 import * as bluebird from 'bluebird';
+import * as contentDisposition from 'content-disposition';
 import * as csv from 'csv';
 import * as express from 'express';
-import * as log from 'app/server/lib/log';
-import * as contentDisposition from 'content-disposition';
 
 export interface DownloadCSVOptions {
   filename: string;
@@ -25,24 +26,12 @@ export async function downloadCSV(activeDoc: ActiveDoc, req: express.Request,
                                   res: express.Response, options: DownloadCSVOptions) {
   log.info('Generating .csv file...');
   const {filename, tableId, viewSectionId, filters, sortOrder} = options;
-
+  const data = viewSectionId ?
-  try {
+    await makeCSVFromViewSection(activeDoc, viewSectionId, sortOrder, filters, req) :
-    const data = viewSectionId ?
+    await makeCSVFromTable(activeDoc, tableId, req);
-      await makeCSVFromViewSection(activeDoc, viewSectionId, sortOrder, filters, req) :
+  res.set('Content-Type', 'text/csv');
-      await makeCSVFromTable(activeDoc, tableId, req);
+  res.setHeader('Content-Disposition', contentDisposition(filename + '.csv'));
-    res.set('Content-Type', 'text/csv');
+  res.send(data);
-    res.setHeader('Content-Disposition', contentDisposition(filename + '.csv'));
-    res.send(data);
-  } catch (err) {
-    log.error("Exporting to CSV has failed. Request url: %s", req.url, err);
-    const errHtml =
-      `<!doctype html>
-<html>
-  <body>There was an unexpected error while generating a csv file.</body>
-</html>
-`;
-    res.status(400).send(errHtml);
-  }
 }
 
 /**
@@ -88,6 +77,10 @@ export async function makeCSVFromTable(
   const tables = activeDoc.docData.getTable('_grist_Tables')!;
   const tableRef = tables.findRow('tableId', tableId);
 
+  if (tableRef === 0) {
+    throw new ApiError(`Table ${tableId} not found.`, 404);
+  }
+
   const data = await exportTable(activeDoc, tableRef, req);
   const file = convertToCsv(data);
   return file;

app/server/lib/ExportXLSX.ts

@@ -16,23 +16,11 @@ export interface DownloadXLSXOptions {
 export async function downloadXLSX(activeDoc: ActiveDoc, req: express.Request,
                                    res: express.Response, {filename}: DownloadXLSXOptions) {
   log.debug(`Generating .xlsx file`);
-  try {
+  const data = await makeXLSX(activeDoc, req);
-    const data = await makeXLSX(activeDoc, req);
+  res.set('Content-Type', 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet');
-    res.set('Content-Type', 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet');
+  res.setHeader('Content-Disposition', contentDisposition(filename + '.xlsx'));
-    res.setHeader('Content-Disposition', contentDisposition(filename + '.xlsx'));
+  res.send(data);
-    res.send(data);
+  log.debug('XLSX file generated');
-    log.debug('XLSX file generated');
-  } catch (err) {
-    log.error("Exporting to XLSX has failed. Request url: %s", req.url, err);
-    // send a generic information to client
-    const errHtml =
-      `<!doctype html>
-<html>
-  <body>There was an unexpected error while generating a xlsx file.</body>
-</html>
-`;
-    res.status(400).send(errHtml);
-  }
 }
 
 /**

app/server/lib/FlexServer.ts

@@ -319,7 +319,7 @@ export class FlexServer implements GristServer {
     if (this._check('router')) { return; }
     this.app.get('/test/router', (req, res) => {
       const act = optStringParam(req.query.act) || 'none';
-      const port = stringParam(req.query.port);  // port is trusted in mock; in prod it is not.
+      const port = stringParam(req.query.port, 'port');  // port is trusted in mock; in prod it is not.
       if (act === 'add' || act === 'remove') {
         const host = `localhost:${port}`;
         return res.status(200).json({

app/server/lib/GoogleAuth.ts

@@ -97,7 +97,7 @@ export async function googleAuthTokenMiddleware(
     try {
       const oAuth2Client = getGoogleAuth();
       // Decrypt code that was send back from Google Auth service. Uses GOOGLE_CLIENT_SECRET key.
-      const tokenResponse = await oAuth2Client.getToken(stringParam(req.query.code));
+      const tokenResponse = await oAuth2Client.getToken(stringParam(req.query.code, 'code'));
       // Get the access token (access token will be present in a default request configuration).
       const access_token = tokenResponse.tokens.access_token!;
       req.query.access_token = access_token;
@@ -136,20 +136,20 @@ export function addGoogleAuthEndpoint(
 
     if (optStringParam(req.query.code)) {
       log.debug("GoogleAuth - response from Google with valid code");
-      messagePage(req, res, { code: stringParam(req.query.code),
+      messagePage(req, res, { code: stringParam(req.query.code, 'code'),
-                              origin: stringParam(req.query.state) });
+                              origin: stringParam(req.query.state, 'state') });
     } else if (optStringParam(req.query.error)) {
-      log.debug("GoogleAuth - response from Google with error code", stringParam(req.query.error));
+      log.debug("GoogleAuth - response from Google with error code", stringParam(req.query.error, 'error'));
-      if (stringParam(req.query.error) === "access_denied") {
+      if (stringParam(req.query.error, 'error') === "access_denied") {
-        messagePage(req, res, { error: stringParam(req.query.error),
+        messagePage(req, res, { error: stringParam(req.query.error, 'error'),
-                                origin: stringParam(req.query.state) });
+                                origin: stringParam(req.query.state, 'state') });
       } else {
         // This should not happen, either code or error is a mandatory query parameter.
         throw new ApiError("Error authenticating with Google", 500);
       }
     } else {
       const oAuth2Client = getGoogleAuth();
-      const scope = stringParam(req.query.scope);
+      const scope = stringParam(req.query.scope, 'scope');
       // Create url for origin parameter for a popup window.
       const origin = getOriginUrl(req);
       const authUrl = oAuth2Client.generateAuthUrl({

app/server/lib/GranularAccess.ts

@@ -1351,7 +1351,7 @@ export class GranularAccess implements GranularAccessForBundle {
     // Look up user information in database.
     if (!this._homeDbManager) { throw new Error('database required'); }
     const dbUser = linkParameters.aclAsUserId ?
-      (await this._homeDbManager.getUser(integerParam(linkParameters.aclAsUserId))) :
+      (await this._homeDbManager.getUser(integerParam(linkParameters.aclAsUserId, 'aclAsUserId'))) :
       (await this._homeDbManager.getExistingUserByLogin(linkParameters.aclAsUser));
     if (!dbUser && linkParameters.aclAsUser) {
       // Look further for the user, in user attribute tables or examples.

app/server/lib/requestUtils.ts

@@ -215,16 +215,16 @@ export function optStringParam(p: any): string|undefined {
   return undefined;
 }
 
-export function stringParam(p: any, allowed?: string[]): string {
+export function stringParam(p: any, name: string, allowed?: string[]): string {
-  if (typeof p !== 'string') { throw new Error(`parameter should be a string: ${p}`); }
+  if (typeof p !== 'string') { throw new Error(`${name} parameter should be a string: ${p}`); }
-  if (allowed && !allowed.includes(p)) { throw new Error(`parameter ${p} should be one of ${allowed}`); }
+  if (allowed && !allowed.includes(p)) { throw new Error(`${name} parameter ${p} should be one of ${allowed}`); }
   return p;
 }
 
-export function integerParam(p: any): number {
+export function integerParam(p: any, name: string): number {
   if (typeof p === 'number') { return Math.floor(p); }
   if (typeof p === 'string') { return parseInt(p, 10); }
-  throw new Error(`parameter should be an integer: ${p}`);
+  throw new Error(`${name} parameter should be an integer: ${p}`);
 }
 
 export function optIntegerParam(p: any): number|undefined {