Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize template addition & update by cross checking the content with system default templates #257

Merged
merged 5 commits into from
Sep 4, 2024
Merged

Optimize template addition & update by cross checking the content with system default templates #257

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9676c85
Optimize template addition & update by cross checking the content wit…
darshanasbg Sep 2, 2024
c0884ff
Add unit tests for DefaultTemplateManager
darshanasbg Sep 3, 2024
3be11c7
Rename DefaultTemplateManager to UnifiedTemplateManager & improve tes…
darshanasbg Sep 4, 2024
d1dcf5b
Improve tests in UnifiedTemplateManagerTest
darshanasbg Sep 4, 2024
8cf3bf4
Rename InMemoryBasedTemplateManager to SystemDefaultTemplateManager
darshanasbg Sep 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions components/email-mgt/org.wso2.carbon.email.mgt/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,11 @@
<artifactId>jacoco-maven-plugin</artifactId>
<version>${jacoco.version}</version>
</dependency>
<dependency>
<groupId>org.wso2.carbon.identity.framework</groupId>
<artifactId>org.wso2.carbon.identity.testutil</artifactId>
<scope>test</scope>
</dependency>
</dependencies>

<build>
Expand Down
6 changes: 4 additions & 2 deletions ...o2.carbon.email.mgt/src/main/java/org/wso2/carbon/email/mgt/EmailTemplateManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.wso2.carbon.email.mgt.constants.I18nMgtConstants;
import org.wso2.carbon.email.mgt.store.DefaultTemplateManager;
import org.wso2.carbon.email.mgt.store.TemplatePersistenceManagerFactory;
import org.wso2.carbon.email.mgt.store.UnifiedTemplateManager;
import org.wso2.carbon.email.mgt.store.TemplatePersistenceManager;
import org.wso2.carbon.email.mgt.exceptions.I18nEmailMgtClientException;
import org.wso2.carbon.email.mgt.exceptions.I18nEmailMgtException;
Expand Down Expand Up @@ -79,7 +80,8 @@ public class EmailTemplateManagerImpl implements EmailTemplateManager, Notificat

public EmailTemplateManagerImpl() {

this.templatePersistenceManager = new DefaultTemplateManager();
TemplatePersistenceManagerFactory templatePersistenceManagerFactory = new TemplatePersistenceManagerFactory();
this.templatePersistenceManager = templatePersistenceManagerFactory.getTemplatePersistenceManager();
}

@Override
Expand Down
20 changes: 19 additions & 1 deletion ...email.mgt/src/main/java/org/wso2/carbon/email/mgt/store/InMemoryBasedTemplateManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
* This class have support for get, list, exists operations and any invocations to modification operations
* throws {@link UnsupportedOperationException}.
* This class expected to be use only with conjunction with another {@link TemplatePersistenceManager} implementation
* which supports full CRUD operations, hence {@link DefaultTemplateManager} provides that aggregation using this as a
* which supports full CRUD operations, hence {@link UnifiedTemplateManager} provides that aggregation using this as a
* fallback provider.
*/
public class InMemoryBasedTemplateManager implements TemplatePersistenceManager {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renaming this to SystemDefaultTemplateManager would make much sense IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.. I also thought InMemoryBasedTemplateManager name only gives the unnecessary implementation detail yet not describe the duty of this class.. Will fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed with 8cf3bf4

Expand Down Expand Up @@ -260,4 +260,22 @@ private Map<String, Map<String, NotificationTemplate>> getTemplateMap(String not
}
return defaultEmailTemplates;
}

