AST analyzers #1841

xuorig · 2018-09-07T15:58:41Z

No description provided.

rmosolgo · 2018-09-07T16:27:38Z

Some quick feedback here: it's important that we keep supporting existing Analysis for people who don't opt into the new interpreter, so new-style analyzers should be implemented as a brand new code path, leaving the existing code in place.

RobertWSaunders

This looks awesome! Noticed one small thing while reading your docs!

guides/language_tools/visitor.md

rmosolgo

Taking a high-level look, this looks really solid! Lots of contextual helpers for an AST visit.

rmosolgo · 2018-10-05T19:19:55Z

spec/graphql/analysis/query_depth_spec.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 require "spec_helper"

-describe GraphQL::Analysis::QueryDepth do
+describe GraphQL::Analysis::AST::QueryDepth do


Should this have AST::?

lib/graphql/analysis/ast/visitor.rb

rmosolgo · 2018-10-05T19:26:46Z

lib/graphql/analysis/ast/query_complexity.rb

+          super
+          @complexities_on_type = [TypeComplexity.new]
+          @skip_stack = [false]
+          @in_fragment_def = false


It looks like some of these IVars were moved to the parent class, is that right?

lib/graphql/analysis/ast/visitor.rb

rmosolgo · 2018-10-05T19:29:32Z

lib/graphql/analysis/ast/query_depth.rb

+
+        def on_leave_fragment_spread(node, _, visitor)
+          visitor.leave_fragment_spread_inline(node)
+        end


It'll be interesting to see if we need this more, maybe a kind of base class that defaults to this handling of fragments will come in handy.

💯 Totally. I was unsure if we needed this now. The base class idea sounds good. If a more "raw" approach is needed they could use this method to do it, but the more common default definitly sounds more like visiting the fragment spreads inline.

lib/graphql/analysis/ast.rb

xuorig · 2018-10-06T19:07:46Z

Worked on exposing this new API.

To keep the existing analysis working, it is possible to switch the analysis engine at the schema level. The new AST analysis engine is opt in, like this:

class MySchema < GraphQL::Schema
  use GraphQL::Analysis::AST
  query_analyzer MyASTAnalyzer 
end

The same hook is used to add AST analyzers or old analyzers, but the selected engine treats them how it wants. It might be good to eventually add a way to select the engine at the validation pipeline / query level. Might be useful to A/B test while migrating for example.

xuorig · 2018-10-06T20:18:44Z

Tests mostly pass except a weird case with a prepare proc returning an execution error during analysis, not sure how this was handled before, will debug on monday.

Edit: ah ok, the authorization analyzer was only added when you used class based before, I need to modify the validation pipeline to check for that 🤔

…ngine

rmosolgo

Looks good! I just had a few questions about the test setup and I think we're set to merge.

If we stick to the plan of removing the authorization analyzer, I'll update the docs later.

rmosolgo · 2018-10-09T16:03:35Z

lib/graphql/language/visitor.rb

-      alias :on_union_type_extension :on_abstract_node
-      alias :on_variable_definition :on_abstract_node
-      alias :on_variable_identifier :on_abstract_node
+      # Don't use make_visit_method becuase it breaks `super`


This should probably be "Don't use alias because it breaks super, right?

rmosolgo · 2018-10-09T16:05:21Z

lib/graphql/schema.rb

@@ -159,6 +160,7 @@ def initialize
      @lazy_methods.set(GraphQL::Execution::Lazy, :value)
      @cursor_encoder = Base64Encoder
      # Default to the built-in execution strategy:
+      @analysis_engine = GraphQL::Analysis


👍 nice approach to generalizing this

rmosolgo · 2018-10-09T16:07:32Z

.travis.yml

@@ -43,6 +43,7 @@ matrix:
  - env:
      - DISPLAY=':99.0'
      - TESTING_INTERPRETER=yes
+      - TESTING_AST_ANALYSIS=yes


This test case only runs the ActionCable subscription tests from spec/dummy, did you mean to set this flag for those tests? I don't see the flag used by those tests.

rmosolgo · 2018-10-09T16:07:51Z

spec/graphql/authorization_spec.rb

@@ -372,6 +372,9 @@ class Schema < GraphQL::Schema
      if TESTING_INTERPRETER
        use GraphQL::Execution::Interpreter
      end
+      if TESTING_AST_ANALYSIS
+        use GraphQL::Analysis::AST
+      end


Should this be removed, since we're bailing on that analyzer? (If it should be removed, then I think we can remove TESTING_AST_ANALYSIS altogether, right?)

yes and yes!

…nalyzers

xuorig · 2018-10-10T15:09:03Z

🍏 ! Ready for an other round or reviews @rmosolgo 🙏

rmosolgo

