Skip to content

Commit 044acd9

Browse files
enricocolasanteenricocolasante
authored
fix: invalidate template cache and use live template when sending notifications [DHIS2-21836] (#24732)
* fix: invalidate template cache and use live template when sending notifications [DHIS2-21836] * fix: defer notification template cache eviction to after commit [DHIS2-21836]
1 parent 7a39dad commit 044acd9

8 files changed

Lines changed: 362 additions & 19 deletions

File tree

‎dhis-2/dhis-api/src/main/java/org/hisp/dhis/program/notification/ProgramNotificationTemplateService.java‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,19 @@ public interface ProgramNotificationTemplateService {
4848
*/
4949
ProgramNotificationTemplate getByUidCached(String uid);
5050

51+
/**
52+
* Evicts the given template from the {@link #getByUidCached(String)} cache. Must be called
53+
* whenever a template is modified outside of {@link #save}/{@link #update}/{@link #delete} (e.g.
54+
* through the metadata import pipeline), otherwise stale templates keep being used when sending
55+
* notifications.
56+
*
57+
* <p>When called within an active transaction the eviction is deferred until after that
58+
* transaction commits, so it never runs before the write is visible to other connections. This
59+
* avoids re-caching a stale template that a concurrent reader could otherwise fetch during the
60+
* eviction-inside-transaction window. Outside a transaction the eviction happens immediately.
61+
*/
62+
void invalidateCache(String uid);
63+
5164
void save(ProgramNotificationTemplate programNotificationTemplate);
5265

5366
void update(ProgramNotificationTemplate programNotificationTemplate);

‎dhis-2/dhis-api/src/main/java/org/hisp/dhis/program/notification/template/NotificationTemplateMapper.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ public static ProgramNotificationTemplateSnapshot toProgramNotificationTemplateS
112112
template.getRecipientDataElement(),
113113
IdentifiableObjectSnapshot::new,
114114
Collections.emptyList())),
115-
t -> t.setSendRepeatable(t.isSendRepeatable()),
115+
t -> t.setSendRepeatable(template.isSendRepeatable()),
116116
t -> t.setRecipientUserGroup(toUserGroupSnapshot(template.getRecipientUserGroup()))));
117117
}
118118

‎dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/program/DefaultProgramNotificationTemplateService.java‎

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,8 @@
4141
import org.hisp.dhis.program.notification.ProgramNotificationTemplateStore;
4242
import org.springframework.stereotype.Service;
4343
import org.springframework.transaction.annotation.Transactional;
44+
import org.springframework.transaction.support.TransactionSynchronization;
45+
import org.springframework.transaction.support.TransactionSynchronizationManager;
4446

