From 103650db89d9c924abede07e4fc4246890f052cc Mon Sep 17 00:00:00 2001 From: Simon Urli Date: Tue, 13 Feb 2024 16:14:48 +0100 Subject: [PATCH] XWIKI-21880: Cannot delete user defined filters (#2881) * Refactor DefaultNotificationFilterPreferenceManager to never throw immediately an exception thrown by a provider, but to only do it if all providers failed * Provide a new test covering this behaviour * Small test improvment * Improve a bit the logic to use standard exception if there's only one provider --- ...ltNotificationFilterPreferenceManager.java | 189 ++++++++++-------- ...tificationFilterPreferenceManagerTest.java | 52 ++++- 2 files changed, 151 insertions(+), 90 deletions(-) diff --git a/xwiki-platform-core/xwiki-platform-notifications/xwiki-platform-notifications-filters/xwiki-platform-notifications-filters-api/src/main/java/org/xwiki/notifications/filters/internal/DefaultNotificationFilterPreferenceManager.java b/xwiki-platform-core/xwiki-platform-notifications/xwiki-platform-notifications-filters/xwiki-platform-notifications-filters-api/src/main/java/org/xwiki/notifications/filters/internal/DefaultNotificationFilterPreferenceManager.java index 6ab7662f768e..b2d02ca16b37 100644 --- a/xwiki-platform-core/xwiki-platform-notifications/xwiki-platform-notifications-filters/xwiki-platform-notifications-filters-api/src/main/java/org/xwiki/notifications/filters/internal/DefaultNotificationFilterPreferenceManager.java +++ b/xwiki-platform-core/xwiki-platform-notifications/xwiki-platform-notifications-filters/xwiki-platform-notifications-filters-api/src/main/java/org/xwiki/notifications/filters/internal/DefaultNotificationFilterPreferenceManager.java @@ -19,6 +19,7 @@ */ package org.xwiki.notifications.filters.internal; +import java.util.ArrayList; import java.util.Collection; import java.util.Date; import java.util.HashMap; @@ -26,6 +27,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.stream.Collectors; import java.util.stream.Stream; import javax.inject.Inject; @@ -56,55 +58,109 @@ @Singleton public class DefaultNotificationFilterPreferenceManager implements NotificationFilterPreferenceManager { - private static final String ENABLE_ERROR_MESSAGE = "Failed to enable or disabled the filter preference [{}]."; - @Inject private ComponentManager componentManager; @Inject private Logger logger; - @Override - public Collection getFilterPreferences(DocumentReference user) - throws NotificationException + @FunctionalInterface + interface ProviderCallable { - Set filterPreferences = new HashSet<>(); - - try { - List providers - = componentManager.getInstanceList(NotificationFilterPreferenceProvider.class); + void doInProvider(NotificationFilterPreferenceProvider provider) throws NotificationException; + } - for (NotificationFilterPreferenceProvider provider : providers) { - filterPreferences.addAll(provider.getFilterPreferences(user)); - } + @FunctionalInterface + interface RetrieveWithProviderCallable + { + Collection retrieveWithProvider(NotificationFilterPreferenceProvider provider) + throws NotificationException; + } - return filterPreferences; + private List getProviderList() throws NotificationException + { + try { + return componentManager.getInstanceList(NotificationFilterPreferenceProvider.class); } catch (ComponentLookupException e) { - throw new NotificationException( - String.format("Unable to fetch a list of notification preference providers with user [%s].", user)); + throw new NotificationException("Error when trying to load the list of providers", e); } } - @Override - public Collection getFilterPreferences(WikiReference wikiReference) - throws NotificationException + private String getProviderDebugMessage(String loggerMessage, NotificationFilterPreferenceProvider provider) { - Set filterPreferences = new HashSet<>(); + return String.format("%s with provider %s", loggerMessage, provider); + } - try { - List providers - = componentManager.getInstanceList(NotificationFilterPreferenceProvider.class); + private String getExceptionMessage(String loggerMessage, List exceptions) + { + return String.format("%s - All providers called failed, see exceptions: [%s].", + loggerMessage, + exceptions.stream().map(ExceptionUtils::getRootCauseMessage).collect(Collectors.joining(","))); + } - for (NotificationFilterPreferenceProvider provider : providers) { - filterPreferences.addAll(provider.getFilterPreferences(wikiReference)); + private void providerExceptionWrapper(ProviderCallable callable, String loggerMessage) throws NotificationException + { + boolean allFailing = true; + List exceptions = new ArrayList<>(); + List providerList = getProviderList(); + if (providerList.size() > 1) { + for (NotificationFilterPreferenceProvider provider : providerList) { + try { + callable.doInProvider(provider); + allFailing = false; + } catch (NotificationException e) { + this.logger.debug(getProviderDebugMessage(loggerMessage, provider), e); + exceptions.add(e); + } } + if (allFailing) { + throw new NotificationException(getExceptionMessage(loggerMessage, exceptions)); + } + } else { + callable.doInProvider(providerList.get(0)); + } + } - return filterPreferences; - } catch (ComponentLookupException e) { - throw new NotificationException( - String.format("Unable to fetch a list of notification preference providers with wiki [%s].", - wikiReference)); + private Collection retrieveWithProviderExceptionWrapper(RetrieveWithProviderCallable callable, + String loggerMessage) throws NotificationException + { + boolean allFailing = true; + List exceptions = new ArrayList<>(); + Set result = new HashSet<>(); + List providerList = getProviderList(); + if (providerList.size() > 1) { + for (NotificationFilterPreferenceProvider provider : getProviderList()) { + try { + result.addAll(callable.retrieveWithProvider(provider)); + allFailing = false; + } catch (NotificationException e) { + this.logger.debug(getProviderDebugMessage(loggerMessage, provider), e); + exceptions.add(e); + } + } + if (allFailing) { + throw new NotificationException(getExceptionMessage(loggerMessage, exceptions)); + } + } else { + result.addAll(callable.retrieveWithProvider(providerList.get(0))); } + return result; + } + + @Override + public Collection getFilterPreferences(DocumentReference user) + throws NotificationException + { + return this.retrieveWithProviderExceptionWrapper(provider -> provider.getFilterPreferences(user), + String.format("Error when trying to get filter preferences for user [%s]", user)); + } + + @Override + public Collection getFilterPreferences(WikiReference wikiReference) + throws NotificationException + { + return this.retrieveWithProviderExceptionWrapper(provider -> provider.getFilterPreferences(wikiReference), + String.format("Error when trying to get filter preferences for wiki [%s]", wikiReference)); } @Override @@ -177,31 +233,18 @@ public void deleteFilterPreference(DocumentReference user, String filterPreferen public void deleteFilterPreferences(DocumentReference user, Set filterPreferenceIds) throws NotificationException { - try { - for (NotificationFilterPreferenceProvider provider - : componentManager.getInstanceList( - NotificationFilterPreferenceProvider.class)) { - provider.deleteFilterPreferences(user, filterPreferenceIds); - } - } catch (ComponentLookupException e) { - logger.info("Failed to remove the user filter preferences {}.", filterPreferenceIds, e); - } + this.providerExceptionWrapper(provider -> provider.deleteFilterPreferences(user, filterPreferenceIds), + String.format("Error when trying to remove filter preferences %s for user [%s]", filterPreferenceIds, + user)); } @Override public void deleteFilterPreference(WikiReference wikiReference, String filterPreferenceId) throws NotificationException { - try { - for (NotificationFilterPreferenceProvider provider - : componentManager.getInstanceList( - NotificationFilterPreferenceProvider.class)) { - provider.deleteFilterPreference(wikiReference, filterPreferenceId); - } - - } catch (ComponentLookupException e) { - logger.info("Failed to remove the wiki filter preference [{}].", filterPreferenceId, e); - } + this.providerExceptionWrapper(provider -> provider.deleteFilterPreference(wikiReference, filterPreferenceId), + String.format("Error when trying to remove filter preference [%s] for wiki [%s]", filterPreferenceId, + wikiReference)); } @@ -209,47 +252,33 @@ public void deleteFilterPreference(WikiReference wikiReference, String filterPre public void setFilterPreferenceEnabled(DocumentReference user, String filterPreferenceId, boolean enabled) throws NotificationException { - try { - for (NotificationFilterPreferenceProvider provider - : componentManager.getInstanceList( - NotificationFilterPreferenceProvider.class)) { - provider.setFilterPreferenceEnabled(user, filterPreferenceId, enabled); - } - - } catch (ComponentLookupException e) { - logger.info(ENABLE_ERROR_MESSAGE, filterPreferenceId, e); - } + this.providerExceptionWrapper(provider -> + provider.setFilterPreferenceEnabled(user, filterPreferenceId, enabled), + String.format("Error when trying to set filter preference [%s] enabled to [%s] for user [%s]", + enabled, + filterPreferenceId, + user)); } @Override public void setFilterPreferenceEnabled(WikiReference wikiReference, String filterPreferenceId, boolean enabled) throws NotificationException { - try { - for (NotificationFilterPreferenceProvider provider - : componentManager.getInstanceList( - NotificationFilterPreferenceProvider.class)) { - provider.setFilterPreferenceEnabled(wikiReference, filterPreferenceId, enabled); - } - - } catch (ComponentLookupException e) { - logger.info(ENABLE_ERROR_MESSAGE, filterPreferenceId, e); - } + this.providerExceptionWrapper(provider -> + provider.setFilterPreferenceEnabled(wikiReference, filterPreferenceId, enabled), + String.format("Error when trying to set filter preference [%s] enabled to [%s] for wiki [%s]", + enabled, + filterPreferenceId, + wikiReference)); } @Override public void setStartDateForUser(DocumentReference user, Date startDate) throws NotificationException { - try { - List providers - = componentManager.getInstanceList(NotificationFilterPreferenceProvider.class); - - for (NotificationFilterPreferenceProvider provider : providers) { - provider.setStartDateForUser(user, startDate); - } - } catch (ComponentLookupException e) { - throw new NotificationException(String.format("Unable to set the starting date for filter preferences" - + " with user [%s].", user)); - } + this.providerExceptionWrapper(provider -> + provider.setStartDateForUser(user, startDate), + String.format("Error when trying to set start date to [%s] for user [%s]", + startDate, + user)); } } diff --git a/xwiki-platform-core/xwiki-platform-notifications/xwiki-platform-notifications-filters/xwiki-platform-notifications-filters-api/src/test/java/org/xwiki/notifications/filters/internal/DefaultNotificationFilterPreferenceManagerTest.java b/xwiki-platform-core/xwiki-platform-notifications/xwiki-platform-notifications-filters/xwiki-platform-notifications-filters-api/src/test/java/org/xwiki/notifications/filters/internal/DefaultNotificationFilterPreferenceManagerTest.java index 522756488981..c35b98b6202a 100644 --- a/xwiki-platform-core/xwiki-platform-notifications/xwiki-platform-notifications-filters/xwiki-platform-notifications-filters-api/src/test/java/org/xwiki/notifications/filters/internal/DefaultNotificationFilterPreferenceManagerTest.java +++ b/xwiki-platform-core/xwiki-platform-notifications/xwiki-platform-notifications-filters/xwiki-platform-notifications-filters-api/src/test/java/org/xwiki/notifications/filters/internal/DefaultNotificationFilterPreferenceManagerTest.java @@ -27,14 +27,13 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.mockito.internal.util.collections.Sets; -import org.xwiki.component.manager.ComponentManager; import org.xwiki.model.reference.DocumentReference; import org.xwiki.model.reference.WikiReference; +import org.xwiki.notifications.NotificationException; import org.xwiki.notifications.filters.NotificationFilter; import org.xwiki.notifications.filters.NotificationFilterPreference; import org.xwiki.notifications.filters.NotificationFilterPreferenceProvider; import org.xwiki.notifications.filters.NotificationFilterType; -import org.xwiki.test.annotation.BeforeComponent; import org.xwiki.test.junit5.mockito.ComponentTest; import org.xwiki.test.junit5.mockito.InjectComponentManager; import org.xwiki.test.junit5.mockito.InjectMockComponents; @@ -42,9 +41,12 @@ import org.xwiki.test.mockito.MockitoComponentManager; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -68,12 +70,6 @@ public class DefaultNotificationFilterPreferenceManagerTest @InjectComponentManager private MockitoComponentManager componentManager; - @BeforeComponent - void beforeComponent() throws Exception - { - this.componentManager.registerComponent(ComponentManager.class, this.componentManager); - } - @BeforeEach public void setUp() throws Exception { @@ -154,6 +150,42 @@ void deleteFilterPreference() throws Exception verify(testProvider).deleteFilterPreferences(testUser, Set.of("myFilter")); } + @Test + void deleteFilterPreferenceProviderException() throws Exception + { + String providerName1 = "providerName1"; + String providerName2 = "providerName2"; + + NotificationFilterPreferenceProvider provider1 = + componentManager.registerMockComponent(NotificationFilterPreferenceProvider.class, providerName1); + NotificationFilterPreferenceProvider provider2 = + componentManager.registerMockComponent(NotificationFilterPreferenceProvider.class, providerName2); + + String filterId = "filterId"; + doThrow(new NotificationException("error provider1")).when(provider1).deleteFilterPreferences(testUser, + Set.of(filterId)); + filterPreferenceManager.deleteFilterPreference(testUser, filterId); + verify(testProvider).deleteFilterPreferences(testUser, Set.of(filterId)); + verify(provider1).deleteFilterPreferences(testUser, Set.of(filterId)); + verify(provider2).deleteFilterPreferences(testUser, Set.of(filterId)); + + doThrow(new NotificationException("error provider2")).when(provider2).deleteFilterPreferences(testUser, + Set.of(filterId)); + filterPreferenceManager.deleteFilterPreference(testUser, filterId); + verify(testProvider, times(2)).deleteFilterPreferences(testUser, Set.of(filterId)); + verify(provider1, times(2)).deleteFilterPreferences(testUser, Set.of(filterId)); + verify(provider2, times(2)).deleteFilterPreferences(testUser, Set.of(filterId)); + + doThrow(new NotificationException("error testprovider")).when(testProvider).deleteFilterPreferences(testUser, + Set.of(filterId)); + NotificationException notificationException = assertThrows(NotificationException.class, + () -> filterPreferenceManager.deleteFilterPreference(testUser, filterId)); + assertEquals("Error when trying to remove filter preferences [filterId] for user [wiki:test.user] - " + + "All providers called failed, see exceptions: [NotificationException: error testprovider," + + "NotificationException: error provider1,NotificationException: error provider2].", + notificationException.getMessage()); + } + @Test void setFilterPreferenceEnabled() throws Exception { @@ -199,9 +231,9 @@ void saveFilterPreferences() throws Exception String providerName2 = "providerName2"; NotificationFilterPreferenceProvider provider1 = - this.componentManager.registerMockComponent(NotificationFilterPreferenceProvider.class, providerName1); + componentManager.registerMockComponent(NotificationFilterPreferenceProvider.class, providerName1); NotificationFilterPreferenceProvider provider2 = - this.componentManager.registerMockComponent(NotificationFilterPreferenceProvider.class, providerName2); + componentManager.registerMockComponent(NotificationFilterPreferenceProvider.class, providerName2); when(pref1.getProviderHint()).thenReturn(providerName1); when(pref2.getProviderHint()).thenReturn(providerName2);