8000 Don't require @alias on plural fragments spread into plural selections · facebook/relay@d3eb42f · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Commit d3eb42f

Browse files
captbaritonefacebook-github-bot
captbaritone
authored and
facebook-github-bot
committed
Don't require @alias on plural fragments spread into plural selections
Reviewed By: gordyf Differential Revision: D64628783 fbshipit-source-id: 687e2a08d88fb8df6742c09ca6084dca309b1424
1 parent da8a092 commit d3eb42f

8 files changed

+189
-23
lines changed

‎compiler/crates/relay-transforms/src/fragment_alias_directive.rs

+57-22
Original file line numberDiff line numberDiff line change
@@ -118,11 +118,17 @@ impl Transformer for AliasedInlineFragmentRemovalTransform {
118118
}
119119
}
120120

121+
#[derive(Clone, Copy)]
122+
struct ParentType {
123+
type_: Type,
124+
is_plural: bool,
125+
}
126+
121127
struct FragmentAliasTransform<'program> {
122128
program: &'program Program,
123129
is_enforced: &'program FeatureFlag,
124130
document_name: Option<StringKey>,
125-
parent_type: Option<Type>,
131+
parent_type: Option<ParentType>,
126132
parent_name: Option<StringKey>,
127133
within_inline_fragment_type_condition: bool,
128134
maybe_condition: Option<Condition>,
@@ -155,7 +161,7 @@ impl<'program> FragmentAliasTransform<'program> {
155161

156162
self.program
157163
.schema
158-
.is_named_type_subtype_of(parent_type, type_condition)
164+
.is_named_type_subtype_of(parent_type.type_, type_condition)
159165
}
160166
None => true,
161167
}
@@ -180,6 +186,20 @@ impl<'program> FragmentAliasTransform<'program> {
180186
// this error as a migration strategy.
181187
return;
182188
}
189+
let fragment = self
190+
.program
191+
.fragment(spread.fragment.item)
192+
.expect("I believe we have already validated that all fragments exist");
193+
194+
let fragment_is_plural =
195+
RelayDirective::find(&fragment.directives).map_or(false, |directive| directive.plural);
196+
197+
if fragment_is_plural && self.parent_type.expect("expect parent type").is_plural {
198+
// Plural fragments handle their own nullability when read. However,
199+
// they can calso be used in a singular selection. In that case,
200+
// they should be aliased.
201+
return;
202+
}
183203
if spread
184204
.directives
185205
.named(MATCH_CONSTANTS.module_directive_name)
@@ -211,10 +231,10 @@ impl<'program> FragmentAliasTransform<'program> {
211231
if !self
212232
.program
213233
.schema
214-
.is_named_type_subtype_of(parent_type, type_condition)
234+
.is_named_type_subtype_of(parent_type.type_, type_condition)
215235
{
216236
let fragment_type_name = self.program.schema.get_type_name(type_condition);
217-
let selection_type_name = self.program.schema.get_type_name(parent_type);
237+
let selection_type_name = self.program.schema.get_type_name(parent_type.type_);
218238
let diagnostic = if self.within_inline_fragment_type_condition {
219239
ValidationMessageWithData::ExpectedAliasOnNonSubtypeSpreadWithinTypedInlineFragment {
220240
fragment_name: spread.fragment.item,
@@ -248,7 +268,10 @@ impl Transformer for FragmentAliasTransform<'_> {
248268
) -> Transformed<FragmentDefinition> {
249269
self.document_name = Some(fragment.name.item.0);
250270
self.parent_name = Some(fragment.name.item.0);
251-
self.parent_type = Some(fragment.type_condition);
271+
self.parent_type = Some(ParentType {
272+
type_: fragment.type_condition,
273+
is_plural: false,
274+
});
252275
let transformed = self.default_transform_fragment(fragment);
253276
self.parent_type = None;
254277
self.parent_name = None;
@@ -261,7 +284,10 @@ impl Transformer for FragmentAliasTransform<'_> {
261284
operation: &OperationDefinition,
262285
) -> Transformed<OperationDefinition> {
263286
self.document_name = Some(operation.name.item.0);
264-
self.parent_type = Some(operation.type_);
287+
self.parent_type = Some(ParentType {
288+
type_: operation.type_,
289+
is_plural: false,
290+
});
265291
self.parent_name = Some(operation.name.item.0);
266292
let transformed = self.default_transform_operation(operation);
267293
self.parent_type = None;
@@ -312,16 +338,24 @@ impl Transformer for FragmentAliasTransform<'_> {
312338
let will_always_match = self.will_always_match(fragment.type_condition);
313339

314340
if let Some(type_condition) = fragment.type_condition {
315-
self.parent_type = Some(type_condition);
341+
let parent_type = self
342+
.parent_type
343+
.expect("Selection should be within a parent type.");
344+
self.parent_type = Some(ParentType {
345+
type_: type_condition,
346+
is_plural: parent_type.is_plural,
347+
});
316348
}
317349

350+
let parent_type = self
351+
.parent_type
352+
.expect("Selection should be within a parent type.");
353+
318354
let alias_metadata = FragmentAliasMetadata {
319355
alias,
320356
type_condition: fragment.type_condition,
321357
non_nullable: will_always_match,
322-
selection_type: self
323-
.parent_type
324-
.expect("Selection should be within a parent type."),
358+
selection_type: parent_type.type_,
325359
wraps_spread: false,
326360
};
327361

@@ -379,10 +413,14 @@ impl Transformer for FragmentAliasTransform<'_> {
379413

380414
match spread.alias() {
381415
Ok(Some(alias)) => {
382-
let is_plural = RelayDirective::find(&fragment.directives)
416+
let fragment_is_plural = RelayDirective::find(&fragment.directives)
383417
.map_or(false, |directive| directive.plural);
384418

385-
if is_plural {
419+
let parent_type = self
420+
.parent_type
421+
.expect("Selection should be within a parent type.");
422+
423+
if fragment_is_plural && parent_type.is_plural {
386424
self.errors.push(Diagnostic::error(
387425
ValidationMessage::PluralFragmentAliasNotSupported,
388426
alias.location,
@@ -393,9 +431,7 @@ impl Transformer for FragmentAliasTransform<'_> {
393431
alias,
394432
type_condition,
395433
non_nullable: self.will_always_match(type_condition),
396-
selection_type: self
397-
.parent_type
398-
.expect("Selection should be within a parent type."),
434+
selection_type: parent_type.type_,
399435
wraps_spread: true,
400436
};
401437

@@ -422,13 +458,12 @@ impl Transformer for FragmentAliasTransform<'_> {
422458
// does not apply to the selections within the linked field.
423459
let parent_condition = self.maybe_condition.take();
424460

425-
self.parent_type = Some(
426-
self.program
427-
.schema
428-
.field(field.definition.item)
429-
.type_
430-
.inner(),
431-
);
461+
let field_definition = self.program.schema.field(field.definition.item);
462+
463+
self.parent_type = Some(ParentType {
464+
type_: field_definition.type_.inner(),
465+
is_plural: field_definition.type_.is_list(),
466+
});
432467

433468
let transformed = self.default_transform_linked_field(field);
434469

‎compiler/crates/relay-transforms/tests/fragment_alias_directive/fixtures/alias_on_spread_of_plural_fragment_into_singular_selection.expected

+35
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
==================================== INPUT ====================================
2+
fragment RelayReaderNamedFragmentsTest_user on User @relay(plural: true) {
3+
name
4+
}
5+
6+
query RelayReaderNamedFragmentsTest2Query {
7+
node {
8+
...RelayReaderNamedFragmentsTest_user @alias
9+
}
10+
}
11+
==================================== OUTPUT ===================================
12+
query RelayReaderNamedFragmentsTest2Query {
13+
node {
14+
... on User @__FragmentAliasMetadata
15+
# FragmentAliasMetadata {
16+
# alias: WithLocation {
17+
# location: alias_on_spread_of_plural_fragment_into_singular_selection.graphql:180:186,
18+
# item: "RelayReaderNamedFragmentsTest_user",
19+
# },
20+
# type_condition: Some(
21+
# Object(70),
22+
# ),
23+
# non_nullable: false,
24+
# selection_type: Interface(5),
25+
# wraps_spread: true,
26+
# }
27+
{
28+
...RelayReaderNamedFragmentsTest_user @alias
29+
}
30+
}
31+
}
32+
33+
fragment RelayReaderNamedFragmentsTest_user on User @relay(plural: true) {
34+
name
35+
}

‎compiler/crates/relay-transforms/tests/fragment_alias_directive/fixtures/alias_on_spread_of_plural_fragment_into_singular_selection.graphql

+9
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
fragment RelayReaderNamedFragmentsTest_user on User @relay(plural: true) {
2+
name
3+
}
4+
5+
query RelayReaderNamedFragmentsTest2Query {
6+
node {
7+
...RelayReaderNamedFragmentsTest_user @alias
8+
}
9+
}

‎compiler/crates/relay-transforms/tests/fragment_alias_directive/fixtures/spread_of_plural_fragment_into_singular_selection_without_alias.invalid.expected

+20
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
==================================== INPUT ====================================
2+
# expected-to-throw
3+
fragment RelayReaderNamedFragmentsTest_user on User @relay(plural: true) {
4+
name
5+
}
6+
7+
query RelayReaderNamedFragmentsTest2Query {
8+
node {
9+
# This might not match, so we need @alias
10+
...RelayReaderNamedFragmentsTest_user
11+
}
12+
}
13+
==================================== ERROR ====================================
14+
✖︎ Expected `@alias` directive. `RelayReaderNamedFragmentsTest_user` is defined on `User` which might not match this selection type of `Node`. Add `@alias` to this spread to expose the fragment reference as a nullable property.
15+
16+
spread_of_plural_fragment_into_singular_selection_without_alias.invalid.graphql:9:8
17+
8 │ # This might not match, so we need @alias
18+
9 │ ...RelayReaderNamedFragmentsTest_user
19+
│ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
20+
10 │ }

‎compiler/crates/relay-transforms/tests/fragment_alias_directive/fixtures/spread_of_plural_fragment_into_singular_selection_without_alias.invalid.graphql

+11
10000
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
# expected-to-throw
2+
fragment RelayReaderNamedFragmentsTest_user on User @relay(plural: true) {
3+
name
4+
}
5+
6+
query RelayReaderNamedFragmentsTest2Query {
7+
node {
8+
# This might not match, so we need @alias
9+
...RelayReaderNamedFragmentsTest_user
10+
}
11+
}

‎compiler/crates/relay-transforms/tests/fragment_alias_directive/fixtures/unaliased_spread_of_plural_fragment_that_might_not_match.expected

+23
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
==================================== INPUT ====================================
2+
fragment RelayReaderNamedFragmentsTest_user on User @relay(plural: true) {
3+
name
4+
}
5+
6+
query RelayReaderNamedFragmentsTest2Query {
7+
nodes {
8+
# This might not match due to type conditions, but that optionality will be
9+
# handled when the plural fragment is read. The array read out from this plural
10+
# fragment key may be sparse.
11+
...RelayReaderNamedFragmentsTest_user
12+
}
13+
}
14+
==================================== OUTPUT ===================================
15+
query RelayReaderNamedFragmentsTest2Query {
16+
nodes {
17+
...RelayReaderNamedFragmentsTest_user
18+
}
19+
}
20+
21+
fragment RelayReaderNamedFragmentsTest_user on User @relay(plural: true) {
22+
name
23+
}

‎compiler/crates/relay-transforms/tests/fragment_alias_directive/fixtures/unaliased_spread_of_plural_fragment_that_might_not_match.graphql

+12
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
fragment RelayReaderNamedFragmentsTest_user on User @relay(plural: true) {
2+
name
3+
}
4+
5+
query RelayReaderNamedFragmentsTest2Query {
6+
nodes {
7+
# This might not match due to type conditions, but that optionality will be
8+
# handled when the plural fragment is read. The array read out from this plural
9+
# fragment key may be sparse.
10+
...RelayReaderNamedFragmentsTest_user
11+
}
12+
}

‎compiler/crates/relay-transforms/tests/fragment_alias_directive_test.rs

+22-1
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<<b3ad5f451c81e311e6efe57ead61c409>>
7+
* @generated SignedSource<<999ec9e0fabce04175790ccc86aa1f29>>
88
*/
99

1010
mod fragment_alias_directive;
@@ -47,6 +47,13 @@ async fn alias_on_named_fragment() {
4747
test_fixture(transform_fixture, file!(), "alias_on_named_fragment.graphql", "fragment_alias_directive/fixtures/alias_on_named_fragment.expected", input, expected).await;
4848
}
4949

50+
#[tokio::test]
51+
async fn alias_on_spread_of_plural_fragment_into_singular_selection() {
52+
let input = include_str!("fragment_alias_directive/fixtures/alias_on_spread_of_plural_fragment_into_singular_selection.graphql");
53+
let expected = include_str!("fragment_alias_directive/fixtures/alias_on_spread_of_plural_fragment_into_singular_selection.expected");
54+
test_fixture(transform_fixture, file!(), "alias_on_spread_of_plural_fragment_into_singular_selection.graphql", "fragment_alias_directive/fixtures/alias_on_spread_of_plural_fragment_into_singular_selection.expected", input, expected).await;
55+
}
56+
5057
#[tokio::test]
5158
async fn alias_on_spread_of_plural_fragment_invalid() {
5259
let input = include_str!("fragment_alias_directive/fixtures/alias_on_spread_of_plural_fragment.invalid.graphql");
@@ -144,3 +151,17 @@ async fn skip_inline_fragment_without_alias() {
144151
let expected = include_str!("fragment_alias_directive/fixtures/skip_inline_fragment_without_alias.expected");
145152
test_fixture(transform_fixture, file!(), "skip_inline_fragment_without_alias.graphql", "fragment_alias_directive/fixtures/skip_inline_fragment_without_alias.expected", input, expected).await;
146153
}
154+
155+
#[tokio::test]
156+
async fn spread_of_plural_fragment_into_singular_selection_without_alias_invalid() {
157+
let input = include_str!("fragment_alias_directive/fixtures/spread_of_plural_fragment_into_singular_selection_without_alias.invalid.graphql");
158+
let expected = include_str!("fragment_alias_directive/fixtures/spread_of_plural_fragment_into_singular_selection_without_alias.invalid.expected");
159+
test_fixture(transform_fixture, file!(), "spread_of_plural_fragment_into_singular_selection_without_alias.invalid.graphql", "fragment_alias_directive/fixtures/spread_of_plural_fragment_into_singular_selection_without_alias.invalid.expected", input, expected).await;
160+
}
161+
162+
#[tokio::test]
163+
async fn unaliased_spread_of_plural_fragment_that_might_not_match() {
164+
let input = include_str!("fragment_alias_directive/fixtures/unaliased_spread_of_plural_fragment_that_might_not_match.graphql");
165+
let expected = include_str!("fragment_alias_directive/fixtures/unaliased_spread_of_plural_fragment_that_might_not_match.expected");
166+
test_fixture(transform_fixture, file!(), "unaliased_spread_of_plural_fragment_that_might_not_match.graphql", "fragment_alias_directive/fixtures/unaliased_spread_of_plural_fragment_that_might_not_match.expected", input, expected).await;
167+
}

0 commit comments

Comments
 (0)
0