Fdc native sql type inference - #11110
yantong-google wants to merge 3 commits into
Conversation
Returning an empty Uint8Array from onMessage ensures pg-gateway sets skipProcessing = true even when PGlite yields 0 backend messages (such as on Flush 'H', or when PGlite discards Describe messages following a Parse error). This avoids: 1. Error overriding: Prevents pg-gateway from injecting a fake "Message code not yet implemented" error that overwrites real PostgreSQL errors. 2. Connection poisoning: Prevents emitting a premature ReadyForQuery that leaves unconsumed messages in the client socket buffer and desynchronizes subsequent queries.
![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 introduces native SQL type inference for Firebase Data Connect, adding the dataconnect:sql:infer command, supporting local Cloud SQL Auth Proxy connections, and integrating the feature with SDK generation and builds under a new experiment flag. Feedback on the changes highlights three critical issues: first, nativeSqlInferMode needs to support configurations with multiple Data Connect services where dataconnect is an array; second, the generator in pgliteServer.ts should yield the empty Uint8Array instead of returning it, as return values are ignored during standard async iteration; and third, the custom signal handlers in cloudSqlProxy.ts must properly propagate signals to prevent the CLI process from hanging on Ctrl+C.
| export function nativeSqlInferMode(config?: Config): string | undefined { | ||
| const mode = config?.get("dataconnect.nativeSqlInferMode") as string | undefined; | ||
| if (!mode) { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
The current implementation of nativeSqlInferMode assumes that dataconnect in firebase.json is always a single object. However, firebase-tools supports multiple Data Connect services configured as an array (i.e., DataConnectMultiple). If a user has multiple services, config.get("dataconnect.nativeSqlInferMode") will return undefined, causing the feature to silently fail or throw a missing configuration error. We should handle both single and multiple service configurations.
| export function nativeSqlInferMode(config?: Config): string | undefined { | |
| const mode = config?.get("dataconnect.nativeSqlInferMode") as string | undefined; | |
| if (!mode) { | |
| return undefined; | |
| } | |
| interface DataConnectServiceConfig { | |
| nativeSqlInferMode?: string; | |
| } | |
| export function nativeSqlInferMode(config?: Config): string | undefined { | |
| const dataconnect = config?.get("dataconnect"); | |
| if (!dataconnect) { | |
| return undefined; | |
| } | |
| let mode: string | undefined; | |
| if (Array.isArray(dataconnect)) { | |
| const services = dataconnect as DataConnectServiceConfig[]; | |
| mode = services.find((s) => s.nativeSqlInferMode)?.nativeSqlInferMode; | |
| } else { | |
| const service = dataconnect as DataConnectServiceConfig; | |
| mode = service.nativeSqlInferMode; | |
| } | |
| if (!mode) { | |
| return undefined; | |
| } |
| // Returning an empty Uint8Array ensures pg-gateway sets skipProcessing = true | ||
| // even when PGlite produces 0 backend messages (such as on Flush 'H', or when | ||
| // PGlite discards Describe messages following a Parse error). | ||
| // | ||
| // This avoids: | ||
| // 1. Error overriding: Prevents pg-gateway from injecting a fake "Message code not yet implemented" | ||
| // error that overwrites the real PostgreSQL error (e.g. syntax or table not found errors). | ||
| // 2. Connection poisoning: Prevents emitting a premature ReadyForQuery that leaves unconsumed | ||
| // messages in the client socket buffer and desynchronizes subsequent queries. | ||
| return new Uint8Array(0); | ||
| }, |
There was a problem hiding this comment.
In JavaScript/TypeScript, standard consumers of async iterables (such as for await...of loops or Readable.from()) completely ignore the return value of a generator function (i.e., when done: true). Since onMessage is an async * generator, returning new Uint8Array(0) on line 82 will be ignored by pg-gateway if it consumes the generator using standard iteration. To ensure pg-gateway receives this empty buffer and sets skipProcessing = true, you should yield the empty array instead of returning it.
| // Returning an empty Uint8Array ensures pg-gateway sets skipProcessing = true | |
| // even when PGlite produces 0 backend messages (such as on Flush 'H', or when | |
| // PGlite discards Describe messages following a Parse error). | |
| // | |
| // This avoids: | |
| // 1. Error overriding: Prevents pg-gateway from injecting a fake "Message code not yet implemented" | |
| // error that overwrites the real PostgreSQL error (e.g. syntax or table not found errors). | |
| // 2. Connection poisoning: Prevents emitting a premature ReadyForQuery that leaves unconsumed | |
| // messages in the client socket buffer and desynchronizes subsequent queries. | |
| return new Uint8Array(0); | |
| }, | |
| // Returning an empty Uint8Array ensures pg-gateway sets skipProcessing = true | |
| // even when PGlite produces 0 backend messages (such as on Flush 'H', or when | |
| // PGlite discards Describe messages following a Parse error). | |
| // | |
| // This avoids: | |
| // 1. Error overriding: Prevents pg-gateway from injecting a fake "Message code not yet implemented" | |
| // error that overwrites the real PostgreSQL error (e.g. syntax or table not found errors). | |
| // 2. Connection poisoning: Prevents emitting a premature ReadyForQuery that leaves unconsumed | |
| // messages in the client socket buffer and desynchronizes subsequent queries. | |
| yield new Uint8Array(0); | |
| }, |
| const onSignal = () => { | ||
| void close(); | ||
| }; | ||
| process.on("SIGINT", onSignal); | ||
| process.on("SIGTERM", onSignal); | ||
| const closeAndUnregister = async (): Promise<void> => { | ||
| process.removeListener("SIGINT", onSignal); | ||
| process.removeListener("SIGTERM", onSignal); | ||
| await close(); | ||
| }; |
There was a problem hiding this comment.
When registering custom SIGINT or SIGTERM handlers in Node.js, the default process termination behavior is overridden. Since onSignal only schedules the asynchronous close() cleanup but does not exit the process or propagate the signal, the CLI process will hang indefinitely on Ctrl+C if there are active handles (such as the Cloud SQL connector or spawned child processes). To prevent hangs, the signal handler should clean up, remove itself, and then re-send the signal to the process so it can terminate naturally with the correct exit status.
const onSignal = async (signal: string) => {
process.removeListener("SIGINT", onSignal);
process.removeListener("SIGTERM", onSignal);
await close();
process.kill(process.pid, signal);
};
process.on("SIGINT", () => void onSignal("SIGINT"));
process.on("SIGTERM", () => void onSignal("SIGTERM"));
const closeAndUnregister = async (): Promise<void> => {
process.removeListener("SIGINT", onSignal);
process.removeListener("SIGTERM", onSignal);
await close();
};2 participants
Description
This PR introduces support for Firebase Data Connect Native SQL Type Inference, allowing developers to write native PostgreSQL queries and have their GraphQL types automatically inferred into
_inferred_types.gqlagainst a live database.1. Two-Phase Architecture
firebase dataconnect:sql:infer):fdc sql infer --config_dir=...).--cloud-sqlis specified: launches an in-process local Cloud SQL Auth Proxy (@google-cloud/cloud-sql-connector) listening on a temporary Unix domain socket (.s.PGSQL.5432) with IAM database authentication.FIREBASE_DATACONNECT_POSTGRESQL_STRING.EmulatorHubClient._inferred_types.gqlin the connector directory.fdc build&dataconnect:sdk:generate):dataconnect.nativeSqlInferMode: "db"is configured,build()(used duringfirebase deploy) anddataconnect:sdk:generatepassSQL_CONNECT_PREVIEW=native_sql_type_inferenceandSQL_CONNECT_INFER_MODE=db, but explicitly setFIREBASE_DATACONNECT_POSTGRESQL_STRING="".firebase deployand SDK generation stay strictly offline and reproducible from committed.gqlsources (including committed_inferred_types.gql), without unintended side-effects on the database.dataconnect:sdk:generateemits a staleness warning advising developers to runfirebase dataconnect:sql:inferif queries changed.2. Feature Gating & Config
fdcnativesqlinfer(default: false).schema/firebase-config.jsonandsrc/firebaseConfig.tsto support"nativeSqlInferMode": "db"underdataconnect.3. Commit Structure
This branch has been rebased cleanly on
upstream/mainwith 3 atomic commits:fix(dataconnect): propagate real Postgres errors from pglite proxy: FixesonMessagegenerator inpgliteServer.tsto yieldnew Uint8Array(0)when PGlite yields 0 messages, preventingpg-gatewayfrom overriding real Postgres errors or poisoning connections.feat(dataconnect): add native SQL type inference and dataconnect:sql:infer command: Core CLI command, config validation, experiment gating, and offlinebuild/sdk:generateintegration.feat(dataconnect): support --cloud-sql local proxy in dataconnect:sql:infer: Unix domain socket Cloud SQL proxy (src/dataconnect/cloudSqlProxy.ts) and--cloud-sqlflag support.4. Open Follow-ups / Handoff Notes for Teammates
CLOUD_IAM_SERVICE_ACCOUNTAuth: Insrc/dataconnect/cloudSqlProxy.ts, service account IAM auth currently relies on Application Default Credentials (new Connector()). Oncecloud-sql-nodejs-connector#61adds support for OAuth2 token suppliers for service accounts, update this to passFBToolsAuthClient.Scenarios Tested
Automated Unit Tests
src/emulator/dataconnectEmulator.spec.ts:sqlInfer: Verified command arguments (--config_dir,--connector_id) and error handling when child process exits non-zero.dataconnect:sql:infercommand:fdcnativesqlinferexperiment is disabled.dataconnect.nativeSqlInferModeis missing or set to an unsupported mode (e.g."pglite").EmulatorHubClient.FIREBASE_DATACONNECT_POSTGRESQL_STRINGwhen emulator is not running.--cloud-sqlflag behavior (proxy not invoked when omitted).MISSING_PROJECT_PLACEHOLDERwhen projectId is not provided.dataconnect:sdk:generate:SQL_CONNECT_PREVIEWandSQL_CONNECT_INFER_MODE=dbare passed.FIREBASE_DATACONNECT_POSTGRESQL_STRING=""is passed to guarantee offline codegen.nativeSqlInferModeis set.dataconnect build:FIREBASE_DATACONNECT_POSTGRESQL_STRING=""is passed to guarantee offline reproducible build during deploy.nativeSqlInferEnv:src/dataconnect/cloudSqlProxy.spec.ts:CLOUD_IAM_USERusesFBToolsAuthClient.CLOUD_IAM_SERVICE_ACCOUNTinitializes ADC connector.src/emulator/dataconnect/pgliteServer.ts:Manual End-to-End Testing
firebase emulators:start.firebase dataconnect:sql:inferin another terminal; confirmed_inferred_types.gqlwas generated from native SQL queries.firebase dataconnect:sql:infer --cloud-sqlagainst a live Cloud SQL instance with IAM user authentication.firebase dataconnect:sdk:generate; confirmed SDK generation consumed_inferred_types.gqlwithout requiring database connectivity.Sample Commands
1. Enable the experiment