From 521bbd9ac10906fbb6781407d38b6bd7879cecea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaros=C5=82aw=20Sadzi=C5=84ski?= Date: Mon, 2 Aug 2021 09:27:16 +0200 Subject: [PATCH] (core) Improving error messages on file imports Summary: Improving error messages that get returned from "Import from URL" plugin. Test Plan: browser tests Reviewers: alexmojaki Reviewed By: alexmojaki Subscribers: dsagal, alexmojaki Differential Revision: https://phab.getgrist.com/D2946 --- app/server/lib/DocPluginManager.ts | 60 +++++++++++-------- app/server/lib/uploads.ts | 93 ++++++++++++++++++++---------- 2 files changed, 99 insertions(+), 54 deletions(-) diff --git a/app/server/lib/DocPluginManager.ts b/app/server/lib/DocPluginManager.ts index 390a3403..6a7ac64b 100644 --- a/app/server/lib/DocPluginManager.ts +++ b/app/server/lib/DocPluginManager.ts @@ -1,18 +1,18 @@ -import {ApplyUAResult} from 'app/common/ActiveDocAPI'; -import {fromTableDataAction, TableColValues} from 'app/common/DocActions'; +import { ApplyUAResult } from 'app/common/ActiveDocAPI'; +import { fromTableDataAction, TableColValues } from 'app/common/DocActions'; import * as gutil from 'app/common/gutil'; -import {LocalPlugin} from 'app/common/plugin'; -import {createRpcLogger, PluginInstance} from 'app/common/PluginInstance'; -import {Promisified} from 'app/common/tpromisified'; -import {ParseFileResult, ParseOptions} from 'app/plugin/FileParserAPI'; -import {checkers, GristTable} from "app/plugin/grist-plugin-api"; -import {GristDocAPI} from "app/plugin/GristAPI"; -import {Storage} from 'app/plugin/StorageAPI'; -import {ActiveDoc} from 'app/server/lib/ActiveDoc'; -import {DocPluginData} from 'app/server/lib/DocPluginData'; -import {makeExceptionalDocSession} from 'app/server/lib/DocSession'; -import {FileParserElement} from 'app/server/lib/FileParserElement'; -import {GristServer} from 'app/server/lib/GristServer'; +import { LocalPlugin } from 'app/common/plugin'; +import { createRpcLogger, PluginInstance } from 'app/common/PluginInstance'; +import { Promisified } from 'app/common/tpromisified'; +import { ParseFileResult, ParseOptions } from 'app/plugin/FileParserAPI'; +import { checkers, GristTable } from "app/plugin/grist-plugin-api"; +import { GristDocAPI } from "app/plugin/GristAPI"; +import { Storage } from 'app/plugin/StorageAPI'; +import { ActiveDoc } from 'app/server/lib/ActiveDoc'; +import { DocPluginData } from 'app/server/lib/DocPluginData'; +import { makeExceptionalDocSession } from 'app/server/lib/DocSession'; +import { FileParserElement } from 'app/server/lib/FileParserElement'; +import { GristServer } from 'app/server/lib/GristServer'; import * as log from 'app/server/lib/log'; import { SafePythonComponent } from 'app/server/lib/SafePythonComponent'; import { UnsafeNodeComponent } from 'app/server/lib/UnsafeNodeComponent'; @@ -28,14 +28,14 @@ promisifyAll(tmp); * Implements GristDocAPI interface. */ class GristDocAPIImpl implements GristDocAPI { - constructor(private _activeDoc: ActiveDoc) {} + constructor(private _activeDoc: ActiveDoc) { } public async getDocName() { return this._activeDoc.docName; } public async listTables(): Promise { const table = this._activeDoc.docData!.getTable('_grist_Tables')!; return (table.getColValues('tableId') as string[]) - .filter(id => !id.startsWith("GristSummary_")).sort(); + .filter(id => !id.startsWith("GristSummary_")).sort(); } public async fetchTable(tableId: string): Promise { @@ -57,7 +57,7 @@ class GristDocAPIImpl implements GristDocAPI { */ export class DocPluginManager { - public readonly plugins: {[s: string]: PluginInstance} = {}; + public readonly plugins: { [s: string]: PluginInstance } = {}; public readonly ready: Promise; public readonly gristDocAPI: GristDocAPI; @@ -101,6 +101,10 @@ export class DocPluginManager { } } + if (path.extname(fileName) === '.grist') { + throw new Error(`To import a grist document use the "Import document" menu option on your home screen`); + } + const matchingFileParsers: FileParserElement[] = FileParserElement.getMatching(this._pluginInstances, fileName); if (!this._tmpDir) { @@ -113,11 +117,11 @@ export class DocPluginManager { filePath = path.relative(this._tmpDir, filePath); log.debug(`parseFile: found ${matchingFileParsers.length} fileParser with matching file extensions`); const messages = []; - for (const {plugin, parseFileStub} of matchingFileParsers) { + for (const { plugin, parseFileStub } of matchingFileParsers) { const name = plugin.definition.id; try { log.info(`DocPluginManager.parseFile: calling to ${name} with ${filePath}`); - const result = await parseFileStub.parseFile({path: filePath, origName: fileName}, parseOptions); + const result = await parseFileStub.parseFile({ path: filePath, origName: fileName }, parseOptions); checkers.ParseFileResult.check(result); checkReferences(result.tables); return result; @@ -128,8 +132,18 @@ export class DocPluginManager { continue; } } - const details = messages.length ? ": " + messages.join("; ") : ""; - throw new Error(`Cannot parse this data${details}`); + + if (messages.length) { + const extToType: Record = { + '.xlsx' : 'Excel', + '.xls' : 'Excel', + '.json' : 'JSON', + '.csv' : 'CSV', + }; + const fileType = extToType[path.extname(fileName)] || path.extname(fileName); + throw new Error(`Failed to parse ${fileType} file. Error: ${messages.join("; ")}`); + } + throw new Error(`File format is not supported.`); } /** @@ -173,7 +187,7 @@ export class DocPluginManager { } private async _initialize(): Promise { - this._tmpDir = await tmp.dirAsync({prefix: 'grist-tmp-', unsafeCleanup: true}); + this._tmpDir = await tmp.dirAsync({ prefix: 'grist-tmp-', unsafeCleanup: true }); for (const plugin of this._localPlugins) { try { // todo: once Comm has been replaced by grain-rpc, pluginInstance.rpc should forward '*' to client @@ -184,7 +198,7 @@ export class DocPluginManager { new DocPluginData(this._activeDoc.docStorage, plugin.id), checkers.Storage); const components = plugin.manifest.components; if (components) { - const {safePython, unsafeNode} = components; + const { safePython, unsafeNode } = components; if (safePython) { const comp = pluginInstance.safePython = new SafePythonComponent(plugin, safePython, this._tmpDir, this._activeDoc.docName, this._server); diff --git a/app/server/lib/uploads.ts b/app/server/lib/uploads.ts index d265a3bf..f0585292 100644 --- a/app/server/lib/uploads.ts +++ b/app/server/lib/uploads.ts @@ -337,36 +337,45 @@ export async function fetchURL(url: string, accessId: string|null): Promise { - const response: FetchResponse = await Deps.fetch(url, { - redirect: 'follow', - follow: 10, - headers - }); - await _checkForError(response); - if (fileName === '') { - const disposition = response.headers.get('content-disposition') || ''; - fileName = contentDisposition.parse(disposition).parameters.filename || 'document.grist'; + try { + const response: FetchResponse = await Deps.fetch(url, { + redirect: 'follow', + follow: 10, + headers + }); + await _checkForError(response); + if (fileName === '') { + const disposition = response.headers.get('content-disposition') || ''; + fileName = contentDisposition.parse(disposition).parameters.filename || 'document.grist'; + } + const mimeType = response.headers.get('content-type'); + const {tmpDir, cleanupCallback} = await createTmpDir({}); + // Any name will do for the single file in tmpDir, but note that fileName may not be valid. + const destPath = path.join(tmpDir, 'upload-content'); + await new Promise((resolve, reject) => { + const dest = fse.createWriteStream(destPath, {autoClose: true}); + response.body.on('error', reject); + dest.on('error', reject); + dest.on('finish', resolve); + response.body.pipe(dest); + }); + const uploadedFile: FileUploadInfo = { + absPath: path.resolve(destPath), + origName: fileName, + size: (await fse.stat(destPath)).size, + ext: await guessExt(destPath, fileName, mimeType), + }; + log.debug(`done fetching url: ${url} to ${destPath}`); + const uploadId = globalUploadSet.registerUpload([uploadedFile], tmpDir, cleanupCallback, accessId); + return {uploadId, files: [pick(uploadedFile, ['origName', 'size', 'ext'])]}; + } catch(err) { + if (err?.code === "EPROTO" || // https vs http error + err?.code === "ECONNREFUSED" || // server does not listen + err?.code === "ENOTFOUND") { // could not resolve domain + throw new ApiError(`Can't connect to the server. The URL seems to be invalid. Error code ${err.code}`, 400); + } + throw err; } - const mimeType = response.headers.get('content-type'); - const {tmpDir, cleanupCallback} = await createTmpDir({}); - // Any name will do for the single file in tmpDir, but note that fileName may not be valid. - const destPath = path.join(tmpDir, 'upload-content'); - await new Promise((resolve, reject) => { - const dest = fse.createWriteStream(destPath, {autoClose: true}); - response.body.on('error', reject); - dest.on('error', reject); - dest.on('finish', resolve); - response.body.pipe(dest); - }); - const uploadedFile: FileUploadInfo = { - absPath: path.resolve(destPath), - origName: fileName, - size: (await fse.stat(destPath)).size, - ext: await guessExt(destPath, fileName, mimeType), - }; - log.debug(`done fetching url: ${url} to ${destPath}`); - const uploadId = globalUploadSet.registerUpload([uploadedFile], tmpDir, cleanupCallback, accessId); - return {uploadId, files: [pick(uploadedFile, ['origName', 'size', 'ext'])]}; } /** @@ -392,9 +401,31 @@ async function fetchDoc(homeUrl: string, docId: string, req: Request, accessId: // Re-issue failures as exceptions. async function _checkForError(response: FetchResponse) { - if (response.ok) { return; } + if (response.status === 403) { + throw new ApiError("Access to this resource was denied.", response.status); + } + if (response.ok) { + const contentType = response.headers.get("content-type"); + if (contentType?.startsWith("text/html")) { + // Probably we hit some login page + if (response.url.startsWith("https://accounts.google.com")) { + throw new ApiError("Importing directly from a Google Drive URL is not supported yet. " + + 'Use the "Import from Google Drive" menu option instead.', 403); + } else { + throw new ApiError("Could not import the requested file, check if you have all required permissions.", 403); + } + } + return; + } const body = await response.json().catch(() => ({})); - throw new ApiError(body.error || response.statusText, response.status, body.details); + if (response.status === 404) { + throw new ApiError("File can't be found at the requested URL.", 404); + } else if (response.status >= 500 && response.status < 600) { + throw new ApiError(`Remote server returned an error (${body.error || response.statusText})`, + response.status, body.details); + } else { + throw new ApiError(body.error || response.statusText, response.status, body.details); + } } /**