chore: Use Spring projection for workspace user pics (#30804)

We use Spring's native support for projections here, instead of setting
the `.fields()` ourselves. The advantage is that this way translates
directly to Postgres.

The cost is that, the field names in projection are duplicated. For
example, if `profilePhotoAssetId` is renamed or otherwise changed, we
won't see a compile error. This can be prevented with a test. The
`findPhotoAssetsByUserIds_WhenPhotoAssetIdExist_ReturnsPhotoAssetId`
test is able to catch this case perfectly well.
This commit is contained in:
Shrikant Sharat Kandula 2024-02-02 05:46:49 +05:30 committed by GitHub
parent af407904bd
commit 6d0b7af54e
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
9 changed files with 51 additions and 71 deletions

View File

@ -0,0 +1,7 @@
package com.appsmith.server.projections;
public interface UserDataProfilePhotoProjection {
String getUserId();
String getProfilePhotoAssetId();
}

View File

@ -3,7 +3,6 @@ package com.appsmith.server.repositories.ce;
import com.appsmith.server.domains.UserData; import com.appsmith.server.domains.UserData;
import com.appsmith.server.repositories.AppsmithRepository; import com.appsmith.server.repositories.AppsmithRepository;
import com.mongodb.client.result.UpdateResult; import com.mongodb.client.result.UpdateResult;
import reactor.core.publisher.Flux;
import reactor.core.publisher.Mono; import reactor.core.publisher.Mono;
import java.util.List; import java.util.List;
@ -14,7 +13,5 @@ public interface CustomUserDataRepositoryCE extends AppsmithRepository<UserData>
Mono<UpdateResult> removeIdFromRecentlyUsedList(String userId, String workspaceId, List<String> applicationIds); Mono<UpdateResult> removeIdFromRecentlyUsedList(String userId, String workspaceId, List<String> applicationIds);
Flux<UserData> findPhotoAssetsByUserIds(Iterable<String> userId);
Mono<String> fetchMostRecentlyUsedWorkspaceId(String userId); Mono<String> fetchMostRecentlyUsedWorkspaceId(String userId);
} }

View File

