-
Notifications
You must be signed in to change notification settings - Fork 529
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
Convert Absinthe.Language.*
structs to graphql representation
#1114
Conversation
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
This is amazing! I'll take a closer look in the next few days.. |
Does the Elixir formatter support different formats for sigils? Would this let us do something like:
|
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
Now you can infer that because the extension of the |
PR for changing the formatter plugins was merged, so the sigil information is now passed through to the formatter :) |
There was a problem hiding this 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"]] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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. |
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.
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 |
Can you elaborate on this? Sensitive parameters from end users should always be passed in as variables, and this already supports sanitization. |
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. |
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. |
@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. |
@benwilson512 think this is ready for merge & release? |
❤️ thank you @maartenvanvliet ! |
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:
Absinthe.Formatter
for an example.