fix: setting global config and secrets when using db config provider (#…

…2237)

# Problem
Setting global config when using db config provider allows creating the
same config name over and over again. e.g.

```bash
❯ ftl config set KEY "OKAY1" --db

❯ ftl config get KEY 
"OKAY1"

❯ ftl config set KEY "OKAY2" --db

❯ ftlprod config get KEY
"OKAY1"
```

running `ftl config list` reveals the problem:
```bash
❯ ftlprod config list
KEY
KEY
```

This problem occurs because of the compound unique constraint
[here](https://github.com/TBD54566975/ftl/blob/8dd18e78e9404832f378101e3cee6f1a8b514f66/backend/controller/sql/schema/20231103205514_init.sql#L518).


In PostgreSQL, `NULL` values are treated as distinct, so when you have a
unique constraint on multiple columns where one of the columns can be
`NULL`, each `NULL` is treated as a different value. This means that
multiple rows can have `NULL` in the module column and the same value in
the name column without violating the unique constraint.


# Fix
This PR proposes treating a `NULL` module value as `''` when creating
the unique compound constraint

> [!NOTE]
> I believe the following would also work
> ```sql
> 
> ALTER TABLE module_configuration
>     ALTER COLUMN module SET DEFAULT '';
> ```

> [!NOTE]
> I went with `''` to prevent colliding with potential module names and
i chose _not_ to default module to `''` to prevent application code from
treating `''` as a not nil value though go does treat `''` as the zero
value for a string but im not sure how that works with
`optional.Option`.

---------

Co-authored-by: Jiyoon Koo <[email protected]>
Co-authored-by: Wes <[email protected]>

backend/controller/sql/schema/20240801160101_edit-db-secrets-and-configuration.sql

@@ -0,0 +1,10 @@
+-- migrate:up
+
+CREATE UNIQUE INDEX module_config_name_unique
+    ON module_configuration ((COALESCE(module, '')), name);
+
+CREATE UNIQUE INDEX module_secret_name_unique
+    ON module_secrets ((COALESCE(module, '')), name);
+
+-- migrate:down
+

cmd/ftl-controller/main.go

@@ -56,7 +56,7 @@ func main() {
 	kctx.FatalIfErrorf(err)
 	configProviders := []cf.Provider[cf.Configuration]{cf.NewDBConfigProvider(configDal)}
 	configResolver := cf.NewDBConfigResolver(configDal)
-	cm, err := cf.New[cf.Configuration](ctx, configResolver, configProviders)
+	cm, err := cf.New(ctx, configResolver, configProviders)
 	kctx.FatalIfErrorf(err)
 
 	ctx = cf.ContextWithConfig(ctx, cm)

common/configuration/dal/dal_test.go

@@ -2,122 +2,229 @@ package dal
 
 import (
 	"context"
+	"errors"
+	"fmt"
 	"testing"
 
-	"github.com/alecthomas/assert/v2"
-	"github.com/alecthomas/types/optional"
-
-	"github.com/TBD54566975/ftl/backend/controller/sql"
 	"github.com/TBD54566975/ftl/backend/controller/sql/sqltest"
+	libdal "github.com/TBD54566975/ftl/backend/dal"
 	"github.com/TBD54566975/ftl/internal/log"
+	"github.com/alecthomas/assert/v2"
+	"github.com/alecthomas/types/optional"
 )
 
-func TestModuleConfiguration(t *testing.T) {
-	ctx := log.ContextWithNewDefaultLogger(context.Background())
-	conn := sqltest.OpenForTesting(ctx, t)
-	dal, err := New(ctx, conn)
-	assert.NoError(t, err)
-	assert.NotZero(t, dal)
-
-	tests := []struct {
-		TestName     string
-		ModuleSet    optional.Option[string]
-		ModuleGet    optional.Option[string]
-		PresetGlobal bool
-	}{
-		{
-			"SetModuleGetModule",
-			optional.Some("echo"),
-			optional.Some("echo"),
-			false,
-		},
-		{
-			"SetGlobalGetGlobal",
-			optional.None[string](),
-			optional.None[string](),
-			false,
-		},
-		{
-			"SetGlobalGetModule",
-			optional.None[string](),
-			optional.Some("echo"),
-			false,
-		},
-		{
-			"SetModuleOverridesGlobal",
-			optional.Some("echo"),
-			optional.Some("echo"),
-			true,
-		},
-	}
-
-	b := []byte(`"asdf"`)
-	for _, test := range tests {
-		t.Run(test.TestName, func(t *testing.T) {
-			if test.PresetGlobal {
-				err := dal.SetModuleConfiguration(ctx, optional.None[string](), "configname", []byte(`"qwerty"`))
-				assert.NoError(t, err)
-			}
-			err := dal.SetModuleConfiguration(ctx, test.ModuleSet, "configname", b)
-			assert.NoError(t, err)
-			gotBytes, err := dal.GetModuleConfiguration(ctx, test.ModuleGet, "configname")
-			assert.NoError(t, err)
-			assert.Equal(t, b, gotBytes)
-			err = dal.UnsetModuleConfiguration(ctx, test.ModuleGet, "configname")
-			assert.NoError(t, err)
-		})
-	}
-
-	t.Run("List", func(t *testing.T) {
-		sortedList := []sql.ModuleConfiguration{
-			{
-				Module: optional.Some("echo"),
-				Name:   "a",
-			},
-			{
-				Module: optional.Some("echo"),
-				Name:   "b",
-			},
-			{
-				Module: optional.None[string](),
-				Name:   "a",
-			},
-		}
-
-		// Insert entries in a separate order from how they should be returned to
-		// test sorting logic in the SQL query
-		err := dal.SetModuleConfiguration(ctx, sortedList[1].Module, sortedList[1].Name, []byte(`""`))
-		assert.NoError(t, err)
-		err = dal.SetModuleConfiguration(ctx, sortedList[2].Module, sortedList[2].Name, []byte(`""`))
-		assert.NoError(t, err)
-		err = dal.SetModuleConfiguration(ctx, sortedList[0].Module, sortedList[0].Name, []byte(`""`))
-		assert.NoError(t, err)
-
-		gotList, err := dal.ListModuleConfiguration(ctx)
-		assert.NoError(t, err)
-		for i := range sortedList {
-			assert.Equal(t, sortedList[i].Module, gotList[i].Module)
-			assert.Equal(t, sortedList[i].Name, gotList[i].Name)
-		}
+func TestDALConfiguration(t *testing.T) {
+	t.Run("ModuleConfiguration", func(t *testing.T) {
+		ctx := log.ContextWithNewDefaultLogger(context.Background())
+		conn := sqltest.OpenForTesting(ctx, t)
+		dal, err := New(ctx, conn)
+		assert.NoError(t, err)
+		assert.NotZero(t, dal)
+
+		err = dal.SetModuleConfiguration(ctx, optional.Some("echo"), "my_config", []byte(`""`))
+		assert.NoError(t, err)
+
+		value, err := dal.GetModuleConfiguration(ctx, optional.Some("echo"), "my_config")
+		assert.NoError(t, err)
+		assert.Equal(t, []byte(`""`), value)
+
+		err = dal.UnsetModuleConfiguration(ctx, optional.Some("echo"), "my_config")
+		assert.NoError(t, err)
+
+		value, err = dal.GetModuleConfiguration(ctx, optional.Some("echo"), "my_config")
+		assert.Error(t, err)
+		assert.True(t, errors.Is(err, libdal.ErrNotFound))
+		assert.Zero(t, value)
+	})
+
+	t.Run("GlobalConfiguration", func(t *testing.T) {
+		ctx := log.ContextWithNewDefaultLogger(context.Background())
+		conn := sqltest.OpenForTesting(ctx, t)
+		dal, err := New(ctx, conn)
+		assert.NoError(t, err)
+		assert.NotZero(t, dal)
+
+		err = dal.SetModuleConfiguration(ctx, optional.None[string](), "my_config", []byte(`""`))
+		assert.NoError(t, err)
+
+		value, err := dal.GetModuleConfiguration(ctx, optional.None[string](), "my_config")
+		assert.NoError(t, err)
+		assert.Equal(t, []byte(`""`), value)
+
+		err = dal.UnsetModuleConfiguration(ctx, optional.None[string](), "my_config")
+		assert.NoError(t, err)
+
+		value, err = dal.GetModuleConfiguration(ctx, optional.None[string](), "my_config")
+		fmt.Printf("value: %v\n", value)
+		assert.Error(t, err)
+		assert.True(t, errors.Is(err, libdal.ErrNotFound))
+		assert.Zero(t, value)
+	})
+
+	t.Run("SetSameGlobalConfigTwice", func(t *testing.T) {
+		ctx := log.ContextWithNewDefaultLogger(context.Background())
+		conn := sqltest.OpenForTesting(ctx, t)
+		dal, err := New(ctx, conn)
+		assert.NoError(t, err)
+		assert.NotZero(t, dal)
+
+		err = dal.SetModuleConfiguration(ctx, optional.None[string](), "my_config", []byte(`""`))
+		assert.NoError(t, err)
+
+		err = dal.SetModuleConfiguration(ctx, optional.None[string](), "my_config", []byte(`"hehe"`))
+		assert.NoError(t, err)
+
+		value, err := dal.GetModuleConfiguration(ctx, optional.None[string](), "my_config")
+		assert.NoError(t, err)
+		assert.Equal(t, []byte(`"hehe"`), value)
+	})
+
+	t.Run("SetModuleOverridesGlobal", func(t *testing.T) {
+		ctx := log.ContextWithNewDefaultLogger(context.Background())
+		conn := sqltest.OpenForTesting(ctx, t)
+		dal, err := New(ctx, conn)
+		assert.NoError(t, err)
+		assert.NotZero(t, dal)
+
+		err = dal.SetModuleConfiguration(ctx, optional.None[string](), "my_config", []byte(`""`))
+		assert.NoError(t, err)
+		err = dal.SetModuleConfiguration(ctx, optional.Some("echo"), "my_config", []byte(`"hehe"`))
+		assert.NoError(t, err)
+
+		value, err := dal.GetModuleConfiguration(ctx, optional.Some("echo"), "my_config")
+		assert.NoError(t, err)
+		assert.Equal(t, []byte(`"hehe"`), value)
 	})
 
 	t.Run("HandlesConflicts", func(t *testing.T) {
-		err := dal.SetModuleConfiguration(ctx, optional.Some("echo"), "my_config", []byte(`""`))
+		ctx := log.ContextWithNewDefaultLogger(context.Background())
+		conn := sqltest.OpenForTesting(ctx, t)
+		dal, err := New(ctx, conn)
+		assert.NoError(t, err)
+		assert.NotZero(t, dal)
+
+		err = dal.SetModuleConfiguration(ctx, optional.Some("echo"), "my_config", []byte(`""`))
 		assert.NoError(t, err)
 		err = dal.SetModuleConfiguration(ctx, optional.Some("echo"), "my_config", []byte(`""`))
 		assert.NoError(t, err)
+
+		err = dal.SetModuleConfiguration(ctx, optional.None[string](), "my_config", []byte(`""`))
+		assert.NoError(t, err)
+		err = dal.SetModuleConfiguration(ctx, optional.None[string](), "my_config", []byte(`"hehe"`))
+		assert.NoError(t, err)
+
+		value, err := dal.GetModuleConfiguration(ctx, optional.None[string](), "my_config")
+		assert.NoError(t, err)
+		assert.Equal(t, []byte(`"hehe"`), value)
 	})
 }
 
-func TestModuleSecrets(t *testing.T) {
-	ctx := log.ContextWithNewDefaultLogger(context.Background())
-	conn := sqltest.OpenForTesting(ctx, t)
-	dal, err := New(ctx, conn)
-	assert.NoError(t, err)
-	assert.NotZero(t, dal)
-
-	err = dal.SetModuleSecretURL(ctx, optional.Some("echo"), "my_secret", "asm://echo.my_secret")
-	assert.NoError(t, err)
-	err = dal.SetModuleSecretURL(ctx, optional.Some("echo"), "my_secret", "asm://echo.my_secret")
-	assert.NoError(t, err)
+func TestDALSecrets(t *testing.T) {
+	t.Run("ModuleSecret", func(t *testing.T) {
+		ctx := log.ContextWithNewDefaultLogger(context.Background())
+		conn := sqltest.OpenForTesting(ctx, t)
+		dal, err := New(ctx, conn)
+		assert.NoError(t, err)
+		assert.NotZero(t, dal)
+
+		err = dal.SetModuleSecretURL(ctx, optional.Some("echo"), "my_secret", "http://example.com")
+		assert.NoError(t, err)
+
+		value, err := dal.GetModuleSecretURL(ctx, optional.Some("echo"), "my_secret")
+		assert.NoError(t, err)
+		assert.Equal(t, "http://example.com", value)
+
+		err = dal.UnsetModuleSecret(ctx, optional.Some("echo"), "my_secret")
+		assert.NoError(t, err)
+
+		value, err = dal.GetModuleSecretURL(ctx, optional.Some("echo"), "my_secret")
+		assert.Error(t, err)
+		assert.True(t, errors.Is(err, libdal.ErrNotFound))
+		assert.Zero(t, value)
+	})
+
+	t.Run("GlobalSecret", func(t *testing.T) {
+		ctx := log.ContextWithNewDefaultLogger(context.Background())
+		conn := sqltest.OpenForTesting(ctx, t)
+		dal, err := New(ctx, conn)
+		assert.NoError(t, err)
+		assert.NotZero(t, dal)
+
+		err = dal.SetModuleSecretURL(ctx, optional.None[string](), "my_secret", "http://example.com")
+		assert.NoError(t, err)
+
+		value, err := dal.GetModuleSecretURL(ctx, optional.None[string](), "my_secret")
+		assert.NoError(t, err)
+		assert.Equal(t, "http://example.com", value)
+
+		err = dal.UnsetModuleSecret(ctx, optional.None[string](), "my_secret")
+		assert.NoError(t, err)
+
+		value, err = dal.GetModuleSecretURL(ctx, optional.None[string](), "my_secret")
+		assert.Error(t, err)
+		assert.True(t, errors.Is(err, libdal.ErrNotFound))
+		assert.Zero(t, value)
+	})
+
+	t.Run("SetSameGlobalSecretTwice", func(t *testing.T) {
+		ctx := log.ContextWithNewDefaultLogger(context.Background())
+		conn := sqltest.OpenForTesting(ctx, t)
+		dal, err := New(ctx, conn)
+		assert.NoError(t, err)
+		assert.NotZero(t, dal)
+
+		err = dal.SetModuleSecretURL(ctx, optional.None[string](), "my_secret", "http://example.com")
+		assert.NoError(t, err)
+
+		err = dal.SetModuleSecretURL(ctx, optional.None[string](), "my_secret", "http://example2.com")
+		assert.NoError(t, err)
+
+		value, err := dal.GetModuleSecretURL(ctx, optional.None[string](), "my_secret")
+		assert.NoError(t, err)
+		assert.Equal(t, "http://example2.com", value)
+	})
+
+	t.Run("SetModuleOverridesGlobal", func(t *testing.T) {
+		ctx := log.ContextWithNewDefaultLogger(context.Background())
+		conn := sqltest.OpenForTesting(ctx, t)
+		dal, err := New(ctx, conn)
+		assert.NoError(t, err)
+		assert.NotZero(t, dal)
+
+		err = dal.SetModuleSecretURL(ctx, optional.None[string](), "my_secret", "http://example.com")
+		assert.NoError(t, err)
+		err = dal.SetModuleSecretURL(ctx, optional.Some("echo"), "my_secret", "http://example2.com")
+		assert.NoError(t, err)
+
+		value, err := dal.GetModuleSecretURL(ctx, optional.Some("echo"), "my_secret")
+		assert.NoError(t, err)
+		assert.Equal(t, "http://example2.com", value)
+	})
+
+	t.Run("HandlesConflicts", func(t *testing.T) {
+		ctx := log.ContextWithNewDefaultLogger(context.Background())
+		conn := sqltest.OpenForTesting(ctx, t)
+		dal, err := New(ctx, conn)
+		assert.NoError(t, err)
+		assert.NotZero(t, dal)
+
+		err = dal.SetModuleSecretURL(ctx, optional.Some("echo"), "my_secret", "http://example.com")
+		assert.NoError(t, err)
+		err = dal.SetModuleSecretURL(ctx, optional.Some("echo"), "my_secret", "http://example2.com")
+		assert.NoError(t, err)
+
+		value, err := dal.GetModuleSecretURL(ctx, optional.Some("echo"), "my_secret")
+		assert.NoError(t, err)
+		assert.Equal(t, "http://example2.com", value)
+
+		err = dal.SetModuleSecretURL(ctx, optional.None[string](), "my_secret", "http://example.com")
+		assert.NoError(t, err)
+		err = dal.SetModuleSecretURL(ctx, optional.None[string](), "my_secret", "http://example2.com")
+		assert.NoError(t, err)
+
+		value, err = dal.GetModuleSecretURL(ctx, optional.None[string](), "my_secret")
+		assert.NoError(t, err)
+		assert.Equal(t, "http://example2.com", value)
+	})
+
 }

common/configuration/db_config_provider_test.go

@@ -49,3 +49,32 @@ func TestDBConfigProvider(t *testing.T) {
 	})
 	assert.NoError(t, err)
 }
+
+func TestDBConfigProvider_Global(t *testing.T) {
+	t.Run("works", func(t *testing.T) {
+		ctx := context.Background()
+		provider := NewDBConfigProvider(mockDBConfigProviderDAL{})
+
+		gotBytes, err := provider.Load(ctx, Ref{
+			Module: optional.None[string](),
+			Name:   "configname",
+		}, &url.URL{Scheme: "db"})
+		assert.NoError(t, err)
+		assert.Equal(t, b, gotBytes)
+
+		gotURL, err := provider.Store(ctx, Ref{
+			Module: optional.None[string](),
+			Name:   "configname",
+		}, b)
+		assert.NoError(t, err)
+		assert.Equal(t, &url.URL{Scheme: "db"}, gotURL)
+
+		err = provider.Delete(ctx, Ref{
+			Module: optional.None[string](),
+			Name:   "configname",
+		})
+		assert.NoError(t, err)
+	})
+
+	// TODO: maybe add a unit test to assert failure to create same global config twice. not sure how to wire up the mocks for this
+}