Skip to content

Accept a string of JSON for variables in query initialisation #32

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

Closed
wants to merge 1 commit into from

Conversation

sgwilym
Copy link
Contributor

@sgwilym sgwilym commented Sep 4, 2015

While working with some mutations, I realised that when you have a mutation that sends a file, the Content-type of the request sent by Relay will no longer be application/json (https://github.com/facebook/relay/blob/master/src/network-layer/default/RelayDefaultNetworkLayer.js#L113) What that means is that any query variables will be interpreted as a String rather than a Hash (at least this is the case in Rails).

The final effect is that your query variables are invisibly swallowed and your query variables are no longer available to your resolve methods. This PR fixes that.

@sgwilym sgwilym force-pushed the variables-json-string branch from d7d1c50 to e9f634c Compare September 4, 2015 09:17
@rmosolgo
Copy link
Owner

rmosolgo commented Sep 4, 2015

Hmmm to me this just feels like a controller issue, not a GraphQL implementation issue. I think it would make a nice private method in a controller:

  def create 
    # ...
    variables = ensure_hash(params[:variables])
    GraphQL::Query.new(Schema, query, variables: variables)
  end

  private 

  # If the request wasn't `Content-Type: application/json`, parse the variables:
  def ensure_hash(variables_param)
    variables_param.kind_of?(Hash) ? variables_param : JSON.parse(variables_param)
  end

I prefer that because it keeps the GraphQL API simple and it keeps HTTP request details in the HTTP handler, not in GraphQL::Query. What do you think?

@sgwilym
Copy link
Contributor Author

sgwilym commented Sep 4, 2015

I had the same thought in the back of my mind this afternoon — you're absolutely right!

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.

2 participants