fix(functions): isolate SourceTokenScraper per codebase#10374
fix(functions): isolate SourceTokenScraper per codebase#10374oudmane wants to merge 1 commit intofirebase:mainfrom
Conversation
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request updates the Fabricator to use separate SourceTokenScraper instances for different codebases and adds a unit test to verify this behavior. The review feedback suggests refactoring the codebase identification and scraper initialization logic using optional chaining and nullish coalescing to reduce boilerplate and nesting, as recommended by the repository's style guide.
| let codebase = "default"; | ||
| if (changes.endpointsToCreate.length > 0) { | ||
| codebase = changes.endpointsToCreate[0].codebase || "default"; | ||
| } else if (changes.endpointsToUpdate.length > 0) { | ||
| codebase = changes.endpointsToUpdate[0].endpoint.codebase || "default"; | ||
| } else if (changes.endpointsToDelete.length > 0) { | ||
| codebase = changes.endpointsToDelete[0].codebase || "default"; | ||
| } else if (changes.endpointsToSkip.length > 0) { | ||
| codebase = changes.endpointsToSkip[0].codebase || "default"; | ||
| } | ||
|
|
||
| let scraperV1 = scrapersV1.get(codebase); | ||
| if (!scraperV1) { | ||
| scraperV1 = new SourceTokenScraper(); | ||
| scrapersV1.set(codebase, scraperV1); | ||
| } | ||
|
|
||
| let scraperV2 = scrapersV2.get(codebase); | ||
| if (!scraperV2) { | ||
| scraperV2 = new SourceTokenScraper(); | ||
| scrapersV2.set(codebase, scraperV2); | ||
| } | ||
|
|
||
| return this.applyUpserts(changes, scraperV1!, scraperV2!); |
There was a problem hiding this comment.
The logic for extracting the codebase and initializing the scrapers can be significantly simplified. Using optional chaining and the nullish coalescing operator reduces boilerplate and eliminates the need for non-null assertions (!), making the code more maintainable and readable.
const codebase =
changes.endpointsToCreate[0]?.codebase ||
changes.endpointsToUpdate[0]?.endpoint.codebase ||
changes.endpointsToDelete[0]?.codebase ||
changes.endpointsToSkip[0]?.codebase ||
"default";
const scraperV1 = scrapersV1.get(codebase) ?? new SourceTokenScraper();
scrapersV1.set(codebase, scraperV1);
const scraperV2 = scrapersV2.get(codebase) ?? new SourceTokenScraper();
scrapersV2.set(codebase, scraperV2);
return this.applyUpserts(changes, scraperV1, scraperV2);References
- The repository style guide recommends reducing nesting as much as possible and folding edge cases into the general case to improve clarity. (link)
Description
Fixes a regression introduced in v15.15.0 (likely via the
Fabricatorrefactor in #10293) whereSourceTokenScraperinstances were moved from a per-changeset scope to a plan-wide scope.In projects utilizing multiple
codebasedefinitions (common in monorepos), each codebase has its own isolated source bundle and ZIP file. Because theSourceTokenScraperis stateful and captures optimization tokens from GCF deployment operations, sharing a single scraper across the entire deployment plan caused different codebases to "leak" source tokens to one another.When Codebase B incorrectly uses a source token intended for Codebase A, Google Cloud reuses the processed module/container from Codebase A. This results in Codebase B being deployed with the wrong code, leading to runtime failures:
Function '...' is not defined in the provided module.This PR restores isolation by maintaining a registry of scrapers keyed by
codebasewithin theFabricator.applyPlanmethod. This ensures that tokens are only shared among functions that actually share the same source (same codebase across different regions), while keeping independent codebases strictly isolated.Scenarios Tested
us-central1andeurope-west1) still correctly share the same scraper instance to maintain existing deployment performance optimizations.Sample Commands
Standard deployment command in a monorepo setup:
firebase deploy --only functionsExample
firebase.jsonstructure where this fix is critical:{ "functions": [ { "codebase": "app-api", "source": "packages/api/dist" }, { "codebase": "app-auth", "source": "packages/auth/dist" } ] }