4547
/**
4648
* @author Zubair Asghar
@@ -97,25 +99,45 @@ private ProgramNotificationTemplate loadAndInitialize(String uid) {
9799
return template;
98100
}
99101

102+
@Override
103+
public void invalidateCache(String uid) {
104+
if (TransactionSynchronizationManager.isSynchronizationActive()) {
105+
// Defer eviction until the surrounding transaction commits. Evicting mid-transaction, before
106+
// the write is visible to other connections (PostgreSQL defaults to READ COMMITTED), lets a
107+
// concurrent reader miss, re-read the still-committed pre-edit row, and re-cache the stale
108+
// template until TTL. Evicting only after commit means a reader has to arrive already-missing
109+
// to re-cache a stale value, a much narrower race.
110+
TransactionSynchronizationManager.registerSynchronization(
111+
new TransactionSynchronization() {
112+
@Override
113+
public void afterCommit() {
114+
templateCache.invalidate(uid);
115+
}
116+
});
117+
} else {
118+
templateCache.invalidate(uid);
119+
}
120+
}
121+
100122
@Override
101123
@Transactional
102124
public void save(ProgramNotificationTemplate programNotificationTemplate) {
103125
store.save(programNotificationTemplate);
104-
templateCache.invalidate(programNotificationTemplate.getUid());
126+
invalidateCache(programNotificationTemplate.getUid());
105127
}
106128

107129
@Override
108130
@Transactional
109131
public void update(ProgramNotificationTemplate programNotificationTemplate) {
110132
store.update(programNotificationTemplate);
111-
templateCache.invalidate(programNotificationTemplate.getUid());
133+
invalidateCache(programNotificationTemplate.getUid());
112134
}
113135

114136
@Override
115137
@Transactional
116138
public void delete(ProgramNotificationTemplate programNotificationTemplate) {
117139
store.delete(programNotificationTemplate);
118-
templateCache.invalidate(programNotificationTemplate.getUid());
140+
invalidateCache(programNotificationTemplate.getUid());
119141
}
120142

121143
@Override

‎dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/program/DefaultProgramNotificationTemplateServiceTest.java‎

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
1+
/*
2+
* Copyright (c) 2004-2022, University of Oslo
3+
* All rights reserved.
4+
*
5+
* Redistribution and use in source and binary forms, with or without
6+
* modification, are permitted provided that the following conditions are met:
7+
*
8+
* 1. Redistributions of source code must retain the above copyright notice, this
9+
* list of conditions and the following disclaimer.
10+
*
11+
* 2. Redistributions in binary form must reproduce the above copyright notice,
12+
* this list of conditions and the following disclaimer in the documentation
13+
* and/or other materials provided with the distribution.
14+
*
15+
* 3. Neither the name of the copyright holder nor the names of its contributors
16+
* may be used to endorse or promote products derived from this software without
17+
* specific prior written permission.
18+
*
19+
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
20+
* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
21+
* WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
22+
* DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
23+
* ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
24+
* (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
25+
* LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
26+
* ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
27+
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
28+
* SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
29+
*/
30+
package org.hisp.dhis.program;
31+
32+
import static org.junit.jupiter.api.Assertions.assertEquals;
33+
import static org.mockito.Mockito.doReturn;
34+
import static org.mockito.Mockito.never;
35+
import static org.mockito.Mockito.verify;
36+
37+
import java.util.List;
38+
import org.hisp.dhis.cache.Cache;
39+
import org.hisp.dhis.cache.CacheProvider;
40+
import org.hisp.dhis.program.notification.ProgramNotificationTemplate;
41+
import org.hisp.dhis.program.notification.ProgramNotificationTemplateOperationParamsMapper;
42+
import org.hisp.dhis.program.notification.ProgramNotificationTemplateStore;
43+
import org.junit.jupiter.api.AfterEach;
44+
import org.junit.jupiter.api.BeforeEach;
45+
import org.junit.jupiter.api.Test;
46+
import org.junit.jupiter.api.extension.ExtendWith;
47+
import org.mockito.Mock;
48+
import org.mockito.junit.jupiter.MockitoExtension;
49+
import org.springframework.transaction.support.TransactionSynchronization;
50+
import org.springframework.transaction.support.TransactionSynchronizationManager;
51+
52+
@ExtendWith(MockitoExtension.class)
53+
class DefaultProgramNotificationTemplateServiceTest {
54+
private static final String UID = "PNT_UID_1";
55+
56+
@Mock private ProgramNotificationTemplateStore store;
57+
58+
@Mock private ProgramNotificationTemplateOperationParamsMapper paramsMapper;
59+
60+
@Mock private CacheProvider cacheProvider;
61+
62+
@Mock private Cache<ProgramNotificationTemplate> templateCache;
63+
64+
private DefaultProgramNotificationTemplateService service;
65+
66+
@BeforeEach
67+
void setUp() {
68+
doReturn(templateCache).when(cacheProvider).createNotificationTemplateCache();
69+
service = new DefaultProgramNotificationTemplateService(store, paramsMapper, cacheProvider);
70+
}
71+
72+
@AfterEach
73+
void tearDown() {
74+
if (TransactionSynchronizationManager.isSynchronizationActive()) {
75+
TransactionSynchronizationManager.clearSynchronization();
76+
}
77+
}
78+
79+
@Test
80+
void shouldEvictImmediatelyWhenNoTransactionIsActive() {
81+
service.invalidateCache(UID);
82+
83+
verify(templateCache).invalidate(UID);
84+
}
85+
86+
@Test
87+
void shouldDeferEvictionUntilAfterCommitWhenTransactionIsActive() {
88+
TransactionSynchronizationManager.initSynchronization();
89+
90+
service.invalidateCache(UID);
91+
92+
// Eviction must not happen while the transaction is still open: the write is not yet visible to
93+
// other connections, so evicting now would let a concurrent reader re-cache the stale template.
94+
verify(templateCache, never()).invalidate(UID);
95+
96+
List<TransactionSynchronization> synchronizations =
97+
TransactionSynchronizationManager.getSynchronizations();
98+
assertEquals(1, synchronizations.size());
99+
100+
synchronizations.get(0).afterCommit();
101+
102+
verify(templateCache).invalidate(UID);
103+
}
104+
105+
@Test
106+
void shouldNotEvictWhenTransactionRollsBack() {
107+
TransactionSynchronizationManager.initSynchronization();
108+
109+
service.invalidateCache(UID);
110+
111+
List<TransactionSynchronization> synchronizations =
112+
TransactionSynchronizationManager.getSynchronizations();
113+
synchronizations.get(0).afterCompletion(TransactionSynchronization.STATUS_ROLLED_BACK);
114+
115+
// The write was rolled back, so the cached template is still valid and must not be evicted.
116+
verify(templateCache, never()).invalidate(UID);
117+
}
118+
}

