Skip to content

Conversation

@bjeanes
Copy link

@bjeanes bjeanes commented Jul 30, 2018

I've wanted this each time I've reached for HTTP so thought I'd propose this as an addition. The use-case here is when using HTTP as an internal client for something like an API client which may connect to different endpoints. Currently, the only (non-persistent) way to provide the hostname to connect to is when building the actual request, which can often mean having to pass more configuration deeper into these wrapper classes.

As a matter of abstraction, there are instances where it is simpler to allow a class to use only relative paths to resources and be passed a fully configured HTTP client to execute them.

This leverages the existing mechanism for building URIs as persisted
connections does but is a matter of convenient, not connection sharing.

I suspect this will need further work, in particular to ensure nothing is broken with persistent connections and to explore the desired error messages or alternate behaviour when attempting to bind to a host but also persistently connect to another.

A contrived example to show usage (in my actual use-case, persistent connections can't be used in 100% of cases, even though they could in this example):

http = HTTP.
  to('https://github.com').
  accept(:json).
  auth('Bearer abc123')

# executes .get('/repos') or whatever
RepositoryArchiver.new(client: http).archive_if { |repo| ... }

This leverages the existing mechanism for building URIs as persisted
connections does but is a matter of convenient, not connection sharing.
@ixti
Copy link
Member

ixti commented Aug 1, 2018

I don't like this idea. This gem aims to be just a simple HTTP client. Not an API client builder.
@httprb/core any thoughts?

@bjeanes
Copy link
Author

bjeanes commented Aug 1, 2018

@ixti I understand that objection.

That being said, that's just my most recent example of a use case. I believe there are others that could justify the addition (whether with the API I've proposed or another).

My perspective here is that forcing the full URI to be present at the final request building stage tends to result in a more complex implementation (not of HTTP, but of code using HTTP).

In order to have any HTTP usage which is "generalisable" over multiple endpoints (practically: a micro-service in staging vs that same micro-service in production), today that code must manually construct URIs at the last possible moment and for all unique interactions. That construction is replicated in every different resource or semantic interaction with the service. In order to avoid this replication, one has to wrap HTTP entirely to DRY this up, losing some of the benefit of HTTP's nice DSL or doing a lot of work to expose an equally fluent interface.

Right now, in HTTP, you can prepare some "common" concerns for a series of interactions, such as headers likely to be the same throughout your interaction (Content-Type, Accept) or setting up authorisation. The hostname you connect to is equally common throughout that interaction.

Perhaps my above articulation will be more persuasive. If not, I'd love to re-frame it again. This has continually been my frustration when picking up HTTP and usually the reason I don't include it in projects other than simple scripts. I think this client is wonderful, otherwise, but find it difficult to write well-factored code for non-trivial integrations.

Regardless, thanks for the awesomeness that is this project.

@bjeanes bjeanes changed the title Add #to for binding client to a hostname RFC: Allow passing server as an option to allow DRYing multiple interactions with same endpoint and parameterising API integrations by environment Aug 1, 2018
@paul
Copy link
Contributor

paul commented Aug 23, 2018

I agree with @bjeanes. I didn't realize it wasn't intended for that, but HTTP.rb is a great API client builder, better than any others. I usually do something like this:

class MyClient

  def some_action(params)
    get("/some/action", params)
  end
  
  # def some more actions

  private

  def get(path, params)
    request(:get, path, params)
  end
 
  # def put/post/etc...

  def request(method, path, params)
    url = URI.join(@host, path).to_s
    http.request(verb, url, params: params)
  end
 
  def http
    @http ||=
      ::HTTP
      .timeout(:global, connect: 2.seconds, read: 10.seconds, write: 10.seconds)
      .auth("Bearer #{@token}")
  end
end

Every time I do this, I'm always surprised I have to the the URI.join(...) part myself, and that HTTP doesn't just have a .host(@host) method I could use to do that automatically. The HTTP Persistence feature does handle this, and I sometimes use .persist solely for this "set-the-default-host" behavior.

I'd vote 👍 for having .to (though I'd prefer it be called .host).

@janko
Copy link
Member

janko commented Aug 23, 2018

I'm ok with this addition, it seems useful based on the examples above. For me http.rb does thrive in its ability to easily build an HTTP client (that's why it has the chainable interface as an alternative to just a hash of options), and specifying the host is a common idiom IMO.

@bjeanes
Copy link
Author

bjeanes commented Aug 23, 2018 via email

@tarcieri
Copy link
Member

tarcieri commented Sep 19, 2018

I think I'd be okay with host (sans URI prefix, but what default scheme? HTTPS?) or origin with full URI prefix

@ixti
Copy link
Member

ixti commented Sep 19, 2018

+1 for origin

@ixti ixti mentioned this pull request Dec 5, 2018
@bjeanes
Copy link
Author

bjeanes commented Dec 11, 2018

Until discussion here and in #513 pans out, here's what I've been doing in my app for this:

class APIClient
  include HTTP::Chainable

  # @param endpoint [URI|String]
  # @param client [HTTP::Chainable]
  def initialize(endpoint, client = ::HTTP)
    @token = token
    @endpoint = URI(endpoint)
    @client = client
  end

  def branch(options)
    self.class.new(@token, @endpoint, super(options))
  end

  # SECURITY: `uri_or_path` should not be controlled by end user
  def request(verb, uri_or_path, options = {})
    uri = URI.join(@endpoint, uri_or_path) 
    @client.request(verb, uri, options)
  end
end

This avoids monkey-patching or sub-classing in lieu of a new class that simply meets the HTTP::Chainable interface. It's quite simple and powerful and has served me well pending HTTP.rb getting an equivalent feature.

(attn @paul, this might be a simpler alternative to what you're doing without having to duplicate a lot of the interface and risk drift away from the core lib)

@ixti ixti mentioned this pull request Jan 16, 2019
@ixti ixti added this to the v4.2.0 milestone Mar 11, 2019
@tarcieri
Copy link
Member

Any interest in picking this PR up? The spirit of the changes look good and it seems like we got lost in the weeds on a few of the details.

@bjeanes
Copy link
Author

bjeanes commented Mar 11, 2019

@tarcieri If we can bring #519 to a resolution/decision first, that would make sense.

I 100% want this in HTTP.rb. That being said, I think #513 by @sj26 looks like a nicer API. What do you think about letting that PR officially subsume this one?

@tarcieri
Copy link
Member

Please follow up on #519 for this

@tarcieri tarcieri closed this Sep 10, 2021
@bjeanes bjeanes deleted the bind-to-host branch September 10, 2021 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants