Align @external with @shareable for how it behaves on a type #2214
Conversation
i.e. marking the type external makes all fields external. Fixes #2156
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
There was a problem hiding this comment.
This looks mostly good as most testing of external appears to properly rely on the ExternalTester, but unfortunately there seems to be a few exceptions within querygrapht.ts (one here and a few similar later).
I'd be suggest adding a new isExternal method similar to the hasDirective of this field, but that calls metadata.isFieldExternal instead of just checking the directive directly (this will leave the hasDirective only used once for @requires but that's probably fine).
| @@ -965,6 +965,11 @@ class GraphBuilderFromSchema extends GraphBuilder { | |||
| return !!metadata && field.hasAppliedDirective(directiveFct(metadata)); | |||
| } | |||
|
|
|||
| private isExternal(field: FieldDefinition<any>): boolean { | |||
| const metadata = federationMetadata(this.schema); | |||
| return !!metadata && metadata.isFieldExternal(field); | |||
There was a problem hiding this comment.
@pcmanus
I followed the pattern you had above, but is there a reason we shouldn't just assert metadata here?
There was a problem hiding this comment.
Yes. We build QueryGraph using this code both for subgraph schema and for the supergraph API schema. The former will always have a metadata but the later won't, but we don't want to hard fail on it.
If we were to guard the calling of those methods with this.isFederatedSubgraph, then yes, we could assert metadata at that point, but not sure it would be much cleaner.
| @@ -965,6 +965,11 @@ class GraphBuilderFromSchema extends GraphBuilder { | |||
| return !!metadata && field.hasAppliedDirective(directiveFct(metadata)); | |||
| } | |||
|
|
|||
| private isExternal(field: FieldDefinition<any>): boolean { | |||
| const metadata = federationMetadata(this.schema); | |||
| return !!metadata && metadata.isFieldExternal(field); | |||
There was a problem hiding this comment.
Yes. We build QueryGraph using this code both for subgraph schema and for the supergraph API schema. The former will always have a metadata but the later won't, but we don't want to hard fail on it.
If we were to guard the calling of those methods with this.isFederatedSubgraph, then yes, we could assert metadata at that point, but not sure it would be much cleaner.
Historically, `@external` has always had `OBJECT` as allowed location, but in federation 1, this was competely ignored and did nothing. Since that has changed in federation 2 with apollographql#2214, this patch ensures any pre-existing use of `@external` on an object type is removed by the fed1 upgrading code.
Historically, `@external` has always had `OBJECT` as allowed location, but in federation 1, this was competely ignored and did nothing. Since that has changed in federation 2 with #2214, this patch ensures any pre-existing use of `@external` on an object type is removed by the fed1 upgrading code.
For historical reasons, `@external` was legal to put on an object type since about forever (even in federation 1) but it used to be completely ignored. We "fixed" this support in apollographql#2214, but to limit surprised on upgrades from fed1, the schema upgrader had been modified to drop any `@external` on an object type (since again, those were legal but ignored in fed1). However, the code for adding @Shareable in that same schema upgrader was not updated correctly and so it _was_ taking `@external` on types into account, and thus it ended up _not_ adding `@shareable` in some cases where it should have.
For historical reasons, `@external` was legal to put on an object type since about forever (even in federation 1) but it used to be completely ignored. We "fixed" this support in #2214, but to limit surprises on upgrades from fed1, the schema upgrader had been modified to drop any `@external` on an object type (since again, those were legal but ignored in fed1). However, the code for adding @Shareable in that same schema upgrader was not updated correctly and so it _was_ taking `@external` on types into account, and thus it ended up _not_ adding `@shareable` in some cases where it should have.
i.e. marking the type external makes all fields external.
Fixes #2156