Skip to content

Commit fb7ffff

Browse files
jungwoo3490meta-codesync[bot]
authored andcommitted
feat: Warn on deprecated enum values used as arguments (#5277)
Summary: GraphQL allows `deprecated` on enum values, but Relay wasn't warning when a deprecated enum value was used as a field or directive argument. This PR adds that validation alongside the existing deprecated field/argument checks. ```graphql enum Status { ACTIVE LEGACY deprecated(reason: "Use ACTIVE instead.") } # Before: no warning # After: The enum value `LEGACY` of type `Status` is deprecated. Deprecation reason: "Use ACTIVE instead." fragment Foo on Query { user(status: LEGACY) } ``` The warning message wording follows the existing patterns in the codebase. Open to suggestions if there's a better way to phrase it! > [!NOTE] > Variable values (`$status`) and deeply nested enum values inside constant objects/arrays are not yet covered. Left a TODO comment in the code. Pull Request resolved: #5277 Reviewed By: captbaritone Differential Revision: D106030387 Pulled By: evanyeung fbshipit-source-id: 16617aa66259c233780624767dfe457f93c9c9d3
1 parent 71b4e33 commit fb7ffff

8 files changed

Lines changed: 172 additions & 34 deletions

File tree

compiler/crates/graphql-ir/src/errors.rs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -498,6 +498,17 @@ pub enum ValidationMessage {
498498
deprecation_reason: Option<StringKey>,
499499
},
500500

501+
#[error("The enum value `{enum_value}` of type `{enum_name}` is deprecated.{}",
502+
match deprecation_reason {
503+
Some(reason) => format!(" Deprecation reason: \"{reason}\""),
504+
None => "".to_string()
505+
})]
506+
DeprecatedEnumValue {
507+
enum_value: StringKey,
508+
enum_name: StringKey,
509+
deprecation_reason: Option<StringKey>,
510+
},
511+
501512
#[error("Missing required {}: `{}`",
502513
if missing_arg_names.len() > 1 { "arguments" } else { "argument" },
503514
missing_arg_names

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

Lines changed: 71 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ use common::DiagnosticTag;
1212
use common::DiagnosticsResult;
1313
use common::WithLocation;
1414
use graphql_ir::Argument;
15+
use graphql_ir::ConstantValue;
1516
use graphql_ir::Directive;
1617
use graphql_ir::ExecutableDefinition;
1718
use graphql_ir::LinkedField;
@@ -24,6 +25,7 @@ use graphql_ir::Value;
2425
use schema::FieldID;
2526
use schema::SDLSchema;
2627
use schema::Schema;
28+
use schema::Type;
2729

2830
use crate::fragment_alias_directive::FRAGMENT_DANGEROUSLY_UNALIAS_DIRECTIVE_NAME;
2931

@@ -81,22 +83,41 @@ impl<'a> DeprecatedFields<'a> {
8183
}
8284

8385
for arg in arguments {
84-
if let Some(arg_definition) = field_definition.arguments.named(arg.name.item)
85-
&& let Some(directive) = arg_definition.deprecated()
86-
{
87-
let parent_type = field_definition.parent_type.unwrap();
88-
let parent_name = schema.get_type_name(parent_type);
89-
90-
self.warnings.push(Diagnostic::hint(
91-
ValidationMessage::DeprecatedFieldArgument {
92-
argument_name: arg.name.item,
93-
field_name: field_definition.name.item,
94-
parent_name,
95-
deprecation_reason: directive.reason,
96-
},
97-
arg.name.location,
98-
vec![DiagnosticTag::DEPRECATED],
99-
));
86+
if let Some(arg_definition) = field_definition.arguments.named(arg.name.item) {
87+
if let Some(directive) = arg_definition.deprecated() {
88+
let parent_type = field_definition.parent_type.unwrap();
89+
let parent_name = schema.get_type_name(parent_type);
90+
91+
self.warnings.push(Diagnostic::hint(
92+
ValidationMessage::DeprecatedFieldArgument {
93+
argument_name: arg.name.item,
94+
field_name: field_definition.name.item,
95+
parent_name,
96+
deprecation_reason: directive.reason,
97+
},
98+
arg.name.location,
99+
vec![DiagnosticTag::DEPRECATED],
100+
));
101+
}
102+
103+
if let Type::Enum(enum_id) = arg_definition.type_.inner() {
104+
let enum_def = schema.enum_(enum_id);
105+
if let Value::Constant(ConstantValue::Enum(value_name)) = &arg.value.item
106+
&& let Some(enum_value) =
107+
enum_def.values.iter().find(|v| v.value == *value_name)
108+
&& let Some(deprecation) = enum_value.deprecated()
109+
{
110+
self.warnings.push(Diagnostic::hint(
111+
ValidationMessage::DeprecatedEnumValue {
112+
enum_value: *value_name,
113+
enum_name: enum_def.name.item.0,
114+
deprecation_reason: deprecation.reason,
115+
},
116+
arg.value.location,
117+
vec![DiagnosticTag::DEPRECATED],
118+
));
119+
}
120+
}
100121
}
101122
}
102123
}
@@ -121,11 +142,9 @@ impl Validator for DeprecatedFields<'_> {
121142
}
122143

