Raise exception on nil status #1042

jonnyom · 2019-09-30T10:51:23Z

Description

If a host responds badly to HTTP requests it's possible for the HTTP status to be returned nil.
It makes sense to raise an exception here that can be rescued from and handled appropriately instead of silently allowing them through.
Fixes #1028

Todos

List any remaining work that needs to be done, i.e:

Tests
Documentation

olleolleolle · 2019-09-30T14:18:29Z

This got me curious about the case where the response code is numeric but in an invalid range.

(I'm out traveling, so this happens from a phone.)

jonnyom · 2019-09-30T15:33:27Z

That's a good point. A base case here for any invalid status could be useful

BobbyMcWho · 2019-10-01T13:57:07Z

lib/faraday/error.rb

@@ -13,7 +13,10 @@ def initialize(exc, response = nil)
        super(exc.message)
        @wrapped_exception = exc
      elsif exc.respond_to?(:each_key)
-        super("the server responded with status #{exc[:status]}")
+        nil_status_message = 'the server responded with a nil status'


Does it make sense to say the server responded with a nil status if the server did not send a status Header at all?

That's a fair point, maybe a more explicit message, something like status couldn't be derived from server response?

I like it, I'd say move it down into the error itself as well rather than in this base error class

BobbyMcWho · 2019-10-01T14:35:00Z

lib/faraday/error.rb

@@ -69,6 +72,10 @@ class ConflictError < ClientError
  class UnprocessableEntityError < ClientError
  end

+  # Raised by Faraday::Response::RaiseError in case of a nil status in response.
+  class NilStatusError < ClientError


Suggested change

class NilStatusError < ClientError

class NilStatusError < ServerError

class NilStatusError < ServerError def initialize(exc = nil, response = nil) message = 'http status could not be derived from the server response' super(message, response) end end

BobbyMcWho · 2019-10-01T14:41:32Z

lib/faraday/response/raise_error.rb

@@ -28,6 +28,8 @@ def on_complete(env)
          raise Faraday::ConflictError, response_values(env)
        when 422
          raise Faraday::UnprocessableEntityError, response_values(env)
+        when nil
+          raise Faraday::NilStatusError, response_values(env)


If you make the suggestions I made above, you'll need to change this line as well
raise Faraday::NilStatusError, nil, response_values(env)

Sounds good 👍 I'll get to this later today

So passing nil to this actually caused a bunch of issues for me when setting the backtrace. So I've opted to not pass it at all. Let me know if there's a better way around this.

🤦‍♂ silly me, we're not using Faraday::NilStatusError.new so the backtrace is implicit

jonnyom · 2019-10-01T15:23:15Z

This is failing lint on an unrelated file
In Circle

Inspecting 100 files
.........W..........................................................................................

Offenses:

lib/faraday/adapter/em_http_ssl_patch.rb:62:34: W: Lint/SendWithMixinArgument: Use include EmHttpSslPatch instead of send(:include, EmHttpSslPatch).
EventMachine::HttpStubConnection.send(:include, EmHttpSslPatch)
                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Locally

~/D/p/faraday ❯❯❯ rubocop                                                                                                                             raise-exception-on-nil-status
Inspecting 100 files
....................................................................................................

100 files inspected, no offenses detected
~/D/p/faraday ❯❯❯ rubocop --require rubocop/formatter/junit_formatter \                                                                               raise-exception-on-nil-status
        --require rubocop-performance \
        --format progress \
        --format RuboCop::Formatter::JUnitFormatter \
        --out ~/test-results/linting/rubocop.xml
Inspecting 100 files
.........W..........................................................................................

Offenses:

lib/faraday/adapter/em_http_ssl_patch.rb:62:34: W: Lint/SendWithMixinArgument: Use include EmHttpSslPatch instead of send(:include, EmHttpSslPatch).
EventMachine::HttpStubConnection.send(:include, EmHttpSslPatch)
                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

100 files inspected, 1 offense detected

I don't have permissions to retrigger the build, but this passes locally without JUnit, but once I've installed it it fails

olleolleolle · 2019-10-01T15:29:53Z

I'd appreciate a separate PR for the linting fix. <3

BobbyMcWho · 2019-10-01T15:34:05Z

I had implemented it in #1043, I can open a new PR to target it alone

jonnyom · 2019-10-01T15:36:16Z

Cool! I'm happy to roll it into this either @BobbyMcWho but its own PR would be cleaner I guess

BobbyMcWho · 2019-10-01T16:52:10Z

@olleolleolle merged it, @jonnyom you should be able to git pull [your lostisland remote] 0.16.x --rebase and be good to (force) push back up to your branch to get that change

BobbyMcWho · 2019-10-02T23:53:27Z

lib/faraday/error.rb

@@ -83,6 +83,14 @@ def initialize(exc = 'timeout', response = nil)
    end
  end

+  # Raised by Faraday::Response::RaiseError in case of a nil status in response.
+  class NilStatusError < ServerError
+    def initialize(response = nil)


I submitted a PR to your fork, this will need two params to handle the stack trace that's sent

I saw that, just lifted what you had and popped it in here, easier than merging things into the fork

jonnyom · 2019-10-09T16:49:51Z

@olleolleolle @BobbyMcWho just wanted to give this a bump

technoweenie

Thanks for adding this! I identified a really minor change, but I think this is ready to go. However, the original PR has an unchecked Documentation item. Did you plan on updating docs/middleware/response/raise_error.md in this PR too? I'm happy to take this on in a new PR, as I'd like to reformat the list of status codes inside the

block.

technoweenie · 2019-10-14T16:47:46Z

lib/faraday/response/raise_error.rb

+        when nil
+          raise Faraday::NilStatusError, response: response_values(env)


I'd rather have the nil check at the bottom, since it's the least likely to occur than any numerical status. Pretty minor issue, so I'm not sure if it's worth the effort :)

