Allow query parameters for ParseURL

[dmke]

Background

In my Go programs, I usually want to modiffy the database connection options using CLI flags or environment variables. For example, to tune Postgres connections, I can set an environment variable like

PG="postgres://user@localhost:5432/testdb?sslmode=verify-full&connect_timeout=10" ./myprog

This is very handy for containerized applications.

In the Go program, one only needs to pass that string into ParseConfig:

config, err := pgx.ParseConfig(os.Getenv("PG"))
conn, err := pgx.Connect(config)

Issue

With the Go Redis library, this is not so simple:

_, err := redis.ParseURL("redis://localhost/0?dial_timeout=10")
//=> redis: no options supported

Here, the library requires me to either setup and parse additional flags/env vars, or extract the parameters from the URL before passing it to ParseURL.

Implementation details

This PR addresses this issue, by allowing (a subset of) the Option fields to be expressed as query parameters.

I've settled on the following behaviour:

• field names are mapped using snake-case conversion
  • max_retries=2 → opt.MaxRetries == 2
  • pool_fifo=true → opt.PoolFIFO == true
  • mapping happens manually, I did not use the reflect package
• query parameters "network", "addr", "username" and "password" are ignored
  • these are covered by the URL Scheme, Host, UserInfo fields
• only scalar type fields are supported
  • i.e. bool, int, time.Duration
  • string is notably absent, because all Option string fields are already covered (but adding support is trivially easy)
  • struct fields (like opt.TLSConfig) are not supported yet. If they would ever be, someone needs to decide how to represent them as query parameters (cf. tls_config_insecure_skip_verify=true vs. tls_config.insecure_skip_verify=true vs. tls_config[insecure_skip_verify]=true)
  • functions (opt.OnConnect) are not supported, and probably will never be
• for duration fields, values must be a valid input for time.ParseDuration
  • for convenience, plain integers are interpreted as seconds
  • read_timeout=60 == read_timeout=1m → opt.ReadTimeout == time.Minute
• only the last value is interpreted if a parameter is given multiple times
  • redis://.../0?db=1&db=2 → opt.DB == 2
• unknown parameter names will result in an error
  • this is closest to the previous behaviour, which produced an error if any parameter was present

[vmihailenco]

This looks good, we could make code a little bit cleaner:

type queryOptions struct {
    q *url.Values
}

// Err returns the first encountered error.
func (q *queryOptions) Err() error {}
func (q *queryOptions) Int64(name string) (int64, ok) {}

q := queryOptions{q: u.Query()}

if db, ok := q.Int64("db"); ok {}
if maxRetries, ok := q.Int64("max_retries"); ok {}

if err := q.Err(); err != nil {
    return err
}

What do you think?

[dmke]

Great suggestion. I'll start working on that.

[dmke]

@vmihailenco Done.

[dmke]

I have one question left: In the doc string, I've written

• query parameters "network", "addr", "username" and "password" are ignored
  (these are covered by other URL attributes)

However, this code will currently fail:

ParseURL("redis://user@localhost/?username=admin")
//=> redis: unexpected option: username

I feel like, this is the correct behaviour, but it's inconsistent with the way other (duplicate) parameters are handled:

opts, _ := ParseURL("redis://user@localhost/0?db=1")
//=> opts.Database == 1

We've got basically three options here:

1. Follow doc string (requires some delete(q.q, "username") before checking r.remaining()).
2. Yield error (needs update to the doc string).
3. Allow updating opts.Username via query parameters.

@vmihailenco, do you have a preference?

[vmihailenco]

Looks like ?db=123 is an exception needed to support Unix sockets unix://foo:bar@/tmp/redis.sock?db=1. Let's not duplicate other options.

However, this code will currently fail:

I think this code should continue to fail (option #2).

[dmke]

Okay, I'll update the doc string later, I'm currently a bit busy with different things.

[dmke]

@vmihailenco Done.

[vmihailenco]

Thanks - this looks very good 👍 I am going to steal this for https://github.com/uptrace/bun/tree/master/driver/pgdriver configuration :)

[dmke]

Sure, go ahead 😀