I'm not so sure about this. I think we're adding too much intelligence to the Request already. Maybe we need to put this logic in a handler that we insert as a middleware.

Also, note that the same class is used for client requests so I wonder if we shouldn't have a context object instead that we pass around between server handlers.

@waj that was my first suggestion. I only added the peeraddr to the REMOTE_ADDR header to be used in middleware. I like the peer_addr on request better. But perhaps we can move the remote ip logic to a handler that sets the REMOTE_ADDR header? Or is there another way of passing that info down the middleware stack without using the headers?

I would really dislike adding custom headers. And a middleware approach would do the detection on every single request, keep in mind it's about 10 header checks per request.

But if you use it for logging purposes you're gonna be doing the lookups on every request either via the middleware or via the request object.
If you only need the remote ip let's say every ten requests then it's 10 headers checks you're not gonna need

But there's always the option of creating an extra class where the logic can be placed. We could also create an HTTP util class.

But in all cases do we agree to the peer_addr being set on the request? Cause I would happily split the pull request into peer_addr and remote_ip

How about having a middleware that calls underlying handlers with request decorator, that has all the additional logic added on top of raw request? i.e.:

class SmartHTTPHandler < HTTP::Handler
  def call(request)
    smart_request = SmartRequest.new(request)
    call_next(smart_request)
  end
end

class SmartRequest
  private getter original_request
  def initialize(@original_request)
  end

  delegate_when_missing_to original_request

  # lazily fetches peer address of request
  def peer_addr
    # ...
  end

  # set peer address of request
  def peer_addr=(value)
    # ...
  end

  # .. and more stuff, like #url_params, #body_params, etc. ..
end

EDIT: of course naming is deliberate and requires some thought to make it right.

EDIT 2: of course everything, that it adds on top, should be as lazy as possible, to not create a performance overhead for unused parts.

EDIT 3: it doesn't necessary need to have all the additional functionality, instead one can have multiple "smart" request decorators and multiple "smart" middlewares allowing one to compose them as needed.

Other point, is that middleware is not required in this case, one can use these decorators directly on demand.

We plan to make a review of http request/response and client with @waj for this, and we'll hopefully come with a good solution.

@asterite now that #3406 is merged WDYT about this?

@sdogruyol It's on our TODO list, just that we won't implement it this way (it doesn't have to be attached to the request, it will probably be in the HTTP::Server::Context).

I originally came here to make sure this doesn't consider the HTTP headers as the authoritative source for IP address since HTTP headers can be spoofed. Glad to see there is an option to rely on the socket information (coming from the TCP connection data itself) or the HTTP header.

As far as code quality is concerned, LGTM. Perhaps some cleanup of the specs according to the comments already left by others.

LGTM 👍

/unsolicited-review

FWIW it would be "convenient" to have this in there I think people would use it. Here's how rails 2.3 does it: https://gist.github.com/rdp/ec8d8d7152988f496f951a2f0d65a320

Still not merged!! I really need that right now 😅 @benoist

Well i think this code is pretty outdated. Happy to make the required changes to get this merged though.

@benoist Yeah I'd say do it, if not I will in a couple weeks I think...

Wow, i still remember the day this issue opened. Time really flies 😄

@rdp Do we know what the required changes are, as I believe @asterite was going to discuss the implementation details with @waj

@sdogruyol It's on our TODO list, just that we won't implement it this way (it doesn't have to be attached to the request, it will probably be in the HTTP::Server::Context).

That was after the review comments from jhass

This is too old so I'm closing it.

We still need this functionality, though, but it will probably come from the core team. Maybe we can copy what Go does, but I don't know (I don't know much about HTTP and proxies).