whichdigital / active-rest-client

ActiveRestClient API Client
https://rubygems.org/gems/active_rest_client
MIT License
385 stars 44 forks source link

Add support for parallel requests #88

Closed nathanhoel closed 9 years ago

nathanhoel commented 9 years ago

I give example usage, then a fairly in depth description and also describe why I switched the response mock on all the specs. If the description is confusing just try reading through the code itself, I left comments where it seemed a bit counter intuitive.

Example Usage (except from README)

# Set adapter to Typhoeus to use parallel requests
ActiveRestClient::Base.adapter = :typhoeus

# Add result type for arrays (default to :object)
class Employer < ActiveRestClient::Base
    get :find, "/Employers/:id"
    get :all, "/Employers", :result_type => :array
end

conn = ActiveRestClient::ConnectionManager.get_connection('http://localhost:3000').session
conn.in_parallel do
    @person = Person.find(1234)
    @employer = Employer.find(10)

    puts @person.name #=> nil
    puts @employer.name #=> nil
end # The requests are all fired in parallel during this end statement

puts @person.name #=> "John"
puts @employer.name #=> "Good Food Inc."

Description Of Changes

The general idea of this change is to leverage Faraday's ability to do requests in parallel. In order to do this we have to be able to setup the request and use Faraday's callback on_complete method to trigger the remaining work to deserialize the JSON into a model once the request actually finishes. (https://github.com/nathanhoel/active-rest-client/blob/support-parallel-requests/lib/active_rest_client/request.rb#L153)

Having the work done during the callback is simple, the biggest hurdle is that we have to still return some result even though we have not actually handled any response. (https://github.com/nathanhoel/active-rest-client/blob/support-parallel-requests/lib/active_rest_client/request.rb#L179)

Whatever we return is initially empty but has to be populated with the data later. Since Ruby does not support pointers or replacing an object at a certain reference we have to create an empty object before the response is handled. To do this we need to know if we expect a JSON object or JSON array to create the @root_object as a new object or a new iterator respectively. (https://github.com/nathanhoel/active-rest-client/blob/support-parallel-requests/lib/active_rest_client/request.rb#L464)

When eventually the callback is called and we begin handling the response if it was an object we simply start adding the attributes to the @root_object. If it was marked as an array we start adding elements to the @root_object. (Handle Object - https://github.com/nathanhoel/active-rest-client/blob/support-parallel-requests/lib/active_rest_client/request.rb#L369) (Handle Array - https://github.com/nathanhoel/active-rest-client/blob/support-parallel-requests/lib/active_rest_client/request.rb#L503)

FaradayResponseProxy

class FaradayResponseProxy is a better proxy than just using OpenStruct. OpenStruct was basically mocking a Faraday response and was sufficient before this PR because ActiveRestClient did not use any of the other features of Faraday responses. Now that this PR leverages the on_complete callback in Faraday the specs need a Faraday response closer to what it actually is.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.2%) to 99.39% when pulling a461dd5186c71353de0b2a2f7b0cc02b5ab97f65 on nathanhoel:support-parallel-requests into 7098d097322c06130ca7fbbd75e9b1add1ab0a18 on whichdigital:master.

nathanhoel commented 9 years ago

So the :result_type is not needed UNLESS you want to make parallel requests. And even then it's only really needed for arrays because it just defaults to :object. I really wanted to implicitly discover this but we have to return the placeholder empty object before we actually send the request so we are not sure if its an array or object before hand. Long story short it is backwards compatible.

Also, you are right I could wrap the Faraday parallel stuff. I was just in the mindset of bridging the gap to get it so Faraday could do the rest. But it's actually fairly closely integrated at this point that we may as well combine those 2 lines as you suggested.

andyjeffries commented 9 years ago

I'd really like it implicitly discovered too, but I've had a quick look and it's not jumping at me how it would work.

The only way I can think is that - if my understanding is correct - as the parallel_to won't return until all requests are finished, in theory none of them will be used until after that point, so the object could be a LazyLoader type of object (for now I'll call it ParallelLoadedResponse). A very minimal proxy that just passes the method calls to the ResultIterator or instantiated object after it's ready. However, it does then mean that debugging could be a nightmare because EVERY object will be a ParallelLoadedResponse, making it harder to inspect and object and have it be obvious what it was.

I like the principle, but I'm not a fan of that specifying the response type mechanism (although I understand this is only if you want to use, I think it risks confusing users new to ARC).

nathanhoel commented 9 years ago

Exactly what you said is what I tried basically. There is a class called 'Simple Delegator' in Ruby that does that basically. It's strictly a passthrough to some object that you can set at a later time. However all objects become SimpleDelegators which would mess with any code that inspects object type. Also the simple delegator had problems with the method missing stuff because it uses method missing to pass on the calls. With our own object (such as ParallelLoadedResponse) we could possibly by pass that one issue. After trying it and see EVERYTHING as a Simple Delegator (or whatever object we used) it looked gross.

If Ruby had the ability to change what's at the end of a pointer it would be fine but alas this is not the world we chose to code for ;)

One of my peers at the company I work at had said explicitly specifying type should be the way to go anyway and I disagreed at first, I really like how it was implicit. However he was right that when you call an end point you pretty much always know whether you are handling an array or object anyway and start treating it that way anyway.

All in all, since it's new functionality asking people to add that one minor setup I feel like it's not too big of a hardship I hope!

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.12%) to 99.31% when pulling b345c15627df8b63a03d07d648bbfdf667c0b3c1 on nathanhoel:support-parallel-requests into 7098d097322c06130ca7fbbd75e9b1add1ab0a18 on whichdigital:master.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.12%) to 99.31% when pulling b345c15627df8b63a03d07d648bbfdf667c0b3c1 on nathanhoel:support-parallel-requests into 7098d097322c06130ca7fbbd75e9b1add1ab0a18 on whichdigital:master.

