Refine @alias enforcement

Reviewed By: tyao1 Differential Revision: D58212880 fbshipit-source-id: 074ad6ba8396d327095ae86ee405851cb46a5b6f
facebook · Jun 11, 2024 · 868183c · 868183c
1 parent dd9d3a0
commit 868183c
Show file tree

Hide file tree

Showing 12 changed files with 255 additions and 15 deletions.
diff --git a/compiler/crates/relay-transforms/src/errors.rs b/compiler/crates/relay-transforms/src/errors.rs
@@ -262,7 +262,7 @@ pub enum ValidationMessageWithData {
     RequiredOnSemanticNonNull,
 
     #[error(
-        "Expected `@alias` directive. `{fragment_name}` is defined on `{fragment_type_name}` which might not match this selection type of `{selection_type_name}`. Add `@alias` to this spread to expose the fragment data as a nullable property."
+        "Expected `@alias` directive. `{fragment_name}` is defined on `{fragment_type_name}` which might not match this selection type of `{selection_type_name}`. Add `@alias` to this spread to expose the fragment reference as a nullable property."
     )]
     ExpectedAliasOnNonSubtypeSpread {
         fragment_name: FragmentDefinitionName,
@@ -271,7 +271,16 @@ pub enum ValidationMessageWithData {
     },
 
     #[error(
-        "Expected `@alias` directive. Fragment spreads with `@{condition_name}` are conditionally fetched. Add `@alias` to this spread to expose the fragment as a nullable property."
+        "Expected `@alias` directive. `{fragment_name}` is defined on `{fragment_type_name}` which might not match this selection type of `{selection_type_name}`. Add `@alias` to this spread to expose the fragment reference as a nullable property. NOTE: The selection type inferred here does not include inline fragments because Relay does not always model inline fragment type refinements in its generated types."
+    )]
+    ExpectedAliasOnNonSubtypeSpreadWithinTypedInlineFragment {
+        fragment_name: FragmentDefinitionName,
+        fragment_type_name: StringKey,
+        selection_type_name: StringKey,
+    },
+
+    #[error(
+        "Expected `@alias` directive. Fragment spreads with `@{condition_name}` are conditionally fetched. Add `@alias` to this spread to expose the fragment reference as a nullable property."
     )]
     ExpectedAliasOnConditionalFragmentSpread { condition_name: String },
 }
@@ -299,6 +308,14 @@ impl WithDiagnosticData for ValidationMessageWithData {
                     Box::new(format!("{fragment_name} @dangerously_unaliased_fixme")),
                 ]
             }