‎dhis-2/dhis-services/dhis-service-dxf2/src/main/java/org/hisp/dhis/dxf2/metadata/objectbundle/hooks/ProgramNotificationTemplateObjectBundleHook.java‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,19 +33,24 @@
3333
import java.util.Map;
3434
import java.util.Set;
3535
import java.util.function.Function;
36+
import lombok.AllArgsConstructor;
3637
import org.hisp.dhis.common.DeliveryChannel;
3738
import org.hisp.dhis.common.ValueType;
3839
import org.hisp.dhis.dxf2.metadata.objectbundle.ObjectBundle;
3940
import org.hisp.dhis.program.notification.ProgramNotificationRecipient;
4041
import org.hisp.dhis.program.notification.ProgramNotificationTemplate;
42+
import org.hisp.dhis.program.notification.ProgramNotificationTemplateService;
4143
import org.springframework.stereotype.Component;
4244

4345
/**
4446
* @author Halvdan Hoem Grelland
4547
*/
4648
@Component
49+
@AllArgsConstructor
4750
public class ProgramNotificationTemplateObjectBundleHook
4851
extends AbstractObjectBundleHook<ProgramNotificationTemplate> {
52+
private final ProgramNotificationTemplateService programNotificationTemplateService;
53+
4954
private static final Map<
5055
ProgramNotificationRecipient, Function<ProgramNotificationTemplate, ValueType>>
5156
RECIPIENT_TO_VALUETYPE_RESOLVER =
@@ -79,11 +84,23 @@ public void preUpdate(
7984
@Override
8085
public void postCreate(ProgramNotificationTemplate template, ObjectBundle bundle) {
8186
postProcess(template);
87+
// Evict any stale entry (e.g. a template deleted then re-created with the same uid) so the
88+
// cached-template send path picks up the newly persisted delivery channels.
89+
programNotificationTemplateService.invalidateCache(template.getUid());
8290
}
8391

8492
@Override
8593
public void postUpdate(ProgramNotificationTemplate template, ObjectBundle bundle) {
8694
postProcess(template);
95+
// The metadata import pipeline persists directly through the store and bypasses
96+
// ProgramNotificationTemplateService, so the getByUidCached cache would otherwise keep serving
97+
// the pre-edit template (e.g. still delivering email after the channel was deselected).
98+
programNotificationTemplateService.invalidateCache(template.getUid());
99+
}
100+
101+
@Override
102+
public void preDelete(ProgramNotificationTemplate template, ObjectBundle bundle) {
103+
programNotificationTemplateService.invalidateCache(template.getUid());
87104
}
88105

89106
/** Removes any non-valid combinations of properties on the template object. */

‎dhis-2/dhis-services/dhis-service-dxf2/src/test/java/org/hisp/dhis/dxf2/metadata/objectbundle/hooks/ProgramNotificationTemplateObjectBundleHookTest.java‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
/*
2+
* Copyright (c) 2004-2022, University of Oslo
3+
* All rights reserved.
4+
*
5+
* Redistribution and use in source and binary forms, with or without
6+
* modification, are permitted provided that the following conditions are met:
7+
*
8+
* 1. Redistributions of source code must retain the above copyright notice, this
9+
* list of conditions and the following disclaimer.
10+
*
11+
* 2. Redistributions in binary form must reproduce the above copyright notice,
12+
* this list of conditions and the following disclaimer in the documentation
13+
* and/or other materials provided with the distribution.
14+
*
15+
* 3. Neither the name of the copyright holder nor the names of its contributors
16+
* may be used to endorse or promote products derived from this software without
17+
* specific prior written permission.
18+
*
19+
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
20+
* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
21+
* WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
22+
* DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
23+
* ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
24+
* (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
25+
* LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
26+
* ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
27+
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
28+
* SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
29+
*/
30+
package org.hisp.dhis.dxf2.metadata.objectbundle.hooks;
31+
32+
import static org.mockito.Mockito.verify;
33+
34+
import org.hisp.dhis.program.notification.ProgramNotificationRecipient;
35+
import org.hisp.dhis.program.notification.ProgramNotificationTemplate;
36+
import org.hisp.dhis.program.notification.ProgramNotificationTemplateService;
37+
import org.junit.jupiter.api.BeforeEach;
38+
import org.junit.jupiter.api.Test;
39+
import org.junit.jupiter.api.extension.ExtendWith;
40+
import org.mockito.Mock;
41+
import org.mockito.junit.jupiter.MockitoExtension;
42+
43+
@ExtendWith(MockitoExtension.class)
44+
class ProgramNotificationTemplateObjectBundleHookTest {
45+
private static final String TEMPLATE_UID = "abcdefghij0";
46+
47+
@Mock private ProgramNotificationTemplateService programNotificationTemplateService;
48+
49+
private ProgramNotificationTemplateObjectBundleHook hook;
50+
51+
@BeforeEach
52+
void setUp() {
53+
hook = new ProgramNotificationTemplateObjectBundleHook(programNotificationTemplateService);
54+
}
55+
56+
@Test
57+
void shouldInvalidateCacheOnPostUpdate() {
58+
hook.postUpdate(template(), null);
59+
60+
verify(programNotificationTemplateService).invalidateCache(TEMPLATE_UID);
61+
}
62+
63+
@Test
64+
void shouldInvalidateCacheOnPostCreate() {
65+
hook.postCreate(template(), null);
66+
67+
verify(programNotificationTemplateService).invalidateCache(TEMPLATE_UID);
68+
}
69+
70+
@Test
71+
void shouldInvalidateCacheOnPreDelete() {
72+
hook.preDelete(template(), null);
73+
74+
verify(programNotificationTemplateService).invalidateCache(TEMPLATE_UID);
75+
}
76+
77+
private ProgramNotificationTemplate template() {
78+
ProgramNotificationTemplate template = new ProgramNotificationTemplate();
79+
template.setUid(TEMPLATE_UID);
80+
template.setNotificationRecipient(ProgramNotificationRecipient.TRACKED_ENTITY_INSTANCE);
81+
return template;
82+
}
83+
}

‎dhis-2/dhis-tracker/src/main/java/org/hisp/dhis/tracker/program/notification/DefaultProgramNotificationService.java‎

Lines changed: 28 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -294,26 +294,39 @@ private NotificationInstanceWithTemplate withTemplate(
294294

295295
private ProgramNotificationTemplate getApplicableTemplate(
296296
ProgramNotificationInstance programNotificationInstance) {
297-
return Optional.of(programNotificationInstance)
298-
.map(ProgramNotificationInstance::getProgramNotificationTemplateSnapshot)
299-
.map(NotificationTemplateMapper::toProgramNotificationTemplate)
300-
.orElseGet(() -> this.getDatabaseTemplate(programNotificationInstance));
297+
// Prefer the live template so edits made after the notification was scheduled (e.g. removing a
298+
// delivery channel) take effect. The frozen jsonb snapshot is only a fallback for when the
299+
// template has since been deleted, so the notification can still be sent.
300+
ProgramNotificationTemplate databaseTemplate = getDatabaseTemplate(programNotificationInstance);
301+
if (databaseTemplate != null) {
302+
return databaseTemplate;
303+
}
304+
return getSnapshotTemplate(programNotificationInstance);
301305
}
302306

303307
private ProgramNotificationTemplate getDatabaseTemplate(
304308
ProgramNotificationInstance programNotificationInstance) {
305-
log.warn("Couldn't use template from jsonb column, using the one from database if possible");
306-
if (Objects.nonNull(programNotificationInstance.getProgramNotificationTemplateId())) {
307-
ProgramNotificationTemplate programNotificationTemplate =
308-
notificationTemplateService.get(
309-
programNotificationInstance.getProgramNotificationTemplateId());
310-
if (Objects.isNull(programNotificationTemplate)) {
311-
log.warn(
312-
"Unable to load program notification template from database, because it might have been deleted.");
313-
}
314-
return programNotificationTemplate;
309+
if (Objects.isNull(programNotificationInstance.getProgramNotificationTemplateId())) {
310+
return null;
315311
}
316-
return null;
312+
return notificationTemplateService.get(
313+
programNotificationInstance.getProgramNotificationTemplateId());
314+
}
315+
316+
private ProgramNotificationTemplate getSnapshotTemplate(
317+
ProgramNotificationInstance programNotificationInstance) {
318+
ProgramNotificationTemplate snapshotTemplate =
319+
Optional.of(programNotificationInstance)
320+
.map(ProgramNotificationInstance::getProgramNotificationTemplateSnapshot)
321+
.map(NotificationTemplateMapper::toProgramNotificationTemplate)
322+
.orElse(null);
323+
if (snapshotTemplate == null) {
324+
log.warn(
325+
"Unable to resolve a program notification template for instance with id: {}. The template "
326+
+ "may have been deleted and no snapshot is available.",
327+
programNotificationInstance.getId());
328+
}
329+
return snapshotTemplate;
317330
}
318331

319332
@Override

0 commit comments

Comments
 (0)