diff --git a/functions/src/export-csv.ts b/functions/src/export-csv.ts index 33a9c7a47..7328a716a 100644 --- a/functions/src/export-csv.ts +++ b/functions/src/export-csv.ts @@ -128,7 +128,6 @@ export async function exportCsvHandler( } } - res.status(StatusCodes.OK); csvStream.end(); } diff --git a/functions/src/export-geojson.spec.ts b/functions/src/export-geojson.spec.ts index 607c93e70..85a0e7570 100644 --- a/functions/src/export-geojson.spec.ts +++ b/functions/src/export-geojson.spec.ts @@ -23,7 +23,6 @@ import { createResponseSpy, } from './testing/http-test-helpers'; import { DecodedIdToken } from 'firebase-admin/auth'; -import { StatusCodes } from 'http-status-codes'; import { DATA_COLLECTOR_ROLE } from './common/auth'; import { resetDatastore } from './common/context'; import { Firestore } from 'firebase-admin/firestore'; @@ -210,7 +209,6 @@ describe('export()', () => { } as DecodedIdToken); // Check post-conditions. - expect(res.status).toHaveBeenCalledOnceWith(StatusCodes.OK); expect(res.type).toHaveBeenCalledOnceWith('application/json'); expect(res.setHeader).toHaveBeenCalledOnceWith( 'Content-Disposition', diff --git a/functions/src/export-geojson.ts b/functions/src/export-geojson.ts index 063858c35..ce2323fc1 100644 --- a/functions/src/export-geojson.ts +++ b/functions/src/export-geojson.ts @@ -84,8 +84,6 @@ export async function exportGeojsonHandler( 'Content-Disposition', 'attachment; filename=' + getFileName(jobName) ); - res.status(StatusCodes.OK); - // Write opening of FeatureCollection manually res.write('{\n "type": "FeatureCollection",\n "features": [\n'); diff --git a/functions/src/handlers.ts b/functions/src/handlers.ts index 0bd11c918..6d32aaa48 100644 --- a/functions/src/handlers.ts +++ b/functions/src/handlers.ts @@ -16,7 +16,7 @@ import cors from 'cors'; import { DecodedIdToken } from 'firebase-admin/auth'; -import { onRequest, Request } from 'firebase-functions/v2/https'; +import { Request, onRequest } from 'firebase-functions/v2/https'; import type { Response } from 'express'; import { StatusCodes } from 'http-status-codes'; import { getDecodedIdToken } from './common/auth'; @@ -58,7 +58,7 @@ async function requireIdToken( } } -function onError(res: any, err: any) { +function onError(res: Response, err: any) { console.error(err); res .status(StatusCodes.INTERNAL_SERVER_ERROR) @@ -78,8 +78,8 @@ export function onHttpsRequest(handler: HttpsRequestHandler) { return onRequest((req: Request, res: Response) => corsMiddleware(req, res, () => cookieParser()( - req as any, - res as any, + req as Request, + res as Response, async () => await requireIdToken(req, res, async (idToken: DecodedIdToken) => { try { @@ -92,81 +92,3 @@ export function onHttpsRequest(handler: HttpsRequestHandler) { ) ); } - -/** A function which is to be called by HTTPS callbacks on failure. */ -export type ErrorHandler = (httpStatusCode: number, message: string) => void; - -/** - * A callback-based HTTPS request handler. Functions of this type are expected to call - * `done()` on completion or `error()` on failure. The function itself may return before - * work is completed, but the HTTPS request will not complete until one of those two - * callbacks are invoked. - */ -export type HttpsRequestCallback = ( - req: Request, - res: Response, - user: DecodedIdToken, - done: () => void, - error: ErrorHandler -) => void; - -export async function invokeCallbackAsync( - callback: HttpsRequestCallback, - req: Request, - res: Response, - user: DecodedIdToken -) { - await new Promise((resolve, reject) => - invokeCallback( - callback, - req, - res, - user, - () => { - res.status(StatusCodes.OK).end(); - resolve(undefined); - }, - (errorCode: number, message: string) => { - res.status(errorCode).end(message); - reject(`${message} (HTTP status ${errorCode})`); - } - ) - ); -} - -function invokeCallback( - callback: HttpsRequestCallback, - req: Request, - res: Response, - user: DecodedIdToken, - done: () => void, - error: ErrorHandler -) { - try { - callback(req, res, user, done, error); - } catch (e: any) { - console.error('Unhandled exception', e); - error(StatusCodes.INTERNAL_SERVER_ERROR, e.toString()); - } -} - -/** - * Call an asynchronous HTTPS request handler. Handlers of this type are expected to call - * `done()` on completion or `error()` on failure. The handler itself may return before - * work is completed, but the HTTPS request will not complete until one of those two - * callbacks are invoked. - */ -export function onHttpsRequestAsync(callback: HttpsRequestCallback) { - return onRequest((req: Request, res: Response) => - corsMiddleware(req, res, () => - cookieParser()( - req as any, - res as any, - async () => - await requireIdToken(req, res, async (idToken: DecodedIdToken) => { - await invokeCallbackAsync(callback, req, res, idToken); - }) - ) - ) - ); -} diff --git a/functions/src/import-geojson.spec.ts b/functions/src/import-geojson.spec.ts index 42e0267a2..132ba8e6f 100644 --- a/functions/src/import-geojson.spec.ts +++ b/functions/src/import-geojson.spec.ts @@ -22,11 +22,10 @@ import { createPostRequestSpy, createResponseSpy, } from './testing/http-test-helpers'; -import { importGeoJsonCallback } from './import-geojson'; +import { importGeoJsonHandler } from './import-geojson'; import { DecodedIdToken } from 'firebase-admin/auth'; import { Blob, FormData } from 'formdata-node'; import { StatusCodes } from 'http-status-codes'; -import { invokeCallbackAsync } from './handlers'; import { SURVEY_ORGANIZER_ROLE } from './common/auth'; import { resetDatastore } from './common/context'; import { Firestore } from 'firebase-admin/firestore'; @@ -267,19 +266,14 @@ describe('importGeoJson()', () => { ); const res = createResponseSpy(); - try { - // Run import GeoJSON function. - // Ideally we would call `importGeoJson` directly rather than via `invokeCallbackAsync`, - // but that would require mocking all middleware which may be overkill. - await invokeCallbackAsync(importGeoJsonCallback, req, res, { - email, - } as DecodedIdToken); - } catch (error) { - console.log(error); - } + await importGeoJsonHandler(req, res, { email } as DecodedIdToken); // Check post-conditions. - expect(res.status).toHaveBeenCalledOnceWith(expectedStatus); + if (expectedStatus === StatusCodes.OK) { + expect(res.json).toHaveBeenCalled(); + } else { + expect(res.status).toHaveBeenCalledOnceWith(expectedStatus); + } expect(await loiData(surveyId)).toEqual(expected); }) ); diff --git a/functions/src/import-geojson.ts b/functions/src/import-geojson.ts index 14e014e98..1906569c0 100644 --- a/functions/src/import-geojson.ts +++ b/functions/src/import-geojson.ts @@ -25,7 +25,6 @@ import { DecodedIdToken } from 'firebase-admin/auth'; import { GroundProtos } from '@ground/proto'; import { isGeometryValid, toDocumentData, toGeometryPb } from '@ground/lib'; import { Feature, GeoJsonProperties } from 'geojson'; -import { ErrorHandler } from './handlers'; import Pb = GroundProtos.ground.v1beta1; @@ -37,18 +36,16 @@ class BadRequestError extends Error { * Read the body of a multipart HTTP POSTed form containing a GeoJson 'file' * and required 'survey' id and 'job' id to the database. */ -export function importGeoJsonCallback( +export async function importGeoJsonHandler( req: Request, res: Response, - user: DecodedIdToken, - done: () => void, - error: ErrorHandler + user: DecodedIdToken ) { if (req.method !== 'POST') { - return error( - StatusCodes.METHOD_NOT_ALLOWED, - `Expected method POST, got ${req.method}` - ); + res + .status(StatusCodes.METHOD_NOT_ALLOWED) + .send(`Expected method POST, got ${req.method}`); + return; } const busboy = Busboy({ headers: req.headers }); @@ -66,77 +63,97 @@ export function importGeoJsonCallback( const ownerId = user.uid; - // This code will process each file uploaded. - busboy.on('file', async (_fieldname, fileStream) => { - const { survey: surveyId, job: jobId } = params; - if (!surveyId || !jobId) { - return error(StatusCodes.BAD_REQUEST, 'Missing survey and/or job ID'); - } - const survey = await db.fetchSurvey(surveyId); - if (!survey.exists) { - return error(StatusCodes.NOT_FOUND, `Survey ${surveyId} not found`); - } - if (!canImport(user, survey)) { - return error( - StatusCodes.FORBIDDEN, - `User does not have permission to import into survey ${surveyId}` - ); - } + try { + await new Promise((resolve, reject) => { + // This code will process each file uploaded. + busboy.on('file', async (_fieldname, fileStream) => { + const { survey: surveyId, job: jobId } = params; + if (!surveyId || !jobId) { + res + .status(StatusCodes.BAD_REQUEST) + .send('Missing survey and/or job ID'); + return reject('Missing survey and/or job ID'); + } + const survey = await db.fetchSurvey(surveyId); + if (!survey.exists) { + res + .status(StatusCodes.NOT_FOUND) + .send(`Survey ${surveyId} not found`); + return reject(`Survey ${surveyId} not found`); + } + if (!canImport(user, survey)) { + res + .status(StatusCodes.FORBIDDEN) + .send( + `User does not have permission to import into survey ${surveyId}` + ); + return reject( + `User does not have permission to import into survey ${surveyId}` + ); + } - console.debug( - `Importing GeoJSON into survey '${surveyId}', job '${jobId}'` - ); - - const parser = JSONStream.parse(['features', true], undefined); - - fileStream.pipe( - parser - .on('header', (data: any) => { - try { - onGeoJsonType(data.type); - if (data.crs) onGeoJsonCrs(data.crs); - } catch (error: any) { - busboy.emit('error', error); - } - }) - .on('data', (data: any) => { - if (!hasError) onGeoJsonFeature(data, surveyId, jobId); - }) - ); - }); + console.debug( + `Importing GeoJSON into survey '${surveyId}', job '${jobId}'` + ); - // Handle non-file fields in the task. survey and job must appear - // before the file for the file handler to work properly. - busboy.on('field', (key, val) => { - params[key] = val; - }); + const parser = JSONStream.parse(['features', true], undefined); - // Triggered once all uploaded files are processed by Busboy. - busboy.on('finish', async () => { - if (hasError) return; - try { - await Promise.all(inserts); - const count = inserts.length; - console.debug(`${count} LOIs imported`); - res.send(JSON.stringify({ count })); - done(); - } catch (err) { - console.debug(err); - error(StatusCodes.BAD_REQUEST, (err as Error).message); - } - }); + fileStream.pipe( + parser + .on('header', (data: any) => { + try { + onGeoJsonType(data.type); + if (data.crs) onGeoJsonCrs(data.crs); + } catch (error: any) { + busboy.emit('error', error); + } + }) + .on('data', (data: any) => { + if (!hasError) onGeoJsonFeature(data, surveyId, jobId); + }) + ); + }); - busboy.on('error', (err: any) => { - console.error('Busboy error', err); - hasError = true; - req.unpipe(busboy); - error(err.statusCode || StatusCodes.INTERNAL_SERVER_ERROR, err.message); - }); + // Handle non-file fields in the task. survey and job must appear + // before the file for the file handler to work properly. + busboy.on('field', (key, val) => { + params[key] = val; + }); - // Start processing the body data. - // Use this for Cloud Functions rather than `req.pipe(busboy)`: - // https://github.com/mscdex/busboy/issues/229#issuecomment-648303108 - busboy.end(req.rawBody); + // Triggered once all uploaded files are processed by Busboy. + busboy.on('finish', async () => { + if (hasError) return; + try { + await Promise.all(inserts); + const count = inserts.length; + console.debug(`${count} LOIs imported`); + res.json({ count }); + resolve(); + } catch (err) { + console.debug(err); + res.status(StatusCodes.BAD_REQUEST).send((err as Error).message); + reject((err as Error).message); + } + }); + + busboy.on('error', (err: any) => { + console.error('Busboy error', err); + hasError = true; + req.unpipe(busboy); + res + .status(err.statusCode || StatusCodes.INTERNAL_SERVER_ERROR) + .send(err.message); + reject(err.message); + }); + + // Start processing the body data. + // Use this for Cloud Functions rather than `req.pipe(busboy)`: + // https://github.com/mscdex/busboy/issues/229#issuecomment-648303108 + busboy.end(req.rawBody); + }); + } catch (err) { + console.debug('Import failed', err); + } /** * This function is called by Busboy during file parsing to ensure that the GeoJSON diff --git a/functions/src/index.ts b/functions/src/index.ts index 1e802aeb6..496563cc8 100644 --- a/functions/src/index.ts +++ b/functions/src/index.ts @@ -19,10 +19,10 @@ import { onDocumentCreated, onDocumentWritten, } from 'firebase-functions/v2/firestore'; -import { onHttpsRequest, onHttpsRequestAsync } from './handlers'; +import { onHttpsRequest } from './handlers'; import { handleProfileRefresh } from './profile-refresh'; import { sessionLoginHandler } from './session-login'; -import { importGeoJsonCallback } from './import-geojson'; +import { importGeoJsonHandler } from './import-geojson'; import { exportCsvHandler } from './export-csv'; import { exportGeojsonHandler } from './export-geojson'; import { onCall } from 'firebase-functions/v2/https'; @@ -68,7 +68,7 @@ export const onCreatePasslistEntry = onDocumentCreated( onCreatePasslistEntryHandler ); -export const importGeoJson = onHttpsRequestAsync(importGeoJsonCallback); +export const importGeoJson = onHttpsRequest(importGeoJsonHandler); export const exportCsv = onHttpsRequest(exportCsvHandler); diff --git a/functions/src/testing/http-test-helpers.ts b/functions/src/testing/http-test-helpers.ts index b9a840f1f..4c7fb378a 100644 --- a/functions/src/testing/http-test-helpers.ts +++ b/functions/src/testing/http-test-helpers.ts @@ -44,6 +44,7 @@ export function createResponseSpy(chunks?: string[]): Response { const res = jasmine.createSpyObj>('response', [ 'send', 'status', + 'json', 'end', 'write', 'type', @@ -54,6 +55,7 @@ export function createResponseSpy(chunks?: string[]): Response { 'removeListener', ]); res.status.and.callThrough().and.returnValue(res); + res.json.and.callThrough().and.returnValue(res); res.end.and.callThrough().and.returnValue(res); res.write.and.callFake((chunk: any): boolean => { chunks?.push(chunk.toString());