Add pluggable executor #2574
Merged
Add pluggable executor #2574
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
This adjusts the request pipeline to format errors before calling `willSendResponse`, and adds a `didEncounterErrors` hook to give the Engine reporting extension access to the raw errors.
martijnwalraven
approved these changes
Apr 14, 2019
4 tasks
abernix
added a commit
that referenced
this pull request
Apr 28, 2019
This is no longer necessary after the changes in @jbaxleyiii's #2574 (and @martijnwalraven's #2416), which changed the error reporting to be captured via the newly introduced `didEncounterErrors`, rather than in `willResolveField` where it used to happen. Yay!
abernix
added a commit
that referenced
this pull request
Apr 30, 2019
…s for reporting. (#2618) * Add errorFilter option * update changelog * Rename errorFilter to filterErrors, return GraphQLError or null * Add back `maskErrorDetails` type to maintain support. ...though it's currently not functional as of this commit. * Introduce additional tests for `filterErrors` and `maskErrorDetails`. Even though `maskErrorDetails` is going away, it seems important to continue to test it along with the new functionality introduced by `filterErrors`. Note: These tests highlight some failures in the current `filterErrors` logic which need to be addressed in follow-up commits. * Finish up new `filterErrors` functionality, sans renaming to a new name. The intention is to rename the `filterErrors` function to `rewriteError` in a follow-up commit, but this maintains the previous name during the re-implementation phase. * Add a test that ensures that the `stack` is not transmitted. The new implementation of `rewriteError` (previously `formatErrors` and prior to that `maskErrorDetails`, do nothing to ensure that the `stack` property (i.e. `Error.prototype.stack`) is regenerated using the new combination of `${err.name}: ${err.message`, suffixed with the rest of `Error.captureStackTrace`. That's okay, but we should at least guard against that and make sure that no future code starts to add the `stack` property, without properly redacting it within `rewriteError` internals. That redaction is _slightly_ less than performant (string manipulation), so it didn't seem worth the effort, since we don't actually send it anywhere. * Rename new `filterErrors` function to a more accurate `rewriteError`. While the new `filterErrors` functionality did allow complete filtering of errors prior to reporting them to Apollo Engine, it also provides the ability to change the error entirely. This more thorough functionality deserves a name which more accurately defines its functionality. The use-defined `rewriteError` function (defined within the `engine` configuration on the `ApolloServer` constructor options) can still be used to "filter" an error from going to Engine by re-writing the error to `null` To accomplish this nullification of an error, an explicit `null` can be returned from the `rewriteError` function rather than the normal practice of returning a `GraphQLError` (in a presumably modified form). * Adjust the line lengths of some comments. Because prettier doesn't do it! * Add CHANGELOG.md for `rewriteError`. * Update deprecated to use rewriteError * Update rewriteError in docs - Deprecate `formatError` in docs in favor of `rewriteError` - lint-fix * Remove now-unnecessary guard which checks for `errors`. This is no longer necessary after the changes in @jbaxleyiii's #2574 (and @martijnwalraven's #2416), which changed the error reporting to be captured via the newly introduced `didEncounterErrors`, rather than in `willResolveField` where it used to happen. Yay! * Remove long-skipped test that was never close to working. I don't think this is providing much value at this point and it's been skipped for over a year. It appears that the test was intended to test `reportErrorFunction`, though the description claims that it's trying to test something else entirely. Regardless, the testing of traces is actually handled elsewhere now, so this is good to remove. * Fix test structure after merge. * Partially Revert "Update rewriteError in docs" This partially reverts commit 21b8b86, which inadvertently changed `formatError`, which is a top-level function which can be used to adjust the response to the client, which is intrinsically different than the new `rewriteError` which modifies the shape of the error in the tracing data which is sent to the Apollo Platform. * Update the API section with more clarity. * Update docs for rewriteError. (#2585) * (docs) Add information on `rewriteError` to the `errors.md`. This adds additional clarity fro the new functionality provided in #1639. * Fix syntax error and use `String.prototype.startsWith` rather `St.pr.match`. Regular expressions are just not as approachable! * Update errors example. The previous example here wasn't a wonderful example since it was suggesting a pattern which is unnecessary when `NODE_ENV=production`, where stack traces are automatically hidden. * Update errors.md * Update errors.md * Update CHANGELOG.md Co-authored-by: GerA <lpgera@gmail.com>
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.
This is a rebase of
add-plugable-executorby @martijnwalraven from #2416