fix: fallback to open rules when Storage emulator rules are missing - #10949
fix: fallback to open rules when Storage emulator rules are missing#10949christhompsongoogle wants to merge 1 commit into
Conversation
### Description Prevent missing configurations for the Storage emulator from crashing emulators:start and blocking the rest of the emulator suite: - Fall back to default open rules with a warning for Storage emulator when rules are unconfigured on non-demo projects. - Fall back to default open rules with a warning when storage target in firebase.json is not configured in .firebaserc. - Refactor duplicated warning logging logic into a helper function. Fixes b/546204399 ### Scenarios Tested - Unit tests in storage/rules/config.spec.ts for missing storage config, missing rules, and demo/non-demo projects. ### Sample Commands - firebase init emulators (select all emulators) - firebase emulators:start
|
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 Storage emulator to fall back to default open rules with a warning when rules or targets are unconfigured, rather than throwing an error. The review feedback correctly identifies that returning default rules immediately within the target loop will discard successfully parsed rules for other targets. The reviewer suggests using continue to skip unconfigured targets, returning default rules only if no targets were successfully configured, and adding a test case to verify this multi-target behavior.
| } | ||
| // Otherwise, requireTarget will error out | ||
| rc.requireTarget(projectId, "storage", targetConfig.target); | ||
| return defaultStorageRules(); |
There was a problem hiding this comment.
Returning defaultStorageRules() immediately here will discard any successfully parsed rules for other targets that were already processed and added to results. Instead, we should continue the loop to skip this unconfigured target, and only fall back to the default rules at the end of the function if no targets were successfully configured.
| return defaultStorageRules(); | |
| continue; |
References
- The style guide recommends using
continuestatements in loops to handle edge cases early and keep main logic flat. (link)
| @@ -79,6 +73,22 @@ export function getStorageRulesConfig( | |||
| return results; | |||
There was a problem hiding this comment.
If all targets were skipped because they were unconfigured (meaning results is empty), we should fall back to the default open rules. Otherwise, we should return the successfully parsed rules in results.
| return results; | |
| if (results.length === 0) { | |
| return defaultStorageRules(); | |
| } | |
| return results; |
| it("should use default config when target is missing in .firebaserc", () => { | ||
| const config = getOptions({ | ||
| data: { | ||
| storage: [{ target: "missing-target", rules: "main.rules" }], | ||
| }, | ||
| path: resolvePath, | ||
| }); | ||
| const result = getStorageRulesConfig(PROJECT_ID, config) as SourceFile; | ||
|
|
||
| expect(result.name).to.contain("templates/emulators/default_storage.rules"); | ||
| expect(result.content).to.contain("allow read, write;"); | ||
| }); |
There was a problem hiding this comment.
Let's add a test case to verify that when multiple targets are configured, but only some are missing in .firebaserc, the emulator successfully parses the configured targets and skips the missing ones instead of discarding all rules.
it("should use default config when target is missing in .firebaserc", () => {
const config = getOptions({
data: {
storage: [{ target: "missing-target", rules: "main.rules" }],
},
path: resolvePath,
});
const result = getStorageRulesConfig(PROJECT_ID, config) as SourceFile;
expect(result.name).to.contain("templates/emulators/default_storage.rules");
expect(result.content).to.contain("allow read, write;");
});
it("should parse configured targets and skip missing targets in .firebaserc", () => {
const mainRulesContent = Buffer.from(StorageRulesFiles.readWriteIfTrue.content);
const mainRulesPath = persistence.appendBytes("storage_main.rules", mainRulesContent);
const config = getOptions({
data: {
storage: [
{ target: "main", rules: mainRulesPath },
{ target: "missing-target", rules: "missing.rules" },
],
},
path: resolvePath,
});
config.rc.applyTarget(PROJECT_ID, "storage", "main", ["bucket_0"]);
const result = getStorageRulesConfig(PROJECT_ID, config) as RulesConfig[];
expect(result.length).to.equal(1);
expect(result[0].resource).to.eql("bucket_0");
expect(result[0].rules.name).to.equal(mainRulesPath);
});
Description
Prevent missing configurations for the Storage emulator from crashing emulators:start and blocking the rest of the emulator suite:
Fixes b/546204399
Scenarios Tested
Sample Commands