fix(composition): fixed unexpected EXTENSION_WITH_NO_BASE errors from upgrading/merging
#8475
Conversation
- to handle types with both non-extension and extension elements.
✅ Docs preview has no changesThe preview was not built because there were no changes. Build ID: c6887964da5ab98ae93ab186 |
|
@duckki, please consider creating a changeset entry in |
| continue; | ||
| }; | ||
|
|
||
| if element.has_non_extension_elements() { |
There was a problem hiding this comment.
I was reading the accompanying apollo-rs PR, but I think this discussion actually belongs over here: in short, do we actually need to be doing this check?
I know that the JS version checks this, but I think there are a lot of the parts of that codebase that are compensating for the fact that they were operating on potentially invalid ASTs parsed by graphql-js. This, in particular, feels like a validation we can delegate to apollo-compiler and omit from our composition port (since we're intentionally relaxing it with adopt_orphaned_extensions).
This is largely based on my gut reaction that the compiler changes are a lot larger than I expected, and they may even be considered breaking due to the change in all the type structs. Are we making life unnecessarily difficult for ourselves by porting the extension-with-no-base check?
There was a problem hiding this comment.
Based on our discussion from yesterday, we probably do need to carry that forward to properly validate Fed 1 schemas prior to upgrade. I've left a comment on the apollo-rs PR to discuss the overall design.
There was a problem hiding this comment.
has_extension_element() and has_non_extension_element() are not mutually exclusive. A schema may have them both.
scalar X
extend scalar X @something
Even if apollo-rs can record origins correctly, FED-772 will persist without this change.
And this is not fed1 specific.
There was a problem hiding this comment.
The predicate we want would be probably type.is_orphan_extension(). has_extension_element() && !has_non_extension_element() was trying to achieve that.
The Schema struct doesn't have that information. But, SchemaBuilder does. I think we can export the orphan_type_extensions info without breaking apollo-compiler API. From that, we can implement is_orphan_extension().
|
#8505 is alternative solution. Closing. |
Upstream PR: apollographql/apollo-rs#1009.
Summary
Merge::check_for_extension_with_no_baseto handle types with both non-extension and extension elementsChecklist
Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.
Exceptions
Composition bug fix