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 function to support query string parameter in finch. #528

Merged
merged 3 commits into from
Aug 5, 2020

Conversation

keyno63
Copy link
Contributor

@keyno63 keyno63 commented Jul 30, 2020

I Implemented function (suport "query" query string as GET)
in the case of finch as details in #421

Paste the result of Test below.

  • curl GET query

$ curl  -X GET "http://localhost:8088/api/graphql?query=\{characters\{name\}\}" \
>   -H 'Host: localhost:8088' \
>   | jq
{
  "data": {
    "characters": [
      {
        "name": "James Holden"
      },
      {
        "name": "Naomi Nagata"
      },
      {
        "name": "Amos Burton"
      },
      {
        "name": "Alex Kamal"
      },
      {
        "name": "Chrisjen Avasarala"
      },
      {
        "name": "Josephus Miller"
      },
      {
        "name": "Roberta Draper"
      }
    ]
  },
  "extensions": {
<-- snip apolo tracing -->
  }
}

  • curl POST query

$ curl -X POST \
>  http://localhost:8088/api/graphql \
>   -H 'Host: localhost:8088' \
>   -H 'Content-Type: application/x-www-form-urlencoded' \
>   -d 'query={characters { name }}' \
>   | jq

{
  "data": {
    "characters": [
      {
        "name": "James Holden"
      },
      {
        "name": "Naomi Nagata"
      },
      {
        "name": "Amos Burton"
      },
      {
        "name": "Alex Kamal"
      },
      {
        "name": "Chrisjen Avasarala"
      },
      {
        "name": "Josephus Miller"
      },
      {
        "name": "Roberta Draper"
      }
    ]
  },
  "extensions": {
<-- snip apolo tracing -->
  }
}

@keyno63
Copy link
Contributor Author

keyno63 commented Jul 30, 2020

... CI is not running.

@keyno63 keyno63 changed the title #421. add function to support query string parameter in finch. add function to support query string parameter in finch. Jul 30, 2020
@ghostdogpr
Copy link
Owner

It looks OK in CircleCI: https://app.circleci.com/pipelines/github/keyno63/caliban/5/workflows/77213d26-d6ab-445e-b8a9-36dc7ac36bb4

(queryRequest: GraphQLRequest, body: Option[String], contentType: String) =>
val request: GraphQLRequest = (queryRequest, body, contentType) match {
case (_, Some(bodyValue), "application/json") =>
parse(bodyValue).flatMap(_.as[GraphQLRequest]).getOrElse(GraphQLRequest())
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm the problem here is that we lose the error message from parse which could be useful.
Maybe you could return a Task[GraphQLRequest] instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wrapping the result in Task and catching error when an error happens.

queryRequest
// treat unmatched content-type as same as None of body.
case _ =>
GraphQLRequest()
Copy link
Owner

Choose a reason for hiding this comment

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

If we return a Task[GraphQLRequest], this could fail with an exception e.g. "Query was not found".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe fixed this.

Copy link
Owner

@ghostdogpr ghostdogpr left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thank you~

@ghostdogpr ghostdogpr merged commit bfb858b into ghostdogpr:master Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants