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
Implicit external check #2449
Merged
Merged
Implicit external check #2449
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In Federation 2 key fields are implicitly external
Update comment on expected behavior of the resolvable argument. Add comment to documentation about external directive.
This may also resolve #2387 |
Nevermind the lack of multi key support, that seems like it's broadly an issue that will have to be addressed #1031 |
Thanks so much! We would love any help on #1031 if you can! |
No worries, I'll pick that up next 99time, which is every other Monday |
awesome. thanks for getting this fixed! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #2443
As specified in the Federation v2 documentation The
@external
directive is no longer required for@key
fields, but it is still required for @requires and @provides. When fields are part of the key they should implicitly be considered@external
.This PR updates the
allFieldsAreExternal
method on the entity to take a federation version parameter, if the verstion is set to 2 it checks if the relevant field is is listed in the@key
directive. If it is, the field is considered external without the@external
directive.This PR also requires the
resolvable
argument to the@key
directive to be set to false for it to consider any fields implicitly external. This is not entirely in line with the spec as it seems that ifresolvable
is false the entity should be excluded irrespective of what fields are external. In this PR, a less significant departure from the current behavior is favored over correctness as the change would potentially cause existing schemas to start failing.This PR does not address reference types that have multiple keys defined. from the documentation it seems like a reference type should only use one key directive. All examples show entities with a single key, but the description is ambiguous:
I have: