feat: add firebase.json schema support and validation for function kits#10842
feat: add firebase.json schema support and validation for function kits#10842wandamora wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for "functions kits" by adding the KitFunctionConfig schema, TypeScript types, validation logic, and corresponding unit tests. The review feedback identifies several critical issues and improvement opportunities: a missing runtime property in ValidatedKitSingle that causes TypeScript compilation errors, a regression in isLocalConfig that breaks kit deployment via requireLocal, a JSON schema generation issue when using Record<string, string> for instances, a validation bypass when kit is an empty string, and a lack of naming constraint validation for kit instance IDs.
| export function isLocalConfig(c: ValidatedSingle): c is ValidatedLocalSingle { | ||
| return (c as ValidatedLocalSingle).source !== undefined; | ||
| return "source" in c && !("kit" in c); | ||
| } |
There was a problem hiding this comment.
Changing isLocalConfig to explicitly exclude kit configs (!("kit" in c)) will cause requireLocal to throw a FirebaseError ("Remote sources are not supported") when a kit config is passed. Since prepare.ts calls requireLocal during deployment, deploying any functions kit will fail.
To fix this, requireLocal should be updated to allow both ValidatedLocalSingle and ValidatedKitSingle by checking !isRemoteConfig(c) instead of isLocalConfig(c):
export function requireLocal(c: ValidatedSingle, purpose?: string): ValidatedLocalSingle | ValidatedKitSingle {
if (isRemoteConfig(c)) {
const msg =
purpose ??
"This operation requires a local functions source directory, but the codebase is configured with a remote source.";
throw new FirebaseError(msg);
}
return c;
}| /** Dictionary mapping instance IDs to their configuration directories */ | ||
| instances?: Record<string, string>; |
There was a problem hiding this comment.
Using Record<string, string> for instances causes the auto-generated JSON schema to define instances with "additionalProperties": false and no explicit properties, which effectively rejects any non-empty configuration dictionary.
To fix this and ensure the generated JSON schema correctly allows arbitrary string properties, use an index signature { [instanceId: string]: string } instead.
| /** Dictionary mapping instance IDs to their configuration directories */ | |
| instances?: Record<string, string>; | |
| /** Dictionary mapping instance IDs to their configuration directories */ | |
| instances?: { [instanceId: string]: string }; |
| if ("kit" in config && config.kit) { | ||
| if (config.codebase) { | ||
| throw new FirebaseError( | ||
| "Cannot specify both 'kit' and 'codebase' in a single functions config.", | ||
| ); | ||
| } | ||
| if (config.remoteSource) { | ||
| throw new FirebaseError( | ||
| "Cannot specify both 'kit' and 'remoteSource' in a single functions config.", | ||
| ); | ||
| } | ||
| if ((config as { prefix?: string }).prefix) { | ||
| throw new FirebaseError("Cannot specify 'prefix' in a functions kit config."); | ||
| } | ||
| if (!config.source) { | ||
| throw new FirebaseError("Must specify 'source' in a functions kit config."); | ||
| } | ||
| return config as ValidatedKitSingle; | ||
| } |
There was a problem hiding this comment.
Checking "kit" in config && config.kit means that if a user specifies an empty string for "kit" (e.g., "kit": ""), the validation block will be skipped and the config will be treated as a local config. This leads to type-guard mismatches later (e.g., isKitConfig will return true because "kit" in c is still true).
Instead, check "kit" in config to identify it as a kit config, and then explicitly throw an error if config.kit is empty. Additionally, the cast (config as { prefix?: string }).prefix is unnecessary since prefix is a common property of the union.
if ("kit" in config) {
if (!config.kit) {
throw new FirebaseError("Must specify 'kit' name in a functions kit config.");
}
if (config.codebase) {
throw new FirebaseError(
"Cannot specify both 'kit' and 'codebase' in a single functions config.",
);
}
if (config.remoteSource) {
throw new FirebaseError(
"Cannot specify both 'kit' and 'remoteSource' in a single functions config.",
);
}
if (config.prefix) {
throw new FirebaseError("Cannot specify 'prefix' in a functions kit config.");
}
if (!config.source) {
throw new FirebaseError("Must specify 'source' in a functions kit config.");
}
return config as ValidatedKitSingle;
}| if ("instances" in c && c.instances) { | ||
| for (const instanceId of Object.keys(c.instances)) { | ||
| if (instanceIds.has(instanceId)) { | ||
| throw new FirebaseError( | ||
| `functions kit instance ID must be unique across all kits, but '${instanceId}' was used more than once.`, | ||
| ); | ||
| } | ||
| instanceIds.add(instanceId); | ||
| } | ||
| } |
There was a problem hiding this comment.
Kit instance IDs are used as codebase identifiers for the deployed functions of that kit instance. Therefore, they must adhere to the same naming constraints as codebase names (less than 64 characters, lowercase letters, numbers, underscores, and dashes).
Without validation, invalid instance IDs (e.g., containing uppercase letters or spaces) will pass configuration validation and fail much later during deployment.
if ("instances" in c && c.instances) {
for (const instanceId of Object.keys(c.instances)) {
if (instanceId.length === 0 || instanceId.length > 63 || !/^[a-z0-9_-]+$/.test(instanceId)) {
throw new FirebaseError(
`Invalid kit instance ID '${instanceId}'. Kit instance IDs must be less than 64 characters and can contain only lowercase letters, numeric characters, underscores, and dashes.`,
);
}
if (instanceIds.has(instanceId)) {
throw new FirebaseError(
`functions kit instance ID must be unique across all kits, but '${instanceId}' was used more than once.`,
);
}
instanceIds.add(instanceId);
}
}Adds support for Function Kits schema in `firebase.json` (`kit` stanza as a peer to `codebase`). This includes: - Added `KitSourcePackage` and `KitFunctionConfig` types to `src/firebaseConfig.ts`. - Updated `FunctionConfig` union to include `KitFunctionConfig`. - Added `ValidatedKitSingle` and validation logic in `src/functions/projectConfig.ts` to enforce: - Mutual exclusivity between `kit` and `codebase`/`remoteSource`/`prefix`. - Project-wide kit name and kit instance ID uniqueness across kit stanzas. - Mutual exclusivity between codebase names and kit instance IDs. - Updated `isLocalConfig` and added `isKitConfig` type guards. - Regenerated JSON schema (`schema/firebase-config.json`). - Validated parsing of valid Function Kit configurations. - Verified validation failures when `kit` is combined with `codebase`, `remoteSource`, or `prefix`. - Verified validation failure when `kit` is missing required `source`. - Verified duplicate `kit` name rejection across stanzas. - Verified duplicate kit instance ID rejection across stanzas. - Verified mutual exclusivity error when a codebase name matches a kit instance ID. - Verified type guard narrowing for `isLocalConfig`, `isRemoteConfig`, and `isKitConfig`. - `npm run generate:json-schema` - `npm run lint:changed-files` - `npx mocha src/functions/projectConfig.spec.ts`
a6f6df7 to
ad01fb4
Compare
This change sets instances as codebases in the backend, and adds a new "kit" identifier to further differentiate standard function codebases with function kits.
If a user specifies a target to deploy that a kit and instance share a name for, confirm with the user if they'd like to deploy the full kit or indiviual instance. The prompt will only appear for kits with more than one instance.
We decided that introducing "kits" filtering overcomplicates the parser.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for 'Functions Kits' under a new experiment flag, updating the configuration schema, validation logic, and deployment helpers to handle kit configurations and instances. The reviewer feedback highlights a critical issue where excluding kits from isLocalConfig breaks deployment via requireLocal, and suggests stricter validation for kit properties and instance IDs. Additionally, the reviewer recommends replacing unsafe type assertions (as any and as unknown as) in the test files with safer alternatives to comply with the repository's style guide.
| export function isLocalConfig(c: ValidatedSingle): c is ValidatedLocalSingle { | ||
| return (c as ValidatedLocalSingle).source !== undefined; | ||
| return "source" in c && !("kit" in c); | ||
| } |
There was a problem hiding this comment.
Excluding 'kit' from isLocalConfig causes requireLocal (which relies on isLocalConfig) to throw a FirebaseError for all functions kit configurations during deployment. Since kit configurations do have a local source directory, they should be supported by requireLocal.
To fix this, you should update requireLocal to check !isRemoteConfig(c) instead of isLocalConfig(c), and update its return type to ValidatedLocalSingle | ValidatedKitSingle.
| if (!config.source) { | ||
| throw new FirebaseError("Must specify 'source' in a functions kit config."); | ||
| } | ||
| return config as ValidatedKitSingle; |
There was a problem hiding this comment.
We should validate that kit is a string and instances (if provided) is a plain object. If instances is a string or array, it could bypass validation and cause runtime TypeErrors during deployment.
| if (!config.source) { | |
| throw new FirebaseError("Must specify 'source' in a functions kit config."); | |
| } | |
| return config as ValidatedKitSingle; | |
| if (!config.source) { | |
| throw new FirebaseError("Must specify 'source' in a functions kit config."); | |
| } | |
| if (typeof config.kit !== "string") { | |
| throw new FirebaseError("Must specify 'kit' as a string in a functions kit config."); | |
| } | |
| if (config.instances && (typeof config.instances !== "object" || Array.isArray(config.instances))) { | |
| throw new FirebaseError("Must specify 'instances' as an object in a functions kit config."); | |
| } | |
| return config as ValidatedKitSingle; |
| if ("instances" in c && c.instances) { | ||
| for (const instanceId of Object.keys(c.instances)) { | ||
| if (instanceIds.has(instanceId)) { | ||
| throw new FirebaseError( | ||
| `functions kit instance ID must be unique across all kits, but '${instanceId}' was used more than once.`, | ||
| ); | ||
| } | ||
| instanceIds.add(instanceId); | ||
| } |
There was a problem hiding this comment.
We should validate that each instanceId is a valid codebase name using validateCodebase(instanceId) since kit instance IDs act as codebase names. Additionally, we should ensure that the configuration directory values are strings.
if ("instances" in c && c.instances) {
for (const [instanceId, configDir] of Object.entries(c.instances)) {
validateCodebase(instanceId);
if (typeof configDir !== "string") {
throw new FirebaseError(
`functions kit instance '${instanceId}' configuration directory must be a string.`,
);
}
if (instanceIds.has(instanceId)) {
throw new FirebaseError(
`functions kit instance ID must be unique across all kits, but '${instanceId}' was used more than once.`,
);
}
instanceIds.add(instanceId);
}
}| const config = [ | ||
| { | ||
| kit: "my-kit", | ||
| source: "kits/my-kit", | ||
| instances: { "inst-1": "cfg1", "inst-2": "cfg2" }, | ||
| }, | ||
| ] as unknown as ValidatedConfig; |
There was a problem hiding this comment.
Avoid using as unknown as ValidatedConfig as an escape hatch. You can cast directly to ValidatedConfig.
| const config = [ | |
| { | |
| kit: "my-kit", | |
| source: "kits/my-kit", | |
| instances: { "inst-1": "cfg1", "inst-2": "cfg2" }, | |
| }, | |
| ] as unknown as ValidatedConfig; | |
| const config = [ | |
| { | |
| kit: "my-kit", | |
| source: "kits/my-kit", | |
| instances: { "inst-1": "cfg1", "inst-2": "cfg2" }, | |
| }, | |
| ] as ValidatedConfig; |
References
- Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards. (link)
| const kitConfig: ValidatedConfig = [ | ||
| { | ||
| kit: "my-kit", | ||
| source: "kits/my-kit", | ||
| instances: { "inst-1": "c1", "inst-2": "c2" }, | ||
| } as any, |
There was a problem hiding this comment.
Avoid using as any as an escape hatch. You can cast the element to ValidatedConfig[number] to maintain type safety.
| const kitConfig: ValidatedConfig = [ | |
| { | |
| kit: "my-kit", | |
| source: "kits/my-kit", | |
| instances: { "inst-1": "c1", "inst-2": "c2" }, | |
| } as any, | |
| const kitConfig: ValidatedConfig = [ | |
| { | |
| kit: "my-kit", | |
| source: "kits/my-kit", | |
| instances: { "inst-1": "c1", "inst-2": "c2" }, | |
| } as ValidatedConfig[number], |
References
- Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards. (link)
Description
Adds support for Function Kits schema in
firebase.json(kitstanza as a peer tocodebase). This includes:KitSourcePackageandKitFunctionConfigtypes tosrc/firebaseConfig.ts.FunctionConfigunion to includeKitFunctionConfig.ValidatedKitSingleand validation logic insrc/functions/projectConfig.tsto enforce:kitandcodebase/remoteSource/prefix.isLocalConfigand addedisKitConfigtype guards.schema/firebase-config.json).Scenarios Tested
Adds support for Function Kits schema in
firebase.json(kitstanza as a peer tocodebase). This includes:KitSourcePackageandKitFunctionConfigtypes tosrc/firebaseConfig.ts.FunctionConfigunion to includeKitFunctionConfig.ValidatedKitSingleand validation logic insrc/functions/projectConfig.tsto enforce:kitandcodebase/remoteSource/prefix.isLocalConfigand addedisKitConfigtype guards.schema/firebase-config.json).