/**
* Checks if there is a template available as a system default template with the exact same details.
* This method is used to avoid managing duplicate templates.
*
* @param template Notification template to check.
* @return True if a template with the same details is available, false otherwise.
*/
boolean hasSameTemplate(NotificationTemplate template) {

if (template == null) {
return false;
}

Map<String, NotificationTemplate> defaultTemplatesForScenario =
getTemplateMap(template.getNotificationChannel()).get(template.getDisplayName().toLowerCase());
return defaultTemplatesForScenario == null ? false : defaultTemplatesForScenario.containsValue(template);
}
}
2 changes: 1 addition & 1 deletion ....mgt/src/main/java/org/wso2/carbon/email/mgt/store/TemplatePersistenceManagerFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,6 @@ public TemplatePersistenceManager getTemplatePersistenceManager() {
if (log.isDebugEnabled()) {
log.debug("Template persistent manager initialized with the type: " + persistenceManager.getClass());
}
return persistenceManager;
return new UnifiedTemplateManager(persistenceManager);
}
}
31 changes: 25 additions & 6 deletions ...ail/mgt/store/DefaultTemplateManager.java → ...ail/mgt/store/UnifiedTemplateManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,14 @@
* to both template persistent manger crafted from the factory and an in-memory manager.
* This class will function as a wrapper class for the template manager produced from the factory.
*/
public class DefaultTemplateManager implements TemplatePersistenceManager {
public class UnifiedTemplateManager implements TemplatePersistenceManager {

private final TemplatePersistenceManager templatePersistenceManager;
private final TemplatePersistenceManager inMemoryTemplateManager = new InMemoryBasedTemplateManager();
private final InMemoryBasedTemplateManager inMemoryTemplateManager = new InMemoryBasedTemplateManager();

public DefaultTemplateManager() {
public UnifiedTemplateManager(TemplatePersistenceManager persistenceManager) {

TemplatePersistenceManagerFactory templatePersistenceManagerFactory = new TemplatePersistenceManagerFactory();
this.templatePersistenceManager = templatePersistenceManagerFactory.getTemplatePersistenceManager();
this.templatePersistenceManager = persistenceManager;
}

@Override
Expand Down Expand Up @@ -87,7 +86,27 @@ public void deleteNotificationTemplateType(String displayName, String notificati
public void addOrUpdateNotificationTemplate(NotificationTemplate notificationTemplate, String applicationUuid,
String tenantDomain) throws NotificationTemplateManagerServerException {

templatePersistenceManager.addOrUpdateNotificationTemplate(notificationTemplate, applicationUuid, tenantDomain);
if (!inMemoryTemplateManager.hasSameTemplate(notificationTemplate)) {
templatePersistenceManager.addOrUpdateNotificationTemplate(notificationTemplate, applicationUuid,
tenantDomain);
} else {
// Template is already managed as a system default template. Handle add or update.
String displayName = notificationTemplate.getDisplayName();
String locale = notificationTemplate.getLocale();
String notificationChannel = notificationTemplate.getNotificationChannel();
boolean isExistsInStorage =
templatePersistenceManager.isNotificationTemplateExists(displayName, locale, notificationChannel,
applicationUuid, tenantDomain);
if (isExistsInStorage) {
// This request is to reset existing template to default content. Hence, delete the existing template.
templatePersistenceManager.deleteNotificationTemplate(displayName, locale, notificationChannel,
applicationUuid, tenantDomain);
} else {
// This request is to add a new template with a same content that is already managed as a system default
// template. Storing such templates is redundant. Hence, avoid storing those templates as duplicate
// contents to optimize the storage.
}
}
}

@Override
Expand Down
51 changes: 31 additions & 20 deletions .../src/test/java/org/wso2/carbon/email/mgt/store/TemplatePersistenceManagerFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,20 +26,24 @@
import org.testng.annotations.Test;
import org.wso2.carbon.base.CarbonBaseConstants;
import org.wso2.carbon.email.mgt.internal.I18nMgtDataHolder;
import org.wso2.carbon.identity.common.testng.WithCarbonHome;
import org.wso2.carbon.identity.core.persistence.registry.RegistryResourceMgtService;
import org.wso2.carbon.identity.core.util.IdentityUtil;

import java.lang.reflect.Field;
import java.nio.file.Paths;

import static org.mockito.MockitoAnnotations.initMocks;
import static org.powermock.api.mockito.PowerMockito.mockStatic;
import static org.powermock.api.mockito.PowerMockito.when;
import static org.testng.Assert.assertTrue;
import static org.testng.Assert.fail;
import static org.wso2.carbon.email.mgt.constants.I18nMgtConstants.NOTIFICATION_TEMPLATES_STORAGE_CONFIG;

/**
* Class that contains the test cases for {@link TemplatePersistenceManagerFactory}.
*/
@WithCarbonHome
@PrepareForTest({I18nMgtDataHolder.class, IdentityUtil.class})
public class TemplatePersistenceManagerFactoryTest extends PowerMockTestCase {

Expand All @@ -52,8 +56,6 @@ public class TemplatePersistenceManagerFactoryTest extends PowerMockTestCase {
@BeforeMethod
public void setUp() {

setUpCarbonHome();

initMocks(this);
mockStatic(I18nMgtDataHolder.class);
i18nMgtDataHolder = PowerMockito.mock(I18nMgtDataHolder.class);
Expand All @@ -65,64 +67,73 @@ public void setUp() {
}

@Test
public void shouldReturnDBBasedTemplateManagerWhenConfigIsDatabase() {
public void shouldUseDBBasedTemplateManagerWhenConfigIsDatabase() {

when(IdentityUtil.getProperty(NOTIFICATION_TEMPLATES_STORAGE_CONFIG)).thenReturn("database");
TemplatePersistenceManager templatePersistenceManager =
templatePersistenceManagerFactory.getTemplatePersistenceManager();
assertTrue(templatePersistenceManager instanceof DBBasedTemplateManager);
assertTrue(templatePersistenceManager instanceof UnifiedTemplateManager);
assertUnderlyingManagerType(templatePersistenceManager, DBBasedTemplateManager.class);
}

@Test
public void shouldReturnHybridTemplateManagerWhenConfigIsOnMigration() {
public void shouldUseHybridTemplateManagerWhenConfigIsOnMigration() {

when(IdentityUtil.getProperty(NOTIFICATION_TEMPLATES_STORAGE_CONFIG)).thenReturn("hybrid");
TemplatePersistenceManager templatePersistenceManager =
templatePersistenceManagerFactory.getTemplatePersistenceManager();
assertTrue(templatePersistenceManager instanceof HybridTemplateManager);
assertTrue(templatePersistenceManager instanceof UnifiedTemplateManager);
assertUnderlyingManagerType(templatePersistenceManager, HybridTemplateManager.class);
}

@Test
public void shouldReturnRegistryBasedTemplateManagerWhenConfigIsRegistry() {
public void shouldUseRegistryBasedTemplateManagerWhenConfigIsRegistry() {

when(IdentityUtil.getProperty(NOTIFICATION_TEMPLATES_STORAGE_CONFIG)).thenReturn("registry");
TemplatePersistenceManager templatePersistenceManager =
templatePersistenceManagerFactory.getTemplatePersistenceManager();
assertTrue(templatePersistenceManager instanceof RegistryBasedTemplateManager);
assertTrue(templatePersistenceManager instanceof UnifiedTemplateManager);
assertUnderlyingManagerType(templatePersistenceManager, RegistryBasedTemplateManager.class);
}

@Test
public void shouldReturnDBBasedTemplateManagerWhenConfigIsInvalid() {
public void shouldUseDBBasedTemplateManagerWhenConfigIsInvalid() {

when(IdentityUtil.getProperty(NOTIFICATION_TEMPLATES_STORAGE_CONFIG)).thenReturn("invalid");
TemplatePersistenceManager templatePersistenceManager =
templatePersistenceManagerFactory.getTemplatePersistenceManager();
assertTrue(templatePersistenceManager instanceof DBBasedTemplateManager);
assertTrue(templatePersistenceManager instanceof UnifiedTemplateManager);
assertUnderlyingManagerType(templatePersistenceManager, DBBasedTemplateManager.class);
}

@Test
public void shouldReturnDBBasedTemplateManagerWhenConfigIsBlank() {
public void shouldUseDBBasedTemplateManagerWhenConfigIsBlank() {

when(IdentityUtil.getProperty(NOTIFICATION_TEMPLATES_STORAGE_CONFIG)).thenReturn("");
TemplatePersistenceManager templatePersistenceManager =
templatePersistenceManagerFactory.getTemplatePersistenceManager();
assertTrue(templatePersistenceManager instanceof DBBasedTemplateManager);
assertTrue(templatePersistenceManager instanceof UnifiedTemplateManager);
assertUnderlyingManagerType(templatePersistenceManager, DBBasedTemplateManager.class);
}

@Test
public void shouldReturnDBBasedTemplateManagerWhenConfigIsNull() {
public void shouldUseDBBasedTemplateManagerWhenConfigIsNull() {

when(IdentityUtil.getProperty(NOTIFICATION_TEMPLATES_STORAGE_CONFIG)).thenReturn(null);
TemplatePersistenceManager templatePersistenceManager =
templatePersistenceManagerFactory.getTemplatePersistenceManager();
assertTrue(templatePersistenceManager instanceof DBBasedTemplateManager);
assertTrue(templatePersistenceManager instanceof UnifiedTemplateManager);
assertUnderlyingManagerType(templatePersistenceManager, DBBasedTemplateManager.class);
}

private static void setUpCarbonHome() {

String carbonHome = Paths.get(System.getProperty("user.dir"), "target", "test-classes").toString();
System.setProperty(CarbonBaseConstants.CARBON_HOME, carbonHome);
System.setProperty(CarbonBaseConstants.CARBON_CONFIG_DIR_PATH, Paths.get(carbonHome,
"repository/conf").toString());
private void assertUnderlyingManagerType(TemplatePersistenceManager templatePersistenceManager, Class<?> expectedClass) {
try {
Field field = UnifiedTemplateManager.class.getDeclaredField("templatePersistenceManager");
field.setAccessible(true);
Object underlyingManager = field.get(templatePersistenceManager);
assertTrue(expectedClass.isInstance(underlyingManager));
} catch (NoSuchFieldException | IllegalAccessException e) {
fail("Failed to access the underlying TemplatePersistenceManager", e);
}
}
}
Loading
Loading