fix(dataconnect): Refactor CRUD helpers to use GraphQL variables and @Allow directive (#3182)

* add tests checking for new GQL output for [insert,upsert](many)

* change mutation functions to use @Allow

* factor out common code after @Allow changes

* update tests to expect @Allow

* update the @Allow directive for list inputs

* initial fix

* address reviewer comments

* fix style and normalize expected + actual query strings in tests

* add fdc to integration test documentation and update integration test artifact ignores

* improve getTableNames variable naming for clarity

* add 10k limit and nested coalesced field keys for @Allow

* de-duplicate code in mutaiton CRUD functions, and refactor tests

* update contributing

* add maxCount to array bulk insert variables

* limit @Allow nesting to _on_ relational fields

* update tests

* address reviewer comments

Author: stephenarosaj

.gitignore

@@ -21,6 +21,10 @@ firebase-admin-*.tgz
 
 docgen/markdown/
 
-# Dataconnect integration test artifacts should not be checked in
+# Integration test artifacts should not be checked in
+**/database-debug.log
+**/firestore-debug.log
 test/integration/dataconnect/dataconnect/.dataconnect
-test/integration/dataconnect/*.log
+**/dataconnect-debug.log
+**/pglite-debug.log
+

CONTRIBUTING.md

@@ -150,9 +150,20 @@ And then:
     'npx mocha \"test/integration/{auth,database,firestore}.spec.ts\" --slow 5000 --timeout 20000 --require ts-node/register'
 ```
 
-Currently, only the Auth, Database, and Firestore test suites work. Some test
-cases will be automatically skipped due to lack of emulator support. The section
-below covers how to run the full test suite against an actual Firebase project.
+Currently, only the Auth, Database, and Firestore test suites work. Some test cases
+will be automatically skipped due to lack of emulator support. 
+
+**Note:** You can also run the Data Connect test suite against the emulators using the same command.
+However, you must isolate the Data Connect tests by using a configuration file specific to Data Connect
+emulator testing:
+
+```bash
+  firebase emulators:exec \
+    --project fake-project-id --only dataconnect --config test/integration/dataconnect/firebase.json \
+    'npx mocha \"test/integration/data-connect.spec.ts\" --slow 5000 --timeout 20000 --require ts-node/register'
+```
+
+The section below covers how to run the full test suite against an actual Firebase project.
 
 #### Integration Tests with an actual Firebase project
 

src/data-connect/data-connect-api-client-internal.ts

@@ -67,6 +67,8 @@ const EXECUTE_GRAPH_QL_READ_ENDPOINT = 'executeGraphqlRead';
 const IMPERSONATE_QUERY_ENDPOINT = 'impersonateQuery';
 const IMPERSONATE_MUTATION_ENDPOINT = 'impersonateMutation';
 
+/** @internal The maximum number of items allowed in the @allow directive's maxCount argument. */
+export const ALLOW_DIRECTIVE_MAX_COUNT = 10_000;
 
 function getHeaders(isUsingGen: boolean): { [key: string]: string } {
   const headerValue = {
@@ -100,6 +102,10 @@ interface ConnectorsUrlParams extends ServicesUrlParams {
   connectorId: string;
 }
 
+interface FieldNode {
+  children: Map<string, FieldNode>;
+}
+
 /**
  * Class that facilitates sending requests to the Firebase Data Connect backend API.
  *
@@ -438,59 +444,22 @@ export class DataConnectApiClient {
   }
 
   /**
-   * Converts JSON data into a GraphQL literal string.
-   * Handles nested objects, arrays, strings, numbers, and booleans.
-   * Ensures strings are properly escaped.
+   * Generates both capitalized and camel-cased variations of a table name.
+   * Capitalization matches the schema types, and camel-case matches mutations.
    */
-  private objectToString(data: unknown): string {
-    if (typeof data === 'string') {
-      return JSON.stringify(data);
+  private getTableNames(tableName: string): { capitalized: string; camelCase: string } {
+    if (!tableName || tableName.length === 0) {
+      return { capitalized: tableName, camelCase: tableName };
     }
-    if (typeof data === 'number' || typeof data === 'boolean' || data === null) {
-      return String(data);
-    }
-    if (validator.isArray(data)) {
-      const elements = data.map(item => this.objectToString(item)).join(', ');
-      return `[${elements}]`;
-    }
-    if (typeof data === 'object' && data !== null) {
-      // Filter out properties where the value is undefined BEFORE mapping
-      const kvPairs = Object.entries(data)
-        .filter(([, val]) => val !== undefined)
-        .map(([key, val]) => {
-          // GraphQL object keys are typically unquoted.
-          return `${key}: ${this.objectToString(val)}`;
-        });
-  
-      if (kvPairs.length === 0) {
-        return '{}'; // Represent an object with no defined properties as {}
-      }
-      return `{ ${kvPairs.join(', ')} }`;
-    }
-    
-    // If value is undefined (and not an object property, which is handled above,
-    // e.g., if objectToString(undefined) is called directly or for an array element)
-    // it should be represented as 'null'.
-    if (typeof data === 'undefined') {
-      return 'null';
-    }
-
-    // Fallback for any other types (e.g., Symbol, BigInt - though less common in GQL contexts)
-    // Consider how these should be handled or if an error should be thrown.
-    // For now, simple string conversion.
-    return String(data);
+    const capitalized = tableName.charAt(0).toUpperCase() + tableName.slice(1);
+    const camelCase = tableName.charAt(0).toLowerCase() + tableName.slice(1);
+    return { capitalized, camelCase };
   }
 
-  private formatTableName(tableName: string): string {
-    // Format tableName: first character to lowercase
-    if (tableName && tableName.length > 0) {
-      return tableName.charAt(0).toLowerCase() + tableName.slice(1);
-    }
-    return tableName;
-  }
+
 
   private handleBulkImportErrors(err: FirebaseDataConnectError): never {
-    if (err.code === `data-connect/${DATA_CONNECT_ERROR_CODE_MAPPING.QUERY_ERROR}`){
+    if (err.code === `data-connect/${DATA_CONNECT_ERROR_CODE_MAPPING.QUERY_ERROR}`) {
       throw new FirebaseDataConnectError({
         code: DATA_CONNECT_ERROR_CODE_MAPPING.QUERY_ERROR,
         message: `${err.message}. Make sure that your table name passed in matches the type name in your `
@@ -508,39 +477,7 @@ export class DataConnectApiClient {
     tableName: string,
     data: Variables,
   ): Promise<ExecuteGraphqlResponse<GraphQlResponse>> {
-    if (!validator.isNonEmptyString(tableName)) {
-      throw new FirebaseDataConnectError({
-        code: DATA_CONNECT_ERROR_CODE_MAPPING.INVALID_ARGUMENT,
-        message: '`tableName` must be a non-empty string.'
-      });
-    }
-    if (validator.isArray(data)) {
-      throw new FirebaseDataConnectError({
-        code: DATA_CONNECT_ERROR_CODE_MAPPING.INVALID_ARGUMENT,
-        message: '`data` must be an object, not an array, for single insert. For arrays, please use '
-          + '`insertMany` function.'
-      });
-    }
-    if (!validator.isNonNullObject(data)) {
-      throw new FirebaseDataConnectError({
-        code: DATA_CONNECT_ERROR_CODE_MAPPING.INVALID_ARGUMENT,
-        message: '`data` must be a non-null object.'
-      });
-    }
-
-    try {
-      tableName = this.formatTableName(tableName);
-      const gqlDataString = this.objectToString(data);
-      const mutation = `mutation { ${tableName}_insert(data: ${gqlDataString}) }`;
-      // Use internal executeGraphql
-      return this.executeGraphql<GraphQlResponse, Variables>(mutation).catch(this.handleBulkImportErrors);
-    } catch (e: any) {
-      throw new FirebaseDataConnectError({
-        code: DATA_CONNECT_ERROR_CODE_MAPPING.INTERNAL,
-        message: `Failed to construct insert mutation: ${e.message}`,
-        cause: e,
-      });
-    }
+    return this.executeSingleMutation<GraphQlResponse, Variables>(tableName, data, 'insert');
   }
 
   /**
@@ -550,32 +487,7 @@ export class DataConnectApiClient {
     tableName: string,
     data: Variables,
   ): Promise<ExecuteGraphqlResponse<GraphQlResponse>> {
-    if (!validator.isNonEmptyString(tableName)) {
-      throw new FirebaseDataConnectError({
-        code: DATA_CONNECT_ERROR_CODE_MAPPING.INVALID_ARGUMENT,
-        message: '`tableName` must be a non-empty string.'
-      });
-    }
-    if (!validator.isNonEmptyArray(data)) {
-      throw new FirebaseDataConnectError({
-        code: DATA_CONNECT_ERROR_CODE_MAPPING.INVALID_ARGUMENT,
-        message: '`data` must be a non-empty array for insertMany.',
-      });
-    }
-
-    try {
-      tableName = this.formatTableName(tableName);
-      const gqlDataString = this.objectToString(data);
-      const mutation = `mutation { ${tableName}_insertMany(data: ${gqlDataString}) }`;
-      // Use internal executeGraphql
-      return this.executeGraphql<GraphQlResponse, Variables>(mutation).catch(this.handleBulkImportErrors);
-    } catch (e: any) {
-      throw new FirebaseDataConnectError({
-        code: DATA_CONNECT_ERROR_CODE_MAPPING.INTERNAL,
-        message: `Failed to construct insertMany mutation: ${e.message}`,
-        cause: e,
-      });
-    }
+    return this.executeBulkMutation<GraphQlResponse, Variables>(tableName, data, 'insertMany');
   }
 
   /**
@@ -584,6 +496,24 @@ export class DataConnectApiClient {
   public async upsert<GraphQlResponse, Variables extends object>(
     tableName: string,
     data: Variables,
+  ): Promise<ExecuteGraphqlResponse<GraphQlResponse>> {
+    return this.executeSingleMutation<GraphQlResponse, Variables>(tableName, data, 'upsert');
+  }
+
+  /**
+   * Insert multiple rows into the specified table, or update them if they already exist.
+   */
+  public async upsertMany<GraphQlResponse, Variables extends Array<unknown>>(
+    tableName: string,
+    data: Variables,
+  ): Promise<ExecuteGraphqlResponse<GraphQlResponse>> {
+    return this.executeBulkMutation<GraphQlResponse, Variables>(tableName, data, 'upsertMany');
+  }
+
+  private async executeSingleMutation<GraphQlResponse, Variables extends object>(
+    tableName: string,
+    data: Variables,
+    operationType: 'insert' | 'upsert'
   ): Promise<ExecuteGraphqlResponse<GraphQlResponse>> {
     if (!validator.isNonEmptyString(tableName)) {
       throw new FirebaseDataConnectError({
@@ -594,8 +524,8 @@ export class DataConnectApiClient {
     if (validator.isArray(data)) {
       throw new FirebaseDataConnectError({
         code: DATA_CONNECT_ERROR_CODE_MAPPING.INVALID_ARGUMENT,
-        message: '`data` must be an object, not an array, for single upsert. For arrays, please use '
-          + '`upsertMany` function.'
+        message: `\`data\` must be an object, not an array, for single ${operationType}.\
+          For arrays, please use \`${operationType}Many\` function.`
       });
     }
     if (!validator.isNonNullObject(data)) {
@@ -606,26 +536,28 @@ export class DataConnectApiClient {
     }
 
     try {
-      tableName = this.formatTableName(tableName);
-      const gqlDataString = this.objectToString(data);
-      const mutation = `mutation { ${tableName}_upsert(data: ${gqlDataString}) }`;
-      // Use internal executeGraphql
-      return this.executeGraphql<GraphQlResponse, Variables>(mutation).catch(this.handleBulkImportErrors);
+      const { capitalized, camelCase } = this.getTableNames(tableName);
+      const keys = getFieldsString(data);
+      const mutation =
+        `mutation($data: ${capitalized}_Data! @allow(fields: "${keys}")) {
+          ${camelCase}_${operationType}(data: $data)
+        }`;
+
+      return this.executeGraphql<GraphQlResponse, { data: Variables }>(mutation, { variables: { data } })
+        .catch(this.handleBulkImportErrors);
     } catch (e: any) {
       throw new FirebaseDataConnectError({
         code: DATA_CONNECT_ERROR_CODE_MAPPING.INTERNAL,
-        message: `Failed to construct upsert mutation: ${e.message}`,
+        message: `Failed to construct ${operationType} mutation: ${e.message}`,
         cause: e,
       });
     }
   }
 
