Skip to content

Commit

Permalink
Add support for @catch on fragments/queries/mutations and aliased inl…
Browse files Browse the repository at this point in the history
…ine fragments (#4838)

Summary:
`catch` was originally conceived as a means for handling field errors, so we built it as a directive on fields. However, as the design evolved we opted to make it additionally catch any errors nested within it. With this newer approach, there’s no reason to limit `catch` to just fields. Any GraphQL construct which gets read as a concrete value should be able to operate as a `catch` boundary.

This PR adds support for marking fields/queries/mutations and aliased fragments as `catch` boundaries. This means:

1. Their value will be read out as either nullable or a `Result` type
2. `semanticNonNull` values nested within the fragment/operation/mutation/aliased fragment will be typed using their semantic type (non-nullable)

One particularly nice thing this enables is that it allows `catch` to act as a non-destructive alternative to `throwOnFieldError`.

## Additional Bug Fixes

Along the way I noticed and fixed a few bugs:

1. Semantic nullability was not respected within inline fragments nested within `catch` or `throwOnFieldError`
2. Errors that bubbled up to a `catch(to: NULL)` directive would not actually cause the value to become null.

Pull Request resolved: #4838

Reviewed By: itamark

Differential Revision: D65432323

Pulled By: captbaritone

fbshipit-source-id: 7fd1313e6d1ae0bda2d6354ef931f10fcfa72e9e
  • Loading branch information
captbaritone authored and facebook-github-bot committed Nov 11, 2024
1 parent 1306086 commit 7c9aebb
Show file tree
Hide file tree
Showing 78 changed files with 3,124 additions and 600 deletions.
40 changes: 30 additions & 10 deletions compiler/crates/relay-codegen/src/build_ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,12 @@ impl<'schema, 'builder, 'config> CodegenBuilder<'schema, 'builder, 'config> {
value: Primitive::Bool(true),
})
}
if let Some(catch_metadata) = CatchMetadataDirective::find(&fragment.directives) {
metadata.push(ObjectEntry {
key: CODEGEN_CONSTANTS.catch_to,
value: Primitive::String(catch_metadata.to.into()),
})
}
if let Some(refetch_metadata) = RefetchableMetadata::find(&fragment.directives) {
let refetch_connection = if let Some(connection_metadata) = connection_metadata {
let metadata = &connection_metadata[0]; // Validated in `transform_refetchable`
Expand Down Expand Up @@ -803,7 +809,7 @@ impl<'schema, 'builder, 'config> CodegenBuilder<'schema, 'builder, 'config> {
if let Some(required_metadata) = RequiredMetadataDirective::find(&field.directives) {
self.build_required_field(required_metadata, resolver_primitive)
} else if let Some(catch_metadata) = CatchMetadataDirective::find(&field.directives) {
self.build_catch_field(catch_metadata, resolver_primitive)
self.build_catch_node(catch_metadata, resolver_primitive)
} else {
resolver_primitive
}
Expand Down Expand Up @@ -1031,7 +1037,7 @@ impl<'schema, 'builder, 'config> CodegenBuilder<'schema, 'builder, 'config> {
}))
}

fn build_catch_field(
fn build_catch_node(
&mut self,
catch_metadata: &CatchMetadataDirective,
primitive: Primitive,
Expand Down Expand Up @@ -1072,7 +1078,7 @@ impl<'schema, 'builder, 'config> CodegenBuilder<'schema, 'builder, 'config> {
if let Some(required_metadata) = RequiredMetadataDirective::find(&field.directives) {
self.build_required_field(required_metadata, primitive)
} else if let Some(catch_metadata) = CatchMetadataDirective::find(&field.directives) {
self.build_catch_field(catch_metadata, primitive)
self.build_catch_node(catch_metadata, primitive)
} else {
primitive
}
Expand Down Expand Up @@ -1172,7 +1178,7 @@ impl<'schema, 'builder, 'config> CodegenBuilder<'schema, 'builder, 'config> {
if let Some(required_metadata) = RequiredMetadataDirective::find(&field.directives) {
self.build_required_field(required_metadata, primitive)
} else if let Some(catch_metadata) = CatchMetadataDirective::find(&field.directives) {
self.build_catch_field(catch_metadata, primitive)
self.build_catch_node(catch_metadata, primitive)
} else {
primitive
}
Expand Down Expand Up @@ -1323,7 +1329,7 @@ impl<'schema, 'builder, 'config> CodegenBuilder<'schema, 'builder, 'config> {
} else if let Some(catch_metadata) =
CatchMetadataDirective::find(&frag_spread.directives)
{
self.build_catch_field(catch_metadata, resolver_primitive)
self.build_catch_node(catch_metadata, resolver_primitive)
} else {
resolver_primitive
}
Expand Down Expand Up @@ -1939,7 +1945,7 @@ impl<'schema, 'builder, 'config> CodegenBuilder<'schema, 'builder, 'config> {
if let Some(required_metadata) = required_metadata {
self.build_required_field(&required_metadata, field)
} else if let Some(catch_metadata) = catch_metadata {
self.build_catch_field(&catch_metadata, field)
self.build_catch_node(&catch_metadata, field)
} else {
field
}
Expand Down Expand Up @@ -1997,11 +2003,18 @@ impl<'schema, 'builder, 'config> CodegenBuilder<'schema, 'builder, 'config> {
type_: Primitive::SkippableNull,
abstract_key: Primitive::SkippableNull,
}));
Primitive::Key(self.object(object! {
let aliased_fragment = Primitive::Key(self.object(object! {
fragment: primitive,
kind: Primitive::String(CODEGEN_CONSTANTS.aliased_inline_fragment_spread),
name: Primitive::String(fragment_alias_metadata.alias.item),
}))
}));
if let Some(catch_metadata) =
CatchMetadataDirective::find(&inline_frag.directives)
{
self.build_catch_node(catch_metadata, aliased_fragment)
} else {
aliased_fragment
}
} else {
// TODO(T63559346): Handle anonymous inline fragments with no directives
panic!(
Expand Down Expand Up @@ -2080,11 +2093,18 @@ impl<'schema, 'builder, 'config> CodegenBuilder<'schema, 'builder, 'config> {
if let Some(fragment_alias_metadata) =
FragmentAliasMetadata::find(&inline_frag.directives)
{
Primitive::Key(self.object(object! {
let aliased_fragment = Primitive::Key(self.object(object! {
fragment: primitive,
kind: Primitive::String(CODEGEN_CONSTANTS.aliased_inline_fragment_spread),
name: Primitive::String(fragment_alias_metadata.alias.item),
}))
}));
if let Some(catch_metadata) =
CatchMetadataDirective::find(&inline_frag.directives)
{
self.build_catch_node(catch_metadata, aliased_fragment)
} else {
aliased_fragment
}
} else {
primitive
}
Expand Down
2 changes: 2 additions & 0 deletions compiler/crates/relay-codegen/src/constants.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ pub struct CodegenConstants {
pub backward: StringKey,
pub cache_id: StringKey,
pub catch_field: StringKey,
pub catch_to: StringKey,
pub client_abstract_types: StringKey,
pub client_edge_backing_field_key: StringKey,
pub client_edge_model_resolvers: StringKey,
Expand Down Expand Up @@ -141,6 +142,7 @@ lazy_static! {
backward: "backward".intern(),
cache_id: "cacheID".intern(),
catch_field: "CatchField".intern(),
catch_to: "catchTo".intern(),
client_abstract_types: "clientAbstractTypes".intern(),
client_edge_backing_field_key: "backingField".intern(),
client_edge_model_resolvers: "modelResolvers".intern(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

use std::sync::Arc;

use common::FeatureFlag;
use common::SourceLocationKey;
use fixture_tests::Fixture;
use graphql_ir::build;
Expand All @@ -20,6 +21,7 @@ use relay_config::ProjectConfig;
use relay_test_schema::get_test_schema;
use relay_test_schema::get_test_schema_with_extensions;
use relay_transforms::catch_directive;
use relay_transforms::fragment_alias_directive;

pub async fn transform_fixture(fixture: &Fixture<'_>) -> Result<String, String> {
let parts: Vec<_> = fixture.content.split("%extensions%").collect();
Expand All @@ -34,7 +36,8 @@ pub async fn transform_fixture(fixture: &Fixture<'_>) -> Result<String, String>
.map_err(|diagnostics| diagnostics_to_sorted_string(fixture.content, &diagnostics))?;
let program = Program::from_definitions(Arc::clone(&schema), ir);

catch_directive(&program)
fragment_alias_directive(&program, &FeatureFlag::Disabled)
.and_then(|next_program| catch_directive(&next_program))
.map(|next_program| {
next_program
.fragments()
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
==================================== INPUT ====================================
fragment MyFragment on Node {
id
... on User @catch(to: RESULT) @alias {
name
}
}
==================================== OUTPUT ===================================
{
"argumentDefinitions": [],
"kind": "Fragment",
"metadata": null,
"name": "MyFragment",
"selections": [
{
"alias": null,
"args": null,
"kind": "ScalarField",
"name": "id",
"storageKey": null
},
{
"kind": "CatchField",
"field": {
"fragment": {
"kind": "InlineFragment",
"selections": [
{
"alias": null,
"args": null,
"kind": "ScalarField",
"name": "name",
"storageKey": null
}
],
"type": "User",
"abstractKey": null
},
"kind": "AliasedInlineFragmentSpread",
"name": "User"
},
"to": "RESULT",
"path": "User"
}
],
"type": "Node",
"abstractKey": "__isNode"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
fragment MyFragment on Node {
id
... on User @catch(to: RESULT) @alias {
name
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
==================================== INPUT ====================================
fragment MyFragment on Node {
id
... @catch(to: RESULT) @alias(as: "myAlias") {
name
}
}
==================================== OUTPUT ===================================
{
"argumentDefinitions": [],
"kind": "Fragment",
"metadata": null,
"name": "MyFragment",
"selections": [
{
"alias": null,
"args": null,
"kind": "ScalarField",
"name": "id",
"storageKey": null
},
{
"kind": "CatchField",
"field": {
"fragment": {
"kind": "InlineFragment",
"selections": [
{
"alias": null,
"args": null,
"kind": "ScalarField",
"name": "name",
"storageKey": null
}
],
"type": null,
"abstractKey": null
},
"kind": "AliasedInlineFragmentSpread",
"name": "myAlias"
},
"to": "RESULT",
"path": "myAlias"
}
],
"type": "Node",
"abstractKey": "__isNode"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
fragment MyFragment on Node {
id
... @catch(to: RESULT) @alias(as: "myAlias") {
name
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
==================================== INPUT ====================================
fragment MyFragment on Node @catch(to: RESULT) {
id
name
}
==================================== OUTPUT ===================================
{
"argumentDefinitions": [],
"kind": "Fragment",
"metadata": {
"catchTo": "RESULT"
},
"name": "MyFragment",
"selections": [
{
"alias": null,
"args": null,
"kind": "ScalarField",
"name": "id",
"storageKey": null
},
{
"alias": null,
"args": null,
"kind": "ScalarField",
"name": "name",
"storageKey": null
}
],
"type": "Node",
"abstractKey": "__isNode"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
fragment MyFragment on Node @catch(to: RESULT) {
id
name
}
Original file line number Diff line number Diff line change
Expand Up @@ -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<<9b3e9405c461aea8f7d13327590c2c37>>
* @generated SignedSource<<517598dad58115fe478729b087af3fff>>
*/

mod catch_directive_codegen;
Expand All @@ -19,6 +19,27 @@ async fn catch_directive() {
test_fixture(transform_fixture, file!(), "catch_directive.graphql", "catch_directive_codegen/fixtures/catch_directive.expected", input, expected).await;
}

#[tokio::test]
async fn catch_directive_aliased_inline_fragment() {
let input = include_str!("catch_directive_codegen/fixtures/catch_directive_aliased_inline_fragment.graphql");
let expected = include_str!("catch_directive_codegen/fixtures/catch_directive_aliased_inline_fragment.expected");
test_fixture(transform_fixture, file!(), "catch_directive_aliased_inline_fragment.graphql", "catch_directive_codegen/fixtures/catch_directive_aliased_inline_fragment.expected", input, expected).await;
}

#[tokio::test]
async fn catch_directive_aliased_inline_fragment_no_condition() {
let input = include_str!("catch_directive_codegen/fixtures/catch_directive_aliased_inline_fragment_no_condition.graphql");
let expected = include_str!("catch_directive_codegen/fixtures/catch_directive_aliased_inline_fragment_no_condition.expected");
test_fixture(transform_fixture, file!(), "catch_directive_aliased_inline_fragment_no_condition.graphql", "catch_directive_codegen/fixtures/catch_directive_aliased_inline_fragment_no_condition.expected", input, expected).await;
}

#[tokio::test]
async fn catch_directive_fragment() {
let input = include_str!("catch_directive_codegen/fixtures/catch_directive_fragment.graphql");
let expected = include_str!("catch_directive_codegen/fixtures/catch_directive_fragment.expected");
test_fixture(transform_fixture, file!(), "catch_directive_fragment.graphql", "catch_directive_codegen/fixtures/catch_directive_fragment.expected", input, expected).await;
}

#[tokio::test]
async fn catch_directive_linked_child_has_to_result() {
let input = include_str!("catch_directive_codegen/fixtures/catch_directive_linked_child_has_to_result.graphql");
Expand Down
Loading

0 comments on commit 7c9aebb

Please sign in to comment.