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 multipart request support #65

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
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
12 changes: 4 additions & 8 deletions .rubocop_todo.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# This configuration was generated by
# `rubocop --auto-gen-config`
# on 2018-08-14 17:57:21 -0400 using RuboCop version 0.56.0.
# on 2019-10-31 13:05:23 +0200 using RuboCop version 0.56.0.
# The point is for the user to remove these configuration records
# one by one as the offenses are removed from the code base.
# Note that changes in the inspected code, or installation of new
Expand All @@ -15,11 +15,7 @@ Lint/UnusedMethodArgument:

# Offense count: 2
Metrics/CyclomaticComplexity:
Max: 8

# Offense count: 1
Metrics/PerceivedComplexity:
Max: 8
Max: 7

# Offense count: 1
# Cop supports --auto-correct.
Expand All @@ -35,13 +31,13 @@ Style/MethodMissingSuper:
- 'lib/graphlient/extensions/query.rb'
- 'lib/graphlient/query.rb'

# Offense count: 5
# Offense count: 7
Style/MultilineBlockChain:
Exclude:
- 'spec/graphlient/client_query_spec.rb'
- 'spec/graphlient/client_schema_spec.rb'

# Offense count: 39
# Offense count: 54
# Configuration parameters: AllowHeredoc, AllowURI, URISchemes, IgnoreCopDirectives, IgnoredPatterns.
# URISchemes: http, https
Metrics/LineLength:
Expand Down
15 changes: 15 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,21 @@ client.query(input: { fee_in_cents: 12_345 }) do
end
```

### Files support

You can send files while using `FaradayMultipartAdapter`.

```ruby
client = Graphlient::Client.new('example.com/graphql',
http: Graphlient::Adapters::HTTP::FaradayMultipartAdapter
)

