-
Notifications
You must be signed in to change notification settings - Fork 23
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
Improve schema validation factory #638
base: master
Are you sure you want to change the base?
Conversation
Test Results 134 files +1 134 suites +1 56s ⏱️ -1s Results for commit a19a800. ± Comparison against base commit bc72914. This pull request removes 1 and adds 2 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
a0a7211
to
9fbd4d7
Compare
b0cf2ca
to
a337f89
Compare
9fbd4d7
to
d193124
Compare
import graphql.nadel.engine.util.unwrapAll | ||
import graphql.schema.GraphQLType | ||
|
||
class NadelAssignableTypeValidation internal constructor( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I was moving NadelTypeWrappingValidation
around I realised I could extract some duplicated code into here.
@Deprecated("Mistake to use this inside validation", level = DeprecationLevel.ERROR) | ||
fun GraphQLFieldDefinition.hasHydratedDefinition(): Nothing { | ||
throw UnsupportedOperationException() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't like this approach, moved this into a archunit test instead.
@@ -59,7 +62,7 @@ class NadelFieldValidation internal constructor( | |||
overallField: GraphQLFieldDefinition, | |||
): NadelSchemaValidationResult { | |||
return if (isRenamed(parent, overallField)) { | |||
renameValidation.validate(parent, overallField) | |||
validateRename(parent, overallField) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extracted the one rename validation function to this class to avoid a cyclic dependency in NadelRenameValidation
* It checks whether the type name and type wrappings e.g. [graphql.schema.GraphQLNonNull] make sense. | ||
*/ | ||
context(NadelValidationContext) | ||
private fun isOutputTypeValid( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this to NadelAssignableTypeValidation
overallType: GraphQLInputType, | ||
underlyingType: GraphQLInputType, | ||
): Boolean { | ||
val typeWrappingValid = typeWrappingValidation.isTypeWrappingValid( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this deleted code was just moved to NadelAssignableTypeValidation.isInputTypeAssignable
No description provided.