Skip to content

Commit 50f81d7

Browse files
overheadhunteroverheadhunter
authored
Merge pull request #377 from cryptomator/feature/syncer-efficiency
more bulk transactions for group members
2 parents 3a6ee52 + 6421fdd commit 50f81d7

File tree

4 files changed

+66
-65
lines changed

4 files changed

+66
-65
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1515

1616
- Updated Keycloak to 26.4.7
1717
- Update Quarkus to 3.20.4 LTS
18+
- Improved efficiency of keycloak-to-hub data sync (#377)
1819
- Improved efficiency of group-based access permission checks (#372)
1920
- Migrated aes-siv and base encoding libraries to [`@noble/ciphers`](https://github.com/paulmillr/noble-ciphers) and [`@scure/base`](https://github.com/paulmillr/scure-base/) (#373)
2021

‎backend/src/main/java/org/cryptomator/hub/entities/Group.java‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,10 +32,6 @@ public Set<Authority> getMembers() {
3232
return members;
3333
}
3434

35-
public void setMembers(Set<Authority> members) {
36-
this.members = members;
37-
}
38-
3935
@Transient
4036
public int getMemberSize() {
4137
return members.size();

‎backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java‎

Lines changed: 40 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import org.cryptomator.hub.entities.Group;
1010
import org.cryptomator.hub.entities.User;
1111

12+
import java.util.HashMap;
1213
import java.util.HashSet;
1314
import java.util.Map;
1415
import java.util.Set;
@@ -36,18 +37,27 @@ void sync() {
3637

3738
@Transactional
3839
void sync(Map<String, KeycloakGroupDto> keycloakGroups, Map<String, KeycloakUserDto> keycloakUsers) {
40+
// get current state from database:
3941
var databaseUsers = userRepo.findAll().stream().collect(Collectors.toMap(User::getId, Function.identity()));
4042
var databaseGroups = groupRepo.findAll().stream().collect(Collectors.toMap(Group::getId, Function.identity()));
41-
syncAddedUsers(keycloakUsers, databaseUsers);
43+
44+
// sync users:
45+
var addedUsers = syncAddedUsers(keycloakUsers, databaseUsers);
4246
var deletedUserIds = syncDeletedUsers(keycloakUsers, databaseUsers);
4347
syncUpdatedUsers(keycloakUsers, databaseUsers, deletedUserIds);
44-
syncAddedGroups(keycloakGroups, databaseGroups, databaseUsers);
48+
49+
// all users after additions and deletions:
50+
Map<String, Authority> allAuthorities = merge(databaseUsers, addedUsers);
51+
deletedUserIds.forEach(allAuthorities::remove);
52+
53+
// sync groups:
54+
var addedGroups = syncAddedGroups(keycloakGroups, databaseGroups, allAuthorities);
4555
var deletedGroupIds = syncDeletedGroups(keycloakGroups, databaseGroups);
46-
syncUpdatedGroups(keycloakGroups, databaseGroups, deletedGroupIds, databaseUsers);
56+
syncUpdatedGroups(keycloakGroups, databaseGroups, deletedGroupIds, allAuthorities);
4757
}
4858

4959
//visible for testing
50-
void syncAddedUsers(Map<String, KeycloakUserDto> keycloakUsers, Map<String, User> databaseUsers) {
60+
Map<String, User> syncAddedUsers(Map<String, KeycloakUserDto> keycloakUsers, Map<String, User> databaseUsers) {
5161
var addedIds = diff(keycloakUsers.keySet(), databaseUsers.keySet());
5262
var added = addedIds.stream().map(id -> {
5363
var keycloakUser = keycloakUsers.get(id);
@@ -57,9 +67,10 @@ void syncAddedUsers(Map<String, KeycloakUserDto> keycloakUsers, Map<String, User
5767
databaseUser.setEmail(keycloakUser.email());
5868
databaseUser.setPictureUrl(keycloakUser.pictureUrl());
5969
return databaseUser;
60-
});
61-
userRepo.persist(added);
70+
}).collect(Collectors.toMap(User::getId, Function.identity()));
71+
userRepo.persist(added.values());
6272
effectiveGroupMembershipRepo.updateUsers(addedIds);
73+
return added;
6374
}
6475

6576
//visible for testing
@@ -83,30 +94,20 @@ void syncUpdatedUsers(Map<String, KeycloakUserDto> keycloakUsers, Map<String, Us
8394
}
8495

8596
//visible for testing
86-
void syncAddedGroups(Map<String, KeycloakGroupDto> keycloakGroups, Map<String, Group> databaseGroups, Map<String, User> databaseUsers) {
97+
Map<String, Group> syncAddedGroups(Map<String, KeycloakGroupDto> keycloakGroups, Map<String, Group> databaseGroups, Map<String, Authority> allAuthorities) {
8798
var addedIds = diff(keycloakGroups.keySet(), databaseGroups.keySet());
8899
var added = addedIds.stream().map(id -> {
89100
var keycloakGroup = keycloakGroups.get(id);
90101
var databaseGroup = new Group();
91102
databaseGroup.setId(keycloakGroup.id());
92103
databaseGroup.setName(keycloakGroup.name());
93104
databaseGroup.setPictureUrl(keycloakGroup.pictureUrl());
94-
Set<Authority> members = new HashSet<>();
95-
for (var keycloakMember : keycloakGroup.members()) {
96-
var databaseUser = databaseUsers.get(keycloakMember.id());
97-
if (databaseUser == null) {
98-
// User might have been just added, fetch from database
99-
databaseUser = userRepo.findById(keycloakMember.id());
100-
}
101-
if (databaseUser != null) {
102-
members.add(databaseUser);
103-
}
104-
}
105-
databaseGroup.setMembers(members);
105+
databaseGroup.getMembers().addAll(keycloakGroup.members().stream().map(KeycloakUserDto::id).map(allAuthorities::get).collect(Collectors.toSet()));
106106
return databaseGroup;
107-
});
108-
groupRepo.persist(added);
107+
}).collect(Collectors.toMap(Group::getId, Function.identity()));
108+
groupRepo.persist(added.values());
109109
effectiveGroupMembershipRepo.updateGroups(addedIds);
110+
return added;
110111
}
111112

112113
//visible for testing
@@ -118,39 +119,39 @@ Set<String> syncDeletedGroups(Map<String, KeycloakGroupDto> keycloakGroups, Map<
118119
}
119120

120121
//visible for testing
121-
void syncUpdatedGroups(Map<String, KeycloakGroupDto> keycloakGroups, Map<String, Group> databaseGroups, Set<String> deletedGroupIds, Map<String, User> databaseUsers) {
122+
void syncUpdatedGroups(Map<String, KeycloakGroupDto> keycloakGroups, Map<String, Group> databaseGroups, Set<String> deletedGroupIds, Map<String, Authority> allAuthorities) {
122123
var toUpdateIds = diff(databaseGroups.keySet(), deletedGroupIds);
123124
var idsOfGroupsWithChangedMembers = new HashSet<String>();
124125
for (var id : toUpdateIds) {
125126
var databaseGroup = databaseGroups.get(id);
126127
var keycloakGroup = keycloakGroups.get(id);
127-
var wantIds = keycloakGroup.members().stream().map(KeycloakUserDto::id).collect(Collectors.toSet());
128-
var haveIds = databaseGroup.getMembers().stream().map(Authority::getId).collect(Collectors.toSet());
129128
databaseGroup.setName(keycloakGroup.name());
130129
databaseGroup.setPictureUrl(keycloakGroup.pictureUrl());
131-
for (var addId : diff(wantIds, haveIds)) {
132-
var databaseUser = databaseUsers.get(addId);
133-
if (databaseUser == null) {
134-
// User might have been just added, fetch from database
135-
databaseUser = userRepo.findById(addId);
136-
}
137-
if (databaseUser != null) {
138-
databaseGroup.getMembers().add(databaseUser);
139-
}
140-
}
141-
for (var removeId : diff(haveIds, wantIds)) {
142-
databaseGroup.getMembers().removeIf(u -> u.getId().equals(removeId));
143-
}
144-
if (!wantIds.containsAll(haveIds) || !haveIds.containsAll(wantIds)) {
130+
131+
// update members:
132+
var kcMemberIds = keycloakGroup.members().stream().map(KeycloakUserDto::id).collect(Collectors.toSet());
133+
var dbMemberIds = databaseGroup.getMembers().stream().map(Authority::getId).collect(Collectors.toSet());
134+
var addedMemberIds = diff(kcMemberIds, dbMemberIds);
135+
var addedMembers = addedMemberIds.stream().map(allAuthorities::get).collect(Collectors.toSet());
136+
databaseGroup.getMembers().addAll(addedMembers);
137+
var removedMemberIds = diff(dbMemberIds, kcMemberIds);
138+
databaseGroup.getMembers().removeIf(u -> removedMemberIds.contains(u.getId()));
139+
if (!addedMemberIds.isEmpty() || !removedMemberIds.isEmpty()) {
145140
idsOfGroupsWithChangedMembers.add(id);
146141
}
147142
}
148143
effectiveGroupMembershipRepo.updateGroups(idsOfGroupsWithChangedMembers);
149144
}
150145

151-
private <T> Set<T> diff(Set<T> base, Set<T> difference) {
146+
private static <T> Set<T> diff(Set<T> base, Set<T> difference) {
152147
var result = new HashSet<>(base);
153148
result.removeAll(difference);
154149
return result;
155150
}
151+
152+
private static <K, V> Map<K, V> merge(Map<K, ? extends V> first, Map<K, ? extends V> second) {
153+
Map<K, V> result = new HashMap<>(first);
154+
result.putAll(second);
155+
return result;
156+
}
156157
}

‎backend/src/test/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPullerTest.java‎

Lines changed: 25 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
import org.cryptomator.hub.entities.User;
77
import org.hamcrest.MatcherAssert;
88
import org.hamcrest.Matchers;
9+
import org.junit.jupiter.api.Assertions;
910
import org.junit.jupiter.api.BeforeEach;
1011
import org.junit.jupiter.api.DisplayName;
1112
import org.junit.jupiter.api.Nested;
@@ -50,16 +51,16 @@ void setUp() {
5051
remoteUserPuller.effectiveGroupMembershipRepo = effectiveGroupMembershipRepo;
5152
persistedUsers.clear();
5253
Mockito.doAnswer(invocation -> {
53-
Stream<User> stream = invocation.getArgument(0);
54-
persistedUsers.addAll(stream.toList());
54+
Iterable<User> iterable = invocation.getArgument(0);
55+
iterable.forEach(persistedUsers::add);
5556
return null;
56-
}).when(userRepo).persist(Mockito.<Stream<User>>any());
57+
}).when(userRepo).persist(Mockito.<Iterable<User>>any());
5758
persistedGroups.clear();
5859
Mockito.doAnswer(invocation -> {
59-
Stream<Group> stream = invocation.getArgument(0);
60-
persistedGroups.addAll(stream.toList());
60+
Iterable<Group> iterable = invocation.getArgument(0);
61+
iterable.forEach(persistedGroups::add);
6162
return null;
62-
}).when(groupRepo).persist(Mockito.<Stream<Group>>any());
63+
}).when(groupRepo).persist(Mockito.<Iterable<Group>>any());
6364
}
6465

6566
@Nested
@@ -77,8 +78,8 @@ public class AddDeleteUsers {
7778
",;,;,"
7879
}, delimiterString = ";")
7980
void testAddUsers(@ConvertWith(StringArrayConverter.class) String[] keycloakUserIdString, @ConvertWith(StringArrayConverter.class) String[] databaseUserIdString, @ConvertWith(StringArrayConverter.class) String[] addedUserIdString) {
80-
Map<String, KeycloakUserDto> keycloakUsers = Mockito.mock(Map.class);
81-
Map<String, User> databaseUsers = Mockito.mock(Map.class);
81+
Map<String, KeycloakUserDto> keycloakUsers = Mockito.mock();
82+
Map<String, User> databaseUsers = Mockito.mock();
8283

8384
var keycloakUserIds = Set.of(keycloakUserIdString);
8485
var databaseUserIds = Set.of(databaseUserIdString);
@@ -92,9 +93,10 @@ void testAddUsers(@ConvertWith(StringArrayConverter.class) String[] keycloakUser
9293
Mockito.when(keycloakUsers.get(userId)).thenReturn(keycloakUser);
9394
}
9495

95-
remoteUserPuller.syncAddedUsers(keycloakUsers, databaseUsers);
96+
var added = remoteUserPuller.syncAddedUsers(keycloakUsers, databaseUsers);
9697

97-
Mockito.verify(userRepo).persist(Mockito.<Stream<User>>any());
98+
Assertions.assertEquals(addedUserIds, added.keySet());
99+
Mockito.verify(userRepo).persist(Mockito.<Iterable<User>>any());
98100
Mockito.verify(effectiveGroupMembershipRepo).updateUsers(Mockito.argThat(addedUserIds::containsAll));
99101
for (var userId : addedUserIds) {
100102
MatcherAssert.assertThat(persistedUsers, Matchers.hasItem(
@@ -119,8 +121,8 @@ void testAddUsers(@ConvertWith(StringArrayConverter.class) String[] keycloakUser
119121
",;,;,"
120122
}, delimiterString = ";")
121123
void testDeleteUsers(@ConvertWith(StringArrayConverter.class) String[] keycloakUserIdString, @ConvertWith(StringArrayConverter.class) String[] databaseUserIdString, @ConvertWith(StringArrayConverter.class) String[] deletedUserIdString) {
122-
Map<String, KeycloakUserDto> keycloakUsers = Mockito.mock(Map.class);
123-
Map<String, User> databaseUsers = Mockito.mock(Map.class);
124+
Map<String, KeycloakUserDto> keycloakUsers = Mockito.mock();
125+
Map<String, User> databaseUsers = Mockito.mock();
124126

125127
var keycloakUserIds = Arrays.stream(keycloakUserIdString).collect(Collectors.toSet());
126128
var databaseUserIds = Arrays.stream(databaseUserIdString).collect(Collectors.toSet());
@@ -155,8 +157,8 @@ public class UpdateUsers {
155157
",;,;,;," // all empty
156158
}, delimiterString = ";")
157159
void testUpdateUsers(@ConvertWith(StringArrayConverter.class) String[] keycloakUserIdString, @ConvertWith(StringArrayConverter.class) String[] databaseUserIdString, @ConvertWith(StringArrayConverter.class) String[] deletedUserIdString, @ConvertWith(StringArrayConverter.class) String[] updatedUserIdString) {
158-
Map<String, KeycloakUserDto> keycloakUsers = Mockito.mock(Map.class);
159-
Map<String, User> databaseUsers = Mockito.mock(Map.class);
160+
Map<String, KeycloakUserDto> keycloakUsers = Mockito.mock();
161+
Map<String, User> databaseUsers = Mockito.mock();
160162

161163
var keycloakUserIds = Arrays.stream(keycloakUserIdString).collect(Collectors.toSet());
162164
var databaseUserIds = Arrays.stream(databaseUserIdString).collect(Collectors.toSet());
@@ -213,7 +215,7 @@ void testAddGroups(@ConvertWith(StringArrayConverter.class) String[] keycloakGro
213215
keycloakGroups.put(gid, dto);
214216
}
215217

216-
Map<String, User> databaseUsers = new HashMap<>();
218+
Map<String, Authority> databaseUsers = new HashMap<>();
217219
for (var keycloakUser : kcUsers.values()) {
218220
var databaseUser = Mockito.mock(User.class);
219221
Mockito.when(databaseUser.getId()).thenReturn(keycloakUser.id());
@@ -227,10 +229,11 @@ void testAddGroups(@ConvertWith(StringArrayConverter.class) String[] keycloakGro
227229
databaseGroups.put(gid, databaseGroup);
228230
}
229231

230-
remoteUserPuller.syncAddedGroups(keycloakGroups, databaseGroups, databaseUsers);
232+
var added = remoteUserPuller.syncAddedGroups(keycloakGroups, databaseGroups, databaseUsers);
231233

232234
var addedGroupIds = Set.of(addedGroupIdString);
233-
Mockito.verify(groupRepo).persist(Mockito.<Stream<Group>>any());
235+
Assertions.assertEquals(addedGroupIds, added.keySet());
236+
Mockito.verify(groupRepo).persist(Mockito.<Iterable<Group>>any());
234237
Mockito.verify(effectiveGroupMembershipRepo).updateGroups(Mockito.argThat(addedGroupIds::containsAll));
235238
for (var groupId : addedGroupIds) {
236239
MatcherAssert.assertThat(persistedGroups, Matchers.hasItem(
@@ -255,8 +258,8 @@ void testAddGroups(@ConvertWith(StringArrayConverter.class) String[] keycloakGro
255258
",;,;,"
256259
}, delimiterString = ";")
257260
void testDeleteGroups(@ConvertWith(StringArrayConverter.class) String[] keycloakGroupIdString, @ConvertWith(StringArrayConverter.class) String[] databaseGroupIdString, @ConvertWith(StringArrayConverter.class) String[] deletedGroupIdString) {
258-
Map<String, KeycloakGroupDto> keycloakGroups = Mockito.mock(Map.class);
259-
Map<String, Group> databaseGroups = Mockito.mock(Map.class);
261+
Map<String, KeycloakGroupDto> keycloakGroups = Mockito.mock();
262+
Map<String, Group> databaseGroups = Mockito.mock();
260263

261264
var kcGroupIds = Arrays.stream(keycloakGroupIdString).collect(Collectors.toSet());
262265
var dbGroupIds = Arrays.stream(databaseGroupIdString).collect(Collectors.toSet());
@@ -291,8 +294,8 @@ void testUpdateGroups(@ConvertWith(StringArrayConverter.class) String[] keycloak
291294
var otherKCUser = Mockito.mock(User.class);
292295
Mockito.when(otherKCUser.getId()).thenReturn("U_otherKC");
293296

294-
Map<String, KeycloakGroupDto> keycloakGroups = Mockito.mock(Map.class);
295-
Map<String, Group> databaseGroups = Mockito.mock(Map.class);
297+
Map<String, KeycloakGroupDto> keycloakGroups = Mockito.mock();
298+
Map<String, Group> databaseGroups = Mockito.mock();
296299

297300
var keycloakGroupIds = Arrays.stream(keycloakGroupIdString).collect(Collectors.toSet());
298301
var databaseGroupIds = Arrays.stream(databaseGroupIdString).collect(Collectors.toSet());
@@ -319,7 +322,7 @@ void testUpdateGroups(@ConvertWith(StringArrayConverter.class) String[] keycloak
319322
Mockito.when(databaseGroups.get(groupId)).thenReturn(dbGroup);
320323
}
321324

322-
Map<String, User> databaseUsers = new HashMap<>();
325+
Map<String, Authority> databaseUsers = new HashMap<>();
323326
var userMock = Mockito.mock(User.class);
324327
Mockito.when(userMock.getId()).thenReturn("U_user");
325328
databaseUsers.put("U_user", userMock);

0 commit comments

Comments
 (0)