nathanhoel commented 9 years ago

Updated that method

ActiveRestClient::ConnectionManager.in_parallel('https://www.example.com') do
    # do stuff
end
coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.12%) to 99.31% when pulling aed5dcb3cdf497b854df08aa42fd518b5c6177ae on nathanhoel:support-parallel-requests into 7098d097322c06130ca7fbbd75e9b1add1ab0a18 on whichdigital:master.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.12%) to 99.31% when pulling aed5dcb3cdf497b854df08aa42fd518b5c6177ae on nathanhoel:support-parallel-requests into 7098d097322c06130ca7fbbd75e9b1add1ab0a18 on whichdigital:master.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.12%) to 99.31% when pulling aed5dcb3cdf497b854df08aa42fd518b5c6177ae on nathanhoel:support-parallel-requests into 7098d097322c06130ca7fbbd75e9b1add1ab0a18 on whichdigital:master.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.12%) to 99.31% when pulling aed5dcb3cdf497b854df08aa42fd518b5c6177ae on nathanhoel:support-parallel-requests into 7098d097322c06130ca7fbbd75e9b1add1ab0a18 on whichdigital:master.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.12%) to 99.31% when pulling aed5dcb3cdf497b854df08aa42fd518b5c6177ae on nathanhoel:support-parallel-requests into 7098d097322c06130ca7fbbd75e9b1add1ab0a18 on whichdigital:master.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.12%) to 99.31% when pulling aed5dcb3cdf497b854df08aa42fd518b5c6177ae on nathanhoel:support-parallel-requests into 7098d097322c06130ca7fbbd75e9b1add1ab0a18 on whichdigital:master.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.12%) to 99.31% when pulling aed5dcb3cdf497b854df08aa42fd518b5c6177ae on nathanhoel:support-parallel-requests into 7098d097322c06130ca7fbbd75e9b1add1ab0a18 on whichdigital:master.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.12%) to 99.31% when pulling aed5dcb3cdf497b854df08aa42fd518b5c6177ae on nathanhoel:support-parallel-requests into 7098d097322c06130ca7fbbd75e9b1add1ab0a18 on whichdigital:master.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.12%) to 99.31% when pulling aed5dcb3cdf497b854df08aa42fd518b5c6177ae on nathanhoel:support-parallel-requests into 7098d097322c06130ca7fbbd75e9b1add1ab0a18 on whichdigital:master.

nathanhoel commented 9 years ago

Sorry about all the coveralls, I had a few pushes but I think coveralls may have been duplicating them too. Weird.

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-0.05%) to 99.05% when pulling 4ccb0630a4edf7c4f2101918cabb72fecbf6635e on nathanhoel:support-parallel-requests into b8ca6977600e3b4917b0d9080f8ae1da65cac8ef on whichdigital:master.

nathanhoel commented 9 years ago

Ok, you got your wish @andyjeffries We have been using this in our staging for a while now and ran into a problem with the way I did it the first time. I am now using the ruby Delegate as I had mentioned before and have managed to work out almost all the kinks with that.

Just working out the last few things here in the next few commits.

andyjeffries commented 9 years ago

When it's all worked out I'll have a look.

I do disagree with your colleague's "However he was right that when you call an end point you pretty much always know whether you are handling an array or object anyway and start treating it that way anyway." I've worked with some APIs in the past where you can pass a single ID to get a hash back, or a comma-separated list of IDs to get back an array of hashes. As this is a general API client library, I don't want to have to say to anyone "sorry, you can't use that API with ARC".

Anyway, sounds like you have a solution, so I'll wait to see the final code - hopefully we both end up with what we need/want from ARC. Thanks for your efforts.

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-0.22%) to 98.89% when pulling f36106b351aff97ea55c55223c294ab4c5ca67fc on nathanhoel:support-parallel-requests into b8ca6977600e3b4917b0d9080f8ae1da65cac8ef on whichdigital:master.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.12%) to 99.23% when pulling 6088f6e11be1b39bf4f905aebfd02e41091a4680 on nathanhoel:support-parallel-requests into b8ca6977600e3b4917b0d9080f8ae1da65cac8ef on whichdigital:master.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.12%) to 99.23% when pulling 6088f6e11be1b39bf4f905aebfd02e41091a4680 on nathanhoel:support-parallel-requests into b8ca6977600e3b4917b0d9080f8ae1da65cac8ef on whichdigital:master.

nathanhoel commented 9 years ago

Yeah, I knew from the start it would be best if we didn't have to select the type of result. It wasn't looking like it was possible but after much testing it seems like we have the solution! I am done my changes/tests you can have another look now @andyjeffries, see if it's to your liking.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.04%) to 99.15% when pulling 034aa6c271502e848d85e49b733334d9a41e3be6 on nathanhoel:support-parallel-requests into b8ca6977600e3b4917b0d9080f8ae1da65cac8ef on whichdigital:master.

andyjeffries commented 9 years ago

Hey @nathanhoel,

I'm happy with the code and happy to merge it. I would say however that the documentation should be clearer around the use of typhoeus - you say "such as", whereas in fact you specifically require 'typhoeus' in the relevant portion of code. So it may cause support questions around what other options are available...

Thanks for putting the work in though, good feature addition.

Cheers,

Andy