From 6b8a625af8296e828c309baeddd547f7bd106315 Mon Sep 17 00:00:00 2001 From: Aditya Singh Date: Fri, 17 Apr 2026 15:08:12 +0530 Subject: [PATCH 1/3] Fix component type update deleting unmanaged relationships --- opslevel/resource_opslevel_component_type.go | 26 +- .../resource_opslevel_component_type_test.go | 512 ++++++++++++++++++ 2 files changed, 530 insertions(+), 8 deletions(-) create mode 100644 opslevel/resource_opslevel_component_type_test.go diff --git a/opslevel/resource_opslevel_component_type.go b/opslevel/resource_opslevel_component_type.go index 0762e31b..0bf57ee6 100644 --- a/opslevel/resource_opslevel_component_type.go +++ b/opslevel/resource_opslevel_component_type.go @@ -462,8 +462,13 @@ func (s ComponentTypeResource) Update(ctx context.Context, req resource.UpdateRe return } - if s.reconcileRelationships(ctx, err, id, resp, planModel) { - return + // Only reconcile relationships if the user manages them via Terraform + // (present in either plan or prior state). This prevents deleting + // API-side relationships that Terraform never managed. + if planModel.Relationships != nil || stateModel.Relationships != nil { + if s.reconcileRelationships(ctx, err, id, resp, planModel, stateModel) { + return + } } finalModel, err := s.NewModel(res, planModel) @@ -473,7 +478,7 @@ func (s ComponentTypeResource) Update(ctx context.Context, req resource.UpdateRe resp.Diagnostics.Append(resp.State.Set(ctx, finalModel)...) } -func (s ComponentTypeResource) reconcileRelationships(ctx context.Context, err error, id string, resp *resource.UpdateResponse, planModel ComponentTypeModel) bool { +func (s ComponentTypeResource) reconcileRelationships(ctx context.Context, err error, id string, resp *resource.UpdateResponse, planModel ComponentTypeModel, stateModel ComponentTypeModel) bool { // Handle relationship definitions // First, get existing relationship definitions existingRels, err := s.client.ListRelationshipDefinitions(&opslevel.PayloadVariables{ @@ -531,11 +536,16 @@ func (s ComponentTypeResource) reconcileRelationships(ctx context.Context, err e } } - // Delete any relationships that were removed - for _, rel := range existingRelMap { - _, err := s.client.DeleteRelationshipDefinition(string(rel.Id)) - if err != nil { - resp.Diagnostics.AddError("opslevel client error", fmt.Sprintf("unable to delete relationship definition '%s', got error: %s", rel.Alias, err)) + // Delete only relationships that were previously managed by Terraform + // (present in prior state) but explicitly removed from the plan. + // Relationships that exist on the API but were never in Terraform state + // are left untouched. + for alias, rel := range existingRelMap { + if _, wasInState := stateModel.Relationships[alias]; wasInState { + _, err := s.client.DeleteRelationshipDefinition(string(rel.Id)) + if err != nil { + resp.Diagnostics.AddError("opslevel client error", fmt.Sprintf("unable to delete relationship definition '%s', got error: %s", rel.Alias, err)) + } } } return false diff --git a/opslevel/resource_opslevel_component_type_test.go b/opslevel/resource_opslevel_component_type_test.go new file mode 100644 index 00000000..fad82a6e --- /dev/null +++ b/opslevel/resource_opslevel_component_type_test.go @@ -0,0 +1,512 @@ +package opslevel + +import ( + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + + "github.com/hashicorp/terraform-plugin-framework/attr" + "github.com/hashicorp/terraform-plugin-framework/resource" + "github.com/hashicorp/terraform-plugin-framework/types" + "github.com/opslevel/opslevel-go/v2026" + "golang.org/x/net/context" +) + +// graphqlRequest represents an incoming GraphQL request +type graphqlRequest struct { + Query string `json:"query"` + Variables json.RawMessage `json:"variables"` +} + +// testAPIServer creates an httptest server that tracks which GraphQL operations +// were called. Returns the server and a pointer to the list of operation names. +func testAPIServer(t *testing.T) (*httptest.Server, *[]string) { + t.Helper() + var mu sync.Mutex + var operations []string + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, err := io.ReadAll(r.Body) + if err != nil { + t.Fatalf("failed to read request body: %v", err) + } + + var gqlReq graphqlRequest + if err := json.Unmarshal(body, &gqlReq); err != nil { + t.Fatalf("failed to unmarshal request: %v", err) + } + + mu.Lock() + defer mu.Unlock() + + query := gqlReq.Query + + w.Header().Set("Content-Type", "application/json") + + switch { + case strings.Contains(query, "relationshipDefinitionDelete"): + operations = append(operations, "relationshipDefinitionDelete") + w.Write([]byte(`{"data":{"relationshipDefinitionDelete":{"deletedId":"Z2lkOi8vMTIz","errors":[]}}}`)) + + case strings.Contains(query, "relationshipDefinitionCreate"): + operations = append(operations, "relationshipDefinitionCreate") + w.Write([]byte(`{"data":{"relationshipDefinitionCreate":{"definition":{"id":"Z2lkOi8vbmV3","alias":"new_rel","name":"New","description":"","metadata":{"allowedCategories":[],"allowedTypes":["team"],"maxItems":0,"minItems":0},"componentType":{"id":"Z2lkOi8vY3Q","aliases":["test_type"]},"managementRules":[]},"errors":[]}}}`)) + + case strings.Contains(query, "relationshipDefinitionUpdate"): + operations = append(operations, "relationshipDefinitionUpdate") + w.Write([]byte(`{"data":{"relationshipDefinitionUpdate":{"definition":{"id":"Z2lkOi8vMTIz","alias":"managed_by","name":"Managed By","description":"","metadata":{"allowedCategories":[],"allowedTypes":["team"],"maxItems":0,"minItems":0},"componentType":{"id":"Z2lkOi8vY3Q","aliases":["test_type"]},"managementRules":[]},"errors":[]}}}`)) + + case strings.Contains(query, "relationshipDefinitions"): + operations = append(operations, "relationshipDefinitionsList") + // Return one existing relationship definition "managed_by" + w.Write([]byte(`{"data":{"account":{"relationshipDefinitions":{"nodes":[{"id":"Z2lkOi8vMTIz","alias":"managed_by","name":"Managed By","description":"A managed relationship","metadata":{"allowedCategories":[],"allowedTypes":["team"],"maxItems":0,"minItems":0},"componentType":{"id":"Z2lkOi8vY3Q","aliases":["test_type"]},"managementRules":[]}],"pageInfo":{"hasNextPage":false,"hasPreviousPage":false,"startCursor":"","endCursor":""}}}}}`)) + + default: + t.Logf("unhandled query: %s", query) + w.WriteHeader(http.StatusBadRequest) + } + })) + + return server, &operations +} + +// testAPIServerTwoRels is like testAPIServer but returns two existing +// relationships: "managed_by" and "depends_on". +func testAPIServerTwoRels(t *testing.T) (*httptest.Server, *[]string) { + t.Helper() + var mu sync.Mutex + var operations []string + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, err := io.ReadAll(r.Body) + if err != nil { + t.Fatalf("failed to read request body: %v", err) + } + + var gqlReq graphqlRequest + if err := json.Unmarshal(body, &gqlReq); err != nil { + t.Fatalf("failed to unmarshal request: %v", err) + } + + mu.Lock() + defer mu.Unlock() + + query := gqlReq.Query + + w.Header().Set("Content-Type", "application/json") + + switch { + case strings.Contains(query, "relationshipDefinitionDelete"): + operations = append(operations, "relationshipDefinitionDelete") + w.Write([]byte(`{"data":{"relationshipDefinitionDelete":{"deletedId":"Z2lkOi8vMTIz","errors":[]}}}`)) + + case strings.Contains(query, "relationshipDefinitionCreate"): + operations = append(operations, "relationshipDefinitionCreate") + w.Write([]byte(`{"data":{"relationshipDefinitionCreate":{"definition":{"id":"Z2lkOi8vbmV3","alias":"new_rel","name":"New","description":"","metadata":{"allowedCategories":[],"allowedTypes":["team"],"maxItems":0,"minItems":0},"componentType":{"id":"Z2lkOi8vY3Q","aliases":["test_type"]},"managementRules":[]},"errors":[]}}}`)) + + case strings.Contains(query, "relationshipDefinitionUpdate"): + operations = append(operations, "relationshipDefinitionUpdate") + w.Write([]byte(`{"data":{"relationshipDefinitionUpdate":{"definition":{"id":"Z2lkOi8vMTIz","alias":"managed_by","name":"Managed By","description":"","metadata":{"allowedCategories":[],"allowedTypes":["team"],"maxItems":0,"minItems":0},"componentType":{"id":"Z2lkOi8vY3Q","aliases":["test_type"]},"managementRules":[]},"errors":[]}}}`)) + + case strings.Contains(query, "relationshipDefinitions"): + operations = append(operations, "relationshipDefinitionsList") + w.Write([]byte(`{"data":{"account":{"relationshipDefinitions":{"nodes":[` + + `{"id":"Z2lkOi8vMTIz","alias":"managed_by","name":"Managed By","description":"","metadata":{"allowedCategories":[],"allowedTypes":["team"],"maxItems":0,"minItems":0},"componentType":{"id":"Z2lkOi8vY3Q","aliases":["test_type"]},"managementRules":[]},` + + `{"id":"Z2lkOi8vNDU2","alias":"depends_on","name":"Depends On","description":"","metadata":{"allowedCategories":[],"allowedTypes":["service"],"maxItems":0,"minItems":0},"componentType":{"id":"Z2lkOi8vY3Q","aliases":["test_type"]},"managementRules":[]}` + + `],"pageInfo":{"hasNextPage":false,"hasPreviousPage":false,"startCursor":"","endCursor":""}}}}}`)) + + default: + t.Logf("unhandled query: %s", query) + w.WriteHeader(http.StatusBadRequest) + } + })) + + return server, &operations +} + +func newTestClient(serverURL string) *opslevel.Client { + return opslevel.NewGQLClient( + opslevel.SetURL(serverURL+"/LOCAL_TESTING/test"), + opslevel.SetAPIToken("test-token"), + opslevel.SetMaxRetries(0), + ) +} + +func managedByRelationship() map[string]RelationshipModel { + return map[string]RelationshipModel{ + "managed_by": { + Name: types.StringValue("Managed By"), + AllowedCategories: types.ListNull(types.StringType), + AllowedTypes: types.ListValueMust(types.StringType, []attr.Value{types.StringValue("team")}), + }, + } +} + +func dependsOnRelationship() map[string]RelationshipModel { + return map[string]RelationshipModel{ + "depends_on": { + Name: types.StringValue("Depends On"), + AllowedCategories: types.ListNull(types.StringType), + AllowedTypes: types.ListValueMust(types.StringType, []attr.Value{types.StringValue("service")}), + }, + } +} + +func bothRelationships() map[string]RelationshipModel { + return map[string]RelationshipModel{ + "managed_by": { + Name: types.StringValue("Managed By"), + AllowedCategories: types.ListNull(types.StringType), + AllowedTypes: types.ListValueMust(types.StringType, []attr.Value{types.StringValue("team")}), + }, + "depends_on": { + Name: types.StringValue("Depends On"), + AllowedCategories: types.ListNull(types.StringType), + AllowedTypes: types.ListValueMust(types.StringType, []attr.Value{types.StringValue("service")}), + }, + } +} + +func countOps(ops []string, name string) int { + count := 0 + for _, op := range ops { + if op == name { + count++ + } + } + return count +} + +// TestReconcileRelationships_NilPlanNilState verifies that when neither plan +// nor state has relationships, no API calls are made to delete existing +// API-side relationships (e.g., ones created via the UI). +// Note: the guard in Update() skips calling reconcileRelationships entirely +// in this case. This test calls reconcileRelationships directly to verify +// it is also safe if called. +func TestReconcileRelationships_NilPlanNilState(t *testing.T) { + server, operations := testAPIServer(t) + defer server.Close() + + client := newTestClient(server.URL) + res := ComponentTypeResource{ + CommonResourceClient: CommonResourceClient{client: client}, + } + + ctx := context.Background() + resp := &resource.UpdateResponse{} + + planModel := ComponentTypeModel{ + Relationships: nil, + } + stateModel := ComponentTypeModel{ + Relationships: nil, + } + + res.reconcileRelationships(ctx, nil, "Z2lkOi8vY3Q", resp, planModel, stateModel) + + if resp.Diagnostics.HasError() { + t.Fatalf("unexpected errors: %v", resp.Diagnostics.Errors()) + } + + ops := *operations + if deleteCount := countOps(ops, "relationshipDefinitionDelete"); deleteCount != 0 { + t.Errorf("expected 0 deletes when plan and state are both nil, got %d. Operations: %v", deleteCount, ops) + } +} + +// TestReconcileRelationships_NilPlanWithState verifies that when relationships +// were in state but the plan drops them (user removed the block), only the +// state-managed relationships are deleted. +func TestReconcileRelationships_NilPlanWithState(t *testing.T) { + server, operations := testAPIServer(t) + defer server.Close() + + client := newTestClient(server.URL) + res := ComponentTypeResource{ + CommonResourceClient: CommonResourceClient{client: client}, + } + + ctx := context.Background() + resp := &resource.UpdateResponse{} + + planModel := ComponentTypeModel{ + Relationships: nil, // user removed the relationships block + } + stateModel := ComponentTypeModel{ + Relationships: managedByRelationship(), // was previously in state + } + + res.reconcileRelationships(ctx, nil, "Z2lkOi8vY3Q", resp, planModel, stateModel) + + if resp.Diagnostics.HasError() { + t.Fatalf("unexpected errors: %v", resp.Diagnostics.Errors()) + } + + ops := *operations + if deleteCount := countOps(ops, "relationshipDefinitionDelete"); deleteCount != 1 { + t.Errorf("expected 1 delete when state had relationship but plan removed it, got %d. Operations: %v", deleteCount, ops) + } +} + +// TestReconcileRelationships_PlanMatchesAPI verifies that when the plan includes +// the same relationships as the API, nothing is deleted -- only updated. +func TestReconcileRelationships_PlanMatchesAPI(t *testing.T) { + server, operations := testAPIServer(t) + defer server.Close() + + client := newTestClient(server.URL) + res := ComponentTypeResource{ + CommonResourceClient: CommonResourceClient{client: client}, + } + + ctx := context.Background() + resp := &resource.UpdateResponse{} + + rels := managedByRelationship() + planModel := ComponentTypeModel{ + Relationships: rels, + } + stateModel := ComponentTypeModel{ + Relationships: rels, + } + + res.reconcileRelationships(ctx, nil, "Z2lkOi8vY3Q", resp, planModel, stateModel) + + if resp.Diagnostics.HasError() { + t.Fatalf("unexpected errors: %v", resp.Diagnostics.Errors()) + } + + ops := *operations + if deleteCount := countOps(ops, "relationshipDefinitionDelete"); deleteCount != 0 { + t.Errorf("expected 0 deletes when plan matches API, got %d. Operations: %v", deleteCount, ops) + } + if updateCount := countOps(ops, "relationshipDefinitionUpdate"); updateCount != 1 { + t.Errorf("expected 1 update when plan matches API, got %d. Operations: %v", updateCount, ops) + } +} + +// TestReconcileRelationships_EmptyPlanNilState verifies that an empty map +// in plan with nil state does not delete API-side relationships. +func TestReconcileRelationships_EmptyPlanNilState(t *testing.T) { + server, operations := testAPIServer(t) + defer server.Close() + + client := newTestClient(server.URL) + res := ComponentTypeResource{ + CommonResourceClient: CommonResourceClient{client: client}, + } + + ctx := context.Background() + resp := &resource.UpdateResponse{} + + planModel := ComponentTypeModel{ + Relationships: map[string]RelationshipModel{}, // empty, not nil + } + stateModel := ComponentTypeModel{ + Relationships: nil, + } + + res.reconcileRelationships(ctx, nil, "Z2lkOi8vY3Q", resp, planModel, stateModel) + + if resp.Diagnostics.HasError() { + t.Fatalf("unexpected errors: %v", resp.Diagnostics.Errors()) + } + + ops := *operations + if deleteCount := countOps(ops, "relationshipDefinitionDelete"); deleteCount != 0 { + t.Errorf("expected 0 deletes with empty plan and nil state, got %d. Operations: %v", deleteCount, ops) + } +} + +// TestReconcileRelationships_APIHasExtraNotInState verifies that relationships +// existing on the API but never managed by Terraform (not in state) are left alone. +func TestReconcileRelationships_APIHasExtraNotInState(t *testing.T) { + server, operations := testAPIServer(t) + defer server.Close() + + client := newTestClient(server.URL) + res := ComponentTypeResource{ + CommonResourceClient: CommonResourceClient{client: client}, + } + + ctx := context.Background() + resp := &resource.UpdateResponse{} + + // Plan and state are both empty -- user never managed relationships via TF. + // But the API has "managed_by" (created via UI). + planModel := ComponentTypeModel{ + Relationships: map[string]RelationshipModel{}, + } + stateModel := ComponentTypeModel{ + Relationships: map[string]RelationshipModel{}, + } + + res.reconcileRelationships(ctx, nil, "Z2lkOi8vY3Q", resp, planModel, stateModel) + + if resp.Diagnostics.HasError() { + t.Fatalf("unexpected errors: %v", resp.Diagnostics.Errors()) + } + + ops := *operations + if deleteCount := countOps(ops, "relationshipDefinitionDelete"); deleteCount != 0 { + t.Errorf("expected 0 deletes for API-only relationships not in state, got %d. Operations: %v", deleteCount, ops) + } +} + +// TestReconcileRelationships_RemoveOneKeepOne verifies that when the user removes +// one relationship from config but keeps another, only the removed one is deleted. +func TestReconcileRelationships_RemoveOneKeepOne(t *testing.T) { + server, operations := testAPIServerTwoRels(t) + defer server.Close() + + client := newTestClient(server.URL) + res := ComponentTypeResource{ + CommonResourceClient: CommonResourceClient{client: client}, + } + + ctx := context.Background() + resp := &resource.UpdateResponse{} + + // Plan keeps "managed_by" but drops "depends_on" + planModel := ComponentTypeModel{ + Relationships: managedByRelationship(), + } + // State had both + stateModel := ComponentTypeModel{ + Relationships: bothRelationships(), + } + + res.reconcileRelationships(ctx, nil, "Z2lkOi8vY3Q", resp, planModel, stateModel) + + if resp.Diagnostics.HasError() { + t.Fatalf("unexpected errors: %v", resp.Diagnostics.Errors()) + } + + ops := *operations + if deleteCount := countOps(ops, "relationshipDefinitionDelete"); deleteCount != 1 { + t.Errorf("expected 1 delete (depends_on removed), got %d. Operations: %v", deleteCount, ops) + } + if updateCount := countOps(ops, "relationshipDefinitionUpdate"); updateCount != 1 { + t.Errorf("expected 1 update (managed_by kept), got %d. Operations: %v", updateCount, ops) + } +} + +// TestReconcileRelationships_AddNewRelationship verifies that when the plan adds +// a relationship not on the API, it gets created. +func TestReconcileRelationships_AddNewRelationship(t *testing.T) { + server, operations := testAPIServer(t) + defer server.Close() + + client := newTestClient(server.URL) + res := ComponentTypeResource{ + CommonResourceClient: CommonResourceClient{client: client}, + } + + ctx := context.Background() + resp := &resource.UpdateResponse{} + + // Plan has existing "managed_by" plus a new "depends_on" + planModel := ComponentTypeModel{ + Relationships: bothRelationships(), + } + // State only had "managed_by" + stateModel := ComponentTypeModel{ + Relationships: managedByRelationship(), + } + + res.reconcileRelationships(ctx, nil, "Z2lkOi8vY3Q", resp, planModel, stateModel) + + if resp.Diagnostics.HasError() { + t.Fatalf("unexpected errors: %v", resp.Diagnostics.Errors()) + } + + ops := *operations + if createCount := countOps(ops, "relationshipDefinitionCreate"); createCount != 1 { + t.Errorf("expected 1 create (depends_on new), got %d. Operations: %v", createCount, ops) + } + if updateCount := countOps(ops, "relationshipDefinitionUpdate"); updateCount != 1 { + t.Errorf("expected 1 update (managed_by existing), got %d. Operations: %v", updateCount, ops) + } + if deleteCount := countOps(ops, "relationshipDefinitionDelete"); deleteCount != 0 { + t.Errorf("expected 0 deletes, got %d. Operations: %v", deleteCount, ops) + } +} + +// TestReconcileRelationships_RemoveAllFromState verifies that when the user +// explicitly removes all relationships (had them in state, plan is now empty), +// all state-managed relationships are deleted. +func TestReconcileRelationships_RemoveAllFromState(t *testing.T) { + server, operations := testAPIServerTwoRels(t) + defer server.Close() + + client := newTestClient(server.URL) + res := ComponentTypeResource{ + CommonResourceClient: CommonResourceClient{client: client}, + } + + ctx := context.Background() + resp := &resource.UpdateResponse{} + + // Plan has empty relationships (user removed the block) + planModel := ComponentTypeModel{ + Relationships: map[string]RelationshipModel{}, + } + // State had both relationships + stateModel := ComponentTypeModel{ + Relationships: bothRelationships(), + } + + res.reconcileRelationships(ctx, nil, "Z2lkOi8vY3Q", resp, planModel, stateModel) + + if resp.Diagnostics.HasError() { + t.Fatalf("unexpected errors: %v", resp.Diagnostics.Errors()) + } + + ops := *operations + if deleteCount := countOps(ops, "relationshipDefinitionDelete"); deleteCount != 2 { + t.Errorf("expected 2 deletes (both removed from plan), got %d. Operations: %v", deleteCount, ops) + } +} + +// TestReconcileRelationships_StateHasOneAPIHasExtraKeepBoth verifies that when +// state manages one relationship and the API has an extra one (created via UI), +// the extra one is not deleted even when plan matches state. +func TestReconcileRelationships_StateHasOneAPIHasExtraKeepBoth(t *testing.T) { + server, operations := testAPIServerTwoRels(t) + defer server.Close() + + client := newTestClient(server.URL) + res := ComponentTypeResource{ + CommonResourceClient: CommonResourceClient{client: client}, + } + + ctx := context.Background() + resp := &resource.UpdateResponse{} + + // Plan and state both have only "managed_by". + // API has both "managed_by" and "depends_on" (depends_on created via UI). + planModel := ComponentTypeModel{ + Relationships: managedByRelationship(), + } + stateModel := ComponentTypeModel{ + Relationships: managedByRelationship(), + } + + res.reconcileRelationships(ctx, nil, "Z2lkOi8vY3Q", resp, planModel, stateModel) + + if resp.Diagnostics.HasError() { + t.Fatalf("unexpected errors: %v", resp.Diagnostics.Errors()) + } + + ops := *operations + if deleteCount := countOps(ops, "relationshipDefinitionDelete"); deleteCount != 0 { + t.Errorf("expected 0 deletes (depends_on is API-only, not in state), got %d. Operations: %v", deleteCount, ops) + } + if updateCount := countOps(ops, "relationshipDefinitionUpdate"); updateCount != 1 { + t.Errorf("expected 1 update (managed_by), got %d. Operations: %v", updateCount, ops) + } +} From e2536a0c4c1d47183b44e20306edc09a6b552c94 Mon Sep 17 00:00:00 2001 From: Aditya Singh Date: Mon, 20 Apr 2026 15:55:39 +0530 Subject: [PATCH 2/3] Address PR review: drop state-aware reconcile, rely on guard --- opslevel/resource_opslevel_component_type.go | 26 +- .../resource_opslevel_component_type_test.go | 233 ++---------------- 2 files changed, 26 insertions(+), 233 deletions(-) diff --git a/opslevel/resource_opslevel_component_type.go b/opslevel/resource_opslevel_component_type.go index 0bf57ee6..0180f9b6 100644 --- a/opslevel/resource_opslevel_component_type.go +++ b/opslevel/resource_opslevel_component_type.go @@ -462,11 +462,12 @@ func (s ComponentTypeResource) Update(ctx context.Context, req resource.UpdateRe return } - // Only reconcile relationships if the user manages them via Terraform - // (present in either plan or prior state). This prevents deleting - // API-side relationships that Terraform never managed. + // Only reconcile relationships when the user manages them via this resource's + // `relationships` attribute (present in either plan or prior state). Standalone + // relationships managed via the `opslevel_relationship_definition` resource are + // not touched here. if planModel.Relationships != nil || stateModel.Relationships != nil { - if s.reconcileRelationships(ctx, err, id, resp, planModel, stateModel) { + if s.reconcileRelationships(ctx, err, id, resp, planModel) { return } } @@ -478,7 +479,7 @@ func (s ComponentTypeResource) Update(ctx context.Context, req resource.UpdateRe resp.Diagnostics.Append(resp.State.Set(ctx, finalModel)...) } -func (s ComponentTypeResource) reconcileRelationships(ctx context.Context, err error, id string, resp *resource.UpdateResponse, planModel ComponentTypeModel, stateModel ComponentTypeModel) bool { +func (s ComponentTypeResource) reconcileRelationships(ctx context.Context, err error, id string, resp *resource.UpdateResponse, planModel ComponentTypeModel) bool { // Handle relationship definitions // First, get existing relationship definitions existingRels, err := s.client.ListRelationshipDefinitions(&opslevel.PayloadVariables{ @@ -536,16 +537,11 @@ func (s ComponentTypeResource) reconcileRelationships(ctx context.Context, err e } } - // Delete only relationships that were previously managed by Terraform - // (present in prior state) but explicitly removed from the plan. - // Relationships that exist on the API but were never in Terraform state - // are left untouched. - for alias, rel := range existingRelMap { - if _, wasInState := stateModel.Relationships[alias]; wasInState { - _, err := s.client.DeleteRelationshipDefinition(string(rel.Id)) - if err != nil { - resp.Diagnostics.AddError("opslevel client error", fmt.Sprintf("unable to delete relationship definition '%s', got error: %s", rel.Alias, err)) - } + // Delete any relationships that were removed from the plan. + for _, rel := range existingRelMap { + _, err := s.client.DeleteRelationshipDefinition(string(rel.Id)) + if err != nil { + resp.Diagnostics.AddError("opslevel client error", fmt.Sprintf("unable to delete relationship definition '%s', got error: %s", rel.Alias, err)) } } return false diff --git a/opslevel/resource_opslevel_component_type_test.go b/opslevel/resource_opslevel_component_type_test.go index fad82a6e..446bf17e 100644 --- a/opslevel/resource_opslevel_component_type_test.go +++ b/opslevel/resource_opslevel_component_type_test.go @@ -146,16 +146,6 @@ func managedByRelationship() map[string]RelationshipModel { } } -func dependsOnRelationship() map[string]RelationshipModel { - return map[string]RelationshipModel{ - "depends_on": { - Name: types.StringValue("Depends On"), - AllowedCategories: types.ListNull(types.StringType), - AllowedTypes: types.ListValueMust(types.StringType, []attr.Value{types.StringValue("service")}), - }, - } -} - func bothRelationships() map[string]RelationshipModel { return map[string]RelationshipModel{ "managed_by": { @@ -181,77 +171,6 @@ func countOps(ops []string, name string) int { return count } -// TestReconcileRelationships_NilPlanNilState verifies that when neither plan -// nor state has relationships, no API calls are made to delete existing -// API-side relationships (e.g., ones created via the UI). -// Note: the guard in Update() skips calling reconcileRelationships entirely -// in this case. This test calls reconcileRelationships directly to verify -// it is also safe if called. -func TestReconcileRelationships_NilPlanNilState(t *testing.T) { - server, operations := testAPIServer(t) - defer server.Close() - - client := newTestClient(server.URL) - res := ComponentTypeResource{ - CommonResourceClient: CommonResourceClient{client: client}, - } - - ctx := context.Background() - resp := &resource.UpdateResponse{} - - planModel := ComponentTypeModel{ - Relationships: nil, - } - stateModel := ComponentTypeModel{ - Relationships: nil, - } - - res.reconcileRelationships(ctx, nil, "Z2lkOi8vY3Q", resp, planModel, stateModel) - - if resp.Diagnostics.HasError() { - t.Fatalf("unexpected errors: %v", resp.Diagnostics.Errors()) - } - - ops := *operations - if deleteCount := countOps(ops, "relationshipDefinitionDelete"); deleteCount != 0 { - t.Errorf("expected 0 deletes when plan and state are both nil, got %d. Operations: %v", deleteCount, ops) - } -} - -// TestReconcileRelationships_NilPlanWithState verifies that when relationships -// were in state but the plan drops them (user removed the block), only the -// state-managed relationships are deleted. -func TestReconcileRelationships_NilPlanWithState(t *testing.T) { - server, operations := testAPIServer(t) - defer server.Close() - - client := newTestClient(server.URL) - res := ComponentTypeResource{ - CommonResourceClient: CommonResourceClient{client: client}, - } - - ctx := context.Background() - resp := &resource.UpdateResponse{} - - planModel := ComponentTypeModel{ - Relationships: nil, // user removed the relationships block - } - stateModel := ComponentTypeModel{ - Relationships: managedByRelationship(), // was previously in state - } - - res.reconcileRelationships(ctx, nil, "Z2lkOi8vY3Q", resp, planModel, stateModel) - - if resp.Diagnostics.HasError() { - t.Fatalf("unexpected errors: %v", resp.Diagnostics.Errors()) - } - - ops := *operations - if deleteCount := countOps(ops, "relationshipDefinitionDelete"); deleteCount != 1 { - t.Errorf("expected 1 delete when state had relationship but plan removed it, got %d. Operations: %v", deleteCount, ops) - } -} - // TestReconcileRelationships_PlanMatchesAPI verifies that when the plan includes // the same relationships as the API, nothing is deleted -- only updated. func TestReconcileRelationships_PlanMatchesAPI(t *testing.T) { @@ -266,15 +185,11 @@ func TestReconcileRelationships_PlanMatchesAPI(t *testing.T) { ctx := context.Background() resp := &resource.UpdateResponse{} - rels := managedByRelationship() planModel := ComponentTypeModel{ - Relationships: rels, - } - stateModel := ComponentTypeModel{ - Relationships: rels, + Relationships: managedByRelationship(), } - res.reconcileRelationships(ctx, nil, "Z2lkOi8vY3Q", resp, planModel, stateModel) + res.reconcileRelationships(ctx, nil, "Z2lkOi8vY3Q", resp, planModel) if resp.Diagnostics.HasError() { t.Fatalf("unexpected errors: %v", resp.Diagnostics.Errors()) @@ -289,76 +204,9 @@ func TestReconcileRelationships_PlanMatchesAPI(t *testing.T) { } } -// TestReconcileRelationships_EmptyPlanNilState verifies that an empty map -// in plan with nil state does not delete API-side relationships. -func TestReconcileRelationships_EmptyPlanNilState(t *testing.T) { - server, operations := testAPIServer(t) - defer server.Close() - - client := newTestClient(server.URL) - res := ComponentTypeResource{ - CommonResourceClient: CommonResourceClient{client: client}, - } - - ctx := context.Background() - resp := &resource.UpdateResponse{} - - planModel := ComponentTypeModel{ - Relationships: map[string]RelationshipModel{}, // empty, not nil - } - stateModel := ComponentTypeModel{ - Relationships: nil, - } - - res.reconcileRelationships(ctx, nil, "Z2lkOi8vY3Q", resp, planModel, stateModel) - - if resp.Diagnostics.HasError() { - t.Fatalf("unexpected errors: %v", resp.Diagnostics.Errors()) - } - - ops := *operations - if deleteCount := countOps(ops, "relationshipDefinitionDelete"); deleteCount != 0 { - t.Errorf("expected 0 deletes with empty plan and nil state, got %d. Operations: %v", deleteCount, ops) - } -} - -// TestReconcileRelationships_APIHasExtraNotInState verifies that relationships -// existing on the API but never managed by Terraform (not in state) are left alone. -func TestReconcileRelationships_APIHasExtraNotInState(t *testing.T) { - server, operations := testAPIServer(t) - defer server.Close() - - client := newTestClient(server.URL) - res := ComponentTypeResource{ - CommonResourceClient: CommonResourceClient{client: client}, - } - - ctx := context.Background() - resp := &resource.UpdateResponse{} - - // Plan and state are both empty -- user never managed relationships via TF. - // But the API has "managed_by" (created via UI). - planModel := ComponentTypeModel{ - Relationships: map[string]RelationshipModel{}, - } - stateModel := ComponentTypeModel{ - Relationships: map[string]RelationshipModel{}, - } - - res.reconcileRelationships(ctx, nil, "Z2lkOi8vY3Q", resp, planModel, stateModel) - - if resp.Diagnostics.HasError() { - t.Fatalf("unexpected errors: %v", resp.Diagnostics.Errors()) - } - - ops := *operations - if deleteCount := countOps(ops, "relationshipDefinitionDelete"); deleteCount != 0 { - t.Errorf("expected 0 deletes for API-only relationships not in state, got %d. Operations: %v", deleteCount, ops) - } -} - -// TestReconcileRelationships_RemoveOneKeepOne verifies that when the user removes -// one relationship from config but keeps another, only the removed one is deleted. +// TestReconcileRelationships_RemoveOneKeepOne verifies that when the plan keeps +// one relationship and drops another, the dropped one is deleted and the kept +// one is updated. func TestReconcileRelationships_RemoveOneKeepOne(t *testing.T) { server, operations := testAPIServerTwoRels(t) defer server.Close() @@ -371,16 +219,11 @@ func TestReconcileRelationships_RemoveOneKeepOne(t *testing.T) { ctx := context.Background() resp := &resource.UpdateResponse{} - // Plan keeps "managed_by" but drops "depends_on" planModel := ComponentTypeModel{ Relationships: managedByRelationship(), } - // State had both - stateModel := ComponentTypeModel{ - Relationships: bothRelationships(), - } - res.reconcileRelationships(ctx, nil, "Z2lkOi8vY3Q", resp, planModel, stateModel) + res.reconcileRelationships(ctx, nil, "Z2lkOi8vY3Q", resp, planModel) if resp.Diagnostics.HasError() { t.Fatalf("unexpected errors: %v", resp.Diagnostics.Errors()) @@ -396,7 +239,7 @@ func TestReconcileRelationships_RemoveOneKeepOne(t *testing.T) { } // TestReconcileRelationships_AddNewRelationship verifies that when the plan adds -// a relationship not on the API, it gets created. +// a relationship not on the API, it gets created and the existing one is updated. func TestReconcileRelationships_AddNewRelationship(t *testing.T) { server, operations := testAPIServer(t) defer server.Close() @@ -413,12 +256,8 @@ func TestReconcileRelationships_AddNewRelationship(t *testing.T) { planModel := ComponentTypeModel{ Relationships: bothRelationships(), } - // State only had "managed_by" - stateModel := ComponentTypeModel{ - Relationships: managedByRelationship(), - } - res.reconcileRelationships(ctx, nil, "Z2lkOi8vY3Q", resp, planModel, stateModel) + res.reconcileRelationships(ctx, nil, "Z2lkOi8vY3Q", resp, planModel) if resp.Diagnostics.HasError() { t.Fatalf("unexpected errors: %v", resp.Diagnostics.Errors()) @@ -436,10 +275,12 @@ func TestReconcileRelationships_AddNewRelationship(t *testing.T) { } } -// TestReconcileRelationships_RemoveAllFromState verifies that when the user -// explicitly removes all relationships (had them in state, plan is now empty), -// all state-managed relationships are deleted. -func TestReconcileRelationships_RemoveAllFromState(t *testing.T) { +// TestReconcileRelationships_EmptyPlanDeletesAll verifies that when the plan +// passes an empty (but non-nil) relationships map -- i.e., the user explicitly +// set `relationships = {}` -- every relationship on the component type is +// deleted. This is the contract the guard in Update() protects: reconcile only +// runs when the user is managing relationships via this resource. +func TestReconcileRelationships_EmptyPlanDeletesAll(t *testing.T) { server, operations := testAPIServerTwoRels(t) defer server.Close() @@ -451,16 +292,11 @@ func TestReconcileRelationships_RemoveAllFromState(t *testing.T) { ctx := context.Background() resp := &resource.UpdateResponse{} - // Plan has empty relationships (user removed the block) planModel := ComponentTypeModel{ Relationships: map[string]RelationshipModel{}, } - // State had both relationships - stateModel := ComponentTypeModel{ - Relationships: bothRelationships(), - } - res.reconcileRelationships(ctx, nil, "Z2lkOi8vY3Q", resp, planModel, stateModel) + res.reconcileRelationships(ctx, nil, "Z2lkOi8vY3Q", resp, planModel) if resp.Diagnostics.HasError() { t.Fatalf("unexpected errors: %v", resp.Diagnostics.Errors()) @@ -471,42 +307,3 @@ func TestReconcileRelationships_RemoveAllFromState(t *testing.T) { t.Errorf("expected 2 deletes (both removed from plan), got %d. Operations: %v", deleteCount, ops) } } - -// TestReconcileRelationships_StateHasOneAPIHasExtraKeepBoth verifies that when -// state manages one relationship and the API has an extra one (created via UI), -// the extra one is not deleted even when plan matches state. -func TestReconcileRelationships_StateHasOneAPIHasExtraKeepBoth(t *testing.T) { - server, operations := testAPIServerTwoRels(t) - defer server.Close() - - client := newTestClient(server.URL) - res := ComponentTypeResource{ - CommonResourceClient: CommonResourceClient{client: client}, - } - - ctx := context.Background() - resp := &resource.UpdateResponse{} - - // Plan and state both have only "managed_by". - // API has both "managed_by" and "depends_on" (depends_on created via UI). - planModel := ComponentTypeModel{ - Relationships: managedByRelationship(), - } - stateModel := ComponentTypeModel{ - Relationships: managedByRelationship(), - } - - res.reconcileRelationships(ctx, nil, "Z2lkOi8vY3Q", resp, planModel, stateModel) - - if resp.Diagnostics.HasError() { - t.Fatalf("unexpected errors: %v", resp.Diagnostics.Errors()) - } - - ops := *operations - if deleteCount := countOps(ops, "relationshipDefinitionDelete"); deleteCount != 0 { - t.Errorf("expected 0 deletes (depends_on is API-only, not in state), got %d. Operations: %v", deleteCount, ops) - } - if updateCount := countOps(ops, "relationshipDefinitionUpdate"); updateCount != 1 { - t.Errorf("expected 1 update (managed_by), got %d. Operations: %v", updateCount, ops) - } -} From 61afb2a0a44f71ac99fff080a9040dcc54e42cfd Mon Sep 17 00:00:00 2001 From: Aditya Singh Date: Tue, 21 Apr 2026 20:45:23 +0530 Subject: [PATCH 3/3] Add changie entry for component_type relationship fix --- .changes/unreleased/Fixed-20260421-204417.yaml | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changes/unreleased/Fixed-20260421-204417.yaml diff --git a/.changes/unreleased/Fixed-20260421-204417.yaml b/.changes/unreleased/Fixed-20260421-204417.yaml new file mode 100644 index 00000000..264d33e5 --- /dev/null +++ b/.changes/unreleased/Fixed-20260421-204417.yaml @@ -0,0 +1,3 @@ +kind: Fixed +body: Fixed `opslevel_component_type` update silently deleting relationship definitions managed via `opslevel_relationship_definition` +time: 2026-04-21T20:44:17.975274+05:30