fix: revert to earlier interface

Signed-off-by: Jonathan Ogilvie <[email protected]>

Author: jcogilvie

CLAUDE.md

@@ -171,6 +171,20 @@ cmd/diff/
 - Compares rendered resources against cluster state via server-side dry-run
 - Detects additions, modifications, and removals
 - Handles `generateName` by matching via labels/annotations (`crossplane.io/composition-resource-name`)
+- Uses two-phase approach to correctly handle nested XRs:
+  1. **Phase 1 - Non-removal diffs**: `CalculateNonRemovalDiffs` computes diffs for all rendered resources
+  2. **Phase 2 - Removal detection**: `CalculateRemovedResourceDiffs` identifies resources to be removed
+  - This separation is critical because nested XRs must be processed before detecting removals
+  - Nested XRs may render additional composed resources that shouldn't be marked as removals
+
+**Resource Management**
+- `ResourceManager` handles all resource fetching and cluster state operations
+- Key responsibilities:
+  - `FetchCurrentObject`: Retrieves existing resource from cluster (for identity preservation)
+  - `FetchObservedResources`: Fetches resource tree to find all composed resources (including nested)
+  - `UpdateOwnerReferences`: Updates owner references with dry-run annotations
+- Separation of concerns: `DiffCalculator` focuses on diff logic, `ResourceManager` handles cluster I/O
+- Identity preservation: Fetches existing nested XRs to maintain their cluster identity across renders
 
 ## Design Principles
 

cmd/diff/diffprocessor/diff_calculator.go

@@ -33,9 +33,9 @@ type DiffCalculator interface {
 	// Returns: (diffs map, rendered resource keys, error)
 	CalculateNonRemovalDiffs(ctx context.Context, xr *cmp.Unstructured, desired render.Outputs) (map[string]*dt.ResourceDiff, map[string]bool, error)
 
-	// DetectRemovedResources identifies resources that exist in the cluster but are not
+	// CalculateRemovedResourceDiffs identifies resources that exist in the cluster but are not
 	// in the rendered set. This is called after nested XR processing is complete.
-	DetectRemovedResources(ctx context.Context, xr *un.Unstructured, renderedResources map[string]bool) (map[string]*dt.ResourceDiff, error)
+	CalculateRemovedResourceDiffs(ctx context.Context, xr *un.Unstructured, renderedResources map[string]bool) (map[string]*dt.ResourceDiff, error)
 }
 
 // DefaultDiffCalculator implements the DiffCalculator interface.
@@ -231,23 +231,6 @@ func (c *DefaultDiffCalculator) CalculateNonRemovalDiffs(ctx context.Context, xr
 	return diffs, renderedResources, nil
 }
 
-// DetectRemovedResources identifies resources that exist in the cluster but are not in the rendered set.
-// This should be called after all nested XRs have been processed to avoid false positives.
-func (c *DefaultDiffCalculator) DetectRemovedResources(ctx context.Context, xr *un.Unstructured, renderedResources map[string]bool) (map[string]*dt.ResourceDiff, error) {
-	xrName := xr.GetName()
-	c.logger.Debug("Finding resources to be removed", "xr", xrName, "renderedCount", len(renderedResources))
-
-	removedDiffs, err := c.CalculateRemovedResourceDiffs(ctx, xr, renderedResources)
-	if err != nil {
-		c.logger.Debug("Error calculating removed resources", "error", err)
-		return nil, err
-	}
-
-	c.logger.Debug("Removal detection complete", "removedCount", len(removedDiffs), "xr", xrName)
-
-	return removedDiffs, nil
-}
-
 // CalculateDiffs computes all diffs including removals for the rendered resources.
 // This is the primary method that most code should use.
 func (c *DefaultDiffCalculator) CalculateDiffs(ctx context.Context, xr *cmp.Unstructured, desired render.Outputs) (map[string]*dt.ResourceDiff, error) {
@@ -258,7 +241,7 @@ func (c *DefaultDiffCalculator) CalculateDiffs(ctx context.Context, xr *cmp.Unst
 	}
 
 	// Then detect removed resources
-	removedDiffs, err := c.DetectRemovedResources(ctx, xr.GetUnstructured(), renderedResources)
+	removedDiffs, err := c.CalculateRemovedResourceDiffs(ctx, xr.GetUnstructured(), renderedResources)
 	if err != nil {
 		return nil, err
 	}

cmd/diff/diffprocessor/diff_calculator_test.go

@@ -731,7 +731,7 @@ func TestDefaultDiffCalculator_CalculateDiffs(t *testing.T) {
 	}
 }
 
