New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't automatically add key fields to union selections #5562
Conversation
✅ Deploy Preview for apollo-android-docs canceled.
|
val requiredFieldNames = if (schema.typeDefinition(parentType) is GQLUnionTypeDefinition) { | ||
// Can't select any fields on unions other than __typename | ||
mutableSetOf() | ||
} else { | ||
schema.keyFields(parentType).toMutableSet() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we allow @typePolicy
on unions in the first place?
# This feels out of place. SomeUnion has no `id` field.
extend union SomeUnion @typePolicy(keyFields: "id")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was wondering about this :) We could forbid it too, it might be less confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the record, we talked about this and decided to keep the directive definition untouched but add validation that the keyFields
actually exists, which should make an error here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out this validation already existed but wasn't performed on interfaces nor unions.
…tion for embeddedFields and connectionFields
65ab702
to
c636040
Compare
i.directives.forEach { directive -> | ||
validateDirective(directive, i) { | ||
issues.add(it.constContextError()) | ||
} | ||
} | ||
|
||
i.fields.forEach { gqlFieldDefinition -> | ||
gqlFieldDefinition.directives.forEach { gqlDirective -> | ||
validateDirective(gqlDirective, gqlFieldDefinition) { | ||
issues.add(it.constContextError()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, this is where having a visitor pattern for validation would be useful. Maybe not right now but at some point.
val keyFieldsArg = directive.arguments.firstOrNull { it.name == "keyFields" } | ||
if (keyFieldsArg != null) { | ||
(keyFieldsArg.value as GQLStringValue).value.parseAsGQLSelections().getOrThrow().forEach { selection -> | ||
if (selection !is GQLField) { | ||
registerIssue("Fragments are not supported in @$TYPE_POLICY directives", keyFieldsArg.sourceLocation) | ||
} else if (selection.selections.isNotEmpty()) { | ||
registerIssue("Composite fields are not supported in @$TYPE_POLICY directives", keyFieldsArg.sourceLocation) | ||
} else { | ||
val definition = fieldDefinitions.firstOrNull { it.name == selection.name } | ||
if (definition == null) { | ||
registerIssue("Field '${selection.name}' is not a valid key field for type '$type'", keyFieldsArg.sourceLocation) | ||
} | ||
} | ||
} | ||
} | ||
|
||
val embeddedFieldsArg = directive.arguments.firstOrNull { it.name == "embeddedFields" } | ||
if (embeddedFieldsArg != null) { | ||
(embeddedFieldsArg.value as GQLStringValue).value.parseAsGQLSelections().getOrThrow().forEach { selection -> | ||
if (selection !is GQLField) { | ||
registerIssue("Fragments are not supported in @$TYPE_POLICY directives", embeddedFieldsArg.sourceLocation) | ||
} else if (selection.selections.isNotEmpty()) { | ||
registerIssue("Composite fields are not supported in @$TYPE_POLICY directives", embeddedFieldsArg.sourceLocation) | ||
} else { | ||
val definition = fieldDefinitions.firstOrNull { it.name == selection.name } | ||
if (definition == null) { | ||
registerIssue("Field '${selection.name}' is not a valid embedded field for type '$type'", embeddedFieldsArg.sourceLocation) | ||
} | ||
} | ||
} | ||
} | ||
|
||
val connectionFieldsArg = directive.arguments.firstOrNull { it.name == "connectionFields" } | ||
if (connectionFieldsArg != null) { | ||
(connectionFieldsArg.value as GQLStringValue).value.parseAsGQLSelections().getOrThrow().forEach { selection -> | ||
if (selection !is GQLField) { | ||
registerIssue("Fragments are not supported in @$TYPE_POLICY directives", connectionFieldsArg.sourceLocation) | ||
} else if (selection.selections.isNotEmpty()) { | ||
registerIssue("Composite fields are not supported in @$TYPE_POLICY directives", connectionFieldsArg.sourceLocation) | ||
} else { | ||
val definition = fieldDefinitions.firstOrNull { it.name == selection.name } | ||
if (definition == null) { | ||
registerIssue("Field '${selection.name}' is not a valid connection field for type '$type'", connectionFieldsArg.sourceLocation) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Also, can this logic be factored in? Looks like it's very similar?
check(issues.any { it.message.contains("Field 'id' is not a valid key field for type 'Animal'") }) | ||
check(issues.any { it.message.contains("Field 'version' is not a valid key field for type 'Node'") }) | ||
check(issues.any { it.message.contains("Field 'x' is not a valid key field for type 'Foo'") }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I'd change the error to something like
"No such field: 'Foo.x'"
I think there's no need to mention "key" since it's easy to see from the context and sourceLocation.
Key fields were added to selections of unions, but that's not valid as unions have no fields (except
__typename
), and was crashing later on here.We can still check if selections have the key fields at least.