improve: extend SecondaryToPrimaryMapper so it also gets the old resource

This might needed in some corner cases where might help with optimizations to which resource to trigger.

Signed-off-by: Attila Mészáros <a_meszaros@apple.com>

Author: csviri

docs/content/en/docs/documentation/eventing.md

@@ -135,6 +135,25 @@ rare corner cases. Returning an empty set means that the mapper considered the s
 resource event as irrelevant and the SDK will thus not trigger a reconciliation of the primary
 resource in that situation.
 
+`SecondaryToPrimaryMapper` exposes two methods:
+
+- `toPrimaryResourceIDs(R resource)` — the original mapper. Implementing it is sufficient for
+  the vast majority of use cases.
+- `toPrimaryResourceIDs(R newResource, R oldResource)` — a variant that is the one actually
+  invoked by the SDK on every secondary event. Its default implementation delegates to the
+  single-argument method, so existing mappers keep working unchanged.
+
+Override the two-argument variant only in edge cases where the set of primary resources to
+reconcile depends on what changed between the previous and the new version of the secondary
+resource (e.g. a reference that moved from one primary to another, where both primaries need
+to be reconciled). **Use it with caution:** `oldResource` is sourced from the informer cache and
+is only populated for genuine update events observed while the controller is already running.
+On controller startup the cache is empty, so the initial events received for resources that
+already exist in the cluster are delivered as adds with `oldResource == null` — even if those
+resources had been updated before the operator came up. `oldResource` is also `null` for delete
+events and for events triggered through the primary-to-secondary index. Implementations must
+therefore handle a `null` `oldResource` gracefully.
+
 Adding a `SecondaryToPrimaryMapper` is typically sufficient when there is a one-to-many relationship
 between primary and secondary resources. The secondary resources can be mapped to its primary
 owner, and this is enough information to also get these secondary resources from the `Context`

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/SecondaryToPrimaryMapper.java

@@ -26,9 +26,48 @@
  */
 @FunctionalInterface
 public interface SecondaryToPrimaryMapper<R> {
+
   /**
-   * @param resource - secondary
-   * @return set of primary resource IDs
+   * Maps a secondary resource to the set of primary resources that should be reconciled in
+   * response. Implementing this single-argument form is sufficient for the vast majority of use
+   * cases — prefer it unless you specifically need access to the previous version of the
+   * secondary resource (see {@link #toPrimaryResourceIDs(Object, Object)}).
+   *
+   * @param resource the secondary resource for which an event was received
+   * @return set of primary resource IDs to enqueue for reconciliation; an empty set means the
+   *     event is irrelevant and no reconciliation is triggered
    */
   Set<ResourceID> toPrimaryResourceIDs(R resource);
+
+  /**
+   * Variant invoked by the framework for every secondary resource event, providing both the new and
+   * the previous version of the resource (when available). The default implementation simply
+   * delegates to {@link #toPrimaryResourceIDs(Object)} and ignores {@code oldResource}, so existing
+   * mappers keep working unchanged.
+   *
+   * <p>Override this method only for edge cases where the set of primary resources to reconcile
+   * depends on what changed between the old and the new version of the secondary resource (for
+   * example, when a reference held by the secondary resource has moved from one primary to another
+   * and both primaries need to be reconciled).
+   *
+   * <p><strong>Use with caution.</strong> {@code oldResource} is sourced from the informer cache
+   * and is therefore only populated for genuine update events observed while the controller is
+   * already running. In particular, when the controller starts up, the cache is empty and the
+   * initial events received for resources that already existed in the cluster are delivered as adds
+   * with {@code oldResource == null} (even if those resources had been updated previously). {@code
+   * oldResource} is also {@code null} for delete events and for events triggered through the
+   * primary-to-secondary index.
+   *
+   * <p>Implementations must therefore handle a {@code null} {@code oldResource} gracefully and not
+   * rely on it being present for correctness — overriding this method is intended for edge cases
+   * only.
+   *
+   * @param newResource the current version of the secondary resource
+   * @param oldResource the previous version of the secondary resource, or {@code null} if not
+   *     available (see above)
+   * @return set of primary resource IDs to enqueue for reconciliation
+   */
+  default Set<ResourceID> toPrimaryResourceIDs(R newResource, R oldResource) {
+    return toPrimaryResourceIDs(newResource);
+  }
 }

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/DefaultPrimaryToSecondaryIndex.java

