Adding sendVariableValues and sendHeaders parameters and specifications to engine #2847

helenwh · 2019-06-14T16:55:52Z

Addressing the issues in: https://apollographql.atlassian.net/browse/BE-93
And a followup to this PR: #2472

Adding a new EngineReportingOptions parameter, sendVariableValues, that accepts custom functions for modifying private or sensitive variable values in addition to the booleans/arrays previously supported by the privateVariables option. Defaults to blocklisting all values.
DEPRECATING privateVariables
Adding a new parameter, sendHeaders, which defaults to a blocklist as well, but otherwise preserves properties of privateHeaders
DEPRECATING privateHeaders

[Question/Discussion] I am thinking about including a VariableModificationType in the messaging for a later PR, but it might be unnecessary; I would need to make changes in the UI for it to be used, and I'm not sure if users care/want to know how their variables are being modified?

(https://apollographql.quip.com/KfS7AWThfQau)

apollo-cla · 2019-06-14T16:55:53Z

@helenwh: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

zionts

I think this looks great! The main thing that's missing is an update to the release notes of Apollo Server, and maybe a parallel PR to the docs! I also brought up some open questions for discussion.

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

packages/apollo-engine-reporting-protobuf/src/reports.proto

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

helenwh · 2019-06-17T17:06:29Z

UPDATE:

Added documentation to CHANGELOG.md and apollo-server.md
Included queryString as argument to custom function
Moved protobuf changes adding privateVariablesEnforcerType to the trace details out of this PR

TODO:

make some more docs changes (e.g. an example custom function input)

Things to consider:

accept the schema as argument to custom function (in addition to the operation string and variables)
change enforcePrivateVariables to an object w. fields for each modifier type
limit what enforcePrivateVariables custom function can modify (e.g. [input keys] = [output keys])

docs/source/api/apollo-server.md

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

glasser

I'm really excited this change is happening!

docs/source/api/apollo-server.md

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

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

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

CHANGELOG.md

…kVariableValues), but this is still TBD - use the same helper for both the deprecated privateVariable option and the new option, since the logic is (basically) the same - added helper (and tests) to enforce that originalVariables.keys == modifiedVariables.keys - updated documentation

helenwh · 2019-06-19T01:00:48Z

@glasser @zionts I made some fixes with your feedback! I meant to push another commit with just the changes, but somewhere along the way I squashed too many... The diff from the last change can be viewed here, sorry!!

TODO: I think we still need to decide on an option name and default value, but as a placeholder for now I just used an alternative suggestion from @glasser.

…s, and the subsequent tests/docs

helenwh · 2019-06-19T18:00:34Z

Update: Going to make parallel changes to the option privateHeaders

create a new option, sendHeaders that defaults to blocklisting (False)

To think about:

does this also need to accept a custom function for modifying values?

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

glasser

I am a bit behind in my individual contributor work and probably can't take another look tomorrow.

CHANGELOG.md

docs/source/api/apollo-server.md

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

glasser · 2019-06-21T07:36:57Z

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

