fix(firestore): accept pipeline references from the same client before initialization - #9196
Open
Om-singhaI wants to merge 1 commit into
Open
Conversation
…e initialization Passing a DocumentReference or CollectionReference to pipeline().documents() or pipeline().collection() threw "INTERNAL ERROR: Client is not yet ready to issue requests." whenever the Firestore instance had no explicit project ID and had not yet issued a request. PipelineSource._validateReference compared reference.firestore.formattedName with the pipeline's own formattedName, and that getter reads the projectId getter, which throws until initializeIfNeeded() has detected the project. Pipeline construction is synchronous and happens before any request, so a fresh client always hit this path. Passing the same location as a string path worked because that code never reads the project ID. A reference created by the same Firestore instance necessarily targets the same database, so _validateReference now returns early when reference.firestore is the pipeline's own instance and only compares formatted names for references that come from a different instance. The existing cross database check is unchanged. Adds unit tests that build and execute a pipeline from a DocumentReference and from a CollectionReference on a client whose project ID is only detected on the first request, plus tests that references targeting a different database are still rejected. Fixes googleapis#9186
Contributor
There was a problem hiding this comment.
Code Review
This pull request modifies the PipelineSource reference validation to bypass database comparison when the reference's Firestore instance is identical to the pipeline's instance, preventing errors when the project ID is not yet detected. It also adds corresponding unit tests. The feedback suggests changing the return statement from 'return true;' to a simple 'return;' to maintain consistency with the validation method's implicit void return type.
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.
fix(firestore): accept pipeline references from the same client before initialization
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #9186 🦕
What was wrong
With a client that has no explicit
projectId(the common case on Cloud Run, Cloud Functions, GCE and anywhere else the project is picked up from the environment), this throws before any request is made:PipelineSource.documents()andPipelineSource.collection()call_validateReference()for everyDocumentReferenceorCollectionReferencethey are given. That method comparesreference.firestore.formattedNameagainstthis.db.formattedName.Firestore.formattedNamereads theprojectIdgetter, and that getter throws untilinitializeIfNeeded()has detected the project, which only happens when the first request is issued. Pipeline construction is synchronous and happens before any request, so a fresh client always hit this path. The same pipeline built from the string path (documents(['users/user1'])) worked becausethis.db.doc(path)never touches the project ID. The stack trace in the issue (get projectId->get formattedName->_validateReference->documents) is exactly this.The fix
_validateReferencenow returns early whenreference.firestore === this.db. A reference created by the pipeline's ownFirestoreinstance necessarily targets the same database, so there is nothing to compare and no reason to read the project ID. References that come from a differentFirestoreinstance still go through the existingformattedNamecomparison, so the cross database error message and behaviour are unchanged.The change is nine lines in
dev/src/pipelines/pipelines.tsand does not touchdocuments(),collection()or any stage, so it should not conflict with #9118.A note on scope: if a reference comes from a different
Firestoreinstance and either instance has not detected its project ID yet, the comparison still throws the sameINTERNAL ERRORas before. Handling that would mean either comparing the private_projectIdfields (which can both beundefinedbefore detection, so there is nothing meaningful to compare) or deferring the check to execution time afterinitializeIfNeeded(). I kept this PR to the case from the issue, which is also the overwhelmingly common one, and would be glad to follow up on the multi instance case if you want it handled.Tests
New
describe('PipelineSource reference validation')block indev/test/pipelines/pipeline.ts:accepts a DocumentReference before the project ID is detected: builds the client withcreateInstance(..., {projectId: undefined})and agetProjectIdoverride, so the project ID is unknown until the first request. Buildspipeline().documents([firestore.doc('foo/bar')]), executes it, and asserts theExecutePipelinerequest targetsprojects/detected-project/databases/(default)with adocumentsstage whose argument is/foo/bar.accepts a CollectionReference before the project ID is detected: same setup withpipeline().collection(firestore.collection('foo')).rejects a DocumentReference that targets a different databaseandrejects a CollectionReference that targets a different database: a reference from a second client withdatabaseId: 'other-db'still throws the existing "does not match the database name" error. These pin the behaviour the early return must not change.Verification
All runs were in
handwritten/firestorewithnpm run compilefollowed bymocha build/test/pipelines/pipeline.js(the same filenpm testruns under c8). The unit tests use the existingcreateInstancefake client and need no GCP credentials.main(48e0941): a script mirroring the gist from the issue printedINTERNAL ERROR: Client is not yet ready to issue requests.for bothdocuments([firestore.doc(...)])andcollection(firestore.collection(...)), with the sameget projectId->get formattedName->_validateReferencestack as the report, while the string path forms succeeded.main(test file only changed): 6 passing, 2 failing. The two "accepts" tests fail withError: INTERNAL ERROR: Client is not yet ready to issue requests.; the two "rejects" tests pass onmainas expected.OKfor all four forms.npm run test-only, which isc8 mocha build/test): 696 passing, 47 pending, 0 failing. The 47 pending tests are pre existing skipped tests; none are in the files this PR touches.mocha build/conformance --exit): 228 passing, 0 failing. Without--exitthe conformance process prints the same 228 passing summary and then stays alive on an open handle; it does exactly the same with the sources frommain, so that is pre existing and unrelated to this change.Coverage: the only new source lines are the early return and its condition, and both branches are exercised by the new tests (same instance returns early, different instance falls through to the existing comparison), so line and branch coverage of
pipelines.tsgo up.Lint
npx prettier --config .prettierrc.js --checkpasses on both changed files. ESLint passes on both files with the package'sgtsconfig,plugin:prettier/recommendedand the Firestore specific overrides copied from the root.eslintrc.json(explicit return types,no-console,no-restricted-propertiesfor.only,no-floating-promises). I ran it this way because I installed only the Firestore package rather than the monorepo root; the rooteslint-plugin-importandeslint-plugin-promiserule sets were therefore not run locally, but the change adds no imports and the new tests useasync/awaitin the same style as the surrounding tests.