👏 🎉 !

rmosolgo force-pushed the 1.9-dev branch from bd9961d to 5098895 Compare September 7, 2018 21:09

RobertWSaunders reviewed Sep 10, 2018

View reviewed changes

guides/language_tools/visitor.md Outdated Show resolved Hide resolved

rmosolgo added this to the 1.9.0 milestone Sep 13, 2018

Marc-Andre Giroux added 2 commits September 13, 2018 15:35

add some tests

dc1e315

well this kinda works

190637a

xuorig force-pushed the ast-analyzers branch from 89957d7 to 190637a Compare September 13, 2018 19:40

Marc-Andre Giroux added 4 commits September 13, 2018 16:07

clean up a bit

d2b0a04

making existing stuff work

d73a4ba

query complexity works 🎉

8e78480

add tests for max query complexity and multiplex

31d494d

xuorig changed the title ~~Big WIP: spike at AST analyzers~~ AST analyzers Oct 1, 2018

Marc-Andre Giroux added 5 commits October 2, 2018 16:36

convert query depth

c65c3ec

use schema graphql definition in test

c0031f4

convert field usage analyzer

a29fc31

✅

de12c78

rubocop

ec3bedd

rmosolgo reviewed Oct 5, 2018

View reviewed changes

rmosolgo mentioned this pull request Oct 5, 2018

Release 1.9.0 #1884

Closed

19 tasks

swalkinshaw reviewed Oct 5, 2018

View reviewed changes

lib/graphql/analysis/ast.rb Show resolved Hide resolved

Marc-Andre Giroux added 10 commits October 5, 2018 18:06

fix spec for old analysis

eb4d17b

ivars moved to visitor class

a175e39

analyze selected operation only

3f34bd7

Merge branch '1.9-dev' into ast-analyzers

3089d17

fix multiplex analyze method signature

22b2471

a bit of documentation

b285f37

fix spec

8fbee06

preliminary guide

8fa7e33

add a per schema analysis engine

d551ec7

detect analysis engine in validation pipeline

2e1759f

xuorig force-pushed the ast-analyzers branch from a0c64c0 to 2e1759f Compare October 6, 2018 18:38

Marc-Andre Giroux added 3 commits October 6, 2018 14:52

authorization analyzer in validation pipeline

ed2d1c3

document new way to use ast analyzers

3c6222c

typo

24e998b

Marc-Andre Giroux added 2 commits October 6, 2018 15:36

test authorization with both ast and not using constant

2831983

remove focus

c592354

Marc-Andre Giroux added 3 commits October 6, 2018 16:19

fix schema api

b3dcc3f

deprecate authorization analyzer, filter it out when using analysis e…

824affe

…ngine

rm focus

40e1711

rmosolgo reviewed Oct 9, 2018

View reviewed changes

Marc-Andre Giroux added 2 commits October 9, 2018 17:25

remove the testing constant, and fix comment

8002816

Merge branch '1.9-dev' of github.com:rmosolgo/graphql-ruby into ast-a…

446ab42

…nalyzers

rmosolgo approved these changes Oct 10, 2018

View reviewed changes

rmosolgo merged commit 4ca8545 into rmosolgo:1.9-dev Oct 10, 2018

xuorig deleted the ast-analyzers branch October 10, 2018 20:03

rmosolgo mentioned this pull request Feb 8, 2023

Remove unused accessible? methods #4336

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AST analyzers #1841

AST analyzers #1841

xuorig commented Sep 7, 2018

rmosolgo commented Sep 7, 2018

RobertWSaunders left a comment

rmosolgo left a comment

rmosolgo Oct 5, 2018

rmosolgo Oct 5, 2018

rmosolgo Oct 5, 2018

xuorig Oct 5, 2018

xuorig commented Oct 6, 2018 •

edited

Loading

xuorig commented Oct 6, 2018 •

edited

Loading

rmosolgo left a comment

rmosolgo Oct 9, 2018

rmosolgo Oct 9, 2018

rmosolgo Oct 9, 2018

rmosolgo Oct 9, 2018

xuorig Oct 9, 2018

xuorig commented Oct 10, 2018

rmosolgo left a comment

AST analyzers #1841

AST analyzers #1841

Conversation

xuorig commented Sep 7, 2018

rmosolgo commented Sep 7, 2018

RobertWSaunders left a comment

Choose a reason for hiding this comment

rmosolgo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuorig commented Oct 6, 2018 • edited Loading

xuorig commented Oct 6, 2018 • edited Loading

rmosolgo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuorig commented Oct 10, 2018

rmosolgo left a comment

Choose a reason for hiding this comment

xuorig commented Oct 6, 2018 •

edited

Loading

xuorig commented Oct 6, 2018 •

edited

Loading