123144
fn validate_value(&mut self, value: &Value) -> DiagnosticsResult<()> {
124-
// TODO: `@deprecated` is allowed on Enum values, so technically we
125-
// should also be validating when someone uses a deprecated enum value
126-
// as an argument, but that will require some additional methods on our
127-
// Schema, and potentially some additional traversal in our validation
128-
// trait to traverse into potentially deep constant objects/arrays.
145+
// TODO: Deprecated enum values inside deeply nested constant objects/arrays
146+
// are not yet validated. Top-level enum values in field and directive arguments
147+
// are handled in validate_field and validate_directive respectively.
129148
self.default_validate_value(value)
130149
}
131150

@@ -143,18 +162,37 @@ impl Validator for DeprecatedFields<'_> {
143162
));
144163
}
145164
for arg in &directive.arguments {
146-
if let Some(arg_definition) = directive_definition.arguments.named(arg.name.item)
147-
&& let Some(deprecation) = arg_definition.deprecated()
148-
{
149-
self.warnings.push(Diagnostic::hint(
150-
ValidationMessage::DeprecatedDirectiveArgument {
151-
argument_name: arg.name.item,
152-
directive_name: directive.name.item,
153-
deprecation_reason: deprecation.reason,
154-
},
155-
arg.name.location,
156-
vec![DiagnosticTag::DEPRECATED],
157-
));
165+
if let Some(arg_definition) = directive_definition.arguments.named(arg.name.item) {
166+
if let Some(deprecation) = arg_definition.deprecated() {
167+
self.warnings.push(Diagnostic::hint(
168+
ValidationMessage::DeprecatedDirectiveArgument {
169+
argument_name: arg.name.item,
170+
directive_name: directive.name.item,
171+
deprecation_reason: deprecation.reason,
172+
},
173+
arg.name.location,
174+
vec![DiagnosticTag::DEPRECATED],
175+
));
176+
}
177+
178+
if let Type::Enum(enum_id) = arg_definition.type_.inner() {
179+
let enum_def = self.schema.enum_(enum_id);
180+
if let Value::Constant(ConstantValue::Enum(value_name)) = &arg.value.item
181+
&& let Some(enum_value) =
182+
enum_def.values.iter().find(|v| v.value == *value_name)
183+
&& let Some(deprecation) = enum_value.deprecated()
184+
{
185+
self.warnings.push(Diagnostic::hint(
186+
ValidationMessage::DeprecatedEnumValue {
187+
enum_value: *value_name,
188+
enum_name: enum_def.name.item.0,
189+
deprecation_reason: deprecation.reason,
190+
},
191+
arg.value.location,
192+
vec![DiagnosticTag::DEPRECATED],
193+
));
194+
}
195+
}
158196
}
159197
}
160198
}
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
==================================== INPUT ====================================
2+
fragment Foo on MyNewType {
3+
some_field(status: LEGACY)
4+
}
5+
%extensions%
6+
enum Status {
7+
ACTIVE
8+
LEGACY @deprecated
9+
}
10+
type MyNewType {
11+
some_field(status: Status): String
12+
}
13+
==================================== OUTPUT ===================================
14+
ℹ The enum value `LEGACY` of type `Status` is deprecated.
15+
16+
deprecated_enum_value.graphql:2:22
17+
1 │ fragment Foo on MyNewType {
18+
2 │ some_field(status: LEGACY)
19+
│ ^^^^^^
20+
3 │ }
Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
fragment Foo on MyNewType {
2+
some_field(status: LEGACY)
3+
}
4+
%extensions%
5+
enum Status {
6+
ACTIVE
7+
LEGACY @deprecated
8+
}
9+
type MyNewType {
10+
some_field(status: Status): String
11+
}
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
==================================== INPUT ====================================
2+
fragment Foo on MyNewType {
3+
some_field(status: LEGACY)
4+
}
5+
%extensions%
6+
enum Status {
7+
ACTIVE
8+
LEGACY @deprecated(reason: "Use ACTIVE instead.")
9+
}
10+
type MyNewType {
11+
some_field(status: Status): String
12+
}
13+
==================================== OUTPUT ===================================
14+
ℹ The enum value `LEGACY` of type `Status` is deprecated. Deprecation reason: "Use ACTIVE instead."
15+
16+
deprecated_enum_value_with_reason.graphql:2:22
17+
1 │ fragment Foo on MyNewType {
18+
2 │ some_field(status: LEGACY)
19+
│ ^^^^^^
20+
3 │ }
Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
fragment Foo on MyNewType {
2+
some_field(status: LEGACY)
3+
}
4+
%extensions%
5+
enum Status {
6+
ACTIVE
7+
LEGACY @deprecated(reason: "Use ACTIVE instead.")
8+
}
9+
type MyNewType {
10+
some_field(status: Status): String
11+
}

