Guard against invalid middleware configuration?

[odlp]

I glossed over this important passage about Advanced middleware usage (emphasis mine):

The order in which middleware is stacked is important. Like with Rack, the first middleware on the list wraps all others, while the last middleware is the innermost one, so that must be the adapter.

This pull request proposes a guard against invalid / incorrect builder usage.

# Incorrect usage
Faraday.new do |b|
  b.adapter :test
  b.request :multipart
end
# => Faraday::ConfigurationError: unexpected middleware set after the adapter

# Correct
Faraday.new do |b|
  b.request :multipart
  b.adapter :test
end
# => #<Faraday::Connection>

I hope this PR will save some other users who didn't fully read the instructions at first, like me, some head-scratching.

[odlp]

I touched this test while adding the guard feature as the test started failing with the new verification.

Originally, using the build_stack helper, the order of the middlewares in this test ended-up being:

Apple
:test (adapter)
Orange

which I assume as being invalid according to the README. I figure this is tangental to the original intent of the test itself, which was to verify the builder is mutable, so I re-wrote the test to avoid the failure.

[iMacTia]

Yes this test probably overlooked the fact that build_stack always adds the adapter at the end, going against the README itself.
Your change works but I see that build_stack and build_handlers_stack are practically the same thing (and the latter accepts a block but doesn't use it).
Another, better in my opinion, approach would be to add an optional parameter to build_stack to skip the adapter insertion at the end. This should reduce code duplication even further.

[odlp]

I did look to parameterize build_stack but the splat argument makes this awkward. If we were targeting Ruby >= 2.0 we could use keyword arguments like so:

def build_stack(*handlers, without_adapter: true)

But this fails on Ruby 1.9.3. I'll find another solution to reduce the duplication.

[iMacTia]

Ah you're right, that makes it a little more difficult.
ActiveSupport comes with this handy extension:

class Hash
  # By default, only instances of Hash itself are extractable.
  # Subclasses of Hash may implement this method and return
  # true to declare themselves as extractable. If a Hash
  # is extractable, Array#extract_options! pops it from
  # the Array when it is the last element of the Array.
  def extractable_options?
    instance_of?(Hash)
  end
end

class Array
  # Extracts options from a set of arguments. Removes and returns the last
  # element in the array if it's a hash, otherwise returns a blank hash.
  #
  #   def options(*args)
  #     args.extract_options!
  #   end
  #
  #   options(1, 2)        # => {}
  #   options(1, 2, a: :b) # => {:a=>:b}
  def extract_options!
    if last.is_a?(Hash) && last.extractable_options?
      pop
    else
      {}
    end
  end
end

but you'll probably need to add it as it's not one of the gem dependencies.
At this point then having another method like you did is probably better, just check if you can reduce the duplication even further

[iMacTia]

Hi @odlp and thanks for your PR.
This actually addresses an annoying issue for many that we've been discussing a lot over time.
However, there are 2 issues with this approach:

• It doesn't work with all cases (see Automatically set @handler to Faraday.default_adapter if left unspecified. #121) as you're assuming every adapter inherits from Faraday::Adapter and is pushed in the stack using the adapter method
• It's not backward compatible, and this is not acceptable for any release < 1.0

These are the main reasons I've been pushing back changes like this and postponed them to Faraday v1.0.

[odlp]

Thanks for the quick reply @iMacTia. I completely support maintaining backwards compatibility & waiting for a 1.0 release!

It doesn't work with all cases [...] is pushed in the stack using the adapter method

I think this currently covers adding an adapter via #adapter or #use, but doesn't cover invalid configuration via #insert, #insert_after, #swap, #delete etc. I can update the PR to raise an exception if the adapter, when present, is no longer in last position, post-modification of @handlers?

you're assuming every adapter inherits from Faraday::Adapter

Could you elaborate when an adapter wouldn't inherit from Faraday::Adapter? Are you thinking of non-library adapters from third-parties? If so, perhaps there's a public method in the interface of an adapter-esque duck can be detected.

[iMacTia]

Can you please explain this unless is_adapter?(klass)?
Does that mean that I'm allowed to insert an adapter after another adapter?

[odlp]

I was unsure whether adapter overwrites should be allowed, so I erred on the side of it being permissive:
https://github.com/lostisland/faraday/pull/685/files#diff-39d210c7f4f77538e42199f60ce1d2f8R151

But I can see how it doesn't make sense, because #adapter= twice means the stack has two adapters. Ideally setting that adapter again should be an overwrite (if permitted), not an additional adapter in the stack. This is a different kind of misconfiguration really.

[odlp]

For the moment I can remove unless is_adapter?(klass) and the associated test

[iMacTia]

Yep that's fine.
Leaving it there will cause multiple adapters to be inserted as you said, which is definitely something we don't want to happen 😃

[iMacTia]

I think this currently covers adding an adapter via #adapter or #use, but doesn't cover invalid configuration via #insert, #insert_after, #swap, #delete etc. I can update the PR to raise an exception if the adapter, when present, is no longer in last position, post-modification of @handlers?

Yes so you're currently covering both adapter and use methods, but not all the index-based methods. It should be enough to check @handlers in the insert method though as all other methods are implemented on top of it.

Could you elaborate when an adapter wouldn't inherit from Faraday::Adapter? Are you thinking of non-library adapters from third-parties? If so, perhaps there's a public method in the interface of an adapter-esque duck can be detected.

Yes I'm referring to custom 3rd party adapters. Basically the only constrain is to have something that replies to call(env) so there's really no way to detect EVERY adapter, we can only safely detect the core ones.

Now both issues up here can be addressed putting some rules in place:

1. Adapters can only be added using the adapter method
2. Adapters MUST inherit from Faraday::Adapter

And I personally agree with both of them, but again we can't put such constraints in place now as it would break backward-compatibility.
So what we can do in the meantime to both help developers and move forward while we wait for Faraday v1.0 is to merge this PR in, but issuing a warning instead of raising an exception.
Something like "This behaviour have been deprecated and won't be accepted from Faraday 1.0".

[odlp]

The additional #insert use cases have now been covered. I've also updated the PR to print a warning to STDERR based on your feedback @iMacTia:

WARNING: Unexpected middleware set after the adapter. This won't be supported from Faraday 1.0.

[iMacTia]

Thank you @odlp.
I was looking at other parts of the gem and I noticed that a plain warn is used to manage warning: https://github.com/lostisland/faraday/search?utf8=✓&q=warn&type=

I would suggest we do the same in this instance, unless you have any particular reason for using $stderr

[odlp]

@iMacTia I've switched to warn.

[iMacTia]

Thank you @odlp,

happy to merge this in.
Devs are told to avoid this now and we'll make it mandatory in v1.0 👍