fix(rpc): restore plain host:port support for -diodeaddrs#248
Conversation
Fix regression introduced by a07e80f where plain `host:port` values could be normalized to `:`, causing single `-diodeaddrs` connections to fail (`server=:` in logs). - prefer `SplitHostPort` for plain `host:port` input - only URL-parse when a scheme exists and host/port are non-empty - apply the same guard in `resolveRelayAddrFromClient` - add regression tests for both plain and URL address formats - keep `ci_test.sh` on a single diodeaddr (override via `DIODE_CI_DIODEADDR`)
Summary of ChangesHello @tuhalf, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a regression that caused single Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes a regression in address parsing by correctly handling plain host:port strings. The change prioritizes net.SplitHostPort over url.Parse, which resolves the issue. The logic is sound and is accompanied by a good set of regression tests. My review includes suggestions for improving code consistency and maintainability in both the implementation and the new tests.
| if host, port, err := net.SplitHostPort(remoteAddr.String()); err == nil { | ||
| return net.JoinHostPort(host, port), host | ||
| } | ||
| if strings.Contains(remoteAddr.String(), "://") { | ||
| if u, err := url.Parse(remoteAddr.String()); err == nil { | ||
| if host, port := strings.TrimSpace(u.Hostname()), strings.TrimSpace(u.Port()); host != "" && port != "" { | ||
| return net.JoinHostPort(host, port), host | ||
| } | ||
| } | ||
| } | ||
| return remoteAddr.String(), remoteAddr.String() |
There was a problem hiding this comment.
To improve readability and consistency, consider storing the result of remoteAddr.String() in a variable to avoid multiple calls. Additionally, the host should be normalized to lowercase for consistency with the normalizeHostPort function. This will make the code more robust and easier to maintain.
| if host, port, err := net.SplitHostPort(remoteAddr.String()); err == nil { | |
| return net.JoinHostPort(host, port), host | |
| } | |
| if strings.Contains(remoteAddr.String(), "://") { | |
| if u, err := url.Parse(remoteAddr.String()); err == nil { | |
| if host, port := strings.TrimSpace(u.Hostname()), strings.TrimSpace(u.Port()); host != "" && port != "" { | |
| return net.JoinHostPort(host, port), host | |
| } | |
| } | |
| } | |
| return remoteAddr.String(), remoteAddr.String() | |
| addrStr := remoteAddr.String() | |
| if host, port, err := net.SplitHostPort(addrStr); err == nil { | |
| host = strings.ToLower(strings.TrimSpace(host)) | |
| return net.JoinHostPort(host, port), host | |
| } | |
| if strings.Contains(addrStr, "://") { | |
| if u, err := url.Parse(addrStr); err == nil { | |
| if host, port := strings.ToLower(strings.TrimSpace(u.Hostname())), strings.TrimSpace(u.Port()); host != "" && port != "" { | |
| return net.JoinHostPort(host, port), host | |
| } | |
| } | |
| } | |
| return addrStr, addrStr |
| func TestNormalizeHostPortPlainHostPort(t *testing.T) { | ||
| got := normalizeHostPort("eu1.prenet.diode.io:41046") | ||
| if got != "eu1.prenet.diode.io:41046" { | ||
| t.Fatalf("expected plain host:port to remain unchanged, got %q", got) | ||
| } | ||
| } | ||
|
|
||
| func TestNormalizeHostPortFromDiodeURL(t *testing.T) { | ||
| got := normalizeHostPort("diode://0x937c492a77ae90de971986d003ffbc5f8bb2232c@eu1.prenet.diode.io:41046") | ||
| if got != "eu1.prenet.diode.io:41046" { | ||
| t.Fatalf("expected diode URL to normalize to host:port, got %q", got) | ||
| } | ||
| } | ||
|
|
||
| func TestNormalizeHostPortFromHTTPSURL(t *testing.T) { | ||
| got := normalizeHostPort("HTTPS://Example.COM:443") | ||
| if got != "example.com:443" { | ||
| t.Fatalf("expected HTTPS URL to normalize to lowercase host:port, got %q", got) | ||
| } | ||
| } |
There was a problem hiding this comment.
To improve the maintainability and readability of the tests, consider refactoring them into a single table-driven test. This is a common Go idiom that makes it easier to add new test cases in the future and reduces boilerplate code.
func TestNormalizeHostPort(t *testing.T) {
testCases := []struct {
name string
input string
want string
}{
{
name: "Plain host:port",
input: "eu1.prenet.diode.io:41046",
want: "eu1.prenet.diode.io:41046",
},
{
name: "Diode URL",
input: "diode://0x937c492a77ae90de971986d003ffbc5f8bb2232c@eu1.prenet.diode.io:41046",
want: "eu1.prenet.diode.io:41046",
},
{
name: "HTTPS URL with mixed case",
input: "HTTPS://Example.COM:443",
want: "example.com:443",
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
got := normalizeHostPort(tc.input)
if got != tc.want {
t.Errorf("normalizeHostPort(%q) = %q, want %q", tc.input, got, tc.want)
}
})
}
}
dominicletz
deleted the
247-diodeaddrs-hostport-stopped-working-after-a07e80f
branch
2 participants
Fix regression introduced by a07e80f where plain
host:portvalues could be normalized to:, causing single-diodeaddrsconnections to fail (server=:in logs).SplitHostPortfor plainhost:portinputresolveRelayAddrFromClientci_test.shon a single diodeaddr (override viaDIODE_CI_DIODEADDR)