fix(firestore): convert app-shaped timestamps and geopoints on admin writes - #19
Closed
EMTumariscal wants to merge 1 commit into
Closed
EMTumariscal wants to merge 1 commit into
EMTumariscal wants to merge 1 commit into
Conversation
10 tasks
Contributor
Author
|
Superseded by #22 — consolidated PR from EMTumariscal:develop to master bundling this change with its original description preserved. Closing per maintainer request. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes a data-corruption bug on the firebase-admin (service-account) write path: updating any document that contains Firestore
Timestamp(orGeoPoint) fields rewrote those fields as plain map objects instead of FirestoreTimestamp/GeoPointvalues.The renderer represents a Firestore
Timestampas a plain{ _seconds, _nanoseconds }object and aGeoPointas{ _latitude, _longitude }(decoded inelectron/utils/firestoreHelpers.js). The Google/OAuth REST write path converts those shapes back throughconvertToFirestoreValue, but the Admin SDK path passed the raw objects straight toset()/update()/batch.set(). The Admin SDK does not recognize them, so it persisted amapValue. Because the UI sends the whole document on save ({ ...docData, [field]: newValue }), every update degraded all existing timestamp/geopoint fields of the document.Reproduction
Timestampfield (created elsewhere, e.g. the Firebase console).timestamp.Fix
toFirestoreAdminValue(value, { Timestamp, GeoPoint, FieldValue })toelectron/utils/firestoreHelpers.js: a recursive, non-mutating converter that mapsDate→Timestamp,{ _seconds, _nanoseconds }/{ seconds, nanoseconds | nanos }→Timestamp,{ _latitude, _longitude }/{ latitude, longitude }→GeoPoint, recurses through arrays and plain maps, passes primitives through, and preservesFieldValuesentinels. Constructors are injected so the module stays free of afirebase-adminimport.electron/controllers/firestoreController.js:firestore:createDocument,firestore:setDocument,firestore:updateDocument,firestore:importDocuments.convertToFirestoreValue) and all read paths are unchanged.Type of Change
AI Code Generation
This change was implemented by an AI coding agent (OpenCode) under human direction. A human defined the task, reviewed the diff, and authorized the commit and this PR. AI was used for the root-cause investigation, the code changes, and the tests. The AI disclosure is provided here in full.
Related Issue
No existing upstream issue was found for this defect (searched open and closed issues for
timestamp,Timestamp object,seconds nanos,update document,firestore timestamp). The reproduction steps are included in the Description above.Technical Context
{ _seconds, _nanoseconds }, produced byfirestoreDocumentToData/parseFirestoreValueinelectron/utils/firestoreHelpers.js.firebase-adminv14,@google-cloud/firestorev8) serializes an unrecognized plain object as amapValue; only a realTimestamp/GeoPointinstance is written as a Firestoretimestamp/geoPoint.Test Environment
opencode-go/deepseek-v4.1-flash).Evidence
pnpm run test(vitest):New/updated coverage:
electron/utils/firestoreHelpers.test.js(new):Date, both timestamp shapes, both geopoint shapes, nested array/map, primitive passthrough,FieldValuesentinel preservation, non-mutation.electron/controllers/firestoreController.test.js:createDocument,setDocument,updateDocument, andimportDocumentsconvert app-shaped values before writing, and the incoming payload is not mutated.Checklist
Makefile/make check; the equivalent is the.github/workflows/ci.ymlsteps, all of which pass:pnpm run lint,pnpm run format:check,pnpm run typecheck,pnpm run test.