Add ability to query builder/connection to see if they contain an adapter.

[avdi]

I want this in Leadlight so I can optionally add a default adapter if there isn't one already.

[travisbot]

This pull request fails (merged 365a57a into ffa01e2).

[mislav]

1. I don't understand the use case. Which are those stacks that don't have an adapter?
2. Why wasn't the shebang portable?
3. Now everyone who creates middleware has to implement self.adapter?, otherwise builder will crash & burn.

[avdi]

On Tue, Jun 19, 2012 at 5:54 PM, Mislav Marohnić <
reply@reply.github.com

wrote:

1. I don't understand the use case. Which are those stacks that don't have
   an adapter?

Ones that I'm building in Leadlight.

1. Why wasn't the shebang portable?

Because env doesn't permit more than one argument on most unixen. Read the
link I provided.

1. Now everyone who creates middleware has to implement self.adapter?,
   otherwise builder will crash & burn.

I deliberately set things up so that wouldn't be the case. What did I miss?

[mislav]

1. If you're building them, then you know if they have or don't have an adapter. You can see why I asked.
2. Gotcha. Most my shebangs are wrong, then.
3. You missed the fact that middleware only has to respond to #call and doesn't have to inherit Faraday::Middleware.

[avdi]

On Tue, Jun 19, 2012 at 5:54 PM, Mislav Marohnić <
reply@reply.github.com

wrote:

1. I don't understand the use case. Which are those stacks that don't
   have an adapter?

To be more specific:

Leadlight enables client code to provide its own middleware stack. If the
client has a particular adapter they want to use, I don't want to jam
another adapter on the stack. OTOH, if they only want to stick a few
middlewares in, I want to make sure it gets a default adapter.

As far as I can tell I can't rely on Faraday for this because it only adds
default adapter if no middlewares whatsoever are supplied. Which seems like
a fine minimal heuristic out of the box, it's just insufficient for my
purposes.

[avdi]

On Tue, Jun 19, 2012 at 6:01 PM, Mislav Marohnić <
reply@reply.github.com

wrote:

1. If you're building them, then you know if they have or don't have an
   adapter. You can see why I asked.

See my last reply. I'm not building them; I'm providing a hook for
third-party code to add their own.

1. You missed the fact that middleware only has to respond to #call and
   doesn't have to inherit Faraday::Middleware.

This is the code I added to Handler:

      def adapter?
        klass.respond_to?(:adapter?) && klass.adapter?
      end

Is that not sufficient?

[avdi]

To make things a little more concrete, here's a sample of a real Leadlight service definition:

  class GithubService
    Leadlight.build_service(self) do
      url 'https://api.github.com'

      build_connection do |c|
        c.adapter :typhoeus
      end

      tint 'root' do
        match_path('/')
        add_link_template '/users/{login}', 'user', 'Find user by login'
      end

      tint 'auth_scopes' do
        extend do
          def oauth_scopes
            __response__.headers['X-OAuth-Scopes'].to_s.strip.split(/\W+/)
          end
        end
      end
    end

    def prepare_request(request)
      request.headers['Authorization'] = "Bearer #{options[:oauth2_token]}"
    end
  end

The important bit is the build_connection part. In this case, the service definition specifies a particular adapter. Other users might just want to add some middlewares and rely on the configured global Leadlight default adapter. I want to support both possibilities.

[mislav]

Whoops, missed the respond_to call. I thought you were querying a middleware class directly. Sorry.

I'm giving you a hard time, but not explaining why.

Basically, I'm not happy with the concept of adapters we have now, and I want to change this for the next release. So I'm not eager to make changes that just tweak the current situation—I want to radically rethink the whole thing, as per #47.

Adapters are middleware right now, but that's just wrong:

