lostisland / faraday

Simple, but flexible HTTP client library, with support for multiple backends.
https://lostisland.github.io/faraday
MIT License
5.72k stars 972 forks source link

Document middlewares order (priority) #1458

Closed AlexWayfer closed 1 year ago

AlexWayfer commented 1 year ago

Basic Info

Issue description

Please, add more documentation about middlewares ordering and priority. With detailed explanations how hooks work, I think.

Steps to reproduce

I had a code like:

Faraday.new(...) do |faraday|
  faraday.request :json
  faraday.response :json

  faraday.response :parse_dates
end

It seems obviously for me that a response firstly being decoded from JSON, then dates will be parsed.

The gem for parsing dates BTW: https://github.com/AlexWayfer/faraday-parse_dates

But it didn't work, until I've change their order.

Why? I believe I've done everything according to middlewares documentation in the faraday-parse_dates gem.

At the https://lostisland.github.io/faraday/middleware/ I've found such documentation:

Swapping middleware means giving the other priority.

How, in which order, some examples and strict rules?

iMacTia commented 1 year ago

Hey @AlexWayfer, thanks for raising this, this is great feedback. The short version of it is that requests traverse the middlewares top-down, while responses travel back bottom-up. We've put this image in the docs that I thought was really good at explaining things:

That said, I understand this can still be confusing for some, so I'll think about another illustrative way to visualise this. If you have any suggestions, they would be very welcome!

AlexWayfer commented 1 year ago

I saw this image. It's confusing for my case because there are middlewares are for request and response, both.

For example, you can define Faraday::Response.register_middleware and not define Faraday::Middleware.register_middleware, so it'll not be suitable.

In a perfect case, I'm not sure if it's possible, I'd just turn the direction for response middlewares reverse. It'd be more intuitive I believe.

iMacTia commented 1 year ago

That is unfortunately not possible without compeltely re-engineering Faraday's internals 😅. The middleware stack is currently built by nesting each middleware into each other, which is what makes things like the "retry middleware" possible, but I agree it's not super-easy to grasp.

It doesn't really matter if the middleware was registered as a request or a response one, the only thing that matter is how they're added to the stack.

Say you have the following:

Faraday.new(...) do |faraday|
  faraday.request :authorization
  faraday.response :json
  faraday.response :parse_dates
end

This will result into a middleware stack like the following:

authorization do
  # authorization request hook
  json do
    # json request hook
    parse_dates do
      # parse_dates request hook
      response = adapter.perform(request)
      # parse_dates response hook
    end
    # json response hook
  end
  # authorization response hook
end

Hence you can see that parse_dates is the LAST middleware processing the request, and the FIRST middleware processing the response

AlexWayfer commented 1 year ago

I see the architecture with call, on_request and on_complete.

And I don't understand why on_ have no super-methods. While call I believe has one, but "you normally don't need to override it".

We could have more flexible approach with the super methods for on_, not only call for both (request and response).

Or we should stick with single call, and everything before super is for request, after — response. I've done this in mine Flame Rack-based framework.

I see a kind of conflict between approaches here.

AlexWayfer commented 1 year ago

But OK, with the single call + super we would have the same problems: later middleware process the request later and the response earlier.

Meh.

There are different types of middlewares, different processes and chains. I'd like to see the super for on_ methods instead of overriding call. I think we should move in this way.

Again, we can left the call, but I guess supers for on_ methods will help with the order… or not. I'm messed up.

Also we can try to change the order of adding middlewares depending on their types: https://github.com/lostisland/faraday/blob/85dfdf8/lib/faraday/middleware_registry.rb#L28

Not to add into the end (merge hashes), but prepend. Not sure it'll work for any middleware, but we can process the basic cases and left old logic for others.

panozzaj commented 1 year ago

I have been working with Faraday a decent amount, and the nested block example above made the diagram above finally click for me. Before, I would just reorder the middlewares until the behavior seemed right. Thanks for providing that example!

iMacTia commented 1 year ago

@panozzaj @AlexWayfer FYI -- I've incorporated your feedback into the docs.

You can see the result on the Middleware page

I'm closing this issue as we're not really planning any internal refactoring on how middleware works, that would be quite the breaking change 😄 !

jpriollaud commented 1 year ago

Another example

Doesn't work:

faraday.use CustomErrorMiddleware
faraday.response :raise_error

Works:

faraday.response :raise_error
faraday.use CustomErrorMiddleware

Not intuitive in the slightest.