@ -6,16 +6,13 @@ import com.appsmith.server.dtos.QRecentlyUsedEntityDTO;
import com.appsmith.server.dtos.RecentlyUsedEntityDTO; import com.appsmith.server.dtos.RecentlyUsedEntityDTO;
import com.appsmith.server.repositories.BaseAppsmithRepositoryImpl; import com.appsmith.server.repositories.BaseAppsmithRepositoryImpl;
import com.appsmith.server.repositories.CacheableRepositoryHelper; import com.appsmith.server.repositories.CacheableRepositoryHelper;
import com.google.common.collect.Lists;
import com.mongodb.BasicDBObject; import com.mongodb.BasicDBObject;
import com.mongodb.client.result.UpdateResult; import com.mongodb.client.result.UpdateResult;
import org.springframework.data.mongodb.core.ReactiveMongoOperations; import org.springframework.data.mongodb.core.ReactiveMongoOperations;
import org.springframework.data.mongodb.core.convert.MongoConverter; import org.springframework.data.mongodb.core.convert.MongoConverter;
import org.springframework.data.mongodb.core.query.Criteria;
import org.springframework.data.mongodb.core.query.Query; import org.springframework.data.mongodb.core.query.Query;
import org.springframework.data.mongodb.core.query.Update; import org.springframework.data.mongodb.core.query.Update;
import org.springframework.util.CollectionUtils; import org.springframework.util.CollectionUtils;
import reactor.core.publisher.Flux;
import reactor.core.publisher.Mono; import reactor.core.publisher.Mono;
import java.util.List; import java.util.List;
@ -56,23 +53,6 @@ public class CustomUserDataRepositoryCEImpl extends BaseAppsmithRepositoryImpl<U
query(where(fieldName(QUserData.userData.userId)).is(userId)), update, UserData.class); query(where(fieldName(QUserData.userData.userId)).is(userId)), update, UserData.class);
} }
/**
* Fetches a list of UserData objects from DB where userId matches with the provided a list of userId.
* The returned UserData objects will have only the userId and photoAssetId fields.
*
* @param userId List of userId as a list
* @return Flux of UserData with only the photoAssetId and userId fields
*/
@Override
public Flux<UserData> findPhotoAssetsByUserIds(Iterable<String> userId) {
// need to convert from Iterable to ArrayList because the "in" method of criteria takes a collection as input
Criteria criteria = where(fieldName(QUserData.userData.userId)).in(Lists.newArrayList(userId));
return queryAll()
.criteria(criteria)
.fields(fieldName(QUserData.userData.profilePhotoAssetId), fieldName(QUserData.userData.userId))
.submit();
}
@Override @Override
public Mono<String> fetchMostRecentlyUsedWorkspaceId(String userId) { public Mono<String> fetchMostRecentlyUsedWorkspaceId(String userId) {
final Query query = query(where(fieldName(QUserData.userData.userId)).is(userId)); final Query query = query(where(fieldName(QUserData.userData.userId)).is(userId));

View File

@ -1,11 +1,24 @@
package com.appsmith.server.repositories.ce; package com.appsmith.server.repositories.ce;
import com.appsmith.server.domains.UserData; import com.appsmith.server.domains.UserData;
import com.appsmith.server.projections.UserDataProfilePhotoProjection;
import com.appsmith.server.repositories.BaseRepository; import com.appsmith.server.repositories.BaseRepository;
import com.appsmith.server.repositories.CustomUserDataRepository; import com.appsmith.server.repositories.CustomUserDataRepository;
import reactor.core.publisher.Flux;
import reactor.core.publisher.Mono; import reactor.core.publisher.Mono;
import java.util.Collection;
public interface UserDataRepositoryCE extends BaseRepository<UserData, String>, CustomUserDataRepository { public interface UserDataRepositoryCE extends BaseRepository<UserData, String>, CustomUserDataRepository {
Mono<UserData> findByUserId(String userId); Mono<UserData> findByUserId(String userId);
/**
* Fetches a list of UserData objects from DB where userId matches with the provided a list of userId.
* The returned UserData objects will have only the userId and photoAssetId fields.
*
* @param userIds Collection of userId strings
* @return Flux of UserData with only the photoAssetId and userId fields
*/
Flux<UserDataProfilePhotoProjection> findByUserIdIn(Collection<String> userIds);
} }

View File

@ -1,11 +1,8 @@
package com.appsmith.server.services; package com.appsmith.server.services;
import com.appsmith.server.notifications.EmailSender;
import com.appsmith.server.repositories.UserDataRepository;
import com.appsmith.server.repositories.UserRepository; import com.appsmith.server.repositories.UserRepository;
import com.appsmith.server.services.ce.UserWorkspaceServiceCEImpl; import com.appsmith.server.services.ce.UserWorkspaceServiceCEImpl;
import com.appsmith.server.solutions.PermissionGroupPermission; import com.appsmith.server.solutions.PermissionGroupPermission;
import com.appsmith.server.solutions.PolicySolution;
import com.appsmith.server.solutions.WorkspacePermission; import com.appsmith.server.solutions.WorkspacePermission;
import lombok.extern.slf4j.Slf4j; import lombok.extern.slf4j.Slf4j;
import org.springframework.stereotype.Service; import org.springframework.stereotype.Service;
@ -18,9 +15,6 @@ public class UserWorkspaceServiceImpl extends UserWorkspaceServiceCEImpl impleme
SessionUserService sessionUserService, SessionUserService sessionUserService,
WorkspaceService workspaceService, WorkspaceService workspaceService,
UserRepository userRepository, UserRepository userRepository,
UserDataRepository userDataRepository,
PolicySolution policySolution,
EmailSender emailSender,
UserDataService userDataService, UserDataService userDataService,
PermissionGroupService permissionGroupService, PermissionGroupService permissionGroupService,
TenantService tenantService, TenantService tenantService,
@ -31,9 +25,6 @@ public class UserWorkspaceServiceImpl extends UserWorkspaceServiceCEImpl impleme
sessionUserService, sessionUserService,
workspaceService, workspaceService,
userRepository, userRepository,
userDataRepository,
policySolution,
emailSender,
userDataService, userDataService,
permissionGroupService, permissionGroupService,
tenantService, tenantService,

View File

@ -9,6 +9,7 @@ import org.springframework.http.codec.multipart.Part;
import org.springframework.web.server.ServerWebExchange; import org.springframework.web.server.ServerWebExchange;
import reactor.core.publisher.Mono; import reactor.core.publisher.Mono;
import java.util.Collection;
import java.util.Map; import java.util.Map;
public interface UserDataServiceCE { public interface UserDataServiceCE {
@ -21,6 +22,8 @@ public interface UserDataServiceCE {
Mono<UserData> getForUserEmail(String email); Mono<UserData> getForUserEmail(String email);
Mono<Map<String, String>> getProfilePhotoAssetIdsForUserIds(Collection<String> userIds);
Mono<UserData> updateForCurrentUser(UserData updates); Mono<UserData> updateForCurrentUser(UserData updates);
Mono<UserData> updateForUser(User user, UserData updates); Mono<UserData> updateForUser(User user, UserData updates);

View File

@ -11,6 +11,7 @@ import com.appsmith.server.dtos.RecentlyUsedEntityDTO;
import com.appsmith.server.exceptions.AppsmithError; import com.appsmith.server.exceptions.AppsmithError;
import com.appsmith.server.exceptions.AppsmithException; import com.appsmith.server.exceptions.AppsmithException;
import com.appsmith.server.helpers.CollectionUtils; import com.appsmith.server.helpers.CollectionUtils;
import com.appsmith.server.projections.UserDataProfilePhotoProjection;
import com.appsmith.server.repositories.ApplicationRepository; import com.appsmith.server.repositories.ApplicationRepository;
import com.appsmith.server.repositories.UserDataRepository; import com.appsmith.server.repositories.UserDataRepository;
import com.appsmith.server.repositories.UserRepository; import com.appsmith.server.repositories.UserRepository;
@ -40,6 +41,7 @@ import reactor.core.scheduler.Scheduler;
import reactor.util.function.Tuple2; import reactor.util.function.Tuple2;
import java.util.ArrayList; import java.util.ArrayList;
import java.util.Collection;
import java.util.List; import java.util.List;
import java.util.Map; import java.util.Map;
@ -125,6 +127,15 @@ public class UserDataServiceCEImpl extends BaseService<UserDataRepository, UserD
.flatMap(this::getForUser); .flatMap(this::getForUser);
} }
@Override
public Mono<Map<String, String>> getProfilePhotoAssetIdsForUserIds(Collection<String> userIds) {
return repository
.findByUserIdIn(userIds)
.collectMap(
UserDataProfilePhotoProjection::getUserId,
UserDataProfilePhotoProjection::getProfilePhotoAssetId);
}
@Override @Override
public Mono<UserData> updateForCurrentUser(UserData updates) { public Mono<UserData> updateForCurrentUser(UserData updates) {
return sessionUserService return sessionUserService

View File

@ -12,8 +12,6 @@ import com.appsmith.server.dtos.UpdatePermissionGroupDTO;
import com.appsmith.server.exceptions.AppsmithError; import com.appsmith.server.exceptions.AppsmithError;
import com.appsmith.server.exceptions.AppsmithException; import com.appsmith.server.exceptions.AppsmithException;
import com.appsmith.server.helpers.AppsmithComparators; import com.appsmith.server.helpers.AppsmithComparators;
import com.appsmith.server.notifications.EmailSender;
import com.appsmith.server.repositories.UserDataRepository;
import com.appsmith.server.repositories.UserRepository; import com.appsmith.server.repositories.UserRepository;
import com.appsmith.server.services.PermissionGroupService; import com.appsmith.server.services.PermissionGroupService;
import com.appsmith.server.services.SessionUserService; import com.appsmith.server.services.SessionUserService;
@ -21,7 +19,6 @@ import com.appsmith.server.services.TenantService;
import com.appsmith.server.services.UserDataService; import com.appsmith.server.services.UserDataService;
import com.appsmith.server.services.WorkspaceService; import com.appsmith.server.services.WorkspaceService;
import com.appsmith.server.solutions.PermissionGroupPermission; import com.appsmith.server.solutions.PermissionGroupPermission;
import com.appsmith.server.solutions.PolicySolution;
import com.appsmith.server.solutions.WorkspacePermission; import com.appsmith.server.solutions.WorkspacePermission;
import com.mongodb.client.result.UpdateResult; import com.mongodb.client.result.UpdateResult;
import lombok.extern.slf4j.Slf4j; import lombok.extern.slf4j.Slf4j;
@ -52,9 +49,6 @@ public class UserWorkspaceServiceCEImpl implements UserWorkspaceServiceCE {
private final SessionUserService sessionUserService; private final SessionUserService sessionUserService;
private final WorkspaceService workspaceService; private final WorkspaceService workspaceService;
private final UserRepository userRepository; private final UserRepository userRepository;
private final UserDataRepository userDataRepository;
private final PolicySolution policySolution;
private final EmailSender emailSender;
private final UserDataService userDataService; private final UserDataService userDataService;
private final PermissionGroupService permissionGroupService; private final PermissionGroupService permissionGroupService;
private final TenantService tenantService; private final TenantService tenantService;
@ -66,9 +60,6 @@ public class UserWorkspaceServiceCEImpl implements UserWorkspaceServiceCE {
SessionUserService sessionUserService, SessionUserService sessionUserService,
WorkspaceService workspaceService, WorkspaceService workspaceService,
UserRepository userRepository, UserRepository userRepository,
UserDataRepository userDataRepository,
PolicySolution policySolution,
EmailSender emailSender,
UserDataService userDataService, UserDataService userDataService,
PermissionGroupService permissionGroupService, PermissionGroupService permissionGroupService,
TenantService tenantService, TenantService tenantService,
@ -77,9 +68,6 @@ public class UserWorkspaceServiceCEImpl implements UserWorkspaceServiceCE {
this.sessionUserService = sessionUserService; this.sessionUserService = sessionUserService;
this.workspaceService = workspaceService; this.workspaceService = workspaceService;
this.userRepository = userRepository; this.userRepository = userRepository;
this.userDataRepository = userDataRepository;
this.policySolution = policySolution;
this.emailSender = emailSender;
this.userDataService = userDataService; this.userDataService = userDataService;
this.permissionGroupService = permissionGroupService; this.permissionGroupService = permissionGroupService;
this.tenantService = tenantService; this.tenantService = tenantService;
@ -261,27 +249,23 @@ public class UserWorkspaceServiceCEImpl implements UserWorkspaceServiceCE {
userIdsMono.flatMapMany(userRepository::findAllById).collectMap(User::getId); userIdsMono.flatMapMany(userRepository::findAllById).collectMap(User::getId);
// Create a map of UserData.userUd to UserData // Create a map of UserData.userUd to UserData
Mono<Map<String, UserData>> userDataMapMono = userIdsMono Mono<Map<String, String>> userDataMapMono = userIdsMono
// get the profile photos of the list of users // get the profile photos of the list of users
.flatMapMany(userIdsSet -> userDataRepository.findPhotoAssetsByUserIds( .flatMap(userIdsSet -> userDataService.getProfilePhotoAssetIdsForUserIds(
userIdsSet.stream().toList())) userIdsSet.stream().toList()));
.collectMap(UserData::getUserId);
// Update name and username in the list of UserAndGroupDTO // Update name and username in the list of UserAndGroupDTO
userAndPermissionGroupDTOsMono = Mono.zip(userAndPermissionGroupDTOsMono, userMapMono, userDataMapMono) userAndPermissionGroupDTOsMono = Mono.zip(userAndPermissionGroupDTOsMono, userMapMono, userDataMapMono)
.map(tuple -> { .map(tuple -> {
List<MemberInfoDTO> workspaceMemberInfoDTOList = tuple.getT1(); List<MemberInfoDTO> workspaceMemberInfoDTOList = tuple.getT1();
Map<String, User> userMap = tuple.getT2(); Map<String, User> userMap = tuple.getT2();
Map<String, UserData> userDataMap = tuple.getT3(); Map<String, String> userDataMap = tuple.getT3();
workspaceMemberInfoDTOList.forEach(userAndPermissionGroupDTO -> { workspaceMemberInfoDTOList.forEach(userAndPermissionGroupDTO -> {
User user = userMap.get(userAndPermissionGroupDTO.getUserId()); User user = userMap.get(userAndPermissionGroupDTO.getUserId());
UserData userData = userDataMap.get(userAndPermissionGroupDTO.getUserId());
userAndPermissionGroupDTO.setName( userAndPermissionGroupDTO.setName(
Optional.ofNullable(user.getName()).orElse(user.computeFirstName())); Optional.ofNullable(user.getName()).orElse(user.computeFirstName()));
userAndPermissionGroupDTO.setUsername(user.getUsername()); userAndPermissionGroupDTO.setUsername(user.getUsername());
if (userData != null) { userAndPermissionGroupDTO.setPhotoId(userDataMap.get(userAndPermissionGroupDTO.getUserId()));
userAndPermissionGroupDTO.setPhotoId(userData.getProfilePhotoAssetId());
}
}); });
return workspaceMemberInfoDTOList; return workspaceMemberInfoDTOList;
}); });
@ -324,9 +308,8 @@ public class UserWorkspaceServiceCEImpl implements UserWorkspaceServiceCE {
userIdsMono.flatMapMany(userRepository::findAllById).collectMap(User::getId); userIdsMono.flatMapMany(userRepository::findAllById).collectMap(User::getId);
// Create a map of UserData.userUd to UserData // Create a map of UserData.userUd to UserData
Mono<Map<String, UserData>> userDataMapMono = userIdsMono Mono<Map<String, String>> userDataMapMono =
.flatMapMany(userDataRepository::findPhotoAssetsByUserIds) userIdsMono.flatMap(userDataService::getProfilePhotoAssetIdsForUserIds);
.collectMap(UserData::getUserId);
Flux<Map<String, Collection<PermissionGroup>>> permissionGroupsByWorkspaceFlux = Flux<Map<String, Collection<PermissionGroup>>> permissionGroupsByWorkspaceFlux =
permissionGroupsByWorkspacesMono.repeat(); permissionGroupsByWorkspacesMono.repeat();
@ -347,16 +330,14 @@ public class UserWorkspaceServiceCEImpl implements UserWorkspaceServiceCE {
.map(tuple1 -> { .map(tuple1 -> {
List<MemberInfoDTO> workspaceMemberInfoDTOList = tuple1.getT1(); List<MemberInfoDTO> workspaceMemberInfoDTOList = tuple1.getT1();
Map<String, User> userMap = tuple1.getT2(); Map<String, User> userMap = tuple1.getT2();
Map<String, UserData> userDataMap = tuple1.getT3(); Map<String, String> userDataMap = tuple1.getT3();
workspaceMemberInfoDTOList.forEach(userAndPermissionGroupDTO -> { workspaceMemberInfoDTOList.forEach(userAndPermissionGroupDTO -> {
User user = userMap.get(userAndPermissionGroupDTO.getUserId()); User user = userMap.get(userAndPermissionGroupDTO.getUserId());
UserData userData = userDataMap.get(userAndPermissionGroupDTO.getUserId());
userAndPermissionGroupDTO.setName( userAndPermissionGroupDTO.setName(
Optional.ofNullable(user.getName()).orElse(user.computeFirstName())); Optional.ofNullable(user.getName()).orElse(user.computeFirstName()));
userAndPermissionGroupDTO.setUsername(user.getUsername()); userAndPermissionGroupDTO.setUsername(user.getUsername());
if (userData != null) { userAndPermissionGroupDTO.setPhotoId(
userAndPermissionGroupDTO.setPhotoId(userData.getProfilePhotoAssetId()); userDataMap.get(userAndPermissionGroupDTO.getUserId()));
}
}); });
return workspaceMemberInfoDTOList; return workspaceMemberInfoDTOList;
}); });

View File

@ -1,6 +1,7 @@
package com.appsmith.server.repositories; package com.appsmith.server.repositories;
import com.appsmith.server.domains.UserData; import com.appsmith.server.domains.UserData;
import com.appsmith.server.projections.UserDataProfilePhotoProjection;
import com.mongodb.client.result.UpdateResult; import com.mongodb.client.result.UpdateResult;
import lombok.extern.slf4j.Slf4j; import lombok.extern.slf4j.Slf4j;
import org.junit.jupiter.api.Test; import org.junit.jupiter.api.Test;
@ -160,29 +161,25 @@ public class CustomUserDataRepositoryTest {
UserData userDataOne = new UserData(); UserData userDataOne = new UserData();
userDataOne.setUserId(firstId); userDataOne.setUserId(firstId);
userDataOne.setProfilePhotoAssetId(photoId); userDataOne.setProfilePhotoAssetId(photoId);
userDataOne.setRecentlyUsedAppIds(List.of("abc"));
UserData userDataTwo = new UserData(); UserData userDataTwo = new UserData();
userDataTwo.setUserId(secondId); userDataTwo.setUserId(secondId);
userDataTwo.setRecentlyUsedAppIds(List.of("abc"));
Flux<UserData> userDataFlux = userDataRepository Flux<UserDataProfilePhotoProjection> userDataFlux = userDataRepository
.saveAll(List.of(userDataOne, userDataTwo)) .saveAll(List.of(userDataOne, userDataTwo))
.map(UserData::getUserId) .map(UserData::getUserId)
.collectList() .collectList()
.flatMapMany(userDataRepository::findPhotoAssetsByUserIds); .flatMapMany(userDataRepository::findByUserIdIn);
StepVerifier.create(userDataFlux.collectMap(UserData::getUserId)) StepVerifier.create(userDataFlux.collectMap(UserDataProfilePhotoProjection::getUserId))
.assertNext(userDataMap -> { .assertNext(userDataMap -> {
assertThat(userDataMap.size()).isEqualTo(2); assertThat(userDataMap).hasSize(2);
UserData firstUserData = userDataMap.get(firstId); UserDataProfilePhotoProjection firstUserData = userDataMap.get(firstId);
assertThat(firstUserData.getProfilePhotoAssetId()).isEqualTo(photoId); assertThat(firstUserData.getProfilePhotoAssetId()).isEqualTo(photoId);
assertThat(firstUserData.getRecentlyUsedAppIds()).isNull();
UserData secondUserData = userDataMap.get(secondId); UserDataProfilePhotoProjection secondUserData = userDataMap.get(secondId);
assertThat(secondUserData.getProfilePhotoAssetId()).isNull(); assertThat(secondUserData.getProfilePhotoAssetId()).isNull();
assertThat(secondUserData.getRecentlyUsedAppIds()).isNull();
}) })
.verifyComplete(); .verifyComplete();
} }