From 4755eb51767e69497466cb35306eb7da7c66a376 Mon Sep 17 00:00:00 2001 From: Yuri Tseretyan Date: Mon, 26 Aug 2024 16:05:38 -0400 Subject: [PATCH] Alerting: Support template UID in template service (#92164) * add uid to template and populate it * update delete method to support both uid and name * update UpdateTemplate to support search by UID and fallback to name + support renaming of the template * update upsert to exit if template not found and uid is specified * update Get method to address by name or uid --------- Co-authored-by: Matthew Jacobson --- pkg/services/ngalert/api/api_provisioning.go | 8 +- .../definitions/provisioning_templates.go | 1 + .../ngalert/provisioning/templates.go | 104 +++-- .../ngalert/provisioning/templates_test.go | 369 +++++++++++++----- 4 files changed, 359 insertions(+), 123 deletions(-) diff --git a/pkg/services/ngalert/api/api_provisioning.go b/pkg/services/ngalert/api/api_provisioning.go index 6471a4a38fa..dc2131ebbdd 100644 --- a/pkg/services/ngalert/api/api_provisioning.go +++ b/pkg/services/ngalert/api/api_provisioning.go @@ -45,9 +45,9 @@ type ContactPointService interface { type TemplateService interface { GetTemplates(ctx context.Context, orgID int64) ([]definitions.NotificationTemplate, error) - GetTemplate(ctx context.Context, orgID int64, name string) (definitions.NotificationTemplate, error) + GetTemplate(ctx context.Context, orgID int64, nameOrUid string) (definitions.NotificationTemplate, error) UpsertTemplate(ctx context.Context, orgID int64, tmpl definitions.NotificationTemplate) (definitions.NotificationTemplate, error) - DeleteTemplate(ctx context.Context, orgID int64, name string, provenance definitions.Provenance, version string) error + DeleteTemplate(ctx context.Context, orgID int64, nameOrUid string, provenance definitions.Provenance, version string) error } type NotificationPolicyService interface { @@ -229,9 +229,9 @@ func (srv *ProvisioningSrv) RoutePutTemplate(c *contextmodel.ReqContext, body de return response.JSON(http.StatusAccepted, modified) } -func (srv *ProvisioningSrv) RouteDeleteTemplate(c *contextmodel.ReqContext, name string) response.Response { +func (srv *ProvisioningSrv) RouteDeleteTemplate(c *contextmodel.ReqContext, nameOrUid string) response.Response { version := c.Query("version") - err := srv.templates.DeleteTemplate(c.Req.Context(), c.SignedInUser.GetOrgID(), name, determineProvenance(c), version) + err := srv.templates.DeleteTemplate(c.Req.Context(), c.SignedInUser.GetOrgID(), nameOrUid, determineProvenance(c), version) if err != nil { return response.ErrOrFallback(http.StatusInternalServerError, "", err) } diff --git a/pkg/services/ngalert/api/tooling/definitions/provisioning_templates.go b/pkg/services/ngalert/api/tooling/definitions/provisioning_templates.go index 9c3537cd18c..f83817f7d90 100644 --- a/pkg/services/ngalert/api/tooling/definitions/provisioning_templates.go +++ b/pkg/services/ngalert/api/tooling/definitions/provisioning_templates.go @@ -55,6 +55,7 @@ type RouteDeleteTemplateParam struct { // swagger:model type NotificationTemplate struct { + UID string `json:"-" yaml:"-"` Name string `json:"name"` Template string `json:"template"` Provenance Provenance `json:"provenance,omitempty"` diff --git a/pkg/services/ngalert/provisioning/templates.go b/pkg/services/ngalert/provisioning/templates.go index 457c4549869..29bc8155e71 100644 --- a/pkg/services/ngalert/provisioning/templates.go +++ b/pkg/services/ngalert/provisioning/templates.go @@ -50,6 +50,7 @@ func (t *TemplateService) GetTemplates(ctx context.Context, orgID int64) ([]defi templates := make([]definitions.NotificationTemplate, 0, len(revision.Config.TemplateFiles)) for name, tmpl := range revision.Config.TemplateFiles { tmpl := definitions.NotificationTemplate{ + UID: legacy_storage.NameToUid(name), Name: name, Template: tmpl, ResourceVersion: calculateTemplateFingerprint(tmpl), @@ -65,30 +66,34 @@ func (t *TemplateService) GetTemplates(ctx context.Context, orgID int64) ([]defi return templates, nil } -func (t *TemplateService) GetTemplate(ctx context.Context, orgID int64, name string) (definitions.NotificationTemplate, error) { +func (t *TemplateService) GetTemplate(ctx context.Context, orgID int64, nameOrUid string) (definitions.NotificationTemplate, error) { revision, err := t.configStore.Get(ctx, orgID) if err != nil { return definitions.NotificationTemplate{}, err } - for tmplName, tmpl := range revision.Config.TemplateFiles { - if tmplName != name { - continue - } - tmpl := definitions.NotificationTemplate{ - Name: name, - Template: tmpl, - ResourceVersion: calculateTemplateFingerprint(tmpl), - } - - provenance, err := t.provenanceStore.GetProvenance(ctx, &tmpl, orgID) - if err != nil { - return definitions.NotificationTemplate{}, err - } - tmpl.Provenance = definitions.Provenance(provenance) - return tmpl, nil + existingName := nameOrUid + existingContent, ok := revision.Config.TemplateFiles[nameOrUid] + if !ok { + existingName, existingContent, ok = getTemplateByUid(revision.Config.TemplateFiles, nameOrUid) } - return definitions.NotificationTemplate{}, ErrTemplateNotFound.Errorf("") + if !ok { + return definitions.NotificationTemplate{}, ErrTemplateNotFound.Errorf("") + } + + tmpl := definitions.NotificationTemplate{ + UID: legacy_storage.NameToUid(existingName), + Name: existingName, + Template: existingContent, + ResourceVersion: calculateTemplateFingerprint(existingContent), + } + + provenance, err := t.provenanceStore.GetProvenance(ctx, &tmpl, orgID) + if err != nil { + return definitions.NotificationTemplate{}, err + } + tmpl.Provenance = definitions.Provenance(provenance) + return tmpl, nil } func (t *TemplateService) UpsertTemplate(ctx context.Context, orgID int64, tmpl definitions.NotificationTemplate) (definitions.NotificationTemplate, error) { @@ -107,7 +112,11 @@ func (t *TemplateService) UpsertTemplate(ctx context.Context, orgID int64, tmpl if !errors.Is(err, ErrTemplateNotFound) { return d, err } - if tmpl.ResourceVersion != "" { // if version is set then it's an update operation. Fail because resource does not exist anymore + // If template was not found, this is assumed to be a create operation except for two cases: + // - If a ResourceVersion is provided: we should assume that this was meant to be a conditional update operation. + // - If UID is provided: custom UID for templates is not currently supported, so this was meant to be an update + // operation without a ResourceVersion. + if tmpl.ResourceVersion != "" || tmpl.UID != "" { return definitions.NotificationTemplate{}, ErrTemplateNotFound.Errorf("") } return t.createTemplate(ctx, revision, orgID, tmpl) @@ -150,6 +159,7 @@ func (t *TemplateService) createTemplate(ctx context.Context, revision *legacy_s } return definitions.NotificationTemplate{ + UID: legacy_storage.NameToUid(tmpl.Name), Name: tmpl.Name, Template: tmpl.Template, Provenance: tmpl.Provenance, @@ -175,12 +185,28 @@ func (t *TemplateService) updateTemplate(ctx context.Context, revision *legacy_s revision.Config.TemplateFiles = map[string]string{} } - existingName := tmpl.Name - exisitingContent, found := revision.Config.TemplateFiles[existingName] + var found bool + var existingName, existingContent string + // if UID is specified, look by UID. + if tmpl.UID != "" { + existingName, existingContent, found = getTemplateByUid(revision.Config.TemplateFiles, tmpl.UID) + // do not fall back to name because we address by UID, and resource can be deleted\renamed + } else { + existingName = tmpl.Name + existingContent, found = revision.Config.TemplateFiles[existingName] + } if !found { return definitions.NotificationTemplate{}, ErrTemplateNotFound.Errorf("") } + if existingName != tmpl.Name { // if template is renamed, check if this name is already taken + _, ok := revision.Config.TemplateFiles[tmpl.Name] + if ok { + // return error if template is being renamed to one that already exists + return definitions.NotificationTemplate{}, ErrTemplateExists.Errorf("") + } + } + // check that provenance is not changed in an invalid way storedProvenance, err := t.provenanceStore.GetProvenance(ctx, &tmpl, orgID) if err != nil { @@ -190,7 +216,7 @@ func (t *TemplateService) updateTemplate(ctx context.Context, revision *legacy_s return definitions.NotificationTemplate{}, err } - err = t.checkOptimisticConcurrency(tmpl.Name, exisitingContent, models.Provenance(tmpl.Provenance), tmpl.ResourceVersion, "update") + err = t.checkOptimisticConcurrency(tmpl.Name, existingContent, models.Provenance(tmpl.Provenance), tmpl.ResourceVersion, "update") if err != nil { return definitions.NotificationTemplate{}, err } @@ -198,6 +224,14 @@ func (t *TemplateService) updateTemplate(ctx context.Context, revision *legacy_s revision.Config.TemplateFiles[tmpl.Name] = tmpl.Template err = t.xact.InTransaction(ctx, func(ctx context.Context) error { + if existingName != tmpl.Name { // if template by was found by UID and it's name is different, then this is the rename operation. Delete old resources. + delete(revision.Config.TemplateFiles, existingName) + err := t.provenanceStore.DeleteProvenance(ctx, &definitions.NotificationTemplate{Name: existingName}, orgID) + if err != nil { + return err + } + } + if err := t.configStore.Save(ctx, revision, orgID); err != nil { return err } @@ -208,6 +242,7 @@ func (t *TemplateService) updateTemplate(ctx context.Context, revision *legacy_s } return definitions.NotificationTemplate{ + UID: legacy_storage.NameToUid(tmpl.Name), // if name was changed, this UID will not match the incoming one Name: tmpl.Name, Template: tmpl.Template, Provenance: tmpl.Provenance, @@ -215,7 +250,7 @@ func (t *TemplateService) updateTemplate(ctx context.Context, revision *legacy_s }, nil } -func (t *TemplateService) DeleteTemplate(ctx context.Context, orgID int64, name string, provenance definitions.Provenance, version string) error { +func (t *TemplateService) DeleteTemplate(ctx context.Context, orgID int64, nameOrUid string, provenance definitions.Provenance, version string) error { revision, err := t.configStore.Get(ctx, orgID) if err != nil { return err @@ -225,18 +260,22 @@ func (t *TemplateService) DeleteTemplate(ctx context.Context, orgID int64, name return nil } - existing, ok := revision.Config.TemplateFiles[name] + existingName := nameOrUid + existing, ok := revision.Config.TemplateFiles[nameOrUid] + if !ok { + existingName, existing, ok = getTemplateByUid(revision.Config.TemplateFiles, nameOrUid) + } if !ok { return nil } - err = t.checkOptimisticConcurrency(name, existing, models.Provenance(provenance), version, "delete") + err = t.checkOptimisticConcurrency(existingName, existing, models.Provenance(provenance), version, "delete") if err != nil { return err } // check that provenance is not changed in an invalid way - storedProvenance, err := t.provenanceStore.GetProvenance(ctx, &definitions.NotificationTemplate{Name: name}, orgID) + storedProvenance, err := t.provenanceStore.GetProvenance(ctx, &definitions.NotificationTemplate{Name: existingName}, orgID) if err != nil { return err } @@ -244,14 +283,14 @@ func (t *TemplateService) DeleteTemplate(ctx context.Context, orgID int64, name return err } - delete(revision.Config.TemplateFiles, name) + delete(revision.Config.TemplateFiles, existingName) return t.xact.InTransaction(ctx, func(ctx context.Context) error { if err := t.configStore.Save(ctx, revision, orgID); err != nil { return err } tgt := definitions.NotificationTemplate{ - Name: name, + Name: existingName, } return t.provenanceStore.DeleteProvenance(ctx, &tgt, orgID) }) @@ -277,3 +316,12 @@ func calculateTemplateFingerprint(t string) string { _, _ = sum.Write(unsafe.Slice(unsafe.StringData(t), len(t))) //nolint:gosec return fmt.Sprintf("%016x", sum.Sum64()) } + +func getTemplateByUid(templates map[string]string, uid string) (string, string, bool) { + for n, tmpl := range templates { + if legacy_storage.NameToUid(n) == uid { + return n, tmpl, true + } + } + return "", "", false +} diff --git a/pkg/services/ngalert/provisioning/templates_test.go b/pkg/services/ngalert/provisioning/templates_test.go index d7e316defcd..050777e7691 100644 --- a/pkg/services/ngalert/provisioning/templates_test.go +++ b/pkg/services/ngalert/provisioning/templates_test.go @@ -46,18 +46,21 @@ func TestGetTemplates(t *testing.T) { expected := []definitions.NotificationTemplate{ { + UID: legacy_storage.NameToUid("template1"), Name: "template1", Template: "test1", Provenance: definitions.Provenance(models.ProvenanceAPI), ResourceVersion: calculateTemplateFingerprint("test1"), }, { + UID: legacy_storage.NameToUid("template2"), Name: "template2", Template: "test2", Provenance: definitions.Provenance(models.ProvenanceFile), ResourceVersion: calculateTemplateFingerprint("test2"), }, { + UID: legacy_storage.NameToUid("template3"), Name: "template3", Template: "test3", Provenance: definitions.Provenance(models.ProvenanceNone), @@ -144,6 +147,7 @@ func TestGetTemplate(t *testing.T) { require.NoError(t, err) expected := definitions.NotificationTemplate{ + UID: legacy_storage.NameToUid(templateName), Name: templateName, Template: templateContent, Provenance: definitions.Provenance(models.ProvenanceAPI), @@ -244,6 +248,7 @@ func TestUpsertTemplate(t *testing.T) { require.NoError(t, err) require.Equal(t, definitions.NotificationTemplate{ + UID: legacy_storage.NameToUid(tmpl.Name), Name: tmpl.Name, Template: tmpl.Template, Provenance: tmpl.Provenance, @@ -285,6 +290,7 @@ func TestUpsertTemplate(t *testing.T) { require.NoError(t, err) assert.Equal(t, definitions.NotificationTemplate{ + UID: legacy_storage.NameToUid(tmpl.Name), Name: tmpl.Name, Template: tmpl.Template, Provenance: tmpl.Provenance, @@ -321,6 +327,7 @@ func TestUpsertTemplate(t *testing.T) { require.NoError(t, err) assert.Equal(t, definitions.NotificationTemplate{ + UID: legacy_storage.NameToUid(tmpl.Name), Name: tmpl.Name, Template: tmpl.Template, Provenance: tmpl.Provenance, @@ -355,6 +362,7 @@ func TestUpsertTemplate(t *testing.T) { expectedContent := fmt.Sprintf("{{ define \"%s\" }}\n content\n{{ end }}", templateName) require.Equal(t, definitions.NotificationTemplate{ + UID: legacy_storage.NameToUid(tmpl.Name), Name: tmpl.Name, Template: expectedContent, Provenance: tmpl.Provenance, @@ -465,6 +473,22 @@ func TestUpsertTemplate(t *testing.T) { _, err := sut.UpsertTemplate(context.Background(), orgID, template) require.ErrorIs(t, err, ErrTemplateNotFound) }) + + t.Run("rejects new template has UID ", func(t *testing.T) { + sut, store, _ := createTemplateServiceSut() + store.GetFn = func(ctx context.Context, org int64) (*legacy_storage.ConfigRevision, error) { + return revision(), nil + } + template := definitions.NotificationTemplate{ + UID: "new-template", + Name: "template2", + Template: "asdf-new", + Provenance: definitions.Provenance(models.ProvenanceNone), + } + _, err := sut.UpsertTemplate(context.Background(), orgID, template) + require.ErrorIs(t, err, ErrTemplateNotFound) + }) + t.Run("propagates errors", func(t *testing.T) { tmpl := definitions.NotificationTemplate{ Name: templateName, @@ -565,6 +589,7 @@ func TestCreateTemplate(t *testing.T) { require.NoError(t, err) require.Equal(t, definitions.NotificationTemplate{ + UID: legacy_storage.NameToUid(tmpl.Name), Name: tmpl.Name, Template: tmpl.Template, Provenance: tmpl.Provenance, @@ -695,7 +720,7 @@ func TestUpdateTemplate(t *testing.T) { } } - t.Run("returns ErrTemplateNotFound if template does not exist", func(t *testing.T) { + t.Run("returns ErrTemplateNotFound if template name does not exist", func(t *testing.T) { sut, store, prov := createTemplateServiceSut() store.GetFn = func(ctx context.Context, org int64) (*legacy_storage.ConfigRevision, error) { assert.Equal(t, orgID, org) @@ -712,62 +737,171 @@ func TestUpdateTemplate(t *testing.T) { prov.AssertExpectations(t) }) - t.Run("updates current template", func(t *testing.T) { - t.Run("when version matches", func(t *testing.T) { - sut, store, prov := createTemplateServiceSut() - store.GetFn = func(ctx context.Context, org int64) (*legacy_storage.ConfigRevision, error) { - return revision(), nil - } - prov.EXPECT().GetProvenance(mock.Anything, mock.Anything, mock.Anything).Return(models.ProvenanceAPI, nil) - prov.EXPECT().SetProvenance(mock.Anything, mock.Anything, mock.Anything, mock.Anything).Run(func(ctx context.Context, o models.Provisionable, org int64, p models.Provenance) { - assertInTransaction(t, ctx) - }).Return(nil) + t.Run("returns ErrTemplateNotFound if template UID does not exist", func(t *testing.T) { + sut, store, prov := createTemplateServiceSut() + store.GetFn = func(ctx context.Context, org int64) (*legacy_storage.ConfigRevision, error) { + assert.Equal(t, orgID, org) + return &legacy_storage.ConfigRevision{ + Config: &definitions.PostableUserConfig{ + TemplateFiles: map[string]string{ + "not-found": "test", // create a template with name that matches UID to make sure we do not search by name + tmpl.Name: "test", + }, + }, + ConcurrencyToken: amConfigToken, + }, nil + } + tmpl := tmpl + tmpl.UID = "not-found" + _, err := sut.UpdateTemplate(context.Background(), orgID, tmpl) - result, err := sut.UpdateTemplate(context.Background(), orgID, tmpl) + require.ErrorIs(t, err, ErrTemplateNotFound) - require.NoError(t, err) - assert.Equal(t, definitions.NotificationTemplate{ - Name: tmpl.Name, - Template: tmpl.Template, - Provenance: tmpl.Provenance, - ResourceVersion: calculateTemplateFingerprint(tmpl.Template), - }, result) + require.Len(t, store.Calls, 1) + prov.AssertExpectations(t) + }) - require.Len(t, store.Calls, 2) - require.Equal(t, "Save", store.Calls[1].Method) - saved := store.Calls[1].Args[1].(*legacy_storage.ConfigRevision) - assert.Equal(t, amConfigToken, saved.ConcurrencyToken) - assert.Contains(t, saved.Config.TemplateFiles, tmpl.Name) - assert.Equal(t, tmpl.Template, saved.Config.TemplateFiles[tmpl.Name]) + testcases := []struct { + name string + templateUid string + }{ + { + name: "by name", + templateUid: "", + }, + { + name: "by uid", + templateUid: legacy_storage.NameToUid(tmpl.UID), + }, + } - prov.AssertExpectations(t) + for _, tt := range testcases { + t.Run(fmt.Sprintf("updates current template %s", tt.name), func(t *testing.T) { + t.Run("when version matches", func(t *testing.T) { + sut, store, prov := createTemplateServiceSut() + store.GetFn = func(ctx context.Context, org int64) (*legacy_storage.ConfigRevision, error) { + return revision(), nil + } + prov.EXPECT().GetProvenance(mock.Anything, mock.Anything, mock.Anything).Return(models.ProvenanceAPI, nil) + prov.EXPECT().SetProvenance(mock.Anything, mock.Anything, mock.Anything, mock.Anything).Run(func(ctx context.Context, o models.Provisionable, org int64, p models.Provenance) { + assertInTransaction(t, ctx) + }).Return(nil) + + tmpl.UID = tt.templateUid + result, err := sut.UpdateTemplate(context.Background(), orgID, tmpl) + + require.NoError(t, err) + assert.Equal(t, definitions.NotificationTemplate{ + UID: legacy_storage.NameToUid(tmpl.Name), + Name: tmpl.Name, + Template: tmpl.Template, + Provenance: tmpl.Provenance, + ResourceVersion: calculateTemplateFingerprint(tmpl.Template), + }, result) + + require.Len(t, store.Calls, 2) + require.Equal(t, "Save", store.Calls[1].Method) + saved := store.Calls[1].Args[1].(*legacy_storage.ConfigRevision) + assert.Equal(t, amConfigToken, saved.ConcurrencyToken) + assert.Contains(t, saved.Config.TemplateFiles, tmpl.Name) + assert.Equal(t, tmpl.Template, saved.Config.TemplateFiles[tmpl.Name]) + + prov.AssertExpectations(t) + }) + t.Run("bypasses optimistic concurrency validation when version is empty", func(t *testing.T) { + sut, store, prov := createTemplateServiceSut() + store.GetFn = func(ctx context.Context, org int64) (*legacy_storage.ConfigRevision, error) { + return revision(), nil + } + prov.EXPECT().GetProvenance(mock.Anything, mock.Anything, mock.Anything).Return(models.ProvenanceAPI, nil) + prov.EXPECT().SetProvenance(mock.Anything, mock.Anything, mock.Anything, mock.Anything).Run(func(ctx context.Context, o models.Provisionable, org int64, p models.Provenance) { + assertInTransaction(t, ctx) + }).Return(nil) + + result, err := sut.UpdateTemplate(context.Background(), orgID, tmpl) + + require.NoError(t, err) + assert.Equal(t, definitions.NotificationTemplate{ + UID: legacy_storage.NameToUid(tmpl.Name), + Name: tmpl.Name, + Template: tmpl.Template, + Provenance: tmpl.Provenance, + ResourceVersion: calculateTemplateFingerprint(tmpl.Template), + }, result) + + require.Equal(t, "Save", store.Calls[1].Method) + saved := store.Calls[1].Args[1].(*legacy_storage.ConfigRevision) + assert.Equal(t, amConfigToken, saved.ConcurrencyToken) + assert.Contains(t, saved.Config.TemplateFiles, tmpl.Name) + assert.Equal(t, tmpl.Template, saved.Config.TemplateFiles[tmpl.Name]) + }) }) - t.Run("bypasses optimistic concurrency validation when version is empty", func(t *testing.T) { - sut, store, prov := createTemplateServiceSut() - store.GetFn = func(ctx context.Context, org int64) (*legacy_storage.ConfigRevision, error) { - return revision(), nil - } - prov.EXPECT().GetProvenance(mock.Anything, mock.Anything, mock.Anything).Return(models.ProvenanceAPI, nil) - prov.EXPECT().SetProvenance(mock.Anything, mock.Anything, mock.Anything, mock.Anything).Run(func(ctx context.Context, o models.Provisionable, org int64, p models.Provenance) { - assertInTransaction(t, ctx) - }).Return(nil) + } - result, err := sut.UpdateTemplate(context.Background(), orgID, tmpl) + t.Run("creates a new template and delete old one when template is renamed", func(t *testing.T) { + sut, store, prov := createTemplateServiceSut() + store.GetFn = func(ctx context.Context, org int64) (*legacy_storage.ConfigRevision, error) { + return revision(), nil + } + prov.EXPECT().GetProvenance(mock.Anything, mock.Anything, mock.Anything).Return(models.ProvenanceAPI, nil) + prov.EXPECT().DeleteProvenance(mock.Anything, mock.Anything, mock.Anything).Return(nil).Run(func(ctx context.Context, o models.Provisionable, org int64) { + assertInTransaction(t, ctx) + }).Return(nil) + prov.EXPECT().SetProvenance(mock.Anything, mock.Anything, mock.Anything, mock.Anything).Run(func(ctx context.Context, o models.Provisionable, org int64, p models.Provenance) { + assertInTransaction(t, ctx) + }).Return(nil) - require.NoError(t, err) - assert.Equal(t, definitions.NotificationTemplate{ - Name: tmpl.Name, - Template: tmpl.Template, - Provenance: tmpl.Provenance, - ResourceVersion: calculateTemplateFingerprint(tmpl.Template), - }, result) + oldName := tmpl.Name + tmpl := tmpl + tmpl.UID = legacy_storage.NameToUid(tmpl.Name) // UID matches the current template + tmpl.Name = "new-template-name" // but name is different + result, err := sut.UpdateTemplate(context.Background(), orgID, tmpl) - require.Equal(t, "Save", store.Calls[1].Method) - saved := store.Calls[1].Args[1].(*legacy_storage.ConfigRevision) - assert.Equal(t, amConfigToken, saved.ConcurrencyToken) - assert.Contains(t, saved.Config.TemplateFiles, tmpl.Name) - assert.Equal(t, tmpl.Template, saved.Config.TemplateFiles[tmpl.Name]) - }) + require.NoError(t, err) + assert.Equal(t, definitions.NotificationTemplate{ + UID: legacy_storage.NameToUid(tmpl.Name), + Name: tmpl.Name, + Template: tmpl.Template, + Provenance: tmpl.Provenance, + ResourceVersion: calculateTemplateFingerprint(tmpl.Template), + }, result) + + require.Len(t, store.Calls, 2) + require.Equal(t, "Save", store.Calls[1].Method) + saved := store.Calls[1].Args[1].(*legacy_storage.ConfigRevision) + assert.Equal(t, amConfigToken, saved.ConcurrencyToken) + assert.Contains(t, saved.Config.TemplateFiles, tmpl.Name) + assert.Equal(t, tmpl.Template, saved.Config.TemplateFiles[tmpl.Name]) + assert.NotContains(t, saved.Config.TemplateFiles, oldName) + + prov.AssertCalled(t, "DeleteProvenance", mock.Anything, mock.MatchedBy(func(t *definitions.NotificationTemplate) bool { + return t.Name == oldName + }), mock.Anything) + prov.AssertExpectations(t) + }) + + t.Run("rejects rename operation if template with the new name exists", func(t *testing.T) { + sut, store, prov := createTemplateServiceSut() + store.GetFn = func(ctx context.Context, org int64) (*legacy_storage.ConfigRevision, error) { + return &legacy_storage.ConfigRevision{ + Config: &definitions.PostableUserConfig{ + TemplateFiles: map[string]string{ + tmpl.Name: currentTemplateContent, + "new-template-name": "test", + }, + }, + ConcurrencyToken: amConfigToken, + }, nil + } + + tmpl := tmpl + tmpl.UID = legacy_storage.NameToUid(tmpl.Name) // UID matches the current template + tmpl.Name = "new-template-name" // but name matches another existing template + _, err := sut.UpdateTemplate(context.Background(), orgID, tmpl) + + require.ErrorIs(t, err, ErrTemplateExists) + + prov.AssertExpectations(t) }) t.Run("rejects templates that fail validation", func(t *testing.T) { @@ -918,61 +1052,114 @@ func TestDeleteTemplate(t *testing.T) { } } - t.Run("deletes template from config file on success", func(t *testing.T) { - t.Run("when version matches", func(t *testing.T) { - sut, store, prov := createTemplateServiceSut() - store.GetFn = func(ctx context.Context, orgID int64) (*legacy_storage.ConfigRevision, error) { - return revision(), nil - } - prov.EXPECT().GetProvenance(mock.Anything, mock.Anything, mock.Anything).Return(models.ProvenanceFile, nil) - prov.EXPECT().DeleteProvenance(mock.Anything, mock.Anything, mock.Anything).Run(func(ctx context.Context, o models.Provisionable, org int64) { - assertInTransaction(t, ctx) - }).Return(nil) + testCase := []struct { + name string + templateNameOrUid string + }{ + { + name: "by name", + templateNameOrUid: templateName, + }, + { + name: "by uid", + templateNameOrUid: legacy_storage.NameToUid(templateName), + }, + } + for _, tt := range testCase { + t.Run(fmt.Sprintf("deletes template from config file %s", tt.name), func(t *testing.T) { + t.Run("when version matches", func(t *testing.T) { + sut, store, prov := createTemplateServiceSut() + store.GetFn = func(ctx context.Context, orgID int64) (*legacy_storage.ConfigRevision, error) { + return revision(), nil + } + prov.EXPECT().GetProvenance(mock.Anything, mock.Anything, mock.Anything).Return(models.ProvenanceFile, nil) + prov.EXPECT().DeleteProvenance(mock.Anything, mock.Anything, mock.Anything).Run(func(ctx context.Context, o models.Provisionable, org int64) { + assertInTransaction(t, ctx) + }).Return(nil) - err := sut.DeleteTemplate(context.Background(), orgID, templateName, definitions.Provenance(models.ProvenanceFile), templateVersion) + err := sut.DeleteTemplate(context.Background(), orgID, tt.templateNameOrUid, definitions.Provenance(models.ProvenanceFile), templateVersion) - require.NoError(t, err) + require.NoError(t, err) - require.Len(t, store.Calls, 2) + require.Len(t, store.Calls, 2) - require.Equal(t, "Save", store.Calls[1].Method) - saved := store.Calls[1].Args[1].(*legacy_storage.ConfigRevision) - assert.Equal(t, amConfigToken, saved.ConcurrencyToken) - assert.NotContains(t, saved.Config.TemplateFiles, templateName) + require.Equal(t, "Save", store.Calls[1].Method) + saved := store.Calls[1].Args[1].(*legacy_storage.ConfigRevision) + assert.Equal(t, amConfigToken, saved.ConcurrencyToken) + assert.NotContains(t, saved.Config.TemplateFiles, templateName) - prov.AssertCalled(t, "DeleteProvenance", mock.Anything, mock.MatchedBy(func(t *definitions.NotificationTemplate) bool { - return t.Name == templateName - }), orgID) + prov.AssertCalled(t, "DeleteProvenance", mock.Anything, mock.MatchedBy(func(t *definitions.NotificationTemplate) bool { + return t.Name == templateName + }), orgID) - prov.AssertExpectations(t) + prov.AssertExpectations(t) + }) + + t.Run("bypasses optimistic concurrency when version is empty", func(t *testing.T) { + sut, store, prov := createTemplateServiceSut() + store.GetFn = func(ctx context.Context, orgID int64) (*legacy_storage.ConfigRevision, error) { + return revision(), nil + } + prov.EXPECT().GetProvenance(mock.Anything, mock.Anything, mock.Anything).Return(models.ProvenanceFile, nil) + prov.EXPECT().DeleteProvenance(mock.Anything, mock.Anything, mock.Anything).Run(func(ctx context.Context, o models.Provisionable, org int64) { + assertInTransaction(t, ctx) + }).Return(nil) + + err := sut.DeleteTemplate(context.Background(), orgID, tt.templateNameOrUid, definitions.Provenance(models.ProvenanceFile), "") + + require.NoError(t, err) + require.Len(t, store.Calls, 2) + + require.Equal(t, "Save", store.Calls[1].Method) + saved := store.Calls[1].Args[1].(*legacy_storage.ConfigRevision) + assert.Equal(t, amConfigToken, saved.ConcurrencyToken) + assert.NotContains(t, saved.Config.TemplateFiles, templateName) + + prov.AssertCalled(t, "DeleteProvenance", mock.Anything, mock.MatchedBy(func(t *definitions.NotificationTemplate) bool { + return t.Name == templateName + }), orgID) + + prov.AssertExpectations(t) + }) }) + } - t.Run("bypasses optimistic concurrency when version is empty", func(t *testing.T) { - sut, store, prov := createTemplateServiceSut() - store.GetFn = func(ctx context.Context, orgID int64) (*legacy_storage.ConfigRevision, error) { - return revision(), nil - } - prov.EXPECT().GetProvenance(mock.Anything, mock.Anything, mock.Anything).Return(models.ProvenanceFile, nil) - prov.EXPECT().DeleteProvenance(mock.Anything, mock.Anything, mock.Anything).Run(func(ctx context.Context, o models.Provisionable, org int64) { - assertInTransaction(t, ctx) - }).Return(nil) + t.Run("should look by name before uid", func(t *testing.T) { + expectedToDelete := legacy_storage.NameToUid(templateName) + sut, store, prov := createTemplateServiceSut() + store.GetFn = func(ctx context.Context, orgID int64) (*legacy_storage.ConfigRevision, error) { + return &legacy_storage.ConfigRevision{ + Config: &definitions.PostableUserConfig{ + TemplateFiles: map[string]string{ + templateName: templateContent, + expectedToDelete: templateContent, + }, + }, + ConcurrencyToken: amConfigToken, + }, nil + } + prov.EXPECT().GetProvenance(mock.Anything, mock.Anything, mock.Anything).Return(models.ProvenanceFile, nil) + prov.EXPECT().DeleteProvenance(mock.Anything, mock.Anything, mock.Anything).Run(func(ctx context.Context, o models.Provisionable, org int64) { + assertInTransaction(t, ctx) + }).Return(nil) - err := sut.DeleteTemplate(context.Background(), orgID, templateName, definitions.Provenance(models.ProvenanceFile), "") + err := sut.DeleteTemplate(context.Background(), orgID, expectedToDelete, definitions.Provenance(models.ProvenanceFile), templateVersion) - require.NoError(t, err) - require.Len(t, store.Calls, 2) + require.NoError(t, err) - require.Equal(t, "Save", store.Calls[1].Method) - saved := store.Calls[1].Args[1].(*legacy_storage.ConfigRevision) - assert.Equal(t, amConfigToken, saved.ConcurrencyToken) - assert.NotContains(t, saved.Config.TemplateFiles, templateName) + require.Len(t, store.Calls, 2) - prov.AssertCalled(t, "DeleteProvenance", mock.Anything, mock.MatchedBy(func(t *definitions.NotificationTemplate) bool { - return t.Name == templateName - }), orgID) + require.Equal(t, "Save", store.Calls[1].Method) + saved := store.Calls[1].Args[1].(*legacy_storage.ConfigRevision) + assert.Equal(t, amConfigToken, saved.ConcurrencyToken) + assert.NotContains(t, saved.Config.TemplateFiles, expectedToDelete) + assert.Contains(t, saved.Config.TemplateFiles, templateName) - prov.AssertExpectations(t) - }) + prov.AssertCalled(t, "DeleteProvenance", mock.Anything, mock.MatchedBy(func(t *definitions.NotificationTemplate) bool { + return t.Name == expectedToDelete + }), orgID) + + prov.AssertExpectations(t) }) t.Run("does not error when deleting templates that do not exist", func(t *testing.T) {