Skip to content

Commit

Permalink
Dissallow some readtime fatures in mutation responses
Browse files Browse the repository at this point in the history
Reviewed By: alunyov

Differential Revision: D52644165

fbshipit-source-id: ab8d8326e16d85b6f62754bd6c30fb7e340b9e8b
  • Loading branch information
captbaritone authored and facebook-github-bot committed Jan 19, 2024
1 parent 2f9e88a commit ada971e
Show file tree
Hide file tree
Showing 29 changed files with 517 additions and 48 deletions.
23 changes: 16 additions & 7 deletions compiler/crates/common/src/feature_flags.rs
Expand Up @@ -84,12 +84,27 @@ pub struct FeatureFlags {
/// Perform strict validations when custom scalar types are used
#[serde(default)]
pub enable_strict_custom_scalars: bool,

/// Relay Resolvers are a read-time feature that are not actually handled in
/// our mutation APIs. We are in the process of removing any existing
/// examples, but this flag is part of a process of removing any existing
/// examples.
#[serde(default)]
pub allow_resolvers_in_mutation_response: FeatureFlag,

/// @required with an action of THROW is read-time feature that is not
/// compatible with our mutation APIs. We are in the process of removing
/// any existing examples, but this flag is part of a process of removing
/// any existing examples.
#[serde(default)]
pub allow_required_in_mutation_response: FeatureFlag,
}

#[derive(Debug, Deserialize, Clone, Serialize)]
#[derive(Debug, Deserialize, Clone, Serialize, Default)]
#[serde(tag = "kind", rename_all = "lowercase")]
pub enum FeatureFlag {
/// Fully disabled: developers may not use this feature
#[default]
Disabled,

/// Fully enabled: developers may use this feature
Expand All @@ -102,12 +117,6 @@ pub enum FeatureFlag {
Rollout { rollout: Rollout },
}

impl Default for FeatureFlag {
fn default() -> Self {
FeatureFlag::Disabled
}
}

impl FeatureFlag {
pub fn is_enabled_for(&self, name: StringKey) -> bool {
match self {
Expand Down
10 changes: 10 additions & 0 deletions compiler/crates/graphql-ir/src/errors.rs
Expand Up @@ -516,6 +516,16 @@ pub enum ValidationMessage {
literal_kind: String,
scalar_type_name: ScalarName,
},

#[error(
"Unexpected `@required(action: THROW)` directive in mutation response. The use of `@required(action: THROW)` is not supported in mutations."
)]
RequiredInMutation,

#[error(
"Unexpected `@RelayResolver` field referenced in mutation response. Relay Resolver fields may not be read as part of a mutation response."
)]
ResolverInMutation,
}

#[derive(Clone, Debug, Error, Eq, PartialEq, Ord, PartialOrd, Hash)]
Expand Down
11 changes: 11 additions & 0 deletions compiler/crates/relay-compiler/src/build_project/validate.rs
Expand Up @@ -15,6 +15,7 @@ use errors::try_all;
use graphql_ir::Program;
use relay_config::ProjectConfig;
use relay_transforms::disallow_circular_no_inline_fragments;
use relay_transforms::disallow_readtime_features_in_mutations;
use relay_transforms::disallow_reserved_aliases;
use relay_transforms::disallow_typename_on_root;
use relay_transforms::validate_assignable_directive;
Expand Down Expand Up @@ -67,6 +68,16 @@ pub fn validate(
} else {
Ok(())
},
disallow_readtime_features_in_mutations(
program,
&project_config
.feature_flags
.allow_resolvers_in_mutation_response,
&project_config
.feature_flags
.allow_required_in_mutation_response,
project_config.feature_flags.enable_relay_resolver_mutations,
),
]);