file = File.read('example.txt')
client.mutation(input: file) # single file
client.mutation(input: [file]) # files as an array
client.mutation(input: [{ val: file }]) # files in nested hash
```

### Parse and Execute Queries Separately

You can `parse` and `execute` queries separately with optional variables. This is highly recommended as parsing a query and validating a query on every request adds performance overhead. Parsing queries early allows validation errors to be discovered before request time and avoids many potential security issues.
Expand Down
1 change: 1 addition & 0 deletions graphlient.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,5 @@ Gem::Specification.new do |s|
s.add_dependency 'faraday'
s.add_dependency 'faraday_middleware'
s.add_dependency 'graphql-client'
s.add_dependency 'mime-types'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Gems related to mime types are known to allocate excessive memory. Have you looked into the mini_mime gem?

Copy link
Author

Choose a reason for hiding this comment

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

Oh, I haven't thought about it.
Yup, mini_mime should work as expected too.

And what do you think if we try to load mime types lib from users environment and if he does not have any mime types library then we throw an expection and enforce user to install mime types lib with a message_No mime types library detected you must add to your Gemfile mime-types or mini_mime gem_?
Code could look something like that:

def mime_type(file)
  if defined?(MIME::TYPES) 
    MIME::TYPES.type_for(file.path)
  if defined?(MiniMyme)
     MiniMyme.lookup_by_filename(file.path)
  else
    raise Graphlient::Errors::NoMimeTypesLibError, 'Please add `mime-types` or `mini_mime` gem to your Gemfile'
  end
end

It would nice for Rails users because Rails already includes mime-types gem.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That sounds like a good idea 👍

end
1 change: 1 addition & 0 deletions lib/graphlient/adapters/http.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
require_relative 'http/adapter'
require_relative 'http/faraday_adapter'
require_relative 'http/faraday_multipart_adapter'
require_relative 'http/http_adapter'
42 changes: 42 additions & 0 deletions lib/graphlient/adapters/http/faraday_multipart_adapter.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
require 'faraday'
require 'faraday_middleware'

module Graphlient
module Adapters
module HTTP
class FaradayMultipartAdapter < Adapter
require 'graphlient/adapters/http/faraday_multipart_adapter/format_multipart_variables'

def execute(document:, operation_name:, variables:, context:)
response = connection.post do |req|
req.headers.merge!(context[:headers] || {})
req.body = {
query: document.to_query_string,
operationName: operation_name,
variables: FormatMultipartVariables.new(variables).call
}
end

response.body
rescue Faraday::ClientError => e
raise Graphlient::Errors::FaradayServerError, e
Copy link
Collaborator

@yuki24 yuki24 Oct 31, 2019

Choose a reason for hiding this comment

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

It doesn't seem right to translate client errors to server errors. Also, you don't have to pass in the e to the second argument as Ruby captures the cause and users have access to it with exception#cause.

Copy link
Author

Choose a reason for hiding this comment

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

oh, it was shamefully copy-pasted from FaradayAdapter
So I cannot tell why it does looks so.

end

def connection
@connection ||= Faraday.new(url: url, headers: headers) do |c|
c.use Faraday::Response::RaiseError
c.request :multipart
c.request :url_encoded
c.response :json

if block_given?
yield c
else
c.use Faraday::Adapter::NetHttp
end
end
end
end
end
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
require 'faraday'
require 'mime/types'
require 'graphlient/errors'

module Graphlient
module Adapters
module HTTP
class FaradayMultipartAdapter
class NoMimeTypeException < Graphlient::Errors::Error; end
# Converts deeply nested File instances to Faraday::UploadIO
class FormatMultipartVariables
def initialize(variables)
@variables = variables
end

def call
deep_transform_values(variables) do |variable|
variable_value(variable)
end
end

private

attr_reader :variables

def deep_transform_values(hash, &block)
return hash unless hash.is_a?(Hash)

transform_hash_values(hash) do |val|
if val.is_a?(Hash)
deep_transform_values(val, &block)
else
yield(val)
end
end
end

def variable_value(variable)
if variable.is_a?(Array)
variable.map { |it| variable_value(it) }
elsif variable.is_a?(Hash)
transform_hash_values(variable) do |value|
variable_value(value)
end
elsif variable.is_a?(File)
file_variable_value(variable)
else
variable
end
end

def file_variable_value(file)
content_type = MIME::Types.type_for(file.path).first
return Faraday::UploadIO.new(file.path, content_type) if content_type
Copy link
Owner

Choose a reason for hiding this comment

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

should we use Faraday::FilePart or Faraday::ParamPart based on this doc?

Copy link
Author

@pucinsk pucinsk Nov 6, 2019

Choose a reason for hiding this comment

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

Mhm. Good point. Faraday::UploadIO will be deprecated. 😞 But in Faraday v0.17.0 UploadIO is only valid way to add files to request.
I think the next Faraday version will v1.0 and it will have Faraday::Parts module where all parts of request will be described.
Maybe we should specify faraday gem version in Gemfile just to make sure that for now Faraday::UploadIO could work?


raise NoMimeTypeException, "Unable to determine mime type for #{file.path}"
end

def transform_hash_values(hash)
hash.each_with_object({}) do |(key, value), result|
result[key] = yield(value)
end
end
end
end
end
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
# frozen_string_literal: true

require 'spec_helper'
require 'graphlient/adapters/http/faraday_multipart_adapter/format_multipart_variables'

RSpec.describe Graphlient::Adapters::HTTP::FaradayMultipartAdapter::FormatMultipartVariables do
subject(:format_multipart_variables) { described_class.new(variables) }

describe '#call' do
subject(:call) { format_multipart_variables.call }

context 'when file does not have mime type' do
let(:variables) { { val: { file: File.new('/dev/null') } } }

it 'raises an error' do
expect { call }.to raise_error(Graphlient::Adapters::HTTP::FaradayMultipartAdapter::NoMimeTypeException)
end
end

context 'when variable is not a file' do
let(:variables) { { val: { name: 'John Doe' } } }

it 'returns correct value' do
expect(call).to eq(variables)
end
end

context 'when file is deeply nested' do
let(:variables) { { val: { file: File.new('spec/support/fixtures/empty.txt') } } }

it 'contverts file to Faraday::UploadIO' do
expect(call[:val][:file]).to be_a(Faraday::UploadIO)
Copy link
Collaborator

@yuki24 yuki24 Oct 31, 2019

Choose a reason for hiding this comment

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

In Ruby, duck typing is more preferable rather than checking classes.

end
end

context 'when files are in array' do
let(:variables) do
{
val: [
File.new('spec/support/fixtures/empty.txt'),
File.new('spec/support/fixtures/empty.txt')
]
}
end

it 'contverts file to Faraday::UploadIO' do
expect(call[:val]).to all be_a(Faraday::UploadIO)
end
end

context 'when file is in array and then nested' do
let(:variables) do
{
val: [
{ file: File.new('spec/support/fixtures/empty.txt') },
{ file: File.new('spec/support/fixtures/empty.txt') }
]
}
end

it 'converts file to Faraday::UploadIO' do
result = call[:val].map { |val| val[:file] }
expect(result).to all be_a(Faraday::UploadIO)
end
end
end
end
61 changes: 61 additions & 0 deletions spec/graphlient/adapters/http/faraday_multipart_adapter_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
require 'spec_helper'

describe Graphlient::Adapters::HTTP::FaradayMultipartAdapter do
let(:app) { Object.new }

context 'with a custom middleware' do
let(:client) do
Graphlient::Client.new('http://example.com/graphql', http: described_class) do |client|
client.http do |h|
h.connection do |c|
c.use Faraday::Adapter::Rack, app
end
end
end
end

it 'inserts a middleware into the connection' do
expect(client.http.connection.builder.handlers).to eq(
[
Faraday::Response::RaiseError,
Faraday::Request::Multipart,
Faraday::Request::UrlEncoded,
FaradayMiddleware::ParseJson,
Faraday::Adapter::Rack
]
)
end
end

context 'with custom url and headers' do
let(:url) { 'http://example.com/graphql' }
let(:headers) { { 'Foo' => 'bar' } }
let(:client) do
Graphlient::Client.new(url, headers: headers, http: described_class)
end

it 'sets url' do
expect(client.http.url).to eq url
end

it 'sets headers' do
expect(client.http.headers).to eq headers
end
end

context 'default' do
let(:url) { 'http://example.com/graphql' }
let(:client) { Graphlient::Client.new(url, http: described_class) }

before do
stub_request(:post, url).to_return(
status: 200,
body: DummySchema.execute(GraphQL::Introspection::INTROSPECTION_QUERY).to_json
)
end

it 'retrieves schema' do
expect(client.schema).to be_a Graphlient::Schema
end
end
end
Empty file added spec/support/fixtures/empty.txt
Empty file.