Copilot commented on code in PR #1468:
URL: https://github.com/apache/pulsar-client-go/pull/1468#discussion_r2881790767
##########
pulsar/internal/service_name_resolver.go:
##########
@@ -83,10 +80,10 @@ func (r *pulsarServiceNameResolver) ResolveHostURI()
(*PulsarServiceURI, error)
return NewPulsarServiceURIFromURIString(hostURL)
Review Comment:
`ResolveHostURI` builds `hostURL` using `host.Hostname()` + ":" +
`host.Port()`. For IPv6 addresses, `Hostname()` drops brackets, so this
produces an invalid URL like `pulsar://fec0::1:6650` and will fail parsing
under strict URL rules. Build the URI using `host.Host` (which preserves
brackets) or use `net.JoinHostPort(host.Hostname(), host.Port())` to ensure
correct IPv6 formatting.
##########
pulsar/internal/service_uri.go:
##########
@@ -166,42 +165,69 @@ func splitURI(r rune) bool {
}
func validateHostName(serviceName string, serviceInfos []string, hostname
string) (string, error) {
- uri, err := url.Parse("dummyscheme://" + hostname)
- if err != nil {
- return "", err
- }
- host := uri.Hostname()
- if strings.Contains(hostname, "[") && strings.Contains(hostname, "]") {
- host = fmt.Sprintf("[%s]", host)
- }
- if host == "" || uri.Scheme == "" {
- return "", errors.New("Invalid hostname : " + hostname)
+ // Trim whitespace to avoid accepting accidental invalid inputs
+ hostname = strings.TrimSpace(hostname)
+ if hostname == "" {
+ return "", errors.New("hostname is empty")
}
- port := uri.Port()
- if uri.Port() == "" {
+ var host, port string
+
+ // Attempt to split host and port using the standard library.
+ //
+ // net.SplitHostPort enforces strict host:port syntax:
+ // - IPv4: "127.0.0.1:6650"
+ // - IPv6: "[fec0::1]:6650"
+ //
+ // It will fail for:
+ // - hosts without a port
+ // - bare IPv6 literals without brackets (e.g. "fec0::1")
+ host, port, err := net.SplitHostPort(hostname)
Review Comment:
`validateHostName` accepts inputs like ":6650" because
`net.SplitHostPort(":6650")` succeeds with an empty host. That would allow
service URLs such as `pulsar://:6650/` to be treated as valid. After splitting
host/port, explicitly reject an empty host (and consider adding a test case for
this).
```suggestion
host, port, err := net.SplitHostPort(hostname)
if err == nil && host == "" {
// net.SplitHostPort accepts ":port" with an empty host, but we
explicitly
// reject such inputs because a non-empty hostname is required.
return "", fmt.Errorf("invalid address: host is empty in %q",
hostname)
}
```
##########
pulsar/internal/service_uri.go:
##########
@@ -55,109 +57,106 @@ func NewPulsarServiceURIFromURIString(uri string)
(*PulsarServiceURI, error) {
return u, nil
}
-func NewPulsarServiceURIFromURL(url *url.URL) (*PulsarServiceURI, error) {
- u, err := fromURL(url)
- if err != nil {
- log.Error(err)
- return nil, err
+func (p *PulsarServiceURI) UseTLS() bool {
+ return p.ServiceName == HTTPSService || slices.Contains(p.ServiceInfos,
SSLService)
+}
+
+func (p *PulsarServiceURI) PrimaryHostName() (string, error) {
+ if len(p.ServiceHosts) > 0 {
+ host, _, err := net.SplitHostPort(p.ServiceHosts[0])
+ if err != nil {
+ return "", err
+ }
+ return host, nil
}
- return u, nil
+
+ return "", errors.New("no hosts available in ServiceHosts")
+}
+
+func (p *PulsarServiceURI) IsHTTP() bool {
+ return strings.HasPrefix(p.ServiceName, HTTPService)
Review Comment:
`IsHTTP` currently uses `strings.HasPrefix(p.ServiceName, HTTPService)`,
which makes `https` return true because it starts with "http". This is easy to
misread and could behave unexpectedly if new schemes are added. Prefer an
explicit check (e.g., `serviceName == "http" || serviceName == "https"`) or
rename the method to reflect that it includes HTTPS.
```suggestion
return p.ServiceName == HTTPService || p.ServiceName == HTTPSService
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
