refactor(community,tariff): fix code review findings - redundant DB queries, inconsistent imports, overly broad cascade

- TariffService: extract ValidatedUserTariffPoints record from validateUserTariffRequest to eliminate redundant MeteringPoint loads in createUserTariff/updateUserTariff (saves 2 SELECT queries per create call)
- TariffService: move source != target check into validateUserTariffRequest for single-responsibility
- MeteringPoint: restrict CascadeType from ALL to PERSIST/MERGE and remove orphanRemoval to prevent accidental cascade deletes of independent Memberships
- MeteringPointService, MembershipService, MeteringDataService, TariffInviteService, TariffService: replace FQ org.springframework.security.access.AccessDeniedException with imported short form for consistency
This commit is contained in:
Bernhard Müller 2026-07-24 10:24:38 +02:00
parent 1b66b25ada
commit 2bb0f7114b
6 changed files with 27 additions and 32 deletions

View File

@ -48,7 +48,7 @@ public class MeteringPoint {
@Column(nullable = false) @Column(nullable = false)
private MakoState makoState = MakoState.NEW; private MakoState makoState = MakoState.NEW;
@OneToMany(mappedBy = "meteringPoint", cascade = CascadeType.ALL, orphanRemoval = true) @OneToMany(mappedBy = "meteringPoint", cascade = {CascadeType.PERSIST, CascadeType.MERGE})
private List<Membership> memberships; private List<Membership> memberships;
public void fireTrigger(MakoTrigger trigger) { public void fireTrigger(MakoTrigger trigger) {

View File

@ -16,6 +16,7 @@ import at.mueller.eeg.backend.iam.domain.User;
import at.mueller.eeg.backend.iam.repository.UserRepository; import at.mueller.eeg.backend.iam.repository.UserRepository;
import lombok.RequiredArgsConstructor; import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j; import lombok.extern.slf4j.Slf4j;
import org.springframework.security.access.AccessDeniedException;
import org.springframework.stereotype.Service; import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional; import org.springframework.transaction.annotation.Transactional;
@ -59,7 +60,7 @@ public class MembershipService {
.orElseThrow(() -> new IllegalArgumentException("Zählpunkt nicht gefunden")); .orElseThrow(() -> new IllegalArgumentException("Zählpunkt nicht gefunden"));
if (!point.getUserId().equals(userId)) { if (!point.getUserId().equals(userId)) {
throw new org.springframework.security.access.AccessDeniedException("Keine Berechtigung für diesen Zählpunkt."); throw new AccessDeniedException("Keine Berechtigung für diesen Zählpunkt.");
} }
EnergyCommunity community = energyCommunityRepository.findById(request.energyCommunityId()) EnergyCommunity community = energyCommunityRepository.findById(request.energyCommunityId())

View File

@ -11,6 +11,7 @@ import at.mueller.eeg.backend.community.repository.MeteringDataUploadRepository;
import at.mueller.eeg.backend.community.repository.MeteringPointRepository; import at.mueller.eeg.backend.community.repository.MeteringPointRepository;
import jakarta.persistence.EntityManager; import jakarta.persistence.EntityManager;
import lombok.RequiredArgsConstructor; import lombok.RequiredArgsConstructor;
import org.springframework.security.access.AccessDeniedException;
import org.springframework.stereotype.Service; import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional; import org.springframework.transaction.annotation.Transactional;
@ -113,7 +114,7 @@ public class MeteringDataService {
private void verifyOwnership(UUID userId, UUID meteringPointId) { private void verifyOwnership(UUID userId, UUID meteringPointId) {
meteringPointRepository.findById(meteringPointId) meteringPointRepository.findById(meteringPointId)
.filter(mp -> mp.getUserId().equals(userId)) .filter(mp -> mp.getUserId().equals(userId))
.orElseThrow(() -> new org.springframework.security.access.AccessDeniedException( .orElseThrow(() -> new AccessDeniedException(
"Keine Berechtigung zum Zugriff auf diesen Zählpunkt")); "Keine Berechtigung zum Zugriff auf diesen Zählpunkt"));
} }

View File

@ -14,6 +14,7 @@ import at.mueller.eeg.backend.iam.domain.User;
import at.mueller.eeg.backend.iam.repository.UserRepository; import at.mueller.eeg.backend.iam.repository.UserRepository;
import lombok.RequiredArgsConstructor; import lombok.RequiredArgsConstructor;
import org.springframework.context.ApplicationEventPublisher; import org.springframework.context.ApplicationEventPublisher;
import org.springframework.security.access.AccessDeniedException;
import org.springframework.stereotype.Service; import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional; import org.springframework.transaction.annotation.Transactional;
@ -76,7 +77,7 @@ public class MeteringPointService {
.orElseThrow(() -> new IllegalArgumentException("Zählpunkt nicht gefunden")); .orElseThrow(() -> new IllegalArgumentException("Zählpunkt nicht gefunden"));
if (!point.getUserId().equals(userId)) { if (!point.getUserId().equals(userId)) {
throw new org.springframework.security.access.AccessDeniedException("Keine Berechtigung zum Bearbeiten dieses Zählpunkts"); throw new AccessDeniedException("Keine Berechtigung zum Bearbeiten dieses Zählpunkts");
} }
if (point.getMakoState() != MakoState.NEW && point.getMakoState() != MakoState.REJECTED && point.getMakoState() != MakoState.ERROR) { if (point.getMakoState() != MakoState.NEW && point.getMakoState() != MakoState.REJECTED && point.getMakoState() != MakoState.ERROR) {
@ -107,7 +108,7 @@ public class MeteringPointService {
.orElseThrow(() -> new IllegalArgumentException("Zählpunkt nicht gefunden")); .orElseThrow(() -> new IllegalArgumentException("Zählpunkt nicht gefunden"));
if (!point.getUserId().equals(userId)) { if (!point.getUserId().equals(userId)) {
throw new org.springframework.security.access.AccessDeniedException("Keine Berechtigung zum Löschen dieses Zählpunkts"); throw new AccessDeniedException("Keine Berechtigung zum Löschen dieses Zählpunkts");
} }
if (point.getMakoState() == MakoState.ACTIVE) { if (point.getMakoState() == MakoState.ACTIVE) {

View File

@ -9,6 +9,7 @@ import at.mueller.eeg.backend.tariff.domain.TariffInvite;
import at.mueller.eeg.backend.tariff.repository.TariffInviteRepository; import at.mueller.eeg.backend.tariff.repository.TariffInviteRepository;
import lombok.RequiredArgsConstructor; import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j; import lombok.extern.slf4j.Slf4j;
import org.springframework.security.access.AccessDeniedException;
import org.springframework.stereotype.Service; import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional; import org.springframework.transaction.annotation.Transactional;
@ -78,7 +79,7 @@ public class TariffInviteService {
.orElseThrow(() -> new IllegalArgumentException("Einladung nicht gefunden: " + inviteId)); .orElseThrow(() -> new IllegalArgumentException("Einladung nicht gefunden: " + inviteId));
if (!invite.getConsumerUserId().equals(consumerUserId)) { if (!invite.getConsumerUserId().equals(consumerUserId)) {
throw new org.springframework.security.access.AccessDeniedException( throw new AccessDeniedException(
"Keine Berechtigung zum Annehmen dieser Einladung."); "Keine Berechtigung zum Annehmen dieser Einladung.");
} }
if (invite.getStatus() != InviteStatus.PENDING) { if (invite.getStatus() != InviteStatus.PENDING) {

View File

@ -20,6 +20,7 @@ import at.mueller.eeg.backend.tariff.repository.UserTariffRepository;
import lombok.RequiredArgsConstructor; import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j; import lombok.extern.slf4j.Slf4j;
import org.springframework.context.ApplicationEventPublisher; import org.springframework.context.ApplicationEventPublisher;
import org.springframework.security.access.AccessDeniedException;
import org.springframework.stereotype.Service; import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional; import org.springframework.transaction.annotation.Transactional;
@ -73,10 +74,6 @@ public class TariffService {
@Transactional @Transactional
public UserTariffResponse createUserTariff(UUID userId, UserTariffRequest request) { public UserTariffResponse createUserTariff(UUID userId, UserTariffRequest request) {
if (request.sourceMeteringPointId().equals(request.targetMeteringPointId())) {
throw new IllegalStateException("Quell- und Ziel-Zählpunkt müssen unterschiedlich sein.");
}
userTariffRepository.findBySourceMeteringPointIdAndTargetMeteringPointId( userTariffRepository.findBySourceMeteringPointIdAndTargetMeteringPointId(
request.sourceMeteringPointId(), request.targetMeteringPointId()) request.sourceMeteringPointId(), request.targetMeteringPointId())
.ifPresent(existing -> { .ifPresent(existing -> {
@ -84,17 +81,10 @@ public class TariffService {
"Es existiert bereits ein Tarif zwischen diesen beiden Zählpunkten."); "Es existiert bereits ein Tarif zwischen diesen beiden Zählpunkten.");
}); });
validateUserTariffRequest(request.energyCommunityId(), request); ValidatedUserTariffPoints validated = validateUserTariffRequest(request.energyCommunityId(), request);
MeteringPoint sourcePoint = meteringPointRepository.findById(request.sourceMeteringPointId())
.orElseThrow(() -> new IllegalArgumentException(
"Quell-Zählpunkt nicht gefunden: " + request.sourceMeteringPointId()));
MeteringPoint targetPoint = meteringPointRepository.findById(request.targetMeteringPointId())
.orElseThrow(() -> new IllegalArgumentException(
"Ziel-Zählpunkt nicht gefunden: " + request.targetMeteringPointId()));
if (!tariffInviteRepository.existsByProducerUserIdAndConsumerUserIdAndEnergyCommunityIdAndStatus( if (!tariffInviteRepository.existsByProducerUserIdAndConsumerUserIdAndEnergyCommunityIdAndStatus(
sourcePoint.getUserId(), targetPoint.getUserId(), request.energyCommunityId(), InviteStatus.ACCEPTED)) { validated.source().getUserId(), validated.target().getUserId(), request.energyCommunityId(), InviteStatus.ACCEPTED)) {
throw new IllegalStateException( throw new IllegalStateException(
"Zwischen Produzent und Konsument muss eine angenommene Einladung bestehen."); "Zwischen Produzent und Konsument muss eine angenommene Einladung bestehen.");
} }
@ -113,7 +103,7 @@ public class TariffService {
request.targetMeteringPointId(), request.pricePerKwhCents()); request.targetMeteringPointId(), request.pricePerKwhCents());
eventPublisher.publishEvent(new UserTariffChangedEvent( eventPublisher.publishEvent(new UserTariffChangedEvent(
request.energyCommunityId(), sourcePoint.getUserId(), targetPoint.getUserId(), "CREATED")); request.energyCommunityId(), validated.source().getUserId(), validated.target().getUserId(), "CREATED"));
return toUserTariffResponse(saved); return toUserTariffResponse(saved);
} }
@ -127,18 +117,11 @@ public class TariffService {
.orElseThrow(() -> new IllegalArgumentException("Quell-Zählpunkt nicht gefunden.")); .orElseThrow(() -> new IllegalArgumentException("Quell-Zählpunkt nicht gefunden."));
if (!sourcePoint.getUserId().equals(userId)) { if (!sourcePoint.getUserId().equals(userId)) {
throw new org.springframework.security.access.AccessDeniedException( throw new AccessDeniedException(
"Keine Berechtigung zum Bearbeiten dieses Tarifs."); "Keine Berechtigung zum Bearbeiten dieses Tarifs.");
} }
if (request.sourceMeteringPointId().equals(request.targetMeteringPointId())) { ValidatedUserTariffPoints validated = validateUserTariffRequest(request.energyCommunityId(), request);
throw new IllegalStateException("Quell- und Ziel-Zählpunkt müssen unterschiedlich sein.");
}
validateUserTariffRequest(request.energyCommunityId(), request);
MeteringPoint newTargetPoint = meteringPointRepository.findById(request.targetMeteringPointId())
.orElseThrow(() -> new IllegalArgumentException("Ziel-Zählpunkt nicht gefunden."));
tariff.setEnergyCommunityId(request.energyCommunityId()); tariff.setEnergyCommunityId(request.energyCommunityId());
tariff.setSourceMeteringPointId(request.sourceMeteringPointId()); tariff.setSourceMeteringPointId(request.sourceMeteringPointId());
@ -152,7 +135,7 @@ public class TariffService {
tariffId, request.pricePerKwhCents()); tariffId, request.pricePerKwhCents());
eventPublisher.publishEvent(new UserTariffChangedEvent( eventPublisher.publishEvent(new UserTariffChangedEvent(
request.energyCommunityId(), sourcePoint.getUserId(), newTargetPoint.getUserId(), "UPDATED")); request.energyCommunityId(), sourcePoint.getUserId(), validated.target().getUserId(), "UPDATED"));
return toUserTariffResponse(saved); return toUserTariffResponse(saved);
} }
@ -166,7 +149,7 @@ public class TariffService {
.orElseThrow(() -> new IllegalArgumentException("Quell-Zählpunkt nicht gefunden.")); .orElseThrow(() -> new IllegalArgumentException("Quell-Zählpunkt nicht gefunden."));
if (!sourcePoint.getUserId().equals(userId)) { if (!sourcePoint.getUserId().equals(userId)) {
throw new org.springframework.security.access.AccessDeniedException( throw new AccessDeniedException(
"Keine Berechtigung zum Löschen dieses Tarifs."); "Keine Berechtigung zum Löschen dieses Tarifs.");
} }
@ -214,7 +197,13 @@ public class TariffService {
); );
} }
private void validateUserTariffRequest(UUID communityId, UserTariffRequest request) { record ValidatedUserTariffPoints(MeteringPoint source, MeteringPoint target) {}
private ValidatedUserTariffPoints validateUserTariffRequest(UUID communityId, UserTariffRequest request) {
if (request.sourceMeteringPointId().equals(request.targetMeteringPointId())) {
throw new IllegalStateException("Quell- und Ziel-Zählpunkt müssen unterschiedlich sein.");
}
MeteringPoint sourcePoint = meteringPointRepository.findById(request.sourceMeteringPointId()) MeteringPoint sourcePoint = meteringPointRepository.findById(request.sourceMeteringPointId())
.orElseThrow(() -> new IllegalArgumentException( .orElseThrow(() -> new IllegalArgumentException(
"Quell-Zählpunkt nicht gefunden: " + request.sourceMeteringPointId())); "Quell-Zählpunkt nicht gefunden: " + request.sourceMeteringPointId()));
@ -248,5 +237,7 @@ public class TariffService {
if (!membershipRepository.isActiveMemberOfCommunity(targetUserId, communityId)) { if (!membershipRepository.isActiveMemberOfCommunity(targetUserId, communityId)) {
throw new IllegalStateException("Ziel-Benutzer ist kein Mitglied der Energiegemeinschaft."); throw new IllegalStateException("Ziel-Benutzer ist kein Mitglied der Energiegemeinschaft.");
} }
return new ValidatedUserTariffPoints(sourcePoint, targetPoint);
} }
} }