Skip to content
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 11 commits into from Dec 5, 2022
Merged

Implicit external check #2449

merged 11 commits into from Dec 5, 2022

Conversation

jclyons52
Copy link
Contributor

@jclyons52 jclyons52 commented Dec 5, 2022

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 if resolvable 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:

A subgraph that references an entity without contributing any fields can include the fields of any @key in its stub definition

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)

@coveralls
Copy link

coveralls commented Dec 5, 2022

Coverage Status

Coverage increased (+0.02%) to 75.66% when pulling 500195a on implicit-external-check into 5065163 on master.

@jclyons52 jclyons52 marked this pull request as ready for review December 5, 2022 03:46
@jclyons52
Copy link
Contributor Author

This may also resolve #2387

@jclyons52
Copy link
Contributor Author

Nevermind the lack of multi key support, that seems like it's broadly an issue that will have to be addressed #1031

@StevenACoffman
Copy link
Collaborator

Thanks so much! We would love any help on #1031 if you can!

@StevenACoffman StevenACoffman merged commit db1e3b8 into master Dec 5, 2022
@StevenACoffman StevenACoffman deleted the implicit-external-check branch December 5, 2022 13:19
@jclyons52
Copy link
Contributor Author

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

@jdavisclark
Copy link

awesome. thanks for getting this fixed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unnecessary entity resolvers generated for stub types in Federation 2
4 participants