From f908dee66672e3b7ffc97a4552a68196ec745974 Mon Sep 17 00:00:00 2001 From: Monica Tang Date: Mon, 18 Mar 2024 11:51:19 -0700 Subject: [PATCH] push validation on weak type/legacy syntax to before model resolver creation Summary: ## Context If the model that the client edge points to is a weak type or if the client edge is pointing to an object defined with legacy syntax, we skip creating model resolvers (for weak client edges, the model resolvers are unnecessary and for legacy edges, the model resolvers don't work properly since the generated id fragment paths don't match). Before, we were checking these two properties in `build_ast`, but we can do so in the `client_edges` transform to avoid generating the model resolver IR when unnecessary. ## This diff Refactors the existing implementation to check for weak and legacy types in `client_edges` transform. Reviewed By: captbaritone Differential Revision: D54556696 fbshipit-source-id: 3e4444856bc8ac66e24bfc12ddcd19745d4c019c --- .../crates/relay-codegen/src/build_ast.rs | 48 +++++------ .../relay-transforms/src/client_edges.rs | 80 +++++++++++++------ .../crates/relay-transforms/src/errors.rs | 8 ++ .../client-edge-to-client-interface.expected | 4 +- .../client-edge-to-client-interface.graphql | 1 + ...-edge-to-client-interface.invalid.expected | 1 + ...t-edge-to-client-interface.invalid.graphql | 1 + .../client-edge-to-client-object.expected | 13 +-- .../fixtures/nested-path-with-alias.expected | 26 +----- .../fixtures/nested-path.expected | 26 +----- .../fixtures/output-type.expected | 13 +-- ...with-output-type-client-interface.expected | 1 + ...-with-output-type-client-interface.graphql | 1 + ...with-output-type-client-interface.expected | 1 + ...-with-output-type-client-interface.graphql | 1 + 15 files changed, 99 insertions(+), 126 deletions(-) diff --git a/compiler/crates/relay-codegen/src/build_ast.rs b/compiler/crates/relay-codegen/src/build_ast.rs index 2fccde7eab520..96f10b5c2eac8 100644 --- a/compiler/crates/relay-codegen/src/build_ast.rs +++ b/compiler/crates/relay-codegen/src/build_ast.rs @@ -1281,19 +1281,15 @@ impl<'schema, 'builder, 'config> CodegenBuilder<'schema, 'builder, 'config> { ) -> Vec { model_resolvers .iter() - .filter_map(|model_resolver| { - if model_resolver.has_model_instance_field { - let type_name = model_resolver.type_name.item.0; - Some(ObjectEntry { - key: type_name, - value: self.build_client_edge_model_resolver( - model_resolver.type_name, - model_resolver.is_live, - relay_resolver_metadata, - ), - }) - } else { - None + .map(|model_resolver| { + let type_name = model_resolver.type_name.item.0; + ObjectEntry { + key: type_name, + value: self.build_client_edge_model_resolver( + model_resolver.type_name, + model_resolver.is_live, + relay_resolver_metadata, + ), } }) .collect() @@ -1803,19 +1799,19 @@ impl<'schema, 'builder, 'config> CodegenBuilder<'schema, 'builder, 'config> { } else { None } - }); - let client_edge_model_resolvers = if let Some(model_resolver_field) = model_resolver_field { - Primitive::Key(model_resolver_field) - } else { - Primitive::Null - }; - Primitive::Key(self.object(object! { - kind: Primitive::String(CODEGEN_CONSTANTS.client_edge_to_client_object), - concrete_type: concrete_type, - client_edge_model_resolvers: client_edge_model_resolvers, - client_edge_backing_field_key: backing_field, - client_edge_selections_key: selections_item, - })) + }); + let client_edge_model_resolvers = if let Some(model_resolver_field) = model_resolver_field { + Primitive::Key(model_resolver_field) + } else { + Primitive::Null + }; + Primitive::Key(self.object(object! { + kind: Primitive::String(CODEGEN_CONSTANTS.client_edge_to_client_object), + concrete_type: concrete_type, + client_edge_model_resolvers: client_edge_model_resolvers, + client_edge_backing_field_key: backing_field, + client_edge_selections_key: selections_item, + })) } } }; diff --git a/compiler/crates/relay-transforms/src/client_edges.rs b/compiler/crates/relay-transforms/src/client_edges.rs index f531b831bf008..0500187fdfe8e 100644 --- a/compiler/crates/relay-transforms/src/client_edges.rs +++ b/compiler/crates/relay-transforms/src/client_edges.rs @@ -19,6 +19,7 @@ use docblock_shared::HAS_OUTPUT_TYPE_ARGUMENT_NAME; use docblock_shared::LIVE_ARGUMENT_NAME; use docblock_shared::RELAY_RESOLVER_DIRECTIVE_NAME; use docblock_shared::RELAY_RESOLVER_MODEL_INSTANCE_FIELD; +use docblock_shared::RELAY_RESOLVER_WEAK_OBJECT_DIRECTIVE; use graphql_ir::associated_data_impl; use graphql_ir::Argument; use graphql_ir::ConstantValue; @@ -90,7 +91,6 @@ associated_data_impl!(ClientEdgeMetadataDirective); #[derive(Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)] pub struct ClientEdgeModelResolver { pub type_name: WithLocation, - pub has_model_instance_field: bool, pub is_live: bool, } @@ -383,7 +383,22 @@ impl<'program, 'pc> ClientEdgesTransform<'program, 'pc> { } let mut model_resolvers: Vec = implementing_objects .iter() - .map(|object_id| self.get_client_edge_model_resolver_for_object(*object_id)) + .filter_map(|object_id| { + let model_resolver = self.get_client_edge_model_resolver_for_object(*object_id); + model_resolver.or_else(|| { + if !self.is_weak_type(*object_id) && self.is_resolver_type(*object_id) { + let model_name = self.program.schema.object(*object_id).name; + self.errors.push(Diagnostic::error( + ValidationMessage::ClientEdgeToClientInterfaceImplementingObjectMissingModelResolver { + interface_name: interface.name.item, + type_name: model_name.item, + }, + model_name.location, + )); + } + None + }) + }) .collect(); model_resolvers.sort(); Some(ClientEdgeMetadataDirective::ClientObject { @@ -402,10 +417,12 @@ impl<'program, 'pc> ClientEdgesTransform<'program, 'pc> { } Type::Object(object_id) => { let type_name = self.program.schema.object(object_id).name.item; - let model_resolver = self.get_client_edge_model_resolver_for_object(object_id); + let model_resolvers = self + .get_client_edge_model_resolver_for_object(object_id) + .map_or(vec![], |model_resolver| vec![model_resolver]); Some(ClientEdgeMetadataDirective::ClientObject { type_name: Some(type_name), - model_resolvers: vec![model_resolver], + model_resolvers, unique_id: self.get_key(), }) } @@ -415,36 +432,47 @@ impl<'program, 'pc> ClientEdgesTransform<'program, 'pc> { } } + fn is_weak_type(&self, object_id: ObjectID) -> bool { + let object = self.program.schema.object(object_id); + object + .directives + .named(*RELAY_RESOLVER_WEAK_OBJECT_DIRECTIVE) + .is_some() + } + + fn is_resolver_type(&self, object_id: ObjectID) -> bool { + let object = self.program.schema.object(object_id); + object + .directives + .named(*RELAY_RESOLVER_DIRECTIVE_NAME) + .is_some() + } + fn get_client_edge_model_resolver_for_object( &mut self, object_id: ObjectID, - ) -> ClientEdgeModelResolver { + ) -> Option { let type_name = self.program.schema.object(object_id).name; - let parent_type = self.program.schema.get_type(type_name.item.0).unwrap(); + let model_type = self.program.schema.get_type(type_name.item.0).unwrap(); let model_field_id = self .program .schema - .named_field(parent_type, *RELAY_RESOLVER_MODEL_INSTANCE_FIELD); - // Note: is_live is only true if the __relay_model_instance field exists on the model field - let is_live = if let Some(id) = model_field_id { - let model_field = self.program.schema.field(id); - let resolver_directive = model_field.directives.named(*RELAY_RESOLVER_DIRECTIVE_NAME); - if let Some(resolver_directive) = resolver_directive { - resolver_directive - .arguments - .iter() - .any(|arg| arg.name.0 == LIVE_ARGUMENT_NAME.0) - } else { - false - } - } else { - false - }; - ClientEdgeModelResolver { - type_name, - has_model_instance_field: model_field_id.is_some(), - is_live, + .named_field(model_type, *RELAY_RESOLVER_MODEL_INSTANCE_FIELD); + // Legacy models (using verbose resolver syntax) do not have a __relay_model_instance field + if self.is_weak_type(object_id) { + return None; } + let id = model_field_id?; + // Note: is_live is only true if the __relay_model_instance field exists on the model field + let model_field = self.program.schema.field(id); + let resolver_directive = model_field.directives.named(*RELAY_RESOLVER_DIRECTIVE_NAME); + let is_live = resolver_directive.map_or(false, |resolver_directive| { + resolver_directive + .arguments + .iter() + .any(|arg| arg.name.0 == LIVE_ARGUMENT_NAME.0) + }); + Some(ClientEdgeModelResolver { type_name, is_live }) } fn get_edge_to_server_object_metadata_directive( diff --git a/compiler/crates/relay-transforms/src/errors.rs b/compiler/crates/relay-transforms/src/errors.rs index 177494afd3fc7..b1f9fb3970d18 100644 --- a/compiler/crates/relay-transforms/src/errors.rs +++ b/compiler/crates/relay-transforms/src/errors.rs @@ -129,6 +129,14 @@ pub enum ValidationMessage { )] ClientEdgeToClientInterface, + #[error( + "The client edge pointing to the interface `{interface_name}` with implementing object, `{type_name}`, is missing its corresponding model resolver. The concrete type `{type_name}` and its resolver fields should be defined with the newer dot notation resolver syntax. See https://relay.dev/docs/guides/relay-resolvers/." + )] + ClientEdgeToClientInterfaceImplementingObjectMissingModelResolver { + interface_name: InterfaceName, + type_name: ObjectName, + }, + #[error( "Client Edges that reference client-defined union types are not currently supported in Relay." )] diff --git a/compiler/crates/relay-transforms/tests/client_edges/fixtures/client-edge-to-client-interface.expected b/compiler/crates/relay-transforms/tests/client_edges/fixtures/client-edge-to-client-interface.expected index 7f5955563ff63..6cc84ba61f937 100644 --- a/compiler/crates/relay-transforms/tests/client_edges/fixtures/client-edge-to-client-interface.expected +++ b/compiler/crates/relay-transforms/tests/client_edges/fixtures/client-edge-to-client-interface.expected @@ -20,6 +20,7 @@ interface ClientOnlyInterface implements Node { # Add a concrete type so that we don't trigger an unrelated compiler error. type BestFriend implements ClientOnlyInterface { id: ID! + __relay_model_instance: RelayResolverValue @relay_resolver(import_path: "BestFriendResolver" fragment_name: "BestFriend__id", generated_fragment: true, inject_fragment_data: "id", import_name: "BestFriend") } extend type User { @@ -43,7 +44,6 @@ fragment Foo_user on User { # "BestFriend", # ), # }, - # has_model_instance_field: false, # is_live: false, # }, # ], @@ -51,7 +51,7 @@ fragment Foo_user on User { { ...BestFriendResolverFragment_name @__RelayResolverMetadata # RelayResolverMetadata { - # field_id: FieldID(525), + # field_id: FieldID(526), # import_path: "BestFriendResolver", # import_name: None, # field_alias: None, diff --git a/compiler/crates/relay-transforms/tests/client_edges/fixtures/client-edge-to-client-interface.graphql b/compiler/crates/relay-transforms/tests/client_edges/fixtures/client-edge-to-client-interface.graphql index e161f051d7f25..cacc8c352852f 100644 --- a/compiler/crates/relay-transforms/tests/client_edges/fixtures/client-edge-to-client-interface.graphql +++ b/compiler/crates/relay-transforms/tests/client_edges/fixtures/client-edge-to-client-interface.graphql @@ -19,6 +19,7 @@ interface ClientOnlyInterface implements Node { # Add a concrete type so that we don't trigger an unrelated compiler error. type BestFriend implements ClientOnlyInterface { id: ID! + __relay_model_instance: RelayResolverValue @relay_resolver(import_path: "BestFriendResolver" fragment_name: "BestFriend__id", generated_fragment: true, inject_fragment_data: "id", import_name: "BestFriend") } extend type User { diff --git a/compiler/crates/relay-transforms/tests/client_edges/fixtures/client-edge-to-client-interface.invalid.expected b/compiler/crates/relay-transforms/tests/client_edges/fixtures/client-edge-to-client-interface.invalid.expected index 0507ac5f45373..37bc1c2576445 100644 --- a/compiler/crates/relay-transforms/tests/client_edges/fixtures/client-edge-to-client-interface.invalid.expected +++ b/compiler/crates/relay-transforms/tests/client_edges/fixtures/client-edge-to-client-interface.invalid.expected @@ -19,6 +19,7 @@ interface ClientOnlyInterface implements Node { # Add a concrete type so that we don't trigger an unrelated compiler error. type BestFriend implements ClientOnlyInterface { id: ID! + __relay_model_instance: RelayResolverValue @relay_resolver(import_path: "BestFriendResolver" fragment_name: "BestFriend__id", generated_fragment: true, inject_fragment_data: "id", import_name: "BestFriend") } extend type User { diff --git a/compiler/crates/relay-transforms/tests/client_edges/fixtures/client-edge-to-client-interface.invalid.graphql b/compiler/crates/relay-transforms/tests/client_edges/fixtures/client-edge-to-client-interface.invalid.graphql index 9a3073baefde6..f4b746c0c9892 100644 --- a/compiler/crates/relay-transforms/tests/client_edges/fixtures/client-edge-to-client-interface.invalid.graphql +++ b/compiler/crates/relay-transforms/tests/client_edges/fixtures/client-edge-to-client-interface.invalid.graphql @@ -18,6 +18,7 @@ interface ClientOnlyInterface implements Node { # Add a concrete type so that we don't trigger an unrelated compiler error. type BestFriend implements ClientOnlyInterface { id: ID! + __relay_model_instance: RelayResolverValue @relay_resolver(import_path: "BestFriendResolver" fragment_name: "BestFriend__id", generated_fragment: true, inject_fragment_data: "id", import_name: "BestFriend") } extend type User { diff --git a/compiler/crates/relay-transforms/tests/client_edges/fixtures/client-edge-to-client-object.expected b/compiler/crates/relay-transforms/tests/client_edges/fixtures/client-edge-to-client-object.expected index b08be1b457040..c2a29d3c90acb 100644 --- a/compiler/crates/relay-transforms/tests/client_edges/fixtures/client-edge-to-client-object.expected +++ b/compiler/crates/relay-transforms/tests/client_edges/fixtures/client-edge-to-client-object.expected @@ -32,18 +32,7 @@ fragment Foo_user on User { # ), # ), # unique_id: 0, - # model_resolvers: [ - # ClientEdgeModelResolver { - # type_name: WithLocation { - # location: :7:21, - # item: ObjectName( - # "ClientOnlyType", - # ), - # }, - # has_model_instance_field: false, - # is_live: false, - # }, - # ], + # model_resolvers: [], # } { ...BestFriendResolverFragment_name @__RelayResolverMetadata diff --git a/compiler/crates/relay-transforms/tests/client_edges/fixtures/nested-path-with-alias.expected b/compiler/crates/relay-transforms/tests/client_edges/fixtures/nested-path-with-alias.expected index 1efbcd808c3f8..a7151fd7cf8b9 100644 --- a/compiler/crates/relay-transforms/tests/client_edges/fixtures/nested-path-with-alias.expected +++ b/compiler/crates/relay-transforms/tests/client_edges/fixtures/nested-path-with-alias.expected @@ -38,18 +38,7 @@ fragment Foo_user on ClientUser { # ), # ), # unique_id: 1, - # model_resolvers: [ - # ClientEdgeModelResolver { - # type_name: WithLocation { - # location: :7:17, - # item: ObjectName( - # "ClientUser", - # ), - # }, - # has_model_instance_field: false, - # is_live: false, - # }, - # ], + # model_resolvers: [], # } { ...BestFriendFragment @__RelayResolverMetadata @@ -76,18 +65,7 @@ fragment Foo_user on ClientUser { # ), # ), # unique_id: 0, - # model_resolvers: [ - # ClientEdgeModelResolver { - # type_name: WithLocation { - # location: :7:17, - # item: ObjectName( - # "ClientUser", - # ), - # }, - # has_model_instance_field: false, - # is_live: false, - # }, - # ], + # model_resolvers: [], # } { ...BestFriendFragment @__RelayResolverMetadata diff --git a/compiler/crates/relay-transforms/tests/client_edges/fixtures/nested-path.expected b/compiler/crates/relay-transforms/tests/client_edges/fixtures/nested-path.expected index cb7532d4a8daa..81d462b49057c 100644 --- a/compiler/crates/relay-transforms/tests/client_edges/fixtures/nested-path.expected +++ b/compiler/crates/relay-transforms/tests/client_edges/fixtures/nested-path.expected @@ -38,18 +38,7 @@ fragment Foo_user on ClientUser { # ), # ), # unique_id: 1, - # model_resolvers: [ - # ClientEdgeModelResolver { - # type_name: WithLocation { - # location: :7:17, - # item: ObjectName( - # "ClientUser", - # ), - # }, - # has_model_instance_field: false, - # is_live: false, - # }, - # ], + # model_resolvers: [], # } { ...BestFriendFragment @__RelayResolverMetadata @@ -74,18 +63,7 @@ fragment Foo_user on ClientUser { # ), # ), # unique_id: 0, - # model_resolvers: [ - # ClientEdgeModelResolver { - # type_name: WithLocation { - # location: :7:17, - # item: ObjectName( - # "ClientUser", - # ), - # }, - # has_model_instance_field: false, - # is_live: false, - # }, - # ], + # model_resolvers: [], # } { ...BestFriendFragment @__RelayResolverMetadata diff --git a/compiler/crates/relay-transforms/tests/client_edges/fixtures/output-type.expected b/compiler/crates/relay-transforms/tests/client_edges/fixtures/output-type.expected index ad58a96d62d8f..c8cddde9388b9 100644 --- a/compiler/crates/relay-transforms/tests/client_edges/fixtures/output-type.expected +++ b/compiler/crates/relay-transforms/tests/client_edges/fixtures/output-type.expected @@ -32,18 +32,7 @@ fragment Foo_user on User { # ), # ), # unique_id: 0, - # model_resolvers: [ - # ClientEdgeModelResolver { - # type_name: WithLocation { - # location: :7:17, - # item: ObjectName( - # "ClientUser", - # ), - # }, - # has_model_instance_field: false, - # is_live: false, - # }, - # ], + # model_resolvers: [], # } { ...BestFriendResolverFragment_name @__RelayResolverMetadata diff --git a/compiler/crates/relay-typegen/tests/generate_flow/fixtures/relay-resolver-with-output-type-client-interface.expected b/compiler/crates/relay-typegen/tests/generate_flow/fixtures/relay-resolver-with-output-type-client-interface.expected index ecf652835730d..272236b150b4e 100644 --- a/compiler/crates/relay-typegen/tests/generate_flow/fixtures/relay-resolver-with-output-type-client-interface.expected +++ b/compiler/crates/relay-typegen/tests/generate_flow/fixtures/relay-resolver-with-output-type-client-interface.expected @@ -25,6 +25,7 @@ interface ClientInterface { type ClientType implements ClientInterface { name: String + __relay_model_instance: RelayResolverValue @relay_resolver(import_path: "ClientTypeResolver" fragment_name: "ClientType__id", generated_fragment: true, inject_fragment_data: "id", import_name: "ClientType") } type ClientTypeWithNestedInterface { diff --git a/compiler/crates/relay-typegen/tests/generate_flow/fixtures/relay-resolver-with-output-type-client-interface.graphql b/compiler/crates/relay-typegen/tests/generate_flow/fixtures/relay-resolver-with-output-type-client-interface.graphql index d6716859b05da..82aa40aac3901 100644 --- a/compiler/crates/relay-typegen/tests/generate_flow/fixtures/relay-resolver-with-output-type-client-interface.graphql +++ b/compiler/crates/relay-typegen/tests/generate_flow/fixtures/relay-resolver-with-output-type-client-interface.graphql @@ -24,6 +24,7 @@ interface ClientInterface { type ClientType implements ClientInterface { name: String + __relay_model_instance: RelayResolverValue @relay_resolver(import_path: "ClientTypeResolver" fragment_name: "ClientType__id", generated_fragment: true, inject_fragment_data: "id", import_name: "ClientType") } type ClientTypeWithNestedInterface { diff --git a/compiler/crates/relay-typegen/tests/generate_typescript/fixtures/relay-resolver-with-output-type-client-interface.expected b/compiler/crates/relay-typegen/tests/generate_typescript/fixtures/relay-resolver-with-output-type-client-interface.expected index 26837e2290f0b..0e483e7a93213 100644 --- a/compiler/crates/relay-typegen/tests/generate_typescript/fixtures/relay-resolver-with-output-type-client-interface.expected +++ b/compiler/crates/relay-typegen/tests/generate_typescript/fixtures/relay-resolver-with-output-type-client-interface.expected @@ -25,6 +25,7 @@ interface ClientInterface { type ClientType implements ClientInterface { name: String + __relay_model_instance: RelayResolverValue @relay_resolver(import_path: "ClientTypeResolver" fragment_name: "ClientType__id", generated_fragment: true, inject_fragment_data: "id", import_name: "ClientType") } type ClientTypeWithNestedInterface { diff --git a/compiler/crates/relay-typegen/tests/generate_typescript/fixtures/relay-resolver-with-output-type-client-interface.graphql b/compiler/crates/relay-typegen/tests/generate_typescript/fixtures/relay-resolver-with-output-type-client-interface.graphql index 15faa3afe0c30..7d40421cee19b 100644 --- a/compiler/crates/relay-typegen/tests/generate_typescript/fixtures/relay-resolver-with-output-type-client-interface.graphql +++ b/compiler/crates/relay-typegen/tests/generate_typescript/fixtures/relay-resolver-with-output-type-client-interface.graphql @@ -24,6 +24,7 @@ interface ClientInterface { type ClientType implements ClientInterface { name: String + __relay_model_instance: RelayResolverValue @relay_resolver(import_path: "ClientTypeResolver" fragment_name: "ClientType__id", generated_fragment: true, inject_fragment_data: "id", import_name: "ClientType") } type ClientTypeWithNestedInterface {