GODRIVER-3092 Fix UNIX socket in URL parsing. #1521
Conversation
qingyang-hu
requested review from
blink1073,
matthewdale and
prestonvasquez
and removed request for
a team and
blink1073
API Change Report./x/mongo/driver/connstringincompatible changesParse: changed from func(string) (ConnString, error) to func(string) (*ConnString, error) compatible changesConnString.RawHosts: added |
qingyang-hu
force-pushed
the
godriver3092
branch
from
8f63170 to
1af1bb2
Compare
bson/encoder_example_test.go
Outdated
| @@ -248,7 +248,7 @@ func ExampleEncoder_IntMinSize() { | |||
| panic(err) | |||
| } | |||
|
|
|||
| enc, err := bson.NewEncoder(vw) | |||
| enc := bson.NewEncoder(vw) | |||
There was a problem hiding this comment.
To fix the build.
| @@ -237,7 +237,6 @@ type ClientOptions struct { | |||
| ZstdLevel *int | |||
|
|
|||
| err error | |||
| uri string | |||
| cs *connstring.ConnString | |||
There was a problem hiding this comment.
Remove the uri so the ConnString.Original is the single point of truth.
qingyang-hu
force-pushed
the
godriver3092
branch
from
ec75183 to
655c007
Compare
qingyang-hu
changed the title
| func (u ConnString) String() string { | ||
| return u.Original | ||
| } |
There was a problem hiding this comment.
The ConnString struct consumes almost 1kb per copy. We should always be passing it by pointer for that reason (including the return value of Parse and ParseAndValidate).
| } | ||
| host, err := url.QueryUnescape(host) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid host %q: %w", host, err) | ||
| return host, fmt.Errorf("invalid host %q: %w", host, err) |
There was a problem hiding this comment.
Go functions conventionally return the zero value if returning an error. Is there a reason to return host instead of ""?
| host, err := url.QueryUnescape(host) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid host %q: %w", host, err) | ||
| return host, fmt.Errorf("invalid host %q: %w", host, err) |
There was a problem hiding this comment.
url.QueryUnescape returns empty string on error. As a result, the error returned here will always have an empty host string. What we want instead is the original, invalid host value.
E.g.
unescaped, err := url.QueryUnescape(host)
if err != nil {
return "", fmt.Errorf("error query unescaping host %q: %w", host, err)
mongo/options/clientoptions.go
Outdated
| func (c *ClientOptions) GetRawHosts() []string { | ||
| if c.cs == nil { | ||
| return nil | ||
| } | ||
| return c.cs.RawHosts | ||
| } | ||
|
|
||
| // GetScheme returns the scheme. If ApplyURI was not called during construction, this returns "". | ||
| func (c *ClientOptions) GetScheme() string { |
There was a problem hiding this comment.
I'm concerned about exposing these values piecemeal in the ClientOptions API. The pattern isn't sustainable if we need to continue to expose more connection string or other config data to users.
Is there another way to pass the data to topology.New without having to add methods to ClientOptions?
qingyang-hu
force-pushed
the
godriver3092
branch
from
63de888 to
31e2a67
Compare
qingyang-hu
force-pushed
the
godriver3092
branch
from
31e2a67 to
b3dc788
Compare
GODRIVER-3092
Summary
url.Parse()call.ConnStringBackground & Motivation
Add
RawHoststoConnString, so we can initialTopologywith scheme and hosts parsed byConnStringfrom the URI.The
RawHostsis directly parsed from the URI string before SRV lookup.