From 5ec6d4f28674c964012309398a1d02c5fa742ce0 Mon Sep 17 00:00:00 2001 From: ashmrtn <3891298+ashmrtn@users.noreply.github.com> Date: Fri, 1 Jul 2022 14:17:15 -0700 Subject: [PATCH] Base model revision (#263) * Update model interface and base struct * Update code and tests for new Model interface --- src/internal/kopia/model_store.go | 22 ++++++------ src/internal/kopia/model_store_test.go | 49 ++++++++++++++++++++------ src/internal/model/model.go | 31 ++++++---------- 3 files changed, 62 insertions(+), 40 deletions(-) diff --git a/src/internal/kopia/model_store.go b/src/internal/kopia/model_store.go index 2fb80fb59..3978a5d9f 100644 --- a/src/internal/kopia/model_store.go +++ b/src/internal/kopia/model_store.go @@ -90,17 +90,15 @@ func putInner( ctx context.Context, w repo.RepositoryWriter, t modelType, - tags map[string]string, m model.Model, create bool, ) error { - // ModelStoreID does not need to be persisted in the model itself. - m.SetModelStoreID("") + base := m.Base() if create { - m.SetStableID(model.ID(uuid.NewString())) + base.StableID = model.ID(uuid.NewString()) } - tmpTags, err := tagsForModelWithID(t, m.GetStableID(), tags) + tmpTags, err := tagsForModelWithID(t, base.StableID, base.Tags) if err != nil { // Will be wrapped at a higher layer. return err @@ -112,7 +110,7 @@ func putInner( return err } - m.SetModelStoreID(id) + base.ModelStoreID = id return nil } @@ -121,7 +119,6 @@ func putInner( func (ms *ModelStore) Put( ctx context.Context, t modelType, - tags map[string]string, m model.Model, ) error { err := repo.WriteSession( @@ -129,7 +126,7 @@ func (ms *ModelStore) Put( ms.wrapper.rep, repo.WriteSessionOptions{Purpose: "ModelStorePut"}, func(innerCtx context.Context, w repo.RepositoryWriter) error { - err := putInner(innerCtx, w, t, tags, m, true) + err := putInner(innerCtx, w, t, m, true) if err != nil { return err } @@ -169,14 +166,19 @@ func (ms *ModelStore) GetWithModelStoreID( return errors.WithStack(errNoModelStoreID) } - _, err := ms.wrapper.rep.GetManifest(ctx, id, data) + metadata, err := ms.wrapper.rep.GetManifest(ctx, id, data) // TODO(ashmrtnz): Should probably return some recognizable, non-kopia error // if not found. That way kopia doesn't need to be imported to higher layers. if err != nil { return errors.Wrap(err, "getting model data") } - data.SetModelStoreID(id) + base := data.Base() + base.Tags = metadata.Labels + // Hide the fact that StableID and modelType are just a tag from the user. + delete(base.Tags, stableIDKey) + delete(base.Tags, manifest.TypeLabelKey) + base.ModelStoreID = id return nil } diff --git a/src/internal/kopia/model_store_test.go b/src/internal/kopia/model_store_test.go index 0cc5e0ebd..428278bee 100644 --- a/src/internal/kopia/model_store_test.go +++ b/src/internal/kopia/model_store_test.go @@ -76,10 +76,12 @@ func (suite *ModelStoreIntegrationSuite) TestBadTagsErrors() { assert.NoError(t, m.wrapper.Close(ctx)) }() - foo := &fooModel{Bar: uuid.NewString()} for _, test := range table { suite.T().Run(test.name, func(t *testing.T) { - assert.Error(t, m.Put(ctx, BackupOpModel, test.tags, foo)) + foo := &fooModel{Bar: uuid.NewString()} + foo.Tags = test.tags + + assert.Error(t, m.Put(ctx, BackupOpModel, foo)) }) } } @@ -94,12 +96,12 @@ func (suite *ModelStoreIntegrationSuite) TestNoIDsErrors() { }() noStableID := &fooModel{Bar: uuid.NewString()} - noStableID.SetStableID("") - noStableID.SetModelStoreID(manifest.ID(uuid.NewString())) + noStableID.Base().StableID = "" + noStableID.Base().ModelStoreID = manifest.ID(uuid.NewString()) noModelStoreID := &fooModel{Bar: uuid.NewString()} - noModelStoreID.SetStableID(model.ID(uuid.NewString())) - noModelStoreID.SetModelStoreID("") + noModelStoreID.Base().StableID = model.ID(uuid.NewString()) + noModelStoreID.Base().ModelStoreID = "" assert.Error(t, m.GetWithModelStoreID(ctx, "", nil)) } @@ -143,25 +145,52 @@ func (suite *ModelStoreIntegrationSuite) TestPutGet() { for _, test := range table { suite.T().Run(test.t.String(), func(t *testing.T) { foo := &fooModel{Bar: uuid.NewString()} + // Avoid some silly test errors from comparing nil to empty map. + foo.Tags = map[string]string{} - err := m.Put(ctx, test.t, nil, foo) + err := m.Put(ctx, test.t, foo) test.check(t, err) if test.hasErr { return } - require.NotEmpty(t, foo.GetModelStoreID()) - require.NotEmpty(t, foo.GetStableID()) + require.NotEmpty(t, foo.Base().ModelStoreID) + require.NotEmpty(t, foo.Base().StableID) returned := &fooModel{} - err = m.GetWithModelStoreID(ctx, foo.GetModelStoreID(), returned) + err = m.GetWithModelStoreID(ctx, foo.Base().ModelStoreID, returned) require.NoError(t, err) assert.Equal(t, foo, returned) }) } } +func (suite *ModelStoreIntegrationSuite) TestPutGet_WithTags() { + ctx := context.Background() + t := suite.T() + + m := getModelStore(t, ctx) + defer func() { + assert.NoError(t, m.wrapper.Close(ctx)) + }() + + foo := &fooModel{Bar: uuid.NewString()} + foo.Tags = map[string]string{ + "bar": "baz", + } + + require.NoError(t, m.Put(ctx, BackupOpModel, foo)) + + require.NotEmpty(t, foo.Base().ModelStoreID) + require.NotEmpty(t, foo.Base().StableID) + + returned := &fooModel{} + err := m.GetWithModelStoreID(ctx, foo.Base().ModelStoreID, returned) + require.NoError(t, err) + assert.Equal(t, foo, returned) +} + func (suite *ModelStoreIntegrationSuite) TestGet_NotFoundErrors() { ctx := context.Background() t := suite.T() diff --git a/src/internal/model/model.go b/src/internal/model/model.go index 3f7c05484..edfc269ca 100644 --- a/src/internal/model/model.go +++ b/src/internal/model/model.go @@ -7,14 +7,13 @@ import ( type ID string type Model interface { - GetStableID() ID - SetStableID(id ID) - GetModelStoreID() manifest.ID - SetModelStoreID(id manifest.ID) + // Returns a handle to the BaseModel for this model. + Base() *BaseModel } // BaseModel defines required fields for models stored in ModelStore. Structs -// that wish to be stored should embed this struct. +// that wish to be stored should embed this struct. This struct also represents +// the common metadata the ModelStore will fill out/use. type BaseModel struct { // StableID is an identifier that other objects can use to refer to this // object in the ModelStore. @@ -25,21 +24,13 @@ type BaseModel struct { // can be used for efficient lookups, but should not be used by other models // to refer to this one. This field may change if the model is updated. This // field should be treated as read-only by users. - ModelStoreID manifest.ID `json:"modelStoreID,omitempty"` + ModelStoreID manifest.ID `json:"-"` + // Tags associated with this model in the store to facilitate lookup. Tags in + // the struct are not serialized directly into the stored model, but are part + // of the metadata for the model. + Tags map[string]string `json:"-"` } -func (bm *BaseModel) GetStableID() ID { - return bm.StableID -} - -func (bm *BaseModel) SetStableID(id ID) { - bm.StableID = id -} - -func (bm *BaseModel) GetModelStoreID() manifest.ID { - return bm.ModelStoreID -} - -func (bm *BaseModel) SetModelStoreID(id manifest.ID) { - bm.ModelStoreID = id +func (bm *BaseModel) Base() *BaseModel { + return bm }