Use Faraday instead of Patron #88

Closed
wants to merge 17 commits into
from

9 participants

@chytreg

Hi,

I did quite big rewrite of client.rb to use Faraday library.
This give us all benefits of Faraday gem https://github.com/lostisland/faraday
The client.rb is much cleaner retry and json parsing are as the middleware.
I change default json pare to yail-ruby because is much quicker and has seamless integration flow.

We can use for example typhoeus adapater:

require 'parse-ruby-client'
require 'typhoeus'
require 'typhoeus/adapters/faraday'

Faraday.default_adapter = :typhoeus
Parse.init {:application_id => "<your_app_id>", :api_key => "<your_api_key>"} do |faraday|
  # here you can pass some additionals to faraday directly
end
@adelevie
Owner

This looks really, really great. I'm just curious as to what the good use-cases for Faraday are? I'm probably going to merge this anyway, but I'm curious.

Also, would be interested to hear from @ericcj on this (and anyone else!).

@chytreg

Faraday is an abstract layer for http adapters, so you use the same interface, but easily change http adapter. Next is based on middlewares so you can move the processing of http requests form main part of you application, and gain much cleaner code. Imho the middleware support is the real power of Faraday.

@ericcj ericcj commented on the diff Apr 29, 2013
test/test_query.rb
- foo = Parse::Object.new "Post"
- foo["random"] = rand
- foo.save
- foo_query = Parse::Query.new("Post").eq("random", foo["random"])
- assert_equal 1, foo_query.get.size
-
- bar = Parse::Object.new "Post"
- bar["random"] = rand
- bar.save
- bar_query = Parse::Query.new("Post").eq("random", bar["random"])
- assert_equal 1, foo_query.get.size
-
- query = foo_query.or(bar_query)
- assert_equal 2, query.get.size
- #end
+ foo = Parse::Object.new "Post"
@ericcj
ericcj added a line comment Apr 29, 2013

was vcr disabled in this one intentionally? it seems like it would work

@chytreg
chytreg added a line comment Apr 29, 2013

Yes because it did't work :/ I think it's because of "random query"
The same is for test_user.rb.

@chytreg
chytreg added a line comment Apr 29, 2013

It's easy to fix, foo["random"] = 'blah' and for the next one bar["random"] = 'meow' instead of using rand

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ericcj ericcj commented on the diff Apr 29, 2013
lib/faraday/extended_parse_json.rb
@@ -0,0 +1,35 @@
+# -*- encoding : utf-8 -*-
+module Faraday
+
+ class ExtendedParseJson < FaradayMiddleware::ParseJson
+
+ def process_response(env)
+ env[:raw_body] = env[:body] if preserve_raw?(env)
+ data = parse(env[:body]) || {}
@ericcj
ericcj added a line comment Apr 29, 2013

does this raise on invalid json? it's important that the existing retry on invalid json behavior be preserved but i can't tell if it is or not here since that test was refactored away.

@chytreg
chytreg added a line comment Apr 29, 2013

It will raise 'Faraday::Error::ParsingError' and will retry for that exception
https://github.com/adelevie/parse-ruby-client/pull/88/files#L17R38

@ericcj
ericcj added a line comment Apr 30, 2013

unfortunately, the parse api returns non-json responses for both client and server error types, so the "&& response.status >= 500" condition on retrying json parse errors in the original code was necessary to prevent retrying things like 404's

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ericcj ericcj commented on the diff Apr 29, 2013
lib/parse/client.rb
@@ -157,71 +70,73 @@ def delete(uri)
end
protected
-
- def log_retry(e, uri, query, body, response)
- logger.warn{"Retrying Parse Error #{e.inspect} on request #{uri} #{CGI.unescape(query.inspect)} #{body.inspect} response #{response.inspect}"}
@ericcj
ericcj added a line comment Apr 29, 2013

we should maintain the existing behavior of logging retried errors. perhaps faraday provides that?

@chytreg
chytreg added a line comment Apr 29, 2013

There is a global Faraday logger set here
https://github.com/adelevie/parse-ruby-client/pull/88/files#L17R34

Not sure you get the right information in the log. Will check this, and if not I will add the custom warning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ericcj

thanks for this pull, it's definitely what i was looking for us to do with client.rb. ship it after my couple concerns

@ericcj

@chytreg would love to see this get merged. will you have a chance to look over those last couple remaining comments? thanks again

@adelevie
Owner

So what are the main issues here that need to be fixed?

@ericcj

mostly the retry logic, and also would be nice to keep the logging on retries

@chytreg
@ericcj

thanks! if you don't get to it we will eventually

@ericcj ericcj referenced this pull request Sep 3, 2013
Closed

Multi Threading | Sidekiq #104

@kwent

Should be nice to merge to master !

@bricker

👍 on this one, would love to see this merged in.

@kenn

Just failed to deploy because patron failed to compile as we don't have curl installed on the server.

And installing libcurl4-openssl-dev has so many dependencies on Debian Wheezy:

comerr-dev krb5-multidev libcurl4-openssl-dev libgcrypt11-dev libgnutls-dev libgnutls-openssl27 libgnutlsxx27 libgpg-error-dev libgssrpc4 libidn11-dev libkadm5clnt-mit8 libkadm5srv-mit8 libkdb5-6 libkrb5-dev libldap2-dev libp11-kit-dev librtmp-dev libssh2-1-dev libtasn1-3-dev

👍 on this PR

@rhymes
Collaborator

Faraday is the library now used to issue HTTP calls. This should be closed.

@xavdid xavdid closed this Jun 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment