refactor(db): extract testable ResolveDatabasePath function (#2193)
Add DatabasePathConfig struct and ResolveDatabasePath function that takes all dependencies as parameters, making it easier to test path resolution logic in isolation. Should also fix the reported cases. Resolves #2189
This commit is contained in:
kolaente
GitHub
parent
8830dc56ad
commit
7fce4694fa
102
pkg/db/db.go
102
pkg/db/db.go
|
|
@ -185,24 +185,74 @@ func initPostgresEngine() (engine *xorm.Engine, err error) {
|
|||
return
|
||||
}
|
||||
|
||||
// DatabasePathConfig holds configuration for database path resolution.
|
||||
// This struct allows the path resolution logic to be tested independently
|
||||
// of the global config package.
|
||||
type DatabasePathConfig struct {
|
||||
ConfiguredPath string // The database.path config value
|
||||
RootPath string // The service.rootpath config value
|
||||
ExecutablePath string // Directory of the executable binary
|
||||
}
|
||||
|
||||
// resolveDatabasePath resolves a database path configuration to an absolute path.
|
||||
//
|
||||
// Resolution rules:
|
||||
// 1. If ConfiguredPath is "memory", returns "memory" (special case for in-memory DB)
|
||||
// 2. If ConfiguredPath is already absolute, returns it as-is (cleaned)
|
||||
// 3. If ConfiguredPath is relative:
|
||||
// a. If RootPath differs from ExecutablePath (explicitly configured),
|
||||
// joins with RootPath
|
||||
// b. Otherwise, joins with platform-specific user data directory
|
||||
//
|
||||
// The getUserDataDir parameter allows injecting a mock for testing.
|
||||
func resolveDatabasePath(cfg DatabasePathConfig, getUserDataDir func() (string, error)) (string, error) {
|
||||
if cfg.ConfiguredPath == "memory" {
|
||||
return "memory", nil
|
||||
}
|
||||
|
||||
var path string
|
||||
|
||||
switch {
|
||||
case filepath.IsAbs(cfg.ConfiguredPath):
|
||||
path = filepath.Clean(cfg.ConfiguredPath)
|
||||
case cfg.RootPath != cfg.ExecutablePath:
|
||||
path = filepath.Join(cfg.RootPath, cfg.ConfiguredPath)
|
||||
default:
|
||||
dataDir, err := getUserDataDir()
|
||||
if err != nil {
|
||||
log.Warningf("Could not get user data directory, falling back to rootpath: %v", err)
|
||||
path = filepath.Join(cfg.RootPath, cfg.ConfiguredPath)
|
||||
} else {
|
||||
path = filepath.Join(dataDir, cfg.ConfiguredPath)
|
||||
}
|
||||
}
|
||||
|
||||
return filepath.Abs(path)
|
||||
}
|
||||
|
||||
func initSqliteEngine() (engine *xorm.Engine, err error) {
|
||||
path := config.DatabasePath.GetString()
|
||||
rootPath := config.ServiceRootpath.GetString()
|
||||
|
||||
executablePath := rootPath
|
||||
if execPath, err := os.Executable(); err == nil {
|
||||
executablePath = filepath.Dir(execPath)
|
||||
}
|
||||
|
||||
cfg := DatabasePathConfig{
|
||||
ConfiguredPath: config.DatabasePath.GetString(),
|
||||
RootPath: rootPath,
|
||||
ExecutablePath: executablePath,
|
||||
}
|
||||
|
||||
path, err := resolveDatabasePath(cfg, getUserDataDir)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("could not resolve database path: %w", err)
|
||||
}
|
||||
|
||||
if path == "memory" {
|
||||
return xorm.NewEngine("sqlite3", "file::memory:?cache=shared")
|
||||
}
|
||||
|
||||
// Resolve relative paths to a safe location instead of the current working directory
|
||||
if !filepath.IsAbs(path) {
|
||||
path = resolveDatabasePath(path)
|
||||
}
|
||||
|
||||
// Convert to absolute path for logging
|
||||
path, err = filepath.Abs(path)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("could not resolve database path to absolute path: %w", err)
|
||||
}
|
||||
|
||||
// Log the resolved database path
|
||||
log.Infof("Using SQLite database at: %s", path)
|
||||
|
||||
|
|
@ -229,34 +279,6 @@ func initSqliteEngine() (engine *xorm.Engine, err error) {
|
|||
return xorm.NewEngine("sqlite3", path)
|
||||
}
|
||||
|
||||
// resolveDatabasePath resolves a relative database path to an appropriate location
|
||||
// based on platform-specific conventions. This prevents SQLite databases from being
|
||||
// created in system directories (like C:\Windows\System32 on Windows) when Vikunja
|
||||
// runs as a service.
|
||||
func resolveDatabasePath(relativePath string) string {
|
||||
// First, check if a rootpath is explicitly configured and use that
|
||||
rootPath := config.ServiceRootpath.GetString()
|
||||
|
||||
// Determine if rootpath appears to be a user-configured value or a default
|
||||
// If it looks like it was explicitly configured (not just the binary location),
|
||||
// use it for backward compatibility
|
||||
execPath, err := os.Executable()
|
||||
if err == nil && rootPath != filepath.Dir(execPath) {
|
||||
// rootpath was explicitly configured, use it
|
||||
return filepath.Join(rootPath, relativePath)
|
||||
}
|
||||
|
||||
// Otherwise, use a platform-appropriate user data directory
|
||||
dataDir, err := getUserDataDir()
|
||||
if err != nil {
|
||||
// Fall back to rootpath if we can't determine user data dir
|
||||
log.Warningf("Could not determine user data directory, falling back to rootpath: %v", err)
|
||||
return filepath.Join(rootPath, relativePath)
|
||||
}
|
||||
|
||||
return filepath.Join(dataDir, relativePath)
|
||||
}
|
||||
|
||||
// getUserDataDir returns the platform-appropriate directory for application data
|
||||
func getUserDataDir() (string, error) {
|
||||
var dataDir string
|
||||
|
|
|
|||
|
|
@ -17,76 +17,285 @@
|
|||
package db
|
||||
|
||||
import (
|
||||
"fmt"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"runtime"
|
||||
"testing"
|
||||
|
||||
"code.vikunja.io/api/pkg/config"
|
||||
"code.vikunja.io/api/pkg/log"
|
||||
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
func TestResolveDatabasePath(t *testing.T) {
|
||||
// Save original config values
|
||||
originalRootpath := config.ServiceRootpath.GetString()
|
||||
defer config.ServiceRootpath.Set(originalRootpath)
|
||||
func TestMain(m *testing.M) {
|
||||
log.InitLogger()
|
||||
os.Exit(m.Run())
|
||||
}
|
||||
|
||||
func Test_resolveDatabasePath(t *testing.T) {
|
||||
mockGetUserDataDir := func(path string) func() (string, error) {
|
||||
return func() (string, error) {
|
||||
return path, nil
|
||||
}
|
||||
}
|
||||
|
||||
mockGetUserDataDirError := func() (string, error) {
|
||||
return "", fmt.Errorf("no home directory")
|
||||
}
|
||||
|
||||
tests := []struct {
|
||||
name string
|
||||
cfg DatabasePathConfig
|
||||
getUserDataDir func() (string, error)
|
||||
expected string
|
||||
expectError bool
|
||||
}{
|
||||
{
|
||||
name: "memory database",
|
||||
cfg: DatabasePathConfig{
|
||||
ConfiguredPath: "memory",
|
||||
RootPath: "/opt/vikunja",
|
||||
ExecutablePath: "/opt/vikunja",
|
||||
},
|
||||
getUserDataDir: mockGetUserDataDir("/home/user/.local/share/vikunja"),
|
||||
expected: "memory",
|
||||
},
|
||||
|
||||
{
|
||||
name: "absolute path should be used as-is",
|
||||
cfg: DatabasePathConfig{
|
||||
ConfiguredPath: "/var/lib/vikunja/vikunja.db",
|
||||
RootPath: "/opt/vikunja",
|
||||
ExecutablePath: "/opt/vikunja",
|
||||
},
|
||||
getUserDataDir: mockGetUserDataDir("/home/user/.local/share/vikunja"),
|
||||
expected: "/var/lib/vikunja/vikunja.db",
|
||||
},
|
||||
{
|
||||
name: "absolute path with different rootpath still used as-is",
|
||||
cfg: DatabasePathConfig{
|
||||
ConfiguredPath: "/data/mydb.db",
|
||||
RootPath: "/custom/path",
|
||||
ExecutablePath: "/opt/vikunja",
|
||||
},
|
||||
getUserDataDir: mockGetUserDataDir("/home/user/.local/share/vikunja"),
|
||||
expected: "/data/mydb.db",
|
||||
},
|
||||
|
||||
{
|
||||
name: "relative path with explicit rootpath",
|
||||
cfg: DatabasePathConfig{
|
||||
ConfiguredPath: "vikunja.db",
|
||||
RootPath: "/var/lib/vikunja",
|
||||
ExecutablePath: "/opt/vikunja",
|
||||
},
|
||||
getUserDataDir: mockGetUserDataDir("/home/user/.local/share/vikunja"),
|
||||
expected: "/var/lib/vikunja/vikunja.db",
|
||||
},
|
||||
{
|
||||
name: "relative subdirectory path with explicit rootpath",
|
||||
cfg: DatabasePathConfig{
|
||||
ConfiguredPath: "data/vikunja.db",
|
||||
RootPath: "/var/lib/vikunja",
|
||||
ExecutablePath: "/opt/vikunja",
|
||||
},
|
||||
getUserDataDir: mockGetUserDataDir("/home/user/.local/share/vikunja"),
|
||||
expected: "/var/lib/vikunja/data/vikunja.db",
|
||||
},
|
||||
|
||||
{
|
||||
name: "relative path with default rootpath uses user data dir",
|
||||
cfg: DatabasePathConfig{
|
||||
ConfiguredPath: "vikunja.db",
|
||||
RootPath: "/opt/vikunja",
|
||||
ExecutablePath: "/opt/vikunja",
|
||||
},
|
||||
getUserDataDir: mockGetUserDataDir("/home/user/.local/share/vikunja"),
|
||||
expected: "/home/user/.local/share/vikunja/vikunja.db",
|
||||
},
|
||||
|
||||
{
|
||||
name: "os.Executable failure falls back to user data dir",
|
||||
cfg: DatabasePathConfig{
|
||||
ConfiguredPath: "vikunja.db",
|
||||
RootPath: "/opt/vikunja",
|
||||
ExecutablePath: "/opt/vikunja",
|
||||
},
|
||||
getUserDataDir: mockGetUserDataDir("/home/user/.local/share/vikunja"),
|
||||
expected: "/home/user/.local/share/vikunja/vikunja.db",
|
||||
},
|
||||
|
||||
{
|
||||
name: "falls back to rootpath when getUserDataDir fails",
|
||||
cfg: DatabasePathConfig{
|
||||
ConfiguredPath: "vikunja.db",
|
||||
RootPath: "/opt/vikunja",
|
||||
ExecutablePath: "/opt/vikunja",
|
||||
},
|
||||
getUserDataDir: mockGetUserDataDirError,
|
||||
expected: "/opt/vikunja/vikunja.db",
|
||||
},
|
||||
|
||||
{
|
||||
name: "empty configured path with explicit rootpath",
|
||||
cfg: DatabasePathConfig{
|
||||
ConfiguredPath: "",
|
||||
RootPath: "/var/lib/vikunja",
|
||||
ExecutablePath: "/opt/vikunja",
|
||||
},
|
||||
getUserDataDir: mockGetUserDataDir("/home/user/.local/share/vikunja"),
|
||||
expected: "/var/lib/vikunja",
|
||||
},
|
||||
{
|
||||
name: "empty configured path with default rootpath",
|
||||
cfg: DatabasePathConfig{
|
||||
ConfiguredPath: "",
|
||||
RootPath: "/opt/vikunja",
|
||||
ExecutablePath: "/opt/vikunja",
|
||||
},
|
||||
getUserDataDir: mockGetUserDataDir("/home/user/.local/share/vikunja"),
|
||||
expected: "/home/user/.local/share/vikunja",
|
||||
},
|
||||
{
|
||||
name: "path with dots normalized",
|
||||
cfg: DatabasePathConfig{
|
||||
ConfiguredPath: "/var/lib/vikunja/../vikunja/./db.db",
|
||||
RootPath: "/opt/vikunja",
|
||||
ExecutablePath: "/opt/vikunja",
|
||||
},
|
||||
getUserDataDir: mockGetUserDataDir("/home/user/.local/share/vikunja"),
|
||||
expected: "/var/lib/vikunja/db.db",
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
result, err := resolveDatabasePath(tt.cfg, tt.getUserDataDir)
|
||||
|
||||
if tt.expectError {
|
||||
require.Error(t, err)
|
||||
return
|
||||
}
|
||||
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, tt.expected, result)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func Test_resolveDatabasePath_Integration(t *testing.T) {
|
||||
t.Run("with explicitly configured rootpath", func(t *testing.T) {
|
||||
// Set a rootpath that is different from the executable location
|
||||
config.ServiceRootpath.Set("/custom/path")
|
||||
cfg := DatabasePathConfig{
|
||||
ConfiguredPath: "vikunja.db",
|
||||
RootPath: "/custom/path",
|
||||
ExecutablePath: "/opt/vikunja",
|
||||
}
|
||||
|
||||
result, err := resolveDatabasePath(cfg, getUserDataDir)
|
||||
require.NoError(t, err)
|
||||
|
||||
result := resolveDatabasePath("vikunja.db")
|
||||
expected := filepath.Join("/custom/path", "vikunja.db")
|
||||
|
||||
assert.Equal(t, expected, result)
|
||||
})
|
||||
|
||||
t.Run("with default rootpath uses user data directory", func(t *testing.T) {
|
||||
// Get the actual executable path and set rootpath to it
|
||||
execPath, err := os.Executable()
|
||||
require.NoError(t, err)
|
||||
config.ServiceRootpath.Set(filepath.Dir(execPath))
|
||||
execDir := filepath.Dir(execPath)
|
||||
|
||||
result := resolveDatabasePath("vikunja.db")
|
||||
cfg := DatabasePathConfig{
|
||||
ConfiguredPath: "vikunja.db",
|
||||
RootPath: execDir,
|
||||
ExecutablePath: execDir,
|
||||
}
|
||||
|
||||
// Result should contain the platform-specific user data directory
|
||||
// and not be in the executable directory
|
||||
assert.NotEqual(t, filepath.Join(filepath.Dir(execPath), "vikunja.db"), result)
|
||||
result, err := resolveDatabasePath(cfg, getUserDataDir)
|
||||
require.NoError(t, err)
|
||||
|
||||
assert.NotEqual(t, filepath.Join(execDir, "vikunja.db"), result)
|
||||
assert.Contains(t, result, "vikunja.db")
|
||||
|
||||
// Verify it's using a platform-appropriate path
|
||||
switch runtime.GOOS {
|
||||
case "windows":
|
||||
// Should be in %LOCALAPPDATA%\Vikunja or %USERPROFILE%\AppData\Local\Vikunja
|
||||
assert.Contains(t, result, "Vikunja")
|
||||
case "darwin":
|
||||
// Should be in ~/Library/Application Support/Vikunja
|
||||
assert.Contains(t, result, "Library")
|
||||
assert.Contains(t, result, "Application Support")
|
||||
default:
|
||||
// Should be in ~/.local/share/vikunja or $XDG_DATA_HOME/vikunja
|
||||
assert.NotEqual(t,
|
||||
filepath.Dir(result),
|
||||
filepath.Dir(execPath),
|
||||
execDir,
|
||||
"Database should not be in executable directory",
|
||||
)
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("with subdirectory path", func(t *testing.T) {
|
||||
config.ServiceRootpath.Set("/custom/path")
|
||||
cfg := DatabasePathConfig{
|
||||
ConfiguredPath: "data/vikunja.db",
|
||||
RootPath: "/custom/path",
|
||||
ExecutablePath: "/opt/vikunja",
|
||||
}
|
||||
|
||||
result, err := resolveDatabasePath(cfg, getUserDataDir)
|
||||
require.NoError(t, err)
|
||||
|
||||
result := resolveDatabasePath("data/vikunja.db")
|
||||
expected := filepath.Join("/custom/path", "data", "vikunja.db")
|
||||
|
||||
assert.Equal(t, expected, result)
|
||||
})
|
||||
}
|
||||
|
||||
func TestGetUserDataDir(t *testing.T) {
|
||||
func Test_resolveDatabasePath_Windows(t *testing.T) {
|
||||
if runtime.GOOS != "windows" {
|
||||
t.Skip("Skipping Windows-specific test on non-Windows platform")
|
||||
}
|
||||
|
||||
mockGetUserDataDir := func(path string) func() (string, error) {
|
||||
return func() (string, error) {
|
||||
return path, nil
|
||||
}
|
||||
}
|
||||
|
||||
tests := []struct {
|
||||
name string
|
||||
cfg DatabasePathConfig
|
||||
getUserDataDir func() (string, error)
|
||||
expected string
|
||||
}{
|
||||
{
|
||||
name: "windows absolute path",
|
||||
cfg: DatabasePathConfig{
|
||||
ConfiguredPath: "C:\\ProgramData\\Vikunja\\vikunja.db",
|
||||
RootPath: "C:\\Program Files\\Vikunja",
|
||||
ExecutablePath: "C:\\Program Files\\Vikunja",
|
||||
},
|
||||
getUserDataDir: mockGetUserDataDir("C:\\Users\\test\\AppData\\Local\\Vikunja"),
|
||||
expected: "C:\\ProgramData\\Vikunja\\vikunja.db",
|
||||
},
|
||||
{
|
||||
name: "windows relative path with explicit rootpath",
|
||||
cfg: DatabasePathConfig{
|
||||
ConfiguredPath: "vikunja.db",
|
||||
RootPath: "C:\\ProgramData\\Vikunja",
|
||||
ExecutablePath: "C:\\Program Files\\Vikunja",
|
||||
},
|
||||
getUserDataDir: mockGetUserDataDir("C:\\Users\\test\\AppData\\Local\\Vikunja"),
|
||||
expected: "C:\\ProgramData\\Vikunja\\vikunja.db",
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
result, err := resolveDatabasePath(tt.cfg, tt.getUserDataDir)
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, tt.expected, result)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestGetUserDataDir(t *testing.T) {
|
||||
test := func() string {
|
||||
dataDir, err := getUserDataDir()
|
||||
require.NoError(t, err)
|
||||
|
|
|
|||
Loading…
Reference in New Issue