-
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
package graphql.nadel.validation | ||
|
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. As I was moving |
||
private val typeWrappingValidation: NadelTypeWrappingValidation, | ||
) { | ||
context(NadelValidationContext) | ||
fun isOutputTypeAssignable( | ||
overallType: GraphQLType, | ||
underlyingType: GraphQLType, | ||
): Boolean { | ||
// Note: the (supplied) value comes from the underlying service, and that value needs to fit the (required) overall field's type | ||
return isTypeAssignable( | ||
suppliedType = underlyingType, | ||
requiredType = overallType, | ||
// Compare underlying type names | ||
suppliedTypeName = underlyingType.unwrapAll().name, | ||
requiredTypeName = getUnderlyingTypeName(overallType.unwrapAll()), | ||
) | ||
} | ||
|
||
context(NadelValidationContext) | ||
fun isInputTypeAssignable( | ||
overallType: GraphQLType, | ||
underlyingType: GraphQLType, | ||
): Boolean { | ||
// Note: the (supplied) value comes from the user (overall schema) and needs to be assigned to the (required) underlying type | ||
return isTypeAssignable( | ||
suppliedType = overallType, | ||
requiredType = underlyingType, | ||
// Compare underlying type names | ||
suppliedTypeName = getUnderlyingTypeName(overallType.unwrapAll()), | ||
requiredTypeName = underlyingType.unwrapAll().name, | ||
) | ||
} | ||
|
||
context(NadelValidationContext) | ||
fun isTypeAssignable( | ||
suppliedType: GraphQLType, | ||
requiredType: GraphQLType, | ||
suppliedTypeName: String, | ||
requiredTypeName: String, | ||
): Boolean { | ||
return suppliedTypeName == requiredTypeName && isTypeWrappingValid(suppliedType, requiredType) | ||
} | ||
|
||
private fun isTypeWrappingValid( | ||
suppliedType: GraphQLType, | ||
requiredType: GraphQLType, | ||
): Boolean { | ||
return typeWrappingValidation.isTypeWrappingValid( | ||
lhs = suppliedType, | ||
rhs = requiredType, | ||
rule = NadelTypeWrappingValidation.Rule.LHS_MUST_BE_STRICTER_OR_SAME, | ||
) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,27 +1,30 @@ | ||
package graphql.nadel.validation | ||
|
||
import graphql.nadel.engine.util.unwrapAll | ||
import graphql.nadel.definition.renamed.NadelRenamedDefinition | ||
import graphql.nadel.engine.blueprint.NadelDeepRenameFieldInstruction | ||
import graphql.nadel.engine.blueprint.NadelFieldInstruction | ||
import graphql.nadel.engine.blueprint.NadelRenameFieldInstruction | ||
import graphql.nadel.engine.transform.query.NadelQueryPath | ||
import graphql.nadel.engine.util.getFieldAt | ||
import graphql.nadel.engine.util.makeFieldCoordinates | ||
import graphql.nadel.schema.NadelDirectives | ||
import graphql.nadel.validation.NadelSchemaValidationError.CannotRenameHydratedField | ||
import graphql.nadel.validation.NadelSchemaValidationError.CannotRenamePartitionedField | ||
import graphql.nadel.validation.NadelSchemaValidationError.IncompatibleArgumentInputType | ||
import graphql.nadel.validation.NadelSchemaValidationError.IncompatibleFieldOutputType | ||
import graphql.nadel.validation.NadelSchemaValidationError.MissingArgumentOnUnderlying | ||
import graphql.nadel.validation.NadelSchemaValidationError.MissingRename | ||
import graphql.nadel.validation.NadelSchemaValidationError.MissingUnderlyingField | ||
import graphql.nadel.validation.NadelTypeWrappingValidation.Rule.LHS_MUST_BE_LOOSER_OR_SAME | ||
import graphql.nadel.validation.hydration.NadelHydrationValidation | ||
import graphql.nadel.validation.util.NadelCombinedTypeUtil.getFieldsThatServiceContributed | ||
import graphql.schema.GraphQLArgument | ||
import graphql.schema.GraphQLFieldDefinition | ||
import graphql.schema.GraphQLNamedSchemaElement | ||
import graphql.schema.GraphQLOutputType | ||
|
||
class NadelFieldValidation internal constructor( | ||
private val hydrationValidation: NadelHydrationValidation, | ||
private val partitionValidation: NadelPartitionValidation, | ||
private val assignableTypeValidation: NadelAssignableTypeValidation, | ||
) { | ||
private val renameValidation = NadelRenameValidation(this) | ||
private val inputValidation = NadelInputValidation() | ||
private val partitionValidation = NadelPartitionValidation() | ||
private val typeWrappingValidation = NadelTypeWrappingValidation() | ||
|
||
context(NadelValidationContext) | ||
fun validate( | ||
schemaElement: NadelServiceSchemaElement.FieldsContainer, | ||
|
@@ -67,7 +70,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 commentThe 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 |
||
} else if (isHydrated(parent, overallField)) { | ||
hydrationValidation.validate(parent, overallField) | ||
} else { | ||
|
@@ -96,14 +99,12 @@ class NadelFieldValidation internal constructor( | |
if (underlyingArg == null) { | ||
MissingArgumentOnUnderlying(parent, overallField, underlyingField, overallArg) | ||
} else { | ||
if (isUnwrappedArgTypeSame(overallArg, underlyingArg)) { | ||
inputValidation | ||
.validate( | ||
parent = parent, | ||
overallField = overallField, | ||
overallInputArgument = overallArg, | ||
underlyingInputArgument = underlyingArg | ||
) | ||
val isArgumentTypeAssignable = assignableTypeValidation.isInputTypeAssignable( | ||
overallType = overallArg.type, | ||
underlyingType = underlyingArg.type | ||
) | ||
if (isArgumentTypeAssignable) { | ||
ok() | ||
} else { | ||
IncompatibleArgumentInputType( | ||
parentType = parent, | ||
|
@@ -123,13 +124,61 @@ class NadelFieldValidation internal constructor( | |
} | ||
|
||
context(NadelValidationContext) | ||
private fun isUnwrappedArgTypeSame( | ||
overallArg: GraphQLArgument, | ||
underlyingArg: GraphQLArgument, | ||
): Boolean { | ||
val overallArgTypeUnwrapped = overallArg.type.unwrapAll() | ||
val underlyingArgTypeUnwrapped = underlyingArg.type.unwrapAll() | ||
return getUnderlyingTypeName(overallArgTypeUnwrapped) == underlyingArgTypeUnwrapped.name | ||
private fun validateRename( | ||
parent: NadelServiceSchemaElement.FieldsContainer, | ||
overallField: GraphQLFieldDefinition, | ||
): NadelSchemaValidationResult { | ||
if (isHydrated(parent, overallField)) { | ||
return CannotRenameHydratedField(parent, overallField) | ||
} | ||
|
||
if (isPartitioned(parent, overallField)) { | ||
return CannotRenamePartitionedField(parent, overallField) | ||
} | ||
|
||
val rename = getRenamedOrNull(parent, overallField) | ||
?: return ok() | ||
|
||
val underlyingField = parent.underlying.getFieldAt(rename.from) | ||
?: return MissingRename(parent, overallField, rename) | ||
|
||
// In theory, this should have .onError { return it } but the schema has too many violations… | ||
val result = validate(parent, overallField, underlyingField) | ||
|
||
return if (parent is NadelServiceSchemaElement.Object) { | ||
results( | ||
result, | ||
NadelValidatedFieldResult( | ||
service = parent.service, | ||
fieldInstruction = makeRenameFieldInstruction(parent, overallField, rename) | ||
), | ||
) | ||
} else { | ||
result | ||
} | ||
} | ||
|
||
context(NadelValidationContext) | ||
private fun makeRenameFieldInstruction( | ||
parent: NadelServiceSchemaElement.FieldsContainer, | ||
overallField: GraphQLFieldDefinition, | ||
rename: NadelRenamedDefinition.Field, | ||
): NadelFieldInstruction { | ||
val location = makeFieldCoordinates( | ||
parentType = parent.overall, | ||
field = overallField, | ||
) | ||
|
||
val underlyingName = rename.from.singleOrNull() | ||
?: return NadelDeepRenameFieldInstruction( | ||
location = location, | ||
queryPathToField = NadelQueryPath(rename.from), | ||
) | ||
|
||
return NadelRenameFieldInstruction( | ||
location = location, | ||
underlyingName = underlyingName, | ||
) | ||
} | ||
|
||
context(NadelValidationContext) | ||
|
@@ -138,8 +187,12 @@ class NadelFieldValidation internal constructor( | |
overallField: GraphQLFieldDefinition, | ||
underlyingField: GraphQLFieldDefinition, | ||
): NadelSchemaValidationResult { | ||
// This checks whether the output type e.g. name or List or NonNull wrappings are valid | ||
return if (isOutputTypeValid(overallType = overallField.type, underlyingType = underlyingField.type)) { | ||
val isUnderlyingTypeAssignable = assignableTypeValidation.isOutputTypeAssignable( | ||
overallType = overallField.type, | ||
underlyingType = underlyingField.type, | ||
) | ||
|
||
return if (isUnderlyingTypeAssignable) { | ||
ok() | ||
} else { | ||
results( | ||
|
@@ -148,25 +201,6 @@ class NadelFieldValidation internal constructor( | |
} | ||
} | ||
|
||
/** | ||
* 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 commentThe reason will be displayed to describe this comment to others. Learn more. Moved this to |
||
overallType: GraphQLOutputType, | ||
underlyingType: GraphQLOutputType, | ||
): Boolean { | ||
val isTypeWrappingValid = typeWrappingValidation | ||
.isTypeWrappingValid( | ||
lhs = overallType, | ||
rhs = underlyingType, | ||
rule = LHS_MUST_BE_LOOSER_OR_SAME, | ||
) | ||
|
||
return isTypeWrappingValid | ||
&& getUnderlyingTypeName(overallType.unwrapAll()) == underlyingType.unwrapAll().name | ||
} | ||
|
||
context(NadelValidationContext) | ||
private fun isCombinedType(type: GraphQLNamedSchemaElement): Boolean { | ||
return type.name in combinedTypeNames | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,19 +1,13 @@ | ||
package graphql.nadel.validation | ||
|
||
import graphql.nadel.engine.util.strictAssociateBy | ||
import graphql.nadel.engine.util.unwrapAll | ||
import graphql.nadel.validation.NadelSchemaValidationError.IncompatibleArgumentInputType | ||
import graphql.nadel.validation.NadelSchemaValidationError.IncompatibleFieldInputType | ||
import graphql.nadel.validation.NadelSchemaValidationError.MissingUnderlyingInputField | ||
import graphql.schema.GraphQLArgument | ||
import graphql.schema.GraphQLFieldDefinition | ||
import graphql.schema.GraphQLInputObjectField | ||
import graphql.schema.GraphQLInputType | ||
import graphql.schema.GraphQLUnmodifiedType | ||
|
||
class NadelInputValidation internal constructor() { | ||
private val typeWrappingValidation = NadelTypeWrappingValidation() | ||
|
||
class NadelInputObjectValidation internal constructor( | ||
private val assignableTypeValidation: NadelAssignableTypeValidation, | ||
) { | ||
context(NadelValidationContext) | ||
fun validate( | ||
schemaElement: NadelServiceSchemaElement.InputObject, | ||
|
@@ -61,46 +55,15 @@ class NadelInputValidation internal constructor() { | |
overallInputField: GraphQLInputObjectField, | ||
underlyingInputField: GraphQLInputObjectField, | ||
): NadelSchemaValidationResult { | ||
return if (!isInputTypeValid(overallInputField.type, underlyingInputField.type)) { | ||
IncompatibleFieldInputType(parent, overallInputField, underlyingInputField) | ||
} else { | ||
ok() | ||
} | ||
} | ||
val isTypeAssignable = assignableTypeValidation.isInputTypeAssignable( | ||
overallType = overallInputField.type, | ||
underlyingType = underlyingInputField.type | ||
) | ||
|
||
context(NadelValidationContext) | ||
fun validate( | ||
parent: NadelServiceSchemaElement.FieldsContainer, | ||
overallField: GraphQLFieldDefinition, | ||
overallInputArgument: GraphQLArgument, | ||
underlyingInputArgument: GraphQLArgument, | ||
): NadelSchemaValidationResult { | ||
return if (!isInputTypeValid(overallInputArgument.type, underlyingInputArgument.type)) { | ||
IncompatibleArgumentInputType(parent, overallField, overallInputArgument, underlyingInputArgument) | ||
} else { | ||
return if (isTypeAssignable) { | ||
ok() | ||
} else { | ||
IncompatibleFieldInputType(parent, overallInputField, underlyingInputField) | ||
} | ||
} | ||
|
||
context(NadelValidationContext) | ||
private fun isInputTypeValid( | ||
overallType: GraphQLInputType, | ||
underlyingType: GraphQLInputType, | ||
): Boolean { | ||
val typeWrappingValid = typeWrappingValidation.isTypeWrappingValid( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All this deleted code was just moved to |
||
lhs = overallType, | ||
rhs = underlyingType, | ||
rule = NadelTypeWrappingValidation.Rule.LHS_MUST_BE_STRICTER_OR_SAME, | ||
) | ||
|
||
return typeWrappingValid && isInputTypeNameValid(overallType.unwrapAll(), underlyingType.unwrapAll()) | ||
} | ||
|
||
context(NadelValidationContext) | ||
private fun isInputTypeNameValid( | ||
overallType: GraphQLUnmodifiedType, | ||
underlyingType: GraphQLUnmodifiedType, | ||
): Boolean { | ||
return getUnderlyingTypeName(overallType) == underlyingType.name | ||
} | ||
} |
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.