I like passing response as a named variable, rather than positional. I've added a note to the v2 wishlist (Faraday Wishlist #953) to make this consistent with the other error types.

jonnyom · 2019-10-15T14:29:30Z

@technoweenie if you want to refactor it then maybe we should just merge this and you can make that refactor including these changes?

(I also wasn't sure where the documentation was, which is why I didn't add it)

Thanks!

technoweenie · 2019-10-15T15:44:49Z

Thanks @jonnyom! I'll take care of the docs.

BobbyMcWho reviewed Oct 1, 2019

View reviewed changes

jonnyom force-pushed the raise-exception-on-nil-status branch from 0569535 to 538e2ae Compare October 2, 2019 09:41

Raise exception on nil status

6adf096

jonnyom changed the base branch from master to 0.16.x October 2, 2019 09:48

jonnyom force-pushed the raise-exception-on-nil-status branch from 538e2ae to 6adf096 Compare October 2, 2019 09:48

BobbyMcWho reviewed Oct 2, 2019

View reviewed changes

Pass the exception correctly

2c81ed6

technoweenie approved these changes Oct 14, 2019

View reviewed changes

technoweenie added this to In progress in v1.0 via automation Oct 14, 2019

technoweenie moved this from In progress to Review in v1.0 Oct 14, 2019

technoweenie merged commit 0f1eea4 into lostisland:0.16.x Oct 15, 2019

v1.0 automation moved this from Review to Done Oct 15, 2019

This was referenced Oct 15, 2019

Update 0.1x branch #1052

Closed

Raise error nil #1053

Merged

technoweenie added a commit that referenced this pull request Oct 17, 2019

Port nil status error from #1042

52ba14e

technoweenie mentioned this pull request Nov 27, 2019

add release notes for Faraday v0.17.1 #1082

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Raise exception on nil status #1042

Raise exception on nil status #1042

jonnyom commented Sep 30, 2019

olleolleolle commented Sep 30, 2019

jonnyom commented Sep 30, 2019

BobbyMcWho Oct 1, 2019

jonnyom Oct 1, 2019

BobbyMcWho Oct 1, 2019

BobbyMcWho Oct 1, 2019

BobbyMcWho Oct 1, 2019

BobbyMcWho Oct 1, 2019

jonnyom Oct 1, 2019

jonnyom Oct 1, 2019

BobbyMcWho Oct 1, 2019

jonnyom commented Oct 1, 2019 •

edited

olleolleolle commented Oct 1, 2019 •

edited

BobbyMcWho commented Oct 1, 2019

jonnyom commented Oct 1, 2019

BobbyMcWho commented Oct 1, 2019

BobbyMcWho Oct 2, 2019

jonnyom Oct 4, 2019

jonnyom commented Oct 9, 2019

technoweenie left a comment

technoweenie Oct 14, 2019

jonnyom commented Oct 15, 2019

technoweenie commented Oct 15, 2019

	class NilStatusError < ClientError
	class NilStatusError < ServerError

		when nil
		raise Faraday::NilStatusError, response: response_values(env)

Raise exception on nil status #1042

Raise exception on nil status #1042

Conversation

jonnyom commented Sep 30, 2019

Description

Todos

olleolleolle commented Sep 30, 2019

jonnyom commented Sep 30, 2019

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

jonnyom commented Oct 1, 2019 • edited

olleolleolle commented Oct 1, 2019 • edited

BobbyMcWho commented Oct 1, 2019

jonnyom commented Oct 1, 2019

BobbyMcWho commented Oct 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonnyom commented Oct 9, 2019

technoweenie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonnyom commented Oct 15, 2019

technoweenie commented Oct 15, 2019

jonnyom commented Oct 1, 2019 •

edited

olleolleolle commented Oct 1, 2019 •

edited