Request/Response lifecycle refactoring #845

iMacTia · 2019-02-14T16:02:18Z

Description

This is probably the biggest refactoring for v1.0 and it's aimed to address a lot of issues that developers raised over time, from the request body being overridden (#517) to the Adapters acting as they were middlewares (#47).

Adapters refactoring
- They're now final endpoints. They don't call @app.call anymore.
Env refactoring
- Refactors Env by moving all response-related fields into the response. The response body doesn't override the request body anymore!
- Ensures backwards compatibility by introducing getter methods in Env.
Middlewares refactoring (mostly backwards compatible):
- All middlewares inherit from the same Faraday::Middleware class.
- Adds new on_request helper so basic middlewares (request and response) don't need to override call anymore.
- Refactor existing middlewares to reflect this change.
Updates UPGRADING.md with all breaking changes.

Fixes #47 once and for all!
Fixes #517

Additional Notes

Review welcome from @olleolleolle
Cc: @mislav (just in case you want to have a look 😄

* They're now final endpoints. They don't call `@app.call` anymore. * Env refactoring * Refactors `Env` by moving all response-related fields into the response. The response body doesn't override the request body anymore! * Ensures backwards compatibility by introducing getter methods in `Env`. * Middlewares refactoring (mostly backwards compatible): * All middlewares inherit from the same `Faraday::Middleware` class. * Adds new `on_request` helper so basic middlewares (request and response) don't need to override `call` anymore. * Refactor existing middlewares to reflect this change. * Updates UPGRADING.md with all breaking changes. Fixes #47 once and for all! Fixes #517

mislav

Thank you for doing all this work!

I think there's too much at once happening in a single PR. I would recommend distilling each change to its own separate PR and keeping unnecessary changes (e.g. whitespace-only changes, or simply moving around code unchanged) out until the last moment (preferrably via follow-up, also separate PRs).

Faraday::Env, Faraday::Response and similar classes are sacred! Really really try not to break any existing APIs unless absolutely necessary. You can soft-deprecate old APIs in favor of new ones, e.g. documentation could suggest that env[:body] should be written like env.response.body going foward.

Re: Faraday adapters not being middleware anymore: finally! Does the stack builder now throw errors if someone attempts to build a stack without an adapter, or putting an adapter elsewhere than the "end" (the deepest point) of the stack? That would be helpful for people who did that mistake in their code but haven't got any error or warning from Faraday, until they tried making requests and had things break confusingly.

mislav · 2019-02-18T17:40:44Z

lib/faraday/adapter/typhoeus.rb

@@ -7,6 +7,11 @@ def call; end

      dependency 'typhoeus'
      dependency 'typhoeus/adapters/faraday'
+
+      def initialize(adapter_options = {})


I think it's fine to instead drop this adapter for a 2+ years old version of Typhoeus

You're right, this class was still around for typhoeus <= 1.3.0, however I changed adapters constructor to not accept the @app anymore (as they don't need to call @app.call(env)) and this breaks even the latest version of the adapter in the gem. Here I'm overriding the initialize method so that it works as expected. The only other solution is to revert the adapter initialzer signature and pass @app anyway although useless.

mislav · 2019-02-18T17:43:45Z

lib/faraday/options/env.rb

+      self[:body]
+    end
+
+    def body


Is an ambiguous body accessor really necessary or should callers instead always use request_body/response_body explicitly?

Agree, request_body/response_body is the preferred way to access bodies from now on. This getter is there purely for backwards-compatibility reasons.

Ah I see. It's been a long time since I've worked on this project and I forgot that Env is a Struct and that env[:body] is equivalent to env.body

mislav · 2019-02-18T17:46:40Z

lib/faraday/request/authorization.rb

@@ -39,11 +39,10 @@ def initialize(app, type, token)
    end

    # @param env [Faraday::Env]
-    def call(env)
+    def on_request(env)


I don't feel that specialized on_request/on_response convenience methods bring much benefit over overriding call and having to invoke app.call(env) manually. It's just more API to maintain over the years and more documentation on how to write middleware that has to be written.

Even if you eventually add these methods, it's best to be kept in a separate PR for ease of reviewing/summarizing the change.

Main reason for introducing them is that I saw people having some hard time understanding middlewares concatenations internals and the stack model. I figured having simple callbacks like on_request and on_complete (we already have the latter) would make like easier for people writing simple middlewares.
You can still override call when you want to have full control over the stack progression (e.g. in the retry middleware).

I do agree however that this was not a necessary change in this PR, will definitely separate on a different one

mislav · 2019-02-18T17:48:59Z

lib/faraday/response.rb

@@ -2,96 +2,68 @@

 module Faraday
  class Response
-    # Used for simple response middleware.
-    class Middleware < Faraday::Middleware


If it's possible to keep Faraday::Response::Middleware, keep it. (Middleware that ships with Faraday don't have to necessarily subclass it.) A lot of people will be disappointed if this is gone for no strong reason. Yes, big 1.0s are in general expected to sometimes break backwards compatibility, but keep in mind the adoption on Faraday everywhere and the emotional and logistical cost of upgrading and dealing with breakages due to changes like these.

I could keep it as a dummy sub-class of Faraday::Middleware for the simple purpose of backwards-compatibility.

mislav · 2019-02-18T17:54:10Z

lib/faraday/adapter/em_http.rb

