From edb65637717d02778615843ad5a770f6716af4eb Mon Sep 17 00:00:00 2001 From: Alexey Palazhchenko Date: Thu, 20 Jul 2023 19:51:30 +0400 Subject: [PATCH] Allow to change SQLite URI in tests --- Taskfile.yml | 1 + integration/Taskfile.yml | 3 + integration/setup/listener.go | 9 +- integration/setup/setup.go | 1 + integration/setup/startup.go | 24 ++- internal/backends/sqlite/metadata/pool/db.go | 7 +- .../backends/sqlite/metadata/pool/pool.go | 62 ++----- .../sqlite/metadata/pool/pool_test.go | 130 +------------- internal/backends/sqlite/metadata/pool/uri.go | 73 ++++++++ .../backends/sqlite/metadata/pool/uri_test.go | 165 ++++++++++++++++++ 10 files changed, 281 insertions(+), 194 deletions(-) create mode 100644 internal/backends/sqlite/metadata/pool/uri.go create mode 100644 internal/backends/sqlite/metadata/pool/uri_test.go diff --git a/Taskfile.yml b/Taskfile.yml index 801dc18ab323..64aa83eb3e2a 100644 --- a/Taskfile.yml +++ b/Taskfile.yml @@ -210,6 +210,7 @@ tasks: go test -count=1 -run='{{or .TEST_RUN .SHARD_RUN}}' -timeout={{.TEST_TIMEOUT}} {{.RACE_FLAG}} -tags={{.BUILD_TAGS}} -shuffle=on -coverpkg=../... -coverprofile=integration-sqlite.txt . -target-backend=ferretdb-sqlite + -sqlite-url=file:../tmp/sqlite-tests/ -target-tls -compat-url='mongodb://username:password@127.0.0.1:47018/?tls=true&tlsCertificateKeyFile=../build/certs/client.pem&tlsCaFile=../build/certs/rootCA-cert.pem' -disable-filter-pushdown diff --git a/integration/Taskfile.yml b/integration/Taskfile.yml index cc08682b9f6d..8122402d902d 100644 --- a/integration/Taskfile.yml +++ b/integration/Taskfile.yml @@ -27,6 +27,7 @@ tasks: go test -count=1 {{.RACE_FLAG}} -run=TestEnvData -tags=ferretdb_testenvdata . -target-backend=ferretdb-sqlite + -sqlite-url=file:../tmp/sqlite-tests/ # no hana yet - > go test -count=1 {{.RACE_FLAG}} -run=TestEnvData @@ -86,6 +87,7 @@ tasks: -log-level=error -bench-docs={{.BENCH_DOCS}} -target-backend=ferretdb-sqlite + -sqlite-url=file:../tmp/sqlite-tests/ | tee new-sqlite.txt - ../bin/benchstat{{exeExt}} old-sqlite.txt new-sqlite.txt @@ -98,6 +100,7 @@ tasks: -log-level=error -bench-docs={{.BENCH_DOCS}} -target-backend=ferretdb-sqlite + -sqlite-url=file:../tmp/sqlite-tests/ -- -disable-filter-pushdown | tee new-sqlite.txt - ../bin/benchstat{{exeExt}} old-sqlite.txt new-sqlite.txt diff --git a/integration/setup/listener.go b/integration/setup/listener.go index e30a110a0713..f361848d53eb 100644 --- a/integration/setup/listener.go +++ b/integration/setup/listener.go @@ -107,16 +107,19 @@ func setupListener(tb testtb.TB, ctx context.Context, logger *zap.Logger) string switch *targetBackendF { case "ferretdb-pg": require.NotEmpty(tb, *postgreSQLURLF, "-postgresql-url must be set for %q", *targetBackendF) + require.Empty(tb, *sqliteURLF, "-sqlite-url must be empty for %q", *targetBackendF) require.Empty(tb, *hanaURLF, "-hana-url must be empty for %q", *targetBackendF) handler = "pg" case "ferretdb-sqlite": require.Empty(tb, *postgreSQLURLF, "-postgresql-url must be empty for %q", *targetBackendF) + require.NotEmpty(tb, *sqliteURLF, "-sqlite-url must be set for %q", *targetBackendF) require.Empty(tb, *hanaURLF, "-hana-url must be empty for %q", *targetBackendF) handler = "sqlite" case "ferretdb-hana": require.Empty(tb, *postgreSQLURLF, "-postgresql-url must be empty for %q", *targetBackendF) + require.Empty(tb, *sqliteURLF, "-sqlite-url must be empty for %q", *targetBackendF) require.NotEmpty(tb, *hanaURLF, "-hana-url must be set for %q", *targetBackendF) handler = "hana" @@ -137,10 +140,8 @@ func setupListener(tb testtb.TB, ctx context.Context, logger *zap.Logger) string StateProvider: p, PostgreSQLURL: *postgreSQLURLF, - - SQLiteURL: sqliteURL.String(), - - HANAURL: *hanaURLF, + SQLiteURL: *sqliteURLF, + HANAURL: *hanaURLF, TestOpts: registry.TestOpts{ DisableFilterPushdown: *disableFilterPushdownF, diff --git a/integration/setup/setup.go b/integration/setup/setup.go index 3cb8c1140c9a..d24e68370e5b 100644 --- a/integration/setup/setup.go +++ b/integration/setup/setup.go @@ -48,6 +48,7 @@ var ( targetUnixSocketF = flag.Bool("target-unix-socket", false, "in-process FerretDB: use Unix socket") postgreSQLURLF = flag.String("postgresql-url", "", "in-process FerretDB: PostgreSQL URL for 'pg' handler.") + sqliteURLF = flag.String("sqlite-url", "", "in-process FerretDB: SQLite URI for 'sqlite' handler.") hanaURLF = flag.String("hana-url", "", "in-process FerretDB: Hana URL for 'hana' handler.") compatURLF = flag.String("compat-url", "", "compat system's (MongoDB) URL for compatibility tests; if empty, they are skipped") diff --git a/integration/setup/startup.go b/integration/setup/startup.go index 21826d96de7d..8804b454f4da 100644 --- a/integration/setup/startup.go +++ b/integration/setup/startup.go @@ -19,6 +19,7 @@ import ( "net/http" "net/url" "os" + "path/filepath" "runtime" "time" @@ -44,11 +45,6 @@ var listenerMetrics = connmetrics.NewListenerMetrics() // jaegerExporter is a shared Jaeger exporter for tests. var jaegerExporter *jaeger.Exporter -// sqliteURL is a URI for SQLite backend tests. -// -// We don't use testing.T.TempDir() or something to make debugging of failed tests easier. -var sqliteURL = must.NotFail(url.Parse("file:../tmp/sqlite-tests/")) - // Startup initializes things that should be initialized only once. func Startup() { logging.Setup(zap.DebugLevel, "") @@ -83,10 +79,20 @@ func Startup() { zap.S().Fatalf("Unknown target backend %q.", *targetBackendF) } - must.BeTrue(sqliteURL.Path == "") - must.BeTrue(sqliteURL.Opaque != "") - _ = os.Remove(sqliteURL.Opaque) - must.NoError(os.MkdirAll(sqliteURL.Opaque, 0o777)) + if *sqliteURLF != "" { + u, err := url.Parse(*sqliteURLF) + if err != nil { + zap.S().Fatalf("Failed to parse -sqlite-url %s: %s", u, err) + } + + must.BeTrue(u.Path == "") + must.BeTrue(u.Opaque != "") + + dir := must.NotFail(filepath.Abs(u.Opaque)) + zap.S().Infof("Recreating %s", dir) + _ = os.Remove(dir) + must.NoError(os.MkdirAll(dir, 0o777)) + } if u := *targetURLF; u != "" { client, err := makeClient(ctx, u) diff --git a/internal/backends/sqlite/metadata/pool/db.go b/internal/backends/sqlite/metadata/pool/db.go index 12cce2b97d8a..0199ff805fd9 100644 --- a/internal/backends/sqlite/metadata/pool/db.go +++ b/internal/backends/sqlite/metadata/pool/db.go @@ -29,7 +29,12 @@ type db struct { token *resource.Token } -// openDB opens existing database. +// openDB opens existing database or creates a new one. +// +// All valid FerretDB database names are valid SQLite database names / file names, +// so no validation is needed. +// One exception is very long full path names for the filesystem, +// but we don't check it. func openDB(uri string) (*db, error) { sqlDB, err := sql.Open("sqlite", uri) if err != nil { diff --git a/internal/backends/sqlite/metadata/pool/pool.go b/internal/backends/sqlite/metadata/pool/pool.go index 6cf584926009..e9d99ab8e2e4 100644 --- a/internal/backends/sqlite/metadata/pool/pool.go +++ b/internal/backends/sqlite/metadata/pool/pool.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Package pool provides access to SQLite database connections. +// Package pool provides access to SQLite databases and their connections. // // It should be used only by the metadata package. package pool @@ -39,7 +39,7 @@ import ( // filenameExtension represents SQLite database filename extension. const filenameExtension = ".sqlite" -// Pool provides access to SQLite database connections. +// Pool provides access to SQLite databases and their connections. // //nolint:vet // for readability type Pool struct { @@ -52,9 +52,11 @@ type Pool struct { token *resource.Token } -// New creates a pool for SQLite databases in the given directory. +// New creates a pool for SQLite databases in the directory specified by SQLite URI. +// +// All databases are opened on creation. func New(u string, l *zap.Logger) (*Pool, error) { - uri, err := validateURI(u) + uri, err := parseURI(u) if err != nil { return nil, fmt.Errorf("failed to parse SQLite URI %q: %s", u, err) } @@ -91,50 +93,6 @@ func New(u string, l *zap.Logger) (*Pool, error) { return p, nil } -// validateURI checks given URI value and returns parsed URL. -// URI should contain 'file' scheme and point to an existing directory. -// Path should end with '/'. Authority should be empty or absent. -// -// Returned URL contains path in both Path and Opaque to make String() method work correctly. -func validateURI(u string) (*url.URL, error) { - uri, err := url.Parse(u) - if err != nil { - return nil, err - } - - if uri.Scheme != "file" { - return nil, fmt.Errorf(`expected "file:" schema, got %q`, uri.Scheme) - } - - if uri.User != nil { - return nil, fmt.Errorf(`expected empty user info, got %q`, uri.User) - } - - if uri.Host != "" { - return nil, fmt.Errorf(`expected empty host, got %q`, uri.Host) - } - - if uri.Path == "" && uri.Opaque != "" { - uri.Path = uri.Opaque - } - uri.Opaque = uri.Path - - if !strings.HasSuffix(uri.Path, "/") { - return nil, fmt.Errorf(`expected path ending with "/", got %q`, uri.Path) - } - - fi, err := os.Stat(uri.Path) - if err != nil { - return nil, fmt.Errorf(`%q should be an existing directory, got %s`, uri.Path, err) - } - - if !fi.IsDir() { - return nil, fmt.Errorf(`%q should be an existing directory`, uri.Path) - } - - return uri, nil -} - // databaseName returns database name for given database file path. func (p *Pool) databaseName(databaseFile string) string { return strings.TrimSuffix(filepath.Base(databaseFile), filenameExtension) @@ -179,7 +137,7 @@ func (p *Pool) List(ctx context.Context) []string { return res } -// GetExisting returns an existing database connection by name, or nil. +// GetExisting returns an existing database by valid name, or nil. func (p *Pool) GetExisting(ctx context.Context, name string) *sql.DB { p.rw.RLock() defer p.rw.RUnlock() @@ -192,9 +150,9 @@ func (p *Pool) GetExisting(ctx context.Context, name string) *sql.DB { return db.sqlDB } -// GetOrCreate returns an existing database connection by name, or creates a new one. +// GetOrCreate returns an existing database by valid name, or creates a new one. // -// Returned boolean value indicates whether the connection was created. +// Returned boolean value indicates whether the database was created. func (p *Pool) GetOrCreate(ctx context.Context, name string) (*sql.DB, bool, error) { sqlDB := p.GetExisting(ctx, name) if sqlDB != nil { @@ -222,7 +180,7 @@ func (p *Pool) GetOrCreate(ctx context.Context, name string) (*sql.DB, bool, err return db.sqlDB, true, nil } -// Drop closes and removes a database by name. +// Drop closes and removes a database by valid name. // // It does nothing if the database does not exist. // diff --git a/internal/backends/sqlite/metadata/pool/pool_test.go b/internal/backends/sqlite/metadata/pool/pool_test.go index fe5c6b037a71..7feaaa355508 100644 --- a/internal/backends/sqlite/metadata/pool/pool_test.go +++ b/internal/backends/sqlite/metadata/pool/pool_test.go @@ -15,11 +15,8 @@ package pool import ( - "net/url" - "os" "testing" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/FerretDB/FerretDB/internal/util/testutil" @@ -28,7 +25,8 @@ import ( func TestCreateDrop(t *testing.T) { ctx := testutil.Ctx(t) - p, err := New("file:"+t.TempDir()+"/", testutil.Logger(t)) + // that also tests that query parameters are preserved + p, err := New("file:/?mode=memory", testutil.Logger(t)) require.NoError(t, err) defer p.Close() @@ -58,127 +56,3 @@ func TestCreateDrop(t *testing.T) { db = p.GetExisting(ctx, t.Name()) require.Nil(t, db) } - -func TestNewBackend(t *testing.T) { - t.Parallel() - - err := os.MkdirAll("tmp/dir", 0o777) - require.NoError(t, err) - - _, err = os.Create("tmp/file") - require.NoError(t, err) - - t.Cleanup(func() { - err := os.RemoveAll("tmp") - require.NoError(t, err) - }) - - testCases := map[string]struct { - value string - uri *url.URL - err string - }{ - "LocalDirectory": { - value: "file:./", - uri: &url.URL{ - Scheme: "file", - Opaque: "./", - Path: "./", - }, - }, - "LocalSubDirectory": { - value: "file:./tmp/", - uri: &url.URL{ - Scheme: "file", - Opaque: "./tmp/", - Path: "./tmp/", - }, - }, - "LocalSubSubDirectory": { - value: "file:./tmp/dir/", - uri: &url.URL{ - Scheme: "file", - Opaque: "./tmp/dir/", - Path: "./tmp/dir/", - }, - }, - "LocalDirectoryWithParameters": { - value: "file:./tmp/?mode=ro", - uri: &url.URL{ - Scheme: "file", - Opaque: "./tmp/", - Path: "./tmp/", - RawQuery: "mode=ro", - }, - }, - "AbsoluteDirectory": { - value: "file:/tmp/", - uri: &url.URL{ - Scheme: "file", - Opaque: "/tmp/", - Path: "/tmp/", - OmitHost: true, - }, - }, - "WithEmptyAuthority": { - value: "file:///tmp/", - uri: &url.URL{ - Scheme: "file", - Opaque: "/tmp/", - Path: "/tmp/", - }, - }, - "WithEmptyAuthorityAndQuery": { - value: "file:///tmp/?mode=ro", - uri: &url.URL{ - Scheme: "file", - Opaque: "/tmp/", - Path: "/tmp/", - RawQuery: "mode=ro", - }, - }, - "HostIsNotEmpty": { - value: "file://localhost/./tmp/?mode=ro", - err: `expected empty host, got "localhost"`, - }, - "UserIsNotEmpty": { - value: "file://user:pass@./tmp/?mode=ro", - err: `expected empty user info, got "user:pass"`, - }, - "NoDirectory": { - value: "file:./nodir/", - err: `"./nodir/" should be an existing directory, got stat ./nodir/: no such file or directory`, - }, - "PathIsNotEndsWithSlash": { - value: "file:./tmp/file", - err: `expected path ending with "/", got "./tmp/file"`, - }, - "FileInsteadOfDirectory": { - value: "file:./tmp/file/", - err: `"./tmp/file/" should be an existing directory, got stat ./tmp/file/: not a directory`, - }, - "MalformedURI": { - value: ":./tmp/", - err: `parse ":./tmp/": missing protocol scheme`, - }, - "NoScheme": { - value: "./tmp/", - err: `expected "file:" schema, got ""`, - }, - } - for name, tc := range testCases { - name, tc := name, tc - t.Run(name, func(t *testing.T) { - t.Parallel() - - u, err := validateURI(tc.value) - if tc.err != "" { - assert.EqualError(t, err, tc.err) - return - } - - require.NoError(t, err) - assert.Equal(t, u, tc.uri) - }) - } -} diff --git a/internal/backends/sqlite/metadata/pool/uri.go b/internal/backends/sqlite/metadata/pool/uri.go new file mode 100644 index 000000000000..d0c83bc2ab66 --- /dev/null +++ b/internal/backends/sqlite/metadata/pool/uri.go @@ -0,0 +1,73 @@ +// Copyright 2021 FerretDB Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package pool + +import ( + "fmt" + "net/url" + "os" + "strings" +) + +// parseURI checks given SQLite URI and returns a parsed form. +// +// URI should contain 'file' scheme and point to an existing directory. +// Path should end with '/'. Authority should be empty or absent. +// +// Returned URL contains path in both Path and Opaque to make String() method work correctly. +// Callers should use Path. +func parseURI(u string) (*url.URL, error) { + uri, err := url.Parse(u) + if err != nil { + return nil, err + } + + // handle mode=memory query parameter + // TODO https://github.com/FerretDB/FerretDB/issues/3084 + + if uri.Scheme != "file" { + return nil, fmt.Errorf(`expected "file:" schema, got %q`, uri.Scheme) + } + + if uri.User != nil { + return nil, fmt.Errorf(`expected empty user info, got %q`, uri.User) + } + + if uri.Host != "" { + return nil, fmt.Errorf(`expected empty host, got %q`, uri.Host) + } + + if uri.Path == "" { + uri.Path = uri.Opaque + } + uri.Opaque = uri.Path + uri.RawPath = "" + uri.OmitHost = true + + if !strings.HasSuffix(uri.Path, "/") { + return nil, fmt.Errorf(`expected path ending with "/", got %q`, uri.Path) + } + + fi, err := os.Stat(uri.Path) + if err != nil { + return nil, fmt.Errorf(`%q should be an existing directory, got %s`, uri.Path, err) + } + + if !fi.IsDir() { + return nil, fmt.Errorf(`%q should be an existing directory`, uri.Path) + } + + return uri, nil +} diff --git a/internal/backends/sqlite/metadata/pool/uri_test.go b/internal/backends/sqlite/metadata/pool/uri_test.go new file mode 100644 index 000000000000..0e9ba89ebbc6 --- /dev/null +++ b/internal/backends/sqlite/metadata/pool/uri_test.go @@ -0,0 +1,165 @@ +// Copyright 2021 FerretDB Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package pool + +import ( + "net/url" + "os" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestParseURI(t *testing.T) { + t.Parallel() + + // tests rely on the fact that both ./tmp and /tmp exist + + err := os.MkdirAll("tmp/dir", 0o777) + require.NoError(t, err) + + _, err = os.Create("tmp/file") + require.NoError(t, err) + + t.Cleanup(func() { + err := os.RemoveAll("tmp") + require.NoError(t, err) + }) + + testCases := map[string]struct { + in string + uri *url.URL + out string // defaults to in + err string + }{ + "LocalDirectory": { + in: "file:./", + uri: &url.URL{ + Scheme: "file", + Opaque: "./", + Path: "./", + OmitHost: true, + }, + }, + "LocalSubDirectory": { + in: "file:./tmp/", + uri: &url.URL{ + Scheme: "file", + Opaque: "./tmp/", + Path: "./tmp/", + OmitHost: true, + }, + }, + "LocalSubSubDirectory": { + in: "file:./tmp/dir/", + uri: &url.URL{ + Scheme: "file", + Opaque: "./tmp/dir/", + Path: "./tmp/dir/", + OmitHost: true, + }, + }, + "LocalDirectoryWithParameters": { + in: "file:./tmp/?mode=memory", + uri: &url.URL{ + Scheme: "file", + Opaque: "./tmp/", + Path: "./tmp/", + OmitHost: true, + RawQuery: "mode=memory", + }, + }, + "AbsoluteDirectory": { + in: "file:/tmp/", + uri: &url.URL{ + Scheme: "file", + Opaque: "/tmp/", + Path: "/tmp/", + OmitHost: true, + }, + }, + "WithEmptyAuthority": { + in: "file:///tmp/", + uri: &url.URL{ + Scheme: "file", + Opaque: "/tmp/", + Path: "/tmp/", + OmitHost: true, + }, + out: "file:/tmp/", + }, + "WithEmptyAuthorityAndQuery": { + in: "file:///tmp/?mode=memory", + uri: &url.URL{ + Scheme: "file", + Opaque: "/tmp/", + Path: "/tmp/", + OmitHost: true, + RawQuery: "mode=memory", + }, + out: "file:/tmp/?mode=memory", + }, + "HostIsNotEmpty": { + in: "file://localhost/./tmp/?mode=memory", + err: `expected empty host, got "localhost"`, + }, + "UserIsNotEmpty": { + in: "file://user:pass@./tmp/?mode=memory", + err: `expected empty user info, got "user:pass"`, + }, + "NoDirectory": { + in: "file:./nodir/", + err: `"./nodir/" should be an existing directory, got stat ./nodir/: no such file or directory`, + }, + "PathIsNotEndsWithSlash": { + in: "file:./tmp/file", + err: `expected path ending with "/", got "./tmp/file"`, + }, + "FileInsteadOfDirectory": { + in: "file:./tmp/file/", + err: `"./tmp/file/" should be an existing directory, got stat ./tmp/file/: not a directory`, + }, + "MalformedURI": { + in: ":./tmp/", + err: `parse ":./tmp/": missing protocol scheme`, + }, + "NoScheme": { + in: "./tmp/", + err: `expected "file:" schema, got ""`, + }, + } + for name, tc := range testCases { + name, tc := name, tc + t.Run(name, func(t *testing.T) { + t.Parallel() + + u, err := parseURI(tc.in) + if tc.err != "" { + assert.EqualError(t, err, tc.err) + return + } + + require.NoError(t, err) + assert.Equal(t, u, tc.uri) + + out := tc.out + if out == "" { + out = tc.in + } + assert.Equal(t, out, u.String()) + }) + } +}