@required should be allowed if field is nullable due to child @required

Reviewed By: captbaritone

Differential Revision: D58704612

fbshipit-source-id: b24944267f1a9c2b1dccf56165a31250e3133d7d

compiler/crates/relay-compiler/src/build_project.rs

@@ -69,6 +69,7 @@ use schema_diff::check::IncrementalBuildSchemaChange;
 use schema_diff::check::SchemaChangeSafety;
 pub use source_control::source_control_for_root;
 pub use validate::validate;
+pub use validate::validate_reader;
 pub use validate::AdditionalValidations;
 
 use self::log_program_stats::print_stats;
@@ -250,6 +251,30 @@ pub fn validate_program(
     result
 }
 
+pub fn validate_reader_program(
+    config: &Config,
+    project_config: &ProjectConfig,
+    program: &Program,
+    log_event: &impl PerfLogEvent,
+) -> Result<Vec<Diagnostic>, BuildProjectError> {
+    let timer = log_event.start("validate_reader_time");
+    log_event.number("validate_reader_documents_count", program.document_count());
+    let result = validate_reader(program, project_config, &config.additional_validations)
+        .map_or_else(
+            |errors| {
+                Err(BuildProjectError::ValidationErrors {
+                    errors,
+                    project_name: project_config.name,
+                })
+            },
+            |result| Ok(result.diagnostics),
+        );
+
+    log_event.stop(timer);
+
+    result
+}
+
 /// Apply various chains of transforms to create a set of output programs.
 pub fn transform_program(
     project_config: &ProjectConfig,
@@ -280,6 +305,7 @@ pub fn transform_program(
     result
 }
 
+#[allow(clippy::too_many_arguments)]
 pub fn build_programs(
     config: &Config,
     project_config: &ProjectConfig,
@@ -369,7 +395,7 @@ pub fn build_programs(
         .map(|program| {
             // Call validation rules that go beyond type checking.
             // FIXME: Return non-fatal diagnostics from transforms (only validations for now)
-            let diagnostics = validate_program(config, project_config, &program, log_event)?;
+            let mut diagnostics = validate_program(config, project_config, &program, log_event)?;
 
             let programs = transform_program(
                 project_config,
@@ -380,6 +406,13 @@ pub fn build_programs(
                 config.custom_transforms.as_ref(),
             )?;
 
+            diagnostics.extend(validate_reader_program(
+                config,
+                project_config,
+                &programs.reader,
+                log_event,
+            )?);
+
             Ok((programs, diagnostics))
         })
         .collect::<Result<Vec<_>, BuildProjectFailure>>()?;

compiler/crates/relay-compiler/src/build_project/validate.rs

@@ -36,6 +36,33 @@ use relay_transforms::validate_updatable_fragment_spread;
 pub type AdditionalValidations =
     Box<dyn Fn(&Program, &ProjectConfig) -> DiagnosticsResult<()> + Sync + Send>;
 
+/// Perform validations on the program schema after it has been transformed
+/// for the reader.
+pub fn validate_reader(
+    program: &Program,
+    project_config: &ProjectConfig,
+    additional_validations: &Option<AdditionalValidations>,
+) -> DiagnosticsResult<WithDiagnostics<()>> {
+    let output = try_all(vec![
+        disallow_required_on_non_null_field(
+            program,
+            project_config
+                .feature_flags
+                .disallow_required_on_non_null_fields,
+            project_config
+                .typegen_config
+                .experimental_emit_semantic_nullability_types,
+        ),
+        if let Some(ref validate) = additional_validations {
+            validate(program, project_config)
+        } else {
+            Ok(())
+        },
+    ]);
+
+    transform_errors(output, project_config)
+}
+
 pub fn validate(
     program: &Program,
     project_config: &ProjectConfig,
@@ -78,17 +105,15 @@ pub fn validate(
                 .allow_required_in_mutation_response,
             project_config.feature_flags.enable_relay_resolver_mutations,
         ),
-        disallow_required_on_non_null_field(
-            program,
-            project_config
-                .feature_flags
-                .disallow_required_on_non_null_fields,
-            project_config
-                .typegen_config
-                .experimental_emit_semantic_nullability_types,
-        ),
     ]);
 
+    transform_errors(output, project_config)
+}
+
+fn transform_errors(
+    output: Result<Vec<()>, Vec<common::Diagnostic>>,
+    project_config: &ProjectConfig,
+) -> Result<WithDiagnostics<()>, Vec<common::Diagnostic>> {
     match output {
         Ok(_) => Ok(WithDiagnostics {
             item: (),

compiler/crates/relay-compiler/tests/relay_compiler_integration/fixtures/semantic_null_require_bubble_to_required_parent.expected

@@ -0,0 +1,102 @@
+==================================== INPUT ====================================
+//- PersonComponent.js
+graphql`fragment PersonComponentFragment on Query @throwOnFieldError {
+  some_person @required(action: LOG) {
+    name @required(action: LOG)
+  }
+}`
+
+//- relay.config.json
+{
+  "language": "flow",
+  "jsModuleFormat": "haste",
+  "schema": "schema.graphql",
+  "experimentalEmitSemanticNullabilityTypes": true
+}
+
+//- schema.graphql
+type Query {
+  some_person: Person @semanticNonNull
+}
+
+
+type Person {
+  name: String
+}
+==================================== OUTPUT ===================================
+//- __generated__/PersonComponentFragment.graphql.js
+/**
+ * <auto-generated> SignedSource<<fb5e66f956e0ae6de1169a4ef911454e>>
+ * @flow
+ * @lightSyntaxTransform
+ * @nogrep
+ */
+
+/* eslint-disable */
+
+'use strict';
+
+/*::
+import type { Fragment, ReaderFragment } from 'relay-runtime';
+import type { FragmentType } from "relay-runtime";
+declare export opaque type PersonComponentFragment$fragmentType: FragmentType;
+export type PersonComponentFragment$data = ?{|
+  +some_person: {|
+    +name: string,
+  |},
+  +$fragmentType: PersonComponentFragment$fragmentType,
+|};
+export type PersonComponentFragment$key = {
+  +$data?: PersonComponentFragment$data,
+  +$fragmentSpreads: PersonComponentFragment$fragmentType,
+  ...
+};
+*/
+
+var node/*: ReaderFragment*/ = {
+  "argumentDefinitions": [],
+  "kind": "Fragment",
+  "metadata": {
+    "throwOnFieldError": true
+  },
+  "name": "PersonComponentFragment",
+  "selections": [
+    {
+      "kind": "RequiredField",
+      "field": {
+        "alias": null,
+        "args": null,
+        "concreteType": "Person",
+        "kind": "LinkedField",
+        "name": "some_person",
+        "plural": false,
+        "selections": [
+          {
+            "kind": "RequiredField",
+            "field": {
+              "alias": null,
+              "args": null,
+              "kind": "ScalarField",
+              "name": "name",
+              "storageKey": null
+            },
+            "action": "LOG",
+            "path": "some_person.name"
+          }
+        ],
+        "storageKey": null
+      },
+      "action": "LOG",
+      "path": "some_person"
+    }
+  ],
+  "type": "Query",
+  "abstractKey": null
+};
+
+(node/*: any*/).hash = "98e50ccb4d4bee0618374c55c0a2c4a8";
+
+module.exports = ((node/*: any*/)/*: Fragment<
+  PersonComponentFragment$fragmentType,
+  PersonComponentFragment$data,
+>*/);

compiler/crates/relay-compiler/tests/relay_compiler_integration/fixtures/semantic_null_require_bubble_to_required_parent.input

@@ -0,0 +1,24 @@
+//- PersonComponent.js
+graphql`fragment PersonComponentFragment on Query @throwOnFieldError {
+  some_person @required(action: LOG) {
+    name @required(action: LOG)
+  }
+}`
+
+//- relay.config.json
+{
+  "language": "flow",
+  "jsModuleFormat": "haste",
+  "schema": "schema.graphql",
+  "experimentalEmitSemanticNullabilityTypes": true
+}
+
+//- schema.graphql
+type Query {
+  some_person: Person @semanticNonNull
+}
+
+
+type Person {
+  name: String
+}

compiler/crates/relay-compiler/tests/relay_compiler_integration_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<<399f84b890051cc63ed0d860bf73cd5e>>
+ * @generated SignedSource<<f7617f554ddbed5ba9c28f3a4c0b5f90>>
  */
 
 mod relay_compiler_integration;
@@ -313,6 +313,13 @@ async fn resolvers_schema_module_apply_to_normalization_ast() {
     test_fixture(transform_fixture, file!(), "resolvers_schema_module_apply_to_normalization_ast.input", "relay_compiler_integration/fixtures/resolvers_schema_module_apply_to_normalization_ast.expected", input, expected).await;
 }
 
+#[tokio::test]
+async fn semantic_null_require_bubble_to_required_parent() {
+    let input = include_str!("relay_compiler_integration/fixtures/semantic_null_require_bubble_to_required_parent.input");
+    let expected = include_str!("relay_compiler_integration/fixtures/semantic_null_require_bubble_to_required_parent.expected");
+    test_fixture(transform_fixture, file!(), "semantic_null_require_bubble_to_required_parent.input", "relay_compiler_integration/fixtures/semantic_null_require_bubble_to_required_parent.expected", input, expected).await;
+}
+
 #[tokio::test]
 async fn semantic_null_require_bubbling() {
     let input = include_str!("relay_compiler_integration/fixtures/semantic_null_require_bubbling.input");

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

@@ -23,6 +23,7 @@ use lazy_static::lazy_static;
 use schema::Schema;
 
 use crate::ValidationMessageWithData;
+use crate::CHILDREN_CAN_BUBBLE_METADATA_KEY;
 use crate::REQUIRED_DIRECTIVE_NAME;
 
 lazy_static! {
@@ -122,7 +123,14 @@ impl<'program> DisallowRequiredOnNonNullField<'program> {
     ) -> DiagnosticsResult<()> {
         try_all(selections.iter().map(|selection| match selection {
             Selection::LinkedField(linked_field) => {
-                self.validate_required_field(linked_field, is_throw_on_field_error)?;
+                if linked_field
+                    .directives()
+                    .named(*CHILDREN_CAN_BUBBLE_METADATA_KEY)
+                    .is_none()
+                {
+                    self.validate_required_field(linked_field, is_throw_on_field_error)?;
+                }
+
                 self.validate_selection_fields(&linked_field.selections, is_throw_on_field_error)
             }
             Selection::ScalarField(scalar_field) => {