-func TestDefaultDiffCalculator_DetectRemovedResources(t *testing.T) {
+func TestDefaultDiffCalculator_CalculateRemovedResourceDiffs(t *testing.T) {
 	ctx := t.Context()
 
 	// Create a test XR
@@ -844,16 +844,16 @@ func TestDefaultDiffCalculator_DetectRemovedResources(t *testing.T) {
 			// Setup mocks
 			applyClient, resourceTreeClient, resourceManager := tt.setupMocks(t)
 
-			// Create a diff calculator with the mocks (concrete type for testing internal method)
-			calculator := &DefaultDiffCalculator{
-				applyClient:     applyClient,
-				treeClient:      resourceTreeClient,
-				resourceManager: resourceManager,
-				logger:          logger,
-				diffOptions:     renderer.DefaultDiffOptions(),
-			}
+			// Create a diff calculator with the mocks
+			calculator := NewDiffCalculator(
+				applyClient,
+				resourceTreeClient,
+				resourceManager,
+				logger,
+				renderer.DefaultDiffOptions(),
+			)
 
-			// Call the internal method under test
+			// Call the method under test
 			diffs, err := calculator.CalculateRemovedResourceDiffs(ctx, xr, tt.renderedResources)
 
 			if tt.wantErr {

cmd/diff/diffprocessor/diff_processor.go

@@ -339,7 +339,7 @@ func (p *DefaultDiffProcessor) diffSingleResourceInternal(ctx context.Context, r
 	// which is only available on the existing cluster XR, not the modified XR from the input file.
 	var existingXR *cmp.Unstructured
 
-	xrDiffKey := fmt.Sprintf("%s/%s/%s", xr.GetAPIVersion(), xr.GetKind(), xr.GetName())
+	xrDiffKey := dt.MakeDiffKey(xr.GetAPIVersion(), xr.GetKind(), xr.GetName())
 	if xrDiff, ok := diffs[xrDiffKey]; ok && xrDiff.Current != nil {
 		// Convert from unstructured.Unstructured to composite.Unstructured
 		existingXR = cmp.New()
@@ -380,7 +380,7 @@ func (p *DefaultDiffProcessor) diffSingleResourceInternal(ctx context.Context, r
 	if detectRemovals && existingXR != nil {
 		p.config.Logger.Debug("Detecting removed resources", "resource", resourceID, "renderedCount", len(renderedResources))
 
-		removedDiffs, err := p.diffCalculator.DetectRemovedResources(ctx, existingXR.GetUnstructured(), renderedResources)
+		removedDiffs, err := p.diffCalculator.CalculateRemovedResourceDiffs(ctx, existingXR.GetUnstructured(), renderedResources)
 		if err != nil {
 			p.config.Logger.Debug("Error detecting removed resources (continuing)", "resource", resourceID, "error", err)
 		} else if len(removedDiffs) > 0 {
@@ -398,9 +398,6 @@ func (p *DefaultDiffProcessor) diffSingleResourceInternal(ctx context.Context, r
 	return diffs, renderedResources, err
 }
 
-// ProcessNestedXRs recursively processes composed resources that are themselves XRs.
-// It checks each composed resource to see if it's an XR, and if so, processes it through
-// its own composition pipeline to get the full tree of diffs.
 // findExistingNestedXR locates an existing nested XR in the observed resources by matching
 // the composition-resource-name annotation and kind.
 func findExistingNestedXR(nestedXR *un.Unstructured, observedResources []cpd.Unstructured) *un.Unstructured {

cmd/diff/diffprocessor/diff_processor_test.go

@@ -1796,8 +1796,8 @@ func TestDefaultDiffProcessor_ProcessNestedXRs(t *testing.T) {
 			},
 			parentResourceID: "XParentResource/parent-xr-abc",
 			depth:            1,
-			// TODO: This will currently fail because the nested XR gets "(generated)" name
-			// After fix, should NOT show managed resources as removed/added
+			// With identity preservation fix, nested XR maintains its cluster identity
+			// so managed resources show as modified rather than removed/added
 			wantDiffCount: 1, // Just the nested XR diff, not its managed resources as separate remove/add
 			wantErr:       false,
 		},

cmd/diff/diffprocessor/requirements_provider.go

@@ -2,12 +2,12 @@ package diffprocessor
 
 import (
 	"context"
-	"fmt"
 	"strings"
 	"sync"
 
 	xp "github.com/crossplane-contrib/crossplane-diff/cmd/diff/client/crossplane"
 	k8 "github.com/crossplane-contrib/crossplane-diff/cmd/diff/client/kubernetes"
+	dt "github.com/crossplane-contrib/crossplane-diff/cmd/diff/renderer/types"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	un "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -76,7 +76,7 @@ func (p *RequirementsProvider) cacheResources(resources []*un.Unstructured) {
 	defer p.cacheMutex.Unlock()
 
 	for _, res := range resources {
-		key := fmt.Sprintf("%s/%s/%s", res.GetAPIVersion(), res.GetKind(), res.GetName())
+		key := dt.MakeDiffKey(res.GetAPIVersion(), res.GetKind(), res.GetName())
 		p.resourceCache[key] = res
 	}
 }
@@ -86,7 +86,7 @@ func (p *RequirementsProvider) getCachedResource(apiVersion, kind, name string)
 	p.cacheMutex.RLock()
 	defer p.cacheMutex.RUnlock()
 
-	key := fmt.Sprintf("%s/%s/%s", apiVersion, kind, name)
+	key := dt.MakeDiffKey(apiVersion, kind, name)
 
 	return p.resourceCache[key]
 }

cmd/diff/testutils/mocks.go

@@ -746,10 +746,10 @@ func (m *MockResourceTreeClient) GetResourceTree(ctx context.Context, root *un.U
 
 // MockDiffCalculator is a mock implementation of DiffCalculator for testing.
 type MockDiffCalculator struct {
-	CalculateDiffFn            func(context.Context, *un.Unstructured, *un.Unstructured) (*dt.ResourceDiff, error)
-	CalculateDiffsFn           func(context.Context, *cmp.Unstructured, render.Outputs) (map[string]*dt.ResourceDiff, error)
-	CalculateNonRemovalDiffsFn func(context.Context, *cmp.Unstructured, render.Outputs) (map[string]*dt.ResourceDiff, map[string]bool, error)
-	DetectRemovedResourcesFn   func(context.Context, *un.Unstructured, map[string]bool) (map[string]*dt.ResourceDiff, error)
+	CalculateDiffFn                 func(context.Context, *un.Unstructured, *un.Unstructured) (*dt.ResourceDiff, error)
+	CalculateDiffsFn                func(context.Context, *cmp.Unstructured, render.Outputs) (map[string]*dt.ResourceDiff, error)
+	CalculateNonRemovalDiffsFn      func(context.Context, *cmp.Unstructured, render.Outputs) (map[string]*dt.ResourceDiff, map[string]bool, error)
+	CalculateRemovedResourceDiffsFn func(context.Context, *un.Unstructured, map[string]bool) (map[string]*dt.ResourceDiff, error)
 }
 
 // CalculateDiff implements DiffCalculator.
@@ -779,10 +779,10 @@ func (m *MockDiffCalculator) CalculateNonRemovalDiffs(ctx context.Context, xr *c
 	return nil, nil, nil
 }
 
-// DetectRemovedResources implements DiffCalculator.
-func (m *MockDiffCalculator) DetectRemovedResources(ctx context.Context, xr *un.Unstructured, renderedResources map[string]bool) (map[string]*dt.ResourceDiff, error) {
-	if m.DetectRemovedResourcesFn != nil {
-		return m.DetectRemovedResourcesFn(ctx, xr, renderedResources)
+// CalculateRemovedResourceDiffs implements DiffCalculator.
+func (m *MockDiffCalculator) CalculateRemovedResourceDiffs(ctx context.Context, xr *un.Unstructured, renderedResources map[string]bool) (map[string]*dt.ResourceDiff, error) {
+	if m.CalculateRemovedResourceDiffsFn != nil {
+		return m.CalculateRemovedResourceDiffsFn(ctx, xr, renderedResources)
 	}
 
 	return nil, nil