feat(firestore): Add support for 16MB documents#10078
Conversation
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request configures gRPC connections to support sending and receiving messages up to 17MB and introduces integration tests for handling large documents (up to 15.9MB). It also updates the default project ID resolution in tests. Feedback on these changes includes: (1) ensuring that emulator project ID resolution takes precedence over other environment variables to prevent breaking local emulator testing, (2) refactoring the oversized payload test to avoid catching the manually thrown error, which obscures the assertion failure, and (3) using a beforeEach hook to set the test timeout globally instead of duplicating it across every test case.
| export const DEFAULT_PROJECT_ID = | ||
| process.env.FIRESTORE_PROJECT_ID || | ||
| process.env.GCLOUD_PROJECT || | ||
| (USE_EMULATOR | ||
| ? process.env.FIRESTORE_EMULATOR_PROJECT_ID || 'test-emulator' | ||
| : PROJECT_CONFIG.projectId); |
There was a problem hiding this comment.
Prioritizing process.env.GCLOUD_PROJECT or process.env.FIRESTORE_PROJECT_ID over USE_EMULATOR will break local emulator testing if those environment variables are set in the developer's environment (which is very common when using the Google Cloud SDK). When USE_EMULATOR is true, we should always default to the emulator project ID.
export const DEFAULT_PROJECT_ID = USE_EMULATOR
? process.env.FIRESTORE_EMULATOR_PROJECT_ID || 'test-emulator'
: process.env.FIRESTORE_PROJECT_ID ||
process.env.GCLOUD_PROJECT ||
PROJECT_CONFIG.projectId;| try { | ||
| await setDoc(docRef, { chunk: largePayload }); | ||
| throw new Error( | ||
| 'Setting a document exceeding the 16MB limit should fail.' | ||
| ); | ||
| } catch (error: any) { | ||
| console.error('Caught error in oversized payloads:', error); | ||
| expect(error.code).to.equal('invalid-argument'); | ||
| } |
There was a problem hiding this comment.
If setDoc unexpectedly succeeds, the throw new Error(...) will be caught by the catch block, and the assertion expect(error.code).to.equal('invalid-argument') will fail with a confusing error message (expected undefined to equal 'invalid-argument') instead of the intended failure message. Capturing the error and asserting on it outside the try-catch block ensures a clear and correct failure message.
let error: any;
try {
await setDoc(docRef, { chunk: largePayload });
} catch (e) {
error = e;
}
expect(error, 'Setting a document exceeding the 16MB limit should fail.').to.exist;
expect(error.code).to.equal('invalid-argument');| before(function () { | ||
| this.timeout(120000); // Tests are very slow because large doc reads have very high latency. | ||
| const runLargeTests = process.env.FIRESTORE_RUN_LARGE_DOC_TESTS; | ||
| if (runLargeTests !== 'YES' && runLargeTests !== 'true') { | ||
| this.skip(); | ||
| } | ||
| if ( | ||
| getTargetBackend() !== TargetBackend.NIGHTLY || | ||
| !getRunEnterpriseTests() | ||
| ) { | ||
| this.skip(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Calling this.timeout(120000) inside the before hook only sets the timeout for the before hook itself, not for the individual it tests. This is why this.timeout(120000) had to be repeated in every single it block. Using a beforeEach hook to set the timeout for all tests in the suite allows removing the duplicate this.timeout(120000) calls from all it blocks.
beforeEach(function () {
this.timeout(120000); // Tests are very slow because large doc reads have very high latency.
});
before(function () {
const runLargeTests = process.env.FIRESTORE_RUN_LARGE_DOC_TESTS;
if (runLargeTests !== 'YES' && runLargeTests !== 'true') {
this.skip();
}
if (
getTargetBackend() !== TargetBackend.NIGHTLY ||
!getRunEnterpriseTests()
) {
this.skip();
}
});
| before(function () { | ||
| this.timeout(120000); // Tests are very slow because large doc reads have very high latency. | ||
| const runLargeTests = process.env.FIRESTORE_RUN_LARGE_DOC_TESTS; | ||
| if (runLargeTests !== 'YES' && runLargeTests !== 'true') { | ||
| this.skip(); | ||
| } | ||
| if ( | ||
| getTargetBackend() !== TargetBackend.NIGHTLY || | ||
| !getRunEnterpriseTests() | ||
| ) { | ||
| this.skip(); | ||
| } | ||
| }); |
2 participants
Adds support for 16 MB documents.
FIRESTORE_RUN_LARGE_DOC_TESTS = true. We don't want to run these tests in the default test suite since they are too slow.Notes:
We can merge these tests for now. A future task is to have these tests not rely on a seeded database, once large doc writes on watch are working.