@@ -33,7 +33,8 @@ public DefaultPrimaryToSecondaryIndex(SecondaryToPrimaryMapper<R> secondaryToPri
 
   @Override
   public synchronized void onAddOrUpdate(R resource) {
-    Set<ResourceID> primaryResources = secondaryToPrimaryMapper.toPrimaryResourceIDs(resource);
+    Set<ResourceID> primaryResources =
+        secondaryToPrimaryMapper.toPrimaryResourceIDs(resource, null);
     primaryResources.forEach(
         primaryResource -> {
           var resourceSet =
@@ -44,7 +45,8 @@ public synchronized void onAddOrUpdate(R resource) {
 
   @Override
   public synchronized void onDelete(R resource) {
-    Set<ResourceID> primaryResources = secondaryToPrimaryMapper.toPrimaryResourceIDs(resource);
+    Set<ResourceID> primaryResources =
+        secondaryToPrimaryMapper.toPrimaryResourceIDs(resource, null);
     primaryResources.forEach(
         primaryResource -> {
           var secondaryResources = index.get(primaryResource);

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java

@@ -126,15 +126,15 @@ public synchronized void onDelete(R resource, boolean deletedFinalStateUnknown)
           primaryToSecondaryIndex.onDelete(resource);
           temporaryResourceCache.onDeleteEvent(resource, deletedFinalStateUnknown);
           if (acceptedByDeleteFilters(resource, deletedFinalStateUnknown)) {
-            propagateEvent(resource);
+            propagateEvent(resource, null);
           }
         });
   }
 
   @Override
   protected void handleEvent(
       ResourceAction action, R resource, R oldResource, Boolean deletedFinalStateUnknown) {
-    propagateEvent(resource);
+    propagateEvent(resource, oldResource);
   }
 
   @Override
@@ -161,15 +161,15 @@ private synchronized void onAddOrUpdate(ResourceAction action, R newObject, R ol
       log.debug(
           "Propagating event for {}, resource with same version not result of a reconciliation.",
           action);
-      propagateEvent(newObject);
+      propagateEvent(newObject, oldObject);
     } else {
       log.debug("Event filtered out for operation: {}, resourceID: {}", action, resourceID);
     }
   }
 
-  private void propagateEvent(R object) {
+  private void propagateEvent(R resource, R oldResource) {
     var primaryResourceIdSet =
-        configuration().getSecondaryToPrimaryMapper().toPrimaryResourceIDs(object);
+        configuration().getSecondaryToPrimaryMapper().toPrimaryResourceIDs(resource, oldResource);
     if (primaryResourceIdSet.isEmpty()) {
       return;
     }

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java

@@ -87,7 +87,7 @@ void setup() {
     SecondaryToPrimaryMapper secondaryToPrimaryMapper = mock(SecondaryToPrimaryMapper.class);
     when(informerEventSourceConfiguration.getSecondaryToPrimaryMapper())
         .thenReturn(secondaryToPrimaryMapper);
-    when(secondaryToPrimaryMapper.toPrimaryResourceIDs(any()))
+    when(secondaryToPrimaryMapper.toPrimaryResourceIDs(any(), any()))
         .thenReturn(Set.of(ResourceID.fromResource(testDeployment())));
     when(informerEventSourceConfiguration.getInformerConfig()).thenReturn(informerConfig);
 

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/MappersTest.java

@@ -40,7 +40,8 @@ void secondaryToPrimaryMapperFromOwnerReference() {
     var secondary = getConfigMap(primary);
     secondary.addOwnerReference(primary);
 
-    var res = Mappers.fromOwnerReferences(TestCustomResource.class).toPrimaryResourceIDs(secondary);
+    var res =
+        Mappers.fromOwnerReferences(TestCustomResource.class).toPrimaryResourceIDs(secondary, null);
 
     assertThat(res).contains(ResourceID.fromResource(primary));
   }
@@ -65,7 +66,7 @@ void secondaryToPrimaryMapperFromOwnerReferenceWhereGroupIdIsEmpty() {
             .build();
     secondary.addOwnerReference(primary);
 
-    var res = Mappers.fromOwnerReferences(ConfigMap.class).toPrimaryResourceIDs(secondary);
+    var res = Mappers.fromOwnerReferences(ConfigMap.class).toPrimaryResourceIDs(secondary, null);
 
     assertThat(res).contains(ResourceID.fromResource(primary));
   }
@@ -79,7 +80,7 @@ void secondaryToPrimaryMapperFromOwnerReferenceFiltersByType() {
 
     var res =
         Mappers.fromOwnerReferences(TestCustomResourceOtherV1.class)
-            .toPrimaryResourceIDs(secondary);
+            .toPrimaryResourceIDs(secondary, null);
 
     assertThat(res).isEmpty();
   }
@@ -103,7 +104,7 @@ void fromOwnerReferenceIgnoresVersionFromApiVersion() {
                 HasMetadata.getGroup(TestCustomResource.class) + "/v2",
                 HasMetadata.getKind(TestCustomResource.class),
                 false)
-            .toPrimaryResourceIDs(secondary);
+            .toPrimaryResourceIDs(secondary, null);
 
     assertThat(res).contains(ResourceID.fromResource(primary));
   }

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/PrimaryToSecondaryIndexTest.java

@@ -46,7 +46,7 @@ class PrimaryToSecondaryIndexTest {
 
   @BeforeEach
   void setup() {
-    when(secondaryToPrimaryMapperMock.toPrimaryResourceIDs(any()))
+    when(secondaryToPrimaryMapperMock.toPrimaryResourceIDs(any(), any()))
         .thenReturn(Set.of(primaryID1, primaryID2));
   }