improvements on imple, fixes, unit tests

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

Author: csviri

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

@@ -210,7 +210,7 @@ public <R> List<ResourceEventSource<R, P>> getResourceEventSourcesFor(Class<R> d
   @Override
   public EventSource dynamicallyRegisterEventSource(EventSource eventSource) {
     synchronized (this) {
-      var actual = eventSources.existing(eventSource);
+      var actual = eventSources.existingEventSourceOfSameNameAndType(eventSource);
       if (actual != null) {
         eventSource = actual;
       } else {

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

@@ -1,10 +1,6 @@
 package io.javaoperatorsdk.operator.processing.event;
 
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.Objects;
+import java.util.*;
 import java.util.concurrent.ConcurrentNavigableMap;
 import java.util.concurrent.ConcurrentSkipListMap;
 import java.util.stream.Collectors;
@@ -19,9 +15,6 @@
 
 class EventSources<R extends HasMetadata> {
 
-  public static final String RETRY_RESCHEDULE_TIMER_EVENT_SOURCE_NAME =
-      "RetryAndRescheduleTimerEventSource";
-
   private final ConcurrentNavigableMap<String, Map<String, EventSource>> sources =
       new ConcurrentSkipListMap<>();
   private final TimerEventSource<R> retryAndRescheduleTimerEventSource = new TimerEventSource<>();
@@ -60,26 +53,42 @@ public void clear() {
     sources.clear();
   }
 
-  public EventSource existing(EventSource source) {
+  public EventSource existingEventSourceOfSameNameAndType(EventSource source) {
     final var eventSources = sources.get(keyFor(source));
-    if (eventSources == null || eventSources.isEmpty()) {
+    if (eventSources == null) {
       return null;
     }
     return eventSources.get(source.name());
   }
 
+
+  public Map<String, EventSource> existingEventSourceOfSameType(EventSource source) {
+    final var eventSources = sources.get(keyFor(source));
+    if (eventSources == null) {
+      return Collections.emptyMap();
+    }
+    return eventSources;
+  }
+
   public void add(EventSource eventSource) {
     final var name = eventSource.name();
-    final var existing = existing(eventSource);
-    if (existing != null && !eventSource.scopeEquals(existing)) {
+    final var existing = existingEventSourceOfSameType(eventSource);
+    if (existing.get(name) != null) {
+      throw new IllegalArgumentException("Event source " + existing
+          + " is already registered with name: " + name);
+    }
+    if (existing.values().stream().anyMatch(eventSource::scopeEquals)) {
       throw new IllegalArgumentException("Event source " + existing
           + " is already registered for the "
           + keyAsString(getResourceType(eventSource), name)
-          + " class/name combination");
+          + " with same scope");
     }
-    sources.computeIfAbsent(keyFor(existing), k -> new HashMap<>()).put(name, eventSource);
+
+    sources.computeIfAbsent(keyFor(eventSource), k -> new HashMap<>()).put(name, eventSource);
   }
 
+
+
   @SuppressWarnings("rawtypes")
   private Class<?> getResourceType(EventSource source) {
     return source instanceof ResourceEventSource

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

@@ -1,5 +1,6 @@
 package io.javaoperatorsdk.operator.processing.event.source;
 
+
 import io.javaoperatorsdk.operator.OperatorException;
 import io.javaoperatorsdk.operator.processing.event.EventHandler;
 
@@ -41,4 +42,5 @@ public AbstractEventSource setEventSourcePriority(
     this.eventSourceStartPriority = eventSourceStartPriority;
     return this;
   }
+
 }

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

@@ -33,9 +33,12 @@ default String name() {
     return getClass().getSimpleName();
   }
 
-  // todo maybe different special method name?
+  /**
+   * todo do not implement here? Check if targets the same resources as the current event source.
+   */
   default boolean scopeEquals(EventSource es) {
     return this == es;
   }
 
+
 }

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventSourceManagerTest.java

@@ -92,7 +92,7 @@ void retrievingEventSourceForClassShouldWork() {
   }
 
   @Test
-  void shouldNotBePossibleToAddEventSourcesForSameTypeAndName() {
+  void notPossibleAddEventSourcesForSameName() {
     EventSourceManager manager = initManager();
     final var name = "name1";
 
@@ -103,25 +103,48 @@ void shouldNotBePossibleToAddEventSourcesForSameTypeAndName() {
 
     eventSource = mock(ManagedInformerEventSource.class);
     when(eventSource.resourceType()).thenReturn(TestCustomResource.class);
+    when(eventSource.name()).thenReturn(name);
     final var source = eventSource;
 
     final var exception = assertThrows(OperatorException.class,
         () -> manager.registerEventSource(source));
     final var cause = exception.getCause();
     assertInstanceOf(IllegalArgumentException.class, cause);
-    assertThat(cause.getMessage()).contains(
-        "is already registered for the (io.javaoperatorsdk.operator.sample.simple.TestCustomResource, "
-            + name + ") class/name combination");
+    assertThat(cause.getMessage()).contains("is already registered with name");
   }
 
   @Test
-  void retrievingAnEventSourceWhenMultipleAreRegisteredForATypeShouldRequireAQualifier() {
+  void notPossibleToAddEventSourceWithSameScopeButDifferentName() {
     EventSourceManager manager = initManager();
+    final var name = "name1";
 
     ManagedInformerEventSource eventSource = mock(ManagedInformerEventSource.class);
+    when(eventSource.name()).thenReturn("name1");
     when(eventSource.resourceType()).thenReturn(TestCustomResource.class);
     manager.registerEventSource(eventSource);
+
+    eventSource = mock(ManagedInformerEventSource.class);
+    when(eventSource.resourceType()).thenReturn(TestCustomResource.class);
+    when(eventSource.name()).thenReturn("name2");
+    when(eventSource.scopeEquals(any())).thenReturn(true);
+    final var source = eventSource;
+
+    final var exception = assertThrows(OperatorException.class,
+        () -> manager.registerEventSource(source));
+    final var cause = exception.getCause();
+    assertInstanceOf(IllegalArgumentException.class, cause);
+    assertThat(cause.getMessage()).contains("with same scope");
+  }
+
+  @Test
+  void retrievingAnEventSourceWhenMultipleAreRegisteredForATypeShouldRequireAQualifier() {
+    EventSourceManager manager = initManager();
+
+    ManagedInformerEventSource eventSource = mock(ManagedInformerEventSource.class);
+    when(eventSource.resourceType()).thenReturn(TestCustomResource.class);
     when(eventSource.name()).thenReturn("name1");
+    manager.registerEventSource(eventSource);
+
 
     ManagedInformerEventSource eventSource2 = mock(ManagedInformerEventSource.class);
     when(eventSource2.name()).thenReturn("name2");

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventSourcesTest.java

@@ -17,6 +17,7 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
@@ -33,25 +34,39 @@ void cannotAddTwoDifferentEventSourcesWithSameName() {
     var es2 = mock(EventSource.class);
     when(es2.name()).thenReturn(EVENT_SOURCE_NAME);
 
+    eventSources.add(es1);
     assertThrows(IllegalArgumentException.class, () -> {
-      eventSources.add(es1);
       eventSources.add(es2);
     });
   }
 
   @Test
-  void cannotAddTwoEventSourcesWithSameNameUnlessTheyAreEqual() {
+  void cannotAddTwoEventSourcesWithSame() {
     final var eventSources = new EventSources();
     final var source = mock(EventSource.class);
     when(source.name()).thenReturn("name");
+    when(source.scopeEquals(any())).thenCallRealMethod();
 
     eventSources.add(source);
-    eventSources.add(source);
-
-    assertThat(eventSources.flatMappedSources())
-        .containsExactly(source);
+    assertThrows(IllegalArgumentException.class, () -> eventSources.add(source));
   }
 
+  @Test
+  void cannotAddEventSourceWithDifferentNameSameScope() {
+    final var es1 = mock(EventSource.class);
+    when(es1.name()).thenReturn("name1");
+
+    final var es2 = mock(EventSource.class);
+    when(es2.name()).thenReturn("name2");
+    when(es2.scopeEquals(any())).thenReturn(true);
+
+    final var eventSources = new EventSources();
+    eventSources.add(es1);
+
+    assertThrows(IllegalArgumentException.class, () -> {
+      eventSources.add(es2);
+    });
+  }
 
   @Test
   void eventSourcesStreamShouldNotReturnControllerEventSource() {
@@ -133,15 +148,16 @@ void getShouldWork() {
         eventSourceMockWithName(ResourceEventSource.class, "name1", HasMetadata.class);
     final var mock2 =
         eventSourceMockWithName(ResourceEventSource.class, "name2", HasMetadata.class);
+    when(mock2.scopeEquals(any())).thenCallRealMethod();
     final var mock3 = eventSourceMockWithName(ResourceEventSource.class, "name2", ConfigMap.class);
 
     eventSources.add(mock1);
-
-    eventSources.add(mock2);
     eventSources.add(mock2);
+    eventSources.add(mock3);
 
     assertEquals(mock1, eventSources.get(HasMetadata.class, "name1"));
     assertEquals(mock2, eventSources.get(HasMetadata.class, "name2"));
+    // todo this should not work?! according new rules
     assertEquals(mock3, eventSources.get(ConfigMap.class, "name2"));
     assertEquals(mock3, eventSources.get(ConfigMap.class, null));