Adding sendVariableValues and sendHeaders parameters and specifications to engine

CHANGELOG.md

@@ -1,6 +1,8 @@
 # Changelog
 
 ### vNext
+- `apollo-engine-reporting`: BREAKING CHANGE: By default, send no GraphQL variable values to Apollo's servers instead of sending all variable values. Use the new EngineReportingOption `sendVariableValues` to send some or all variable values, possibly after transforming them. 
+This replaces the `privateVariables` option, which is now deprecated. [PR #2847](https://github.com/apollographql/apollo-server/pull/2847) [PR #2472](https://github.com/apollographql/apollo-server/pull/2472)
 
 ### v2.6.2
 

docs/source/api/apollo-server.md

@@ -119,7 +119,7 @@ new ApolloServer({
 
 * `engine`: <`EngineReportingOptions`> | boolean
 
-  Provided the `ENGINE_API_KEY` environment variable is set, the engine reporting agent will be started automatically. The API key can also be provided as the `apiKey` field in an object passed as the `engine` field. See the [EngineReportingOptions](#enginereportingoptions) section for a full description of how to configure the reporting agent, including how to blacklist variables. When using the Engine proxy, this option should be set to false.
+  Provided the `ENGINE_API_KEY` environment variable is set, the engine reporting agent will be started automatically. The API key can also be provided as the `apiKey` field in an object passed as the `engine` field. See the [EngineReportingOptions](#enginereportingoptions) section for a full description of how to configure the reporting agent, including how to blocklist variables. When using the Engine proxy, this option should be set to false.
 
 * `persistedQueries`: <`Object`> | false
 
@@ -343,14 +343,31 @@ addMockFunctionsToSchema({
    to standard error. Specify this function to process errors in a different
    way.
 
-*  `privateVariables`: Array<String> | boolean
+*  `privateVariables`: Array<String\> | boolean
 
+   DEPRECATING IN VERSION XX.XX.XX for `sendVariableValues`, which will support the same
+   functionalities but allow for more flexibility.
+
    A case-sensitive list of names of variables whose values should not be sent
    to Apollo servers, or 'true' to leave out all variables. In the former
    case, the report will indicate that each private variable was redacted in
    the latter case, no variables are sent at all.
-
-*  `privateHeaders`: Array<String> | boolean
+
+* `sendVariableValues`: { valueModifier: (options: { variables: Record<string, any>, operationString?: string } ) => Record<string, any> }
+                     | { exceptVariableNames: Array<String\> }
+                     | { whitelistAll: boolean }
+
+    By default, Apollo Server does not send the values of any GraphQL variables to Apollo's servers, because variable values often contain the private data of your app's users. If you'd like variable values to be included in traces, set this option. This option can take several forms:
+
+    - { whitelistAll: ... }: false to blocklist, or true to whitelist all variable values
+    - { valueModifier: ... }: a custom function for modifying variable values
+    - { exceptVariableNames: ... }: a case-sensitive list of names of variables whose values should not be sent to Apollo servers
+
+    Defaults to blocklisting all variable values if both this parameter and
+    the to-be-deprecated `privateVariables` are not set. The report will also
+    indicate each private variable key redacted by { whitelistAll: false } or { exceptVariableNames: [...] }.
+
+*  `privateHeaders`: Array<String\> | boolean
 
    A case-insensitive list of names of HTTP headers whose values should not be
    sent to Apollo servers, or 'true' to leave out all HTTP headers. Unlike

packages/apollo-engine-reporting/src/__tests__/extension.test.ts

@@ -6,7 +6,11 @@ import {
 import { Trace } from 'apollo-engine-reporting-protobuf';
 import { graphql } from 'graphql';
 import { Request } from 'node-fetch';
-import { EngineReportingExtension } from '../extension';
+import {
+  EngineReportingExtension,
+  makeTraceDetails,
+  makeTraceDetailsLegacy,
+} from '../extension';
 import { InMemoryLRUCache } from 'apollo-server-caching';
 
 test('trace construction', async () => {
@@ -83,3 +87,150 @@ test('trace construction', async () => {
   requestDidEnd();
   // XXX actually write some tests
 });
+
+const variables: Record<string, any> = {
+  testing: 'testing',
+  t2: 2,
+};
+
+test('check variableJson output for sendVariableValues null or undefined (default)', () => {
+  // Case 1: No keys/values in variables to be filtered/not filtered
+  const emptyOutput = new Trace.Details();
+  expect(makeTraceDetails({}, null)).toEqual(emptyOutput);
+  expect(makeTraceDetails({}, undefined)).toEqual(emptyOutput);
+  expect(makeTraceDetails({})).toEqual(emptyOutput);
+
+  // Case 2: Filter all variables
+  const filteredOutput = new Trace.Details();
+  Object.keys(variables).forEach(name => {
+    filteredOutput.variablesJson[name] = '';
+  });
+  expect(makeTraceDetails(variables)).toEqual(filteredOutput);
+  expect(makeTraceDetails(variables, null)).toEqual(filteredOutput);
+  expect(makeTraceDetails(variables, undefined)).toEqual(filteredOutput);
+});
+
+test('check variableJson output for sendVariableValues whitelist type', () => {
+  // Case 1: No keys/values in variables to be filtered/not filtered
+  const emptyOutput = new Trace.Details();
+  expect(makeTraceDetails({}, { always: true })).toEqual(emptyOutput);
+  expect(makeTraceDetails({}, { always: false })).toEqual(emptyOutput);
+
+  // Case 2: Filter all variables
+  const filteredOutput = new Trace.Details();
+  Object.keys(variables).forEach(name => {
+    filteredOutput.variablesJson[name] = '';
+  });
+  expect(makeTraceDetails(variables, { whitelistAll: false })).toEqual(
+    filteredOutput,
+  );
+
+  // Case 3: Do not filter variables
+  const nonFilteredOutput = new Trace.Details();
+  Object.keys(variables).forEach(name => {
+    nonFilteredOutput.variablesJson[name] = JSON.stringify(variables[name]);
+  });
+  expect(makeTraceDetails(variables, { whitelistAll: true })).toEqual(
+    nonFilteredOutput,
+  );
+});
+
+test('variableJson output for privacyEnforcer Array type', () => {
+  const privateVariablesArray: string[] = ['testing', 'notInVariables'];
+  const expectedVariablesJson = {
+    testing: '',
+    t2: JSON.stringify(2),
+  };
+  expect(
+    makeTraceDetails(variables, { exceptVariableNames: privateVariablesArray })
+      .variablesJson,
+  ).toEqual(expectedVariablesJson);
+});
+
+test('variableJson output for privacyEnforcer custom function', () => {
+  // Custom function that redacts every variable to 100;
+  const modifiedValue = 100;
+  const customModifier = (input: {
+    variables: Record<string, any>;
+  }): Record<string, any> => {
+    let out: Record<string, any> = {};
+    Object.keys(input.variables).map((name: string) => {
+      out[name] = modifiedValue;
+    });
+    return out;
+  };
+
+  // Expected output
+  const output = new Trace.Details();
+  Object.keys(variables).forEach(name => {
+    output.variablesJson[name] = JSON.stringify(modifiedValue);
+  });
+
+  expect(
+    makeTraceDetails(variables, { valueModifier: customModifier }),
+  ).toEqual(output);
+});
+
+test('whitelist=False equivalent to Array(all variables)', () => {
+  let privateVariablesArray: string[] = ['testing', 't2'];
+  expect(
+    makeTraceDetails(variables, { whitelistAll: false }).variablesJson,
+  ).toEqual(
+    makeTraceDetails(variables, { exceptVariableNames: privateVariablesArray })
+      .variablesJson,
+  );
+});
+
+test('original keys in variables match the modified keys', () => {
+  const origKeys = Object.keys(variables);
+  const firstKey = origKeys[0];
+  // remove the first key
+  const modifiedKeys = Object.keys(variables).slice(1);
+  // add a key
+  modifiedKeys.push('new v');
+
+  const modifier = (input: {
+    variables: Record<string, any>;
+  }): Record<string, any> => {
+    let out: Record<string, any> = {};
+    Object.keys(modifiedKeys).map((name: string) => {
+      out[name] = 100;
+    });
+    return out;
+  };
+
+  expect(
+    Object.keys(
+      makeTraceDetails(variables, { valueModifier: modifier }).variablesJson,
+    ).sort(),
+  ).toEqual(origKeys.sort());
+
+  // expect empty string for keys removed by the custom modifier
+  expect(
+    makeTraceDetails(variables, { valueModifier: modifier }).variablesJson[
+      firstKey
+    ],
+  ).toEqual('');
+});
+
+/**
+ * Tests for old privateVariables reporting option
+ */
+test('test variableJson output for to-be-deprecated privateVariable option', () => {
+  // Case 1: privateVariables == False; same as whitelist all
+  expect(makeTraceDetailsLegacy({}, false)).toEqual(
+    makeTraceDetails({}, { whitelistAll: false }),
+  );
+  expect(makeTraceDetailsLegacy(variables, false)).toEqual(
+    makeTraceDetails(variables, { whitelistAll: true }),
+  );
+
+  // Case 2: privateVariables is an Array; same as makeTraceDetails
+  const privacyEnforcerArray: string[] = ['testing', 'notInVariables'];
+  expect(
+    makeTraceDetailsLegacy(variables, privacyEnforcerArray).variablesJson,
+  ).toEqual(
+    makeTraceDetails(variables, { exceptVariableNames: privacyEnforcerArray })
+      .variablesJson,
+  );
+});

packages/apollo-engine-reporting/src/agent.ts

@@ -25,6 +25,20 @@ export interface ClientInfo {
   clientReferenceId?: string;
 }
 
+export type VariableValueModifierOptions = {
+  variables: Record<string, any>;
+  operationString?: string;
+};
+
+export type VariableValueOptions =
+  | {
+      valueModifier: (
+        options: VariableValueModifierOptions,
+      ) => Record<string, any>;
+    }
+  | { exceptVariableNames: Array<String> }
+  | { whitelistAll: boolean };
+
 export type GenerateClientInfo<TContext> = (
   requestContext: GraphQLRequestContext<TContext>,
 ) => ClientInfo;
@@ -82,12 +96,29 @@ export interface EngineReportingOptions<TContext> {
    */
   reportErrorFunction?: (err: Error) => void;
   /**
-   * A case-sensitive list of names of variables whose values should not be sent
-   * to Apollo servers, or 'true' to leave out all variables. In the former
+   * [TO-BE-DEPRECATED] A case-sensitive list of names of variables whose values should
+   * not be sent to Apollo servers, or 'true' to leave out all variables. In the former
    * case, the report will indicate that each private variable was redacted; in
    * the latter case, no variables are sent at all.
    */
   privateVariables?: Array<String> | boolean;
+  /**
+   * By default, Apollo Server does not send the values of any GraphQL variables to Apollo's servers, because variable
+   * values often contain the private data of your app's users. If you'd like variable values to be included in traces, set this option.
+   * This option can take several forms:
+   * - { whitelistAll: ... }: false to blocklist, or true to whitelist all variable values
+   * - { valueModifier: ... }: a custom function for modifying variable values
+   * - { exceptVariableNames: ... }: a case-sensitive list of names of variables whose values should not be sent to Apollo servers
+   *
+   * Defaults to blocklisting all variable values if both this parameter and
+   * the to-be-deprecated `privateVariables` are not set. The report will also
+   * indicate each private variable key redacted by { whitelistAll: false } or { exceptVariableNames: [...] }.
+   *
+   * TODO(helen): Add new flag to the trace details (and modify the protobuf message structure) to indicate the type of modification. Then, add the following description to the docs:
+   * "The report will indicate that variable values were modified by a custom function, or will list all private variables redacted."
+   * TODO(helen): LINK TO EXAMPLE FUNCTION? e.g. a function recursively search for keys to be blocklisted
+   */
+  sendVariableValues?: VariableValueOptions;
   /**
    * A case-insensitive list of names of HTTP headers whose values should not be
    * sent to Apollo servers, or 'true' to leave out all HTTP headers. Unlike