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

Add param to GraphQLRequest to identify GET requests #2329

Merged
merged 5 commits into from
Sep 20, 2024
Merged
Show file tree
Hide file tree
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
Original file line number Diff line number Diff line change
Expand Up @@ -168,10 +168,8 @@ final private class QuickRequestHandler[R](

private def executeRequest(method: Method, req: GraphQLRequest)(implicit
trace: Trace
): ZIO[R, Response, GraphQLResponse[Any]] = {
val calibanMethod = if (method == Method.GET) HttpRequestMethod.GET else HttpRequestMethod.POST
HttpRequestMethod.setWith(calibanMethod)(interpreter.executeRequest(req))
}
): ZIO[R, Response, GraphQLResponse[Any]] =
interpreter.executeRequest(if (method == Method.GET) req.asHttpGetRequest else req)

private def responseHeaders(headers: Headers, cacheDirective: Option[String]): Headers =
cacheDirective match {
Expand Down Expand Up @@ -204,7 +202,7 @@ final private class QuickRequestHandler[R](
encodeSingleResponse(resp, keepDataOnErrors = !isBadRequest, hasCacheDirective = cacheDirective.isDefined)
)
case resp =>
val isBadRequest = resp.errors.contains(HttpRequestMethod.MutationOverGetError)
val isBadRequest = resp.errors.contains(HttpUtils.MutationOverGetError)
Response(
status = if (isBadRequest) Status.BadRequest else Status.Ok,
headers = responseHeaders(ContentTypeJson, cacheDirective),
Expand Down
4 changes: 3 additions & 1 deletion build.sbt
Original file line number Diff line number Diff line change
Expand Up @@ -734,7 +734,9 @@ lazy val enableMimaSettingsJVM =
mimaPreviousArtifacts := previousStableVersion.value.map(organization.value %% moduleName.value % _).toSet,
mimaBinaryIssueFilters := Seq(
ProblemFilters.exclude[DirectMissingMethodProblem]("caliban.quick.*"),
ProblemFilters.exclude[DirectMissingMethodProblem]("caliban.QuickAdapter.*")
ProblemFilters.exclude[DirectMissingMethodProblem]("caliban.QuickAdapter.*"),
ProblemFilters.exclude[DirectMissingMethodProblem]("caliban.GraphQLRequest.*"),
ProblemFilters.exclude[Problem]("caliban.HttpRequestMethod*")
)
)

Expand Down
27 changes: 13 additions & 14 deletions core/src/main/scala/caliban/GraphQL.scala
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ trait GraphQL[-R] { self =>
(for {
doc <- wrap(parseZIO)(parsingWrappers, request.query.getOrElse(""))
coercedVars <- coerceVariables(doc, request.variables.getOrElse(Map.empty))
executionReq <- wrap(validation(request.operationName, coercedVars))(validationWrappers, doc)
executionReq <- wrap(validation(request, coercedVars))(validationWrappers, doc)
result <- wrap(execution(schemaToExecute(doc), fieldWrappers))(executionWrappers, executionReq)
} yield result).catchAll(Executor.fail)
)(overallWrappers, request)
Expand All @@ -138,20 +138,20 @@ trait GraphQL[-R] { self =>
}

private def validation(
operationName: Option[String],
req: GraphQLRequest,
coercedVars: Map[String, InputValue]
)(doc: Document)(implicit trace: Trace): IO[ValidationError, ExecutionRequest] =
Configurator.ref.getWith { config =>
Validator.prepare(
doc,
typeToValidate(doc),
operationName,
req.operationName,
coercedVars,
config.skipValidation,
config.validations
) match {
case Right(value) => checkHttpMethod(config)(value)
case Left(error) => ZIO.fail(error)
case Right(value) => checkHttpMethod(config)(req, value)
case Left(error) => Exit.fail(error)
}
}

Expand Down Expand Up @@ -183,16 +183,15 @@ trait GraphQL[-R] { self =>
private def schemaToExecute(doc: Document) =
if (doc.isIntrospection) introspectionRootSchema else schema

private def checkHttpMethod(cfg: ExecutionConfiguration)(req: ExecutionRequest)(implicit
trace: Trace
): IO[ValidationError, ExecutionRequest] =
if ((req.operationType eq OperationType.Mutation) && !cfg.allowMutationsOverGetRequests)
HttpRequestMethod.getWith {
case HttpRequestMethod.GET => ZIO.fail(HttpRequestMethod.MutationOverGetError)
case _ => Exit.succeed(req)
}
private def checkHttpMethod(
cfg: ExecutionConfiguration
)(gqlReq: GraphQLRequest, req: ExecutionRequest): IO[ValidationError, ExecutionRequest] =
if (
req.operationType == OperationType.Mutation &&
!cfg.allowMutationsOverGetRequests &&
gqlReq.isHttpGetRequest
) Exit.fail(HttpUtils.MutationOverGetError)
else Exit.succeed(req)

}
}

