Skip to content

Commit a8588f2

Browse files
fix(server): open one transaction per delivery-policy entry point
#1517 added a rule that a @transactional method may not be reached by self-invocation, because the call never passes the proxy and the inner annotation is decorative. Both classes this branch adds broke it seven times: the convenience overloads of the coverage service and the delivery policy delegated to each other, so only the outermost annotation ever took effect. Each public entry point now opens its own read-only transaction and goes straight to a private implementation that assumes one. Behaviour is unchanged — the reads were already running inside the outer transaction — but the annotations now say what actually happens. The same merge made the two policy entry points public, which is what makes their @transactional apply at all.
1 parent 4defc29 commit a8588f2

2 files changed

Lines changed: 32 additions & 8 deletions

File tree

server/src/main/java/de/tum/cit/aet/hephaestus/agent/handler/PracticeFeedbackDeliveryPolicy.java

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ public class PracticeFeedbackDeliveryPolicy {
7878

7979
@Transactional(readOnly = true)
8080
public Decision<Issue> evaluateIssue(AgentJob job) {
81-
return evaluateIssue(job, DeliveryPolicyStage.AUTOMATIC, null);
81+
return evaluateIssue(job, DeliveryPolicyStage.AUTOMATIC, null, DeliveryPolicySurface.ARTIFACT, Set.of());
8282
}
8383

8484
@Transactional(readOnly = true)
@@ -191,11 +191,15 @@ private Decision<Issue> evaluateIssue(
191191

192192
@Transactional(readOnly = true)
193193
public Decision<PullRequest> evaluatePullRequest(AgentJob job) {
194-
return evaluatePullRequest(job, DeliveryPolicyStage.AUTOMATIC, null);
194+
return evaluatePullRequest(job, DeliveryPolicyStage.AUTOMATIC, null, DeliveryPolicySurface.ARTIFACT, Set.of());
195195
}
196196

197197
@Transactional(readOnly = true)
198-
public Decision<PullRequest> evaluatePullRequest(AgentJob job, DeliveryPolicyStage stage, @Nullable UUID feedbackId) {
198+
public Decision<PullRequest> evaluatePullRequest(
199+
AgentJob job,
200+
DeliveryPolicyStage stage,
201+
@Nullable UUID feedbackId
202+
) {
199203
return evaluatePullRequest(job, stage, feedbackId, DeliveryPolicySurface.ARTIFACT, Set.of());
200204
}
201205

server/src/main/java/de/tum/cit/aet/hephaestus/practices/review/PracticeReviewCoverageService.java

Lines changed: 25 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,10 @@ public class PracticeReviewCoverageService {
4141

4242
@Transactional(readOnly = true)
4343
public WorkspaceReviewScope scope(Workspace workspace) {
44+
return readScope(workspace);
45+
}
46+
47+
private WorkspaceReviewScope readScope(Workspace workspace) {
4448
long workspaceId = workspace.getId();
4549
List<PracticeReviewRepositoryTarget> targets = repositoryTargetRepository.findByWorkspaceId(workspaceId);
4650
Map<Long, RepositoryToMonitor> monitorsById = monitorRepository
@@ -78,7 +82,7 @@ public WorkspaceReviewScope scope(Workspace workspace) {
7882

7983
@Transactional(readOnly = true)
8084
public PracticeReviewCoverageSummaryDTO summary(Workspace workspace, int recentReviewVolume) {
81-
return summary(workspace, scope(workspace), recentReviewVolume);
85+
return summary(workspace, readScope(workspace), recentReviewVolume);
8286
}
8387

8488
@Transactional(readOnly = true)
@@ -88,7 +92,7 @@ public PracticeReviewCoveragePreviewDTO preview(
8892
int recentReviewVolume
8993
) {
9094
validate(workspace.getId(), proposed);
91-
WorkspaceReviewScope current = scope(workspace);
95+
WorkspaceReviewScope current = readScope(workspace);
9296
return new PracticeReviewCoveragePreviewDTO(
9397
summary(workspace, current, recentReviewVolume),
9498
summary(workspace, proposed, recentReviewVolume),
@@ -186,7 +190,7 @@ public boolean admits(
186190
String baseBranch,
187191
ReviewSubject subject
188192
) {
189-
return admits(workspace, repositoryNameWithOwner, baseBranch, subject, true);
193+
return readAssessment(workspace, repositoryNameWithOwner, baseBranch, subject, true).admitted();
190194
}
191195

192196
@Transactional(readOnly = true)
@@ -197,7 +201,13 @@ public boolean admits(
197201
ReviewSubject subject,
198202
boolean branchRestrictionsApply
199203
) {
200-
return assess(workspace, repositoryNameWithOwner, baseBranch, subject, branchRestrictionsApply).admitted();
204+
return readAssessment(
205+
workspace,
206+
repositoryNameWithOwner,
207+
baseBranch,
208+
subject,
209+
branchRestrictionsApply
210+
).admitted();
201211
}
202212

203213
@Transactional(readOnly = true)
@@ -208,7 +218,17 @@ public CoverageAssessment assess(
208218
ReviewSubject subject,
209219
boolean branchRestrictionsApply
210220
) {
211-
WorkspaceReviewScope scope = scope(workspace);
221+
return readAssessment(workspace, repositoryNameWithOwner, baseBranch, subject, branchRestrictionsApply);
222+
}
223+
224+
private CoverageAssessment readAssessment(
225+
Workspace workspace,
226+
String repositoryNameWithOwner,
227+
String baseBranch,
228+
ReviewSubject subject,
229+
boolean branchRestrictionsApply
230+
) {
231+
WorkspaceReviewScope scope = readScope(workspace);
212232
SubjectStatus subjectStatus;
213233
if (subject == null || subject.actorId() == null) {
214234
subjectStatus = SubjectStatus.MISSING;

0 commit comments

Comments
 (0)