fix: project delete function uses correct string representation of id (#1733)

* fix: project delete function uses correct string representation of id

* add tests

* imrpove mocking to make sure test fail with previous implementation

* unify handling non API errors with API errors

---------

Co-authored-by: Agustin Bettati <[email protected]>

Author: oarbusi

internal/service/project/resource_project.go

@@ -269,7 +269,7 @@ func (r *projectRS) Create(ctx context.Context, req resource.CreateRequest, resp
 
 		_, _, err := connV2.TeamsApi.AddAllTeamsToProject(ctx, project.GetId(), NewTeamRoleList(ctx, teams)).Execute()
 		if err != nil {
-			errd := deleteProject(ctx, r.Client.AtlasV2, project.Id)
+			errd := deleteProject(ctx, r.Client.AtlasV2, project.GetId())
 			if errd != nil {
 				resp.Diagnostics.AddError("error during project deletion when adding teams", fmt.Sprintf(errorProjectDelete, project.GetId(), err.Error()))
 				return
@@ -290,7 +290,7 @@ func (r *projectRS) Create(ctx context.Context, req resource.CreateRequest, resp
 			}
 			_, _, err := connV2.ProjectsApi.SetProjectLimit(ctx, limit.Name.ValueString(), project.GetId(), dataFederationLimit).Execute()
 			if err != nil {
-				errd := deleteProject(ctx, r.Client.AtlasV2, project.Id)
+				errd := deleteProject(ctx, r.Client.AtlasV2, project.GetId())
 				if errd != nil {
 					resp.Diagnostics.AddError("error during project deletion when adding limits", fmt.Sprintf(errorProjectDelete, project.GetId(), err.Error()))
 					return
@@ -304,7 +304,7 @@ func (r *projectRS) Create(ctx context.Context, req resource.CreateRequest, resp
 	// add settings
 	projectSettings, _, err := connV2.ProjectsApi.GetProjectSettings(ctx, *project.Id).Execute()
 	if err != nil {
-		errd := deleteProject(ctx, r.Client.AtlasV2, project.Id)
+		errd := deleteProject(ctx, r.Client.AtlasV2, project.GetId())
 		if errd != nil {
 			resp.Diagnostics.AddError("error during project deletion when getting project settings", fmt.Sprintf(errorProjectDelete, project.GetId(), err.Error()))
 			return
@@ -333,7 +333,7 @@ func (r *projectRS) Create(ctx context.Context, req resource.CreateRequest, resp
 	}
 
 	if _, _, err = connV2.ProjectsApi.UpdateProjectSettings(ctx, project.GetId(), projectSettings).Execute(); err != nil {
-		errd := deleteProject(ctx, r.Client.AtlasV2, project.Id)
+		errd := deleteProject(ctx, r.Client.AtlasV2, project.GetId())
 		if errd != nil {
 			resp.Diagnostics.AddError("error during project deletion when updating project settings", fmt.Sprintf(errorProjectDelete, project.GetId(), err.Error()))
 			return
@@ -494,7 +494,7 @@ func (r *projectRS) Delete(ctx context.Context, req resource.DeleteRequest, resp
 	}
 
 	projectID := project.ID.ValueString()
-	err := deleteProject(ctx, r.Client.AtlasV2, &projectID)
+	err := deleteProject(ctx, r.Client.AtlasV2, projectID)
 
 	if err != nil {
 		resp.Diagnostics.AddError("error when destroying resource", fmt.Sprintf(errorProjectDelete, projectID, err.Error()))
@@ -725,11 +725,11 @@ func UpdateProject(ctx context.Context, client GroupProjectService, projectState
 	return nil
 }
 
-func deleteProject(ctx context.Context, connV2 *admin.APIClient, projectID *string) error {
+func deleteProject(ctx context.Context, connV2 *admin.APIClient, projectID string) error {
 	stateConf := &retry.StateChangeConf{
 		Pending:    []string{projectDependentsStateDeleting, projectDependentsStateRetry},
 		Target:     []string{projectDependentsStateIdle},
-		Refresh:    resourceProjectDependentsDeletingRefreshFunc(ctx, projectID, connV2),
+		Refresh:    ResourceProjectDependentsDeletingRefreshFunc(ctx, projectID, ServiceFromClient(connV2)),
 		Timeout:    30 * time.Minute,
 		MinTimeout: 30 * time.Second,
 		Delay:      0,
@@ -738,10 +738,10 @@ func deleteProject(ctx context.Context, connV2 *admin.APIClient, projectID *stri
 	_, err := stateConf.WaitForStateContext(ctx)
 
 	if err != nil {
-		tflog.Info(ctx, fmt.Sprintf("[ERROR] could not determine MongoDB project %s dependents status: %s", *projectID, err.Error()))
+		tflog.Info(ctx, fmt.Sprintf("[ERROR] could not determine MongoDB project %s dependents status: %s", projectID, err.Error()))
 	}
 
-	_, _, err = connV2.ProjectsApi.DeleteProject(ctx, *projectID).Execute()
+	_, _, err = connV2.ProjectsApi.DeleteProject(ctx, projectID).Execute()
 
 	return err
 }
@@ -755,18 +755,13 @@ Else consider the aggregate dependents idle.
 If we get a defined error response, return that right away
 Else retry
 */
-func resourceProjectDependentsDeletingRefreshFunc(ctx context.Context, projectID *string, connV2 *admin.APIClient) retry.StateRefreshFunc {
+func ResourceProjectDependentsDeletingRefreshFunc(ctx context.Context, projectID string, client GroupProjectService) retry.StateRefreshFunc {
 	return func() (any, string, error) {
-		nonNullProjectID := conversion.StringPtrNullIfEmpty(projectID)
-		clusters, _, err := connV2.ClustersApi.ListClusters(ctx, nonNullProjectID.String()).Execute()
+		clusters, _, listClustersErr := client.ListClusters(ctx, projectID)
 		dependents := AtlasProjectDependants{AdvancedClusters: clusters}
 
-		if _, ok := admin.AsError(err); ok {
-			return nil, "", err
-		}
-
-		if err != nil {
-			return nil, projectDependentsStateRetry, nil
+		if listClustersErr != nil {
+			return nil, "", listClustersErr
 		}
 
 		if *dependents.AdvancedClusters.TotalCount == 0 {
@@ -779,7 +774,7 @@ func resourceProjectDependentsDeletingRefreshFunc(ctx context.Context, projectID
 			}
 		}
 
-		log.Printf("[DEBUG] status for MongoDB project %s dependents: %s", nonNullProjectID, projectDependentsStateDeleting)
+		log.Printf("[DEBUG] status for MongoDB project %s dependents: %s", projectID, projectDependentsStateDeleting)
 
 		return dependents, projectDependentsStateDeleting, nil
 	}

internal/service/project/resource_project_test.go

@@ -42,7 +42,7 @@ var (
 	projectStateNameDiff = project.TfProjectRSModel{
 		Name: diffName,
 	}
-	dummyProjectID = "projectId"
+	dummyProjectID = "6575af27f93c7a6a4b50b239"
 )
 
 func TestGetProjectPropsFromAPI(t *testing.T) {
@@ -418,6 +418,59 @@ func TestUpdateProjectTeams(t *testing.T) {
 	}
 }
 
+func TestResourceProjectDependentsDeletingRefreshFunc(t *testing.T) {
+	testCases := []struct {
+		name          string
+		mockResponses ProjectResponse
+		expectedError bool
+	}{
+		{
+			name: "Error not from the API",
+			mockResponses: ProjectResponse{
+				clusterReponse: &admin.PaginatedAdvancedClusterDescription{},
+				Err:            errors.New("Non-API error"),
+			},
+			expectedError: true,
+		},
+		{
+			name: "Error from the API",
+			mockResponses: ProjectResponse{
+				clusterReponse: &admin.PaginatedAdvancedClusterDescription{},
+				Err:            &admin.GenericOpenAPIError{},
+			},
+			expectedError: true,
+		},
+		{
+			name: "Successful API call",
+			mockResponses: ProjectResponse{
+				clusterReponse: &admin.PaginatedAdvancedClusterDescription{
+					TotalCount: conversion.IntPtr(2),
+					Results: []admin.AdvancedClusterDescription{
+						{StateName: conversion.StringPtr("IDLE")},
+						{StateName: conversion.StringPtr("DELETING")},
+					},
+				},
+				Err: nil,
+			},
+			expectedError: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			testObject := new(MockProjectService)
+
+			testObject.On("ListClusters", mock.Anything, dummyProjectID).Return(tc.mockResponses)
+
+			_, _, err := project.ResourceProjectDependentsDeletingRefreshFunc(context.Background(), dummyProjectID, testObject)()
+
+			if (err != nil) != tc.expectedError {
+				t.Errorf("Case %s: Received unexpected error: %v", tc.name, err)
+			}
+		})
+	}
+}
+
 func TestAccProjectRSProject_basic(t *testing.T) {
 	var (
 		group        admin.Group
@@ -901,10 +954,17 @@ func (a *MockProjectService) AddAllTeamsToProject(ctx context.Context, groupID s
 	return response.ProjectTeamResp, response.HTTPResponse, response.Err
 }
 
+func (a *MockProjectService) ListClusters(ctx context.Context, groupID string) (*admin.PaginatedAdvancedClusterDescription, *http.Response, error) {
+	args := a.Called(ctx, groupID)
+	var response = args.Get(0).(ProjectResponse)
+	return response.clusterReponse, response.HTTPResponse, response.Err
+}
+
 type ProjectResponse struct {
 	ProjectResp                *admin.Group
 	ProjectTeamResp            *admin.PaginatedTeamRole
 	GroupSettingsResponse      *admin.GroupSettings
+	clusterReponse             *admin.PaginatedAdvancedClusterDescription
 	HTTPResponse               *http.Response
 	Err                        error
 	LimitResponse              admin.DataFederationLimit

internal/service/project/service_project.go

@@ -17,6 +17,7 @@ type GroupProjectService interface {
 	RemoveProjectTeam(ctx context.Context, groupID, teamID string) (*http.Response, error)
 	UpdateTeamRoles(ctx context.Context, groupID, teamID string, teamRole *admin.TeamRole) (*admin.PaginatedTeamRole, *http.Response, error)
 	AddAllTeamsToProject(ctx context.Context, groupID string, teamRole *[]admin.TeamRole) (*admin.PaginatedTeamRole, *http.Response, error)
+	ListClusters(ctx context.Context, groupID string) (*admin.PaginatedAdvancedClusterDescription, *http.Response, error)
 }
 
 type GroupProjectServiceFromClient struct {
@@ -60,6 +61,10 @@ func (a *GroupProjectServiceFromClient) AddAllTeamsToProject(ctx context.Context
 	return a.client.TeamsApi.AddAllTeamsToProject(ctx, groupID, teamRole).Execute()
 }
 
+func (a *GroupProjectServiceFromClient) ListClusters(ctx context.Context, groupID string) (*admin.PaginatedAdvancedClusterDescription, *http.Response, error) {
+	return a.client.ClustersApi.ListClusters(ctx, groupID).Execute()
+}
+
 func ServiceFromClient(client *admin.APIClient) GroupProjectService {
 	return &GroupProjectServiceFromClient{
 		client: client,