match output {
Expand Down
Expand Up @@ -122,6 +122,8 @@ pub async fn transform_fixture(fixture: &Fixture<'_>) -> Result<String, String>
relay_resolvers_allow_legacy_verbose_syntax: FeatureFlag::Disabled,
enable_relay_resolver_mutations: false,
enable_strict_custom_scalars: false,
allow_required_in_mutation_response: FeatureFlag::Disabled,
allow_resolvers_in_mutation_response: FeatureFlag::Disabled,
};

let default_project_config = ProjectConfig {
Expand Down
Expand Up @@ -101,6 +101,8 @@ pub async fn transform_fixture(fixture: &Fixture<'_>) -> Result<String, String>
relay_resolvers_allow_legacy_verbose_syntax: FeatureFlag::Disabled,
enable_relay_resolver_mutations: false,
enable_strict_custom_scalars: false,
allow_required_in_mutation_response: FeatureFlag::Disabled,
allow_resolvers_in_mutation_response: FeatureFlag::Disabled,
};

let default_schema_config = SchemaConfig::default();
Expand Down
6 changes: 5 additions & 1 deletion compiler/crates/relay-transforms/Cargo.toml
@@ -1,4 +1,4 @@
# @generated by autocargo from //relay/oss/crates/relay-transforms:[apply_fragment_arguments_test,assignable_directive_test,assignable_fragment_spread_test,client_edges_test,client_extensions_test,declarative_connection_test,disallow_typename_on_root_test,fragment_alias_directive_test,generate_data_driven_dependency_metadata_test,generate_live_query_metadata_test,generate_relay_resolvers_operations_for_nested_objects_test,graphql-client_extensions_abstract_types-test,graphql-defer_stream-test,graphql-disallow_non_node_id_fields-test,graphql-disallow_reserved_aliases-test,graphql-flatten-test,graphql-generate_id_field-test,graphql-generate_typename-test,graphql-inline_fragments-test,graphql-mask-test,graphql-match-local-test,graphql-match-test,graphql-node_identifier-test,graphql-refetchable_fragment_test,graphql-skip_client_extensions-test,graphql-skip_redundant_nodes-test,graphql-skip_unreachable_nodes-test,graphql-sort_selections-test,graphql-subscription_transform-test,graphql-validate_deprecated_fields_test,graphql-validate_module_names-test,graphql-validate_relay_directives-test,graphql-validate_required_arguments_test,graphql-validate_server_only_directives-test,graphql-validate_unused_variables-test,inline_data_fragment_test,provided-variable-fragment-transform-test,relay-actor-change-test,relay-transforms,relay_resolvers_abstract_types_test,relay_resolvers_test,relay_test_operation_test,required_directive_test,skip_unused_variables_test,transform_connections_test,updatable_directive_test,updatable_fragment_spread_test,validate_connections_schema_test,validate_connections_test,validate_global_variable_names_test,validate_global_variables-test,validate_no_double_underscore_alias_test,validate_no_unselectable_selections_test,validate_static_args]
# @generated by autocargo from //relay/oss/crates/relay-transforms:[apply_fragment_arguments_test,assignable_directive_test,assignable_fragment_spread_test,client_edges_test,client_extensions_test,declarative_connection_test,disallow_typename_on_root_test,fragment_alias_directive_test,generate_data_driven_dependency_metadata_test,generate_live_query_metadata_test,generate_relay_resolvers_operations_for_nested_objects_test,graphql-client_extensions_abstract_types-test,graphql-defer_stream-test,graphql-disallow_non_node_id_fields-test,graphql-disallow_reserved_aliases-test,graphql-disallowreadtime_features_in_mutations-test,graphql-flatten-test,graphql-generate_id_field-test,graphql-generate_typename-test,graphql-inline_fragments-test,graphql-mask-test,graphql-match-local-test,graphql-match-test,graphql-node_identifier-test,graphql-refetchable_fragment_test,graphql-skip_client_extensions-test,graphql-skip_redundant_nodes-test,graphql-skip_unreachable_nodes-test,graphql-sort_selections-test,graphql-subscription_transform-test,graphql-validate_deprecated_fields_test,graphql-validate_module_names-test,graphql-validate_relay_directives-test,graphql-validate_required_arguments_test,graphql-validate_server_only_directives-test,graphql-validate_unused_variables-test,inline_data_fragment_test,provided-variable-fragment-transform-test,relay-actor-change-test,relay-transforms,relay_resolvers_abstract_types_test,relay_resolvers_test,relay_test_operation_test,required_directive_test,skip_unused_variables_test,transform_connections_test,updatable_directive_test,updatable_fragment_spread_test,validate_connections_schema_test,validate_connections_test,validate_global_variable_names_test,validate_global_variables-test,validate_no_double_underscore_alias_test,validate_no_unselectable_selections_test,validate_static_args]

[package]
name = "relay-transforms"
Expand All @@ -24,6 +24,10 @@ path = "tests/disallow_non_node_id_fields_test.rs"
name = "graphql_disallow_reserved_aliases_test"
path = "tests/disallow_reserved_aliases_test.rs"

[[test]]
name = "graphql_disallowreadtime_features_in_mutations_test"
path = "tests/disallow_readtime_features_in_mutations_test.rs"

[[test]]
name = "graphql_flatten_test"
path = "tests/flatten_test.rs"
Expand Down
1 change: 1 addition & 0 deletions compiler/crates/relay-transforms/src/lib.rs
Expand Up @@ -179,6 +179,7 @@ pub use required_directive::RequiredMetadataDirective;
pub use required_directive::ACTION_ARGUMENT;
pub use required_directive::CHILDREN_CAN_BUBBLE_METADATA_KEY;
pub use required_directive::REQUIRED_DIRECTIVE_NAME;
pub use required_directive::THROW_ACTION;
pub use skip_client_directives::skip_client_directives;
pub use skip_client_extensions::skip_client_extensions;
pub use skip_null_arguments_transform::skip_null_arguments_transform;
Expand Down
Expand Up @@ -50,7 +50,7 @@ lazy_static! {
pub static ref ACTION_ARGUMENT: ArgumentName = ArgumentName("action".intern());
pub static ref CHILDREN_CAN_BUBBLE_METADATA_KEY: DirectiveName =
DirectiveName("__childrenCanBubbleNull".intern());
static ref THROW_ACTION: StringKey = "THROW".intern();
pub static ref THROW_ACTION: StringKey = "THROW".intern();
static ref LOG_ACTION: StringKey = "LOG".intern();
static ref NONE_ACTION: StringKey = "NONE".intern();
static ref INLINE_DIRECTIVE_NAME: DirectiveName = DirectiveName("inline".intern());
Expand Down
@@ -0,0 +1,153 @@
/*
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

use common::Diagnostic;
use common::DiagnosticsResult;
use common::FeatureFlag;
use common::NamedItem;
use docblock_shared::RELAY_RESOLVER_DIRECTIVE_NAME;
use graphql_ir::ConstantValue;
use graphql_ir::Field;
use graphql_ir::FragmentDefinition;
use graphql_ir::FragmentSpread;
use graphql_ir::LinkedField;
use graphql_ir::OperationDefinition;
use graphql_ir::Program;
use graphql_ir::ScalarField;
use graphql_ir::ValidationMessage;
use graphql_ir::Validator;
use schema::Schema;

use crate::ACTION_ARGUMENT;
use crate::REQUIRED_DIRECTIVE_NAME;
use crate::THROW_ACTION;

/// Some Relay features will cause a field to throw or suspend at read time.
/// These behaviors are incompatible with our mutation APIs.
/// This validator checks that no such features are used in mutations.
pub fn disallow_readtime_features_in_mutations(
program: &Program,
allow_resolvers_mutation_response: &FeatureFlag,
allow_required_in_mutation_response: &FeatureFlag,
enable_relay_resolver_mutations: bool,
) -> DiagnosticsResult<()> {
let mut validator = DisallowReadtimeFeaturesInMutations::new(
program,
allow_resolvers_mutation_response.clone(),
allow_required_in_mutation_response.clone(),
enable_relay_resolver_mutations,
);
validator.validate_program(program)
}

struct DisallowReadtimeFeaturesInMutations<'program> {
program: &'program Program,
allow_resolvers_mutation_response: FeatureFlag,
allow_required_in_mutation_response: FeatureFlag,
enable_relay_resolver_mutations: bool,
allow_resolvers_for_this_mutation: bool,
allow_required_for_this_mutation: bool,
}

impl<'program> DisallowReadtimeFeaturesInMutations<'program> {
fn new(
program: &'program Program,
allow_resolvers_mutation_response: FeatureFlag,
allow_required_in_mutation_response: FeatureFlag,
enable_relay_resolver_mutations: bool,
) -> Self {
Self {
program,
allow_resolvers_mutation_response,
allow_required_in_mutation_response,
enable_relay_resolver_mutations,
allow_resolvers_for_this_mutation: false,
allow_required_for_this_mutation: false,
}
}

fn validate_field(&self, field: &impl Field) -> DiagnosticsResult<()> {
if !self.allow_required_for_this_mutation {
if let Some(directive) = field.directives().named(*REQUIRED_DIRECTIVE_NAME) {
let action = directive
.arguments
.named(*ACTION_ARGUMENT)
.and_then(|arg| arg.value.item.get_constant());
if let Some(ConstantValue::Enum(action)) = action {
if *action == *THROW_ACTION {
return Err(vec![Diagnostic::error(
ValidationMessage::RequiredInMutation,
directive.name.location,
)]);
}
}
}
}
if !self.allow_resolvers_for_this_mutation
&& self
.program
.schema
.field(field.definition().item)
.directives
.named(*RELAY_RESOLVER_DIRECTIVE_NAME)
.is_some()
{
return Err(vec![Diagnostic::error(
ValidationMessage::ResolverInMutation,
field.alias_or_name_location(),
)]);
}

Ok(())
}
}

impl Validator for DisallowReadtimeFeaturesInMutations<'_> {
const NAME: &'static str = "disallow_readtime_features_in_mutations";
const VALIDATE_ARGUMENTS: bool = false;
const VALIDATE_DIRECTIVES: bool = false;

fn validate_operation(&mut self, operation: &OperationDefinition) -> DiagnosticsResult<()> {
if !operation.is_mutation() {
// No need to traverse into non-mutation operations
return Ok(());
}
self.allow_resolvers_for_this_mutation = self.enable_relay_resolver_mutations
|| self
.allow_resolvers_mutation_response
.is_enabled_for(operation.name.item.0);
self.allow_required_for_this_mutation = self
.allow_required_in_mutation_response
.is_enabled_for(operation.name.item.0);
let result = self.default_validate_operation(operation);

// Reset state
self.allow_resolvers_for_this_mutation = false;
self.allow_required_for_this_mutation = false;

result
}

fn validate_fragment(&mut self, _fragment: &FragmentDefinition) -> DiagnosticsResult<()> {
// We only care about mutations
Ok(())
}

fn validate_fragment_spread(&mut self, _spread: &FragmentSpread) -> DiagnosticsResult<()> {
// Values nested within fragment spreads are fine since they are not read as part of the
// mutation response.
Ok(())
}

fn validate_scalar_field(&mut self, field: &ScalarField) -> DiagnosticsResult<()> {
self.validate_field(field)
}
fn validate_linked_field(&mut self, field: &LinkedField) -> DiagnosticsResult<()> {
self.validate_field(field)?;
self.default_validate_linked_field(field)
}
}
2 changes: 2 additions & 0 deletions compiler/crates/relay-transforms/src/validations/mod.rs
Expand Up @@ -8,6 +8,7 @@
mod deprecated_fields;
mod disallow_circular_no_inline_fragments;
mod disallow_non_node_id_fields;
mod disallow_readtime_features_in_mutations;
mod disallow_reserved_aliases;
mod disallow_typename_on_root;
mod validate_connections;
Expand All @@ -29,6 +30,7 @@ pub use deprecated_fields::deprecated_fields;
pub use deprecated_fields::deprecated_fields_for_executable_definition;
pub use disallow_circular_no_inline_fragments::disallow_circular_no_inline_fragments;
pub use disallow_non_node_id_fields::disallow_non_node_id_fields;
pub use disallow_readtime_features_in_mutations::disallow_readtime_features_in_mutations;
pub use disallow_reserved_aliases::disallow_reserved_aliases;
pub use disallow_typename_on_root::disallow_typename_on_root;
pub use validate_connections::validate_connections;
Expand Down
@@ -0,0 +1,14 @@
==================================== INPUT ====================================
mutation MyMutation {
setName(name: "Alice") {
...myFragment
}
}

fragment myFragment on User {
name @required(action: THROW)
}

# %extensions%
==================================== OUTPUT ===================================
OK
@@ -0,0 +1,11 @@
mutation MyMutation {
setName(name: "Alice") {
...myFragment
}
}

fragment myFragment on User {
name @required(action: THROW)
}

# %extensions%
@@ -0,0 +1,21 @@
==================================== INPUT ====================================
# expected-to-throw
mutation MyMutation {
some_resolver
}

# %extensions%
type SomeType {
id: ID
}
extend type Mutation {
some_resolver: SomeType @relay_resolver
}
==================================== ERROR ====================================
✖︎ Expected selections on field `some_resolver` of type `Mutation`

mutation_with_linked_resolver.invalid.graphql:3:3
2 │ mutation MyMutation {
3 │ some_resolver
│ ^^^^^^^^^^^^^
4 │ }
@@ -0,0 +1,12 @@
# expected-to-throw
mutation MyMutation {
some_resolver
}

# %extensions%
type SomeType {
id: ID
}
extend type Mutation {
some_resolver: SomeType @relay_resolver
}
@@ -0,0 +1,17 @@
==================================== INPUT ====================================
# expected-to-throw
mutation MyMutation {
setName(name: "Alice") {
name @required(action: THROW)
}
}

# %extensions%
==================================== ERROR ====================================
✖︎ Unexpected `@required(action: THROW)` directive in mutation response. The use of `@required(action: THROW)` is not supported in mutations.

mutation_with_required_field.invalid.graphql:4:10
3 │ setName(name: "Alice") {
4 │ name @required(action: THROW)
│ ^^^^^^^^^
5 │ }
@@ -0,0 +1,8 @@
# expected-to-throw
mutation MyMutation {
setName(name: "Alice") {
name @required(action: THROW)
}
}

# %extensions%

0 comments on commit ada971e

Please sign in to comment.