Have `HTTP::Client.get(path)` make the request object available?

Crystal Contrib
1

I’m looking at 1.7.3’s HTTP::Client and I was wondering if this change could be made? Not to one function, probably to most of the functions in the file.

   def exec(method : String, path, headers : HTTP::Headers? = nil, body : BodyType = nil)
-    exec(new_request(method, path, headers, body)) do |response|
-       yield response, request
-     end
+    request = new_request(method, path, headers, body)
+    exec(request) do |response|
+       yield response, request
+     end
   end

I have a scenario where I’d like to do some stuff where I’m able to use the request object in some of my error reporting

request = HTTP::Request.new("GET", "/api/example-endpoint")
response = client.exec request
ok_or_raise request, response
# do other stuff

and it would be nicer to write this as:

client.get "/api/example-endpoint" do |response, request|
    ok_or_raise response, request
    # do other stuff
end

both samples using this macro:

macro ok_or_raise(request, response)
  unless {{response.id}}.success?
    %msg = "#{{{response.id}}.status_code} #{{{response.id}}.status} #{{{request.id}}.path} "
    Log.error { "Unexpected status code: #{{{response.id}}.status_code}" }
    Log.error { %msg }
    Log.error { {{response.id}}.body }
    raise Exception.new(%msg)
  end
end

the order of |response, request| is backwards from what most people are use to in this example, I only have it that way for backwards compatibility. Also, all my samples are a rough draft, I barely know this language and never used ruby, so maybe there is some obvious utility I’m not aware exists.

2

If all you’re wanting the request object for is the path, you could use either version just doing like:

response = client.get path = "/api/example-endpoint"
ok_or_raise response, path

or

client.get path = "/api/example-endpoint" do |response|
    ok_or_raise response, path
    # do other stuff
end

Not directly related, and I’m not sure if it makes a difference for you, but the yielding and non-yielding variants of #exec (and the other request methods) are not semantically identical. The yielding variant provides streaming access to response body such that you could use it w/o loading the entire body into memory while the non-yielding variant does just that.

EDIT: Maybe another option would be to have the option to raise on failed requests?
EDIT2: Related: Redesign HTTP::Client to implement missing features · Issue #6011 · crystal-lang/crystal · GitHub

3

FWIW I’m not currently trying to use the streaming versions of HTTP.

but something like this would be good enough for me:

class HTTP::Client::RaisedResponse < Exception
    getter request : HTTP::Request
    getter response : HTTP::Client::Response
    getter message : String?
    def initialize(@request, @response)
        @message = "Unexpected response status code: #{@response.status_code}"
    end
end

client = HTTP::Client.new()
client.raise_unless_status_codes= 200..204
client.raise_unless_success
# might raise
response = client.get("/api/example-endpoint")

or:

client = HTTP::Client.new()
# custom variant
# I 
client.add_response_hander { |request, response|
  unless response.success?
     raise HTTP::Client::RaisedResponse.new(request, response)
  end
}
# or convenient built-in
client.add_response_hander HTTP::Client::Handler::SuccessOrRaise.new
# might raise
response = client.get("/api/example-endpoint")
4

This is possible if you wrap or subclass HTTP::Client. For example, subclassing it to add response handlers:

require "http"

class MyClient < HTTP::Client
  alias HandlerProc = HTTP::Request, HTTP::Client::Response -> Nil
  @response_handlers = [] of Handler | HandlerProc

  def add_response_handler(handler : Handler)
    @response_handlers << handler
  end

  def add_response_handler(&handler : HandlerProc)
    @response_handlers << handler
  end

  def exec_internal(request)
    response = super
    @response_handlers.each(&.call(request, response))
    response
  end

  module Handler
    abstract def call(request : HTTP::Request, response : HTTP::Client::Response)
  end
end

struct SuccessOrRaise
  include MyClient::Handler

  def call(request, response)
    raise Failure.new("Response was unsuccessful", response) unless (200...400).includes? response.status_code
  end

  class Failure < Exception
    getter response : HTTP::Client::Response
    def initialize(message, @response)
      super message
    end
  end
end

client = MyClient.new(URI.parse("https://zomglol.wtf"))
client.add_response_handler do |request, response|
  pp response.status
end
client.add_response_handler SuccessOrRaise.new

client.get "/@jamie"

I don’t think it makes sense for HTTP::Client itself to include configuration for raising exceptions or anything like that itself, though. HTTP is just a protocol and error responses are not exceptional (they happen all the time). How those responses are handled is a concern for what’s running on top of that protocol.

With that said, it might make sense to add callbacks similar to HTTP::Client#before_request that runs after you get the response back but before it returns it to the caller. The before_request callback is great for adding headers (it’s used by OAuth2 to add access tokens to requests made by that client, for example), and something like after_request that yields the request/response pair could be useful for instrumentation.

2 Likes