feat(apollo-engine-reportoing): sendVariableValues and sendHeaders.
#2931
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zionts
approved these changes
Jun 26, 2019
abernix
requested changes
Jun 27, 2019
There was a problem hiding this comment.
I've got a number of nits, a couple documentation formatting things, and just one thing in particular that could be problematic down the line we don't change it now with regards to using object literals. I do think most of the TODO items should/could be quelled?
That said, let's get this into 2.7.0 alpha today!
| // This probably means that the value contains a circular reference, | ||
| // causing `JSON.stringify()` to throw a TypeError: | ||
| // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#Issue_with_JSON.stringify()_when_serializing_circular_references | ||
| details.variablesJson![name] = JSON.stringify( |
There was a problem hiding this comment.
Let's be defensive and make sure that it does! (and maybe just use the standardized error message?)
if (e.name === 'TypeError' && e.message === 'Converting circular structure to JSON') {
details.variablesJson![name] = JSON.stringify(e.message);
} else {
// Re-throw when it doesn't meet our expectation so we don't
// inadvertently swallow anything as a "cycle error" when its not.
throw e;
}There was a problem hiding this comment.
Is there a good way to check against the standardized error message? It looks like it differs per browser:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Cyclic_object_value
helenwh
force-pushed
the
EnforcingPrivateVariables
branch
4 times, most recently
from
90ef77c
to
20ac4db
Compare
…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
…s, and the subsequent tests/docs
… fixing test cases
…omparisons, constant declarations), more test cases Co-Authored-By: Jesse Rosenberger <git@jro.cc>
abernix
force-pushed
the
EnforcingPrivateVariables
branch
from
9ece335
to
c8655ef
Compare
abernix
force-pushed
the
EnforcingPrivateVariables
branch
from
c8655ef
to
8f9ca40
Compare
Node.js only raises this particular error when cycles are detected. While I first thought it was more defensive to catch the exact error we anticipated, I'm slightly reconsidering whether this is defensive enough and if we should, in fact, change this back to catching any error, particularly since this runs async and might go undetected or cause a whole string of a user's errors to not pass any variables. Thoughts, @glasser?
I thought it made sense to catch only cycles, but I've changed my mind. I think there could be just arbitrary errors in a `toString` or `toJSON` implementation which we'd still want to guard, particularly because these errors are shipped async. Ref: 4b2f9d3 Ref: #2931 (comment)
abernix
approved these changes
Jun 27, 2019
There was a problem hiding this comment.
LGTM! Thanks for working on this!
This is a wonderful PR with excellent test coverage and documentation: A huge improvement over the previous privateVariables implementation.
Note that I rescinded on one of my suggestions in #2931 (comment) and reverted it back to how you had it. See 4b2f9d3 and 6c9aa8f for the follow-ups and thoughts that led to that.
abernix
changed the title
sendVariableValues and sendHeaders.
|
This was referenced Jun 28, 2019
helenwh
pushed a commit
that referenced
this pull request
Jul 17, 2019
…Option (#3049) * fix error handling bug in `sendVariableValues` EngineReportingOption * fixing docs to describe error more precisely Co-Authored-By: Jesse Rosenberger <git@jro.cc> * updating changelog w/ info on error fix, adding a line to the 2.7.0 notes on the `sendHeaders` option that was missed in PR #2931
abernix
added a commit
that referenced
this pull request
Apr 14, 2020
This APQ flag reporting was almost surely changed incorrectly in #2931 and not caught during review, causing this APQ boolean to not be properly set when the request was, in fact, an APQ request, unless `sendHeaders` was enabled - which it is _not_ by default after the change that released it. Ref: https://github.com/apollographql/apollo-server/pull/2931/files#diff-24ae042112f9dcc18543ac851c4d8f7cR97-R102
abernix
added a commit
that referenced
this pull request
Apr 14, 2020
This APQ flag reporting was almost surely changed incorrectly in #2931 and not caught during review, causing this APQ boolean to not be properly set when the request was, in fact, an APQ request, unless `sendHeaders` was enabled - which it is _not_ by default after the change that released it. Ref: https://github.com/apollographql/apollo-server/pull/2931/files#diff-24ae042112f9dcc18543ac851c4d8f7cR97-R102
abernix
added a commit
that referenced
this pull request
Apr 15, 2020
This APQ flag reporting was almost surely changed incorrectly in #2931 and not caught during review, causing this APQ boolean to not be properly set when the request was, in fact, an APQ request, unless `sendHeaders` was enabled - which it is _not_ by default after the change that released it. Ref: https://github.com/apollographql/apollo-server/pull/2931/files#diff-24ae042112f9dcc18543ac851c4d8f7cR97-R102
1 task
abernix
added a commit
that referenced
this pull request
Apr 16, 2020
* fix: report APQ hit/registration when `sendHeaders` is disabled. This APQ flag reporting was almost surely changed incorrectly in #2931 and not caught during review, causing this APQ boolean to not be properly set when the request was, in fact, an APQ request, unless `sendHeaders` was enabled - which it is _not_ by default after the change that released it. Ref: https://github.com/apollographql/apollo-server/pull/2931/files#diff-24ae042112f9dcc18543ac851c4d8f7cR97-R102 * Add CHANGELOG.md for #3986.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reopening: #2847
After the revert: #2930
(Copied from PR #2847)
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
privateVariablesAdding a new parameter,
sendHeaders, which defaults to a blocklist as well, but otherwise preserves properties ofprivateHeadersDEPRECATING
privateHeadersDocs preview: https://deploy-preview-2937--apollo-server-docs.netlify.com/docs/apollo-server/api/apollo-server/#enginereportingoptions
[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?