+            ValidationMessageWithData::ExpectedAliasOnNonSubtypeSpreadWithinTypedInlineFragment {
+                fragment_name, ..
+            } => {
+                vec![
+                    Box::new(format!("{fragment_name} @alias")),
+                    Box::new(format!("{fragment_name} @dangerously_unaliased_fixme")),
+                ]
+            }
             ValidationMessageWithData::ExpectedAliasOnConditionalFragmentSpread {
                 condition_name,
                 ..

diff --git a/compiler/crates/relay-transforms/src/fragment_alias_directive.rs b/compiler/crates/relay-transforms/src/fragment_alias_directive.rs
@@ -35,6 +35,7 @@ use schema::Type;
 use crate::RelayDirective;
 use crate::ValidationMessage;
 use crate::ValidationMessageWithData;
+use crate::MATCH_CONSTANTS;
 
 lazy_static! {
     pub static ref FRAGMENT_ALIAS_DIRECTIVE_NAME: DirectiveName = DirectiveName("alias".intern());
@@ -74,6 +75,7 @@ struct FragmentAliasTransform<'program> {
     is_enforced: bool,
     document_name: Option<StringKey>,
     parent_type: Option<Type>,
+    within_inline_fragment_type_condition: bool,
     maybe_condition: Option<Condition>,
     errors: Vec<Diagnostic>,
 }
@@ -86,6 +88,7 @@ impl<'program> FragmentAliasTransform<'program> {
             is_enforced: enforced,
             document_name: None,
             parent_type: None,
+            within_inline_fragment_type_condition: false,
             maybe_condition: None,
             errors: Vec::new(),
         }
@@ -126,6 +129,16 @@ impl<'program> FragmentAliasTransform<'program> {
             // this error as a migration strategy.
             return;
         }
+        if spread
+            .directives
+            .named(MATCH_CONSTANTS.module_directive_name)
+            .is_some()
+        {
+            // Fragments that have `@module` are likely going to be accessed with a
+            // MatchContainer which should handle the possibility that this fragment
+            // will not match.
+            return;
+        }
         if let Some(condition) = &self.maybe_condition {
             self.errors.push(Diagnostic::error_with_data(
                 ValidationMessageWithData::ExpectedAliasOnConditionalFragmentSpread {
@@ -147,12 +160,21 @@ impl<'program> FragmentAliasTransform<'program> {
             {
                 let fragment_type_name = self.program.schema.get_type_name(type_condition);
                 let selection_type_name = self.program.schema.get_type_name(parent_type);
-                self.errors.push(Diagnostic::error_with_data(
+                let diagnostic = if self.within_inline_fragment_type_condition {
+                    ValidationMessageWithData::ExpectedAliasOnNonSubtypeSpreadWithinTypedInlineFragment {
+                        fragment_name: spread.fragment.item,
+                        fragment_type_name,
+                        selection_type_name,
+                    }
+                } else {
                     ValidationMessageWithData::ExpectedAliasOnNonSubtypeSpread {
                         fragment_name: spread.fragment.item,
                         fragment_type_name,
                         selection_type_name,
-                    },
+                    }
+                };
+                self.errors.push(Diagnostic::error_with_data(
+                    diagnostic,
                     spread.fragment.location,
                 ))
             }
@@ -222,13 +244,10 @@ impl Transformer for FragmentAliasTransform<'_> {
 
     fn transform_inline_fragment(&mut self, fragment: &InlineFragment) -> Transformed<Selection> {
         let previous_parent_type = self.parent_type;
+        let previous_within_inline_fragment_type_condition =
+            self.within_inline_fragment_type_condition;
 
-        // Note: This must be called before we set self.parent_type
-        let will_always_match = self.will_always_match(fragment.type_condition);
-
-        if let Some(type_condition) = fragment.type_condition {
-            self.parent_type = Some(type_condition);
-        }
+        self.within_inline_fragment_type_condition = fragment.type_condition.is_some();
 
         let transformed = match fragment.alias(&self.program.schema) {
             Ok(Some(alias)) => {
@@ -239,6 +258,14 @@ impl Transformer for FragmentAliasTransform<'_> {
                     ));
                     self.default_transform_inline_fragment(fragment);
                 }
+
+                // Note: This must be called before we set self.parent_type
+                let will_always_match = self.will_always_match(fragment.type_condition);
+
+                if let Some(type_condition) = fragment.type_condition {
+                    self.parent_type = Some(type_condition);
+                }
+
                 let alias_metadata = FragmentAliasMetadata {
                     alias,
                     type_condition: fragment.type_condition,
@@ -260,14 +287,27 @@ impl Transformer for FragmentAliasTransform<'_> {
                     spread_location: fragment.spread_location,
                 })))
             }
-            Ok(None) => self.default_transform_inline_fragment(fragment),
+            Ok(None) => {
+                // Note: We intentionally don't set self.parent_type here, even if we
+                // have at type conditions. This is because Relay does not always accurately model
+                // inline fragment type refinements as discriminated unions in its
+                // Flow/TypeScript types. This means the inline fragment might not actually result
+                // in a spread that can only be accessed when the type condition has
+                // been set.
+                //
+                // By leaving the parent selection's parent type we will require
+                // `@alias` on any spread that could fail to match with its top level
+                // selection set type.
+                self.default_transform_inline_fragment(fragment)
+            }
             Err(diagnostics) => {
                 self.errors.extend(diagnostics);
                 self.default_transform_inline_fragment(fragment)
             }
         };
 
         self.parent_type = previous_parent_type;
+        self.within_inline_fragment_type_condition = previous_within_inline_fragment_type_condition;
         transformed
     }
 

diff --git a/...s/tests/fragment_alias_directive/fixtures/alias_not_required_on_module_fragments.expected b/...s/tests/fragment_alias_directive/fixtures/alias_not_required_on_module_fragments.expected
@@ -0,0 +1,51 @@
+==================================== INPUT ====================================
+fragment NameRendererFragment on User {
+  id
+  nameRenderer @match {
+    ...PlainUserNameRenderer_name @module(name: "PlainUserNameRenderer.react")
+    ...MarkdownUserNameRenderer_name
+      @module(name: "MarkdownUserNameRenderer.react")
+  }
+  plainNameRenderer: nameRenderer @match {
+    ...PlainUserNameRenderer_name @module(name: "PlainUserNameRenderer.react")
+  }
+}
+
+fragment PlainUserNameRenderer_name on PlainUserNameRenderer {
+  plaintext
+  data {
+    text
+  }
+}
+
+fragment MarkdownUserNameRenderer_name on MarkdownUserNameRenderer {
+  markdown
+  data {
+    markup
+  }
+}
+==================================== OUTPUT ===================================
+fragment MarkdownUserNameRenderer_name on MarkdownUserNameRenderer {
+  markdown
+  data {
+    markup
+  }
+}
+
+fragment NameRendererFragment on User {
+  id
+  nameRenderer @match {
+    ...PlainUserNameRenderer_name @module(name: "PlainUserNameRenderer.react")
+    ...MarkdownUserNameRenderer_name @module(name: "MarkdownUserNameRenderer.react")
+  }
+  plainNameRenderer: nameRenderer @match {
+    ...PlainUserNameRenderer_name @module(name: "PlainUserNameRenderer.react")
+  }
+}
+
+fragment PlainUserNameRenderer_name on PlainUserNameRenderer {
+  plaintext
+  data {
+    text
+  }
+}
diff --git a/...ms/tests/fragment_alias_directive/fixtures/alias_not_required_on_module_fragments.graphql b/...ms/tests/fragment_alias_directive/fixtures/alias_not_required_on_module_fragments.graphql
@@ -0,0 +1,25 @@
+fragment NameRendererFragment on User {
+  id
+  nameRenderer @match {
+    ...PlainUserNameRenderer_name @module(name: "PlainUserNameRenderer.react")
+    ...MarkdownUserNameRenderer_name
+      @module(name: "MarkdownUserNameRenderer.react")
+  }
+  plainNameRenderer: nameRenderer @match {
+    ...PlainUserNameRenderer_name @module(name: "PlainUserNameRenderer.react")
+  }
+}
+
+fragment PlainUserNameRenderer_name on PlainUserNameRenderer {
+  plaintext
+  data {
+    text
+  }
+}
+
+fragment MarkdownUserNameRenderer_name on MarkdownUserNameRenderer {
+  markdown
+  data {
+    markup
+  }
+}
diff --git a/...ias_directive/fixtures/alias_not_required_within_aliased_refined_inline_fragment.expected b/...ias_directive/fixtures/alias_not_required_within_aliased_refined_inline_fragment.expected
@@ -0,0 +1,36 @@
+==================================== INPUT ====================================
+fragment RelayReaderNamedFragmentsTest_user on User {
+  name
+}
+
+query RelayReaderNamedFragmentsTest2Query {
+  node {
+    ... on User @alias {
+      ...RelayReaderNamedFragmentsTest_user
+    }
+  }
+}
+==================================== OUTPUT ===================================
+query RelayReaderNamedFragmentsTest2Query {
+  node {
+    ... on User @alias @__FragmentAliasMetadata
+    # FragmentAliasMetadata {
+    #     alias: WithLocation {
+    #         location: alias_not_required_within_aliased_refined_inline_fragment.graphql:133:139,
+    #         item: "User",
+    #     },
+    #     type_condition: Some(
+    #         Object(70),
+    #     ),
+    #     non_nullable: false,
+    #     selection_type: Object(70),
+    # }
+     {
+      ...RelayReaderNamedFragmentsTest_user
+    }
+  }
+}
+
+fragment RelayReaderNamedFragmentsTest_user on User {
+  name
+}
diff --git a/...lias_directive/fixtures/alias_not_required_within_aliased_refined_inline_fragment.graphql b/...lias_directive/fixtures/alias_not_required_within_aliased_refined_inline_fragment.graphql
@@ -0,0 +1,11 @@
+fragment RelayReaderNamedFragmentsTest_user on User {
+  name
+}
+
+query RelayReaderNamedFragmentsTest2Query {
+  node {
+    ... on User @alias {
+      ...RelayReaderNamedFragmentsTest_user
+    }
+  }
+}
diff --git a/...t_alias_directive/fixtures/alias_required_within_refined_inline_fragment.invalid.expected b/...t_alias_directive/fixtures/alias_required_within_refined_inline_fragment.invalid.expected
@@ -0,0 +1,24 @@
+==================================== INPUT ====================================
+# expected-to-throw
+fragment RelayReaderNamedFragmentsTest_user on User {
+  name
+}
+
+query RelayReaderNamedFragmentsTest2Query {
+  node {
+    # Relay is not reliable in modeling this as a discriminated union in its
+    # typescript/flow types. To be safe we need the user to supply an alias.
+    ... on User {
+      # This might not match!
+      ...RelayReaderNamedFragmentsTest_user
+    }
+  }
+}
+==================================== ERROR ====================================
+✖︎ Expected `@alias` directive. `RelayReaderNamedFragmentsTest_user` is defined on `User` which might not match this selection type of `Node`. Add `@alias` to this spread to expose the fragment reference as a nullable property. NOTE: The selection type inferred here does not include inline fragments because Relay does not always model inline fragment type refinements in its generated types.
+
+  alias_required_within_refined_inline_fragment.invalid.graphql:12:10
+   11 │       # This might not match!
+   12 │       ...RelayReaderNamedFragmentsTest_user
+      │          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   13 │     }
diff --git a/...nt_alias_directive/fixtures/alias_required_within_refined_inline_fragment.invalid.graphql b/...nt_alias_directive/fixtures/alias_required_within_refined_inline_fragment.invalid.graphql
@@ -0,0 +1,15 @@
+# expected-to-throw
+fragment RelayReaderNamedFragmentsTest_user on User {
+  name
+}
+
+query RelayReaderNamedFragmentsTest2Query {
+  node {
+    # Relay is not reliable in modeling this as a discriminated union in its
+    # typescript/flow types. To be safe we need the user to supply an alias.
+    ... on User {
+      # This might not match!
+      ...RelayReaderNamedFragmentsTest_user
+    }
+  }
+}
diff --git a/...nt_alias_directive/fixtures/fragment_spread_into_supertype_without_alias.invalid.expected b/...nt_alias_directive/fixtures/fragment_spread_into_supertype_without_alias.invalid.expected
@@ -11,7 +11,7 @@ query RelayReaderNamedFragmentsTest2Query {
   }
 }
 ==================================== ERROR ====================================
