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

Question: How to add a default middleware to all faraday client instances? #946

Closed
danielgatis opened this issue Mar 14, 2019 · 8 comments
Closed

Comments

@danielgatis
Copy link

Hi,
How is the correct way to add a default middleware to all faraday client instances?

Example:

My middleware:

class MyMiddlware < Faraday::Middleware 
  def call ...
end

My clients:

conn1 = Faraday.new(url)
conn2 = Faraday.new(url)

I expected that all my clients (conn1, conn2) use MyMiddlware as default.

@iMacTia
Copy link
Member

iMacTia commented Mar 14, 2019

@danielgatis I'm afraid this is currently not possible.
To be honest I'm not sure this is something developers should need, as you usually setup only 1 connection for each 3rd party service you use. My guess is that you need this feature because you're creating too many connections so I may help reducing that if you give more details about your implementation.

If you you're not abusing connection and still need this in place, you have a couple of options.

  1. You can centralise the middlewares you want all connection to use by using a function:
def shared_stack(conn)
  conn.request :url_encoded
  conn.use MyMiddleware
  conn.adapter Faraday.default_adapter
end

conn1 = Faraday.new(url1) { |conn| shared_stack(conn) }
conn2 = Faraday.new(url2) { |conn| shared_stack(conn) }
  1. You can set the default connection and use the shorthand for running requests:
Faraday.default_connection = Faraday.new do |conn|
  conn.request :url_encoded
  conn.use MyMiddleware
  conn.adapter Faraday.default_adapter
end

# Now that you setup a default connection, you can simply call the urls without creating a new connection every time
Faraday.get(url1)
Faraday.get(url2)

Let me know if any of the above helps

@danielgatis
Copy link
Author

Thanks for help @iMacTia.

@marcheiligers
Copy link

I needed to do this too and after much searching, came across this issue. In my case, I was testing an APM service. I have a number of API client gems in my app. I know they all use Faraday. So I was hoping to just be able to set it up so any connection would get the APM Middleware. I do think this is a legit use case.

For my own testing purposes I did the following evil monkey patch:

module Faraday
  class Connection
    alias_method :orig_initialize, :initialize

    def initialize(url = nil, options = nil)
      orig_initialize(url, options)
      yield self if block_given?
      builder.use :apm_thingymagick_here
    end
  end
end

I do not recommend this, but if anyone else finding this issue needs a quick temporary solution, this works for now... you will get the following warning which indicates it will stop working at some point in the future:

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

@doliveirakn
Copy link

+1 for the above.

An APM is the only use case I've seen code a default middleware. For example see datadog's APM

They suggest to do something like this:

# Configure default Faraday tracing behavior
Datadog.configure do |c|
  c.use :faraday, options
end

# Configure Faraday tracing behavior for single connection
connection = Faraday.new('https://example.com') do |builder|
  builder.use(:ddtrace, options)
  builder.adapter Faraday.default_adapter
end

connection.get('/foo')

If you don't add the builder.use(:ddtrace) bit, it doesn't seems to use the middleware. This means that we could use the APM for any Faraday connection we manage, but for any client library that uses Faraday, we wouldn't be able to measure them.

@iMacTia
Copy link
Member

iMacTia commented Jun 10, 2019

@marcheiligers @doliveirakn I know this sounds like a reasonable and simple use case, but in reality it's actually more complicated than what it looks like.
The middleware order in Faraday connection stacks is extremely important (hence the warning you get about v1.0) and swapping the order of middleware can cause the connection not to work properly.
If we were to allow injection on middleware even in 3rd party gems that use Faraday, this could very easily break the stack built internally in the app.

In short, it is something only "pro" users should do and only if they understand what they're actually doing. As things stand, we don't feel like this should be a feature provided by Faraday and available to everyone and I'd ask you to stick with the Monkey Patch you proposed.
You should be able to solve the warning by calling builder.use :apm_thingymagick_here BEFORE yield self if block_given?.

@doliveirakn
Copy link

I get that the middleware order is really important. I think it would be really nice to be able to provide a way to flag middleware as default when registering it, and then when a new builder is made, applying the default middleware first (so it is the outermost), and then any custom middleware and then finally the adapter.

For other's that may require this behaviour, this is the "fix" that I took to get this to work within our stack so that the ddtrace middleware is always first, and we also don't get the same warning message that @marcheiligers reported.

  module FaradayConnectionOptions
    def new_builder(block)
      super.tap do |builder|
        builder.use(:ddtrace)
      end
    end
  end

  Faraday::ConnectionOptions.prepend(FaradayConnectionOptions)

@iMacTia
Copy link
Member

iMacTia commented Jun 12, 2019

@doliveirakn thanks for sharing that snippet, that is exactly the way we could implement it in Faraday as well, probably by letting you define a list of "default_middlewares" as a global setting in Faraday (similar to Faraday.default_adapter).

However as you pointed out we need to decide where the middleware should be injected, with you suggesting to go for the top. That might work for your specific use case, however it won't work for others. For example, I might want to send all requests to a logging/monitoring service, however injecting such a middleware at the beginning will cause parameters not to be encoded and middleware that changes the request (e.g. setting headers) won't have run yet, causing the logged request to be incomplete.

So then we would be asked to change the implementation allowing to choose if the middleware should go at the beginning or the end (still doable). That's it, until someone finds a use case where they need the middleware to be positioned in an exact position to avoid conflicts.

Even worse, you might inject a middleware that causes the stack to fail, potentially causing developers to open issue on the gem that builds that stack using Faraday, even though they have no clue of why that happens.
And if this is caused by a gems combo (gem B uses the new feature to inject middleware and breaks gem A usage of Faraday), that is going to cause a lot of headaches for the gems maintainers to figure out where the problem is.
Keeping the Faraday stack "closed" from outside modification allows gems using Faraday to control the conditions in which the request is performed.

So here is our take on this:

  1. The Faraday team has discussed this and although this is technically possible, we're quite sceptic on the practicality and maintainability of such solution (see above for more details on this).
  2. The best solution in our opinion is for the gems to allow you to configure their internal Faraday stack. This way this is an opt-in approach. Although we understand this would mean opening PRs on all those gems.
  3. That said, we're not completely opposed to the idea and we recognise this might be useful in some cases. If someone has a good implementation, or even an idea of an implementation that could work while solving/mitigating the conflicts issues, we'd be happy to discuss it.
  4. For the time being, we strongly suggest to use the snippet provided in Question: How to add a default middleware to all faraday client instances? #946 (comment)

@jensljungblad
Copy link

Kind of in the same situation. Would like to enable the shipped instrumentation middleware for all Faraday connections. The use case is to instrument external requests made by other gems we do not control. In this case, the stripe and elasticsearch gems, both use Faraday.

I do agree that the best solution would be for these gems to allow configuring the Faraday stack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants