diff --git a/functions/src/common/datastore.ts b/functions/src/common/datastore.ts index f7612930f..063009caa 100644 --- a/functions/src/common/datastore.ts +++ b/functions/src/common/datastore.ts @@ -227,6 +227,18 @@ export class Datastore { await this.db_.doc(survey(surveyId)).collection('lois').add(loiDoc); } + async insertLocationsOfInterest( + surveyId: string, + loiDocs: DocumentData[] + ): Promise { + const bulkWriter = this.db_.bulkWriter(); + const collectionRef = this.db_.collection(lois(surveyId)); + for (const loiDoc of loiDocs) { + bulkWriter.create(collectionRef.doc(), loiDoc); + } + await bulkWriter.close(); + } + async countSubmissionsForLoi( surveyId: string, loiId: string diff --git a/functions/src/handlers.ts b/functions/src/handlers.ts index ef32684a6..ff95dd7f5 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, 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'; @@ -159,8 +159,11 @@ function invokeCallback( * 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) => +export function onHttpsRequestAsync( + callback: HttpsRequestCallback, + options: HttpsOptions = {} +) { + return onRequest(options, (req: Request, res: Response) => corsMiddleware(req, res, () => cookieParser()( req as any, diff --git a/functions/src/import-geojson.spec.ts b/functions/src/import-geojson.spec.ts index 6c5f28317..b656babcb 100644 --- a/functions/src/import-geojson.spec.ts +++ b/functions/src/import-geojson.spec.ts @@ -231,22 +231,22 @@ describe('importGeoJson()', () => { }, ]; + let insertLocationsOfInterestSpy: jasmine.Spy; + beforeEach(() => { mockFirestore = createMockFirestore(); stubAdminApi(mockFirestore); + const db = getDatastore(); + insertLocationsOfInterestSpy = spyOn( + db, + 'insertLocationsOfInterest' + ).and.returnValue(Promise.resolve()); }); afterEach(() => { resetDatastore(); }); - async function loiData(surveyId: string) { - const lois = await mockFirestore - .collection(`surveys/${surveyId}/lois`) - .get(); - return lois.docs.map(doc => doc.data()); - } - function createPostData(surveyId: string, jobId: string, geoJson: object) { const form = new FormData(); form.append('survey', surveyId); @@ -255,35 +255,76 @@ describe('importGeoJson()', () => { return form; } + async function runImport(geoJson: object) { + const req = await createPostRequestSpy( + { url: '/importGeoJson' }, + createPostData(surveyId, jobId, geoJson) + ); + const res = createResponseSpy(); + try { + // 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); + } + return res; + } + testCases.forEach(({ desc, input, expectedStatus, expected }) => it(desc, async () => { // Add survey. mockFirestore.doc(`surveys/${surveyId}`).set(survey); - // Build mock request and response. - const req = await createPostRequestSpy( - { url: '/importGeoJson' }, - createPostData(surveyId, jobId, input) - ); - 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); - } + const res = await runImport(input); - // Check post-conditions. expect(res.status).toHaveBeenCalledOnceWith(expectedStatus); - expect(await loiData(surveyId)).toEqual(expected); + if (expectedStatus === StatusCodes.OK) { + expect(insertLocationsOfInterestSpy).toHaveBeenCalledOnceWith( + surveyId, + expected + ); + } else { + expect(insertLocationsOfInterestSpy).not.toHaveBeenCalled(); + } }) ); + it('bulk-inserts all features in a single call', async () => { + mockFirestore.doc(`surveys/${surveyId}`).set(survey); + const geoJsonWithMultiplePoints = { + type: 'FeatureCollection', + features: [ + { + type: 'Feature', + geometry: { type: 'Point', coordinates: [125.6, 10.1] }, + properties: {}, + }, + { + type: 'Feature', + geometry: { type: 'Point', coordinates: [126.0, 11.0] }, + properties: {}, + }, + { + type: 'Feature', + geometry: { type: 'Point', coordinates: [127.0, 12.0] }, + properties: {}, + }, + ], + }; + + await runImport(geoJsonWithMultiplePoints); + + // All features must be passed to a single bulk insert call, not one call per feature. + expect(insertLocationsOfInterestSpy).toHaveBeenCalledTimes(1); + const [calledSurveyId, calledDocs] = + insertLocationsOfInterestSpy.calls.mostRecent().args; + expect(calledSurveyId).toBe(surveyId); + expect(calledDocs.length).toBe(3); + }); + it('surfaces errors thrown during async file processing', async () => { // Make fetchSurvey reject to simulate an unexpected async error in the file handler. const db = getDatastore(); diff --git a/functions/src/import-geojson.ts b/functions/src/import-geojson.ts index 21e2debd3..7816190ca 100644 --- a/functions/src/import-geojson.ts +++ b/functions/src/import-geojson.ts @@ -22,6 +22,7 @@ import Busboy from 'busboy'; import JSONStream from 'jsonstream-ts'; import { canImport } from './common/auth'; import { DecodedIdToken } from 'firebase-admin/auth'; +import { DocumentData } from 'firebase-admin/firestore'; import { GroundProtos } from '@ground/proto'; import { isGeometryValid, toDocumentData, toGeometryPb } from '@ground/lib'; import { Feature, GeoJsonProperties } from 'geojson'; @@ -58,9 +59,8 @@ export function importGeoJsonCallback( // Dictionary used to accumulate task step values, keyed by step name. const params: { [name: string]: string } = {}; - // Accumulate Promises for insert operations, so we don't finalize the res - // stream before operations are complete. - const inserts: any[] = []; + // Accumulate LOI documents to be bulk-inserted once the file is fully parsed. + const loiDocs: DocumentData[] = []; const db = getDatastore(); @@ -119,8 +119,8 @@ export function importGeoJsonCallback( busboy.on('finish', async () => { if (hasError) return; try { - await Promise.all(inserts); - const count = inserts.length; + await db.insertLocationsOfInterest(params.survey, loiDocs); + const count = loiDocs.length; console.debug(`${count} LOIs imported`); res.send(JSON.stringify({ count })); done(); @@ -207,10 +207,9 @@ export function importGeoJsonCallback( return; } try { - const loi = toDocumentData( - toLoiPb(geoJsonFeature as Feature, jobId, ownerId) + loiDocs.push( + toDocumentData(toLoiPb(geoJsonFeature as Feature, jobId, ownerId)) ); - inserts.push(db.insertLocationOfInterest(surveyId, loi)); } catch (loiErr) { console.debug('Skipping LOI', loiErr); } diff --git a/functions/src/index.ts b/functions/src/index.ts index fdb52cbff..75b966178 100644 --- a/functions/src/index.ts +++ b/functions/src/index.ts @@ -68,7 +68,11 @@ export const onCreatePasslistEntry = onDocumentCreated( onCreatePasslistEntryHandler ); -export const importGeoJson = onHttpsRequestAsync(importGeoJsonCallback); +export const importGeoJson = onHttpsRequestAsync(importGeoJsonCallback, { + memory: '4GiB', + timeoutSeconds: 3600, + cpu: 2, +}); export const exportCsv = onHttpsRequest(exportCsvHandler, { memory: '4GiB', diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 4dc1cf73a..6b9c628c5 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -88,8 +88,8 @@ catalogs: specifier: ^12.1.0 version: 12.1.0 firebase-functions: - specifier: ^7.2.2 - version: 7.2.3 + specifier: ^7.2.5 + version: 7.2.5 google-auth-library: specifier: ^6.1.3 version: 6.1.3 @@ -463,7 +463,7 @@ importers: version: 12.1.0(encoding@0.1.13) firebase-functions: specifier: 'catalog:' - version: 7.2.3(firebase-admin@12.1.0(encoding@0.1.13)) + version: 7.2.5(firebase-admin@12.1.0(encoding@0.1.13)) google-auth-library: specifier: 'catalog:' version: 6.1.3(encoding@0.1.13) @@ -521,7 +521,7 @@ importers: version: 2.0.0 firebase-functions-test: specifier: catalog:dev - version: 3.4.1(firebase-admin@12.1.0(encoding@0.1.13))(firebase-functions@7.2.3(firebase-admin@12.1.0(encoding@0.1.13)))(jest@30.2.0(@types/node@20.19.30)(babel-plugin-macros@3.1.0)(ts-node@10.9.2(@types/node@20.19.30)(typescript@5.9.3))) + version: 3.4.1(firebase-admin@12.1.0(encoding@0.1.13))(firebase-functions@7.2.5(firebase-admin@12.1.0(encoding@0.1.13)))(jest@30.2.0(@types/node@20.19.30)(babel-plugin-macros@3.1.0)(ts-node@10.9.2(@types/node@20.19.30)(typescript@5.9.3))) form-data-encoder: specifier: catalog:dev version: 4.1.0 @@ -542,7 +542,7 @@ importers: version: 12.1.0(encoding@0.1.13) firebase-functions: specifier: 'catalog:' - version: 7.2.3(firebase-admin@12.1.0(encoding@0.1.13)) + version: 7.2.5(firebase-admin@12.1.0(encoding@0.1.13)) protobufjs: specifier: 'catalog:' version: 7.5.4 @@ -6932,8 +6932,8 @@ packages: firebase-functions: '>=4.9.0' jest: '>=28.0.0' - firebase-functions@7.2.3: - resolution: {integrity: sha512-myrnxuvmfAQYsaat2Crv1k2qpb2BuRUVRfr1V/nVLNO4sObgd06NT08TsVF3PA6EkxyUsOKXmcvmGFiJC0R2fQ==} + firebase-functions@7.2.5: + resolution: {integrity: sha512-K+pP0AknluAguLRbD96hibyXbnOgwnvd4hkExWdGrxnNCLoj8LBFj08uvJYxyvhsCgYzQumrUaHBW4lsXKSiRg==} engines: {node: '>=18.0.0'} hasBin: true peerDependencies: @@ -20642,16 +20642,16 @@ snapshots: - encoding - supports-color - firebase-functions-test@3.4.1(firebase-admin@12.1.0(encoding@0.1.13))(firebase-functions@7.2.3(firebase-admin@12.1.0(encoding@0.1.13)))(jest@30.2.0(@types/node@20.19.30)(babel-plugin-macros@3.1.0)(ts-node@10.9.2(@types/node@20.19.30)(typescript@5.9.3))): + firebase-functions-test@3.4.1(firebase-admin@12.1.0(encoding@0.1.13))(firebase-functions@7.2.5(firebase-admin@12.1.0(encoding@0.1.13)))(jest@30.2.0(@types/node@20.19.30)(babel-plugin-macros@3.1.0)(ts-node@10.9.2(@types/node@20.19.30)(typescript@5.9.3))): dependencies: '@types/lodash': 4.17.23 firebase-admin: 12.1.0(encoding@0.1.13) - firebase-functions: 7.2.3(firebase-admin@12.1.0(encoding@0.1.13)) + firebase-functions: 7.2.5(firebase-admin@12.1.0(encoding@0.1.13)) jest: 30.2.0(@types/node@20.19.30)(babel-plugin-macros@3.1.0)(ts-node@10.9.2(@types/node@20.19.30)(typescript@5.9.3)) lodash: 4.17.23 ts-deepmerge: 2.0.7 - firebase-functions@7.2.3(firebase-admin@12.1.0(encoding@0.1.13)): + firebase-functions@7.2.5(firebase-admin@12.1.0(encoding@0.1.13)): dependencies: '@types/cors': 2.8.17 '@types/express': 4.17.25 diff --git a/pnpm-workspace.yaml b/pnpm-workspace.yaml index 99a3f0924..ca3625791 100644 --- a/pnpm-workspace.yaml +++ b/pnpm-workspace.yaml @@ -34,7 +34,7 @@ catalogs: cors: ^2.8.5 firebase: ^10.12.2 firebase-admin: ^12.1.0 - firebase-functions: ^7.2.2 + firebase-functions: ^7.2.5 google-auth-library: ^6.1.3 googleapis: ^64.0.0 http-status-codes: ^2.3.0