Emit OK HTTP status in import callback instead of handler

functions/src/export-csv.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 { SURVEY_ORGANIZER_ROLE } from './common/auth';
 import { getDatastore, resetDatastore } from './common/context';
 import { Firestore, QueryDocumentSnapshot } from 'firebase-admin/firestore';
@@ -409,7 +408,6 @@ describe('exportCsv()', () => {
         await exportCsvHandler(req, res, { email } as DecodedIdToken);
 
         // Check post-conditions.
-        expect(res.status).toHaveBeenCalledOnceWith(StatusCodes.OK);
         expect(res.type).toHaveBeenCalledOnceWith('text/csv');
         expect(res.setHeader).toHaveBeenCalledOnceWith(
           'Content-Disposition',

functions/src/export-csv.ts

@@ -142,7 +142,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, HttpsOptions, Request } from 'firebase-functions/v2/https';
+import { HttpsOptions, 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)
@@ -81,8 +81,8 @@ export function onHttpsRequest(
   return onRequest(options, (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 {
@@ -95,81 +95,3 @@ export function onHttpsRequest(
     )
   );
 }
-
-/** 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 { getDatastore, 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 (err) {
-        console.log(err);
-      }
+      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);
     })
   );
@@ -297,13 +291,7 @@ describe('importGeoJson()', () => {
     );
     const res = createResponseSpy();
 
-    try {
-      await invokeCallbackAsync(importGeoJsonCallback, req, res, {
-        email,
-      } as DecodedIdToken);
-    } catch {
-      // Expected to reject.
-    }
+    await importGeoJsonHandler(req, res, { email } as DecodedIdToken);
 
     expect(res.status).toHaveBeenCalledWith(StatusCodes.INTERNAL_SERVER_ERROR);
   });