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

fix: GQL syntax strings are incorrectly validated as enums #1134

Merged
merged 3 commits into from
Nov 10, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 12 additions & 10 deletions core/src/main/scala/caliban/GraphQL.scala
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import caliban.introspection.Introspector
import caliban.introspection.adt._
import caliban.parsing.adt.Definition.TypeSystemDefinition.SchemaDefinition
import caliban.parsing.adt.{ Document, OperationType }
import caliban.parsing.{ Parser, SourceMapper }
import caliban.parsing.{ Parser, SourceMapper, VariablesUpdater }
import caliban.schema._
import caliban.validation.Validator
import caliban.wrappers.Wrapper
Expand Down Expand Up @@ -94,21 +94,23 @@ trait GraphQL[-R] { self =>
case (overallWrappers, parsingWrappers, validationWrappers, executionWrappers, fieldWrappers, _) =>
wrap((request: GraphQLRequest) =>
(for {
doc <- wrap(Parser.parseQuery)(parsingWrappers, request.query.getOrElse(""))
intro = Introspector.isIntrospection(doc)
_ <- IO.when(intro && !enableIntrospection) {
IO.fail(CalibanError.ValidationError("Introspection is disabled", ""))
}
typeToValidate = if (intro) introspectionRootType else rootType
schemaToExecute = if (intro) introspectionRootSchema else schema
doc <- wrap(Parser.parseQuery)(parsingWrappers, request.query.getOrElse(""))
intro = Introspector.isIntrospection(doc)
_ <- IO.when(intro && !enableIntrospection) {
IO.fail(CalibanError.ValidationError("Introspection is disabled", ""))
}
typeToValidate = if (intro) introspectionRootType else rootType
schemaToExecute = if (intro) introspectionRootSchema else schema
updatedRequest = VariablesUpdater.updateVariables(request, doc, typeToValidate)

validate = (doc: Document) =>
Validator
.prepare(
doc,
typeToValidate,
schemaToExecute,
request.operationName,
request.variables.getOrElse(Map()),
updatedRequest.operationName,
updatedRequest.variables.getOrElse(Map.empty),
skipValidation
)
executionRequest <- wrap(validate)(validationWrappers, doc)
Expand Down
30 changes: 1 addition & 29 deletions core/src/main/scala/caliban/execution/Field.scala
Original file line number Diff line number Diff line change
Expand Up @@ -109,40 +109,12 @@ object Field {
definition <- variableDefinitions.find(_.name == name)
defaultValue = definition.defaultValue getOrElse NullValue
value = variableValues.getOrElse(name, defaultValue)
} yield resolveEnumValues(value, definition, rootType)) getOrElse NullValue
} yield value) getOrElse NullValue
case value: Value => value
}
arguments.map { case (k, v) => k -> resolveVariable(v) }
}

// Since we cannot separate a String from an Enum when variables
// are parsed, we need to translate from strings to enums here
// if we have a valid enum field.
private def resolveEnumValues(
value: InputValue,
definition: VariableDefinition,
rootType: RootType
): InputValue = {
val t = Type
.innerType(definition.variableType)

rootType.types
.get(t)
.map(_.kind)
.flatMap { kind =>
(kind, value) match {
case (__TypeKind.ENUM, InputValue.ListValue(v)) =>
Some(
InputValue.ListValue(v.map(resolveEnumValues(_, definition, rootType)))
)
case (__TypeKind.ENUM, Value.StringValue(v)) =>
Some(Value.EnumValue(v))
case _ => None
}
}
.getOrElse(value)
}

private def subtypeNames(typeName: String, rootType: RootType): Option[List[String]] =
rootType.types
.get(typeName)
Expand Down
53 changes: 53 additions & 0 deletions core/src/main/scala/caliban/parsing/VariablesUpdater.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
package caliban.parsing

import caliban.GraphQLRequest
import caliban.introspection.adt._
import caliban.parsing.adt.Definition.ExecutableDefinition.OperationDefinition
import caliban.parsing.adt._
import caliban.schema.RootType
import caliban.{ InputValue, Value }
import zio.{ IO, UIO }

object VariablesUpdater {
def updateVariables(
req: GraphQLRequest,
doc: Document,
rootType: RootType
): GraphQLRequest = {
val variableDefinitions = doc.operationDefinitions.flatMap(_.variableDefinitions)
val updated = req.variables.getOrElse(Map.empty).map { case (key, value) =>
val v = variableDefinitions.find(_.name == key).map(resolveEnumValues(value, _, rootType)).getOrElse(value)
key -> v
}

req.copy(variables = Some(updated))
}

// Since we cannot separate a String from an Enum when variables
// are parsed, we need to translate from strings to enums here
// if we have a valid enum field.
private def resolveEnumValues(
value: InputValue,
definition: VariableDefinition,
rootType: RootType
): InputValue = {
val t = Type
.innerType(definition.variableType)

rootType.types
.get(t)
.map(_.kind)
.flatMap { kind =>
(kind, value) match {
case (__TypeKind.ENUM, InputValue.ListValue(v)) =>
Some(
InputValue.ListValue(v.map(resolveEnumValues(_, definition, rootType)))
)
case (__TypeKind.ENUM, Value.StringValue(v)) =>
Some(Value.EnumValue(v))
case _ => None
}
}
.getOrElse(value)
}
}
6 changes: 2 additions & 4 deletions core/src/main/scala/caliban/validation/ValueValidator.scala
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,9 @@ object ValueValidator {
}
case ENUM =>
argValue match {
case EnumValue(value) =>
case EnumValue(value) =>
validateEnum(value, inputType, errorContext)
case StringValue(value) =>
validateEnum(value, inputType, errorContext)
case _ =>
case _ =>
failValidation(
s"$errorContext has invalid type: $argValue",
"Input field was supposed to be an enum value."
Expand Down
22 changes: 22 additions & 0 deletions core/src/test/scala/caliban/execution/FieldArgsSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import caliban.Value.EnumValue
import zio._
import zio.test._
import zio.test.environment.TestEnvironment
import Assertion._
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: I usually use full paths, so zio.test.Assertion._

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


object FieldArgsSpec extends DefaultRunnableSpec {
sealed trait COLOR
Expand Down Expand Up @@ -155,6 +156,27 @@ object FieldArgsSpec extends DefaultRunnableSpec {
)
)
)
},
testM("it doesn't allow strings as enums in GQL syntax") {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed GraphQL string literals must not be accepted as an enum input and instead raise a query error. 👍

case class QueryInput(color: COLOR)
case class Query(query: QueryInput => UIO[String])
val query =
"""query {
| query(color: "BLUE")
|}""".stripMargin

val api = graphQL(
RootResolver(
Query(
query = i => ZIO.succeed(i.toString)
)
)
)

for {
interpreter <- api.interpreter
res <- interpreter.execute(query)
} yield assert(res.errors.headOption)(isSome(anything))
}
)
}