+
+// Helper for makeTraceDetails() to enforce that the keys of a modified 'variables'
+// matches that of the original 'variables'
+function cleanModifiedVariables(


I dunno that this is particularly necessary (if people want to send us weird data, they can send us weird data) but I'm not opposed to it.

@glasser @zionts It would actually be really nice to be able to add additional data to the variables. In my case I'm looking to add a userId so that I can identify particular users inside of apollo engine's reporting interface.

… fixing test cases

helenwh · 2019-06-24T17:07:58Z

THINGS LEFT TODO:

I updated the docs here, but I also will need to do these:

glasser

This is looking great. Just a few tweaks left. I'm really impressed by your tests!

docs/source/api/apollo-server.md

glasser · 2019-06-25T01:27:53Z

docs/source/api/apollo-server.md

+
+   NOTE: An error will be thrown if both this deprecated option and its replacement, `sendVariableValues` are defined.
+
+* `sendHeaders`: { exceptNames: Array&lt;String&gt; } | { includeNames: Array&lt;String&gt; } | { sendAll: boolean } | { sendNone: boolean }


Seems a little surprising to not include transform here too, though I suppose the main use case of transform is to tweak nested values which isn't relevant here.

I didn't include that option because I wasn't sure there was a need for it, maybe this is something that we could get feedback on? But otherwise I think this would be a relatively simple extension to make!

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

docs/source/api/apollo-server.md

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

zionts · 2019-06-19T18:02:17Z

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. 


I think to @glasser's point in #2472 (comment), we could give guidance to users on how to migrate to the new API without it being a breaking change if they're using the defaults.

Yeah I think Adam's right: this should explicitly say that this is a breaking change only if you don't currently specify privateVariables at all, and to preserve the old default, you'll want to pass new ApolloServer({engine: {sendVariableValues: {sendAll: true}}}). (I do think it's good to put it in the context of new ApolloServer since that's where people will actually write it.)

Hmm, is it odd that the top option name starts with send and some but not all of the sub-option names do? Should they be all and none instead?

I'll update the CHANGELOG to be more specific! Do you think it would be worth noting this in the docs specific to the deprecated options (privateVariables, privateHeaders) as well?

I think since the new option names already begin with send..., that the suboptions all and none should be clear enough. Will also make that change!

glasser

other than the last comment, lgtm! sorry for the delay.

glasser · 2019-06-26T00:32:41Z

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. 


Yeah I think Adam's right: this should explicitly say that this is a breaking change only if you don't currently specify privateVariables at all, and to preserve the old default, you'll want to pass new ApolloServer({engine: {sendVariableValues: {sendAll: true}}}). (I do think it's good to put it in the context of new ApolloServer since that's where people will actually write it.)

Hmm, is it odd that the top option name starts with send and some but not all of the sub-option names do? Should they be all and none instead?

glasser · 2019-06-26T18:18:49Z

Looks great!

…ifications to engine (#2847)" This reverts commit ec03745.

…ifications to engine (#2847)" (#2930) This reverts commit ec03745.

helenwh force-pushed the EnforcingPrivateVariables branch from 8fc0a03 to 7ed6bf6 Compare June 14, 2019 17:06

helenwh requested review from evans and glasser June 14, 2019 20:52

helenwh marked this pull request as ready for review June 14, 2019 21:02

helenwh force-pushed the EnforcingPrivateVariables branch from 7ed6bf6 to dbce299 Compare June 14, 2019 21:28

zionts reviewed Jun 14, 2019

View reviewed changes

helenwh force-pushed the EnforcingPrivateVariables branch 3 times, most recently from 7d557c1 to c5895c6 Compare June 17, 2019 17:00

zionts reviewed Jun 18, 2019

View reviewed changes

docs/source/api/apollo-server.md Outdated Show resolved Hide resolved

packages/apollo-engine-reporting/src/extension.ts Outdated Show resolved Hide resolved

glasser reviewed Jun 18, 2019

View reviewed changes

helenwh force-pushed the EnforcingPrivateVariables branch from 8e30644 to 1ebc472 Compare June 19, 2019 00:39

Changed new option to sendVariableValues instead of maskVariableValue…

862017e

…s, and the subsequent tests/docs

helenwh commented Jun 19, 2019

View reviewed changes

packages/apollo-engine-reporting/src/extension.ts Outdated Show resolved Hide resolved

helenwh commented Jun 19, 2019

View reviewed changes

packages/apollo-engine-reporting/src/extension.ts Outdated Show resolved Hide resolved

helenwh commented Jun 19, 2019

View reviewed changes

packages/apollo-engine-reporting/src/extension.ts Outdated Show resolved Hide resolved

helenwh commented Jun 19, 2019

View reviewed changes

packages/apollo-engine-reporting/src/extension.ts Outdated Show resolved Hide resolved

adding new reporting option sendHeaders, fixing some names

896c311

helenwh force-pushed the EnforcingPrivateVariables branch from bbebd83 to 896c311 Compare June 19, 2019 22:11

helenwh removed the request for review from evans June 19, 2019 22:13

glasser reviewed Jun 21, 2019

View reviewed changes

helenwh force-pushed the EnforcingPrivateVariables branch 4 times, most recently from 96d0038 to 203e7fa Compare June 21, 2019 22:00

helenwh force-pushed the EnforcingPrivateVariables branch 3 times, most recently from 48c4b06 to e04a997 Compare June 21, 2019 23:34

addressing comments, name/argument changes, updating docs, adding and…

8f9bbaf

… fixing test cases

helenwh force-pushed the EnforcingPrivateVariables branch from e04a997 to 8f9bbaf Compare June 24, 2019 16:34

helenwh changed the title ~~Adding enforcePrivateVariable parameter and specifications to engine …~~ Adding sendVariableValues and sendHeaders parameters and specifications to engine Jun 24, 2019

adding another parameter for specifying safelisted variable names

09946ae

helenwh force-pushed the EnforcingPrivateVariables branch from 2981485 to 09946ae Compare June 24, 2019 19:29

glasser requested changes Jun 25, 2019

View reviewed changes

more doc fixes, adding some checks for invalid inputs

cf1c21d

zionts reviewed Jun 25, 2019

View reviewed changes

glasser approved these changes Jun 26, 2019

View reviewed changes

changed suboption name, updating docs with more info on breaking change

db9c49f

Merge branch 'master' into EnforcingPrivateVariables

8636c3e

helenwh merged commit ec03745 into master Jun 26, 2019

helenwh pushed a commit that referenced this pull request Jun 26, 2019

Revert "Adding sendVariableValues and sendHeaders parameters and spec…

c7b6f7c

…ifications to engine (#2847)" This reverts commit ec03745.

This was referenced Jun 26, 2019

Revert "Adding sendVariableValues and sendHeaders parameters and specifications to engine" #2930

Merged

feat(apollo-engine-reportoing): sendVariableValues and sendHeaders. #2931

Merged

helenwh pushed a commit that referenced this pull request Jun 26, 2019

Revert "Adding sendVariableValues and sendHeaders parameters and spec…

5f06c22

…ifications to engine (#2847)" (#2930) This reverts commit ec03745.

github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding sendVariableValues and sendHeaders parameters and specifications to engine #2847

Adding sendVariableValues and sendHeaders parameters and specifications to engine #2847

helenwh commented Jun 14, 2019 •

edited

apollo-cla commented Jun 14, 2019

zionts left a comment

helenwh commented Jun 17, 2019 •

edited

glasser left a comment

helenwh commented Jun 19, 2019

helenwh commented Jun 19, 2019 •

edited

glasser left a comment

glasser Jun 21, 2019

aconran May 14, 2020

helenwh commented Jun 24, 2019 •

edited

glasser left a comment

glasser Jun 25, 2019

helenwh Jun 25, 2019

zionts Jun 19, 2019

glasser Jun 26, 2019

helenwh Jun 26, 2019

glasser left a comment

glasser Jun 26, 2019

glasser commented Jun 26, 2019


		NOTE: An error will be thrown if both this deprecated option and its replacement, `sendVariableValues` are defined.

		* `sendHeaders`: { exceptNames: Array<String> } \| { includeNames: Array<String> } \| { sendAll: boolean } \| { sendNone: boolean }

Adding sendVariableValues and sendHeaders parameters and specifications to engine #2847

Adding sendVariableValues and sendHeaders parameters and specifications to engine #2847

Conversation

helenwh commented Jun 14, 2019 • edited

apollo-cla commented Jun 14, 2019

zionts left a comment

Choose a reason for hiding this comment

helenwh commented Jun 17, 2019 • edited

glasser left a comment

Choose a reason for hiding this comment

helenwh commented Jun 19, 2019

helenwh commented Jun 19, 2019 • edited

glasser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helenwh commented Jun 24, 2019 • edited

glasser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasser commented Jun 26, 2019

helenwh commented Jun 14, 2019 •

edited

helenwh commented Jun 17, 2019 •

edited

helenwh commented Jun 19, 2019 •

edited

helenwh commented Jun 24, 2019 •

edited