fix: make Data Connect emulator startup and configuration errors non-blocking - #10950
fix: make Data Connect emulator startup and configuration errors non-blocking#10950christhompsongoogle wants to merge 1 commit into
Conversation
…blocking ### Description Prevent missing configurations and startup failures for the Data Connect emulator from crashing emulators:start and blocking the rest of the emulator suite: - Gracefully skip Data Connect emulator with an error and remediation instructions (firebase init dataconnect) in shouldStart when no services are configured in firebase.json. - Gracefully handle Data Connect emulator startup failure in startAll by cleaning up the failed instance and logging the error without crashing other emulators. Fixes b/546204399 ### Scenarios Tested - Unit tests in controller.spec.ts for Data Connect shouldStart handling. ### 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 ensures that the Data Connect emulator handles startup failures and unconfigured services gracefully without blocking other emulators. It updates shouldStart to verify the configuration and logs an error if it is missing, wraps the startup in a try-catch block in startAll, and adds corresponding unit tests. Feedback is provided to remove a redundant configuration length check in startAll to reduce nesting and improve code readability.
| const config = readFirebaseJson(options.config); | ||
| if (!config.length) { | ||
| throw new FirebaseError("No SQL Connect service found in firebase.json"); | ||
| } else if (config.length > 1) { | ||
| logger.warn( | ||
| `TODO: Add support for multiple services in the SQL Connect emulator. Currently emulating first service ${config[0].source}`, | ||
| dataconnectLogger.logLabeled( | ||
| "ERROR", | ||
| "dataconnect", | ||
| `Failed to start Data Connect emulator: No valid Data Connect configuration detected in firebase.json. Run ${clc.bold("firebase init dataconnect")} to configure it.`, | ||
| ); | ||
| } | ||
| } else { | ||
| if (config.length > 1) { | ||
| logger.warn( | ||
| `TODO: Add support for multiple services in the SQL Connect emulator. Currently emulating first service ${config[0].source}`, | ||
| ); | ||
| } | ||
|
|
||
| const args: DataConnectEmulatorArgs = { | ||
| listen: listenForEmulator.dataconnect, | ||
| projectId, | ||
| auto_download: true, | ||
| configDir: config[0].source, | ||
| config: options.config, | ||
| autoconnectToPostgres: true, | ||
| postgresListen: listenForEmulator["dataconnect.postgres"], | ||
| enable_output_generated_sdk: true, // TODO: source from arguments | ||
| enable_output_schema_extensions: true, | ||
| debug: options.debug, | ||
| account, | ||
| }; | ||
| const args: DataConnectEmulatorArgs = { | ||
| listen: listenForEmulator.dataconnect, | ||
| projectId, | ||
| auto_download: true, | ||
| configDir: config[0].source, | ||
| config: options.config, | ||
| autoconnectToPostgres: true, | ||
| postgresListen: listenForEmulator["dataconnect.postgres"], | ||
| enable_output_generated_sdk: true, // TODO: source from arguments | ||
| enable_output_schema_extensions: true, | ||
| debug: options.debug, | ||
| account, | ||
| }; | ||
|
|
||
| if (exportMetadata.dataconnect) { | ||
| utils.assertIsString(options.import); | ||
| const importDirAbsPath = path.resolve(options.import); | ||
| const exportMetadataFilePath = path.resolve( | ||
| importDirAbsPath, | ||
| exportMetadata.dataconnect.path, | ||
| ); | ||
| const dataDirectory = options.config.get("emulators.dataconnect.dataDir"); | ||
| if (exportMetadataFilePath && dataDirectory) { | ||
| dataconnectLogger.logLabeled( | ||
| "WARN", | ||
| "dataconnect", | ||
| "'firebase.json#emulators.dataconnect.dataDir' is set and `--import` flag was passed. " + | ||
| "This will overwrite any data saved from previous runs.", | ||
| ); | ||
| if ( | ||
| !options.nonInteractive && | ||
| !(await confirm({ | ||
| message: `Do you wish to continue and overwrite data in ${dataDirectory}?`, | ||
| default: false, | ||
| })) | ||
| ) { | ||
| await cleanShutdown(); | ||
| throw new FirebaseError("Command aborted"); | ||
| } | ||
| } | ||
|
|
||
| if (exportMetadata.dataconnect) { | ||
| utils.assertIsString(options.import); | ||
| const importDirAbsPath = path.resolve(options.import); | ||
| const exportMetadataFilePath = path.resolve( | ||
| importDirAbsPath, | ||
| exportMetadata.dataconnect.path, | ||
| ); | ||
| const dataDirectory = options.config.get("emulators.dataconnect.dataDir"); | ||
| if (exportMetadataFilePath && dataDirectory) { | ||
| EmulatorLogger.forEmulator(Emulators.DATACONNECT).logLabeled( | ||
| "WARN", | ||
| dataconnectLogger.logLabeled( | ||
| "BULLET", | ||
| "dataconnect", | ||
| "'firebase.json#emulators.dataconnect.dataDir' is set and `--import` flag was passed. " + | ||
| "This will overwrite any data saved from previous runs.", | ||
| `Importing data from ${exportMetadataFilePath}`, | ||
| ); | ||
| if ( | ||
| !options.nonInteractive && | ||
| !(await confirm({ | ||
| message: `Do you wish to continue and overwrite data in ${dataDirectory}?`, | ||
| default: false, | ||
| })) | ||
| ) { | ||
| await cleanShutdown(); | ||
| throw new FirebaseError("Command aborted"); | ||
| } | ||
| args.importPath = exportMetadataFilePath; | ||
| void trackEmulator("emulator_import", { | ||
| initiated_by: "start", | ||
| emulator_name: Emulators.DATACONNECT, | ||
| }); | ||
| } | ||
|
|
||
| EmulatorLogger.forEmulator(Emulators.DATACONNECT).logLabeled( | ||
| "BULLET", | ||
| "dataconnect", | ||
| `Importing data from ${exportMetadataFilePath}`, | ||
| ); | ||
| args.importPath = exportMetadataFilePath; | ||
| void trackEmulator("emulator_import", { | ||
| initiated_by: "start", | ||
| emulator_name: Emulators.DATACONNECT, | ||
| }); | ||
| try { | ||
| const dataConnectEmulator = new DataConnectEmulator(args); | ||
| await startEmulator(dataConnectEmulator); | ||
| } catch (err: unknown) { | ||
| try { | ||
| await EmulatorRegistry.stop(Emulators.DATACONNECT); | ||
| } catch { | ||
| // Ignore errors stopping failed instance | ||
| } | ||
| dataconnectLogger.logLabeled( | ||
| "ERROR", | ||
| "dataconnect", | ||
| `Failed to start Data Connect emulator: ${getErrMsg(err)}`, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
The check !config.length in startAll is redundant because shouldStart already performs this exact check and returns false if no Data Connect configuration is detected. Since listenForEmulator.dataconnect can only be truthy if shouldStart returned true, config.length is guaranteed to be greater than 0 here.
Removing this redundant check allows us to eliminate the nested else block, reducing the nesting level and improving readability in accordance with the repository style guide.
const config = readFirebaseJson(options.config);
if (config.length > 1) {
logger.warn(
"TODO: Add support for multiple services in the SQL Connect emulator. Currently emulating first service " + config[0].source,
);
}
const args: DataConnectEmulatorArgs = {
listen: listenForEmulator.dataconnect,
projectId,
auto_download: true,
configDir: config[0].source,
config: options.config,
autoconnectToPostgres: true,
postgresListen: listenForEmulator["dataconnect.postgres"],
enable_output_generated_sdk: true, // TODO: source from arguments
enable_output_schema_extensions: true,
debug: options.debug,
account,
};
if (exportMetadata.dataconnect) {
utils.assertIsString(options.import);
const importDirAbsPath = path.resolve(options.import);
const exportMetadataFilePath = path.resolve(
importDirAbsPath,
exportMetadata.dataconnect.path,
);
const dataDirectory = options.config.get("emulators.dataconnect.dataDir");
if (exportMetadataFilePath && dataDirectory) {
dataconnectLogger.logLabeled(
"WARN",
"dataconnect",
"'firebase.json#emulators.dataconnect.dataDir' is set and `--import` flag was passed. " +
"This will overwrite any data saved from previous runs.",
);
if (
!options.nonInteractive &&
!(await confirm({
message: "Do you wish to continue and overwrite data in " + dataDirectory + "?",
default: false,
}))
) {
await cleanShutdown();
throw new FirebaseError("Command aborted");
}
}
dataconnectLogger.logLabeled(
"BULLET",
"dataconnect",
"Importing data from " + exportMetadataFilePath,
);
args.importPath = exportMetadataFilePath;
void trackEmulator("emulator_import", {
initiated_by: "start",
emulator_name: Emulators.DATACONNECT,
});
}
try {
const dataConnectEmulator = new DataConnectEmulator(args);
await startEmulator(dataConnectEmulator);
} catch (err: unknown) {
try {
await EmulatorRegistry.stop(Emulators.DATACONNECT);
} catch {
// Ignore errors stopping failed instance
}
dataconnectLogger.logLabeled(
"ERROR",
"dataconnect",
"Failed to start Data Connect emulator: " + getErrMsg(err),
);
}References
- Reduce nesting as much as possible: Code should avoid unnecessarily deep nesting or long periods of nesting. Use early returns, continue, and break statements in functions and loops to handle edge cases early and keep main logic flat. Consider helper functions to encapsulate complex branching. (link)
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try the Wiz Code extension for VS Code, JetBrains, or Visual Studio. |
Description
Prevent missing configurations and startup failures for the Data Connect emulator from crashing emulators:start and blocking the rest of the emulator suite:
Fixes b/546204399
Scenarios Tested
Sample Commands