Skip to content

Commit 0028aa3

Browse files
fdauganfdaugan
committed
imp: cache optimization on deletion
1 parent 0792f36 commit 0028aa3

File tree

6 files changed

+158
-73
lines changed

6 files changed

+158
-73
lines changed

‎src/main/java/org/ligoj/app/plugin/id/dao/AbstractMemCacheRepository.java

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -263,10 +263,12 @@ protected IUserRepository getUser() {
263263
*/
264264
public void removeGroupFromGroup(final GroupOrg subGroup, final GroupOrg group) {
265265
// Remove from JPA cache
266-
cache.removeGroupFromGroup(subGroup, group);
266+
if (group != null) {
267+
cache.removeGroupFromGroup(subGroup, group);
267268

268-
// Also update the membership cache
269-
group.getSubGroups().remove(subGroup.getId());
269+
// Also update the membership cache
270+
group.getSubGroups().remove(subGroup.getId());
271+
}
270272
subGroup.setParent(null);
271273
}
272274

‎src/main/java/org/ligoj/app/plugin/id/dao/IdCacheDaoImpl.java

Lines changed: 76 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
import jakarta.persistence.EntityManager;
77
import jakarta.persistence.PersistenceContext;
88
import jakarta.persistence.PersistenceContextType;
9+
import jakarta.persistence.Query;
910
import jakarta.transaction.Transactional;
1011
import lombok.Getter;
1112
import lombok.extern.slf4j.Slf4j;
@@ -31,6 +32,7 @@
3132
import java.util.function.Consumer;
3233
import java.util.function.Function;
3334
import java.util.stream.Collectors;
35+
import java.util.stream.Stream;
3436

3537
/**
3638
* Cache synchronization from SQL cache to database.
@@ -85,9 +87,9 @@ private void updateUserToGroupInternal(final CacheUser entity, final CacheGroup
8587
/**
8688
* Associate a subgroup to a group using the cache groups to prevent duplicate entries.
8789
*/
88-
private void updateGroupToGroupInternal(final CacheGroup subGroup, final CacheGroup group, final Set<String> cacheGroups) {
89-
if (!cacheGroups.contains(group.getId())) {
90-
// New membership
90+
private void updateGroupToGroupInternal(final CacheGroup subGroup, final CacheGroup group, final Set<String> cacheSubGroups) {
91+
if (!cacheSubGroups.contains(subGroup.getId())) {
92+
// New membership to put in cache
9193
final var membership = new CacheMembership();
9294
membership.setSubGroup(subGroup);
9395
membership.setGroup(group);
@@ -117,7 +119,7 @@ public void create(final UserOrg user) {
117119
// Set the company if defined
118120
entity.setCompany(Optional.ofNullable(user.getCompany()).map(c -> {
119121
final var company = new CacheCompany();
120-
company.setId(user.getCompany());
122+
company.setId(c);
121123
return company;
122124
}).orElse(null));
123125
em.persist(entity);
@@ -176,37 +178,34 @@ private CacheUser createInternal(final UserOrg user, final Map<String, CacheUser
176178

177179
@Override
178180
public void delete(final CompanyOrg company) {
179-
em.createQuery("DELETE FROM CacheCompany WHERE id=:id").setParameter("id", company.getId()).executeUpdate();
180-
em.flush();
181-
em.clear();
181+
removeAll(em.createQuery("FROM CacheCompany WHERE id=:id").setParameter("id", company.getId()));
182182
}
183183

184184
@Override
185185
public void delete(final GroupOrg group) {
186-
em.createQuery("DELETE FROM CacheProjectGroup WHERE group.id=:id").setParameter("id", group.getId())
187-
.executeUpdate();
188-
em.createQuery("DELETE FROM CacheMembership WHERE group.id=:id OR subGroup.id=:id")
189-
.setParameter("id", group.getId()).executeUpdate();
190-
em.createQuery("DELETE FROM CacheGroup WHERE id=:id").setParameter("id", group.getId()).executeUpdate();
186+
removeAll(
187+
em.createQuery("FROM CacheMembership WHERE group.id=:id OR subGroup.id=:id").setParameter("id", group.getId()),
188+
em.createQuery("FROM CacheProjectGroup WHERE group.id=:id").setParameter("id", group.getId()),
189+
em.createQuery("FROM CacheGroup WHERE id=:id").setParameter("id", group.getId())
190+
);
191+
}
192+
193+
private void removeAll(Query... queries) {
194+
Stream.of(queries).map(Query::getResultList).forEach(l -> l.forEach(em::remove));
191195
em.flush();
192-
em.clear();
193196
}
194197

195198
@Override
196199
public void delete(final UserOrg user) {
197-
em.createQuery("DELETE FROM CacheMembership WHERE user.id=:id").setParameter("id", user.getId())
198-
.executeUpdate();
199-
em.createQuery("DELETE FROM CacheUser WHERE id=:id").setParameter("id", user.getId()).executeUpdate();
200-
em.flush();
201-
em.clear();
200+
removeAll(
201+
em.createQuery("FROM CacheMembership WHERE user.id=:id").setParameter("id", user.getId()),
202+
em.createQuery("FROM CacheUser WHERE id=:id").setParameter("id", user.getId())
203+
);
202204
}
203205

204206
@Override
205207
public void empty(final GroupOrg group) {
206-
em.createQuery("DELETE FROM CacheMembership WHERE group.id=:id").setParameter("id", group.getId())
207-
.executeUpdate();
208-
em.flush();
209-
em.clear();
208+
removeAll(em.createQuery("FROM CacheMembership WHERE group.id=:id").setParameter("id", group.getId()));
210209
}
211210

212211
/**
@@ -229,16 +228,22 @@ private int persistUsersAndMemberships(final Map<String, UserOrg> users, final M
229228
final Map<String, CacheCompany> cacheCompanies) {
230229
final var cacheUsers = em.createQuery("FROM CacheUser", CacheUser.class)
231230
.getResultList().stream().collect(Collectors.toMap(CacheUser::getId, Function.identity()));
232-
final var userMemberships = em.createQuery("FROM CacheMembership WHERE user is not null", CacheMembership.class)
233-
.getResultList().stream().collect(
234-
Collectors.groupingBy(c -> c.getUser().getId(),
235-
Collectors.mapping(c -> c.getGroup().getId(),
236-
Collectors.toSet())));
237-
final var groupMemberships = em.createQuery("FROM CacheMembership WHERE subGroup is not null", CacheMembership.class)
238-
.getResultList().stream().collect(
239-
Collectors.groupingBy(c -> c.getGroup().getId(),
240-
Collectors.mapping(c -> c.getSubGroup().getId(),
241-
Collectors.toSet())));
231+
232+
final var userMemberships = em.createQuery("FROM CacheMembership WHERE user is not null", CacheMembership.class).getResultList();
233+
final var groupMemberships = em.createQuery("FROM CacheMembership WHERE subGroup is not null", CacheMembership.class).getResultList();
234+
235+
// Remove duplicates
236+
groupMemberships.stream().collect(Collectors.groupingBy(c -> c.getSubGroup().getId() + "-" + c.getGroup()))
237+
.values().stream().filter(l -> l.size() > 1).forEach(l -> l.stream().skip(1).forEach(em::remove));
238+
userMemberships.stream().collect(Collectors.groupingBy(c -> c.getUser().getId() + "-" + c.getGroup()))
239+
.values().stream().filter(l -> l.size() > 1).forEach(l -> l.stream().skip(1).forEach(em::remove));
240+
241+
242+
final var membershipsByUser = userMemberships.stream().collect(
243+
Collectors.groupingBy(c -> c.getUser().getId(), Collectors.mapping(c -> c.getGroup().getId(), Collectors.toSet())));
244+
final var membershipsByGroup = groupMemberships.stream().collect(
245+
Collectors.groupingBy(c -> c.getGroup().getId(), Collectors.mapping(c -> c.getSubGroup().getId(), Collectors.toSet())));
246+
242247
var memberships = 0;
243248

244249
// Persist users and memberships
@@ -247,7 +252,7 @@ private int persistUsersAndMemberships(final Map<String, UserOrg> users, final M
247252
final var entity = createInternal(user, cacheUsers, cacheCompanies);
248253

249254
// Create/update membership
250-
final var cacheUserGroups = userMemberships.getOrDefault(user.getId(), Collections.emptySet());
255+
final var cacheUserGroups = membershipsByUser.getOrDefault(user.getId(), Collections.emptySet());
251256
for (final var group : user.getGroups()) {
252257
updateUserToGroupInternal(entity, cacheGroups.get(group), cacheUserGroups);
253258
}
@@ -267,7 +272,7 @@ private int persistUsersAndMemberships(final Map<String, UserOrg> users, final M
267272
// Persist subgroups and memberships
268273
for (final var group : groups.values()) {
269274
final var cachedGroup = cacheGroups.get(group.getId());
270-
final var cacheSubGroups = groupMemberships.getOrDefault(group.getId(), Collections.emptySet());
275+
final var cacheSubGroups = membershipsByGroup.getOrDefault(group.getId(), Collections.emptySet());
271276
for (final var subGroup : group.getSubGroups()) {
272277
updateGroupToGroupInternal(cacheGroups.get(subGroup), cachedGroup, cacheSubGroups);
273278
}
@@ -298,36 +303,42 @@ private int persistUsersAndMemberships(final Map<String, UserOrg> users, final M
298303
* @return the amount of persisted relations.
299304
*/
300305
private int persistProjectGroups(final Map<String, CacheGroup> groups) {
301-
final var entities = em.createQuery("FROM CacheProjectGroup WHERE group is not null", CacheProjectGroup.class)
302-
.getResultList().stream().collect(
303-
Collectors.groupingBy(c -> c.getProject().getId(),
304-
Collectors.mapping(c -> c.getGroup().getId(),
305-
Collectors.toSet())));
306-
307-
final var allProjectGroup = cacheProjectGroupRepository.findAllProjectGroup();
308-
for (final var projectGroup : allProjectGroup) {
309-
final var projectId = (int) projectGroup[0];
310-
final var groupId = (String) projectGroup[1];
311-
final var projectGroupIds = entities.get(projectId);
312-
if ((projectGroupIds == null || !projectGroupIds.contains(groupId)) && groups.containsKey(groupId)) {
313-
// New association
314-
final var project = new Project();
315-
project.setId(projectId);
316-
final var entity = new CacheProjectGroup();
317-
entity.setProject(project);
318-
entity.setGroup(groups.get(groupId));
319-
em.persist(entity);
320-
}
321-
322-
// Purge the old entries
323-
if (projectGroupIds != null) {
324-
projectGroupIds.remove(groupId);
306+
final var cachedProjectGroups = em.createQuery("FROM CacheProjectGroup WHERE group is not null", CacheProjectGroup.class)
307+
.getResultList();
308+
309+
// Remove duplicates
310+
cachedProjectGroups.stream().collect(Collectors.groupingBy(c -> c.getProject().getId() + "-" + c.getGroup()))
311+
.values().stream().filter(l -> l.size() > 1).forEach(l -> l.stream().skip(1).forEach(em::remove));
312+
313+
// Create missing cached project groups as needed
314+
final var cachedProjectGroupsByProject = cachedProjectGroups.stream().collect(
315+
Collectors.groupingBy(c -> c.getProject().getId(),
316+
Collectors.mapping(c -> c.getGroup().getId(), Collectors.toSet())));
317+
final var allProjectGroups = cacheProjectGroupRepository.findAllProjectGroup().stream().collect(
318+
Collectors.groupingBy(pg -> (Integer) pg[0],
319+
Collectors.mapping(pg -> (String) pg[1], Collectors.toSet())));
320+
for (final var projectGroups : allProjectGroups.entrySet()) {
321+
final var projectId = projectGroups.getKey();
322+
final var groupIds = projectGroups.getValue();
323+
final var cachedProjectGroupIds = cachedProjectGroupsByProject.get(projectId);
324+
for (final var groupId : groupIds) {
325+
if ((cachedProjectGroupIds == null || !cachedProjectGroupIds.contains(groupId)) && groups.containsKey(groupId)) {
326+
// New association
327+
final var project = new Project();
328+
project.setId(projectId);
329+
final var entity = new CacheProjectGroup();
330+
entity.setProject(project);
331+
entity.setGroup(groups.get(groupId));
332+
em.persist(entity);
333+
} else if (cachedProjectGroupIds != null) {
334+
cachedProjectGroupIds.remove(groupId);
335+
}
325336
}
326337
}
327338

328339
// Remove old memberships
329-
entities.keySet().forEach(project ->
330-
entities.get(project).forEach(group -> {
340+
cachedProjectGroupsByProject.keySet().forEach(project ->
341+
cachedProjectGroupsByProject.get(project).forEach(group -> {
331342
log.info("Deleting removed cache entry {}#{}-{}", CacheProjectGroup.class.getSimpleName(), project, group);
332343
em.createQuery("DELETE FROM CacheProjectGroup WHERE project.id=:project and group.id=:group")
333344
.setParameter("project", project)
@@ -337,19 +348,19 @@ private int persistProjectGroups(final Map<String, CacheGroup> groups) {
337348
);
338349

339350

340-
return allProjectGroup.size();
351+
return allProjectGroups.size();
341352
}
342353

343354
@Override
344355
public void removeGroupFromGroup(final GroupOrg subGroup, final GroupOrg group) {
345-
em.createQuery("DELETE FROM CacheMembership WHERE subGroup.id=:subGroup AND group.id=:group")
346-
.setParameter(GROUP_ATTRIBUTE, group.getId()).setParameter("subGroup", subGroup.getId()).executeUpdate();
356+
removeAll(em.createQuery("FROM CacheMembership WHERE subGroup.id=:subGroup AND group.id=:group")
357+
.setParameter(GROUP_ATTRIBUTE, group.getId()).setParameter("subGroup", subGroup.getId()));
347358
}
348359

349360
@Override
350361
public void removeUserFromGroup(final UserOrg user, final GroupOrg group) {
351-
em.createQuery("DELETE FROM CacheMembership WHERE user.id=:user AND group.id=:group")
352-
.setParameter(GROUP_ATTRIBUTE, group.getId()).setParameter(USER_ATTRIBUTE, user.getId()).executeUpdate();
362+
removeAll(em.createQuery("FROM CacheMembership WHERE user.id=:user AND group.id=:group")
363+
.setParameter(GROUP_ATTRIBUTE, group.getId()).setParameter(USER_ATTRIBUTE, user.getId()));
353364
}
354365

355366
private void deleteBatch(Class<?> cls, List<String> ids, Consumer<List<String>> batchConsumer) {

‎src/main/java/org/ligoj/app/plugin/id/resource/UserOrgResource.java

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -283,6 +283,9 @@ private void updateGroupUser(final String user, final String group, final BiPred
283283
final var userOrg = getUserRepository().findByIdExpected(user);
284284

285285
// Check the implied group
286+
if (getGroupRepository().findById(securityHelper.getLogin(), group) == null) {
287+
throw new ValidationJsonException(GROUP, "not-exist", "0", GROUP, "1", group);
288+
}
286289
validateWriteGroup(group, delegates);
287290

288291
// Compute the new groups
@@ -526,7 +529,7 @@ String mapToString(Map<String, String> map) {
526529
/**
527530
* Indicates the two user details have attribute differences.
528531
*/
529-
private boolean hasAttributeChange(final UserOrgEditionVo importEntry, final UserOrg userOrg) {
532+
boolean hasAttributeChange(final UserOrgEditionVo importEntry, final UserOrg userOrg) {
530533
return hasAttributeChange(importEntry, userOrg == null, "new")
531534
|| hasAttributeChange(importEntry, userOrg, SimpleUser::getFirstName, SimpleUser::getLastName, SimpleUser::getCompany, SimpleUser::getLocalId, SimpleUser::getDepartment)
532535
|| hasAttributeChange(importEntry, !mapToString(importEntry.getCustomAttributes()).equals(mapToString(userOrg.getCustomAttributes())), "customAttributes")

‎src/test/java/org/ligoj/app/plugin/id/dao/IdCacheDaoTest.java

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@
2727
* Test class of {@link IdCacheDao}
2828
*/
2929
@ExtendWith(SpringExtension.class)
30-
@ContextConfiguration(locations = {"classpath:/META-INF/spring/application-context-test.xml"})
30+
@ContextConfiguration(locations = "classpath:/META-INF/spring/application-context-test.xml")
3131
@Rollback
3232
@Transactional
3333
class IdCacheDaoTest extends AbstractJpaTest {
@@ -390,6 +390,18 @@ void reset() {
390390
membershipSubGroup.setSubGroup(cacheGroup2);
391391
em.persist(membershipSubGroup);
392392

393+
// Duplicates -> will be purged
394+
final var cacheMembership2Duplicate = new CacheMembership();
395+
cacheMembership2Duplicate.setGroup(cacheGroup2);
396+
cacheMembership2Duplicate.setUser(cacheUser2);
397+
em.persist(cacheMembership2Duplicate);
398+
399+
// Duplicates -> will be purged
400+
final var membershipSubGroupDuplicate = new CacheMembership();
401+
membershipSubGroupDuplicate.setGroup(em.find(CacheGroup.class, "group"));
402+
membershipSubGroupDuplicate.setSubGroup(cacheGroup2);
403+
em.persist(membershipSubGroupDuplicate);
404+
393405
final var deletedCompany = new CacheCompany();
394406
deletedCompany.setId("deleted-company");
395407
deletedCompany.setDescription("deleted-company");
@@ -409,7 +421,6 @@ void reset() {
409421
deletedMembershipFromDeletedUser.setUser(deletedUser);
410422
em.persist(deletedMembershipFromDeletedUser);
411423

412-
413424
// Unlinked memberships
414425
final var cacheGroup3 = new CacheGroup();
415426
cacheGroup3.setId("group3");
@@ -448,6 +459,13 @@ void reset() {
448459
em.persist(unlinkedProjectGroup);
449460
em.flush();
450461

462+
463+
final var unlinkedProjectGroupDuplicate = new CacheProjectGroup();
464+
unlinkedProjectGroupDuplicate.setGroup(cacheGroup2);
465+
unlinkedProjectGroupDuplicate.setProject(em.createQuery("FROM Project WHERE pkey = :pkey", Project.class).setParameter("pkey", "pj").getSingleResult());
466+
em.persist(unlinkedProjectGroupDuplicate);
467+
em.flush();
468+
451469
final var brokenProjectGroup2 = new CacheProjectGroup();
452470
brokenProjectGroup2.setGroup(brokenGroup);
453471
brokenProjectGroup2.setProject(project);
@@ -539,6 +557,10 @@ void reset() {
539557
.getResultList();
540558
Assertions.assertEquals(2, pGroups.size()); // Group & Group2
541559

560+
Assertions.assertNull(em.find(CacheMembership.class, cacheMembership2Duplicate.getId()));
561+
Assertions.assertNull(em.find(CacheMembership.class, membershipSubGroupDuplicate.getId()));
562+
Assertions.assertNull(em.find(CacheMembership.class, unlinkedProjectGroupDuplicate.getId()));
563+
542564
// Redundant reset
543565
dao.reset(companies, groups, users);
544566
}

‎src/test/java/org/ligoj/app/plugin/id/dao/TestAbstractMemCacheRepositoryTest.java

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,18 @@ void removeGroupFromGroup() {
174174
Assertions.assertEquals(0, parent.getSubGroups().size());
175175
}
176176

177+
@Test
178+
void removeGroupFromGroupNull() {
179+
final var parent = groupLdap2;
180+
final var child = groupLdap;
181+
child.setParent(parent.getId());
182+
Assertions.assertNotNull(child.getParent());
183+
repository.removeGroupFromGroup(child, null);
184+
185+
// Check the new status
186+
Assertions.assertNull(child.getParent());
187+
}
188+
177189
@Test
178190
void createGroup() {
179191
final var newGroupLdap = new GroupOrg("dn3", "G3", new HashSet<>());

0 commit comments

Comments
 (0)