Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make query sanitization more extensible #3167

Merged
merged 1 commit into from
Sep 29, 2020

Conversation

jturkel
Copy link
Contributor

@jturkel jturkel commented Sep 23, 2020

We (Salsify) are implementing logging of GraphQL queries and variables but our approach is slightly different than the default behavior of GraphQL::Language::SanitizedPrinter:

  • We log queries and variables separately to get a more faithful representation of the requests we're getting from clients
  • We extended our GraphQL::Schema::Argument subclass to support a redact option rather than just redacting all strings

We were able to implement this by subclassing GraphQL::Language::SanitizedPrinter but it involved some unfortunate copying of code from GraphQL::Language::Printer to restore overridden behavior and copying of code from GraphQL::Language::SanitizedPrinter where we need to inject logic in the middle of a method. Alternatively we could directly subclass GraphQL::Language::Printer but then we would have lost the type bookkeeping from GraphQL::Language::SanitizedPrinter.

This PR proposes a few changes to GraphQL::Language::SanitizedPrinter to make it easier to customize the behavior:

  1. Making the inlining of variable values optional with a default that retains the current behavior
  2. Adding redact_argument_value? and redacted_argument_value hooks that subclasses can use to more easily customize the redaction behavior

@jturkel jturkel force-pushed the feature/sanitization-improvements branch from 2b4dbe8 to 7366d90 Compare September 29, 2020 16:24
@jturkel
Copy link
Contributor Author

jturkel commented Sep 29, 2020

Rebased to resolve the conflicts with #3171.

@rmosolgo
Copy link
Owner

Thanks, these changes look great!

@rmosolgo rmosolgo added this to the 1.11.5 milestone Sep 29, 2020
@rmosolgo rmosolgo merged commit 462ba26 into rmosolgo:master Sep 29, 2020
@jturkel
Copy link
Contributor Author

jturkel commented Sep 30, 2020

Thanks @rmosolgo. Any plans for a 1.11.5 release soon?

@rmosolgo
Copy link
Owner

Any plans

I don't usually plan for point releases, but feel free to ask anytime!

🚢 💁‍♂️ https://rubygems.org/gems/graphql/versions/1.11.5

@jturkel
Copy link
Contributor Author

jturkel commented Sep 30, 2020

Woohoo! Thanks @rmosolgo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants