Skip to content

Commit

Permalink
add validations for custom scalar arguments
Browse files Browse the repository at this point in the history 
Summary:
NOTE: the majority of this diff is test cases, please review carefully to ensure that all relevant cases are captured

A future diff will enable this for projects in www.

Add a flag `enable_strict_custom_scalars` which will perform more validations when custom scalar types are used. The only validation added here is to check that the user doesn't try to pass literal values (strings, ints, etc) in positions where a custom scalar type is expected, as this can break type safety since Relay can't know whether those literals conform to the underlying custom type.

This also fixes some existing issues where you could pass an object or list literal to a place expecting a custom scalar, despite it not being a scalar.

Keeping this behind a flag for backwards compatibility, as some users depend on this (e.g. see #3730).

Reviewed By: alunyov

Differential Revision: D51219598

fbshipit-source-id: 71d9ee85e4428c6b3e958929114ac9e06e8967e3
  • Loading branch information
@facebook-github-bot
Alex Danoff authored and facebook-github-bot committed Nov 28, 2023
1 parent adabbb0 commit b683e8c
Show file tree
Hide file tree
Showing 46 changed files with 1,948 additions and 118 deletions.
4 changes: 4 additions & 0 deletions compiler/crates/common/src/feature_flags.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,10 @@ pub struct FeatureFlags {
/// Allow relay resolvers to extend the Mutation type
#[serde(default)]
pub enable_relay_resolver_mutations: bool,

/// Perform strict validations when custom scalar types are used
#[serde(default)]
pub enable_strict_custom_scalars: bool,
}

#[derive(Debug, Deserialize, Clone, Serialize)]
Expand Down
283 changes: 188 additions & 95 deletions compiler/crates/graphql-ir/src/build.rs
View file

Large diffs are not rendered by default.

17 changes: 17 additions & 0 deletions compiler/crates/graphql-ir/src/errors.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use std::fmt::Display;
use common::ArgumentName;
use common::DiagnosticDisplay;
use common::DirectiveName;
use common::ScalarName;
use common::WithDiagnosticData;
use graphql_syntax::OperationKind;
use intern::string_key::StringKey;
Expand Down Expand Up @@ -499,6 +500,22 @@ pub enum ValidationMessage {

#[error("No fields can have an alias that start with two underscores.")]
NoDoubleUnderscoreAlias,

#[error(
"Unexpected scalar literal `{literal_value}` provided in a position expecting custom scalar type `{scalar_type_name}`. This value should come from a variable."
)]
UnexpectedCustomScalarLiteral {
literal_value: String,
scalar_type_name: ScalarName,
},

#[error(
"Unexpected {literal_kind} literal provided in a position expecting custom scalar type `{scalar_type_name}`."
)]
UnexpectedNonScalarLiteralForCustomScalar {
literal_kind: String,
scalar_type_name: ScalarName,
},
}

#[derive(Clone, Debug, Error, Eq, PartialEq, Ord, PartialOrd, Hash)]
Expand Down
42 changes: 42 additions & 0 deletions ...phql-ir/tests/parse_with_extensions/fixtures/custom_scalar_directive_arg.invalid.expected
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
==================================== INPUT ====================================
# expected-to-throw
# relay:no_custom_scalar_literals

query CustomScalarLiteralArgQuery {
extension_field @customScalarDirective(arg: "foo") {
__typename
}
extension_scalar_field @customScalarDirective(arg: "bar")
}

# %extensions%

scalar CustomScalarType

directive @customScalarDirective(arg: CustomScalarType!) on FIELD

type Obj {
some_key: Int!
}

extend type Query {
extension_field: Obj
extension_scalar_field: Int
}
==================================== ERROR ====================================
✖︎ Unexpected scalar literal `"foo"` provided in a position expecting custom scalar type `CustomScalarType`. This value should come from a variable.

custom_scalar_directive_arg.invalid.graphql:5:49
4 │ query CustomScalarLiteralArgQuery {
5 │ extension_field @customScalarDirective(arg: "foo") {
│ ^^^^^
6 │ __typename


✖︎ Unexpected scalar literal `"bar"` provided in a position expecting custom scalar type `CustomScalarType`. This value should come from a variable.

custom_scalar_directive_arg.invalid.graphql:8:56
7 │ }
8 │ extension_scalar_field @customScalarDirective(arg: "bar")
│ ^^^^^
9 │ }
24 changes: 24 additions & 0 deletions ...aphql-ir/tests/parse_with_extensions/fixtures/custom_scalar_directive_arg.invalid.graphql
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# expected-to-throw
# relay:no_custom_scalar_literals

query CustomScalarLiteralArgQuery {
extension_field @customScalarDirective(arg: "foo") {
__typename
}
extension_scalar_field @customScalarDirective(arg: "bar")
}

# %extensions%

scalar CustomScalarType

directive @customScalarDirective(arg: CustomScalarType!) on FIELD

type Obj {
some_key: Int!
}

extend type Query {
extension_field: Obj
extension_scalar_field: Int
}
164 changes: 164 additions & 0 deletions ...hql-ir/tests/parse_with_extensions/fixtures/custom_scalar_directive_arg_variable.expected
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,164 @@
==================================== INPUT ====================================
# relay:no_custom_scalar_literals

query CustomScalarLiteralArgQuery($var: CustomScalarType!) {
extension_field @customScalarDirective(arg: $var) {
__typename
}
extension_scalar_field @customScalarDirective(arg: $var)
}

# %extensions%

scalar CustomScalarType

