required-on-nonnull-field checking to consider fields in sibling inline fragments

Reviewed By: captbaritone

Differential Revision: D77563673

fbshipit-source-id: ae23cc510427ecb28b287f94f07a0887714d70e5

Author: gordyf

compiler/crates/relay-transforms/src/validations/disallow_required_on_non_null_field.rs

@@ -5,13 +5,15 @@
  * LICENSE file in the root directory of this source tree.
  */
 
+use std::collections::HashMap;
 use std::sync::Arc;
 
 use ::errors::try_all;
 use common::Diagnostic;
 use common::DiagnosticTag;
 use common::DiagnosticsResult;
 use common::DirectiveName;
+use common::Location;
 use common::NamedItem;
 use errors::try2;
 use graphql_ir::Field;
@@ -20,6 +22,7 @@ use graphql_ir::Program;
 use graphql_ir::Selection;
 use graphql_ir::Validator;
 use graphql_ir::reexport::Intern;
+use intern::string_key::StringKey;
 use lazy_static::lazy_static;
 use schema::SDLSchema;
 use schema::Schema;
@@ -47,19 +50,53 @@ pub fn disallow_required_on_non_null_field(program: &Program) -> DiagnosticsResu
     }
 }
 
+type FieldPath = Vec<StringKey>;
+
 struct DisallowRequiredOnNonNullField<'a> {
     schema: &'a Arc<SDLSchema>,
     warnings: Vec<Diagnostic>,
+    path: FieldPath,
+    modifiable_fields: HashMap<FieldPath, Action>,
 }
 
 impl<'a> DisallowRequiredOnNonNullField<'a> {
     fn new(schema: &'a Arc<SDLSchema>) -> Self {
         Self {
             schema,
             warnings: vec![],
+            path: vec![],
+            modifiable_fields: HashMap::new(),
         }
     }
 
+    // Tracks field required-directive removal eligibility. If a field's required directive,
+    // is not removable, it remains not removable forever. Otherwise it is removable and we
+    // accumulate the locations where it can be removed... unless it becomes not removable
+    // later on.
+    fn update_field_action(&mut self, new_action: Action) {
+        let mut path = self.path.clone();
+        path.reverse();
+        let existing_action = self.modifiable_fields.get(&path);
+        self.modifiable_fields.insert(
+            path,
+            match existing_action {
+                None => new_action,
+                Some(Action::NotRemovable) => {
+                    // already not removable, keep it that way
+                    Action::NotRemovable
+                }
+                Some(Action::Removable(list)) => match new_action {
+                    Action::NotRemovable => Action::NotRemovable,
+                    Action::Removable(new_list) => {
+                        let mut new_list = new_list.clone();
+                        new_list.extend(list.clone());
+                        Action::Removable(new_list)
+                    }
+                },
+            },
+        );
+    }
+
     fn validate_required_field(
         &mut self,
         field: &Arc<impl Field>,
@@ -81,11 +118,10 @@ impl<'a> DisallowRequiredOnNonNullField<'a> {
             .type_
             .is_non_null()
         {
-            self.warnings.push(Diagnostic::hint_with_data(
-                ValidationMessageWithData::RequiredOnNonNull,
-                required_directive.unwrap().location,
-                vec![DiagnosticTag::UNNECESSARY],
-            ));
+            self.update_field_action(Action::Removable(vec![Message {
+                message: ValidationMessageWithData::RequiredOnNonNull,
+                location: required_directive.unwrap().location,
+            }]));
         } else if self
             .schema
             .field(field.definition().item)
@@ -94,11 +130,12 @@ impl<'a> DisallowRequiredOnNonNullField<'a> {
             .is_some()
         {
             // @required on a semantically-non-null field is unnecessary
-            self.warnings.push(Diagnostic::hint_with_data(
-                ValidationMessageWithData::RequiredOnSemanticNonNull,
-                required_directive.unwrap().location,
-                vec![DiagnosticTag::UNNECESSARY],
-            ));
+            self.update_field_action(Action::Removable(vec![Message {
+                message: ValidationMessageWithData::RequiredOnSemanticNonNull,
+                location: required_directive.unwrap().location,
+            }]));
+        } else {
+            self.update_field_action(Action::NotRemovable);
         }
 
         Ok(())
@@ -125,22 +162,52 @@ impl<'a> DisallowRequiredOnNonNullField<'a> {
                     None => self.validate_required_field(linked_field, errors_are_caught),
                 };
 
+                self.path.push(linked_field.alias_or_name(self.schema));
+
                 let selection_result =
                     self.validate_selection_fields(&linked_field.selections, errors_are_caught);
 
                 try2(field_result, selection_result)?;
+                self.path.pop();
                 Ok(())
             }
             Selection::ScalarField(scalar_field) => {
                 self.validate_required_field(scalar_field, errors_are_caught)
             }
             Selection::InlineFragment(fragment) => {
-                self.validate_selection_fields(&fragment.selections, errors_are_caught)
+                if let Ok(Some(alias)) = fragment.alias(self.schema) {
+                    self.path.push(alias.item);
+                    let result =
+                        self.validate_selection_fields(&fragment.selections, errors_are_caught);
+                    self.path.pop();
+                    result
+                } else {
+                    self.validate_selection_fields(&fragment.selections, errors_are_caught)
+                }
             }
             _ => Ok(()),
         }))?;
         Ok(())
     }