1. Adapters should come last and nothing should come after them. Lots of people new to Faraday trip on this (i.e. they put it in the middle or even the beginning).
2. You should be able to not specify the adapter to let the default one be used. Lots of people try this and get surprised that their stack is a dead end now (requests don't go anywhere).

My idea is to redesign Faraday::Connection to start with default adapter, offer you to customize the middleware stack, but if you don't specify an explicit adapter, it will just keep using the default one. Also, adapters would then be "Rack endpoints", not middleware. This is a significant change, but would be perfectly backwards compatible with any userland code.

This would solve your problem since you could then initialize a Connection with an adapter of your choice, and let the user customize their middleware stack, but they can choose to leave out the adapter.

[avdi]

One final clarification: when I say that client code can add its own middleware stack, I mean that it can augment the stack. Leadlight has some middlewares that it will always add regardless of what the client code adds.

[avdi]

FWIW, I completely agree with this long-term direction. Having adapters as middlewares is confusing and error-prone.

I feel like most of this changeset is compatible with your rewrite: it seems to me that it would still make sense to ask an connection if its adapter slot has been filled. The only difference would be that there'd no longer be a need to iterate over the middleware list to answer that question.

Thoughts?

[mislav]

There won't be a need to ask a connection if it has an adapter, since it won't be possible to have a connection without an adapter. So, this changeset doesn't actually help with this goal :(

Leadlight has some middlewares that it will always add regardless of what the client code adds

How are you adding this middleware? I'm interested in how 3rd party libs allow users to customize the stack, while still forcing middleware of their own. It can be tricky/brittle, so I'm wondering if there's something we can improve about this. (This is actually not much related to our original discussion.)

[avdi]

So a connection without an adapter will be invalid?

[mislav]

You won't be able to have a connection without an adapter. At instantiation, it would inherit Faraday.default_middleware. While building the middleware stack, you can override adapter the usual way. Even after the stack was setup, you will be able to change adapter with Connection#adapter method. You won't be able to unset it, naturally, since a Connection without an adapter is useless.

[travisbot]

This pull request passes (merged 7a0cb13 into ffa01e2).

[avdi]

Try the above commit on for size. It's an attempt at adding an API that separates the adapter from the rest of the stack, without actually changing anything internally (yet) or introducing backwards-incompatibilities.

See the full commit message for details.

[mislav]

Interesting. I'll look at this again when it's not 2am, let you know what I think

I have a feeling that it can be done without so many changes to Builder, but I might be wrong (theory is always easier than code)

[avdi]

As an aside, I was thinking as I worked on this that a Builder is a fundamentally separate concept from the thing being built (a handler stack). Would you be interested in a commit that separates the two?

[mislav]

This separation would be correct, theoretically, but I'm not sure it would benefit us much. Builder isn't an area where we spend a significant amount of development time, or plan to spend for that matter.

My idea: remove the concept of an adapter from Builder / middleware stack. As I said, adapters will be endpoints not middleware. This change would be breaking for anyone calling #adapter on a builder instance but most people are calling #adapter on connection since that's what's yielded from Faraday::Connection.new.

Adapters would be handled by Connection here: https://github.com/technoweenie/faraday/blob/master/lib/faraday/connection.rb#L117-127

That also means this awkward lambda with Response handling will go away, yay (will be done inside each individual middleware)

[myronmarston]

Interesting discussion. I've dealt with some of the same issues in my usage of Faraday, so I'll chime in a bit here :).

Adapters should come last and nothing should come after them. Lots of people new to Faraday trip on this (i.e. they put it in the middle or even the beginning).

I've seen this in the wild, too--it was actually the source of a VCR bug I had to track down--the instagram gem put a middleware after an adapter and it screwed things up for VCR's faraday middleware.

How are you adding this middleware? I'm interested in how 3rd party libs allow users to customize the stack, while still forcing middleware of their own. It can be tricky/brittle, so I'm wondering if there's something we can improve about this. (This is actually not much related to our original discussion.)

I've done this a couple of ways:

• In VCR, I've got some code that hooks into Faraday::Builder#lock! in order to inject VCR's faraday middleware into every faraday connection.
• At work we have an internal client gem I wrote for one of our internal services. It's built on Faraday and allows users of the client to reconfigure the faraday stack using the builder API. Here's a gist showing the code for it, and here's how it can be used:

Silo::Client.configure do |config|
  config.middleware do |builder|
    builder.insert 0, MyCoolMiddleware
  end
end

[avdi]

On Wed, Jun 20, 2012 at 12:09 AM, Myron Marston <
reply@reply.github.com

wrote:

I've seen this in the wild, too--it was actually the source of a VCR bug I had to track down--the
instagram gem put a middleware after an adapter and it screwed things up
for VCR's faraday middleware.

Hah, yeah a lot of this stemmed from going through and eliminating your
(very helpful!) warnings about that in VCR :-)

[iMacTia]

Hello everyone,

I can see this PR have been open for quite a long time.
Code would need changes to be compatible with latest version, but now that we're approaching v1.0 (#620) we already decided to address this issue properly.

I'm sorry to close this after having it open for so long, all this discussion will be useful to decide how to address this issue the right way in v1.0