Fix fed1 schema upgraded when @external is on a type.
#2343
Conversation
👷 Deploy request for apollo-federation-docs pending review.Visit the deploys page to approve it
|
🦋 Changeset detectedLatest commit: d63fc8e The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
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. |
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.
94eeaa7 to
8d8c40d
Compare
internals-js/src/federation.ts
Outdated
| // We do not collect @external on types for fed1 schema as those are ignored and will be discarded by the schema upgrader | ||
| // Do note however that the schema upgrader relying on the methods of this tester to figure out if a subgraph resolves | ||
| // a field in order to know when @shareable should be automatically added, hence the need for the short-cut here. | ||
| if (!this.isFed2Schema) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
For my own understanding I've rewritten this comment a bit. Is this correct, and if so would you consider using it here or updating yours a bit? Your PR description explains this but the comment misses this part which was important for me to understand it:
In the fed1 case, if the map is populated then
@shareablewon't be added in places where it should have.
My interpretation / suggestion:
We do not collect @external on types for fed1 schema since those will be discarded by the schema upgrader.
The schema upgrader relies on the populated externalFieldsOnType object to inform when @shareable should be automatically added. In the fed1 case, if the map is populated then @shareable won't be added in places where it should have.
There was a problem hiding this comment.
The updates are correct, and are probably clearer; made the change.
.changeset/flat-pandas-cross.md
Outdated
| "@apollo/federation-internals": patch | ||
| --- | ||
|
|
||
| Fix fed1 schema upgraded when `@external` is on a type. |
There was a problem hiding this comment.
I'm not sure a user reading this will understand how this affects their usage. I think the context in your PR description would be useful here (and in the manual changelog entries).
There was a problem hiding this comment.
You're right, this wasn't that helpful as it could be. I've reworded it so it's more obvious for a user if they may be affected or not by this and care about.
For historical reasons,
@externalwas 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 surprised on upgrades from fed1, the schema upgrader had been modified to drop any@externalon 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@externalon types into account, and thus it ended up not adding@shareablein some cases where it should have.