Emit OK HTTP status in import callback instead of handler

functions/src/export-csv.ts

@@ -128,7 +128,6 @@ export async function exportCsvHandler(
     }
   }
 
-  res.status(StatusCodes.OK);
   csvStream.end();
 }
 

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',

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');
 

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<any>,
-  user: DecodedIdToken,
-  done: () => void,
-  error: ErrorHandler
-) => void;
-
-export async function invokeCallbackAsync(
-  callback: HttpsRequestCallback,
-  req: Request,
-  res: Response<any>,
-  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<any>,
-  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);
-          })
-      )
-    )
-  );
-}

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);
     })
   );

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<void>((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