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

Respect variables with no value provided #1195

Merged
merged 1 commit into from
Dec 9, 2021
Merged
Changes from all 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
Respect variables with no value provided
When resolving a value for a variable, if a value has not been provided
and it has no default then do not include it instead of setting it to
`null`.
  • Loading branch information
guymers committed Dec 9, 2021
commit 75164a6db3a6e759ca6093a11e4f7dccf16b23c4
21 changes: 11 additions & 10 deletions core/src/main/scala/caliban/execution/Field.scala
Original file line number Diff line number Diff line change
@@ -97,20 +97,21 @@ object Field {
variableDefinitions: List[VariableDefinition],
variableValues: Map[String, InputValue]
): Map[String, InputValue] = {
def resolveVariable(value: InputValue): InputValue =
def resolveVariable(value: InputValue): Option[InputValue] =
value match {
case InputValue.ListValue(values) => InputValue.ListValue(values.map(resolveVariable))
case InputValue.ListValue(values) =>
Some(InputValue.ListValue(values.flatMap(resolveVariable)))
case InputValue.ObjectValue(fields) =>
InputValue.ObjectValue(fields.map { case (k, v) => k -> resolveVariable(v) })
Some(InputValue.ObjectValue(fields.flatMap { case (k, v) => resolveVariable(v).map(k -> _) }))
case InputValue.VariableValue(name) =>
(for {
definition <- variableDefinitions.find(_.name == name)
defaultValue = definition.defaultValue getOrElse NullValue
value = variableValues.getOrElse(name, defaultValue)
frekw marked this conversation as resolved.
Show resolved Hide resolved
} yield value) getOrElse NullValue
case value: Value => value
for {
definition <- variableDefinitions.find(_.name == name)
value <- variableValues.get(name).orElse(definition.defaultValue)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit 💅 : getOrElse

Copy link
Contributor Author

Choose a reason for hiding this comment

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

definition.defaultValue is an Option so getOrElse is not applicable?

Copy link
Owner

@ghostdogpr ghostdogpr Dec 9, 2021

Choose a reason for hiding this comment

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

Yeah I think you're right

Copy link
Collaborator

Choose a reason for hiding this comment

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

But so is variableValues.get(name) since you're getting it from a map, right?

Copy link
Owner

Choose a reason for hiding this comment

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

@frekw getOrElse expects an A, but defaultValue is an Option[A]. It's orElse, not getOrElse.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

But in either case, doesn't matter much.

Copy link
Owner

Choose a reason for hiding this comment

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

No, b is of type Any if you do that :P

https://scastie.scala-lang.org/YjZ63y0XRXSxAMGDC6K65Q

Copy link
Collaborator

Choose a reason for hiding this comment

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

😬

Copy link
Collaborator

Choose a reason for hiding this comment

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

And also 🤦 I guess

} yield value
case value: Value =>
Some(value)
}
arguments.map { case (k, v) => k -> resolveVariable(v) }
arguments.flatMap { case (k, v) => resolveVariable(v).map(k -> _) }
}

private def subtypeNames(typeName: String, rootType: RootType): Option[List[String]] =
58 changes: 57 additions & 1 deletion core/src/test/scala/caliban/execution/ExecutionSpec.scala
Original file line number Diff line number Diff line change
@@ -6,7 +6,7 @@ import caliban.CalibanError.ExecutionError
import caliban.GraphQL._
import caliban.Macros.gqldoc
import caliban.TestUtils._
import caliban.Value.{ BooleanValue, IntValue, StringValue }
import caliban.Value.{ BooleanValue, IntValue, NullValue, StringValue }
import caliban.introspection.adt.__Type
import caliban.parsing.adt.LocationInfo
import caliban.schema.Annotations.{ GQLInterface, GQLName, GQLValueType }
@@ -147,6 +147,62 @@ object ExecutionSpec extends DefaultRunnableSpec {
api.interpreter.flatMap(_.execute(query, None, Map("term" -> StringValue("search")))).map(_.asJson.noSpaces)
)(equalTo("""{"data":{"getId":null}}"""))
},
testM("respects variables that are not provided") {
sealed trait ThreeState
object ThreeState {
case object Undefined extends ThreeState
case object Null extends ThreeState
case object Value extends ThreeState

def fromOption[T](o: Option[T]) = o.fold[ThreeState](Null)(_ => Value)

implicit val schema: Schema[Any, ThreeState] = Schema.optionSchema(Schema.booleanSchema).contramap {
case Undefined => None
case Null => Some(false)
case Value => Some(true)
}
implicit val argBuilder: ArgBuilder[ThreeState] = new ArgBuilder[ThreeState] {
private val base = ArgBuilder.option(ArgBuilder.boolean)

override def build(input: InputValue) = base.build(input).map(fromOption(_))
override def buildMissing(default: Option[String]) = default match {
case None => Right(Undefined)
case Some(v) => base.buildMissing(Some(v)).map(fromOption(_))
}
}
}

case class Args(term: String, state: ThreeState)
case class Test(getState: Args => ThreeState)
val api = graphQL(RootResolver(Test(_.state)))
val query = """query test($term: String!, $state: Boolean) { getState(term: $term, state: $state) }"""
val queryDefault =
"""query test($term: String!, $state: Boolean = null) { getState(term: $term, state: $state) }"""

def execute(query: String, state: ThreeState) = {
val vars = Map(
"term" -> Some(StringValue("search")),
"state" -> (state match {
case ThreeState.Undefined => None
case ThreeState.Null => Some(NullValue)
case ThreeState.Value => Some(BooleanValue(false))
})
).collect { case (k, Some(v)) => k -> v }
api.interpreter.flatMap(_.execute(query, None, vars))
}

for {
undefined <- execute(query, ThreeState.Undefined)
nul <- execute(query, ThreeState.Null)
value <- execute(query, ThreeState.Value)
default <- execute(queryDefault, ThreeState.Undefined)
defaultValue <- execute(queryDefault, ThreeState.Value)
} yield assertTrue(undefined.data.toString == """{"getState":null}""") &&
assertTrue(nul.data.toString == """{"getState":false}""") &&
assertTrue(value.data.toString == """{"getState":true}""") &&
assertTrue(default.data.toString == """{"getState":false}""") &&
assertTrue(defaultValue.data.toString == """{"getState":true}""")
},
testM("field function") {
import io.circe.syntax._