compiler/crates/relay-transforms/tests/validate_deprecated_fields_test.rs

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
* This source code is licensed under the MIT license found in the
55
* LICENSE file in the root directory of this source tree.
66
*
7-
* @generated SignedSource<<523e6262355cff63df80d9ea70e0d2ef>>
7+
* @generated SignedSource<<617806504dd54b22a1692ec28a9db160>>
88
*/
99

1010
mod validate_deprecated_fields;
@@ -26,6 +26,20 @@ async fn deprecated_directive_arg_with_reason() {
2626
test_fixture(transform_fixture, file!(), "deprecated_directive_arg_with_reason.graphql", "validate_deprecated_fields/fixtures/deprecated_directive_arg_with_reason.expected", input, expected).await;
2727
}
2828

29+
#[tokio::test]
30+
async fn deprecated_enum_value() {
31+
let input = include_str!("validate_deprecated_fields/fixtures/deprecated_enum_value.graphql");
32+
let expected = include_str!("validate_deprecated_fields/fixtures/deprecated_enum_value.expected");
33+
test_fixture(transform_fixture, file!(), "deprecated_enum_value.graphql", "validate_deprecated_fields/fixtures/deprecated_enum_value.expected", input, expected).await;
34+
}
35+
36+
#[tokio::test]
37+
async fn deprecated_enum_value_with_reason() {
38+
let input = include_str!("validate_deprecated_fields/fixtures/deprecated_enum_value_with_reason.graphql");
39+
let expected = include_str!("validate_deprecated_fields/fixtures/deprecated_enum_value_with_reason.expected");
40+
test_fixture(transform_fixture, file!(), "deprecated_enum_value_with_reason.graphql", "validate_deprecated_fields/fixtures/deprecated_enum_value_with_reason.expected", input, expected).await;
41+
}
42+
2943
#[tokio::test]
3044
async fn deprecated_field_arg() {
3145
let input = include_str!("validate_deprecated_fields/fixtures/deprecated_field_arg.graphql");

compiler/crates/schema/src/definitions.rs

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -695,6 +695,19 @@ pub struct EnumValue {
695695
pub description: Option<StringKey>,
696696
}
697697

698+
impl EnumValue {
699+
pub fn deprecated(&self) -> Option<Deprecation> {
700+
self.directives
701+
.named(*DIRECTIVE_DEPRECATED)
702+
.map(|directive| Deprecation {
703+
reason: directive
704+
.arguments
705+
.named(*ARGUMENT_REASON)
706+
.and_then(|reason| reason.value.get_string_literal()),
707+
})
708+
}
709+
}
710+
698711
#[derive(
699712
Clone,
700713
Eq,

0 commit comments

Comments
 (0)