ureq icon indicating copy to clipboard operation
ureq copied to clipboard

Add Response::remote_addr()

Open mvforell opened this issue 2 years ago • 1 comments

Fixes #488.

mvforell avatar Mar 05 '22 11:03 mvforell

Sorry for the delay, I've finally found time to look into this again.

I've pushed a new implementation that doesn't use TcpStream::peer_addr() and where Response::remote_addr() returns a SocketAddr instead of an Option<SocketAddr>.

I'm still unsure about two things regarding this new implementation:

  1. I'm not familiar with SOCKS proxies. I think with this implementation, Response::remote_addr() returns the socket address of the proxy. I'm not sure if this is the wanted behaviour; if not, I would need a few more pointers on how to change it so that the actual server's address is returned. Either way, this should probably be documented, e.g. by adding "When using a SOCKS proxy, this returns the proxy's address." to Response::remote_addr()'s documentation.
  2. When using Stream::from_vec(), it is not clear what the remote address should be. If I understand correctly, this method is only used for tests and for Response::from_str(). In tests, the remote address isn't used; Response::from_str() already hard-codes example.com as the request URL, so I took the liberty to hard-code the remote address to 0.0.0.0:0. Please let me know if you disagree with this.

mvforell avatar May 23 '22 22:05 mvforell

Whoops, I have to admit I forgot about this. 😅

I'd be happy to rebase and resolve the conflicts, but before diving back into ureq's code, it'd be great if you could give me some feedback regarding the current implementation and my two concerns mentioned in the last comment.

mvforell avatar Sep 15 '22 18:09 mvforell

Glad to hear that! :) I've merged the upstream changes, please take a final look.

mvforell avatar Sep 28 '22 18:09 mvforell

Would you mind rebasing off the latest main, so we can see a clean test run?

algesten avatar Sep 29 '22 20:09 algesten

Done

mvforell avatar Sep 29 '22 20:09 mvforell

Alright, now that I ran cargo test with all the features enabled all the tests should compile and pass 😅

mvforell avatar Sep 30 '22 20:09 mvforell

There's also the test.sh bash script that runs a few of the feature flag combos the CI runs.

algesten avatar Sep 30 '22 21:09 algesten

Good to know!

mvforell avatar Sep 30 '22 21:09 mvforell

@mvforell There are some conflicts against main.

algesten avatar Oct 01 '22 19:10 algesten

@mvforell There are some conflicts against main.

Hm, GitHub says "This branch has no conflicts with the base branch", and my local checkout says:

> git fetch upstream && git merge upstream/main
Already up to date.

mvforell avatar Oct 01 '22 19:10 mvforell

@mvforell sorry! must have had an old browser tab or something.

algesten avatar Oct 01 '22 21:10 algesten

I'm really happy to see this merged, thank you for your feedback and pointers!

Do you have an estimate for when the next version including this will be released?

mvforell avatar Oct 03 '22 20:10 mvforell