Expand Down
9 changes: 6 additions & 3 deletions core/src/main/scala/caliban/GraphQLRequest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,21 @@ case class GraphQLRequest(
query: Option[String] = None,
operationName: Option[String] = None,
variables: Option[Map[String, InputValue]] = None,
extensions: Option[Map[String, InputValue]] = None
) {
extensions: Option[Map[String, InputValue]] = None,
@transient isHttpGetRequest: Boolean = false
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does this do in the context of a case class member?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This one comes from jsoniter (it's not the Scala one). It excludes the case class member from serialization and always uses the default value for deserialization

) { self =>

def withExtension(key: String, value: InputValue): GraphQLRequest =
copy(extensions = Some(extensions.foldLeft(Map(key -> value))(_ ++ _)))

def withFederatedTracing: GraphQLRequest =
withExtension(`apollo-federation-include-trace`, StringValue(ftv1))

private[caliban] def asHttpGetRequest: GraphQLRequest =
new GraphQLRequest(query, operationName, variables, extensions, isHttpGetRequest = true)

private[caliban] def isEmpty: Boolean =
operationName.isEmpty && query.isEmpty && extensions.isEmpty

}

object GraphQLRequest {
Expand Down
25 changes: 0 additions & 25 deletions core/src/main/scala/caliban/HttpRequestMethod.scala

This file was deleted.

4 changes: 4 additions & 0 deletions core/src/main/scala/caliban/HttpUtils.scala
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package caliban

import caliban.CalibanError.ValidationError
import caliban.ResponseValue.{ ObjectValue, StreamValue }
import caliban.Value.NullValue
import caliban.wrappers.Caching
Expand All @@ -8,6 +9,9 @@ import zio.{ Cause, Chunk, Trace }

private[caliban] object HttpUtils {

val MutationOverGetError: ValidationError =
ValidationError("Mutations are not allowed for GET requests", "")

object DeferMultipart {
private val Newline = "\r\n"
private val ContentType = "Content-Type: application/json; charset=utf-8"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,14 @@ object GraphQLRequestJsoniterSpec extends ZIOSpecDefault {
writeToString(res) ==
"""{"query":"{}","operationName":"op","variables":{"hello":"world","answer":42,"isAwesome":true,"name":null}}"""
)
},
test("isHttpGetRequest is ignored when serializing to JSON") {
val res = writeToString(GraphQLRequest(isHttpGetRequest = true))
assertTrue(res == """{}""")
},
test("isHttpGetRequest is ignored when deserializing from JSON") {
val res = readFromString[GraphQLRequest]("""{"isHttpGetRequest":true}""").isHttpGetRequest
assertTrue(!res)
}
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -73,16 +73,10 @@ object HttpInterpreter {
def executeRequest[BS](
graphQLRequest: GraphQLRequest,
serverRequest: ServerRequest
)(implicit streamConstructor: StreamConstructor[BS]): ZIO[R, TapirResponse, CalibanResponse[BS]] =
setRequestMethod(serverRequest)(interpreter.executeRequest(graphQLRequest))
.map(buildHttpResponse[E, BS](serverRequest))

private def setRequestMethod(req: ServerRequest)(exec: URIO[R, GraphQLResponse[E]]): URIO[R, GraphQLResponse[E]] =
req.method match {
case Method.POST => HttpRequestMethod.setWith(HttpRequestMethod.POST)(exec)
case Method.GET => HttpRequestMethod.setWith(HttpRequestMethod.GET)(exec)
case _ => exec
}
)(implicit streamConstructor: StreamConstructor[BS]): ZIO[R, TapirResponse, CalibanResponse[BS]] = {
val req = if (serverRequest.method == Method.GET) graphQLRequest.asHttpGetRequest else graphQLRequest
interpreter.executeRequest(req).map(buildHttpResponse[E, BS](serverRequest))
}
}

private case class Prepended[R, E](
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ object TapirAdapter {
encodeTextEventStreamResponse(resp)
)
case resp =>
val isBadRequest = response.errors.contains(HttpRequestMethod.MutationOverGetError: Any)
val isBadRequest = response.errors.contains(HttpUtils.MutationOverGetError: Any)
(
MediaType.ApplicationJson,
if (isBadRequest) StatusCode.BadRequest else StatusCode.Ok,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,12 @@ object TapirAdapterSpec {
ZIO
.serviceWithZIO[SttpBackend[Task, Capabilities]](run(GraphQLRequest(None)).send(_))
.map(r => assertTrue(r.code.code == 400))
},
test("returns 400 for mutations over GET methods") {
runHttpRequest(
method = Method.GET.method,
query = """mutation{ deleteCharacter(name: "Amos Burton") }"""
).map(r => assertTrue(r.code.code == 400))
}
)
),
Expand Down
Loading