ISSUE-1566: Relax federated type validation rules #1571
Merged
+27
−262
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
samuelAndalon
added
module: generator
dariuszkuc
reviewed
Oct 11, 2022
| @@ -128,7 +129,7 @@ class FederatedKeyDirectiveIT { | |||
| assertEquals(expected, exception.message) | |||
| } | |||
|
|
|||
| @Test | |||
| @Disabled("This is a valid case according to Federation 2 spec") | |||
There was a problem hiding this comment.
instead of disabling this test, this code (and referenced sample project) should be deleted
dariuszkuc
reviewed
Oct 11, 2022
| @@ -70,7 +71,7 @@ class FederatedRequiresDirectiveIT { | |||
| assertEquals(expected, exception.message) | |||
| } | |||
|
|
|||
| @Test | |||
| @Disabled("this is a valid use case according to Federation 2 spec") | |||
There was a problem hiding this comment.
instead of disabling this test, this code (and referenced sample project) should be deleted
dariuszkuc
reviewed
Oct 11, 2022
| errors.addAll(validateDirective("$validatedType.${validatedField.name}", REQUIRES_DIRECTIVE_NAME, validatedField.allAppliedDirectivesByName, fieldMap, extendedType)) | ||
| } else { | ||
| errors.add("base $validatedType type has fields marked with @requires directive, validatedField=${validatedField.name}") | ||
| return validateDirective("$validatedType.${validatedField.name}", REQUIRES_DIRECTIVE_NAME, validatedField.allAppliedDirectivesByName, fieldMap, extendedType) |
There was a problem hiding this comment.
this validation should be applied for both base and extended type -> it doesn't make sense to keep this file anymore, move this validateDirective call directly to FederatedSchemaValidator
dariuszkuc
reviewed
Oct 11, 2022
| @@ -225,7 +226,7 @@ class FederatedSchemaValidatorTest { | |||
| * } | |||
| */ | |||
| @Test | |||
| fun `validate local GraphQLObjectType with valid key directive and field with invalid external directive`() { | |||
| fun `validate local GraphQLObjectType with valid key directive and field with external directive`() { | |||
There was a problem hiding this comment.
lets just drop this test
dariuszkuc
reviewed
Oct 11, 2022
| @@ -45,7 +45,7 @@ class ValidateRequiresDirectiveKtTest { | |||
| * } | |||
| */ | |||
| @Test | |||
| fun `Verify non extended types and non requires fields returns an error`() { | |||
| fun `Verify non extended types and non requires fields returns no error`() { | |||
There was a problem hiding this comment.
i think those tests should be dropped
dariuszkuc
approved these changes
Oct 12, 2022
|
retest |
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.
📝 Description
Relax federated type validation rules
@externaldirective@requiresdirective@requiresdirective referring to fields with@externaldirective🔗 Related Issues
#1566