From dc2fc545b5e5d70bf42232930359058b50f7db52 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Sun, 9 Jan 2022 11:24:32 +0100 Subject: [PATCH 1/8] feat: Add mapping for encoding subject into UUIDs A new table was added that stores a mapping between an arbitraty string and a UUIDv5 of that string, namespaced by a namespace UUID that the user can set in the namespace configuration. --- internal/driver/registry.go | 2 + internal/driver/registry_default.go | 8 +++ internal/persistence/definitions.go | 2 + .../sql/migrations/single_table_test.go | 10 ++++ ...eate-uuid-mapping-table.cockroach.down.sql | 0 ...create-uuid-mapping-table.cockroach.up.sql | 10 ++++ ...0_create-uuid-mapping-table.mysql.down.sql | 0 ...000_create-uuid-mapping-table.mysql.up.sql | 10 ++++ ...reate-uuid-mapping-table.postgres.down.sql | 0 ..._create-uuid-mapping-table.postgres.up.sql | 10 ++++ ...create-uuid-mapping-table.sqlite3.down.sql | 0 ...0_create-uuid-mapping-table.sqlite3.up.sql | 10 ++++ ...eate-uuid-mapping-table.cockroach.down.sql | 1 + ...create-uuid-mapping-table.cockroach.up.sql | 2 + ...1_create-uuid-mapping-table.mysql.down.sql | 1 + ...001_create-uuid-mapping-table.mysql.up.sql | 2 + ...reate-uuid-mapping-table.postgres.down.sql | 1 + ..._create-uuid-mapping-table.postgres.up.sql | 2 + ...create-uuid-mapping-table.sqlite3.down.sql | 1 + ...1_create-uuid-mapping-table.sqlite3.up.sql | 2 + ...0200400_create-uuid-mapping-table.down.sql | 1 + ...400_create-uuid-mapping-table.mysql.up.sql | 12 +++++ ...110200400_create-uuid-mapping-table.up.sql | 12 +++++ internal/persistence/sql/uuid_mapping.go | 45 ++++++++++++++++ internal/persistence/sql/uuid_mapping_test.go | 52 +++++++++++++++++++ internal/uuidmapping/definitions.go | 17 ++++++ 26 files changed, 213 insertions(+) create mode 100644 internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.cockroach.down.sql create mode 100644 internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.cockroach.up.sql create mode 100644 internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.mysql.down.sql create mode 100644 internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.mysql.up.sql create mode 100644 internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.postgres.down.sql create mode 100644 internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.postgres.up.sql create mode 100644 internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.sqlite3.down.sql create mode 100644 internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.sqlite3.up.sql create mode 100644 internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.cockroach.down.sql create mode 100644 internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.cockroach.up.sql create mode 100644 internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.mysql.down.sql create mode 100644 internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.mysql.up.sql create mode 100644 internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.postgres.down.sql create mode 100644 internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.postgres.up.sql create mode 100644 internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.sqlite3.down.sql create mode 100644 internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.sqlite3.up.sql create mode 100644 internal/persistence/sql/migrations/templates/20220110200400_create-uuid-mapping-table.down.sql create mode 100644 internal/persistence/sql/migrations/templates/20220110200400_create-uuid-mapping-table.mysql.up.sql create mode 100644 internal/persistence/sql/migrations/templates/20220110200400_create-uuid-mapping-table.up.sql create mode 100644 internal/persistence/sql/uuid_mapping.go create mode 100644 internal/persistence/sql/uuid_mapping_test.go create mode 100644 internal/uuidmapping/definitions.go diff --git a/internal/driver/registry.go b/internal/driver/registry.go index 481c895d3..e5a207052 100644 --- a/internal/driver/registry.go +++ b/internal/driver/registry.go @@ -7,6 +7,7 @@ import ( "github.com/gobuffalo/pop/v6" "github.com/ory/keto/internal/driver/config" + "github.com/ory/keto/internal/uuidmapping" "github.com/spf13/cobra" @@ -34,6 +35,7 @@ type ( x.WriterProvider relationtuple.ManagerProvider + uuidmapping.ManagerProvider expand.EngineProvider check.EngineProvider persistence.Migrator diff --git a/internal/driver/registry_default.go b/internal/driver/registry_default.go index f18cb2d39..0a0675194 100644 --- a/internal/driver/registry_default.go +++ b/internal/driver/registry_default.go @@ -31,6 +31,7 @@ import ( grpcHealthV1 "google.golang.org/grpc/health/grpc_health_v1" "github.com/ory/keto/internal/driver/config" + "github.com/ory/keto/internal/uuidmapping" "github.com/ory/herodot" "github.com/ory/x/healthx" @@ -149,6 +150,13 @@ func (r *RegistryDefault) RelationTupleManager() relationtuple.Manager { return r.p } +func (r *RegistryDefault) UUIDMappingManager() uuidmapping.Manager { + if r.p == nil { + panic("no relation tuple manager, but expected to have one") + } + return r.p +} + func (r *RegistryDefault) Persister() persistence.Persister { if r.p == nil { panic("no persister, but expected to have one") diff --git a/internal/persistence/definitions.go b/internal/persistence/definitions.go index b2c9c1536..320568f13 100644 --- a/internal/persistence/definitions.go +++ b/internal/persistence/definitions.go @@ -9,11 +9,13 @@ import ( "github.com/gobuffalo/pop/v6" "github.com/ory/keto/internal/relationtuple" + "github.com/ory/keto/internal/uuidmapping" ) type ( Persister interface { relationtuple.Manager + uuidmapping.Manager Connection(ctx context.Context) *pop.Connection } diff --git a/internal/persistence/sql/migrations/single_table_test.go b/internal/persistence/sql/migrations/single_table_test.go index 13e3f8cf2..c3c004947 100644 --- a/internal/persistence/sql/migrations/single_table_test.go +++ b/internal/persistence/sql/migrations/single_table_test.go @@ -5,6 +5,7 @@ import ( "strconv" "testing" + "github.com/gofrs/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -96,6 +97,15 @@ func TestToSingleTableMigrator(t *testing.T) { assert.Equal(t, sSet, rts[1]) }) + t.Run("case=uuid mapping", func(t *testing.T) { + id := uuid.Must(uuid.NewV4()) + rep1 := "foo" + require.NoError(t, r.UUIDMappingManager().AddUUIDMapping(ctx, id, rep1)) + rep2, err := r.UUIDMappingManager().LookupUUID(ctx, id) + assert.NoError(t, err) + assert.Equal(t, rep1, rep2) + }) + t.Run("case=paginates", func(t *testing.T) { n := setup(t) diff --git a/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.cockroach.down.sql b/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.cockroach.down.sql new file mode 100644 index 000000000..e69de29bb diff --git a/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.cockroach.up.sql b/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.cockroach.up.sql new file mode 100644 index 000000000..48641a7a7 --- /dev/null +++ b/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.cockroach.up.sql @@ -0,0 +1,10 @@ +CREATE TABLE keto_uuid_mappings +( + id UUID NOT NULL, + string_representation VARCHAR(64) NOT NULL CHECK (string_representation <> ''), + + PRIMARY KEY (id), + + -- enforce uniqueness + CONSTRAINT chk_keto_uuid_map_uniq UNIQUE (id, string_representation) +); \ No newline at end of file diff --git a/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.mysql.down.sql b/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.mysql.down.sql new file mode 100644 index 000000000..e69de29bb diff --git a/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.mysql.up.sql b/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.mysql.up.sql new file mode 100644 index 000000000..0d707ec15 --- /dev/null +++ b/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.mysql.up.sql @@ -0,0 +1,10 @@ +CREATE TABLE keto_uuid_mappings +( + id VARCHAR(64) NOT NULL, + string_representation VARCHAR(64) NOT NULL CHECK (string_representation <> ''), + + PRIMARY KEY (id), + + -- enforce uniqueness + CONSTRAINT chk_keto_uuid_map_uniq UNIQUE (id, string_representation) +); \ No newline at end of file diff --git a/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.postgres.down.sql b/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.postgres.down.sql new file mode 100644 index 000000000..e69de29bb diff --git a/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.postgres.up.sql b/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.postgres.up.sql new file mode 100644 index 000000000..48641a7a7 --- /dev/null +++ b/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.postgres.up.sql @@ -0,0 +1,10 @@ +CREATE TABLE keto_uuid_mappings +( + id UUID NOT NULL, + string_representation VARCHAR(64) NOT NULL CHECK (string_representation <> ''), + + PRIMARY KEY (id), + + -- enforce uniqueness + CONSTRAINT chk_keto_uuid_map_uniq UNIQUE (id, string_representation) +); \ No newline at end of file diff --git a/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.sqlite3.down.sql b/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.sqlite3.down.sql new file mode 100644 index 000000000..e69de29bb diff --git a/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.sqlite3.up.sql b/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.sqlite3.up.sql new file mode 100644 index 000000000..48641a7a7 --- /dev/null +++ b/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.sqlite3.up.sql @@ -0,0 +1,10 @@ +CREATE TABLE keto_uuid_mappings +( + id UUID NOT NULL, + string_representation VARCHAR(64) NOT NULL CHECK (string_representation <> ''), + + PRIMARY KEY (id), + + -- enforce uniqueness + CONSTRAINT chk_keto_uuid_map_uniq UNIQUE (id, string_representation) +); \ No newline at end of file diff --git a/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.cockroach.down.sql b/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.cockroach.down.sql new file mode 100644 index 000000000..1a0e9eb51 --- /dev/null +++ b/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.cockroach.down.sql @@ -0,0 +1 @@ +DROP TABLE keto_uuid_mappings; \ No newline at end of file diff --git a/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.cockroach.up.sql b/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.cockroach.up.sql new file mode 100644 index 000000000..41af24927 --- /dev/null +++ b/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.cockroach.up.sql @@ -0,0 +1,2 @@ + +CREATE INDEX keto_uuid_mappings_subject_ids_idx ON keto_uuid_mappings (string_representation); \ No newline at end of file diff --git a/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.mysql.down.sql b/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.mysql.down.sql new file mode 100644 index 000000000..1a0e9eb51 --- /dev/null +++ b/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.mysql.down.sql @@ -0,0 +1 @@ +DROP TABLE keto_uuid_mappings; \ No newline at end of file diff --git a/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.mysql.up.sql b/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.mysql.up.sql new file mode 100644 index 000000000..41af24927 --- /dev/null +++ b/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.mysql.up.sql @@ -0,0 +1,2 @@ + +CREATE INDEX keto_uuid_mappings_subject_ids_idx ON keto_uuid_mappings (string_representation); \ No newline at end of file diff --git a/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.postgres.down.sql b/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.postgres.down.sql new file mode 100644 index 000000000..1a0e9eb51 --- /dev/null +++ b/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.postgres.down.sql @@ -0,0 +1 @@ +DROP TABLE keto_uuid_mappings; \ No newline at end of file diff --git a/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.postgres.up.sql b/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.postgres.up.sql new file mode 100644 index 000000000..41af24927 --- /dev/null +++ b/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.postgres.up.sql @@ -0,0 +1,2 @@ + +CREATE INDEX keto_uuid_mappings_subject_ids_idx ON keto_uuid_mappings (string_representation); \ No newline at end of file diff --git a/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.sqlite3.down.sql b/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.sqlite3.down.sql new file mode 100644 index 000000000..1a0e9eb51 --- /dev/null +++ b/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.sqlite3.down.sql @@ -0,0 +1 @@ +DROP TABLE keto_uuid_mappings; \ No newline at end of file diff --git a/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.sqlite3.up.sql b/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.sqlite3.up.sql new file mode 100644 index 000000000..41af24927 --- /dev/null +++ b/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.sqlite3.up.sql @@ -0,0 +1,2 @@ + +CREATE INDEX keto_uuid_mappings_subject_ids_idx ON keto_uuid_mappings (string_representation); \ No newline at end of file diff --git a/internal/persistence/sql/migrations/templates/20220110200400_create-uuid-mapping-table.down.sql b/internal/persistence/sql/migrations/templates/20220110200400_create-uuid-mapping-table.down.sql new file mode 100644 index 000000000..09ae2c1f8 --- /dev/null +++ b/internal/persistence/sql/migrations/templates/20220110200400_create-uuid-mapping-table.down.sql @@ -0,0 +1 @@ +DROP TABLE keto_uuid_mappings; diff --git a/internal/persistence/sql/migrations/templates/20220110200400_create-uuid-mapping-table.mysql.up.sql b/internal/persistence/sql/migrations/templates/20220110200400_create-uuid-mapping-table.mysql.up.sql new file mode 100644 index 000000000..a780d79c8 --- /dev/null +++ b/internal/persistence/sql/migrations/templates/20220110200400_create-uuid-mapping-table.mysql.up.sql @@ -0,0 +1,12 @@ +CREATE TABLE keto_uuid_mappings +( + id VARCHAR(64) NOT NULL, + string_representation VARCHAR(64) NOT NULL CHECK (string_representation <> ''), + + PRIMARY KEY (id), + + -- enforce uniqueness + CONSTRAINT chk_keto_uuid_map_uniq UNIQUE (id, string_representation) +); + +CREATE INDEX keto_uuid_mappings_subject_ids_idx ON keto_uuid_mappings (string_representation); \ No newline at end of file diff --git a/internal/persistence/sql/migrations/templates/20220110200400_create-uuid-mapping-table.up.sql b/internal/persistence/sql/migrations/templates/20220110200400_create-uuid-mapping-table.up.sql new file mode 100644 index 000000000..04ea3054b --- /dev/null +++ b/internal/persistence/sql/migrations/templates/20220110200400_create-uuid-mapping-table.up.sql @@ -0,0 +1,12 @@ +CREATE TABLE keto_uuid_mappings +( + id UUID NOT NULL, + string_representation VARCHAR(64) NOT NULL CHECK (string_representation <> ''), + + PRIMARY KEY (id), + + -- enforce uniqueness + CONSTRAINT chk_keto_uuid_map_uniq UNIQUE (id, string_representation) +); + +CREATE INDEX keto_uuid_mappings_subject_ids_idx ON keto_uuid_mappings (string_representation); \ No newline at end of file diff --git a/internal/persistence/sql/uuid_mapping.go b/internal/persistence/sql/uuid_mapping.go new file mode 100644 index 000000000..d89d360f3 --- /dev/null +++ b/internal/persistence/sql/uuid_mapping.go @@ -0,0 +1,45 @@ +package sql + +import ( + "context" + + "github.com/gofrs/uuid" + "github.com/ory/x/sqlcon" +) + +type ( + UUIDMapping struct { + ID uuid.UUID `db:"id"` + StringRepresentation string `db:"string_representation"` + } + UUIDMappings []*UUIDMapping +) + +func (UUIDMappings) TableName() string { + return "keto_uuid_mappings" +} + +func (UUIDMapping) TableName() string { + return "keto_uuid_mappings" +} + +func (p *Persister) AddUUIDMapping(ctx context.Context, id uuid.UUID, representation string) error { + m := &UUIDMapping{ + ID: id, + StringRepresentation: representation, + } + p.d.Logger().Trace("adding UUID mapping") + + return sqlcon.HandleError(p.Connection(ctx).Create(m)) +} + +func (p *Persister) LookupUUID(ctx context.Context, id uuid.UUID) (rep string, err error) { + p.d.Logger().Trace("looking up UUID") + + m := &UUIDMapping{} + if err := sqlcon.HandleError(p.Connection(ctx).Find(m, id)); err != nil { + return "", err + } + + return m.StringRepresentation, nil +} diff --git a/internal/persistence/sql/uuid_mapping_test.go b/internal/persistence/sql/uuid_mapping_test.go new file mode 100644 index 000000000..4019225df --- /dev/null +++ b/internal/persistence/sql/uuid_mapping_test.go @@ -0,0 +1,52 @@ +package sql_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/ory/keto/internal/driver" + "github.com/ory/keto/internal/persistence/sql" + "github.com/ory/keto/internal/x/dbx" +) + +func TestUUIDMapping(t *testing.T) { + for _, dsn := range dbx.GetDSNs(t, false) { + t.Run("dsn="+dsn.Name, func(t *testing.T) { + reg := driver.NewTestRegistry(t, dsn) + c, err := reg.PopConnection() + require.NoError(t, err) + + for _, tc := range []struct { + desc string + mappings interface{} + shouldErr bool + }{{ + desc: "empty should fail on constraint", + mappings: &sql.UUIDMapping{}, + shouldErr: true, + }, { + desc: "single with string rep should succeed", + mappings: &sql.UUIDMapping{StringRepresentation: "foo"}, + shouldErr: false, + }, { + desc: "two with same rep should fail on constraint", + mappings: sql.UUIDMappings{ + &sql.UUIDMapping{StringRepresentation: "bar"}, + &sql.UUIDMapping{StringRepresentation: "bar"}, + }, + shouldErr: true, + }} { + t.Run("case="+tc.desc, func(t *testing.T) { + err = c.Create(tc.mappings) + if tc.shouldErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } + }) + } +} diff --git a/internal/uuidmapping/definitions.go b/internal/uuidmapping/definitions.go new file mode 100644 index 000000000..7eb994827 --- /dev/null +++ b/internal/uuidmapping/definitions.go @@ -0,0 +1,17 @@ +package uuidmapping + +import ( + "context" + + "github.com/gofrs/uuid" +) + +type ( + ManagerProvider interface { + UUIDMappingManager() Manager + } + Manager interface { + AddUUIDMapping(ctx context.Context, id uuid.UUID, representation string) error + LookupUUID(ctx context.Context, id uuid.UUID) (rep string, err error) + } +) From bd9b2e0beeab793b160a8ef30e9a34a1a49ee19f Mon Sep 17 00:00:00 2001 From: Patrik Date: Mon, 17 Jan 2022 11:24:13 +0100 Subject: [PATCH 2/8] test: move UUID mapping manager test to the right place --- internal/persistence/sql/full_test.go | 7 +++++++ .../sql/migrations/single_table_test.go | 10 ---------- internal/uuidmapping/definitions.go | 16 ++++++++++++++++ 3 files changed, 23 insertions(+), 10 deletions(-) diff --git a/internal/persistence/sql/full_test.go b/internal/persistence/sql/full_test.go index ba88082c0..a90d94c65 100644 --- a/internal/persistence/sql/full_test.go +++ b/internal/persistence/sql/full_test.go @@ -5,6 +5,8 @@ import ( "fmt" "testing" + "github.com/ory/keto/internal/uuidmapping" + "github.com/gofrs/uuid" "github.com/ory/keto/internal/x/dbx" @@ -66,6 +68,11 @@ func TestPersister(t *testing.T) { // same registry, but different persisters only differing in the network ID relationtuple.IsolationTest(t, p0, p1, addNamespace(r, nspaces)) }) + + t.Run("uuidmapping.ManagerTest", func(t *testing.T) { + p, _, _ := setup(t, dsn) + uuidmapping.ManagerTest(t, p) + }) }) } } diff --git a/internal/persistence/sql/migrations/single_table_test.go b/internal/persistence/sql/migrations/single_table_test.go index c3c004947..13e3f8cf2 100644 --- a/internal/persistence/sql/migrations/single_table_test.go +++ b/internal/persistence/sql/migrations/single_table_test.go @@ -5,7 +5,6 @@ import ( "strconv" "testing" - "github.com/gofrs/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -97,15 +96,6 @@ func TestToSingleTableMigrator(t *testing.T) { assert.Equal(t, sSet, rts[1]) }) - t.Run("case=uuid mapping", func(t *testing.T) { - id := uuid.Must(uuid.NewV4()) - rep1 := "foo" - require.NoError(t, r.UUIDMappingManager().AddUUIDMapping(ctx, id, rep1)) - rep2, err := r.UUIDMappingManager().LookupUUID(ctx, id) - assert.NoError(t, err) - assert.Equal(t, rep1, rep2) - }) - t.Run("case=paginates", func(t *testing.T) { n := setup(t) diff --git a/internal/uuidmapping/definitions.go b/internal/uuidmapping/definitions.go index 7eb994827..13870aa76 100644 --- a/internal/uuidmapping/definitions.go +++ b/internal/uuidmapping/definitions.go @@ -2,6 +2,10 @@ package uuidmapping import ( "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/gofrs/uuid" ) @@ -15,3 +19,15 @@ type ( LookupUUID(ctx context.Context, id uuid.UUID) (rep string, err error) } ) + +func ManagerTest(t *testing.T, m Manager) { + ctx := context.Background() + + id := uuid.Must(uuid.NewV4()) + rep1 := "foo" + require.NoError(t, m.AddUUIDMapping(ctx, id, rep1)) + + rep2, err := m.LookupUUID(ctx, id) + assert.NoError(t, err) + assert.Equal(t, rep1, rep2) +} From fb7af042c1081944f773158173697133ebab4974 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Thu, 20 Jan 2022 18:03:54 +0100 Subject: [PATCH 3/8] feat: Add migration for UUID mapping --- .../sql/migrations/single_table_test.go | 2 +- .../sql/migrations/uuid_mapping.go | 56 ++++++++++++ .../sql/migrations/uuid_mapping_test.go | 85 +++++++++++++++++++ 3 files changed, 142 insertions(+), 1 deletion(-) create mode 100644 internal/persistence/sql/migrations/uuid_mapping.go create mode 100644 internal/persistence/sql/migrations/uuid_mapping_test.go diff --git a/internal/persistence/sql/migrations/single_table_test.go b/internal/persistence/sql/migrations/single_table_test.go index 13e3f8cf2..f41d3e010 100644 --- a/internal/persistence/sql/migrations/single_table_test.go +++ b/internal/persistence/sql/migrations/single_table_test.go @@ -226,7 +226,7 @@ func TestToSingleTableMigrator_HasLegacyTable(t *testing.T) { ln, err := m.LegacyNamespaces(ctx) require.NoError(t, err) - assert.Equal(t, nspaces, ln) + assert.ElementsMatch(t, nspaces, ln) }) }) } diff --git a/internal/persistence/sql/migrations/uuid_mapping.go b/internal/persistence/sql/migrations/uuid_mapping.go new file mode 100644 index 000000000..3472017d9 --- /dev/null +++ b/internal/persistence/sql/migrations/uuid_mapping.go @@ -0,0 +1,56 @@ +package migrations + +import ( + "context" + + "github.com/gobuffalo/pop/v6" + "github.com/gofrs/uuid" + + "github.com/ory/keto/internal/persistence/sql" +) + +type ( + toUUIDMappingMigrator struct { + d dependencies + } +) + +// NewToUUIDMappingMigrator creates a new UUID mapping migrator. +func NewToUUIDMappingMigrator(d dependencies) *toUUIDMappingMigrator { + return &toUUIDMappingMigrator{d: d} +} + +// MigrateUUIDMappings migrates to UUID-mapped subject IDs for all relation +// tuples in the database. +func (m *toUUIDMappingMigrator) MigrateUUIDMappings(ctx context.Context) error { + p, ok := m.d.Persister().(*sql.Persister) + if !ok { + panic("got unexpected persister") + } + + return p.Transaction(ctx, func(ctx context.Context, c *pop.Connection) error { + var relationTuples []*sql.RelationTuple + + if err := p.Connection(ctx).All(&relationTuples); err != nil { + return err + } + + for _, rt := range relationTuples { + _, err := uuid.FromString(rt.SubjectID.String) + if err == nil || !rt.SubjectID.Valid || rt.SubjectID.String == "" { + continue + } + + id := uuid.Must(uuid.NewV4()) + if err := p.AddUUIDMapping(ctx, id, rt.SubjectID.String); err != nil { + return err + } + + rt.SubjectID.String = id.String() + if err := c.Update(rt); err != nil { + return err + } + } + return nil + }) +} diff --git a/internal/persistence/sql/migrations/uuid_mapping_test.go b/internal/persistence/sql/migrations/uuid_mapping_test.go new file mode 100644 index 000000000..1f469b9ba --- /dev/null +++ b/internal/persistence/sql/migrations/uuid_mapping_test.go @@ -0,0 +1,85 @@ +package migrations + +import ( + "context" + dbsql "database/sql" + "testing" + "time" + + "github.com/gofrs/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/ory/keto/internal/driver" + "github.com/ory/keto/internal/persistence/sql" + "github.com/ory/keto/internal/x/dbx" +) + +func TestToUUIDMappingMigrator(t *testing.T) { + const debugOnDisk = false + + for _, dsn := range dbx.GetDSNs(t, debugOnDisk) { + t.Run("db="+dsn.Name, func(t *testing.T) { + ctx := context.Background() + r := driver.NewTestRegistry(t, dsn) + m := NewToUUIDMappingMigrator(r) + p := m.d.Persister().(*sql.Persister) + conn := p.Connection(ctx) + + testCases := []struct { + name string + rt *sql.RelationTuple + expectMapping bool + }{{ + name: "with string subject", + rt: &sql.RelationTuple{ + ID: uuid.Must(uuid.NewV4()), + SubjectID: dbsql.NullString{String: "a", Valid: true}, + CommitTime: time.Now(), + }, + expectMapping: true, + }, { + name: "with null subject", + rt: &sql.RelationTuple{ + ID: uuid.Must(uuid.NewV4()), + SubjectID: dbsql.NullString{String: "", Valid: false}, + SubjectSetNamespaceID: dbsql.NullInt32{Int32: 0, Valid: true}, + SubjectSetObject: dbsql.NullString{String: "obj", Valid: true}, + SubjectSetRelation: dbsql.NullString{String: "rel", Valid: true}, + CommitTime: time.Now(), + }, + expectMapping: false, + }, { + name: "with UUID subject", + rt: &sql.RelationTuple{ + ID: uuid.Must(uuid.NewV4()), + SubjectID: dbsql.NullString{String: uuid.Must(uuid.NewV4()).String(), Valid: true}, + CommitTime: time.Now(), + }, + expectMapping: false, + }} + + for _, tc := range testCases { + t.Run("case="+tc.name, func(t *testing.T) { + require.NoError(t, conn.Create(tc.rt)) + require.NoError(t, m.MigrateUUIDMappings(ctx)) + + newRt := &sql.RelationTuple{} + require.NoError(t, conn.Find(newRt, tc.rt.ID)) + + if tc.expectMapping { + // Check that a mapping was created + mapping := &sql.UUIDMapping{} + require.NoError(t, conn.Find(mapping, newRt.SubjectID)) + assert.NotEqual(t, tc.rt.SubjectID, newRt.SubjectID) + assert.Equal(t, tc.rt.SubjectID.String, mapping.StringRepresentation) + } else { + // Nothing should have changed (ignoring commit time) + newRt.CommitTime = tc.rt.CommitTime + assert.Equal(t, tc.rt, newRt) + } + }) + } + }) + } +} From acbe5def4f12f8facef370b79ac86ca7531fd2b9 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Fri, 21 Jan 2022 09:47:30 +0100 Subject: [PATCH 4/8] feat: Increase size of mapped UUID column --- ...110200400000000_create-uuid-mapping-table.cockroach.up.sql | 4 ++-- ...0220110200400000000_create-uuid-mapping-table.mysql.up.sql | 4 ++-- ...0110200400000000_create-uuid-mapping-table.postgres.up.sql | 4 ++-- ...20110200400000000_create-uuid-mapping-table.sqlite3.up.sql | 4 ++-- .../20220110200400_create-uuid-mapping-table.mysql.up.sql | 4 ++-- .../templates/20220110200400_create-uuid-mapping-table.up.sql | 4 ++-- 6 files changed, 12 insertions(+), 12 deletions(-) diff --git a/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.cockroach.up.sql b/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.cockroach.up.sql index 48641a7a7..72e1a5f85 100644 --- a/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.cockroach.up.sql +++ b/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.cockroach.up.sql @@ -1,7 +1,7 @@ CREATE TABLE keto_uuid_mappings ( - id UUID NOT NULL, - string_representation VARCHAR(64) NOT NULL CHECK (string_representation <> ''), + id UUID NOT NULL, + string_representation VARCHAR(255) NOT NULL CHECK (string_representation <> ''), PRIMARY KEY (id), diff --git a/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.mysql.up.sql b/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.mysql.up.sql index 0d707ec15..1a1055c47 100644 --- a/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.mysql.up.sql +++ b/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.mysql.up.sql @@ -1,7 +1,7 @@ CREATE TABLE keto_uuid_mappings ( - id VARCHAR(64) NOT NULL, - string_representation VARCHAR(64) NOT NULL CHECK (string_representation <> ''), + id VARCHAR(64) NOT NULL, + string_representation VARCHAR(255) NOT NULL CHECK (string_representation <> ''), PRIMARY KEY (id), diff --git a/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.postgres.up.sql b/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.postgres.up.sql index 48641a7a7..72e1a5f85 100644 --- a/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.postgres.up.sql +++ b/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.postgres.up.sql @@ -1,7 +1,7 @@ CREATE TABLE keto_uuid_mappings ( - id UUID NOT NULL, - string_representation VARCHAR(64) NOT NULL CHECK (string_representation <> ''), + id UUID NOT NULL, + string_representation VARCHAR(255) NOT NULL CHECK (string_representation <> ''), PRIMARY KEY (id), diff --git a/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.sqlite3.up.sql b/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.sqlite3.up.sql index 48641a7a7..72e1a5f85 100644 --- a/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.sqlite3.up.sql +++ b/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.sqlite3.up.sql @@ -1,7 +1,7 @@ CREATE TABLE keto_uuid_mappings ( - id UUID NOT NULL, - string_representation VARCHAR(64) NOT NULL CHECK (string_representation <> ''), + id UUID NOT NULL, + string_representation VARCHAR(255) NOT NULL CHECK (string_representation <> ''), PRIMARY KEY (id), diff --git a/internal/persistence/sql/migrations/templates/20220110200400_create-uuid-mapping-table.mysql.up.sql b/internal/persistence/sql/migrations/templates/20220110200400_create-uuid-mapping-table.mysql.up.sql index a780d79c8..cf8fb4f82 100644 --- a/internal/persistence/sql/migrations/templates/20220110200400_create-uuid-mapping-table.mysql.up.sql +++ b/internal/persistence/sql/migrations/templates/20220110200400_create-uuid-mapping-table.mysql.up.sql @@ -1,7 +1,7 @@ CREATE TABLE keto_uuid_mappings ( - id VARCHAR(64) NOT NULL, - string_representation VARCHAR(64) NOT NULL CHECK (string_representation <> ''), + id VARCHAR(64) NOT NULL, + string_representation VARCHAR(255) NOT NULL CHECK (string_representation <> ''), PRIMARY KEY (id), diff --git a/internal/persistence/sql/migrations/templates/20220110200400_create-uuid-mapping-table.up.sql b/internal/persistence/sql/migrations/templates/20220110200400_create-uuid-mapping-table.up.sql index 04ea3054b..0304e7729 100644 --- a/internal/persistence/sql/migrations/templates/20220110200400_create-uuid-mapping-table.up.sql +++ b/internal/persistence/sql/migrations/templates/20220110200400_create-uuid-mapping-table.up.sql @@ -1,7 +1,7 @@ CREATE TABLE keto_uuid_mappings ( - id UUID NOT NULL, - string_representation VARCHAR(64) NOT NULL CHECK (string_representation <> ''), + id UUID NOT NULL, + string_representation VARCHAR(255) NOT NULL CHECK (string_representation <> ''), PRIMARY KEY (id), From 20925a60cbe423ce1559363f06d4271877f23f19 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Fri, 21 Jan 2022 17:44:14 +0100 Subject: [PATCH 5/8] feat: Add pagination to the migrator --- .../sql/migrations/uuid_mapping.go | 56 +++++++++++++------ 1 file changed, 39 insertions(+), 17 deletions(-) diff --git a/internal/persistence/sql/migrations/uuid_mapping.go b/internal/persistence/sql/migrations/uuid_mapping.go index 3472017d9..92a7169fb 100644 --- a/internal/persistence/sql/migrations/uuid_mapping.go +++ b/internal/persistence/sql/migrations/uuid_mapping.go @@ -7,17 +7,19 @@ import ( "github.com/gofrs/uuid" "github.com/ory/keto/internal/persistence/sql" + "github.com/ory/x/sqlcon" ) type ( toUUIDMappingMigrator struct { - d dependencies + d dependencies + perPage int } ) // NewToUUIDMappingMigrator creates a new UUID mapping migrator. func NewToUUIDMappingMigrator(d dependencies) *toUUIDMappingMigrator { - return &toUUIDMappingMigrator{d: d} + return &toUUIDMappingMigrator{d: d, perPage: 100} } // MigrateUUIDMappings migrates to UUID-mapped subject IDs for all relation @@ -29,28 +31,48 @@ func (m *toUUIDMappingMigrator) MigrateUUIDMappings(ctx context.Context) error { } return p.Transaction(ctx, func(ctx context.Context, c *pop.Connection) error { - var relationTuples []*sql.RelationTuple - - if err := p.Connection(ctx).All(&relationTuples); err != nil { - return err - } - - for _, rt := range relationTuples { - _, err := uuid.FromString(rt.SubjectID.String) - if err == nil || !rt.SubjectID.Valid || rt.SubjectID.String == "" { - continue + for page := 1; ; page++ { + relationTuples, hasNext, err := m.getRelationTuples(ctx, page) + if err != nil { + return err } - id := uuid.Must(uuid.NewV4()) - if err := p.AddUUIDMapping(ctx, id, rt.SubjectID.String); err != nil { + if err := p.Connection(ctx).All(&relationTuples); err != nil { return err } - rt.SubjectID.String = id.String() - if err := c.Update(rt); err != nil { - return err + for _, rt := range relationTuples { + _, err := uuid.FromString(rt.SubjectID.String) + if err == nil || !rt.SubjectID.Valid || rt.SubjectID.String == "" { + continue + } + + id := uuid.Must(uuid.NewV4()) + if err := p.AddUUIDMapping(ctx, id, rt.SubjectID.String); err != nil { + return err + } + + rt.SubjectID.String = id.String() + if err := c.Update(rt); err != nil { + return err + } + } + + if !hasNext { + break } } return nil }) } + +func (m *toUUIDMappingMigrator) getRelationTuples(ctx context.Context, page int) (res []*sql.RelationTuple, hasNext bool, err error) { + q := m.d.Persister().Connection(ctx). + Order("nid, shard_id"). + Paginate(page, m.perPage) + + if err := q.All(&res); err != nil { + return nil, false, sqlcon.HandleError(err) + } + return res, q.Paginator.Page < q.Paginator.TotalPages, nil +} From 7438c280e590a4a784e7ee2b19f7d0ffe1b35492 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Sun, 23 Jan 2022 13:05:44 +0100 Subject: [PATCH 6/8] feat: Add mapping migration for objects --- .../sql/migrations/uuid_mapping.go | 52 +++++++++++++++---- .../sql/migrations/uuid_mapping_test.go | 35 +++++++++---- internal/persistence/sql/uuid_mapping.go | 16 ++++++ internal/uuidmapping/definitions.go | 30 ++++++++--- 4 files changed, 109 insertions(+), 24 deletions(-) diff --git a/internal/persistence/sql/migrations/uuid_mapping.go b/internal/persistence/sql/migrations/uuid_mapping.go index 92a7169fb..aba324de3 100644 --- a/internal/persistence/sql/migrations/uuid_mapping.go +++ b/internal/persistence/sql/migrations/uuid_mapping.go @@ -6,8 +6,9 @@ import ( "github.com/gobuffalo/pop/v6" "github.com/gofrs/uuid" - "github.com/ory/keto/internal/persistence/sql" "github.com/ory/x/sqlcon" + + "github.com/ory/keto/internal/persistence/sql" ) type ( @@ -42,17 +43,15 @@ func (m *toUUIDMappingMigrator) MigrateUUIDMappings(ctx context.Context) error { } for _, rt := range relationTuples { - _, err := uuid.FromString(rt.SubjectID.String) - if err == nil || !rt.SubjectID.Valid || rt.SubjectID.String == "" { - continue + if err := m.migrateSubjectID(ctx, rt); err != nil { + return err } - - id := uuid.Must(uuid.NewV4()) - if err := p.AddUUIDMapping(ctx, id, rt.SubjectID.String); err != nil { + if err := m.migrateSubjectSetObject(ctx, rt); err != nil { + return err + } + if err := m.migrateObject(ctx, rt); err != nil { return err } - - rt.SubjectID.String = id.String() if err := c.Update(rt); err != nil { return err } @@ -66,6 +65,41 @@ func (m *toUUIDMappingMigrator) MigrateUUIDMappings(ctx context.Context) error { }) } +func (m *toUUIDMappingMigrator) migrateSubjectID(ctx context.Context, rt *sql.RelationTuple) error { + _, err := uuid.FromString(rt.SubjectID.String) + if err == nil || !rt.SubjectID.Valid || rt.SubjectID.String == "" { + return nil + } + + rt.SubjectID.String, err = m.addUUIDMapping(ctx, rt.SubjectID.String) + return err +} + +func (m *toUUIDMappingMigrator) migrateSubjectSetObject(ctx context.Context, rt *sql.RelationTuple) error { + _, err := uuid.FromString(rt.SubjectSetObject.String) + if err == nil || !rt.SubjectSetObject.Valid || rt.SubjectSetObject.String == "" { + return nil + } + + rt.SubjectSetObject.String, err = m.addUUIDMapping(ctx, rt.SubjectSetObject.String) + return err +} + +func (m *toUUIDMappingMigrator) migrateObject(ctx context.Context, rt *sql.RelationTuple) error { + _, err := uuid.FromString(rt.Object) + if err == nil || rt.Object == "" { + return nil + } + + rt.Object, err = m.addUUIDMapping(ctx, rt.Object) + return err +} + +func (m *toUUIDMappingMigrator) addUUIDMapping(ctx context.Context, value string) (id string, err error) { + uid, err := m.d.Persister().MappedUUID(ctx, value) + return uid.String(), err +} + func (m *toUUIDMappingMigrator) getRelationTuples(ctx context.Context, page int) (res []*sql.RelationTuple, hasNext bool, err error) { q := m.d.Persister().Connection(ctx). Order("nid, shard_id"). diff --git a/internal/persistence/sql/migrations/uuid_mapping_test.go b/internal/persistence/sql/migrations/uuid_mapping_test.go index 1f469b9ba..bbfe3684d 100644 --- a/internal/persistence/sql/migrations/uuid_mapping_test.go +++ b/internal/persistence/sql/migrations/uuid_mapping_test.go @@ -6,6 +6,7 @@ import ( "testing" "time" + "github.com/gobuffalo/pop/v6" "github.com/gofrs/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -34,7 +35,8 @@ func TestToUUIDMappingMigrator(t *testing.T) { name: "with string subject", rt: &sql.RelationTuple{ ID: uuid.Must(uuid.NewV4()), - SubjectID: dbsql.NullString{String: "a", Valid: true}, + Object: "object", + SubjectID: dbsql.NullString{String: "subject", Valid: true}, CommitTime: time.Now(), }, expectMapping: true, @@ -44,11 +46,12 @@ func TestToUUIDMappingMigrator(t *testing.T) { ID: uuid.Must(uuid.NewV4()), SubjectID: dbsql.NullString{String: "", Valid: false}, SubjectSetNamespaceID: dbsql.NullInt32{Int32: 0, Valid: true}, - SubjectSetObject: dbsql.NullString{String: "obj", Valid: true}, - SubjectSetRelation: dbsql.NullString{String: "rel", Valid: true}, + SubjectSetObject: dbsql.NullString{String: "object", Valid: true}, + SubjectSetRelation: dbsql.NullString{String: "subject_set_relation", Valid: true}, + Object: "object", CommitTime: time.Now(), }, - expectMapping: false, + expectMapping: true, }, { name: "with UUID subject", rt: &sql.RelationTuple{ @@ -68,11 +71,15 @@ func TestToUUIDMappingMigrator(t *testing.T) { require.NoError(t, conn.Find(newRt, tc.rt.ID)) if tc.expectMapping { - // Check that a mapping was created - mapping := &sql.UUIDMapping{} - require.NoError(t, conn.Find(mapping, newRt.SubjectID)) - assert.NotEqual(t, tc.rt.SubjectID, newRt.SubjectID) - assert.Equal(t, tc.rt.SubjectID.String, mapping.StringRepresentation) + // Check subject mapping + if tc.rt.SubjectID.Valid { + assertHasMapping(t, conn, tc.rt.SubjectID.String, newRt.SubjectID.String) + } else { + assertHasMapping(t, conn, tc.rt.SubjectSetObject.String, newRt.SubjectSetObject.String) + // check that both "OBJECT" strings are mapped to same UUID. + assert.Equal(t, newRt.Object, newRt.SubjectSetObject.String) + } + assertHasMapping(t, conn, tc.rt.Object, newRt.Object) } else { // Nothing should have changed (ignoring commit time) newRt.CommitTime = tc.rt.CommitTime @@ -83,3 +90,13 @@ func TestToUUIDMappingMigrator(t *testing.T) { }) } } + +// assertHasMapping checks that there is a mapping from the given string (value) +// to the given UUID (uid). +func assertHasMapping(t *testing.T, conn *pop.Connection, value, uid string) { + t.Helper() + mapping := &sql.UUIDMapping{} + require.NoError(t, conn.Find(mapping, uid), "Could not find mapping for %q", uid) + assert.NotEqual(t, value, uid, "value was not replaced by UUID") + assert.Equal(t, value, mapping.StringRepresentation) +} diff --git a/internal/persistence/sql/uuid_mapping.go b/internal/persistence/sql/uuid_mapping.go index d89d360f3..f4b695ae7 100644 --- a/internal/persistence/sql/uuid_mapping.go +++ b/internal/persistence/sql/uuid_mapping.go @@ -2,6 +2,7 @@ package sql import ( "context" + "errors" "github.com/gofrs/uuid" "github.com/ory/x/sqlcon" @@ -43,3 +44,18 @@ func (p *Persister) LookupUUID(ctx context.Context, id uuid.UUID) (rep string, e return m.StringRepresentation, nil } + +func (p *Persister) MappedUUID(ctx context.Context, representation string) (uuid.UUID, error) { + p.d.Logger().Trace("looking up mapped UUID") + + m := &UUIDMapping{} + if err := sqlcon.HandleError(p.Connection(ctx).Where("string_representation = ?", representation).First(m)); err != nil { + if errors.Is(err, sqlcon.ErrNoRows) { + id := uuid.Must(uuid.NewV4()) + return id, p.AddUUIDMapping(ctx, id, representation) + } + return uuid.Nil, err + } + + return m.ID, nil +} diff --git a/internal/uuidmapping/definitions.go b/internal/uuidmapping/definitions.go index 13870aa76..ea96018ce 100644 --- a/internal/uuidmapping/definitions.go +++ b/internal/uuidmapping/definitions.go @@ -15,7 +15,15 @@ type ( UUIDMappingManager() Manager } Manager interface { + // MappedUUID returns the mapped UUID for the given string + // representation. If the string representation is not mapped, a new + // UUID will be created automatically. + MappedUUID(ctx context.Context, representation string) (uuid.UUID, error) + + // AddUUIDMapping adds a new mapping between a UUID and a string. AddUUIDMapping(ctx context.Context, id uuid.UUID, representation string) error + + // LookupUUID returns the string representation for the given UUID. LookupUUID(ctx context.Context, id uuid.UUID) (rep string, err error) } ) @@ -23,11 +31,21 @@ type ( func ManagerTest(t *testing.T, m Manager) { ctx := context.Background() - id := uuid.Must(uuid.NewV4()) - rep1 := "foo" - require.NoError(t, m.AddUUIDMapping(ctx, id, rep1)) + t.Run("case=add_lookup", func(t *testing.T) { + id := uuid.Must(uuid.NewV4()) + rep1 := "foo" + require.NoError(t, m.AddUUIDMapping(ctx, id, rep1)) + + rep2, err := m.LookupUUID(ctx, id) + assert.NoError(t, err) + assert.Equal(t, rep1, rep2) + }) - rep2, err := m.LookupUUID(ctx, id) - assert.NoError(t, err) - assert.Equal(t, rep1, rep2) + t.Run("case=MappedUUID", func(t *testing.T) { + id1, err := m.MappedUUID(ctx, "string") + require.NoError(t, err) + id2, err := m.MappedUUID(ctx, "string") + require.NoError(t, err) + assert.Equal(t, id1, id2) + }) } From cb8a996b726fda48d8f6ff8bc4836d10e14e9d69 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Wed, 2 Feb 2022 19:25:01 +0100 Subject: [PATCH 7/8] feat: map subjects/objects using UUIDv5 --- cmd/migrate/migrate_uuid_mapping.go | 49 +++++++++++++++++++ cmd/migrate/root.go | 1 + ...create-uuid-mapping-table.cockroach.up.sql | 9 ++-- ...000_create-uuid-mapping-table.mysql.up.sql | 9 ++-- ..._create-uuid-mapping-table.postgres.up.sql | 9 ++-- ...0_create-uuid-mapping-table.sqlite3.up.sql | 9 ++-- ...create-uuid-mapping-table.cockroach.up.sql | 2 - ...001_create-uuid-mapping-table.mysql.up.sql | 2 - ..._create-uuid-mapping-table.postgres.up.sql | 2 - ...1_create-uuid-mapping-table.sqlite3.up.sql | 2 - ...400_create-uuid-mapping-table.mysql.up.sql | 13 ++--- ...110200400_create-uuid-mapping-table.up.sql | 13 ++--- .../sql/migrations/uuid_mapping.go | 3 +- internal/persistence/sql/uuid_mapping.go | 45 +++++++++-------- internal/uuidmapping/definitions.go | 33 ++++++------- 15 files changed, 108 insertions(+), 93 deletions(-) create mode 100644 cmd/migrate/migrate_uuid_mapping.go diff --git a/cmd/migrate/migrate_uuid_mapping.go b/cmd/migrate/migrate_uuid_mapping.go new file mode 100644 index 000000000..8c15c60c4 --- /dev/null +++ b/cmd/migrate/migrate_uuid_mapping.go @@ -0,0 +1,49 @@ +package migrate + +import ( + "fmt" + + "github.com/ory/x/cmdx" + "github.com/ory/x/flagx" + "github.com/pkg/errors" + "github.com/spf13/cobra" + + "github.com/ory/keto/internal/driver" + "github.com/ory/keto/internal/persistence" + "github.com/ory/keto/internal/persistence/sql/migrations" +) + +func newMigrateUUIDMappingCmd() *cobra.Command { + cmd := &cobra.Command{ + Use: "uuid-mapping", + Short: "Migrate the non-UUID subject and object names to UUIDs.", + Long: `Migrate the non-UUID subject and object names to UUIDs. +This step only has to be executed once. +Please ensure that you have a backup in case something goes wrong!"`, + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, _ []string) error { + reg, err := driver.NewDefaultRegistry(cmd.Context(), cmd.Flags(), false) + if errors.Is(err, persistence.ErrNetworkMigrationsMissing) { + _, _ = fmt.Fprintln(cmd.ErrOrStderr(), + "Migrations were not applied yet, please apply them first using `keto migrate up`.") + return cmdx.FailSilently(cmd) + } else if err != nil { + return err + } + + if !flagx.MustGetBool(cmd, FlagYes) && + !cmdx.AskForConfirmation( + "Are you sure you want to migrate the subject and object names to UUIDs?", + cmd.InOrStdin(), cmd.OutOrStdout()) { + _, _ = fmt.Fprintln(cmd.OutOrStdout(), "OK, aborting.") + return nil + } + + migrator := migrations.NewToUUIDMappingMigrator(reg) + return migrator.MigrateUUIDMappings(cmd.Context()) + }, + } + RegisterYesFlag(cmd.Flags()) + + return cmd +} diff --git a/cmd/migrate/root.go b/cmd/migrate/root.go index fc92a1e7c..a4388b6f1 100644 --- a/cmd/migrate/root.go +++ b/cmd/migrate/root.go @@ -13,6 +13,7 @@ func newMigrateCmd() *cobra.Command { newStatusCmd(), newUpCmd(), newDownCmd(), + newMigrateUUIDMappingCmd(), ) return cmd } diff --git a/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.cockroach.up.sql b/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.cockroach.up.sql index 72e1a5f85..9aad41258 100644 --- a/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.cockroach.up.sql +++ b/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.cockroach.up.sql @@ -1,10 +1,7 @@ CREATE TABLE keto_uuid_mappings ( - id UUID NOT NULL, - string_representation VARCHAR(255) NOT NULL CHECK (string_representation <> ''), + id UUID NOT NULL, + string_representation TEXT NOT NULL CHECK (string_representation <> ''), - PRIMARY KEY (id), - - -- enforce uniqueness - CONSTRAINT chk_keto_uuid_map_uniq UNIQUE (id, string_representation) + PRIMARY KEY (id) ); \ No newline at end of file diff --git a/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.mysql.up.sql b/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.mysql.up.sql index 1a1055c47..5db883292 100644 --- a/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.mysql.up.sql +++ b/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.mysql.up.sql @@ -1,10 +1,7 @@ CREATE TABLE keto_uuid_mappings ( - id VARCHAR(64) NOT NULL, - string_representation VARCHAR(255) NOT NULL CHECK (string_representation <> ''), + id VARCHAR(64) NOT NULL, + string_representation TEXT NOT NULL CHECK (string_representation <> ''), - PRIMARY KEY (id), - - -- enforce uniqueness - CONSTRAINT chk_keto_uuid_map_uniq UNIQUE (id, string_representation) + PRIMARY KEY (id) ); \ No newline at end of file diff --git a/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.postgres.up.sql b/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.postgres.up.sql index 72e1a5f85..9aad41258 100644 --- a/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.postgres.up.sql +++ b/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.postgres.up.sql @@ -1,10 +1,7 @@ CREATE TABLE keto_uuid_mappings ( - id UUID NOT NULL, - string_representation VARCHAR(255) NOT NULL CHECK (string_representation <> ''), + id UUID NOT NULL, + string_representation TEXT NOT NULL CHECK (string_representation <> ''), - PRIMARY KEY (id), - - -- enforce uniqueness - CONSTRAINT chk_keto_uuid_map_uniq UNIQUE (id, string_representation) + PRIMARY KEY (id) ); \ No newline at end of file diff --git a/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.sqlite3.up.sql b/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.sqlite3.up.sql index 72e1a5f85..9aad41258 100644 --- a/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.sqlite3.up.sql +++ b/internal/persistence/sql/migrations/sql/20220110200400000000_create-uuid-mapping-table.sqlite3.up.sql @@ -1,10 +1,7 @@ CREATE TABLE keto_uuid_mappings ( - id UUID NOT NULL, - string_representation VARCHAR(255) NOT NULL CHECK (string_representation <> ''), + id UUID NOT NULL, + string_representation TEXT NOT NULL CHECK (string_representation <> ''), - PRIMARY KEY (id), - - -- enforce uniqueness - CONSTRAINT chk_keto_uuid_map_uniq UNIQUE (id, string_representation) + PRIMARY KEY (id) ); \ No newline at end of file diff --git a/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.cockroach.up.sql b/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.cockroach.up.sql index 41af24927..e69de29bb 100644 --- a/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.cockroach.up.sql +++ b/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.cockroach.up.sql @@ -1,2 +0,0 @@ - -CREATE INDEX keto_uuid_mappings_subject_ids_idx ON keto_uuid_mappings (string_representation); \ No newline at end of file diff --git a/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.mysql.up.sql b/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.mysql.up.sql index 41af24927..e69de29bb 100644 --- a/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.mysql.up.sql +++ b/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.mysql.up.sql @@ -1,2 +0,0 @@ - -CREATE INDEX keto_uuid_mappings_subject_ids_idx ON keto_uuid_mappings (string_representation); \ No newline at end of file diff --git a/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.postgres.up.sql b/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.postgres.up.sql index 41af24927..e69de29bb 100644 --- a/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.postgres.up.sql +++ b/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.postgres.up.sql @@ -1,2 +0,0 @@ - -CREATE INDEX keto_uuid_mappings_subject_ids_idx ON keto_uuid_mappings (string_representation); \ No newline at end of file diff --git a/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.sqlite3.up.sql b/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.sqlite3.up.sql index 41af24927..e69de29bb 100644 --- a/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.sqlite3.up.sql +++ b/internal/persistence/sql/migrations/sql/20220110200400000001_create-uuid-mapping-table.sqlite3.up.sql @@ -1,2 +0,0 @@ - -CREATE INDEX keto_uuid_mappings_subject_ids_idx ON keto_uuid_mappings (string_representation); \ No newline at end of file diff --git a/internal/persistence/sql/migrations/templates/20220110200400_create-uuid-mapping-table.mysql.up.sql b/internal/persistence/sql/migrations/templates/20220110200400_create-uuid-mapping-table.mysql.up.sql index cf8fb4f82..5db883292 100644 --- a/internal/persistence/sql/migrations/templates/20220110200400_create-uuid-mapping-table.mysql.up.sql +++ b/internal/persistence/sql/migrations/templates/20220110200400_create-uuid-mapping-table.mysql.up.sql @@ -1,12 +1,7 @@ CREATE TABLE keto_uuid_mappings ( - id VARCHAR(64) NOT NULL, - string_representation VARCHAR(255) NOT NULL CHECK (string_representation <> ''), + id VARCHAR(64) NOT NULL, + string_representation TEXT NOT NULL CHECK (string_representation <> ''), - PRIMARY KEY (id), - - -- enforce uniqueness - CONSTRAINT chk_keto_uuid_map_uniq UNIQUE (id, string_representation) -); - -CREATE INDEX keto_uuid_mappings_subject_ids_idx ON keto_uuid_mappings (string_representation); \ No newline at end of file + PRIMARY KEY (id) +); \ No newline at end of file diff --git a/internal/persistence/sql/migrations/templates/20220110200400_create-uuid-mapping-table.up.sql b/internal/persistence/sql/migrations/templates/20220110200400_create-uuid-mapping-table.up.sql index 0304e7729..9aad41258 100644 --- a/internal/persistence/sql/migrations/templates/20220110200400_create-uuid-mapping-table.up.sql +++ b/internal/persistence/sql/migrations/templates/20220110200400_create-uuid-mapping-table.up.sql @@ -1,12 +1,7 @@ CREATE TABLE keto_uuid_mappings ( - id UUID NOT NULL, - string_representation VARCHAR(255) NOT NULL CHECK (string_representation <> ''), + id UUID NOT NULL, + string_representation TEXT NOT NULL CHECK (string_representation <> ''), - PRIMARY KEY (id), - - -- enforce uniqueness - CONSTRAINT chk_keto_uuid_map_uniq UNIQUE (id, string_representation) -); - -CREATE INDEX keto_uuid_mappings_subject_ids_idx ON keto_uuid_mappings (string_representation); \ No newline at end of file + PRIMARY KEY (id) +); \ No newline at end of file diff --git a/internal/persistence/sql/migrations/uuid_mapping.go b/internal/persistence/sql/migrations/uuid_mapping.go index aba324de3..0b2e7b28b 100644 --- a/internal/persistence/sql/migrations/uuid_mapping.go +++ b/internal/persistence/sql/migrations/uuid_mapping.go @@ -5,7 +5,6 @@ import ( "github.com/gobuffalo/pop/v6" "github.com/gofrs/uuid" - "github.com/ory/x/sqlcon" "github.com/ory/keto/internal/persistence/sql" @@ -96,7 +95,7 @@ func (m *toUUIDMappingMigrator) migrateObject(ctx context.Context, rt *sql.Relat } func (m *toUUIDMappingMigrator) addUUIDMapping(ctx context.Context, value string) (id string, err error) { - uid, err := m.d.Persister().MappedUUID(ctx, value) + uid, err := m.d.Persister().ToUUID(ctx, value) return uid.String(), err } diff --git a/internal/persistence/sql/uuid_mapping.go b/internal/persistence/sql/uuid_mapping.go index f4b695ae7..95ed4b5e5 100644 --- a/internal/persistence/sql/uuid_mapping.go +++ b/internal/persistence/sql/uuid_mapping.go @@ -2,7 +2,6 @@ package sql import ( "context" - "errors" "github.com/gofrs/uuid" "github.com/ory/x/sqlcon" @@ -24,17 +23,32 @@ func (UUIDMapping) TableName() string { return "keto_uuid_mappings" } -func (p *Persister) AddUUIDMapping(ctx context.Context, id uuid.UUID, representation string) error { - m := &UUIDMapping{ - ID: id, - StringRepresentation: representation, - } +func (p *Persister) ToUUID(ctx context.Context, text string) (uuid.UUID, error) { + id := uuid.NewV5(p.NetworkID(ctx), text) p.d.Logger().Trace("adding UUID mapping") - return sqlcon.HandleError(p.Connection(ctx).Create(m)) + // We need to write manual SQL here because the INSERT should not fail if + // the UUID already exists, but we still want to return an error if anything + // else goes wrong. + var query string + switch d := p.Connection(ctx).Dialect.Name(); d { + case "mysql": + query = ` + INSERT IGNORE INTO keto_uuid_mappings (id, string_representation) + VALUES (?, ?)` + default: + query = ` + INSERT INTO keto_uuid_mappings (id, string_representation) + VALUES (?, ?) + ON CONFLICT (id) DO NOTHING` + } + + return id, sqlcon.HandleError( + p.Connection(ctx).RawQuery(query, id, text).Exec(), + ) } -func (p *Persister) LookupUUID(ctx context.Context, id uuid.UUID) (rep string, err error) { +func (p *Persister) FromUUID(ctx context.Context, id uuid.UUID) (rep string, err error) { p.d.Logger().Trace("looking up UUID") m := &UUIDMapping{} @@ -44,18 +58,3 @@ func (p *Persister) LookupUUID(ctx context.Context, id uuid.UUID) (rep string, e return m.StringRepresentation, nil } - -func (p *Persister) MappedUUID(ctx context.Context, representation string) (uuid.UUID, error) { - p.d.Logger().Trace("looking up mapped UUID") - - m := &UUIDMapping{} - if err := sqlcon.HandleError(p.Connection(ctx).Where("string_representation = ?", representation).First(m)); err != nil { - if errors.Is(err, sqlcon.ErrNoRows) { - id := uuid.Must(uuid.NewV4()) - return id, p.AddUUIDMapping(ctx, id, representation) - } - return uuid.Nil, err - } - - return m.ID, nil -} diff --git a/internal/uuidmapping/definitions.go b/internal/uuidmapping/definitions.go index ea96018ce..9ddeac751 100644 --- a/internal/uuidmapping/definitions.go +++ b/internal/uuidmapping/definitions.go @@ -15,37 +15,34 @@ type ( UUIDMappingManager() Manager } Manager interface { - // MappedUUID returns the mapped UUID for the given string - // representation. If the string representation is not mapped, a new - // UUID will be created automatically. - MappedUUID(ctx context.Context, representation string) (uuid.UUID, error) + // ToUUID returns the mapped UUID for the given string representation. + // If the string representation is not mapped, a new UUID will be + // created automatically. + ToUUID(ctx context.Context, representation string) (uuid.UUID, error) - // AddUUIDMapping adds a new mapping between a UUID and a string. - AddUUIDMapping(ctx context.Context, id uuid.UUID, representation string) error - - // LookupUUID returns the string representation for the given UUID. - LookupUUID(ctx context.Context, id uuid.UUID) (rep string, err error) + // FromUUID returns the text representation for the given UUID. + FromUUID(ctx context.Context, id uuid.UUID) (text string, err error) } ) func ManagerTest(t *testing.T, m Manager) { ctx := context.Background() - t.Run("case=add_lookup", func(t *testing.T) { - id := uuid.Must(uuid.NewV4()) + t.Run("case=ToUUID_FromUUID", func(t *testing.T) { rep1 := "foo" - require.NoError(t, m.AddUUIDMapping(ctx, id, rep1)) + id, err := m.ToUUID(ctx, rep1) + require.NoError(t, err) - rep2, err := m.LookupUUID(ctx, id) + rep2, err := m.FromUUID(ctx, id) assert.NoError(t, err) assert.Equal(t, rep1, rep2) }) - t.Run("case=MappedUUID", func(t *testing.T) { - id1, err := m.MappedUUID(ctx, "string") - require.NoError(t, err) - id2, err := m.MappedUUID(ctx, "string") - require.NoError(t, err) + t.Run("case=FromUUID", func(t *testing.T) { + id1, err := m.ToUUID(ctx, "string") + assert.NoError(t, err) + id2, err := m.ToUUID(ctx, "string") + assert.NoError(t, err) assert.Equal(t, id1, id2) }) } From c3858a29fa26c39f91e27d0ecd562dff4ac75d6f Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Wed, 16 Feb 2022 17:44:34 +0100 Subject: [PATCH 8/8] test: Add pagination test --- .../sql/migrations/uuid_mapping.go | 7 +++-- .../sql/migrations/uuid_mapping_test.go | 26 +++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/internal/persistence/sql/migrations/uuid_mapping.go b/internal/persistence/sql/migrations/uuid_mapping.go index 0b2e7b28b..960256ab4 100644 --- a/internal/persistence/sql/migrations/uuid_mapping.go +++ b/internal/persistence/sql/migrations/uuid_mapping.go @@ -12,8 +12,9 @@ import ( type ( toUUIDMappingMigrator struct { - d dependencies - perPage int + d dependencies + perPage int + requestedPages int // track the number of pages we requested for testing. } ) @@ -31,7 +32,9 @@ func (m *toUUIDMappingMigrator) MigrateUUIDMappings(ctx context.Context) error { } return p.Transaction(ctx, func(ctx context.Context, c *pop.Connection) error { + m.requestedPages = 0 for page := 1; ; page++ { + m.requestedPages++ relationTuples, hasNext, err := m.getRelationTuples(ctx, page) if err != nil { return err diff --git a/internal/persistence/sql/migrations/uuid_mapping_test.go b/internal/persistence/sql/migrations/uuid_mapping_test.go index bbfe3684d..d47233b11 100644 --- a/internal/persistence/sql/migrations/uuid_mapping_test.go +++ b/internal/persistence/sql/migrations/uuid_mapping_test.go @@ -91,6 +91,32 @@ func TestToUUIDMappingMigrator(t *testing.T) { } } +func TestToUUIDMappingMigrator_paginates(t *testing.T) { + numTuples := 10 + perPage := 2 + + dsn := dbx.GetSqlite(t, dbx.SQLiteMemory) + ctx := context.Background() + r := driver.NewTestRegistry(t, dsn) + m := &toUUIDMappingMigrator{d: r, perPage: perPage} + p := m.d.Persister().(*sql.Persister) + conn := p.Connection(ctx) + + // Create a bunch of relation tuples + for i := 0; i < numTuples; i++ { + rt := &sql.RelationTuple{ + ID: uuid.Must(uuid.NewV4()), + Object: "object", + SubjectID: dbsql.NullString{String: "subject", Valid: true}, + CommitTime: time.Now(), + } + require.NoError(t, conn.Create(rt)) + } + + require.NoError(t, m.MigrateUUIDMappings(ctx)) + assert.Equal(t, numTuples/perPage, m.requestedPages) +} + // assertHasMapping checks that there is a mapping from the given string (value) // to the given UUID (uid). func assertHasMapping(t *testing.T, conn *pop.Connection, value, uid string) {