@@ -18,7 +18,7 @@ def connection_config(env)
        def request_config(env)
          options = {
            :body => read_body(env),
-            :head => env[:request_headers],
+            :head => env.request_headers,


If env[:request_headers] still works, unnecessary changes like these make it more difficult to see the actual changes in this PR. It's best to keep the old env[:foo] approach for now in code (but document the method-based approach in usage examples) and update internal usage in its own PR.

Agree, will isolate in separate PR

mislav · 2019-02-18T17:57:25Z

lib/faraday/rack_builder.rb

@@ -34,8 +35,13 @@ def initialize(klass, *args, &block)
        @args, @block = args, block
      end

-      def klass() @@constants[@name] end
-      def inspect() @name end
+      def klass()


Whitespace-only changes like these are unnecessary in this PR and make it hard to spot the actual changes in this diff

Agree, this is RubyMine auto-indenting. I usually revert these changes back when I spot them but I missed this one

iMacTia · 2019-02-18T18:56:11Z

@mislav you're right, I got the same feedback from @olleolleolle as well, I pulled too many changes on a single PR.
I can identify 3 main changes here that I might be able to isolate into separate PRs:

Adapters refactoring
Env refactoring
Middlewares refactoring

I'll try to create 3 separate branches and mirror only the changes for a particular section, this way you should have a much better time to review them.

Re: Faraday adapters not being middleware anymore: finally! Does the stack builder now throw errors if someone attempts to build a stack without an adapter, or putting an adapter elsewhere than the "end" (the deepest point) of the stack? That would be helpful for people who did that mistake in their code but haven't got any error or warning from Faraday, until they tried making requests and had things break confusingly.

I'm so proud I can finally answer YES to this one 😄. It's not bulletproof, but I started this change on a previous PR and you can see the details at the end of the UPGRADING.md

In order to specify the adapter you now MUST use the #adapter method on the connection builder. If you don't do so and your adapter inherits from Faraday::Adapter then Faraday will raise an exception. Otherwise, Faraday will automatically push the default adapter at the end of the stack causing your request to be executed twice.

If you do use #adapter then faraday stores the adapter separately and always put it at the end automatically for you.
Finally, if you forget to call #adapter, then the default one is automatically pushed at the end of the stack for you.

olleolleolle

(Teeny-tiny Suggestion changes to wording.)

This is an amazing piece of news and thank you so much for building this.

UPGRADING.md

olleolleolle · 2019-02-18T18:17:52Z

UPGRADING.md

+But now you can access the request body even after a request has been completed using the `request_body` getter.
+
+### Middlewares
+Middlewares has been refactored, there's no `Faraday::Response::Middleware` class anymore and all "response" middlewares now inherit from the same `Faraday::Middleware` class as the "request" ones.


Suggested change

Middlewares has been refactored, there's no `Faraday::Response::Middleware` class anymore and all "response" middlewares now inherit from the same `Faraday::Middleware` class as the "request" ones.

Middlewares have been refactored, there's no `Faraday::Response::Middleware` class anymore and all "response" middlewares now inherit from the same `Faraday::Middleware` class as the "request" ones.

Co-Authored-By: iMacTia <iMacTia@users.noreply.github.com>

iMacTia mentioned this pull request Feb 15, 2019

don't stomp on request body #517

Closed

mislav reviewed Feb 18, 2019

View reviewed changes

olleolleolle reviewed Feb 18, 2019

View reviewed changes

olleolleolle and others added 2 commits February 18, 2019 19:12

Update UPGRADING.md

2fd9094

Co-Authored-By: iMacTia <iMacTia@users.noreply.github.com>

Updated UPGRADING.md

306b512

iMacTia mentioned this pull request Feb 18, 2019

Refactor Adapter as final endpoints #846

Merged

iMacTia closed this Feb 20, 2019

iMacTia deleted the feature/request-response-cycle-refactoring branch February 20, 2019 18:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Request/Response lifecycle refactoring #845

Request/Response lifecycle refactoring #845

iMacTia commented Feb 14, 2019 •

edited

Loading

mislav left a comment •

edited

Loading

mislav Feb 18, 2019

iMacTia Feb 18, 2019

mislav Feb 18, 2019

iMacTia Feb 18, 2019

mislav Feb 18, 2019

mislav Feb 18, 2019

iMacTia Feb 18, 2019

mislav Feb 18, 2019

iMacTia Feb 18, 2019

mislav Feb 18, 2019 •

edited

Loading

iMacTia Feb 18, 2019

mislav Feb 18, 2019

iMacTia Feb 18, 2019

iMacTia commented Feb 18, 2019

olleolleolle left a comment •

edited

Loading

olleolleolle Feb 18, 2019

	Middlewares has been refactored, there's no `Faraday::Response::Middleware` class anymore and all "response" middlewares now inherit from the same `Faraday::Middleware` class as the "request" ones.
	Middlewares have been refactored, there's no `Faraday::Response::Middleware` class anymore and all "response" middlewares now inherit from the same `Faraday::Middleware` class as the "request" ones.

Request/Response lifecycle refactoring #845

Request/Response lifecycle refactoring #845

Conversation

iMacTia commented Feb 14, 2019 • edited Loading

Description

Additional Notes

mislav left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mislav Feb 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iMacTia commented Feb 18, 2019

olleolleolle left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iMacTia commented Feb 14, 2019 •

edited

Loading

mislav left a comment •

edited

Loading

mislav Feb 18, 2019 •

edited

Loading

olleolleolle left a comment •

edited

Loading