fix: throw ResourceNotFoundException instead of returning null, replace full-table-scan list queries with JPQL
This commit is contained in:
+11
@@ -0,0 +1,11 @@
|
|||||||
|
package de.zendric.app.xpensely_server.model.Exception;
|
||||||
|
|
||||||
|
import org.springframework.http.HttpStatus;
|
||||||
|
import org.springframework.web.bind.annotation.ResponseStatus;
|
||||||
|
|
||||||
|
@ResponseStatus(HttpStatus.NOT_FOUND)
|
||||||
|
public class ResourceNotFoundException extends RuntimeException {
|
||||||
|
public ResourceNotFoundException(String message) {
|
||||||
|
super(message);
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -3,13 +3,24 @@ package de.zendric.app.xpensely_server.repo;
|
|||||||
import java.util.List;
|
import java.util.List;
|
||||||
|
|
||||||
import org.springframework.data.jpa.repository.JpaRepository;
|
import org.springframework.data.jpa.repository.JpaRepository;
|
||||||
|
import org.springframework.data.jpa.repository.Query;
|
||||||
|
import org.springframework.data.repository.query.Param;
|
||||||
import org.springframework.stereotype.Repository;
|
import org.springframework.stereotype.Repository;
|
||||||
|
|
||||||
import de.zendric.app.xpensely_server.model.ExpenseList;
|
import de.zendric.app.xpensely_server.model.ExpenseList;
|
||||||
|
|
||||||
@Repository
|
@Repository
|
||||||
public interface ExpenseListRepository extends JpaRepository<ExpenseList, Long> {
|
public interface ExpenseListRepository extends JpaRepository<ExpenseList, Long> {
|
||||||
|
|
||||||
List<ExpenseList> findByOwnerId(Long ownerId);
|
List<ExpenseList> findByOwnerId(Long ownerId);
|
||||||
|
|
||||||
ExpenseList findByInviteCode(String inviteCode);
|
ExpenseList findByInviteCode(String inviteCode);
|
||||||
|
|
||||||
|
@Query("SELECT el FROM ExpenseList el WHERE el.owner.id = :userId OR el.sharedWith.id = :sharedUserId")
|
||||||
|
List<ExpenseList> findByOwnerIdOrSharedWithId(@Param("userId") Long userId,
|
||||||
|
@Param("sharedUserId") Long sharedUserId);
|
||||||
|
|
||||||
|
@Query("SELECT el FROM ExpenseList el WHERE el.owner.username = :username OR el.sharedWith.username = :sharedUsername")
|
||||||
|
List<ExpenseList> findByOwnerUsernameOrSharedWithUsername(@Param("username") String username,
|
||||||
|
@Param("sharedUsername") String sharedUsername);
|
||||||
}
|
}
|
||||||
@@ -7,7 +7,6 @@ import java.util.List;
|
|||||||
import java.util.Optional;
|
import java.util.Optional;
|
||||||
import java.util.UUID;
|
import java.util.UUID;
|
||||||
|
|
||||||
import org.springframework.beans.factory.annotation.Autowired;
|
|
||||||
import org.springframework.stereotype.Service;
|
import org.springframework.stereotype.Service;
|
||||||
import org.springframework.transaction.annotation.Transactional;
|
import org.springframework.transaction.annotation.Transactional;
|
||||||
|
|
||||||
@@ -18,20 +17,15 @@ import de.zendric.app.xpensely_server.model.XpenselyCustomCategory;
|
|||||||
import de.zendric.app.xpensely_server.repo.ExpenseListRepository;
|
import de.zendric.app.xpensely_server.repo.ExpenseListRepository;
|
||||||
import de.zendric.app.xpensely_server.repo.ExpenseRepository;
|
import de.zendric.app.xpensely_server.repo.ExpenseRepository;
|
||||||
import de.zendric.app.xpensely_server.repo.XpenselyCustomCategoryRepository;
|
import de.zendric.app.xpensely_server.repo.XpenselyCustomCategoryRepository;
|
||||||
import jakarta.persistence.EntityManager;
|
|
||||||
|
|
||||||
@Service
|
@Service
|
||||||
@Transactional
|
@Transactional
|
||||||
public class ExpenseListService {
|
public class ExpenseListService {
|
||||||
|
|
||||||
private ExpenseListRepository repository;
|
private final ExpenseListRepository repository;
|
||||||
private final ExpenseRepository expenseRepository;
|
private final ExpenseRepository expenseRepository;
|
||||||
private XpenselyCustomCategoryRepository customCategoryRepository;
|
private final XpenselyCustomCategoryRepository customCategoryRepository;
|
||||||
|
|
||||||
@Autowired
|
|
||||||
private EntityManager entityManager;
|
|
||||||
|
|
||||||
@Autowired
|
|
||||||
public ExpenseListService(ExpenseListRepository repository, ExpenseRepository expenseRepository,
|
public ExpenseListService(ExpenseListRepository repository, ExpenseRepository expenseRepository,
|
||||||
XpenselyCustomCategoryRepository customCategoryRepository) {
|
XpenselyCustomCategoryRepository customCategoryRepository) {
|
||||||
this.repository = repository;
|
this.repository = repository;
|
||||||
@@ -68,38 +62,11 @@ public class ExpenseListService {
|
|||||||
}
|
}
|
||||||
|
|
||||||
public List<ExpenseList> findByUserId(Long id) {
|
public List<ExpenseList> findByUserId(Long id) {
|
||||||
List<ExpenseList> allLists = repository.findAll();
|
return repository.findByOwnerIdOrSharedWithId(id, id);
|
||||||
List<ExpenseList> userSpecificList = new ArrayList<>();
|
|
||||||
for (ExpenseList expenseList : allLists) {
|
|
||||||
AppUser sharedWith = expenseList.getSharedWith();
|
|
||||||
|
|
||||||
if (expenseList.getOwner().getId().equals(id)) {
|
|
||||||
userSpecificList.add(expenseList);
|
|
||||||
} else {
|
|
||||||
if (sharedWith != null && sharedWith.getId().equals(id)) {
|
|
||||||
userSpecificList.add(expenseList);
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
return userSpecificList;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
public List<ExpenseList> findByUsername(String username) {
|
public List<ExpenseList> findByUsername(String username) {
|
||||||
List<ExpenseList> allLists = repository.findAll();
|
return repository.findByOwnerUsernameOrSharedWithUsername(username, username);
|
||||||
List<ExpenseList> userSpecificList = new ArrayList<>();
|
|
||||||
for (ExpenseList expenseList : allLists) {
|
|
||||||
AppUser sharedWith = expenseList.getSharedWith();
|
|
||||||
|
|
||||||
if (expenseList.getOwner().getUsername().equals(username)) {
|
|
||||||
userSpecificList.add(expenseList);
|
|
||||||
} else {
|
|
||||||
if (sharedWith != null && sharedWith.getUsername().equals(username)) {
|
|
||||||
userSpecificList.add(expenseList);
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
return userSpecificList;
|
|
||||||
|
|
||||||
}
|
}
|
||||||
|
|
||||||
public Expense addExpenseToList(Long expenseListId, Expense expense) {
|
public Expense addExpenseToList(Long expenseListId, Expense expense) {
|
||||||
|
|||||||
@@ -1,11 +1,11 @@
|
|||||||
package de.zendric.app.xpensely_server.services;
|
package de.zendric.app.xpensely_server.services;
|
||||||
|
|
||||||
import java.util.List;
|
import java.util.List;
|
||||||
import java.util.Optional;
|
|
||||||
|
|
||||||
import org.springframework.stereotype.Service;
|
import org.springframework.stereotype.Service;
|
||||||
|
|
||||||
import de.zendric.app.xpensely_server.model.AppUser;
|
import de.zendric.app.xpensely_server.model.AppUser;
|
||||||
|
import de.zendric.app.xpensely_server.model.Exception.ResourceNotFoundException;
|
||||||
import de.zendric.app.xpensely_server.model.Exception.UsernameAlreadyExistsException;
|
import de.zendric.app.xpensely_server.model.Exception.UsernameAlreadyExistsException;
|
||||||
import de.zendric.app.xpensely_server.repo.UserRepository;
|
import de.zendric.app.xpensely_server.repo.UserRepository;
|
||||||
|
|
||||||
@@ -29,36 +29,24 @@ public class UserService {
|
|||||||
}
|
}
|
||||||
|
|
||||||
public AppUser getUser(Long id) {
|
public AppUser getUser(Long id) {
|
||||||
Optional<AppUser> user = userRepository.findById(id);
|
return userRepository.findById(id)
|
||||||
if (user.isPresent()) {
|
.orElseThrow(() -> new ResourceNotFoundException("User not found with id: " + id));
|
||||||
return user.get();
|
|
||||||
} else
|
|
||||||
return null;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
public AppUser deleteUserById(Long id) {
|
public AppUser deleteUserById(Long id) {
|
||||||
Optional<AppUser> user = userRepository.findById(id);
|
AppUser user = userRepository.findById(id)
|
||||||
if (user.isPresent()) {
|
.orElseThrow(() -> new ResourceNotFoundException("User not found with id: " + id));
|
||||||
userRepository.deleteById(id);
|
userRepository.deleteById(id);
|
||||||
return user.get();
|
return user;
|
||||||
} else
|
|
||||||
return null;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
public AppUser getUserByName(String username) {
|
public AppUser getUserByName(String username) {
|
||||||
Optional<AppUser> optUser = userRepository.findByUsername(username);
|
return userRepository.findByUsername(username)
|
||||||
if (optUser.isPresent()) {
|
.orElseThrow(() -> new ResourceNotFoundException("User not found: " + username));
|
||||||
return optUser.get();
|
|
||||||
} else
|
|
||||||
return null;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
public AppUser getUserByGoogleId(String id) {
|
public AppUser getUserByGoogleId(String id) {
|
||||||
Optional<AppUser> optUser = userRepository.findByGoogleId(id);
|
return userRepository.findByGoogleId(id)
|
||||||
if (optUser.isPresent()) {
|
.orElseThrow(() -> new ResourceNotFoundException("User not found with Google ID: " + id));
|
||||||
return optUser.get();
|
|
||||||
} else
|
|
||||||
return null;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
}
|
}
|
||||||
@@ -0,0 +1,57 @@
|
|||||||
|
package de.zendric.app.xpensely_Server.services;
|
||||||
|
|
||||||
|
import de.zendric.app.xpensely_server.model.AppUser;
|
||||||
|
import de.zendric.app.xpensely_server.model.ExpenseList;
|
||||||
|
import de.zendric.app.xpensely_server.repo.ExpenseListRepository;
|
||||||
|
import de.zendric.app.xpensely_server.repo.ExpenseRepository;
|
||||||
|
import de.zendric.app.xpensely_server.repo.XpenselyCustomCategoryRepository;
|
||||||
|
import de.zendric.app.xpensely_server.services.ExpenseListService;
|
||||||
|
import org.junit.jupiter.api.Test;
|
||||||
|
import org.junit.jupiter.api.extension.ExtendWith;
|
||||||
|
import org.mockito.InjectMocks;
|
||||||
|
import org.mockito.Mock;
|
||||||
|
import org.mockito.junit.jupiter.MockitoExtension;
|
||||||
|
|
||||||
|
import java.util.List;
|
||||||
|
|
||||||
|
import static org.assertj.core.api.Assertions.assertThat;
|
||||||
|
import static org.mockito.Mockito.verify;
|
||||||
|
import static org.mockito.Mockito.when;
|
||||||
|
import static org.mockito.Mockito.never;
|
||||||
|
|
||||||
|
@ExtendWith(MockitoExtension.class)
|
||||||
|
class ExpenseListServiceTest {
|
||||||
|
|
||||||
|
@Mock ExpenseListRepository repository;
|
||||||
|
@Mock ExpenseRepository expenseRepository;
|
||||||
|
@Mock XpenselyCustomCategoryRepository customCategoryRepository;
|
||||||
|
|
||||||
|
@InjectMocks
|
||||||
|
ExpenseListService service;
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void findByUserId_usesRepositoryQuery_notFindAll() {
|
||||||
|
AppUser owner = new AppUser(); owner.setId(1L);
|
||||||
|
ExpenseList list = new ExpenseList(); list.setId(10L); list.setOwner(owner);
|
||||||
|
when(repository.findByOwnerIdOrSharedWithId(1L, 1L)).thenReturn(List.of(list));
|
||||||
|
|
||||||
|
List<ExpenseList> result = service.findByUserId(1L);
|
||||||
|
|
||||||
|
assertThat(result).hasSize(1);
|
||||||
|
verify(repository).findByOwnerIdOrSharedWithId(1L, 1L);
|
||||||
|
verify(repository, never()).findAll();
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void findByUsername_usesRepositoryQuery_notFindAll() {
|
||||||
|
AppUser owner = new AppUser(); owner.setId(1L); owner.setUsername("alice");
|
||||||
|
ExpenseList list = new ExpenseList(); list.setId(10L); list.setOwner(owner);
|
||||||
|
when(repository.findByOwnerUsernameOrSharedWithUsername("alice", "alice")).thenReturn(List.of(list));
|
||||||
|
|
||||||
|
List<ExpenseList> result = service.findByUsername("alice");
|
||||||
|
|
||||||
|
assertThat(result).hasSize(1);
|
||||||
|
verify(repository).findByOwnerUsernameOrSharedWithUsername("alice", "alice");
|
||||||
|
verify(repository, never()).findAll();
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -0,0 +1,75 @@
|
|||||||
|
package de.zendric.app.xpensely_Server.services;
|
||||||
|
|
||||||
|
import de.zendric.app.xpensely_server.model.AppUser;
|
||||||
|
import de.zendric.app.xpensely_server.model.Exception.ResourceNotFoundException;
|
||||||
|
import de.zendric.app.xpensely_server.repo.UserRepository;
|
||||||
|
import de.zendric.app.xpensely_server.services.UserService;
|
||||||
|
import org.junit.jupiter.api.Test;
|
||||||
|
import org.junit.jupiter.api.extension.ExtendWith;
|
||||||
|
import org.mockito.InjectMocks;
|
||||||
|
import org.mockito.Mock;
|
||||||
|
import org.mockito.junit.jupiter.MockitoExtension;
|
||||||
|
|
||||||
|
import java.util.Optional;
|
||||||
|
|
||||||
|
import static org.assertj.core.api.Assertions.assertThat;
|
||||||
|
import static org.assertj.core.api.Assertions.assertThatThrownBy;
|
||||||
|
import static org.mockito.Mockito.when;
|
||||||
|
|
||||||
|
@ExtendWith(MockitoExtension.class)
|
||||||
|
class UserServiceTest {
|
||||||
|
|
||||||
|
@Mock
|
||||||
|
UserRepository userRepository;
|
||||||
|
|
||||||
|
@InjectMocks
|
||||||
|
UserService userService;
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void getUserByName_throwsResourceNotFound_whenUserMissing() {
|
||||||
|
when(userRepository.findByUsername("ghost")).thenReturn(Optional.empty());
|
||||||
|
|
||||||
|
assertThatThrownBy(() -> userService.getUserByName("ghost"))
|
||||||
|
.isInstanceOf(ResourceNotFoundException.class)
|
||||||
|
.hasMessageContaining("ghost");
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void getUserByName_returnsUser_whenFound() {
|
||||||
|
AppUser user = new AppUser();
|
||||||
|
user.setId(1L);
|
||||||
|
user.setUsername("alice");
|
||||||
|
when(userRepository.findByUsername("alice")).thenReturn(Optional.of(user));
|
||||||
|
|
||||||
|
AppUser result = userService.getUserByName("alice");
|
||||||
|
|
||||||
|
assertThat(result.getUsername()).isEqualTo("alice");
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void getUser_throwsResourceNotFound_whenIdMissing() {
|
||||||
|
when(userRepository.findById(99L)).thenReturn(Optional.empty());
|
||||||
|
|
||||||
|
assertThatThrownBy(() -> userService.getUser(99L))
|
||||||
|
.isInstanceOf(ResourceNotFoundException.class)
|
||||||
|
.hasMessageContaining("99");
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void deleteUserById_throwsResourceNotFound_whenIdMissing() {
|
||||||
|
when(userRepository.findById(99L)).thenReturn(Optional.empty());
|
||||||
|
|
||||||
|
assertThatThrownBy(() -> userService.deleteUserById(99L))
|
||||||
|
.isInstanceOf(ResourceNotFoundException.class)
|
||||||
|
.hasMessageContaining("99");
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void getUserByGoogleId_throwsResourceNotFound_whenMissing() {
|
||||||
|
when(userRepository.findByGoogleId("gid-404")).thenReturn(Optional.empty());
|
||||||
|
|
||||||
|
assertThatThrownBy(() -> userService.getUserByGoogleId("gid-404"))
|
||||||
|
.isInstanceOf(ResourceNotFoundException.class)
|
||||||
|
.hasMessageContaining("gid-404");
|
||||||
|
}
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user