vikunja
/
vikunja
6
43
Fork 72
Code Issues 150 Pull Requests 19 Packages Releases 32 Activity

Add postgres support #135

Merged
konrad merged 26 commits from jtojnar/api:pgsql into master 2020-02-16 21:42:05 +00:00
Conversation 12 Commits 26 Files Changed 107 +35 -22
1 changed files with 35 additions and 22 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 53a6297808 - Show all commits

57
pkg/db/db.go
View File

@ -21,6 +21,7 @@ import (
"code.vikunja.io/api/pkg/log"
"encoding/gob"
"fmt"
"net/url"
"strconv"
"strings"
"time"
@ -120,31 +121,43 @@ func initMysqlEngine() (engine *xorm.Engine, err error) {
return
}
jtojnar commented 2020-02-15 10:16:02 +00:00
Outdated
Review
Copy Link

This function is super ugly but I am not sure if there is a better way to do this. Maybe iterating over

params :=map[string]string{
    "user":      config.DatabaseUser.GetString(),
    "password":  config.DatabasePassword.GetString(),
    "host":      config.DatabaseHost.GetString(),
    "dbname":    config.DatabaseDatabase.GetString(),
}
This function is super ugly but I am not sure if there is a better way to do this. Maybe iterating over ``` params :=map[string]string{ "user": config.DatabaseUser.GetString(), "password": config.DatabasePassword.GetString(), "host": config.DatabaseHost.GetString(), "dbname": config.DatabaseDatabase.GetString(), } ```
konrad commented 2020-02-15 10:26:40 +00:00
Outdated
Review
Copy Link

A much cleaner way would be to use something like

connStr := fmt.Sprintf("user=%s password=% dbname=%s host=%s", config.DatabaseUser.GetString(), config.DatabasePassword.GetString(), config.DatabaseDatabase.GetString(), config.DatabaseHost.GetString())
A much cleaner way would be to use something like ```golang connStr := fmt.Sprintf("user=%s password=% dbname=%s host=%s", config.DatabaseUser.GetString(), config.DatabasePassword.GetString(), config.DatabaseDatabase.GetString(), config.DatabaseHost.GetString()) ```
jtojnar commented 2020-02-15 10:29:56 +00:00
Outdated
Review
Copy Link

Well, we still want to be able to not pass some of those keys. For example, I connect to the database via socket so username and password does not make sense.

Well, we still want to be able to not pass some of those keys. For example, I connect to the database via socket so username and password does not make sense.
konrad commented 2020-02-15 11:18:20 +00:00
Outdated
Review
Copy Link

I think we could use something like this: ad1b6d439f/modules/setting/database.go (L152-L162)

I think we could use something like this: https://github.com/go-gitea/gitea/blob/ad1b6d439fe0e0875e54227e0bc23a74411f490e/modules/setting/database.go#L152-L162
// parsePostgreSQLHostPort parses given input in various forms defined in
// https://www.postgresql.org/docs/current/static/libpq-connect.html#LIBPQ-CONNSTRING
// and returns proper host and port number.
func parsePostgreSQLHostPort(info string) (string, string) {
host, port := "127.0.0.1", "5432"
if strings.Contains(info, ":") && !strings.HasSuffix(info, "]") {
idx := strings.LastIndex(info, ":")
host = info[:idx]
port = info[idx+1:]
} else if len(info) > 0 {
host = info
}
return host, port
}
jtojnar commented 2020-02-15 07:50:13 +00:00
Outdated
Review
Copy Link

We will probably need to allow passing the rest of the options. The Go library recommended by xorm decided to stray from libpq’s defaults and forces sslmode=required, which fails on my laptop where I do not have TLS set up.

Or maybe we can just accept raw connection string?

We will probably need to allow passing the rest of the options. The Go library recommended by xorm decided to stray from libpq’s defaults and forces `sslmode=required`, which fails on my laptop where I do not have TLS set up. Or maybe we can just accept raw connection string?
konrad commented 2020-02-15 10:10:48 +00:00
Outdated
Review
Copy Link

I'd say we probably should add a setting for sslmode.

I'd say we probably should add a setting for `sslmode`.
func initPostgresEngine() (engine *xorm.Engine, err error) {
var connStr strings.Builder
// https://pkg.go.dev/github.com/lib/pq?tab=doc#hdr-Connection_String_Parameters
params := map[string]string{
"user": config.DatabaseUser.GetString(),
"password": config.DatabasePassword.GetString(),
"host": config.DatabaseHost.GetString(),
"dbname": config.DatabaseDatabase.GetString(),
"sslmode": config.DatabaseSslMode.GetString(),
var connStr string
host, port := parsePostgreSQLHostPort(config.DatabaseHost.GetString())
if strings.HasPrefix(config.DatabaseHost.GetString(), "/") { // looks like a unix socket
connStr = fmt.Sprintf("postgres://%s:%s@:%s/%ssslmode=%s&host=%s",
jtojnar commented 2020-02-16 13:48:03 +00:00
Outdated
Review
Copy Link

Why not just use the attr=name format instead of URI format? Or use this one for both UNIX socket and TCP connection? This only needs to be used for UNIX sockets because URI hostnames cannot contain / but the other way around is fine.

Why not just use the `attr=name` format instead of URI format? Or use this one for both UNIX socket and TCP connection? This only needs to be used for UNIX sockets because URI hostnames cannot contain `/` but the other way around is fine.
konrad commented 2020-02-16 14:07:22 +00:00
Outdated
Review
Copy Link

Good point. What do think would be better suited, using the same for both or using the attr=name format?

Good point. What do think would be better suited, using the same for both or using the `attr=name` format?
jtojnar commented 2020-02-16 16:54:48 +00:00
Outdated
Review
Copy Link

I think attr=%s is clearer.

I think `attr=%s` is clearer.
konrad commented 2020-02-16 16:56:38 +00:00
Outdated
Review
Copy Link

Then lets change it.

Then lets change it.
url.PathEscape(config.DatabaseUser.GetString()),
url.PathEscape(config.DatabasePassword.GetString()),
port,
config.DatabaseDatabase.GetString(),
config.DatabaseSslMode.GetString(),
jtojnar commented 2020-02-15 07:16:34 +00:00
Outdated
Review
Copy Link

I am not sure if these flags are needed.

I am not sure if these flags are needed.
jtojnar commented 2020-02-15 07:51:29 +00:00
Outdated
Review
Copy Link

I am not sure if these flags are needed.

I am not sure if these flags are needed.
konrad commented 2020-02-15 10:08:36 +00:00
Outdated
Review
Copy Link

I'd say yes, since the max number of open connections can be a limiting factor for performance.

I'd say yes, since the max number of open connections can be a limiting factor for performance.
host)
} else {
connStr = fmt.Sprintf("postgres://%s:%s@%s:%s/%ssslmode=%s",
url.PathEscape(config.DatabaseUser.GetString()),
url.PathEscape(config.DatabasePassword.GetString()),
host,
port,
config.DatabaseDatabase.GetString(),
config.DatabaseSslMode.GetString())
}
for name, value := range params {
if name != "" {
value = strings.ReplaceAll(value, "\\", "\\\\")
value = strings.ReplaceAll(value, "'", "\\'")
if connStr.Len() > 0 {
connStr.WriteString(" ")
}
connStr.WriteString(fmt.Sprintf("%s='%s'", name, value))
}
}
engine, err = xorm.NewEngine("postgres", connStr.String())
engine, err = xorm.NewEngine("postgres", connStr)
if err != nil {
return
}