#301 fix NPE in NotificationCenter

sialcasa · Sep 7, 2015 · 23eed47 · 23eed47
1 parent 0c0e733
commit 23eed47
Show file tree

Hide file tree

Showing 3 changed files with 68 additions and 26 deletions.
diff --git a/mvvmfx/src/main/java/de/saxsys/mvvmfx/utils/notifications/DefaultNotificationCenter.java b/mvvmfx/src/main/java/de/saxsys/mvvmfx/utils/notifications/DefaultNotificationCenter.java
@@ -73,8 +73,9 @@ public void publish(String messageName, Object... payload) {
 	 */
 	@Override
 	public void publish(ViewModel viewModel, String messageName, Object[] payload) {
-		ObserverMap observerMap = viewModelObservers.get(viewModel);
-		if (observerMap != null) {
+		if(viewModelObservers.containsKey(viewModel)) {
+			final ObserverMap observerMap = viewModelObservers.get(viewModel);
+
 			if (Platform.isFxApplicationThread()) {
 				publish(messageName, payload, observerMap);
 			} else {
@@ -96,28 +97,30 @@ public void publish(ViewModel viewModel, String messageName, Object[] payload) {
 
 
 	@Override
-	public void subscribe(ViewModel view, String messageName, NotificationObserver observer) {
-		ObserverMap observerMap = viewModelObservers.get(view);
-		if (observerMap == null) {
-			observerMap = new ObserverMap();
-			viewModelObservers.put(view, observerMap);
+	public void subscribe(ViewModel viewModel, String messageName, NotificationObserver observer) {
+		if(!viewModelObservers.containsKey(viewModel)) {
+			viewModelObservers.put(viewModel, new ObserverMap());
 		}
+
+		final ObserverMap observerMap = viewModelObservers.get(viewModel);
 		addObserver(messageName, observer, observerMap);
 	}
 
 	@Override
-	public void unsubscribe(ViewModel view, String messageName, NotificationObserver observer) {
-		ObserverMap observerMap = viewModelObservers.get(view);
-		if (observerMap != null) {
+	public void unsubscribe(ViewModel viewModel, String messageName, NotificationObserver observer) {
+		if(viewModelObservers.containsKey(viewModel)) {
+			final ObserverMap observerMap = viewModelObservers.get(viewModel);
 			removeObserversForMessageName(messageName, observer, observerMap);
 		}
 	}
 
 
 	@Override
 	public void unsubscribe(ViewModel viewModel, NotificationObserver observer) {
-		ObserverMap observerMap = viewModelObservers.get(viewModel);
-		removeObserverFromObserverMap(observer, observerMap);
+		if(viewModelObservers.containsKey(viewModel)){
+			ObserverMap observerMap = viewModelObservers.get(viewModel);
+			removeObserverFromObserverMap(observer, observerMap);
+		}
 	}
 
 	/*
@@ -138,11 +141,11 @@ private void publish(String messageName, Object[] payload, ObserverMap observerM
 	}
 
 	private void addObserver(String messageName, NotificationObserver observer, ObserverMap observerMap) {
-		List<NotificationObserver> observers = observerMap.get(messageName);
-		if (observers == null) {
-			observerMap.put(messageName, new ArrayList<NotificationObserver>());
+		if(!observerMap.containsKey(messageName)) {
+			observerMap.put(messageName, new ArrayList<>());
 		}
-		observers = observerMap.get(messageName);
+
+		final List<NotificationObserver> observers = observerMap.get(messageName);
 
 		if(observers.contains(observer)) {
 			LOG.warn("Subscribe the observer ["+ observer + "] for the message [" + messageName + 
@@ -157,21 +160,19 @@ private void removeObserverFromObserverMap(NotificationObserver observer, Observ
 		for (String key : observerMap.keySet()) {
 			final List<NotificationObserver> observers = observerMap.get(key);
 
-			final List<NotificationObserver> observersToBeRemoved = observers
-					.stream()
-					.filter(actualObserver -> actualObserver.equals(observer))
-					.collect(Collectors.toList());
-
-			observers.removeAll(observersToBeRemoved);
+			observers.removeIf(actualObserver -> actualObserver.equals(observer));
 		}
 	}
 
 	private void removeObserversForMessageName(String messageName, NotificationObserver observer,
 			ObserverMap observerMap) {
-		List<NotificationObserver> observers = observerMap.get(messageName);
-		observers.remove(observer);
-		if (observers.size() == 0) {
-			observerMap.remove(messageName);
+
+		if(observerMap.containsKey(messageName)) {
+			final List<NotificationObserver> observers = observerMap.get(messageName);
+			observers.removeIf(actualObserver -> actualObserver.equals(observer));
+			if (observers.size() == 0) {
+				observerMap.remove(messageName);
+			}
 		}
 	}
 

diff --git a/mvvmfx/src/test/java/de/saxsys/mvvmfx/utils/notifications/DefaultNotificationCenterTest.java b/mvvmfx/src/test/java/de/saxsys/mvvmfx/utils/notifications/DefaultNotificationCenterTest.java
@@ -120,6 +120,36 @@ public void unsubscribeObserverThatWasSubscribedMultipleTimes() {
 		Mockito.verify(observer1, Mockito.never()).receivedNotification(TEST_NOTIFICATION);
 	}
 
+
+	/**
+	 * This is the same as {@link #unsubscribeObserverThatWasSubscribedMultipleTimes()} with the
+	 * difference that here we use the overloaded unsubscribe method {@link NotificationCenter#unsubscribe(String, NotificationObserver)} that takes
+	 * the message key as first parameter.
+	 */
+	@Test
+	public void unsubscribeObserverThatWasSubscribedMultipleTimesViaMessageName() {
+		defaultCenter.subscribe(TEST_NOTIFICATION, observer1);
+		defaultCenter.subscribe(TEST_NOTIFICATION, observer1);
+		defaultCenter.subscribe(TEST_NOTIFICATION, observer1);
+
+		defaultCenter.unsubscribe(TEST_NOTIFICATION, observer1);
+
+		defaultCenter.publish(TEST_NOTIFICATION);
+		Mockito.verify(observer1, Mockito.never()).receivedNotification(TEST_NOTIFICATION);
+	}
+
+
+	/**
+	 * In some use cases it's convenient to unregister an observer even if it wasn't subscribed yet.
+	 * For example to prevent a duplicated subscription. 
+	 * 
+	 * This test case reproduces <a href="https://github.com/sialcasa/mvvmFX/issues/301">bug #301</a>.
+	 */
+	@Test
+	public void removeObserverThatWasNotRegisteredYet() {
+		defaultCenter.unsubscribe(TEST_NOTIFICATION, observer1);
+	}
+
 	@Test
 	public void observerForViewModelIsCalledFromUiThread() throws InterruptedException, ExecutionException, TimeoutException {
 		// Check that there is a UI-Thread available. This JUnit-Test isn't running on the UI-Thread but there needs to

diff --git a/mvvmfx/src/test/java/de/saxsys/mvvmfx/utils/notifications/ViewModelTest.java b/mvvmfx/src/test/java/de/saxsys/mvvmfx/utils/notifications/ViewModelTest.java
@@ -101,6 +101,17 @@ public void addMultipleObserverAndRemoveOneAndPublish() throws Exception {
 		Mockito.verify(observer3).receivedNotification(TEST_NOTIFICATION,
 				OBJECT_ARRAY_FOR_NOTIFICATION);
 	}
+
+
+	/**
+	 * See {@link DefaultNotificationCenterTest#removeObserverThatWasNotRegisteredYet()}.
+	 */
+	@Test
+	public void removeObserverThatWasNotRegisteredYet() {
+		viewModel.unsubscribe(observer1);
+
+		viewModel.unsubscribe(TEST_NOTIFICATION, observer1);
+	}
 
 	/**
 	 * This method is used to wait until the UI thread has done all work that was queued via