Code Review: In-memory UserService: код-ревью и правки

26. Отрефакторить in-memory UserService

Условие задачи:
Дан Spring-сервис, который хранит пользователей в памяти приложения и используется одновременно из контроллеров и других сервисов.

Нужно провести code review и предложить более безопасную реализацию для работы под нагрузкой: проверить потокобезопасность, контракты методов, работу с изменяемыми объектами и ограничения in-memory хранения.

Код:

@Service
public class UserService {
    private Map<Integer, User> usersCache = new HashMap<>();

    public void addUser(User user) {
        usersCache.put(user.getId(), user);
    }

    public User getUser(int id) {
        return usersCache.get(id);
    }

    public List<User> getAllUsers() {
        return new ArrayList<>(usersCache.values());
    }
}

Спойлеры к решению

Подсказки

💡 UserService — singleton, поэтому к коллекции могут одновременно обращаться несколько потоков.
💡 Потокобезопасная коллекция ещё не делает потокобезопасными объекты, которые в ней хранятся.
💡 Определи поведение при добавлении пользователя с уже существующим id.
💡 Подумай, что произойдёт с данными после рестарта или при запуске нескольких экземпляров приложения.

Решение

Проблемы:

HashMap не потокобезопасен для конкурентного чтения и записи;
поле хранилища лучше сделать final;
нет проверки user и его id;
put() молча перезаписывает пользователя с тем же id;
getUser() возвращает null, если пользователь не найден;
если User изменяемый, внешний код может изменить объект в обход UserService;
даже с ConcurrentHashMap изменяемый User сам по себе не становится потокобезопасным;
getAllUsers() не гарантирует атомарный снимок состояния при одновременных изменениях;
данные теряются при перезапуске приложения;
в нескольких экземплярах приложения у каждого будет собственный набор пользователей.

Как лучше исправить

Если это действительно локальное in-memory хранилище, можно использовать ConcurrentHashMap и явно определить поведение при дубликатах:

@Service
public class UserService {

    private final ConcurrentMap<Integer, User> users =
            new ConcurrentHashMap<>();

    public User create(User user) {
        validate(user);

        User previous = users.putIfAbsent(
                user.getId(),
                user
        );

        if (previous != null) {
            throw new IllegalStateException(
                    "User already exists: " + user.getId()
            );
        }

        return user;
    }

    public Optional<User> getUser(int id) {
        return Optional.ofNullable(users.get(id));
    }

    public List<User> getAllUsers() {
        return List.copyOf(users.values());
    }

    public boolean deleteUser(int id) {
        return users.remove(id) != null;
    }

    private void validate(User user) {
        if (user == null || user.getId() == null) {
            throw new IllegalArgumentException(
                    "User and user.id must not be null"
            );
        }
    }
}

List.copyOf() защищает сам возвращаемый список от изменений, но не делает элементы внутри него immutable. Поэтому для безопасного API лучше хранить неизменяемые объекты или возвращать DTO-копии.

Также важно понимать границы решения: ConcurrentHashMap подходит для локального кэша или временного хранилища, но не для постоянного хранения пользователей. При рестарте данные исчезнут, а при нескольких pod’ах состояние будет различаться. Для общего состояния нужен внешний storage, например БД или распределённый кэш.