From a77abffb1bb05ed9878345bacd45213bf65e23bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Klaus=20Strie=C3=9Fnig?= Date: Tue, 9 Dec 2025 14:25:25 +0100 Subject: [PATCH 1/2] feat(document): generate delete file with objectID if a customID is set If a custom ID is set, the direct reference (type and objectId) should be used for the delete file --- cmd/monaco/generate/deletefile/deletefile.go | 8 +++++ test/commands/deletefile_test.go | 30 ++++++++++++++++++- .../project/document-dashboard/config.yaml | 9 ++++++ 3 files changed, 46 insertions(+), 1 deletion(-) diff --git a/cmd/monaco/generate/deletefile/deletefile.go b/cmd/monaco/generate/deletefile/deletefile.go index 081970175..1c0f6bd78 100644 --- a/cmd/monaco/generate/deletefile/deletefile.go +++ b/cmd/monaco/generate/deletefile/deletefile.go @@ -227,6 +227,14 @@ func createDeleteEntry(c config.Config, apis api.APIs, project project.Project) return createConfigAPIEntry(c, apis, project) } + // An externalID can't be used if there is a customID set on a DocumentType + if docType, isDocument := c.Type.(config.DocumentType); isDocument && docType.CustomID != "" { + return persistence.DeleteEntry{ + Type: c.Coordinate.Type, + ObjectId: docType.CustomID, + }, nil + } + return persistence.DeleteEntry{ Project: c.Coordinate.Project, Type: c.Coordinate.Type, diff --git a/test/commands/deletefile_test.go b/test/commands/deletefile_test.go index dc5695f65..6f1097b9d 100644 --- a/test/commands/deletefile_test.go +++ b/test/commands/deletefile_test.go @@ -101,7 +101,7 @@ func TestGeneratesValidDeleteFile(t *testing.T) { assertDeleteEntries(t, entries, "scheduling-rule", "ca-scheduling-rule") assertDeleteEntries(t, entries, "workflow", "ca-jira-issue-workflow") assertDeleteEntries(t, entries, "bucket", "my-bucket") - assertDeleteEntries(t, entries, "document", "my-dashboard", "my-notebook") + assertDeleteEntries(t, entries, "document", "my-dashboard", "my-notebook", "" /*non-coordinate references (direct ones) don't have an identifier */) assertDeleteEntries(t, entries, "segment", "segmentID") assert.Empty(t, entries[api.DashboardShareSettings]) @@ -134,6 +134,34 @@ func TestGeneratesValidDeleteFileWithCustomValues(t *testing.T) { }) } +func TestGeneratesValidDeleteFileWithCorrectDocumentPointers(t *testing.T) { + t.Setenv("TOKEN", "some-value") + + fs := testutils.CreateTestFileSystem() + outputFolder := "output-folder" + err := monaco.Run(t, fs, fmt.Sprintf("monaco generate deletefile ./testdata/deletefile/manifest.yaml --types=document --output-folder=%s", outputFolder)) + require.NoError(t, err) + + expectedFile := filepath.Join(outputFolder, "delete.yaml") + assertFileExists(t, fs, expectedFile) + + deleteFileContent := readFile(t, fs, expectedFile) + + var deleteEntries persistence.FullFileDefinition + err = yaml.Unmarshal(deleteFileContent, &deleteEntries) + require.NoError(t, err) + + assert.Contains(t, deleteEntries.DeleteEntries, persistence.DeleteEntry{ + Type: string(config.DocumentTypeID), + ObjectId: "custom-id", + }) + assert.Contains(t, deleteEntries.DeleteEntries, persistence.DeleteEntry{ + Type: string(config.DocumentTypeID), + Project: "project", + ConfigId: "my-dashboard", + }) +} + func TestGeneratesValidDeleteFileWithFilter(t *testing.T) { t.Setenv("TOKEN", "some-value") diff --git a/test/commands/testdata/deletefile/project/document-dashboard/config.yaml b/test/commands/testdata/deletefile/project/document-dashboard/config.yaml index c451bf3da..fb88f8993 100644 --- a/test/commands/testdata/deletefile/project/document-dashboard/config.yaml +++ b/test/commands/testdata/deletefile/project/document-dashboard/config.yaml @@ -7,3 +7,12 @@ configs: type: document: kind: dashboard +- id: my-dashboard-with-custom-id + config: + name: My dashboard with a custom ID + template: dashboard.json + skip: false + type: + document: + kind: dashboard + id: custom-id From 2f20736781ab4605c3ce9c5f3dec4f104a33de5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Klaus=20Strie=C3=9Fnig?= Date: Tue, 9 Dec 2025 14:26:56 +0100 Subject: [PATCH 2/2] refactor(document): remove externalID usage in delete and treat it as an ID The external ID should be treated as an ID. Therefore, the delete doesn't filter the documents based on the externalID anymore and just tries to delete a document via the external ID. --- pkg/delete/delete_test.go | 28 +--------- pkg/resource/document/delete.go | 35 +----------- pkg/resource/document/delete_test.go | 83 ++-------------------------- 3 files changed, 9 insertions(+), 137 deletions(-) diff --git a/pkg/delete/delete_test.go b/pkg/delete/delete_test.go index c57de016e..2b4d72d3c 100644 --- a/pkg/delete/delete_test.go +++ b/pkg/delete/delete_test.go @@ -38,7 +38,6 @@ import ( "github.com/dynatrace/dynatrace-configuration-as-code-core/api/rest" "github.com/dynatrace/dynatrace-configuration-as-code-core/clients/automation" "github.com/dynatrace/dynatrace-configuration-as-code-core/clients/buckets" - "github.com/dynatrace/dynatrace-configuration-as-code-core/clients/documents" "github.com/dynatrace/dynatrace-configuration-as-code/v2/internal/idutils" "github.com/dynatrace/dynatrace-configuration-as-code/v2/internal/log" "github.com/dynatrace/dynatrace-configuration-as-code/v2/internal/testutils/matcher" @@ -954,10 +953,7 @@ func TestDelete_Documents(t *testing.T) { externalID := idutils.GenerateExternalID(given.AsCoordinate()) c := client.NewMockDocumentClient(gomock.NewController(t)) - c.EXPECT().List(gomock.Any(), gomock.Eq(fmt.Sprintf("externalId=='%s'", externalID))). - Times(1). - Return(documents.ListResponse{Responses: []documents.Response{{Metadata: documents.Metadata{ID: "originObjectID"}}}}, nil) - c.EXPECT().Delete(gomock.Any(), gomock.Eq("originObjectID")).Times(1) + c.EXPECT().Delete(gomock.Any(), gomock.Eq(externalID)).Times(1) entriesToDelete := delete.DeleteEntries{given.Type: {given}} err := delete.Configs(t.Context(), client.ClientSet{DocumentClient: c}, entriesToDelete) @@ -973,33 +969,13 @@ func TestDelete_Documents(t *testing.T) { externalID := idutils.GenerateExternalID(given.AsCoordinate()) c := client.NewMockDocumentClient(gomock.NewController(t)) - c.EXPECT().List(gomock.Any(), gomock.Eq(fmt.Sprintf("externalId=='%s'", externalID))). - Times(1). - Return(documents.ListResponse{Responses: []documents.Response{}}, nil) + c.EXPECT().Delete(gomock.Any(), gomock.Eq(externalID)).Times(1).Return(libAPI.Response{}, coreapi.APIError{StatusCode: http.StatusNotFound}) entriesToDelete := delete.DeleteEntries{given.Type: {given}} err := delete.Configs(t.Context(), client.ClientSet{DocumentClient: c}, entriesToDelete) assert.NoError(t, err) }) - t.Run("config declared via coordinate have multiple match - no delete, no error", func(t *testing.T) { - given := pointer.DeletePointer{ - Type: "document", - Identifier: "monaco_identifier", - Project: "project", - } - - externalID := idutils.GenerateExternalID(given.AsCoordinate()) - c := client.NewMockDocumentClient(gomock.NewController(t)) - c.EXPECT().List(gomock.Any(), gomock.Eq(fmt.Sprintf("externalId=='%s'", externalID))). - Times(1). - Return(documents.ListResponse{Responses: []documents.Response{{Metadata: documents.Metadata{ID: "originObjectID_1"}}, {Metadata: documents.Metadata{ID: "originObjectID_2"}}}}, nil) - - entriesToDelete := delete.DeleteEntries{given.Type: {given}} - err := delete.Configs(t.Context(), client.ClientSet{DocumentClient: c}, entriesToDelete) - assert.Error(t, err) - }) - t.Run("delete via originID", func(t *testing.T) { c := client.NewMockDocumentClient(gomock.NewController(t)) c.EXPECT().Delete(gomock.Any(), gomock.Eq("originObjectID")).Times(1) diff --git a/pkg/resource/document/delete.go b/pkg/resource/document/delete.go index 6f62c4e4e..ed9316a1d 100644 --- a/pkg/resource/document/delete.go +++ b/pkg/resource/document/delete.go @@ -63,22 +63,8 @@ func (d Deleter) deleteSingle(ctx context.Context, dp pointer.DeletePointer) err if id == "" { coordinate := dp.AsCoordinate() - logger = slog.With(log.CoordinateAttr(coordinate)) - extID := idutils.GenerateExternalID(coordinate) - - var err error - id, err = d.tryGetDocumentIDByExternalID(ctx, extID) - if err != nil { - logger.ErrorContext(ctx, "Failed to get document by external ID", slog.String("externalId", extID), log.ErrorAttr(err)) - return err - } - - if id == "" { - logger.DebugContext(ctx, "No document found with external ID", slog.String("externalId", extID)) - return nil - } - - logger = logger.With(slog.String("id", id)) + id = idutils.GenerateExternalID(coordinate) + logger = slog.With(log.CoordinateAttr(coordinate), slog.String("id", id)) } _, err := d.source.Delete(ctx, id) @@ -91,23 +77,6 @@ func (d Deleter) deleteSingle(ctx context.Context, dp pointer.DeletePointer) err return nil } -func (d Deleter) tryGetDocumentIDByExternalID(ctx context.Context, externalId string) (string, error) { - switch listResponse, err := d.source.List(ctx, fmt.Sprintf("externalId=='%s'", externalId)); { - case err != nil: - return "", err - case len(listResponse.Responses) == 0: - return "", nil - case len(listResponse.Responses) > 1: - var ids []string - for _, r := range listResponse.Responses { - ids = append(ids, r.ID) - } - return "", fmt.Errorf("found more than one document with same externalId (%s); matching document IDs: %s", externalId, ids) - default: - return listResponse.Responses[0].ID, nil - } -} - func (d Deleter) DeleteAll(ctx context.Context) error { listResponse, err := d.source.List(ctx, getFilterForAllSupportedDocumentTypes()) if err != nil { diff --git a/pkg/resource/document/delete_test.go b/pkg/resource/document/delete_test.go index eb849d566..dc4c7a1a0 100644 --- a/pkg/resource/document/delete_test.go +++ b/pkg/resource/document/delete_test.go @@ -21,7 +21,6 @@ package document_test import ( "context" "errors" - "fmt" "net/http" "testing" @@ -49,10 +48,6 @@ func (s *deleteStubClient) Delete(_ context.Context, id string) (libAPI.Response return s.delete(id) } -func externalIDToFilter(externalId string) string { - return fmt.Sprintf("externalId=='%s'", externalId) -} - func TestDeleteByCoordinate(t *testing.T) { t.Run("success if one document matches generated external ID", func(t *testing.T) { given := pointer.DeletePointer{ @@ -63,20 +58,8 @@ func TestDeleteByCoordinate(t *testing.T) { externalID := idutils.GenerateExternalID(given.AsCoordinate()) c := deleteStubClient{ - list: func(filter string) (documents.ListResponse, error) { - assert.Equal(t, externalIDToFilter(externalID), filter) - return documents.ListResponse{ - libAPI.Response{}, - []documents.Response{ - { - libAPI.Response{}, - documents.Metadata{ID: "uid_1"}, - }, - }, - }, nil - }, delete: func(id string) (libAPI.Response, error) { - assert.Equal(t, "uid_1", id) + assert.Equal(t, externalID, id) return libAPI.Response{}, nil }, } @@ -95,71 +78,15 @@ func TestDeleteByCoordinate(t *testing.T) { externalID := idutils.GenerateExternalID(given.AsCoordinate()) c := deleteStubClient{ - list: func(filter string) (documents.ListResponse, error) { - assert.Equal(t, externalIDToFilter(externalID), filter) - return documents.ListResponse{ - libAPI.Response{}, - []documents.Response{}, - }, nil + delete: func(id string) (libAPI.Response, error) { + assert.Equal(t, externalID, id) + return libAPI.Response{}, libAPI.APIError{StatusCode: http.StatusNotFound} }, } err := document.NewDeleter(&c).Delete(t.Context(), []pointer.DeletePointer{given}) assert.NoError(t, err) - assert.False(t, c.deleteCalled, "delete command was invoked") - }) - - t.Run("error if multiple documents are matching generated external ID", func(t *testing.T) { - given := pointer.DeletePointer{ - Type: "document", - Identifier: "monaco_identifier", - Project: "project", - } - - externalID := idutils.GenerateExternalID(given.AsCoordinate()) - c := deleteStubClient{ - list: func(filter string) (documents.ListResponse, error) { - assert.Equal(t, externalIDToFilter(externalID), filter) - return documents.ListResponse{ - libAPI.Response{}, - []documents.Response{ - { - libAPI.Response{}, - documents.Metadata{ID: "uid_1"}, - }, - { - libAPI.Response{}, - documents.Metadata{ID: "uid_2"}, - }, - }, - }, nil - }, - } - - err := document.NewDeleter(&c).Delete(t.Context(), []pointer.DeletePointer{given}) - assert.Error(t, err) - assert.False(t, c.deleteCalled, "it's not known what needs to be deleted") - }) - - t.Run("error if list fails", func(t *testing.T) { - given := pointer.DeletePointer{ - Type: "document", - Identifier: "monaco_identifier", - Project: "project", - } - - c := deleteStubClient{ - list: func(filter string) (documents.ListResponse, error) { - return documents.ListResponse{ - libAPI.Response{}, - []documents.Response{}, - }, errors.New("some unpredictable error") - }, - } - - err := document.NewDeleter(&c).Delete(t.Context(), []pointer.DeletePointer{given}) - assert.Error(t, err) - assert.False(t, c.deleteCalled, "delete command was invoked") + assert.True(t, c.deleteCalled, "delete command was invoked") }) }