[graphql/rpc] allow @Skip and @include directives (#15570)

gonative-cc · Jan 5, 2024 · 92032e7 · 92032e7
1 parent 8cccebb
commit 92032e7
Show file tree

Hide file tree

Showing 3 changed files with 88 additions and 26 deletions.
diff --git a/crates/sui-graphql-e2e-tests/tests/limits/directives.exp b/crates/sui-graphql-e2e-tests/tests/limits/directives.exp
@@ -1,4 +1,4 @@
-processed 3 tasks
+processed 7 tasks
 
 init:
 A: object(0,0)
@@ -8,11 +8,11 @@ Response: {
   "data": null,
   "errors": [
     {
-      "message": "Fields with directives are not supported",
+      "message": "Directive `@deprecated` is not supported. Supported directives are `@include`, `@skip`",
       "locations": [
         {
           "line": 2,
-          "column": 3
+          "column": 19
         }
       ],
       "extensions": {
@@ -27,11 +27,11 @@ Response: {
   "data": null,
   "errors": [
     {
-      "message": "Fragments with directives are not supported",
+      "message": "Directive `@deprecated` is not supported. Supported directives are `@include`, `@skip`",
       "locations": [
         {
           "line": 1,
-          "column": 1
+          "column": 29
         }
       ],
       "extensions": {
@@ -40,3 +40,27 @@ Response: {
     }
   ]
 }
+
+task 3 'run-graphql'. lines 42-46:
+Response: {
+  "data": {}
+}
+
+task 4 'run-graphql'. lines 48-52:
+Response: {
+  "data": {
+    "chainIdentifier": "d0ebf23a"
+  }
+}
+
+task 5 'run-graphql'. lines 54-58:
+Response: {
+  "data": {
+    "chainIdentifier": "d0ebf23a"
+  }
+}
+
+task 6 'run-graphql'. lines 60-64:
+Response: {
+  "data": {}
+}
diff --git a/crates/sui-graphql-e2e-tests/tests/limits/directives.move b/crates/sui-graphql-e2e-tests/tests/limits/directives.move
@@ -38,3 +38,27 @@ fragment Modules on Object  @deprecated {
         }
     }
 }
+
+//# run-graphql
+
+{
+  chainIdentifier @skip(if: true)
+}
+
+//# run-graphql
+
+{
+  chainIdentifier @skip(if: false)
+}
+
+//# run-graphql
+
+{
+  chainIdentifier @include(if: true)
+}
+
+//# run-graphql
+
+{
+  chainIdentifier @include(if: false)
+}
diff --git a/crates/sui-graphql-rpc/src/extensions/query_limits_checker.rs b/crates/sui-graphql-rpc/src/extensions/query_limits_checker.rs
@@ -10,6 +10,7 @@ use crate::error::graphql_error_at_pos;
 use crate::metrics::RequestMetrics;
 use async_graphql::extensions::NextParseQuery;
 use async_graphql::extensions::NextRequest;
+use async_graphql::parser::types::Directive;
 use async_graphql::parser::types::ExecutableDocument;
 use async_graphql::parser::types::FragmentDefinition;
 use async_graphql::parser::types::Selection;
@@ -28,6 +29,8 @@ use async_graphql::{
 use axum::headers;
 use axum::http::HeaderName;
 use axum::http::HeaderValue;
+use once_cell::sync::Lazy;
+use std::collections::BTreeSet;
 use std::collections::HashMap;
 use std::collections::VecDeque;
 use std::sync::Arc;
@@ -221,13 +224,7 @@ impl QueryLimitsChecker {
 
                 match &curr_sel.node {
                     Selection::Field(f) => {
-                        if !f.node.directives.is_empty() {
-                            return Err(graphql_error_at_pos(
-                                INTERNAL_SERVER_ERROR,
-                                "Fields with directives are not supported",
-                                f.pos,
-                            ));
-                        }
+                        check_directives(&f.node.directives)?;
                         for field_sel in f.node.selection_set.node.items.iter() {
                             que.push_back(field_sel);
                             cost.num_nodes += 1;
@@ -250,27 +247,15 @@ impl QueryLimitsChecker {
                         // TODO: this is inefficient as we might loop over same fragment multiple times
                         // Ideally web should cache the costs of fragments we've seen before
                         // Will do as enhancement
-                        if !frag_def.node.directives.is_empty() {
-                            return Err(graphql_error_at_pos(
-                                INTERNAL_SERVER_ERROR,
-                                "Fragments with directives are not supported",
-                                frag_def.pos,
-                            ));
-                        }
+                        check_directives(&frag_def.node.directives)?;
                         for frag_sel in frag_def.node.selection_set.node.items.iter() {
                             que.push_back(frag_sel);
                             cost.num_nodes += 1;
                             check_limits(limits, cost.num_nodes, cost.depth, Some(frag_sel.pos))?;
                         }
                     }
                     Selection::InlineFragment(fs) => {
-                        if !fs.node.directives.is_empty() {
-                            return Err(graphql_error_at_pos(
-                                INTERNAL_SERVER_ERROR,
-                                "Inline fragments with directives are not supported",
-                                fs.pos,
-                            ));
-                        }
+                        check_directives(&fs.node.directives)?;
                         for in_frag_sel in fs.node.selection_set.node.items.iter() {
                             que.push_back(in_frag_sel);
                             cost.num_nodes += 1;
@@ -314,3 +299,32 @@ fn check_limits(limits: &Limits, nodes: u32, depth: u32, pos: Option<Pos>) -> Se
 
     Ok(())
 }
+
+// TODO: make this configurable
+fn allowed_directives() -> &'static BTreeSet<&'static str> {
+    static DIRECTIVES: Lazy<BTreeSet<&str>> =
+        Lazy::new(|| BTreeSet::from_iter(["skip", "include"]));
+
+    Lazy::force(&DIRECTIVES)
+}
+
+fn check_directives(directives: &[Positioned<Directive>]) -> ServerResult<()> {
+    for directive in directives {
+        if !allowed_directives().contains(&directive.node.name.node.as_str()) {
+            return Err(graphql_error_at_pos(
+                INTERNAL_SERVER_ERROR,
+                format!(
+                    "Directive `@{}` is not supported. Supported directives are {}",
+                    directive.node.name.node,
+                    allowed_directives()
+                        .iter()
+                        .map(|s| format!("`@{}`", s))
+                        .collect::<Vec<_>>()
+                        .join(", ")
+                ),
+                directive.pos,
+            ));
+        }
+    }
+    Ok(())
+}