go/web
1
1
Fork 2
Code Issues 1 Pull requests 1 Releases Activity Actions

Add RemoteIP #10

Open
hanz wants to merge 3 commits from hanz/web:main into main
pull from: hanz/web:main
merge into: go:main
Conversation 5 Commits 3 Files changed 5 +88 -5
hanz commented 2025-10-05 02:57:10 +00:00
First-time contributor
Copy link

Enable server to get remote IP

Enable server to get remote IP
feat: Added remote_ip
All checks were successful
/ test (pull_request) Successful in 26s
Details
fa43127528
hanz requested review from ed 2025-10-05 02:57:39 +00:00
hanz commented 2025-10-05 03:29:09 +00:00
Author
First-time contributor
Copy link

For anyone who would like to use this feature, you can add this line on your go.mod:

replace git.urbach.dev/go/web => git.urbach.dev/hanz/web v0.0.0-20251006115005-3928f72f577d

Updated with the latest commit

For anyone who would like to use this feature, you can add this line on your `go.mod`: ``` replace git.urbach.dev/go/web => git.urbach.dev/hanz/web v0.0.0-20251006115005-3928f72f577d ``` Updated with the latest commit
ed commented 2025-10-06 10:35:29 +00:00
Owner
Copy link

Hi hanz,

just wanted to let you know that I'm very busy at the moment so I can't do a full review yet.

One thing that immediately caught my attention is that the remote IP creates additional overhead on every request even when it's not requested. This should be improved for performance reasons.

Hi hanz, just wanted to let you know that I'm very busy at the moment so I can't do a full review yet. One thing that immediately caught my attention is that the remote IP creates additional overhead on every request even when it's not requested. This should be improved for performance reasons.
refactor: Moved RemoteIP() to Context instead
All checks were successful
/ test (pull_request) Successful in 19s
Details
3928f72f57
hanz commented 2025-10-06 11:51:56 +00:00
Author
First-time contributor
Copy link

Hi @ed thank you for responding

I have pushed a new commit that moved remote_ip from handleConnection to Context.RemoteIP() which will only fetch the data when needed.

Hi @ed thank you for responding I have pushed a new commit that moved `remote_ip` from `handleConnection` to `Context.RemoteIP()` which will only fetch the data when needed.
ed requested changes 2025-10-07 08:22:33 +00:00
ed left a comment
Owner
Copy link

Added a few points to improve (not including test code for now).

Added a few points to improve (not including test code for now).
Context.go Outdated
@ -13,6 +14,7 @@ type Context interface {
Redirect(int, string) error
Request() Request
Response() Response
RemoteIP() string
ed commented 2025-10-07 08:07:56 +00:00
Owner
Copy link

Alphabetical order should be maintained.

Alphabetical order should be maintained.
hanz marked this conversation as resolved
Context.go Outdated
@ -21,6 +23,7 @@ type Context interface {
type context struct {
request
response
conn net.Conn
ed commented 2025-10-07 08:18:54 +00:00
Owner
Copy link

Variable names should have a length that reflects their lifetime.
This is a struct member (long lifetime), so the name should not be shortened (use connection).

Variable names should have a length that reflects their lifetime. This is a struct member (long lifetime), so the name should not be shortened (use `connection`).
hanz marked this conversation as resolved
Context.go Outdated
@ -76,6 +79,15 @@ func (ctx *context) Response() Response {
return &ctx.response
}
func (ctx *context) RemoteIP() string {
ed commented 2025-10-07 08:15:11 +00:00
Owner
Copy link

Even if it feels self-explanatory, a comment here would help follow the style of the other functions.

Even if it feels self-explanatory, a comment here would help follow the style of the other functions.
hanz marked this conversation as resolved
Context.go
@ -77,2 +80,4 @@
}
func (ctx *context) RemoteIP() string {
remote_ip, _, err := net.SplitHostPort(ctx.conn.RemoteAddr().String())
ed commented 2025-10-07 08:08:53 +00:00
Owner
Copy link

This doesn't follow Go's naming standards. Just ip is fine.

This doesn't follow Go's naming standards. Just `ip` is fine.
hanz marked this conversation as resolved
Context.go Outdated
@ -78,1 +81,4 @@
func (ctx *context) RemoteIP() string {
remote_ip, _, err := net.SplitHostPort(ctx.conn.RemoteAddr().String())
if err != nil {
ed commented 2025-10-07 08:10:39 +00:00
Owner
Copy link

Project specific style: Newlines before and after a block (unless it's at function start/end).

Project specific style: Newlines before and after a block (unless it's at function start/end).
hanz marked this conversation as resolved
Context_test.go Outdated
@ -5,0 +6,4 @@
"net"
"net/http"
"os"
os_signal "os/signal"
ed commented 2025-10-07 08:16:34 +00:00
Owner
Copy link

Renaming a package should be avoided unless absolutely necessary.

Renaming a package should be avoided unless absolutely necessary.
hanz marked this conversation as resolved
refactor: Fixed styling
All checks were successful
/ test (pull_request) Successful in 18s
Details
a998f2e2e5
ed requested changes 2025-10-10 12:43:59 +00:00
Context.go
@ -11,6 +12,7 @@ type Context interface {
Handler() Handler
Next(Context) error
Redirect(int, string) error
RemoteIP() string
ed commented 2025-10-10 12:34:37 +00:00
Owner
Copy link

The function name is alphabetically sorted in the interface but the actual function should also appear in a sorted spot.

The function name is alphabetically sorted in the interface but the actual function should also appear in a sorted spot.
Context.go
@ -76,6 +79,17 @@ func (ctx *context) Response() Response {
return &ctx.response
}
// RemoteIP extracts the client's IP address from the connection's remote address.
ed commented 2025-10-10 12:30:50 +00:00
Owner
Copy link

This comment can be misinterpreted. I suggest the following:

// RemoteIP returns the remote IP address. This will return
// the IP address of the endpoint (e.g. a proxy) but not
// necessarily the IP of the client.
This comment can be misinterpreted. I suggest the following: ``` // RemoteIP returns the remote IP address. This will return // the IP address of the endpoint (e.g. a proxy) but not // necessarily the IP of the client. ```
Context.go
@ -78,1 +81,4 @@
// RemoteIP extracts the client's IP address from the connection's remote address.
func (ctx *context) RemoteIP() string {
ip, _, err := net.SplitHostPort(ctx.connection.RemoteAddr().String())
ed commented 2025-10-10 12:36:52 +00:00
Owner
Copy link

Is it possible for net.SplitHostPort to cause an error on its own RemoteAddr?
If the answer is yes, it's probably better to return an error alongside the IP.
if the answer is no, err variable should be removed.

Is it possible for `net.SplitHostPort` to cause an error on its own `RemoteAddr`? If the answer is yes, it's probably better to return an error alongside the IP. if the answer is no, `err` variable should be removed.
Context.go
@ -79,0 +84,4 @@
ip, _, err := net.SplitHostPort(ctx.connection.RemoteAddr().String())
if err != nil {
ip = ""
ed commented 2025-10-10 12:38:48 +00:00
Owner
Copy link

IP is always empty in the error case, so this entire branch effectively does nothing.

IP is always empty in the error case, so this entire branch effectively does nothing.
All checks were successful
/ test (pull_request) Successful in 18s
Details
This pull request can be merged automatically.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u main:hanz-main
git switch hanz-main

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch main
git merge --no-ff hanz-main
git switch hanz-main
git rebase main
git switch main
git merge --ff-only hanz-main
git switch hanz-main
git rebase main
git switch main
git merge --no-ff hanz-main
git switch main
git merge --squash hanz-main
git switch main
git merge --ff-only hanz-main
git switch main
git merge hanz-main
git push origin main
Sign in to join this conversation.
Reviewers
No reviewers
ed
Labels
Clear labels   
bug

Something is not working

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
help wanted

Need some help

  
invalid

Something is wrong

  
question

More information is needed

  
wontfix

This won't be fixed

No labels
bug
duplicate
enhancement
help wanted
invalid
question
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
go/web!10
No description provided.