-✖︎ Expected `@alias` directive. `RelayReaderNamedFragmentsTest_user` is defined on `User` which might not match this selection type of `Node`. Add `@alias` to this spread to expose the fragment data as a nullable property.
+✖︎ Expected `@alias` directive. `RelayReaderNamedFragmentsTest_user` is defined on `User` which might not match this selection type of `Node`. Add `@alias` to this spread to expose the fragment reference as a nullable property.
 
   fragment_spread_into_supertype_without_alias.invalid.graphql:9:8
     8 │     # This might not match!

diff --git a/...sts/fragment_alias_directive/fixtures/skip_fragment_spread_without_alias.invalid.expected b/...sts/fragment_alias_directive/fixtures/skip_fragment_spread_without_alias.invalid.expected
@@ -11,7 +11,7 @@ query RelayReaderNamedFragmentsTest2Query($someCondition: Boolean!) {
   }
 }
 ==================================== ERROR ====================================
-✖︎ Expected `@alias` directive. Fragment spreads with `@skip` are conditionally fetched. Add `@alias` to this spread to expose the fragment as a nullable property.
+✖︎ Expected `@alias` directive. Fragment spreads with `@skip` are conditionally fetched. Add `@alias` to this spread to expose the fragment reference as a nullable property.
 
   skip_fragment_spread_without_alias.invalid.graphql:9:43
     8 │     # This might not match!

