Copilot commented on code in PR #1468:
URL: https://github.com/apache/pulsar-client-go/pull/1468#discussion_r2963442362
##########
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)
}
func fromString(uriStr string) (*PulsarServiceURI, error) {
- if uriStr == "" || len(uriStr) == 0 {
- return nil, errors.New("service uriStr string is null")
+ if uriStr == "" {
+ return nil, errors.New("service URI cannot be empty")
}
- if strings.Contains(uriStr, "[") && strings.Contains(uriStr, "]") {
- // deal with ipv6 address
- hosts := strings.FieldsFunc(uriStr, splitURI)
- if len(hosts) > 1 {
- // deal with ipv6 address
- firstHost := hosts[0]
- lastHost := hosts[len(hosts)-1]
- hasPath := strings.Contains(lastHost, "/")
- path := ""
- if hasPath {
- idx := strings.Index(lastHost, "/")
- path = lastHost[idx:]
- }
- firstHost += path
- url, err := url.Parse(firstHost)
- if err != nil {
- return nil, err
- }
- serviceURI, err := fromURL(url)
- if err != nil {
- return nil, err
- }
- var mHosts []string
- var multiHosts []string
- mHosts = append(mHosts, serviceURI.ServiceHosts[0])
- mHosts = append(mHosts, hosts[1:]...)
-
- for _, v := range mHosts {
- h, err :=
validateHostName(serviceURI.ServiceName, serviceURI.ServiceInfos, v)
- if err == nil {
- multiHosts = append(multiHosts, h)
- } else {
- return nil, err
- }
- }
- return &PulsarServiceURI{
- serviceURI.ServiceName,
- serviceURI.ServiceInfos,
- multiHosts,
- serviceURI.servicePath,
- serviceURI.URL,
- }, nil
+ // 1. Find first host delimiter (, or ;)
+ firstDelimIdx := strings.IndexFunc(uriStr, splitURI)
+
+ var singleHostURI string
+ var additionalHosts string
+
+ if firstDelimIdx >= 0 {
+ remainder := uriStr[firstDelimIdx:]
+ endIdx := strings.IndexAny(remainder, "/?#")
Review Comment:
fromString() uses strings.IndexFunc(uriStr, splitURI) to detect multi-host
delimiters, but this scans the *entire* URI, so a ',' or ';' in the
path/query/fragment would be misinterpreted as a host delimiter and break
parsing. The delimiter search should be limited to the authority/host portion
(between "//" and the first "/?#").
##########
pulsar/internal/service_name_resolver_test.go:
##########
@@ -22,52 +22,53 @@ import (
"testing"
"github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
)
func TestResolveBeforeUpdateServiceUrl(t *testing.T) {
- resolver := NewPulsarServiceNameResolver(nil)
+ resolver, err := NewPulsarServiceNameResolver("")
+ require.NoError(t, err)
u, err := resolver.ResolveHost()
assert.Nil(t, u)
assert.NotNil(t, err)
assert.EqualError(t, err, "no service url is provided yet")
}
-func TestResolveUriBeforeUpdateServiceUrl(t *testing.T) {
- resolver := NewPulsarServiceNameResolver(nil)
- u, err := resolver.ResolveHostURI()
+func TestResolveBeforeUpdateServiceUrlReturnsErrorWithoutServiceURL(t
*testing.T) {
+ resolver, err := NewPulsarServiceNameResolver("")
+ require.NoError(t, err)
+ u, err := resolver.ResolveHost()
assert.Nil(t, u)
assert.NotNil(t, err)
assert.EqualError(t, err, "no service url is provided yet")
}
Review Comment:
TestResolveBeforeUpdateServiceUrl and
TestResolveBeforeUpdateServiceUrlReturnsErrorWithoutServiceURL are currently
identical (both call ResolveHost without setting a service URL and assert the
same error). Keeping both provides no additional coverage; consider removing
one or changing one to cover a distinct behavior.
##########
pulsar/client_impl.go:
##########
@@ -80,31 +79,31 @@ func newClient(options ClientOptions) (Client, error) {
return nil, newError(InvalidConfiguration, "URL is required for
client")
}
- url, err := url.Parse(options.URL)
+ pulsarServiceURI, err :=
internal.NewPulsarServiceURIFromURIString(options.URL)
if err != nil {
logger.WithError(err).Error("Failed to parse service URL")
return nil, newError(InvalidConfiguration, "Invalid service
URL")
}
Review Comment:
newClient() used to return a specific InvalidConfiguration error for
unsupported schemes (e.g., "Invalid URL scheme 'ftp'"). After switching to
NewPulsarServiceURIFromURIString, unsupported schemes now fall into the generic
"Invalid service URL" path, which is less actionable for users. Consider
preserving the explicit scheme validation/error (e.g., by extracting the scheme
before parsing hosts) so callers still get a clear unsupported-scheme message.
##########
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)
+ 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)
+ 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)
}
- port := uri.Port()
- if uri.Port() == "" {
+ if err != nil {
+ // If the hostname contains ':' but is not bracketed, it is
very likely
+ // an invalid IPv6 literal or an invalid host with too many
colons.
+ //
+ // Examples rejected here:
+ // - "fec0::1"
+ // - "fec0::1:6650"
+ // - "localhost:6650:6651"
+ if strings.Count(hostname, ":") > 0 &&
!strings.HasPrefix(hostname, "[") {
+ return "", fmt.Errorf("invalid address (maybe missing
brackets for IPv6 or too many colons): %s", hostname)
+ }
+
+ // Otherwise, treat the entire hostname as host without an
explicit port.
+ // In this case, we fall back to the default port derived from
the service.
+ host = hostname
p := getServicePort(serviceName, serviceInfos)
if p == -1 {
- return "", fmt.Errorf("invalid port : %d", p)
+ return "", fmt.Errorf("no port found")
}
- port = fmt.Sprint(p)
+ port = strconv.Itoa(p)
}
- result := host + ":" + port
- _, _, err = net.SplitHostPort(result)
- if err != nil {
- return "", err
+
+ // Remove surrounding brackets if present.
+ //
+ // Note:
+ // "[fec0::1]" is NOT an IPv6 address itself, but a URI/host syntax
+ // representation used to disambiguate IPv6 literals when ports are
involved.
+ // We strip brackets here so the address can be validated and
normalized.
+ cleanHost := host
+ if len(host) >= 2 && host[0] == '[' && host[len(host)-1] == ']' {
+ cleanHost = host[1 : len(host)-1]
}
- return result, nil
+
+ return net.JoinHostPort(cleanHost, port), nil
Review Comment:
validateHostName() strips brackets and then returns
net.JoinHostPort(cleanHost, port) without validating that a bracketed host is
actually an IPv6 literal. As a result, inputs like "pulsar://[example]:6650"
will be accepted (and normalized to "example:6650"), contradicting the new
invalid-URI test case. Add an explicit check that any host provided in brackets
parses as IPv6 (optionally handling zone IDs), and reject bracketed non-IPv6
hosts.
--
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]
