fix(firestore-bigquery-export): fixed reference path #2437
Conversation
Fixed the Firestore doc reference path by passing the part after the "/documents/" only, which is what is needed for the doc function to work properly.
cabljac
requested review from
cabljac and
Copilot
and removed request for
cabljac
|
Hi, thanks for opening this! Really appreciate contributions. I'll review ASAP |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes how Firestore document reference paths are handled by stripping the full resource name down to the relative path required by firebase.firestore().doc.
- Adjusts
startAtto use only the path after/documents/ - Adjusts
endBeforesimilarly forendAt - Retains existing offset logic untouched
Comments suppressed due to low confidence (1)
firestore-bigquery-export/scripts/import/src/worker.ts:51
- There are no tests covering the case where the reference value includes the full resource name. Add unit tests for
processDocumentsto ensure both valid and malformedreferenceValueinputs are handled correctly.
if (serializableQuery.startAt?.values?.[0]?.referenceValue) {
| const relativePath = resourceName.split("/documents/")[1]; | ||
| query = query.startAt(firebase.firestore().doc(relativePath)); | ||
| } | ||
| if (serializableQuery.endAt?.values?.[0]?.referenceValue) { | ||
| const endPath = serializableQuery.endAt.values[0].referenceValue; | ||
| query = query.endBefore(firebase.firestore().doc(endPath)); | ||
| // similarly strip to relative path | ||
| const resourceName = serializableQuery.endAt.values[0].referenceValue; | ||
| const relativePath = resourceName.split("/documents/")[1]; |
There was a problem hiding this comment.
If resourceName does not contain /documents/, split("/documents/")[1] will be undefined, causing doc(undefined) to fail. Consider validating the split result or using a more robust extraction (e.g., const idx = resourceName.indexOf('/documents/'); if (idx === -1) throw ...; const relativePath = resourceName.substring(idx + 11);).
Copilot uses AI. Check for mistakes.
| // Apply partition boundaries from the serialized query | ||
| // These define the range of documents this worker should process | ||
| if (serializableQuery.startAt?.values?.[0]?.referenceValue) { | ||
| const startPath = serializableQuery.startAt.values[0].referenceValue; | ||
| query = query.startAt(firebase.firestore().doc(startPath)); | ||
| // strip off the full resource name, keep only the part after "/documents/" | ||
| const resourceName = serializableQuery.startAt.values[0].referenceValue; | ||
| const relativePath = resourceName.split("/documents/")[1]; | ||
| query = query.startAt(firebase.firestore().doc(relativePath)); | ||
| } | ||
| if (serializableQuery.endAt?.values?.[0]?.referenceValue) { | ||
| const endPath = serializableQuery.endAt.values[0].referenceValue; | ||
| query = query.endBefore(firebase.firestore().doc(endPath)); | ||
| // similarly strip to relative path | ||
| const resourceName = serializableQuery.endAt.values[0].referenceValue; | ||
| const relativePath = resourceName.split("/documents/")[1]; |
There was a problem hiding this comment.
[nitpick] The logic to strip the /documents/ prefix is duplicated for startAt and endAt. Extract this into a helper function (e.g. getRelativePath(resourceName)) to reduce repetition and improve clarity.
Copilot uses AI. Check for mistakes.
Fixed the Firestore doc reference path by passing the part after the "/documents/" only, which is what is needed for the doc function to work properly.
#2436