diff --git a/compiler/crates/relay-transforms/tests/fragment_alias_directive_test.rs b/compiler/crates/relay-transforms/tests/fragment_alias_directive_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<<36d896c88d8f613a56a4df3c63de84a4>>
+ * @generated SignedSource<<9999601a445e2154561da251e0ca30b4>>
  */
 
 mod fragment_alias_directive;
@@ -19,6 +19,20 @@ async fn alias_as_empty_string_invalid() {
     test_fixture(transform_fixture, file!(), "alias_as_empty_string.invalid.graphql", "fragment_alias_directive/fixtures/alias_as_empty_string.invalid.expected", input, expected).await;
 }
 
+#[tokio::test]
+async fn alias_not_required_on_module_fragments() {
+    let input = include_str!("fragment_alias_directive/fixtures/alias_not_required_on_module_fragments.graphql");
+    let expected = include_str!("fragment_alias_directive/fixtures/alias_not_required_on_module_fragments.expected");
+    test_fixture(transform_fixture, file!(), "alias_not_required_on_module_fragments.graphql", "fragment_alias_directive/fixtures/alias_not_required_on_module_fragments.expected", input, expected).await;
+}
+
+#[tokio::test]
+async fn alias_not_required_within_aliased_refined_inline_fragment() {
+    let input = include_str!("fragment_alias_directive/fixtures/alias_not_required_within_aliased_refined_inline_fragment.graphql");
+    let expected = include_str!("fragment_alias_directive/fixtures/alias_not_required_within_aliased_refined_inline_fragment.expected");
+    test_fixture(transform_fixture, file!(), "alias_not_required_within_aliased_refined_inline_fragment.graphql", "fragment_alias_directive/fixtures/alias_not_required_within_aliased_refined_inline_fragment.expected", input, expected).await;
+}
+
 #[tokio::test]
 async fn alias_on_abstract_type() {
     let input = include_str!("fragment_alias_directive/fixtures/alias_on_abstract_type.graphql");
@@ -40,6 +54,13 @@ async fn alias_on_spread_of_plural_fragment_invalid() {
     test_fixture(transform_fixture, file!(), "alias_on_spread_of_plural_fragment.invalid.graphql", "fragment_alias_directive/fixtures/alias_on_spread_of_plural_fragment.invalid.expected", input, expected).await;
 }
 
+#[tokio::test]
+async fn alias_required_within_refined_inline_fragment_invalid() {
+    let input = include_str!("fragment_alias_directive/fixtures/alias_required_within_refined_inline_fragment.invalid.graphql");
+    let expected = include_str!("fragment_alias_directive/fixtures/alias_required_within_refined_inline_fragment.invalid.expected");
+    test_fixture(transform_fixture, file!(), "alias_required_within_refined_inline_fragment.invalid.graphql", "fragment_alias_directive/fixtures/alias_required_within_refined_inline_fragment.invalid.expected", input, expected).await;
+}
+
 #[tokio::test]
 async fn aliased_inline_fragment() {
     let input = include_str!("fragment_alias_directive/fixtures/aliased_inline_fragment.graphql");