Ensure incremental builds include all @rootfragment fragments

captbaritone · facebook-github-bot · commit f0e802a7c1f4 · 2023-05-09T15:45:51.000-07:00
Reviewed By: tyao1

Differential Revision: D45709148

fbshipit-source-id: 6d305dcb60fe92f9c41cffe25f7a69e304dc18a7
diff --git a/compiler/crates/dependency-analyzer/src/ir.rs b/compiler/crates/dependency-analyzer/src/ir.rs
@@ -189,7 +189,6 @@ fn visit_selections(
             Selection::LinkedField(linked_field) => {
                 if let Some(fragment_name) = get_resolver_fragment_dependency_name(
                     schema.field(linked_field.definition.item),
-                    schema,
                 ) {
                     update_dependency_graph(
                         fragment_name.into(),
@@ -209,7 +208,6 @@ fn visit_selections(
             Selection::ScalarField(scalar_field) => {
                 if let Some(fragment_name) = get_resolver_fragment_dependency_name(
                     schema.field(scalar_field.definition.item),
-                    schema,
                 ) {
                     update_dependency_graph(
                         fragment_name.into(),
diff --git a/compiler/crates/dependency-analyzer/tests/ir/fixtures/new-resolver-model-field-with-custom-fragment.expected b/compiler/crates/dependency-analyzer/tests/ir/fixtures/new-resolver-model-field-with-custom-fragment.expected
@@ -0,0 +1,38 @@
+==================================== INPUT ====================================
+# F1
+query Q1 {
+  client_user {
+    ...F1
+  }
+}
+fragment F1 on ClientUser {
+  pop_star_name
+}
+
+fragment ClientUserPopStarName on ClientUser {
+  id
+}
+
+# %definitions%
+
+%extensions%
+
+extend type Query {
+  client_user: ClientUser
+}
+
+type ClientUser @__RelayResolverModel {
+  id: ID!
+}
+
+# This resolver's fragment is _not_ generated even though it's on a @__RelayResolverModel type.
+# Dependency tracker _should_ look for this fragment.
+extend type ClientUser {
+  pop_star_name: String @relay_resolver(fragment_name: "ClientUserPopStarName", import_path: "PopStarNameResolver")
+}
+==================================== OUTPUT ===================================
+Fragment: ClientUserPopStarName
+
+Fragment: F1
+
+Operation: Q1
diff --git a/compiler/crates/dependency-analyzer/tests/ir/fixtures/new-resolver-model-field-with-custom-fragment.graphql b/compiler/crates/dependency-analyzer/tests/ir/fixtures/new-resolver-model-field-with-custom-fragment.graphql
@@ -0,0 +1,31 @@
+# F1
+query Q1 {
+  client_user {
+    ...F1
+  }
+}
+fragment F1 on ClientUser {
+  pop_star_name
+}
+
+fragment ClientUserPopStarName on ClientUser {
+  id
+}
+
+# %definitions%
+
+%extensions%
+
+extend type Query {
+  client_user: ClientUser
+}
+
+type ClientUser @__RelayResolverModel {
+  id: ID!
+}
+
+# This resolver's fragment is _not_ generated even though it's on a @__RelayResolverModel type.
+# Dependency tracker _should_ look for this fragment.
+extend type ClientUser {
+  pop_star_name: String @relay_resolver(fragment_name: "ClientUserPopStarName", import_path: "PopStarNameResolver")
+}
diff --git a/compiler/crates/dependency-analyzer/tests/ir/fixtures/new-resolver-model-field.expected b/compiler/crates/dependency-analyzer/tests/ir/fixtures/new-resolver-model-field.expected
@@ -21,10 +21,12 @@ type ClientUser @__RelayResolverModel {
   id: ID!
 }
 
-# This resolver's fragment is generated because it's on a @__RelayResolverModel type.
+# This resolver's fragment is generated because it's on a @__RelayResolverModel
+# type and does not define its own fragment using @rootFragment. This is indicated with
+# the `generated_fragment` directive argument.
 # Dependency tracker should not look for this fragment.
 extend type ClientUser {
-  pop_star_name: String @relay_resolver(fragment_name: "this___is___generated", import_path: "PopStarNameResolver")
+  pop_star_name: String @relay_resolver(fragment_name: "this___is___generated", import_path: "PopStarNameResolver", generated_fragment: true)
 }
 ==================================== OUTPUT ===================================
 Fragment: F1
diff --git a/compiler/crates/dependency-analyzer/tests/ir/fixtures/new-resolver-model-field.graphql b/compiler/crates/dependency-analyzer/tests/ir/fixtures/new-resolver-model-field.graphql
@@ -20,8 +20,10 @@ type ClientUser @__RelayResolverModel {
   id: ID!
 }
 
-# This resolver's fragment is generated because it's on a @__RelayResolverModel type.
+# This resolver's fragment is generated because it's on a @__RelayResolverModel
+# type and does not define its own fragment using @rootFragment. This is indicated with
+# the `generated_fragment` directive argument.
 # Dependency tracker should not look for this fragment.
 extend type ClientUser {
-  pop_star_name: String @relay_resolver(fragment_name: "this___is___generated", import_path: "PopStarNameResolver")
+  pop_star_name: String @relay_resolver(fragment_name: "this___is___generated", import_path: "PopStarNameResolver", generated_fragment: true)
 }
diff --git a/compiler/crates/dependency-analyzer/tests/ir_test.rs b/compiler/crates/dependency-analyzer/tests/ir_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<<080b76ec4b2c6ea6cde4bc6b7c0fea2b>>
+ * @generated SignedSource<<1370ea763208b340fd15717d45729c87>>
  */
 
 mod ir;
@@ -75,6 +75,13 @@ fn new_resolver_model_field() {
     test_fixture(transform_fixture, "new-resolver-model-field.graphql", "ir/fixtures/new-resolver-model-field.expected", input, expected);
 }
 
+#[test]
+fn new_resolver_model_field_with_custom_fragment() {
+    let input = include_str!("ir/fixtures/new-resolver-model-field-with-custom-fragment.graphql");
+    let expected = include_str!("ir/fixtures/new-resolver-model-field-with-custom-fragment.expected");
+    test_fixture(transform_fixture, "new-resolver-model-field-with-custom-fragment.graphql", "ir/fixtures/new-resolver-model-field-with-custom-fragment.expected", input, expected);
+}
+
 #[test]
 fn query_then_fragment() {
     let input = include_str!("ir/fixtures/query-then-fragment.graphql");
diff --git a/compiler/crates/docblock-shared/src/lib.rs b/compiler/crates/docblock-shared/src/lib.rs
@@ -31,6 +31,8 @@ lazy_static! {
     pub static ref IMPORT_PATH_ARGUMENT_NAME: ArgumentName = ArgumentName("import_path".intern());
     pub static ref INJECT_FRAGMENT_DATA_ARGUMENT_NAME: ArgumentName =
         ArgumentName("inject_fragment_data".intern());
+    pub static ref GENERATED_FRAGMENT_ARGUMENT_NAME: ArgumentName =
+        ArgumentName("generated_fragment".intern());
     pub static ref FIELD_NAME_FIELD: StringKey = "fieldName".intern();
     pub static ref ON_TYPE_FIELD: StringKey = "onType".intern();
     pub static ref ON_INTERFACE_FIELD: StringKey = "onInterface".intern();
diff --git a/compiler/crates/graphql-syntax/src/node/constant_value.rs b/compiler/crates/graphql-syntax/src/node/constant_value.rs
@@ -44,6 +44,13 @@ impl ConstantValue {
             _ => None,
         }
     }
+
+    pub fn get_bool_literal(&self) -> Option<bool> {
+        match self {
+            ConstantValue::Boolean(BooleanNode { value, .. }) => Some(*value),
+            _ => None,
+        }
+    }
 }
 
 impl fmt::Display for ConstantValue {
diff --git a/compiler/crates/relay-compiler/src/build_project/project_asts.rs b/compiler/crates/relay-compiler/src/build_project/project_asts.rs
@@ -156,7 +156,7 @@ fn find_base_resolver_fragment_asts(
 ) -> Vec<ExecutableDefinition> {
     let mut base_resolver_fragments = ExecutableDefinitionNameSet::default();
     for field in schema.fields() {
-        if let Some(fragment_name) = get_resolver_fragment_dependency_name(field, schema) {
+        if let Some(fragment_name) = get_resolver_fragment_dependency_name(field) {
             if base_definition_asts.contains(&fragment_name.into()) {
                 base_resolver_fragments.insert(fragment_name.into());
             }
diff --git a/compiler/crates/relay-docblock/src/ir.rs b/compiler/crates/relay-docblock/src/ir.rs
@@ -19,6 +19,7 @@ use common::ObjectName;
 use common::Span;
 use common::WithLocation;
 use docblock_shared::FRAGMENT_KEY_ARGUMENT_NAME;
+use docblock_shared::GENERATED_FRAGMENT_ARGUMENT_NAME;
 use docblock_shared::HAS_OUTPUT_TYPE_ARGUMENT_NAME;
 use docblock_shared::IMPORT_NAME_ARGUMENT_NAME;
 use docblock_shared::IMPORT_PATH_ARGUMENT_NAME;
@@ -260,6 +261,7 @@ pub enum FragmentDataInjectionMode {
 
 pub struct RootFragment {
     fragment: WithLocation<FragmentDefinitionName>,
+    generated: bool,
     // For Model resolvers, we need to pass the `id` or `__relay_model_instance` field
     // from the fragment data to the resolver function
     inject_fragment_data: Option<FragmentDataInjectionMode>,
@@ -338,6 +340,13 @@ trait ResolverIr: Sized {
                 root_fragment.fragment.map(|x| x.0),
             ));
 
+            if root_fragment.generated {
+                arguments.push(true_argument(
+                    GENERATED_FRAGMENT_ARGUMENT_NAME.0,
+                    Location::generated(),
+                ))
+            }
+
             if let Some(inject_fragment_data) = root_fragment.inject_fragment_data {
                 match inject_fragment_data {
                     FragmentDataInjectionMode::Field(field_name) => {
@@ -668,6 +677,7 @@ impl ResolverIr for TerseRelayResolverIr {
         self.root_fragment
             .map(|fragment| RootFragment {
                 fragment,
+                generated: false,
                 inject_fragment_data: None,
             })
             .or_else(|| get_root_fragment_for_object(object))
@@ -832,6 +842,7 @@ impl ResolverIr for RelayResolverIr {
         self.root_fragment
             .map(|fragment| RootFragment {
                 fragment,
+                generated: false,
                 inject_fragment_data: None,
             })
             .or_else(|| get_root_fragment_for_object(object))
@@ -1073,6 +1084,7 @@ impl ResolverIr for StrongObjectIr {
     ) -> Option<RootFragment> {
         Some(RootFragment {
             fragment: self.root_fragment,
+            generated: true,
             inject_fragment_data: Some(FragmentDataInjectionMode::Field(
                 schema_info.config.node_interface_id_field,
             )),
@@ -1335,6 +1347,7 @@ fn get_root_fragment_for_object(object: Option<&Object>) -> Option<RootFragment>
                 )
                 .intern(),
             )),
+            generated: true,
             inject_fragment_data: Some(FragmentDataInjectionMode::Field(
                 *RESOLVER_MODEL_INSTANCE_FIELD_NAME,
             )),
diff --git a/compiler/crates/relay-docblock/tests/to_schema/fixtures/legacy-relay-resolver-with-root-fragment-on-model.expected b/compiler/crates/relay-docblock/tests/to_schema/fixtures/legacy-relay-resolver-with-root-fragment-on-model.expected
@@ -25,7 +25,7 @@ graphql`
 ==================================== OUTPUT ===================================
 type MyType @__RelayResolverModel {
   id: ID!
-  __relay_model_instance: Int @relay_resolver(import_path: "/path/to/test/fixture/legacy-relay-resolver-with-root-fragment-on-model.js", fragment_name: "MyType__id", inject_fragment_data: "id", import_name: "MyType") @unselectable(reason: "This field is intended only for Relay's internal use")
+  __relay_model_instance: Int @relay_resolver(import_path: "/path/to/test/fixture/legacy-relay-resolver-with-root-fragment-on-model.js", fragment_name: "MyType__id", generated_fragment: true, inject_fragment_data: "id", import_name: "MyType") @unselectable(reason: "This field is intended only for Relay's internal use")
 }
 
 
diff --git a/compiler/crates/relay-docblock/tests/to_schema/fixtures/relay-resolver-strong-object-with-implements.expected b/compiler/crates/relay-docblock/tests/to_schema/fixtures/relay-resolver-strong-object-with-implements.expected
@@ -21,5 +21,5 @@ interface IFoo {
 ==================================== OUTPUT ===================================
 type ClientUser implements IFoo @__RelayResolverModel {
   id: ID!
-  __relay_model_instance: Int @relay_resolver(import_path: "/path/to/test/fixture/relay-resolver-strong-object-with-implements.js", fragment_name: "ClientUser__id", inject_fragment_data: "id", import_name: "ClientUser") @unselectable(reason: "This field is intended only for Relay's internal use")
+  __relay_model_instance: Int @relay_resolver(import_path: "/path/to/test/fixture/relay-resolver-strong-object-with-implements.js", fragment_name: "ClientUser__id", generated_fragment: true, inject_fragment_data: "id", import_name: "ClientUser") @unselectable(reason: "This field is intended only for Relay's internal use")
 }
diff --git a/compiler/crates/relay-docblock/tests/to_schema/fixtures/relay-resolver-strong-object-with-multiple-implements.expected b/compiler/crates/relay-docblock/tests/to_schema/fixtures/relay-resolver-strong-object-with-multiple-implements.expected
@@ -27,5 +27,5 @@ interface IBar {
 ==================================== OUTPUT ===================================
 type ClientUser implements IFoo & IBar @__RelayResolverModel {
   id: ID!
-  __relay_model_instance: Int @relay_resolver(import_path: "/path/to/test/fixture/relay-resolver-strong-object-with-multiple-implements.js", fragment_name: "ClientUser__id", inject_fragment_data: "id", import_name: "ClientUser") @unselectable(reason: "This field is intended only for Relay's internal use")
+  __relay_model_instance: Int @relay_resolver(import_path: "/path/to/test/fixture/relay-resolver-strong-object-with-multiple-implements.js", fragment_name: "ClientUser__id", generated_fragment: true, inject_fragment_data: "id", import_name: "ClientUser") @unselectable(reason: "This field is intended only for Relay's internal use")
 }
diff --git a/compiler/crates/relay-docblock/tests/to_schema/fixtures/relay-resolver-strong-object.expected b/compiler/crates/relay-docblock/tests/to_schema/fixtures/relay-resolver-strong-object.expected
@@ -12,5 +12,5 @@
 ==================================== OUTPUT ===================================
 type ClientUser @__RelayResolverModel {
   id: ID!
-  __relay_model_instance: Int @relay_resolver(import_path: "/path/to/test/fixture/relay-resolver-strong-object.js", fragment_name: "ClientUser__id", inject_fragment_data: "id", import_name: "ClientUser") @unselectable(reason: "This field is intended only for Relay's internal use")
+  __relay_model_instance: Int @relay_resolver(import_path: "/path/to/test/fixture/relay-resolver-strong-object.js", fragment_name: "ClientUser__id", generated_fragment: true, inject_fragment_data: "id", import_name: "ClientUser") @unselectable(reason: "This field is intended only for Relay's internal use")
 }
diff --git a/compiler/crates/relay-docblock/tests/to_schema/fixtures/terse-relay-resolver-with-root-fragment-on-model.expected b/compiler/crates/relay-docblock/tests/to_schema/fixtures/terse-relay-resolver-with-root-fragment-on-model.expected
@@ -23,7 +23,7 @@ graphql`
 ==================================== OUTPUT ===================================
 type MyType @__RelayResolverModel {
   id: ID!
-  __relay_model_instance: Int @relay_resolver(import_path: "/path/to/test/fixture/terse-relay-resolver-with-root-fragment-on-model.js", fragment_name: "MyType__id", inject_fragment_data: "id", import_name: "MyType") @unselectable(reason: "This field is intended only for Relay's internal use")
+  __relay_model_instance: Int @relay_resolver(import_path: "/path/to/test/fixture/terse-relay-resolver-with-root-fragment-on-model.js", fragment_name: "MyType__id", generated_fragment: true, inject_fragment_data: "id", import_name: "MyType") @unselectable(reason: "This field is intended only for Relay's internal use")
 }
 
 
diff --git a/compiler/crates/relay-transforms/src/relay_resolvers.rs b/compiler/crates/relay-transforms/src/relay_resolvers.rs
@@ -14,13 +14,13 @@ use common::Location;
 use common::NamedItem;
 use common::WithLocation;
 use docblock_shared::FRAGMENT_KEY_ARGUMENT_NAME;
+use docblock_shared::GENERATED_FRAGMENT_ARGUMENT_NAME;
 use docblock_shared::HAS_OUTPUT_TYPE_ARGUMENT_NAME;
 use docblock_shared::IMPORT_NAME_ARGUMENT_NAME;
 use docblock_shared::IMPORT_PATH_ARGUMENT_NAME;
 use docblock_shared::INJECT_FRAGMENT_DATA_ARGUMENT_NAME;
 use docblock_shared::LIVE_ARGUMENT_NAME;
 use docblock_shared::RELAY_RESOLVER_DIRECTIVE_NAME;
-use docblock_shared::RELAY_RESOLVER_MODEL_DIRECTIVE_NAME;
 use docblock_shared::RELAY_RESOLVER_WEAK_OBJECT_DIRECTIVE;
 use graphql_ir::associated_data_impl;
 use graphql_ir::Argument;
@@ -725,47 +725,30 @@ pub(crate) fn get_bool_argument_is_true(
 
 // If the field is a resolver, return its user defined fragment name. Does not
 // return generated fragment names.
-pub fn get_resolver_fragment_dependency_name(
-    field: &Field,
-    schema: &SDLSchema,
-) -> Option<FragmentDefinitionName> {
+pub fn get_resolver_fragment_dependency_name(field: &Field) -> Option<FragmentDefinitionName> {
     if !field.is_extension {
         return None;
     }
 
     field
         .directives
         .named(*RELAY_RESOLVER_DIRECTIVE_NAME)
+        .filter(|resolver_directive| {
+            let generated = resolver_directive
+                .arguments
+                .named(*GENERATED_FRAGMENT_ARGUMENT_NAME)
+                .and_then(|arg| arg.value.get_bool_literal())
+                .unwrap_or(false);
+            !generated
+        })
         .and_then(|resolver_directive| {
             resolver_directive
                 .arguments
                 .named(*FRAGMENT_KEY_ARGUMENT_NAME)
         })
-        .filter(|_| {
-            // Resolvers on relay model types use generated fragments, and
-            // therefore have no user-defined fragment dependency.
-            !is_field_of_relay_model(schema, field)
-        })
         .and_then(|arg| arg.value.get_string_literal().map(FragmentDefinitionName))
 }
 
-fn is_field_of_relay_model(schema: &SDLSchema, field: &Field) -> bool {
-    if let Some(parent_type) = field.parent_type {
-        let directives = match parent_type {
-            schema::Type::Object(object_id) => &schema.object(object_id).directives,
-            schema::Type::Interface(interface_id) => &schema.interface(interface_id).directives,
-            schema::Type::Union(union_id) => &schema.union(union_id).directives,
-            _ => panic!("Expected parent to be an object, interface or union."),
-        };
-
-        directives
-            .named(*RELAY_RESOLVER_MODEL_DIRECTIVE_NAME)
-            .is_some()
-    } else {
-        false
-    }
-}
-
 fn to_camel_case(non_camelized_string: String) -> String {
     let mut camelized_string = String::with_capacity(non_camelized_string.len());
     let mut last_character_was_not_alphanumeric = false;

Original file line number	Diff line number	Diff line change
`@@ -21,10 +21,12 @@ type ClientUser @__RelayResolverModel {`
`21`	`21`	`id: ID!`
`22`	`22`	`}`
`23`	`23`
`24`		`-# This resolver's fragment is generated because it's on a @__RelayResolverModel type.`
	`24`	`+# This resolver's fragment is generated because it's on a @__RelayResolverModel`
	`25`	`+# type and does not define its own fragment using @rootFragment. This is indicated with`
	`26`	+# the `generated_fragment` directive argument.
`25`	`27`	`# Dependency tracker should not look for this fragment.`
`26`	`28`	`extend type ClientUser {`
`27`		`- pop_star_name: String @relay_resolver(fragment_name: "this___is___generated", import_path: "PopStarNameResolver")`
	`29`	`+ pop_star_name: String @relay_resolver(fragment_name: "this___is___generated", import_path: "PopStarNameResolver", generated_fragment: true)`
`28`	`30`	`}`
`29`	`31`	`==================================== OUTPUT ===================================`
`30`	`32`	`Fragment: F1`
Original file line number	Diff line number	Diff line change
`@@ -20,8 +20,10 @@ type ClientUser @__RelayResolverModel {`
`20`	`20`	`id: ID!`
`21`	`21`	`}`
`22`	`22`
`23`		`-# This resolver's fragment is generated because it's on a @__RelayResolverModel type.`
	`23`	`+# This resolver's fragment is generated because it's on a @__RelayResolverModel`
	`24`	`+# type and does not define its own fragment using @rootFragment. This is indicated with`
	`25`	+# the `generated_fragment` directive argument.
`24`	`26`	`# Dependency tracker should not look for this fragment.`
`25`	`27`	`extend type ClientUser {`
`26`		`- pop_star_name: String @relay_resolver(fragment_name: "this___is___generated", import_path: "PopStarNameResolver")`
	`28`	`+ pop_star_name: String @relay_resolver(fragment_name: "this___is___generated", import_path: "PopStarNameResolver", generated_fragment: true)`
`27`	`29`	`}`
Original file line number	Diff line number	Diff line change
`@@ -44,6 +44,13 @@ impl ConstantValue {`
`44`	`44`	`_ => None,`
`45`	`45`	`}`
`46`	`46`	`}`
	`47`	`+`
	`48`	`+ pub fn get_bool_literal(&self) -> Option<bool> {`
	`49`	`+ match self {`
	`50`	`+ ConstantValue::Boolean(BooleanNode { value, .. }) => Some(*value),`
	`51`	`+ _ => None,`
	`52`	`+ }`
	`53`	`+ }`
`47`	`54`	`}`
`48`	`55`
`49`	`56`	`impl fmt::Display for ConstantValue {`
Original file line number	Diff line number	Diff line change
`@@ -156,7 +156,7 @@ fn find_base_resolver_fragment_asts(`
`156`	`156`	`) -> Vec<ExecutableDefinition> {`
`157`	`157`	`let mut base_resolver_fragments = ExecutableDefinitionNameSet::default();`
`158`	`158`	`for field in schema.fields() {`
`159`		`- if let Some(fragment_name) = get_resolver_fragment_dependency_name(field, schema) {`
	`159`	`+ if let Some(fragment_name) = get_resolver_fragment_dependency_name(field) {`
`160`	`160`	`if base_definition_asts.contains(&fragment_name.into()) {`
`161`	`161`	`base_resolver_fragments.insert(fragment_name.into());`
`162`	`162`	`}`