+
+    fn modifiable_fields_to_warnings(&mut self) {
+        for (path, action) in self.modifiable_fields.iter() {
+            match action {
+                Action::NotRemovable => {}
+                Action::Removable(message_list) => {
+                    for message in message_list {
+                        self.warnings.push(Diagnostic::hint_with_data(
+                            message.message.clone(),
+                            message.location,
+                            vec![DiagnosticTag::UNNECESSARY],
+                        ));
+                    }
+                }
+            }
+
+            println!("path: {:?}", path);
+        }
+    }
 }
 
 impl Validator for DisallowRequiredOnNonNullField<'_> {
@@ -149,22 +216,41 @@ impl Validator for DisallowRequiredOnNonNullField<'_> {
     const VALIDATE_DIRECTIVES: bool = false;
 
     fn validate_fragment(&mut self, fragment: &FragmentDefinition) -> DiagnosticsResult<()> {
+        self.modifiable_fields.clear();
         let throw_on_field_error_directive =
             fragment.directives.named(*THROW_ON_FIELD_ERROR_DIRECTIVE);
 
         let has_throw_on_field_error_directive = throw_on_field_error_directive.is_some();
 
-        self.validate_selection_fields(&fragment.selections, has_throw_on_field_error_directive)
+        let ret = self
+            .validate_selection_fields(&fragment.selections, has_throw_on_field_error_directive);
+        self.modifiable_fields_to_warnings();
+        ret
     }
 
     fn validate_operation(
         &mut self,
         operation: &graphql_ir::OperationDefinition,
     ) -> DiagnosticsResult<()> {
+        self.modifiable_fields.clear();
         let throw_on_field_error_directive =
             operation.directives.named(*THROW_ON_FIELD_ERROR_DIRECTIVE);
 
         let has_throw_on_field_error_directive = throw_on_field_error_directive.is_some();
-        self.validate_selection_fields(&operation.selections, has_throw_on_field_error_directive)
+        let result = self
+            .validate_selection_fields(&operation.selections, has_throw_on_field_error_directive);
+        self.modifiable_fields_to_warnings();
+        result
     }
 }
+
+#[derive(Clone, Debug)]
+struct Message {
+    location: Location,
+    message: ValidationMessageWithData,
+}
+
+enum Action {
+    NotRemovable,
+    Removable(Vec<Message>),
+}

compiler/crates/relay-transforms/tests/disallow_required_on_non_null_field/fixtures/fragment_with_multiple_inline_fragment_required_fields.expected

@@ -0,0 +1,24 @@
+==================================== INPUT ====================================
+fragment MyFragment on Animal @throwOnFieldError {
+  ... on Cat {
+    noise @required(action: THROW) # can't be removed due to Dog's noise also being present
+  }
+  ... on Dog {
+    noise @required(action: THROW) # can't be removed
+  }
+}
+
+# %extensions%
+
+interface Animal {
+  name: String!
+}
+
+type Cat implements Animal {
+  noise: String!
+}
+type Dog implements Animal {
+  noise: String # nullable
+}
+==================================== OUTPUT ===================================
+OK

compiler/crates/relay-transforms/tests/disallow_required_on_non_null_field/fixtures/fragment_with_multiple_inline_fragment_required_fields.graphql

@@ -0,0 +1,21 @@
+fragment MyFragment on Animal @throwOnFieldError {
+  ... on Cat {
+    noise @required(action: THROW) # can't be removed due to Dog's noise also being present
+  }
+  ... on Dog {
+    noise @required(action: THROW) # can't be removed
+  }
+}
+
+# %extensions%
+
+interface Animal {
+  name: String!
+}
+
+type Cat implements Animal {
+  noise: String!
+}
+type Dog implements Animal {
+  noise: String # nullable
+}

compiler/crates/relay-transforms/tests/disallow_required_on_non_null_field_test.rs

@@ -4,7 +4,7 @@
  * This source code is licensed under the MIT license found in the
  * LICENSE file in the root directory of this source tree.
  *
- * @generated SignedSource<<b7654cbc0db7924c3b85107ada9f49ba>>
+ * @generated SignedSource<<a5fdf04a1c7ede81277ad2983d671375>>
  */
 
 mod disallow_required_on_non_null_field;
@@ -19,6 +19,13 @@ async fn fragment_with_inline_fragment_required_non_null_fields_invalid() {
     test_fixture(transform_fixture, file!(), "fragment_with_inline_fragment_required_non_null_fields.invalid.graphql", "disallow_required_on_non_null_field/fixtures/fragment_with_inline_fragment_required_non_null_fields.invalid.expected", input, expected).await;
 }
 
+#[tokio::test]
+async fn fragment_with_multiple_inline_fragment_required_fields() {
+    let input = include_str!("disallow_required_on_non_null_field/fixtures/fragment_with_multiple_inline_fragment_required_fields.graphql");
+    let expected = include_str!("disallow_required_on_non_null_field/fixtures/fragment_with_multiple_inline_fragment_required_fields.expected");
+    test_fixture(transform_fixture, file!(), "fragment_with_multiple_inline_fragment_required_fields.graphql", "disallow_required_on_non_null_field/fixtures/fragment_with_multiple_inline_fragment_required_fields.expected", input, expected).await;
+}
+
 #[tokio::test]
 async fn fragment_with_multiple_required_non_null_fields_invalid() {
     let input = include_str!("disallow_required_on_non_null_field/fixtures/fragment_with_multiple_required_non_null_fields.invalid.graphql");