directive @customScalarDirective(arg: CustomScalarType!) on FIELD

type Obj {
some_key: Int!
}

extend type Query {
extension_field: Obj
extension_scalar_field: Int
}
==================================== OUTPUT ===================================
[
Operation(
OperationDefinition {
kind: Query,
name: WithLocation {
location: custom_scalar_directive_arg_variable.graphql:41:68,
item: OperationDefinitionName(
"CustomScalarLiteralArgQuery",
),
},
type_: Object(0),
variable_definitions: [
VariableDefinition {
name: WithLocation {
location: custom_scalar_directive_arg_variable.graphql:69:73,
item: VariableName(
"var",
),
},
type_: NonNull(
Named(
Scalar(8),
),
),
default_value: None,
directives: [],
},
],
directives: [],
selections: [
LinkedField {
alias: None,
definition: WithLocation {
location: custom_scalar_directive_arg_variable.graphql:100:115,
item: FieldID(517),
},
arguments: [],
directives: [
Directive {
name: WithLocation {
location: custom_scalar_directive_arg_variable.graphql:116:138,
item: DirectiveName(
"customScalarDirective",
),
},
arguments: [
Argument {
name: WithLocation {
location: custom_scalar_directive_arg_variable.graphql:139:142,
item: ArgumentName(
"arg",
),
},
value: WithLocation {
location: custom_scalar_directive_arg_variable.graphql:144:148,
item: Variable(
Variable {
name: WithLocation {
location: custom_scalar_directive_arg_variable.graphql:144:148,
item: VariableName(
"var",
),
},
type_: NonNull(
Named(
Scalar(8),
),
),
},
),
},
},
],
data: None,
},
],
selections: [
ScalarField {
alias: None,
definition: WithLocation {
location: custom_scalar_directive_arg_variable.graphql:160:170,
item: FieldID(519),
},
arguments: [],
directives: [],
},
],
},
ScalarField {
alias: None,
definition: WithLocation {
location: custom_scalar_directive_arg_variable.graphql:181:203,
item: FieldID(518),
},
arguments: [],
directives: [
Directive {
name: WithLocation {
location: custom_scalar_directive_arg_variable.graphql:204:226,
item: DirectiveName(
"customScalarDirective",
),
},
arguments: [
Argument {
name: WithLocation {
location: custom_scalar_directive_arg_variable.graphql:227:230,
item: ArgumentName(
"arg",
),
},
value: WithLocation {
location: custom_scalar_directive_arg_variable.graphql:232:236,
item: Variable(
Variable {
name: WithLocation {
location: custom_scalar_directive_arg_variable.graphql:232:236,
item: VariableName(
"var",
),
},
type_: NonNull(
Named(
Scalar(8),
),
),
},
),
},
},
],
data: None,
},
],
},
],
},
),
]
23 changes: 23 additions & 0 deletions ...phql-ir/tests/parse_with_extensions/fixtures/custom_scalar_directive_arg_variable.graphql
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# relay:no_custom_scalar_literals

query CustomScalarLiteralArgQuery($var: CustomScalarType!) {
extension_field @customScalarDirective(arg: $var) {
__typename
}
extension_scalar_field @customScalarDirective(arg: $var)
}

# %extensions%

scalar CustomScalarType

directive @customScalarDirective(arg: CustomScalarType!) on FIELD

type Obj {
some_key: Int!
}

extend type Query {
extension_field: Obj
extension_scalar_field: Int
}
40 changes: 40 additions & 0 deletions ...l-ir/tests/parse_with_extensions/fixtures/custom_scalar_list_literal_arg.invalid.expected
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
==================================== INPUT ====================================
# expected-to-throw
# relay:no_custom_scalar_literals

query CustomScalarLiteralArgQuery {
extension_field(custom_scalar_arg: ["1234", "5678"]) {
__typename
}
extension_scalar_field(custom_scalar_arg: [])
}

# %extensions%

scalar CustomScalarType

type Obj {
some_key: Int!
}

extend type Query {
extension_field(custom_scalar_arg: CustomScalarType!): Obj
extension_scalar_field(custom_scalar_arg: CustomScalarType!): Int
}
==================================== ERROR ====================================
✖︎ Unexpected list literal provided in a position expecting custom scalar type `CustomScalarType`.

custom_scalar_list_literal_arg.invalid.graphql:5:40
4 │ query CustomScalarLiteralArgQuery {
5 │ extension_field(custom_scalar_arg: ["1234", "5678"]) {
│ ^^^^^^^^^^^^^^^^
6 │ __typename


✖︎ Unexpected list literal provided in a position expecting custom scalar type `CustomScalarType`.

custom_scalar_list_literal_arg.invalid.graphql:8:47
7 │ }
8 │ extension_scalar_field(custom_scalar_arg: [])
│ ^^
9 │ }
22 changes: 22 additions & 0 deletions ...ql-ir/tests/parse_with_extensions/fixtures/custom_scalar_list_literal_arg.invalid.graphql
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# expected-to-throw
# relay:no_custom_scalar_literals

query CustomScalarLiteralArgQuery {
extension_field(custom_scalar_arg: ["1234", "5678"]) {
__typename
}
extension_scalar_field(custom_scalar_arg: [])
}

# %extensions%

scalar CustomScalarType

type Obj {
some_key: Int!
}

extend type Query {
extension_field(custom_scalar_arg: CustomScalarType!): Obj
extension_scalar_field(custom_scalar_arg: CustomScalarType!): Int
}
Loading

0 comments on commit b683e8c

Please sign in to comment.