-  /**
-   * Insert multiple rows into the specified table, or update them if they already exist.
-   */
-  public async upsertMany<GraphQlResponse, Variables extends Array<unknown>>(
+  private async executeBulkMutation<GraphQlResponse, Variables extends Array<unknown>>(
     tableName: string,
     data: Variables,
+    operationType: 'insertMany' | 'upsertMany'
   ): Promise<ExecuteGraphqlResponse<GraphQlResponse>> {
     if (!validator.isNonEmptyString(tableName)) {
       throw new FirebaseDataConnectError({
@@ -636,20 +568,30 @@ export class DataConnectApiClient {
     if (!validator.isNonEmptyArray(data)) {
       throw new FirebaseDataConnectError({
         code: DATA_CONNECT_ERROR_CODE_MAPPING.INVALID_ARGUMENT,
-        message: '`data` must be a non-empty array for upsertMany.'
+        message: `\`data\` must be a non-empty array for ${operationType}.`
+      });
+    }
+    if (data.length > ALLOW_DIRECTIVE_MAX_COUNT) {
+      throw new FirebaseDataConnectError({
+        code: DATA_CONNECT_ERROR_CODE_MAPPING.INVALID_ARGUMENT,
+        message: `\`data\` array exceeds the maximum limit of ${ALLOW_DIRECTIVE_MAX_COUNT} items.`
       });
     }
 
     try {
-      tableName = this.formatTableName(tableName);
-      const gqlDataString = this.objectToString(data);
-      const mutation = `mutation { ${tableName}_upsertMany(data: ${gqlDataString}) }`;
-      // Use internal executeGraphql
-      return this.executeGraphql<GraphQlResponse, Variables>(mutation).catch(this.handleBulkImportErrors);
+      const { capitalized, camelCase } = this.getTableNames(tableName);
+      const keys = getFieldsString(data);
+      const mutation =
+        `mutation($data: [${capitalized}_Data!]! @allow(fields: "${keys}", maxCount: ${ALLOW_DIRECTIVE_MAX_COUNT})) {
+          ${camelCase}_${operationType}(data: $data)
+        }`;
+
+      return this.executeGraphql<GraphQlResponse, { data: Variables }>(mutation, { variables: { data } })
+        .catch(this.handleBulkImportErrors);
     } catch (e: any) {
       throw new FirebaseDataConnectError({
         code: DATA_CONNECT_ERROR_CODE_MAPPING.INTERNAL,
-        message: `Failed to construct upsertMany mutation: ${e.message}`,
+        message: `Failed to construct ${operationType} mutation: ${e.message}`,
         cause: e,
       });
     }
@@ -689,3 +631,60 @@ interface ServerError {
   message?: string;
   status?: string;
 }
+
+/**
+ * Extracts property keys from an object or array of objects as a space-separated string,
+ * including recursively nested object/array fields for the `@allow(fields: ...)` directive.
+ * Leverages a hierarchical tree to deduplicate and merge fields.
+ *
+ * @internal
+ */
+export function getFieldsString(data: unknown): string {
+  const root: FieldNode = { children: new Map() };
+  mergeFieldsIntoTree(data, root);
+  return serializeFieldNode(root);
+}
+
+function mergeFieldsIntoTree(data: unknown, node: FieldNode, visited: Set<unknown> = new Set<unknown>()): void {
+  if (validator.isArray(data)) {
+    data.forEach((item) => mergeFieldsIntoTree(item, node, visited));
+    return;
+  }
+  if (!validator.isNonNullObject(data) || data instanceof Date) {
+    return;
+  }
+  if (visited.has(data)) {
+    throw new Error('Circular reference detected in input.');
+  }
+  visited.add(data);
+  const record = data as Record<string, unknown>;
+  for (const [key, val] of Object.entries(record)) {
+    if (val === undefined) {
+      continue;
+    }
+    let childNode = node.children.get(key);
+    if (!childNode) {
+      childNode = { children: new Map() };
+      node.children.set(key, childNode);
+    }
+    if (key.includes('_on_')) {
+      mergeFieldsIntoTree(val, childNode, visited);
+    }
+  }
+  visited.delete(data);
+}
+
+function serializeFieldNode(node: FieldNode): string {
+  const parts: string[] = [];
+  const sortedKeys = Array.from(node.children.keys()).sort((a, b) => a.localeCompare(b));
+  for (const key of sortedKeys) {
+    const childNode = node.children.get(key)!;
+    if (childNode.children.size > 0) {
+      const nestedString = serializeFieldNode(childNode);
+      parts.push(`${key} { ${nestedString} }`);
+    } else {
+      parts.push(key);
+    }
+  }
+  return parts.join(' ');
+}