Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
## Upcoming

- `apollo`
- <First `apollo` related entry goes here>
- Support disabling literal stripping when extracting queries. [1703](https://github.com/apollographql/apollo-tooling/pull/1703)
- `apollo-codegen-flow`
- <First `apollo-codegen-flow` related entry goes here>
- `apollo-codegen-scala`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ exports[`defaultOperationRegistrySignature fragments in various order 1`] = `"fr

exports[`defaultOperationRegistrySignature full test 1`] = `"fragment Bar on User{age@skip(if:$a)...Nested}fragment Nested on User{blah}query Foo($a:Boolean,$b:Int){user(age:0,name:\\"\\"){aliased:name tz...Bar...on User{bee hello}}}"`;

exports[`defaultOperationRegistrySignature test with stripLiterals=false 1`] = `"query Foo($b:Int){user(age:5,name:\\"hello\\"){a@skip(if:true)b@include(if:false)c(value:4){d}...Bar...on User{hello@directive(arg:\\"Value!\\")}}}"`;

exports[`defaultOperationRegistrySignature with various argument types 1`] = `"query OpName($a:[[Boolean!]!],$b:EnumType,$c:Int!){user{name(apple:$a,bag:$b,cat:$c)}}"`;

exports[`defaultOperationRegistrySignature with various inline types 1`] = `"query OpName{user{name(apple:[[0]],bag:{input:\\"\\"},cat:ENUM_VALUE)}}"`;
24 changes: 22 additions & 2 deletions packages/apollo-graphql/src/__tests__/operationId.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -273,12 +273,32 @@ describe("defaultOperationRegistrySignature", () => {
blah
}
`
},
{
name: "test with stripLiterals=false",
operationName: "Foo",
input: gql`
query Foo($b: Int) {
user(name: "hello", age: 5) {
...Bar
a @skip(if: true)
b @include(if: false)
c(value: 4) {
d
}
... on User {
hello @directive(arg: "Value!")
}
}
}
`,
options: { stripLiterals: false }
}
];
cases.forEach(({ name, operationName, input }) => {
cases.forEach(({ name, operationName, input, options }) => {
test(name, () => {
expect(
defaultOperationRegistrySignature(input, operationName)
defaultOperationRegistrySignature(input, operationName, options)
).toMatchSnapshot();
});
});
Expand Down
26 changes: 10 additions & 16 deletions packages/apollo-graphql/src/operationId.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,25 +69,19 @@ export function defaultEngineReportingSignature(
}

// The operation registry signature function consists of removing extra whitespace,
// sorting the AST in a deterministic manner, hiding string and numeric literals,
// and removing unused definitions. This is a less aggressive transform than its
// engine reporting signature counterpart.
//
// XXX
// The hiding of literals is currently being discussed. This behavior
// should not be depended on in the future, as it's likely we will choose not
// to hide them at all. The rationale being, if queries are being shipped to
// a client bundle, exposing PII via this signature is a very small concern,
// relatively speaking.
// sorting the AST in a deterministic manner, potentially hiding string and numeric
// literals, and removing unused definitions. This is a less aggressive transform
// than its engine reporting signature counterpart.
export function defaultOperationRegistrySignature(
ast: DocumentNode,
operationName: string
operationName: string,
options: { stripLiterals: boolean } = { stripLiterals: true }
): string {
return printWithReducedWhitespace(
sortAST(
hideStringAndNumericLiterals(dropUnusedDefinitions(ast, operationName))
)
);
const withoutUnusedDefs = dropUnusedDefinitions(ast, operationName);
const maybeWithLiterals = options.stripLiterals
? hideStringAndNumericLiterals(withoutUnusedDefs)
: withoutUnusedDefs;
return printWithReducedWhitespace(sortAST(maybeWithLiterals));
}

export function operationHash(operation: string): string {
Expand Down
2 changes: 2 additions & 0 deletions packages/apollo/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,8 @@ OPTIONS

--queries=queries Deprecated in favor of the includes flag

--[no-]stripLiterals Whether to strip literals from extracted queries. DEFAULT: true

--tagName=tagName Name of the template literal tag used to identify template literals containing
GraphQL queries in Javascript/Typescript code
```
Expand Down
13 changes: 11 additions & 2 deletions packages/apollo/src/commands/client/extract.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { flags } from "@oclif/command";
import { writeFileSync } from "fs";
import { ClientCommand } from "../../Command";
import {
Expand All @@ -9,7 +10,13 @@ import { ClientIdentity } from "apollo-language-server";
export default class ClientExtract extends ClientCommand {
static description = "Extract queries from a client";
static flags = {
...ClientCommand.flags
...ClientCommand.flags,
stripLiterals: flags.boolean({
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since "literals" also applies to object and list literals (which are not stripped), perhaps we should be a bit more clear with this option name. Even if it's a bit wordy, the intent is more clear, I think? (And its less necessary for someone to understand the notion of negated flags and perhaps more intrinsic understanding of the default behavior?). This change would require changing the default to false below and flipping other conditionals too, of course.)

Suggested change
stripLiterals: flags.boolean({
preserveStringAndNumericLiterals: flags.boolean({

description:
"Whether to strip literals from extracted queries. DEFAULT: true",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"Whether to strip literals from extracted queries. DEFAULT: true",
"Disable redaction of string and numerical literals. Without this flag, these values will be replaced with empty strings (`''`) and zeroes (`0`) respectively. This redaction is intended to avoid inadvertently outputting potentially personally identifiable information (e.g. embedded passwords or API keys) into operation manifests.",

default: true,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If heeding my suggestion above, this default would become false.

allowNo: true
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we could get rid of this if the default was false.

Suggested change
allowNo: true

})
};

static args = [
Expand All @@ -30,7 +37,9 @@ export default class ClientExtract extends ClientCommand {
{
title: "Extracting operations from project",
task: async ctx => {
ctx.operations = getOperationManifestFromProject(this.project);
ctx.operations = getOperationManifestFromProject(this.project, {
stripLiterals: flags.stripLiterals
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also suggest renaming this to more accurately describe its scope:

Suggested change
stripLiterals: flags.stripLiterals
preserveStringAndNumericLiterals: flags.preserveStringAndNumericLiterals

});
ctx.clientIdentity = config.client;
}
},
Expand Down
6 changes: 4 additions & 2 deletions packages/apollo/src/utils/getOperationManifestFromProject.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,16 @@ export interface ManifestEntry {
}

export function getOperationManifestFromProject(
project: GraphQLClientProject
project: GraphQLClientProject,
options: { stripLiterals: boolean } = { stripLiterals: true }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
options: { stripLiterals: boolean } = { stripLiterals: true }
options: {
preserveStringAndNumericLiterals: boolean,
} = {
preserveStringAndNumericLiterals: true,
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably, the default should also flip to false? :)

): ManifestEntry[] {
const manifest = Object.entries(
project.mergedOperationsAndFragmentsForService
).map(([operationName, operationAST]) => {
const printed = defaultOperationRegistrySignature(
operationAST,
operationName
operationName,
{ stripLiterals: options.stripLiterals }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{ stripLiterals: options.stripLiterals }
{
preserveStringAndNumericLiterals:
options.preserveStringAndNumericLiterals
}

);

return {
Expand Down