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

Convert Absinthe.Language.* structs to graphql representation #1114

Conversation

maartenvanvliet
Copy link
Contributor

This PR adds the inspect protocol for the Absinthe.Language.Document struct.

A parsed graphql document can now be converted back to its graphql string representation. This gives the possible for a few new features:

It's not necessary for the execution of a document to preserve
this. For converting Absinthe.Language AST to a string it is however.

Note that it's not a goal that the document -> AST -> document
transformation is perfect in all cases. E.g. commas or lack thereof is
not preserved. The shorthand notation however seemed easy enough
to implement.

See https://spec.graphql.org/draft/#sec-Language.Operations.Query-shorthand
Note it doesn't implement the Mix.Tasks.Format behaviour since this
would give a warning pre Elixir 1.13
@binaryseed
Copy link
Contributor

This is amazing! I'll take a closer look in the next few days..

@benwilson512
Copy link
Contributor

Does the Elixir formatter support different formats for sigils? Would this let us do something like:

~A"""
{ foo {  bar } } 
"""

@maartenvanvliet
Copy link
Contributor Author

Interesting, so you could apply different formatting rules depending on whether it's a file or a sigil.

Its not directly available right now but it can be inferred. The opts argument to format/2 is passed something like the following:

[
  file: "some_mutation_test.exs",
  plugins: [Your.Formatter],
  import_deps: [:absinthe],
  inputs: ["*.{ex,exs}", "priv/*/seeds.exs", "{config,lib,test}/**/*.{ex,exs}", "{test, lib, priv}/**/*.{gql,graphql}"],
  subdirectories: ["priv/*/migrations"],
  locals_without_parens: [
    mutation: 2,
    query: 1,
    subscription: 2,
    ...
  ]
]

Now you can infer that because the extension of the :file entry is not .graphql we are dealing with a sigil instead of a .graphql file. Which sigil or if any sigil modifiers are applied is not available.

@maartenvanvliet
Copy link
Contributor Author

PR for changing the formatter plugins was merged, so the sigil information is now passed through to the formatter :)

Copy link
Contributor

@binaryseed binaryseed left a comment

Choose a reason for hiding this comment

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

This is looking really great!

Would you mind reviewing the test cases in this repo:

At New Relic, we built a query renderer but it didn't use the Algebra. We did however, run into a variety of edge cases over time and fixed them each alongside tests. If all those test queries render nicely, I think we'll be good to go on this!

"""

def features(_opts) do
[sigils: [], extensions: [".graphql", ".gql"]]
Copy link
Contributor

@binaryseed binaryseed Nov 4, 2021

Choose a reason for hiding this comment

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

Is there a use case for a :G / :A sigil here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since Absinthe does not define a sigil of its own I've left it out here. It's fairly simple for users to define their own formatter plugin and include any custom sigils.

lib/absinthe/language/render.ex Outdated Show resolved Hide resolved
@maartenvanvliet
Copy link
Contributor Author

I've run the documents in https://github.com/newrelic/absinthe-schema-stitching-example/blob/master/apps/schema_stitch/test/query_generator_test.exs and they render nicely :)

The absinthe parser also accepts SDL. Rendering the Language.* structs for SDL however is not implemented yet. For completeness I'll see if I can do that as a followup.

@opsb
Copy link

opsb commented Nov 8, 2021

This looks super useful. We've been wanting to sanitise queries for logging so I started down the road of creating a renderer for the blueprint structs. My plan is to allow fields to be whitelisted for logging in the schema e.g.

input_object :update_user_input do
  field :id, non_null(:id)
  field :email, :string, meta: [log: true]
  field :first_name, :string
  field :last_name, :string
end

The reason I'd thought to use the blueprint structs rather than the language structs is that they have the schema_nodes populated so it's possible to query the meta and determine if a field should be included in the rendered output or not. It seems excessive to have renderers both for the blueprint and language structs so I'm now pondering what the best path forwards is. Given this renderer exists, what's the best approach for log masking?

@benwilson512
Copy link
Contributor

We've been wanting to sanitise queries for logging so I started down the road of creating a renderer for the blueprint structs.

Can you elaborate on this? Sensitive parameters from end users should always be passed in as variables, and this already supports sanitization.

@opsb
Copy link

opsb commented Nov 9, 2021

Hey @benwilson512, our absinthe API is client facing so we're not building the queries ourselves. Our position is that the spec supports inline arguments and variables so we should support them too. We do encourage using variables but for instance when exploring our API for the first time it's nicer to be able to just using arguments inline.

@benwilson512
Copy link
Contributor

Our position is that the spec supports inline arguments and variables so we should support them too

That's fair, but understand that there are reasonable limits to what this sport entails. For example, it is much easier to have syntax errors in the GraphQL than in JSON, and if the GraphQL has syntax errors then it can't easily be stripped of sensitive information. This is particularly complex because Absinthe.Plug logs values prior to parsing, so this is not a simple change.

@opsb I think this question should be followed up in an issue on Absinthe.Plug after this PR is merged, since I think it is largely orthogonal to getting this PR over the finish line.

@opsb
Copy link

opsb commented Nov 9, 2021

@benwilson512 that's an interesting point about syntax errors - possibly we'd just not log the query at all in that case. I think we'd probably skip the built in logger and introduce our own logging phase further on. But yeah, agree it make sense to explore this more in a follow on ticket.

@binaryseed
Copy link
Contributor

@benwilson512 think this is ready for merge & release?

@benwilson512 benwilson512 merged commit d7837a7 into absinthe-graphql:master Nov 11, 2021
@benwilson512
Copy link
Contributor

❤️ thank you @maartenvanvliet !

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.

None yet

4 participants