mirror of
https://github.com/Stirling-Tools/Stirling-PDF.git
synced 2026-09-03 05:10:16 +03:00
Report editor-originated failures into the same queue (Review Flow PR 2) (#7296)
Review Flow PR 2 of 5. Editor tool failures now reach the same durable queue as failures from folders, buckets and webhooks. ## What's added **A report endpoint** — `POST /api/v1/file-run-events/reports`, open to any authenticated user. Takes four fields: `operation`, `errorCode`, `fileIds`, `detail`. No team, no actor, no filename: the first two come from the session, the third is never a field. Refused with 400 above 200 file ids, and nothing is written when refused. **Automatic reporting from every tool** — wired into `useToolOperation`, so no per-tool work is needed. Client-side refusals (an unsupported format that never reaches the server) are reported too. User cancellations are not. **Error codes parsed from Blob bodies as well as JSON** — a download-typed tool call fails with a Blob, so `errorCodeOf` handles both shapes. **Source attribution for unattended runs** — `sourceId` is threaded from `PolicyRunner` through `PolicyRun` to the recorded row and out to the wire, so a folder, bucket or webhook failure names what fed it. Previously it had none. **Deleting a file closes its failures** — `FileContext.removeFiles` notifies `POST /removed-files`, which transitions those incidents to `FILE_REMOVED`. Terminal, so they leave every reviewer's queue. The rows stay for audit. **The queue can be emptied** — reads now default to open statuses only; ask for a status explicitly to see closed rows. ## Behaviour changes - **Editor failures dedup per person.** `RecordFailure.scopeRef()` includes the actor for TOOL-origin rows, so two people hitting the same failure on the same file are two incidents rather than one. Processor rows are unaffected and their dedup key is byte-identical to before. - **`UNKNOWN` offers only Dismiss.** Acknowledge is no longer offered on it. - **Background reports no longer raise a toast.** Both calls pass `suppressErrorToast`, so a failed report is silent as intended; previously a core build showed the user a "Not Found" toast on every tool failure. ## What is stored File ids only, never names. The request type has no filename field, and a `fileNames` value handed to the client reporter is accepted and ignored. One caveat to review deliberately: the free-text `detail` is stored **verbatim**. `RecordFailure` truncates it at 2000 characters and nothing else; the redaction that used to strip name-shaped text was reverted in `024899f3f6` because it made an unclassified failure impossible to act on. A backend message that embeds a filename (LibreOffice conversion errors, IO errors) will therefore persist that text and show it to a team leader. ## How to test Needs a proprietary or SaaS build with login enabled. `task dev:all` gives you one. 1. **Report a failure from a tool.** Open a PDF, run **Remove Password** on it with a wrong password. Nothing visible changes for you: reporting is silent by design. 2. **See it recorded.** Go to `/processor/documents` and scroll to **Failures** (dev builds only). A row appears titled "Password-protected document", with `Hit by <your user>`. Press **Show raw JSON** to see exactly what was stored. 3. **Confirm no filename is stored as data.** In that JSON, `fileId` is an opaque uuid and there is no name field. Note the `detail` string may contain a filename if the backend put one in its message, per the caveat above. 4. **Confirm the request is capped.** In DevTools, POST to `/api/v1/file-run-events/reports` with 201 entries in `fileIds`. It returns 400 naming the limit, and no rows are added. 5. **Deleting a file clears its failure.** Back in the editor, delete the file you just failed on. Refresh the failures list: its row is gone from the default view. Filter by `FILE_REMOVED` to see it still exists. 6. **Two people, two incidents.** Have a colleague fail the same tool on their own copy of the same file. Two rows, not one occurrence count. ## Migration `source_id` is a new column and `FILE_REMOVED` a new status value. Both are already in the SaaS migration ([Stirling-PDF-SaaS #322](https://github.com/Stirling-Tools/Stirling-PDF-SaaS/pull/322)); self-hosted picks them up from `ddl-auto`.
This commit is contained in:
+47
@@ -0,0 +1,47 @@
|
||||
package stirling.software.proprietary.failure;
|
||||
|
||||
import java.util.List;
|
||||
|
||||
/**
|
||||
* A failure a user hit in the editor, reported by their own client. The editor calls tools directly
|
||||
* rather than through the policy engine, so nothing server-side sees these unless the client says
|
||||
* so.
|
||||
*
|
||||
* <p>Note what the client cannot supply: no team, no actor, no document name. The first two come
|
||||
* from the authenticated session, and the third is never stored.
|
||||
*
|
||||
* @param operation the tool that failed, e.g. {@code remove-password}
|
||||
* @param errorCode the code from the tool's Problem Details response, or null when there was none
|
||||
* @param fileIds opaque client-side ids of the documents involved; empty when none is attributable
|
||||
* @param detail the message the user saw
|
||||
*/
|
||||
public record EditorFailureReport(
|
||||
String operation, String errorCode, List<String> fileIds, String detail) {
|
||||
|
||||
/**
|
||||
* Cap on the files one report may name. Each one becomes a permanent incident and this endpoint
|
||||
* is open to any authenticated user, so without a bound a single call can flood a leader's
|
||||
* queue. 200 is several times the largest batch an editor session plausibly fails on, and the
|
||||
* most the review queue shows in one page, so a real report never meets it.
|
||||
*
|
||||
* <p>An oversized report is refused whole rather than trimmed: see {@link
|
||||
* FileRunEventController#report}.
|
||||
*/
|
||||
static final int MAX_FILE_IDS = 200;
|
||||
|
||||
public EditorFailureReport {
|
||||
fileIds = fileIds == null ? List.of() : List.copyOf(fileIds);
|
||||
}
|
||||
|
||||
boolean hasOperation() {
|
||||
return operation != null && !operation.isBlank();
|
||||
}
|
||||
|
||||
/**
|
||||
* Counted before the blank ids are dropped, because this bounds the request rather than the
|
||||
* rows it would produce.
|
||||
*/
|
||||
boolean namesTooManyFiles() {
|
||||
return fileIds.size() > MAX_FILE_IDS;
|
||||
}
|
||||
}
|
||||
+4
-1
@@ -23,6 +23,10 @@ import lombok.Getter;
|
||||
* <p>Actions are declared here but implemented in {@link FailureAction} beans resolved by id, so a
|
||||
* new kind ships as a registry entry plus copy. Two members today: {@link #UNKNOWN} gives every
|
||||
* failed run a record, and kinds get promoted out of it as production shows what occurs.
|
||||
*
|
||||
* <p>A kind offers an acknowledgement only where there is something to acknowledge <em>doing</em>.
|
||||
* With nothing to fix, "seen it" and "clear it" are the same decision, so the row offers only the
|
||||
* one that clears it.
|
||||
*/
|
||||
@Getter
|
||||
public enum FailureKind {
|
||||
@@ -43,7 +47,6 @@ public enum FailureKind {
|
||||
FailureScope.RUN,
|
||||
noErrorCodes(),
|
||||
fallback("This run failed for a reason Stirling does not yet recognise."),
|
||||
offer(ACKNOWLEDGE),
|
||||
offer(DISMISS));
|
||||
|
||||
private static final String KEY_PREFIX = "portal.failures.kind.";
|
||||
|
||||
@@ -20,6 +20,7 @@ public record FileRunEvent(
|
||||
FailureOrigin origin,
|
||||
String policyId,
|
||||
String runId,
|
||||
String sourceId,
|
||||
String fileId,
|
||||
String detail,
|
||||
String dedupKey,
|
||||
@@ -46,6 +47,7 @@ public record FileRunEvent(
|
||||
entity.getOrigin(),
|
||||
entity.getPolicyId(),
|
||||
entity.getRunId(),
|
||||
entity.getSourceId(),
|
||||
entity.getFileId(),
|
||||
entity.getDetail(),
|
||||
entity.getDedupKey(),
|
||||
|
||||
+61
@@ -5,6 +5,7 @@ import java.util.List;
|
||||
import java.util.Map;
|
||||
|
||||
import org.springframework.http.HttpStatus;
|
||||
import org.springframework.http.ResponseEntity;
|
||||
import org.springframework.web.bind.annotation.GetMapping;
|
||||
import org.springframework.web.bind.annotation.PathVariable;
|
||||
import org.springframework.web.bind.annotation.PostMapping;
|
||||
@@ -91,6 +92,50 @@ public class FileRunEventController {
|
||||
}
|
||||
}
|
||||
|
||||
@PostMapping("/reports")
|
||||
@Operation(
|
||||
summary = "Report a failure hit in the editor",
|
||||
description =
|
||||
"For failures the server never sees, because the editor calls tools directly."
|
||||
+ " Open to any authenticated user, unlike the read and triage endpoints:"
|
||||
+ " whoever's work failed can say so, and a leader reviews it. Rejected"
|
||||
+ " with 400 if it names more files than one report may carry.")
|
||||
public ResponseEntity<Void> report(@RequestBody EditorFailureReport report) {
|
||||
if (report == null || !report.hasOperation()) {
|
||||
throw new ResponseStatusException(
|
||||
HttpStatus.BAD_REQUEST, "operation is required to report a failure");
|
||||
}
|
||||
// Refused whole rather than trimmed, and refused before the first write, so an oversized
|
||||
// report leaves no rows at all. Trimming would hand a reviewer part of a set with nothing
|
||||
// saying the rest existed, which is what the cap inside the service used to do. The limit
|
||||
// is stated in the message because the editor reports in the background: a client author
|
||||
// reading a log is the only person who will ever see this.
|
||||
if (report.namesTooManyFiles()) {
|
||||
throw new ResponseStatusException(
|
||||
HttpStatus.BAD_REQUEST,
|
||||
"a report may name at most "
|
||||
+ EditorFailureReport.MAX_FILE_IDS
|
||||
+ " files, and this one named "
|
||||
+ report.fileIds().size());
|
||||
}
|
||||
service.report(report);
|
||||
// No body: the editor reports and moves on, and has nothing to do with the row.
|
||||
return ResponseEntity.noContent().build();
|
||||
}
|
||||
|
||||
@PostMapping("/removed-files")
|
||||
@Operation(
|
||||
summary = "Close the incidents about files deleted from the editor",
|
||||
description =
|
||||
"Deleting the document leaves nothing to act on, so its incidents drop out of"
|
||||
+ " the queue while the rows stay for audit. Open to any authenticated"
|
||||
+ " user, and applies only to their own editor rows.")
|
||||
public ResponseEntity<Void> filesRemoved(@RequestBody(required = false) RemovedFiles request) {
|
||||
service.forgetFiles(request == null ? List.of() : request.safeFileIds());
|
||||
// No body: the editor is telling the server, not asking it anything.
|
||||
return ResponseEntity.noContent().build();
|
||||
}
|
||||
|
||||
@GetMapping("/kinds")
|
||||
@Operation(
|
||||
summary = "List known failure kinds",
|
||||
@@ -136,6 +181,22 @@ public class FileRunEventController {
|
||||
/** Wrapped rather than a bare array so pagination can be added without breaking clients. */
|
||||
public record FileRunEventsResponse(List<FileRunEventView> events) {}
|
||||
|
||||
/**
|
||||
* Files gone from the caller's editor. Opaque ids only, as everywhere else on this API.
|
||||
*
|
||||
* <p>Deliberately uncapped where a report is capped, because this creates nothing: it closes
|
||||
* rows the caller already owns, so however long the list is, it can only ever touch incidents
|
||||
* that already exist. Refusing an oversized one would also be the harmful direction here, since
|
||||
* the editor says this once and never retries: those incidents would sit in the queue asking
|
||||
* for attention about files that no longer exist.
|
||||
*/
|
||||
public record RemovedFiles(List<String> fileIds) {
|
||||
|
||||
List<String> safeFileIds() {
|
||||
return fileIds == null ? List.of() : fileIds;
|
||||
}
|
||||
}
|
||||
|
||||
/** Inputs an action declared it needs. Empty for both actions that exist today. */
|
||||
public record ActionRequest(Map<String, String> inputs) {
|
||||
|
||||
|
||||
+4
@@ -84,6 +84,10 @@ public class FileRunEventEntity implements Serializable {
|
||||
@Column(name = "run_id")
|
||||
private String runId;
|
||||
|
||||
/** Which folder, bucket or webhook fed the run. Null when a user supplied the file. */
|
||||
@Column(name = "source_id")
|
||||
private String sourceId;
|
||||
|
||||
@Column(name = "file_id")
|
||||
private String fileId;
|
||||
|
||||
|
||||
+36
-9
@@ -17,19 +17,21 @@ import org.springframework.transaction.annotation.Transactional;
|
||||
public interface FileRunEventRepository extends JpaRepository<FileRunEventEntity, String> {
|
||||
|
||||
/**
|
||||
* This team's events, newest first, scoped in the query rather than loaded and filtered. A
|
||||
* {@code null} teamId matches the rows with no team (login disabled), mirroring {@link
|
||||
* stirling.software.proprietary.policy.source.SourceRepository#findByTeam}, since a plain
|
||||
* {@code = null} would return nothing.
|
||||
* As {@link #findByTeamAndStatus} but for a set of statuses, e.g. the open ones. The kind
|
||||
* filter is in the query, before the limit: filtering an already-limited page could return
|
||||
* nothing while matching rows exist.
|
||||
*/
|
||||
@Query(
|
||||
"select e from FileRunEventEntity e where ((:teamId is null and e.teamId is null) or"
|
||||
+ " e.teamId = :teamId) and (:kindId is null or e.kindId = :kindId)"
|
||||
+ " order by e.lastSeenAt desc")
|
||||
List<FileRunEventEntity> findByTeam(
|
||||
@Param("teamId") Long teamId, @Param("kindId") String kindId, Pageable pageable);
|
||||
+ " e.teamId = :teamId) and e.status in :statuses"
|
||||
+ " and (:kindId is null or e.kindId = :kindId) order by e.lastSeenAt desc")
|
||||
List<FileRunEventEntity> findByTeamAndStatusIn(
|
||||
@Param("teamId") Long teamId,
|
||||
@Param("statuses") List<FileRunEventStatus> statuses,
|
||||
@Param("kindId") String kindId,
|
||||
Pageable pageable);
|
||||
|
||||
/** As {@link #findByTeam} but restricted to one status, for the review surface's filters. */
|
||||
/** As {@link #findByTeamAndStatusIn} but for exactly one status, for the surface's filters. */
|
||||
@Query(
|
||||
"select e from FileRunEventEntity e where ((:teamId is null and e.teamId is null) or"
|
||||
+ " e.teamId = :teamId) and e.status = :status"
|
||||
@@ -85,6 +87,31 @@ public interface FileRunEventRepository extends JpaRepository<FileRunEventEntity
|
||||
@Param("now") Instant now,
|
||||
@Param("allowedFrom") Collection<FileRunEventStatus> allowedFrom);
|
||||
|
||||
/**
|
||||
* Close the incidents about documents their owner deleted from the editor: the queue is what
|
||||
* needs attention, and a document that no longer exists needs none.
|
||||
*
|
||||
* <p>Restricted to that owner's own editor rows. File ids are minted by the client, so scoping
|
||||
* on team alone would let one caller close a colleague's incidents by naming ids. Processor
|
||||
* rows are excluded outright: nothing was deleted from an editor there.
|
||||
*/
|
||||
@Modifying(clearAutomatically = true)
|
||||
@Transactional
|
||||
@Query(
|
||||
"update FileRunEventEntity e set e.status ="
|
||||
+ " stirling.software.proprietary.failure.FileRunEventStatus.FILE_REMOVED,"
|
||||
+ " e.statusActor = :actor, e.statusAt = :now where e.origin ="
|
||||
+ " stirling.software.proprietary.failure.FailureOrigin.TOOL and ((:teamId is"
|
||||
+ " null and e.teamId is null) or e.teamId = :teamId) and ((:actor is null and"
|
||||
+ " e.actor is null) or e.actor = :actor) and e.fileId in :fileIds and e.status in"
|
||||
+ " :allowedFrom")
|
||||
int markFilesRemoved(
|
||||
@Param("teamId") Long teamId,
|
||||
@Param("actor") String actor,
|
||||
@Param("fileIds") Collection<String> fileIds,
|
||||
@Param("now") Instant now,
|
||||
@Param("allowedFrom") Collection<FileRunEventStatus> allowedFrom);
|
||||
|
||||
/**
|
||||
* The most recent row for this exact failure, so the rollup can increment an existing incident
|
||||
* instead of opening a new one. Team-scoped, so the same failure in two teams stays two rows.
|
||||
|
||||
+61
@@ -31,6 +31,67 @@ public class FileRunEventService {
|
||||
private final UserServiceInterface userService;
|
||||
private final ApplicationProperties applicationProperties;
|
||||
|
||||
/**
|
||||
* Record a failure a user hit in the editor. One incident per named file, so each document
|
||||
* stays separately actionable; one unattributed incident when the report names none.
|
||||
*
|
||||
* <p>The team and actor come from the session rather than the report, and the kind is
|
||||
* classified from the reported code, falling back to {@link FailureKind#UNKNOWN} for a code no
|
||||
* kind claims.
|
||||
*/
|
||||
public List<FileRunEvent> report(EditorFailureReport report) {
|
||||
FailureKind kind = FailureKind.byErrorCode(report.errorCode()).orElse(FailureKind.UNKNOWN);
|
||||
Long teamId = scope().teamId();
|
||||
String actor = currentActor();
|
||||
String detail = detailFor(report);
|
||||
|
||||
List<String> fileIds =
|
||||
report.fileIds().stream().filter(id -> id != null && !id.isBlank()).toList();
|
||||
if (fileIds.isEmpty()) {
|
||||
return List.of(recordReported(kind, teamId, actor, null, detail));
|
||||
}
|
||||
// Every named file gets its row. An earlier cap here silently dropped the rest, which lost
|
||||
// failures a reviewer needed and was inconsistent with the processor path, where a sweep
|
||||
// records one row per failing file with no limit at all. How many files one report may name
|
||||
// is bounded at the boundary instead (see EditorFailureReport#MAX_FILE_IDS), where an
|
||||
// oversized report can be refused whole before anything is written.
|
||||
return fileIds.stream()
|
||||
.map(fileId -> recordReported(kind, teamId, actor, fileId, detail))
|
||||
.toList();
|
||||
}
|
||||
|
||||
private FileRunEvent recordReported(
|
||||
FailureKind kind, Long teamId, String actor, String fileId, String detail) {
|
||||
return store.record(RecordFailure.forEditor(kind, teamId, actor, fileId, detail));
|
||||
}
|
||||
|
||||
/**
|
||||
* The operation is the context a reviewer needs, since an editor failure has no policy or run.
|
||||
*/
|
||||
private String detailFor(EditorFailureReport report) {
|
||||
String message = report.detail() == null ? "" : report.detail();
|
||||
return message.isBlank() ? report.operation() : report.operation() + ": " + message;
|
||||
}
|
||||
|
||||
/**
|
||||
* Close the incidents about documents the caller has deleted from their editor. The queue means
|
||||
* "needs attention", and a document that no longer exists needs none; the rows stay for audit.
|
||||
*
|
||||
* <p>Best-effort by nature: this only arrives if the browser that owns the file says so, and a
|
||||
* cleared cache or another device never will. Rows left open that way are retention's problem,
|
||||
* not this method's.
|
||||
*
|
||||
* @return how many incidents were closed
|
||||
*/
|
||||
public int forgetFiles(List<String> fileIds) {
|
||||
TeamScope scope = scope();
|
||||
if (!scope.permitted()) {
|
||||
return 0;
|
||||
}
|
||||
List<String> named = fileIds.stream().filter(id -> id != null && !id.isBlank()).toList();
|
||||
return store.markFilesRemoved(scope.teamId(), currentActor(), named);
|
||||
}
|
||||
|
||||
/** The calling user's events, newest first. Empty when their team cannot be resolved. */
|
||||
public List<FileRunEvent> list(FileRunEventStatus status, String kindId, int limit) {
|
||||
TeamScope scope = scope();
|
||||
|
||||
+8
-1
@@ -11,7 +11,14 @@ public enum FileRunEventStatus {
|
||||
NEW(false),
|
||||
ACKNOWLEDGED(false),
|
||||
DISMISSED(true),
|
||||
RESOLVED(true);
|
||||
RESOLVED(true),
|
||||
|
||||
/**
|
||||
* The document this incident was about was deleted from its owner's editor, so there is nothing
|
||||
* left to act on. Distinct from {@code DISMISSED}, which is a reviewer's decision, and from
|
||||
* {@code RESOLVED}, which reopens on recurrence: this one cannot recur, the file is gone.
|
||||
*/
|
||||
FILE_REMOVED(true);
|
||||
|
||||
/** The statuses a review queue shows by default: everything still needing a decision. */
|
||||
private static final List<FileRunEventStatus> OPEN =
|
||||
|
||||
+27
-4
@@ -98,6 +98,7 @@ public class FileRunEventStore {
|
||||
entity.setOrigin(command.origin());
|
||||
entity.setPolicyId(command.policyId());
|
||||
entity.setRunId(command.runId());
|
||||
entity.setSourceId(command.sourceId());
|
||||
entity.setFileId(command.fileId());
|
||||
entity.setDetail(command.detail());
|
||||
entity.setDedupKey(dedupKey);
|
||||
@@ -112,9 +113,14 @@ public class FileRunEventStore {
|
||||
}
|
||||
|
||||
/**
|
||||
* A page of incidents, newest first, optionally narrowed to one status and one kind. The kind
|
||||
* filter lives in the query, before the limit: filtering a already-limited page could return
|
||||
* nothing while matching rows exist.
|
||||
* A page of incidents, newest first, optionally narrowed to one status and one kind.
|
||||
*
|
||||
* <p>With no status asked for this is the <em>open</em> queue rather than every row ever
|
||||
* recorded: a dismissed failure has been dealt with, and leaving it in the default view means
|
||||
* the list can never be cleared. Ask for a status to see closed rows.
|
||||
*
|
||||
* <p>Both filters live in the query, before the limit: filtering an already-limited page could
|
||||
* return nothing while matching rows exist.
|
||||
*/
|
||||
@Transactional(readOnly = true)
|
||||
public List<FileRunEvent> list(
|
||||
@@ -122,7 +128,8 @@ public class FileRunEventStore {
|
||||
Pageable page = PageRequest.of(0, Math.max(1, limit));
|
||||
List<FileRunEventEntity> rows =
|
||||
status == null
|
||||
? repository.findByTeam(teamId, kindId, page)
|
||||
? repository.findByTeamAndStatusIn(
|
||||
teamId, FileRunEventStatus.open(), kindId, page)
|
||||
: repository.findByTeamAndStatus(teamId, status, kindId, page);
|
||||
return rows.stream().map(FileRunEvent::of).toList();
|
||||
}
|
||||
@@ -184,6 +191,22 @@ public class FileRunEventStore {
|
||||
.orElseThrow(() -> refusalFor(id, teamId));
|
||||
}
|
||||
|
||||
/**
|
||||
* Close this owner's open incidents about {@code fileIds}, because the documents are gone. The
|
||||
* rows stay for audit; they just leave the queue. Only open rows move, so a reviewer's dismiss
|
||||
* keeps its meaning and its actor.
|
||||
*
|
||||
* @return how many incidents were closed
|
||||
*/
|
||||
@Transactional
|
||||
public int markFilesRemoved(Long teamId, String actor, Collection<String> fileIds) {
|
||||
if (fileIds.isEmpty()) {
|
||||
return 0;
|
||||
}
|
||||
return repository.markFilesRemoved(
|
||||
teamId, actor, fileIds, Instant.now(), FileRunEventStatus.open());
|
||||
}
|
||||
|
||||
/**
|
||||
* Why the guarded UPDATE refused, worked out only once it has. Missing and closed are told
|
||||
* apart after the fact rather than before, so the answer describes the row the UPDATE saw.
|
||||
|
||||
+2
@@ -22,6 +22,7 @@ public record FileRunEventView(
|
||||
String detail,
|
||||
String policyId,
|
||||
String runId,
|
||||
String sourceId,
|
||||
String fileId,
|
||||
String actor,
|
||||
int occurrences,
|
||||
@@ -48,6 +49,7 @@ public record FileRunEventView(
|
||||
event.detail(),
|
||||
event.policyId(),
|
||||
event.runId(),
|
||||
event.sourceId(),
|
||||
event.fileId(),
|
||||
event.actor(),
|
||||
event.occurrences(),
|
||||
|
||||
+21
-6
@@ -30,11 +30,12 @@ public class PolicyFailureRecorder {
|
||||
public void recordRunFailure(
|
||||
String runId,
|
||||
String policyId,
|
||||
String actor,
|
||||
String sourceId,
|
||||
String fileIdentity,
|
||||
String actor,
|
||||
String detail,
|
||||
Throwable cause) {
|
||||
record(classifier.classify(cause), runId, policyId, actor, fileIdentity, detail);
|
||||
record(classifier.classify(cause), runId, policyId, sourceId, fileIdentity, actor, detail);
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -44,21 +45,35 @@ public class PolicyFailureRecorder {
|
||||
* pick the wrong one.
|
||||
*/
|
||||
public void recordRunFailureAs(
|
||||
FailureKind kind, String runId, String policyId, String actor, String detail) {
|
||||
record(kind, runId, policyId, actor, null, detail);
|
||||
FailureKind kind,
|
||||
String runId,
|
||||
String policyId,
|
||||
String sourceId,
|
||||
String actor,
|
||||
String detail) {
|
||||
// No document reference: a run rejected at admission never got as far as one.
|
||||
record(kind, runId, policyId, sourceId, null, actor, detail);
|
||||
}
|
||||
|
||||
private void record(
|
||||
FailureKind kind,
|
||||
String runId,
|
||||
String policyId,
|
||||
String actor,
|
||||
String sourceId,
|
||||
String fileIdentity,
|
||||
String actor,
|
||||
String detail) {
|
||||
try {
|
||||
store.record(
|
||||
RecordFailure.forRun(
|
||||
kind, teamFor(policyId), actor, policyId, runId, fileIdentity, detail));
|
||||
kind,
|
||||
teamFor(policyId),
|
||||
actor,
|
||||
policyId,
|
||||
runId,
|
||||
sourceId,
|
||||
fileIdentity,
|
||||
detail));
|
||||
} catch (RuntimeException e) {
|
||||
// Deliberately swallowed: see the class comment.
|
||||
log.warn("Could not record failure event for run {} (kind {})", runId, kind.getId(), e);
|
||||
|
||||
+40
-10
@@ -38,17 +38,40 @@ public record RecordFailure(
|
||||
detail = truncate(detail);
|
||||
}
|
||||
|
||||
/** A processor-side failure with no file or source context, e.g. a run that failed outright. */
|
||||
/**
|
||||
* A processor-side run failure. {@code sourceId} says which folder, bucket or webhook fed the
|
||||
* run, and is the only attribution an unattended failure has: there is no user to name. {@code
|
||||
* fileId} is the source's opaque reference to the document, already hashed upstream.
|
||||
*/
|
||||
public static RecordFailure forRun(
|
||||
FailureKind kind,
|
||||
Long teamId,
|
||||
String actor,
|
||||
String policyId,
|
||||
String runId,
|
||||
String sourceId,
|
||||
String fileId,
|
||||
String detail) {
|
||||
return new RecordFailure(
|
||||
kind, FailureOrigin.POLICY, teamId, actor, policyId, runId, null, fileId, detail);
|
||||
kind,
|
||||
FailureOrigin.POLICY,
|
||||
teamId,
|
||||
actor,
|
||||
policyId,
|
||||
runId,
|
||||
sourceId,
|
||||
fileId,
|
||||
detail);
|
||||
}
|
||||
|
||||
/**
|
||||
* A failure a user hit in their own editor. There is no policy, run or source: the user is the
|
||||
* attribution, and {@code fileId} may be null when the report named no file.
|
||||
*/
|
||||
public static RecordFailure forEditor(
|
||||
FailureKind kind, Long teamId, String actor, String fileId, String detail) {
|
||||
return new RecordFailure(
|
||||
kind, FailureOrigin.TOOL, teamId, actor, null, null, null, fileId, detail);
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -56,14 +79,21 @@ public record RecordFailure(
|
||||
* reference are the same incident; see {@link #dedupKey()}.
|
||||
*/
|
||||
public String scopeRef() {
|
||||
return switch (kind.getScope()) {
|
||||
case FILE -> nullToEmpty(policyId) + "|" + fileOrRun();
|
||||
case RUN -> nullToEmpty(runId);
|
||||
case POLICY -> nullToEmpty(policyId);
|
||||
case SOURCE -> nullToEmpty(sourceId);
|
||||
// One server-wide condition is one incident regardless of which run tripped over it.
|
||||
case SERVER -> "";
|
||||
};
|
||||
String about =
|
||||
switch (kind.getScope()) {
|
||||
case FILE -> nullToEmpty(policyId) + "|" + fileOrRun();
|
||||
// An editor report has no run, so a RUN-scoped kind would otherwise put every
|
||||
// such failure in a team into one incident: fall back to the document.
|
||||
case RUN -> isBlank(runId) ? fileOrRun() : nullToEmpty(runId);
|
||||
case POLICY -> nullToEmpty(policyId);
|
||||
case SOURCE -> nullToEmpty(sourceId);
|
||||
// One server-wide condition is one incident regardless of which run hit it.
|
||||
case SERVER -> "";
|
||||
};
|
||||
// An editor failure belongs to the person who hit it, so two colleagues hitting the same
|
||||
// thing are two incidents. Folding them would credit one actor for both and offer the
|
||||
// wrong person the row. Unattended runs have no such owner and are unaffected.
|
||||
return origin == FailureOrigin.TOOL ? nullToEmpty(actor) + "|" + about : about;
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
+26
-14
@@ -130,25 +130,27 @@ public class PolicyEngine {
|
||||
// worker.
|
||||
String principal = currentActingPrincipal();
|
||||
return submitForPrincipal(
|
||||
principal, principal, policyId, definition, inputs, null, listener);
|
||||
principal, principal, policyId, definition, inputs, listener, null, null);
|
||||
}
|
||||
|
||||
/** Run a stored policy on demand. {@code enabled} gates triggers, not explicit runs. */
|
||||
public PolicyRunHandle runPolicy(
|
||||
Policy policy, PolicyInputs inputs, PolicyProgressListener listener) {
|
||||
return runPolicy(policy, inputs, null, listener);
|
||||
return runPolicy(policy, inputs, listener, null, null);
|
||||
}
|
||||
|
||||
/**
|
||||
* As {@link #runPolicy(Policy, PolicyInputs, PolicyProgressListener)}, with the source's opaque
|
||||
* reference to the document being run. Carried so a failure can say which document it was
|
||||
* about, and so the same document failing again folds into one incident.
|
||||
* As {@link #runPolicy(Policy, PolicyInputs, PolicyProgressListener)}, recording which source
|
||||
* fed the run and its opaque reference to the document. The first says where an unattended
|
||||
* failure came from; the second says which document, and is what lets the same document failing
|
||||
* again fold into one incident. Both null for a user's upload.
|
||||
*/
|
||||
public PolicyRunHandle runPolicy(
|
||||
Policy policy,
|
||||
PolicyInputs inputs,
|
||||
String fileIdentity,
|
||||
PolicyProgressListener listener) {
|
||||
PolicyProgressListener listener,
|
||||
String sourceId,
|
||||
String fileIdentity) {
|
||||
// Bill the policy owner: trigger-fired runs have no security context, and the async worker
|
||||
// doesn't inherit the caller's, so the owner (stamped at policy creation) is the reliable
|
||||
// billing identity — and for org-wide policies the org/owner is meant to pay. But own the
|
||||
@@ -172,9 +174,12 @@ public class PolicyEngine {
|
||||
fileOwner,
|
||||
policy.id(),
|
||||
definition,
|
||||
// main's asset-resolved inputs, not the raw ones: stored certificates and watermark
|
||||
// images bind here, before the async hop, because worker threads have no principal.
|
||||
resolved,
|
||||
fileIdentity,
|
||||
listener);
|
||||
listener,
|
||||
sourceId,
|
||||
fileIdentity);
|
||||
}
|
||||
|
||||
private PolicyRunHandle submitForPrincipal(
|
||||
@@ -183,8 +188,9 @@ public class PolicyEngine {
|
||||
String policyId,
|
||||
PipelineDefinition definition,
|
||||
PolicyInputs inputs,
|
||||
String fileIdentity,
|
||||
PolicyProgressListener listener) {
|
||||
PolicyProgressListener listener,
|
||||
String sourceId,
|
||||
String fileIdentity) {
|
||||
// Scope the run id to the current user (this request thread) so the file-download
|
||||
// ownership check passes. No-op when security is off.
|
||||
String runId = jobOwnershipService.createScopedJobKey(UUID.randomUUID().toString());
|
||||
@@ -193,7 +199,7 @@ public class PolicyEngine {
|
||||
if (policyId != null) {
|
||||
taskManager.putMetadata(runId, "policyId", policyId);
|
||||
}
|
||||
PolicyRun run = new PolicyRun(runId, policyId, definition, fileIdentity);
|
||||
PolicyRun run = new PolicyRun(runId, policyId, definition, sourceId, fileIdentity);
|
||||
registry.register(run);
|
||||
CompletableFuture<PolicyRun> completion = new CompletableFuture<>();
|
||||
PolicyProgressListener tracking = trackingListener(runId, run, listener);
|
||||
@@ -359,7 +365,12 @@ public class PolicyEngine {
|
||||
// No exception to classify here: nothing was thrown by a tool, the run simply was not
|
||||
// admitted. Record it explicitly so a run lost to load pressure is still accounted for.
|
||||
failureRecorder.recordRunFailureAs(
|
||||
FailureKind.UNKNOWN, run.getRunId(), run.getPolicyId(), null, message);
|
||||
FailureKind.UNKNOWN,
|
||||
run.getRunId(),
|
||||
run.getPolicyId(),
|
||||
run.getSourceId(),
|
||||
null,
|
||||
message);
|
||||
completion.complete(run);
|
||||
}
|
||||
return null;
|
||||
@@ -373,8 +384,9 @@ public class PolicyEngine {
|
||||
failureRecorder.recordRunFailure(
|
||||
run.getRunId(),
|
||||
run.getPolicyId(),
|
||||
MDC.get(AUDIT_PRINCIPAL_MDC_KEY),
|
||||
run.getSourceId(),
|
||||
run.getFileIdentity(),
|
||||
MDC.get(AUDIT_PRINCIPAL_MDC_KEY),
|
||||
message,
|
||||
cause);
|
||||
}
|
||||
|
||||
+16
-4
@@ -79,7 +79,8 @@ public class PolicyRunner {
|
||||
// Generator pipeline: one run with no input. Still fall through to the cleanup
|
||||
// below so rows recorded for its folder outputs are pruned like anything else,
|
||||
// instead of accumulating until the policy is deleted.
|
||||
runIds.add(startRun(policy, PolicyInputs.of(List.of()), null, unused -> {}));
|
||||
// Generator pipeline: no input, so neither a source nor a document to attribute to.
|
||||
runIds.add(startRun(policy, null, null, PolicyInputs.of(List.of()), unused -> {}));
|
||||
}
|
||||
for (PipelineInput input : inputs) {
|
||||
String sourceId = input.sourceId();
|
||||
@@ -167,7 +168,13 @@ public class PolicyRunner {
|
||||
List<String> runIds = new ArrayList<>();
|
||||
long docsFed = 0;
|
||||
for (ResolvedInput unit : work) {
|
||||
runIds.add(startRun(policy, unit.inputs(), unit.fileIdentity(), unit.onComplete()));
|
||||
runIds.add(
|
||||
startRun(
|
||||
policy,
|
||||
sourceId,
|
||||
unit.fileIdentity(),
|
||||
unit.inputs(),
|
||||
unit.onComplete()));
|
||||
docsFed += unit.inputs().primary().size();
|
||||
}
|
||||
docCounter.record(sourceId, docsFed);
|
||||
@@ -175,10 +182,15 @@ public class PolicyRunner {
|
||||
}
|
||||
|
||||
private String startRun(
|
||||
Policy policy, PolicyInputs inputs, String fileIdentity, Consumer<Boolean> onComplete) {
|
||||
Policy policy,
|
||||
String sourceId,
|
||||
String fileIdentity,
|
||||
PolicyInputs inputs,
|
||||
Consumer<Boolean> onComplete) {
|
||||
log.info("Running policy {} ({})", policy.id(), policy.name());
|
||||
PolicyRunHandle handle =
|
||||
policyEngine.runPolicy(policy, inputs, fileIdentity, PolicyProgressListener.NOOP);
|
||||
policyEngine.runPolicy(
|
||||
policy, inputs, PolicyProgressListener.NOOP, sourceId, fileIdentity);
|
||||
handle.completion()
|
||||
.whenComplete((run, throwable) -> onComplete.accept(succeeded(run, throwable)));
|
||||
return handle.runId();
|
||||
|
||||
+18
-1
@@ -21,6 +21,12 @@ public class PolicyRun {
|
||||
/** ID of the stored policy that produced this run; null for ad-hoc pipelines. */
|
||||
private final String policyId;
|
||||
|
||||
/**
|
||||
* ID of the source the input came from (folder, S3, webhook), or null when a user supplied the
|
||||
* files. Recorded on a failure so a reviewer can see where an unattended file came from.
|
||||
*/
|
||||
private final String sourceId;
|
||||
|
||||
private final PipelineDefinition definition;
|
||||
|
||||
/**
|
||||
@@ -56,10 +62,21 @@ public class PolicyRun {
|
||||
private volatile List<ResultFile> outputs = List.of();
|
||||
private volatile Instant updatedAt = Instant.now();
|
||||
|
||||
/**
|
||||
* Both references are required rather than defaulted: a run with neither is a real case (a
|
||||
* user's upload, an ad-hoc pipeline), but it should be stated at the call site. Overloads that
|
||||
* omitted them would make losing the attribution the frictionless option, which is how both
|
||||
* fields went unpopulated in the first place.
|
||||
*/
|
||||
public PolicyRun(
|
||||
String runId, String policyId, PipelineDefinition definition, String fileIdentity) {
|
||||
String runId,
|
||||
String policyId,
|
||||
PipelineDefinition definition,
|
||||
String sourceId,
|
||||
String fileIdentity) {
|
||||
this.runId = runId;
|
||||
this.policyId = policyId;
|
||||
this.sourceId = sourceId;
|
||||
this.definition = definition;
|
||||
this.fileIdentity = fileIdentity;
|
||||
}
|
||||
|
||||
+11
-5
@@ -182,10 +182,10 @@ class FailureKindTest {
|
||||
class Unknown {
|
||||
|
||||
@Test
|
||||
void existsAndCanBeTriaged() {
|
||||
assertThat(FailureKind.UNKNOWN.getActions())
|
||||
.containsExactlyInAnyOrder(
|
||||
FailureActionId.ACKNOWLEDGE, FailureActionId.DISMISS);
|
||||
void offersOnlyTheActionThatClearsIt() {
|
||||
// Nothing here can be fixed, so "seen it" and "clear it" would be the same decision.
|
||||
// Offering both just asks the reviewer to press two buttons to reach one outcome.
|
||||
assertThat(FailureKind.UNKNOWN.getActions()).containsExactly(FailureActionId.DISMISS);
|
||||
}
|
||||
|
||||
@Test
|
||||
@@ -233,8 +233,14 @@ class FailureKindTest {
|
||||
|
||||
@Test
|
||||
void declaresOnlyWhatItLists() {
|
||||
assertThat(FailureKind.UNKNOWN.declares(FailureActionId.ACKNOWLEDGE)).isTrue();
|
||||
assertThat(FailureKind.UNKNOWN.declares(FailureActionId.DISMISS)).isTrue();
|
||||
assertThat(FailureKind.UNKNOWN.declares(FailureActionId.ACKNOWLEDGE)).isFalse();
|
||||
}
|
||||
|
||||
@Test
|
||||
void aKindWithSomethingToFixOffersTheFixAndAWayToSkipIt() {
|
||||
assertThat(FailureKind.INPUT_PASSWORD_PROTECTED.getActions())
|
||||
.containsExactly(FailureActionId.ACKNOWLEDGE, FailureActionId.DISMISS);
|
||||
}
|
||||
|
||||
@Test
|
||||
|
||||
+123
-14
@@ -74,6 +74,20 @@ class FileRunEventControllerTest {
|
||||
"the raw failure message"));
|
||||
}
|
||||
|
||||
private static List<String> fileIds(int count) {
|
||||
return java.util.stream.IntStream.range(0, count).mapToObj(i -> "f-" + i).toList();
|
||||
}
|
||||
|
||||
/** The status a refused call came back with. Fails the test if the call was allowed. */
|
||||
private HttpStatus statusOf(Runnable call) {
|
||||
try {
|
||||
call.run();
|
||||
} catch (ResponseStatusException e) {
|
||||
return HttpStatus.valueOf(e.getStatusCode().value());
|
||||
}
|
||||
throw new AssertionError("expected the call to be refused");
|
||||
}
|
||||
|
||||
@Nested
|
||||
@DisplayName("listing")
|
||||
class Listing {
|
||||
@@ -117,6 +131,7 @@ class FileRunEventControllerTest {
|
||||
|
||||
@Test
|
||||
void showsAClosedRowsActionsDisabledWithAReasonRatherThanHidingThem() {
|
||||
// Only visible by asking for the closed status: the default queue drops it.
|
||||
FileRunEvent event = given(FailureKind.UNKNOWN, TEAM, "f1");
|
||||
controller.act(event.id(), "DISMISS", null);
|
||||
|
||||
@@ -134,15 +149,16 @@ class FileRunEventControllerTest {
|
||||
|
||||
@Test
|
||||
void filtersByStatusAndByKind() {
|
||||
FileRunEvent open = given(FailureKind.UNKNOWN, TEAM, "open");
|
||||
given(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "locked");
|
||||
controller.act(open.id(), "ACKNOWLEDGE", null);
|
||||
FileRunEvent locked = given(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "locked");
|
||||
given(FailureKind.UNKNOWN, TEAM, "open");
|
||||
controller.act(locked.id(), "ACKNOWLEDGE", null);
|
||||
|
||||
assertThat(controller.list(FileRunEventStatus.ACKNOWLEDGED, null, null).events())
|
||||
.hasSize(1);
|
||||
assertThat(controller.list(null, "INPUT_PASSWORD_PROTECTED", null).events())
|
||||
.extracting(FileRunEventView::fileId)
|
||||
.containsExactly("locked");
|
||||
// Acknowledged is still open work, so it stays in the default queue.
|
||||
assertThat(controller.list(null, "NO_SUCH_KIND", null).events()).isEmpty();
|
||||
}
|
||||
|
||||
@@ -182,7 +198,7 @@ class FileRunEventControllerTest {
|
||||
|
||||
@Test
|
||||
void appliesADeclaredActionAndReturnsTheUpdatedRow() {
|
||||
FileRunEvent event = given(FailureKind.UNKNOWN, TEAM, "f1");
|
||||
FileRunEvent event = given(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1");
|
||||
|
||||
FileRunEventView updated = controller.act(event.id(), "ACKNOWLEDGE", null);
|
||||
|
||||
@@ -218,21 +234,12 @@ class FileRunEventControllerTest {
|
||||
@Test
|
||||
void anAlreadyClosedRowIsAConflict() {
|
||||
// The request was well formed and would have been valid a moment earlier.
|
||||
FileRunEvent event = given(FailureKind.UNKNOWN, TEAM, "f1");
|
||||
FileRunEvent event = given(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1");
|
||||
controller.act(event.id(), "DISMISS", null);
|
||||
|
||||
assertThat(statusOf(() -> controller.act(event.id(), "ACKNOWLEDGE", null)))
|
||||
.isEqualTo(HttpStatus.CONFLICT);
|
||||
}
|
||||
|
||||
private HttpStatus statusOf(Runnable call) {
|
||||
try {
|
||||
call.run();
|
||||
} catch (ResponseStatusException e) {
|
||||
return HttpStatus.valueOf(e.getStatusCode().value());
|
||||
}
|
||||
throw new AssertionError("expected the call to be refused");
|
||||
}
|
||||
}
|
||||
|
||||
@Nested
|
||||
@@ -387,6 +394,108 @@ class FileRunEventControllerTest {
|
||||
}
|
||||
}
|
||||
|
||||
@Nested
|
||||
@DisplayName("reporting from the editor")
|
||||
class Reporting {
|
||||
|
||||
@Test
|
||||
void aMemberMayReportEvenThoughTheyMayNotRead() {
|
||||
// The asymmetry is the point: anyone whose work failed can say so, but only a leader
|
||||
// reviews the queue.
|
||||
when(authority.canEditPolicies()).thenReturn(false);
|
||||
|
||||
assertThatCode(
|
||||
() ->
|
||||
controller.report(
|
||||
new EditorFailureReport(
|
||||
"compress", "E004", List.of("f-1"), "boom")))
|
||||
.doesNotThrowAnyException();
|
||||
assertThatThrownBy(() -> controller.list(null, null, null))
|
||||
.isInstanceOf(ResponseStatusException.class);
|
||||
}
|
||||
|
||||
@Test
|
||||
void answersWithNoContentSoTheEditorNeverWaitsOnABody() {
|
||||
EditorFailureReport report =
|
||||
new EditorFailureReport("compress", "E004", List.of("f-1"), "boom");
|
||||
|
||||
assertThat(controller.report(report).getStatusCode()).isEqualTo(HttpStatus.NO_CONTENT);
|
||||
}
|
||||
|
||||
@Test
|
||||
void rejectsAReportWithNoOperation() {
|
||||
assertThat(
|
||||
statusOf(
|
||||
() ->
|
||||
controller.report(
|
||||
new EditorFailureReport(
|
||||
" ", "E004", List.of("f-1"), "boom"))))
|
||||
.isEqualTo(HttpStatus.BAD_REQUEST);
|
||||
}
|
||||
|
||||
@Test
|
||||
void acceptsAReportAtTheFileLimitAndRecordsEveryRow() {
|
||||
List<String> atLimit = fileIds(EditorFailureReport.MAX_FILE_IDS);
|
||||
|
||||
EditorFailureReport report =
|
||||
new EditorFailureReport("compress", "E004", atLimit, "boom");
|
||||
|
||||
assertThat(controller.report(report).getStatusCode()).isEqualTo(HttpStatus.NO_CONTENT);
|
||||
assertThat(store.list(TEAM, null, null, EditorFailureReport.MAX_FILE_IDS + 10))
|
||||
.hasSize(EditorFailureReport.MAX_FILE_IDS);
|
||||
}
|
||||
|
||||
@Test
|
||||
void refusesAReportOverTheFileLimitAndRecordsNothing() {
|
||||
// One call used to be able to mint an unbounded number of permanent incidents, since
|
||||
// each named file gets its own row and TOOL dedup keys never fold across ids. Refused
|
||||
// rather than trimmed so nothing is lost silently, and refused before the first write
|
||||
// so a rejected report cannot leave a partial set behind either.
|
||||
List<String> overLimit = fileIds(EditorFailureReport.MAX_FILE_IDS + 1);
|
||||
|
||||
assertThat(
|
||||
statusOf(
|
||||
() ->
|
||||
controller.report(
|
||||
new EditorFailureReport(
|
||||
"compress",
|
||||
"E004",
|
||||
overLimit,
|
||||
"boom"))))
|
||||
.isEqualTo(HttpStatus.BAD_REQUEST);
|
||||
assertThat(store.list(TEAM, null, null, EditorFailureReport.MAX_FILE_IDS + 10))
|
||||
.isEmpty();
|
||||
}
|
||||
|
||||
@Test
|
||||
void saysWhatTheLimitIsSoAClientAuthorCanSeeWhatHappened() {
|
||||
// The editor reports in the background, so the message is the only place this surfaces.
|
||||
assertThatThrownBy(
|
||||
() ->
|
||||
controller.report(
|
||||
new EditorFailureReport(
|
||||
"compress",
|
||||
"E004",
|
||||
fileIds(EditorFailureReport.MAX_FILE_IDS + 1),
|
||||
"boom")))
|
||||
.isInstanceOf(ResponseStatusException.class)
|
||||
.hasMessageContaining(String.valueOf(EditorFailureReport.MAX_FILE_IDS));
|
||||
}
|
||||
|
||||
@Test
|
||||
void theReportHasNoTeamOrFileNameToSupply() {
|
||||
// Stated as a test because the absence of those fields is the property. Adding either
|
||||
// to
|
||||
// EditorFailureReport breaks this at compile time.
|
||||
List<String> components =
|
||||
java.util.Arrays.stream(EditorFailureReport.class.getRecordComponents())
|
||||
.map(java.lang.reflect.RecordComponent::getName)
|
||||
.toList();
|
||||
|
||||
assertThat(components).containsExactly("operation", "errorCode", "fileIds", "detail");
|
||||
}
|
||||
}
|
||||
|
||||
@Nested
|
||||
@DisplayName("action request body")
|
||||
class RequestBody {
|
||||
|
||||
+74
-5
@@ -152,9 +152,9 @@ class FileRunEventHttpIntegrationTest {
|
||||
|
||||
@Test
|
||||
void coercesQueryParametersAndFiltersOnThem() throws Exception {
|
||||
String open = seed(FailureKind.UNKNOWN, TEAM, "open", "a");
|
||||
seed(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "locked", "b");
|
||||
post("/api/v1/file-run-events/" + open + "/actions/ACKNOWLEDGE", "{\"inputs\":{}}");
|
||||
String locked = seed(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "locked", "b");
|
||||
seed(FailureKind.UNKNOWN, TEAM, "open", "a");
|
||||
post("/api/v1/file-run-events/" + locked + "/actions/ACKNOWLEDGE", "{\"inputs\":{}}");
|
||||
|
||||
JsonNode acknowledged =
|
||||
mapper.readTree(get("/api/v1/file-run-events?status=ACKNOWLEDGED").body())
|
||||
@@ -189,6 +189,75 @@ class FileRunEventHttpIntegrationTest {
|
||||
}
|
||||
}
|
||||
|
||||
@Nested
|
||||
@DisplayName("reporting from the editor")
|
||||
class Reporting {
|
||||
|
||||
@Test
|
||||
void bindsAReportBodyAndAnswersNoContent() throws Exception {
|
||||
HttpResponse<String> response =
|
||||
post(
|
||||
"/api/v1/file-run-events/reports",
|
||||
"{\"operation\":\"remove-password\",\"errorCode\":\"E004\","
|
||||
+ "\"fileIds\":[\"f-1\",\"f-2\"],\"detail\":\"locked\"}");
|
||||
|
||||
assertThat(response.statusCode()).isEqualTo(204);
|
||||
assertThat(response.body()).isEmpty();
|
||||
|
||||
JsonNode events = mapper.readTree(get("/api/v1/file-run-events").body()).get("events");
|
||||
assertThat(events).hasSize(2);
|
||||
assertThat(events.get(0).get("origin").asString()).isEqualTo("TOOL");
|
||||
assertThat(events.get(0).get("kindId").asString())
|
||||
.isEqualTo("INPUT_PASSWORD_PROTECTED");
|
||||
}
|
||||
|
||||
@Test
|
||||
void acceptsAReportWithNoCodeOrFiles() throws Exception {
|
||||
HttpResponse<String> response =
|
||||
post(
|
||||
"/api/v1/file-run-events/reports",
|
||||
"{\"operation\":\"compress\",\"detail\":\"network died\"}");
|
||||
|
||||
assertThat(response.statusCode()).isEqualTo(204);
|
||||
JsonNode events = mapper.readTree(get("/api/v1/file-run-events").body()).get("events");
|
||||
assertThat(events).hasSize(1);
|
||||
assertThat(events.get(0).get("kindId").asString()).isEqualTo("UNKNOWN");
|
||||
assertThat(events.get(0).get("fileId").isNull()).isTrue();
|
||||
}
|
||||
|
||||
@Test
|
||||
void rejectsAReportWithNoOperation() throws Exception {
|
||||
assertThat(
|
||||
post(
|
||||
"/api/v1/file-run-events/reports",
|
||||
"{\"errorCode\":\"E004\",\"detail\":\"boom\"}")
|
||||
.statusCode())
|
||||
.isEqualTo(400);
|
||||
}
|
||||
|
||||
@Test
|
||||
void rejectsAnOversizedReportWithoutRecordingAnyOfIt() throws Exception {
|
||||
// Over the wire because that is where the flood would arrive: one request, an
|
||||
// arbitrarily long fileIds array, a permanent row per entry. The read-back is the point
|
||||
// of the test, since a partial write would be worse than either accepting or refusing.
|
||||
String ids =
|
||||
java.util.stream.IntStream.range(0, EditorFailureReport.MAX_FILE_IDS + 1)
|
||||
.mapToObj(i -> "\"f-" + i + "\"")
|
||||
.collect(java.util.stream.Collectors.joining(","));
|
||||
|
||||
HttpResponse<String> response =
|
||||
post(
|
||||
"/api/v1/file-run-events/reports",
|
||||
"{\"operation\":\"compress\",\"errorCode\":\"E004\",\"fileIds\":["
|
||||
+ ids
|
||||
+ "],\"detail\":\"boom\"}");
|
||||
|
||||
assertThat(response.statusCode()).isEqualTo(400);
|
||||
assertThat(mapper.readTree(get("/api/v1/file-run-events").body()).get("events"))
|
||||
.isEmpty();
|
||||
}
|
||||
}
|
||||
|
||||
@Nested
|
||||
@DisplayName("action dispatch")
|
||||
class Dispatch {
|
||||
@@ -197,7 +266,7 @@ class FileRunEventHttpIntegrationTest {
|
||||
void bindsTheRequestBodyAndReturnsTheUpdatedRow() throws Exception {
|
||||
// The regression guard: an object body, sent as real JSON over the wire, binding into
|
||||
// ActionRequest. A double-encoded string would fail here.
|
||||
String id = seed(FailureKind.UNKNOWN, TEAM, "f1", "boom");
|
||||
String id = seed(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1", "boom");
|
||||
|
||||
HttpResponse<String> response =
|
||||
post(
|
||||
@@ -262,7 +331,7 @@ class FileRunEventHttpIntegrationTest {
|
||||
|
||||
@Test
|
||||
void mapsAnAlreadyClosedRowToConflict() throws Exception {
|
||||
String id = seed(FailureKind.UNKNOWN, TEAM, "f1", "boom");
|
||||
String id = seed(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1", "boom");
|
||||
post("/api/v1/file-run-events/" + id + "/actions/DISMISS", "{\"inputs\":{}}");
|
||||
|
||||
assertThat(
|
||||
|
||||
+254
-6
@@ -76,7 +76,7 @@ class FileRunEventServiceTest {
|
||||
|
||||
@Test
|
||||
void movesANewEventToAcknowledgedAndStampsTheActor() {
|
||||
FileRunEvent event = given(FailureKind.UNKNOWN, TEAM, "f1");
|
||||
FileRunEvent event = given(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1");
|
||||
|
||||
FileRunEvent updated = service.dispatch(event.id(), "ACKNOWLEDGE", Map.of());
|
||||
|
||||
@@ -87,7 +87,7 @@ class FileRunEventServiceTest {
|
||||
|
||||
@Test
|
||||
void isANoOpWhenAlreadyAcknowledgedSoOwnershipIsNotStolen() {
|
||||
FileRunEvent event = given(FailureKind.UNKNOWN, TEAM, "f1");
|
||||
FileRunEvent event = given(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1");
|
||||
FileRunEvent first = service.dispatch(event.id(), "ACKNOWLEDGE", Map.of());
|
||||
Instant originalAt = first.statusAt();
|
||||
|
||||
@@ -114,7 +114,7 @@ class FileRunEventServiceTest {
|
||||
|
||||
@Test
|
||||
void closesAnAcknowledgedEvent() {
|
||||
FileRunEvent event = given(FailureKind.UNKNOWN, TEAM, "f1");
|
||||
FileRunEvent event = given(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1");
|
||||
service.dispatch(event.id(), "ACKNOWLEDGE", Map.of());
|
||||
|
||||
assertThat(service.dispatch(event.id(), "DISMISS", Map.of()).status())
|
||||
@@ -225,7 +225,7 @@ class FileRunEventServiceTest {
|
||||
|
||||
@Test
|
||||
void aClosedEventCannotBeActedOnAgain() {
|
||||
FileRunEvent event = given(FailureKind.UNKNOWN, TEAM, "f1");
|
||||
FileRunEvent event = given(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1");
|
||||
service.dispatch(event.id(), "DISMISS", Map.of());
|
||||
|
||||
assertThatThrownBy(() -> service.dispatch(event.id(), "ACKNOWLEDGE", Map.of()))
|
||||
@@ -286,8 +286,7 @@ class FileRunEventServiceTest {
|
||||
|
||||
assertThat(service.availableActions(event))
|
||||
.extracting(FileRunEventService.AvailableAction::labelKey)
|
||||
.containsExactlyInAnyOrder(
|
||||
"portal.failures.action.acknowledge", "portal.failures.action.dismiss");
|
||||
.containsExactly("portal.failures.action.dismiss");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -390,4 +389,253 @@ class FileRunEventServiceTest {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@Nested
|
||||
@DisplayName("reporting a failure the user hit in the editor")
|
||||
class Reporting {
|
||||
|
||||
@Test
|
||||
void classifiesTheReportedCodeAndStampsItAsEditorOrigin() {
|
||||
service.report(
|
||||
new EditorFailureReport("remove-password", "E004", List.of("f-1"), "boom"));
|
||||
|
||||
FileRunEvent event = store.list(TEAM, null, null, 10).getFirst();
|
||||
assertThat(event.kind()).isEqualTo(FailureKind.INPUT_PASSWORD_PROTECTED);
|
||||
assertThat(event.origin()).isEqualTo(FailureOrigin.TOOL);
|
||||
assertThat(event.fileId()).isEqualTo("f-1");
|
||||
assertThat(event.detail()).contains("boom");
|
||||
}
|
||||
|
||||
@Test
|
||||
void takesTheTeamAndActorFromThePrincipalNotTheReport() {
|
||||
// The report carries no team or actor field at all; both come from the caller's
|
||||
// session.
|
||||
service.report(new EditorFailureReport("compress", "E004", List.of("f-1"), "boom"));
|
||||
|
||||
FileRunEvent event = store.list(TEAM, null, null, 10).getFirst();
|
||||
assertThat(event.teamId()).isEqualTo(TEAM);
|
||||
assertThat(event.actor()).isEqualTo(ACTOR);
|
||||
}
|
||||
|
||||
@Test
|
||||
void recordsAnUnrecognisedCodeAsUnknownRatherThanDroppingIt() {
|
||||
service.report(new EditorFailureReport("ocr", "E999", List.of("f-1"), "no idea"));
|
||||
|
||||
assertThat(store.list(TEAM, null, null, 10).getFirst().kind())
|
||||
.isEqualTo(FailureKind.UNKNOWN);
|
||||
}
|
||||
|
||||
@Test
|
||||
void recordsAnAbsentCodeAsUnknown() {
|
||||
service.report(new EditorFailureReport("ocr", null, List.of("f-1"), "network died"));
|
||||
|
||||
assertThat(store.list(TEAM, null, null, 10).getFirst().kind())
|
||||
.isEqualTo(FailureKind.UNKNOWN);
|
||||
}
|
||||
|
||||
@Test
|
||||
void recordsOneIncidentPerFileSoEachDocumentStaysActionable() {
|
||||
service.report(
|
||||
new EditorFailureReport(
|
||||
"compress", "E004", List.of("f-1", "f-2", "f-3"), "boom"));
|
||||
|
||||
assertThat(store.list(TEAM, null, null, 10))
|
||||
.hasSize(3)
|
||||
.extracting(FileRunEvent::fileId)
|
||||
.containsExactlyInAnyOrder("f-1", "f-2", "f-3");
|
||||
}
|
||||
|
||||
@Test
|
||||
void foldsARepeatOfTheSameFileIntoTheExistingIncident() {
|
||||
service.report(new EditorFailureReport("compress", "E004", List.of("f-1"), "boom"));
|
||||
service.report(
|
||||
new EditorFailureReport("compress", "E004", List.of("f-1"), "boom again"));
|
||||
|
||||
assertThat(store.list(TEAM, null, null, 10))
|
||||
.singleElement()
|
||||
.extracting(FileRunEvent::occurrences)
|
||||
.isEqualTo(2);
|
||||
}
|
||||
|
||||
@Test
|
||||
void recordsOneUnattributedIncidentWhenNoFileWasNamed() {
|
||||
service.report(new EditorFailureReport("compress", "E004", List.of(), "boom"));
|
||||
|
||||
assertThat(store.list(TEAM, null, null, 10))
|
||||
.singleElement()
|
||||
.extracting(FileRunEvent::fileId)
|
||||
.isNull();
|
||||
}
|
||||
|
||||
@Test
|
||||
void recordsEveryFileInALargeBatchRatherThanTrimmingIt() {
|
||||
// A cap here used to drop the overflow silently, so a reviewer saw 25 of 60 failures
|
||||
// with nothing indicating the rest existed. The processor path has never had one.
|
||||
List<String> many =
|
||||
java.util.stream.IntStream.range(0, 60).mapToObj(i -> "f-" + i).toList();
|
||||
|
||||
service.report(new EditorFailureReport("compress", "E004", many, "boom"));
|
||||
|
||||
assertThat(store.list(TEAM, null, null, 200)).hasSize(60);
|
||||
}
|
||||
|
||||
@Test
|
||||
void keepsTheOperationNameOutOfTheStoredDocumentReferences() {
|
||||
// The operation is context for the reviewer, not a document reference: it belongs in
|
||||
// detail, never in fileId.
|
||||
service.report(
|
||||
new EditorFailureReport("remove-password", "E004", List.of("f-1"), "boom"));
|
||||
|
||||
FileRunEvent event = store.list(TEAM, null, null, 10).getFirst();
|
||||
assertThat(event.detail()).contains("remove-password");
|
||||
assertThat(event.fileId()).isEqualTo("f-1");
|
||||
}
|
||||
|
||||
@Test
|
||||
void storesTheReportedMessageVerbatimAlongsideTheOperation() {
|
||||
// The user's own error about their own file. The operation is prefixed because an
|
||||
// editor failure has no run to give a reviewer context.
|
||||
service.report(
|
||||
new EditorFailureReport(
|
||||
"compress", "E004", List.of("f-1"), "Failed on Q4 report.pdf"));
|
||||
|
||||
assertThat(store.list(TEAM, null, null, 10).getFirst().detail())
|
||||
.isEqualTo("compress: Failed on Q4 report.pdf");
|
||||
}
|
||||
|
||||
@Test
|
||||
void theServiceHoldsNothingThatCouldReachADocument() {
|
||||
// Asserted structurally rather than with verifyNoInteractions on unwired mocks, which
|
||||
// is how the version of this test on the other branch passed without proving anything.
|
||||
List<Class<?>> forbidden =
|
||||
List.of(
|
||||
stirling.software.proprietary.policy.ledger.ProcessedLedger.class,
|
||||
stirling.software.common.service.FileStorage.class,
|
||||
stirling.software.proprietary.policy.output.PolicyOutputSink.class);
|
||||
|
||||
List<Class<?>> held =
|
||||
Arrays.stream(FileRunEventService.class.getDeclaredFields())
|
||||
.filter(field -> !field.isSynthetic())
|
||||
.map(Field::getType)
|
||||
.toList();
|
||||
|
||||
assertThat(held).isNotEmpty().doesNotContainAnyElementsOf(forbidden);
|
||||
}
|
||||
}
|
||||
|
||||
@Nested
|
||||
@DisplayName("editor incidents stay separate")
|
||||
class EditorIncidentIdentity {
|
||||
|
||||
private void reportedBy(String actor, String fileId) {
|
||||
when(userService.getCurrentUsername()).thenReturn(actor);
|
||||
service.report(new EditorFailureReport("compress", null, List.of(fileId), "boom"));
|
||||
}
|
||||
|
||||
@Test
|
||||
void twoPeopleHittingTheSameUnclassifiedFailureAreTwoIncidents() {
|
||||
// UNKNOWN is RUN scoped and an editor report has no run, so without the fallback every
|
||||
// unclassified editor failure in a team collapsed into one row: one actor credited for
|
||||
// everyone's, and the wrong person offered the row.
|
||||
reportedBy("alice@example.com", "a-1");
|
||||
reportedBy("bob@example.com", "b-1");
|
||||
|
||||
assertThat(store.list(TEAM, null, null, 10))
|
||||
.extracting(FileRunEvent::actor)
|
||||
.containsExactlyInAnyOrder("alice@example.com", "bob@example.com");
|
||||
}
|
||||
|
||||
@Test
|
||||
void onePersonsTwoBrokenFilesAreTwoIncidents() {
|
||||
reportedBy("alice@example.com", "a-1");
|
||||
reportedBy("alice@example.com", "a-2");
|
||||
|
||||
assertThat(store.list(TEAM, null, null, 10))
|
||||
.extracting(FileRunEvent::fileId)
|
||||
.containsExactlyInAnyOrder("a-1", "a-2");
|
||||
}
|
||||
|
||||
@Test
|
||||
void theSamePersonHittingTheSameFileTwiceIsOneIncident() {
|
||||
reportedBy("alice@example.com", "a-1");
|
||||
reportedBy("alice@example.com", "a-1");
|
||||
|
||||
assertThat(store.list(TEAM, null, null, 10))
|
||||
.singleElement()
|
||||
.extracting(FileRunEvent::occurrences)
|
||||
.isEqualTo(2);
|
||||
}
|
||||
}
|
||||
|
||||
@Nested
|
||||
@DisplayName("files deleted from the editor")
|
||||
class RemovedFiles {
|
||||
|
||||
private void reported(String fileId) {
|
||||
service.report(new EditorFailureReport("compress", "E004", List.of(fileId), "boom"));
|
||||
}
|
||||
|
||||
@Test
|
||||
void closeTheirIncidentsSoTheQueueStopsAskingAboutThem() {
|
||||
reported("f-1");
|
||||
|
||||
assertThat(service.forgetFiles(List.of("f-1"))).isEqualTo(1);
|
||||
assertThat(service.list(null, null, 10))
|
||||
.as("the open queue is what the reviewer works from")
|
||||
.isEmpty();
|
||||
}
|
||||
|
||||
@Test
|
||||
void theRowSurvivesForAudit() {
|
||||
reported("f-1");
|
||||
service.forgetFiles(List.of("f-1"));
|
||||
|
||||
assertThat(service.list(FileRunEventStatus.FILE_REMOVED, null, 10))
|
||||
.singleElement()
|
||||
.satisfies(
|
||||
event -> {
|
||||
assertThat(event.fileId()).isEqualTo("f-1");
|
||||
assertThat(event.detail()).contains("boom");
|
||||
});
|
||||
}
|
||||
|
||||
@Test
|
||||
void aReviewersDismissKeepsItsMeaningAndItsActor() {
|
||||
reported("f-1");
|
||||
FileRunEvent event = service.list(null, null, 10).getFirst();
|
||||
service.dispatch(event.id(), "DISMISS", Map.of());
|
||||
|
||||
assertThat(service.forgetFiles(List.of("f-1")))
|
||||
.as("only open rows move; a closed one has already been decided")
|
||||
.isZero();
|
||||
assertThat(service.list(FileRunEventStatus.DISMISSED, null, 10)).hasSize(1);
|
||||
}
|
||||
|
||||
@Test
|
||||
void aColleaguesIncidentIsUntouched() {
|
||||
// File ids come from the client, so naming one must not close someone else's row.
|
||||
store.record(
|
||||
RecordFailure.forEditor(
|
||||
FailureKind.UNKNOWN, TEAM, "employee@example.com", "f-1", "theirs"));
|
||||
|
||||
assertThat(service.forgetFiles(List.of("f-1"))).isZero();
|
||||
}
|
||||
|
||||
@Test
|
||||
void aProcessorIncidentIsUntouchedEvenOnTheSameFileId() {
|
||||
// Nothing was deleted from an editor there, and the file may still be in the bucket.
|
||||
given(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f-1");
|
||||
|
||||
assertThat(service.forgetFiles(List.of("f-1"))).isZero();
|
||||
}
|
||||
|
||||
@Test
|
||||
void namingNoFilesClosesNothing() {
|
||||
reported("f-1");
|
||||
|
||||
assertThat(service.forgetFiles(List.of())).isZero();
|
||||
assertThat(service.forgetFiles(java.util.Arrays.asList(null, " "))).isZero();
|
||||
assertThat(service.list(null, null, 10)).hasSize(1);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
+55
@@ -6,6 +6,7 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy;
|
||||
import java.lang.reflect.InvocationTargetException;
|
||||
import java.lang.reflect.Proxy;
|
||||
import java.time.Instant;
|
||||
import java.util.List;
|
||||
import java.util.UUID;
|
||||
import java.util.concurrent.atomic.AtomicBoolean;
|
||||
|
||||
@@ -198,12 +199,66 @@ class FileRunEventStoreDbTest {
|
||||
FailureKind.INPUT_PASSWORD_PROTECTED,
|
||||
TEAM,
|
||||
null,
|
||||
null,
|
||||
"policy-1",
|
||||
runId,
|
||||
fileId,
|
||||
"locked");
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("closing deleted files touches only that owner's own open editor rows")
|
||||
void markFilesRemovedIsScopedBySqlNotByTheCaller() {
|
||||
// The scoping is entirely in the JPQL, so the in-memory fake proves nothing about it:
|
||||
// it implements the same rules by hand and would agree with a wrong query.
|
||||
FileRunEvent mine =
|
||||
store.record(
|
||||
RecordFailure.forEditor(
|
||||
FailureKind.UNKNOWN, TEAM, "owner@example.com", "f-1", "boom"));
|
||||
FileRunEvent theirs =
|
||||
store.record(
|
||||
RecordFailure.forEditor(
|
||||
FailureKind.UNKNOWN, TEAM, "colleague@example.com", "f-1", "boom"));
|
||||
FileRunEvent otherTeam =
|
||||
store.record(
|
||||
RecordFailure.forEditor(
|
||||
FailureKind.UNKNOWN,
|
||||
OTHER_TEAM,
|
||||
"owner@example.com",
|
||||
"f-1",
|
||||
"boom"));
|
||||
FileRunEvent fromProcessor = store.record(failure(FailureKind.UNKNOWN, TEAM, "f-1"));
|
||||
|
||||
int closed = store.markFilesRemoved(TEAM, "owner@example.com", List.of("f-1"));
|
||||
|
||||
assertThat(closed).isEqualTo(1);
|
||||
assertThat(store.find(mine.id(), TEAM).orElseThrow().status())
|
||||
.isEqualTo(FileRunEventStatus.FILE_REMOVED);
|
||||
assertThat(store.find(theirs.id(), TEAM).orElseThrow().status())
|
||||
.as("another person's incident about their own file")
|
||||
.isEqualTo(FileRunEventStatus.NEW);
|
||||
assertThat(store.find(otherTeam.id(), OTHER_TEAM).orElseThrow().status())
|
||||
.as("another team entirely")
|
||||
.isEqualTo(FileRunEventStatus.NEW);
|
||||
assertThat(store.find(fromProcessor.id(), TEAM).orElseThrow().status())
|
||||
.as("nothing was deleted from an editor here")
|
||||
.isEqualTo(FileRunEventStatus.NEW);
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("a row already closed by a reviewer is left as they left it")
|
||||
void markFilesRemovedLeavesClosedRowsAlone() {
|
||||
FileRunEvent event =
|
||||
store.record(
|
||||
RecordFailure.forEditor(
|
||||
FailureKind.UNKNOWN, TEAM, "owner@example.com", "f-1", "boom"));
|
||||
store.applyStatus(event.id(), TEAM, FileRunEventStatus.DISMISSED, "reviewer@example.com");
|
||||
|
||||
assertThat(store.markFilesRemoved(TEAM, "owner@example.com", List.of("f-1"))).isZero();
|
||||
assertThat(store.find(event.id(), TEAM).orElseThrow().statusActor())
|
||||
.isEqualTo("reviewer@example.com");
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("the kind filter applies before the limit, not after")
|
||||
void kindFilterAppliesBeforeTheLimit() {
|
||||
|
||||
+19
@@ -298,6 +298,25 @@ class FileRunEventStoreTest {
|
||||
.containsExactly("unteamed");
|
||||
}
|
||||
|
||||
@Test
|
||||
void dismissingARowClearsItFromTheDefaultQueue() {
|
||||
// The reviewer's whole complaint: without this, dismissing changes the buttons and
|
||||
// leaves the row sitting there, so the list can only ever grow.
|
||||
FileRunEvent event = store.record(failure(FailureKind.UNKNOWN, TEAM, "f1", "boom"));
|
||||
store.applyStatus(event.id(), TEAM, FileRunEventStatus.DISMISSED, "reviewer");
|
||||
|
||||
assertThat(store.list(TEAM, null, null, 10)).isEmpty();
|
||||
assertThat(store.list(TEAM, FileRunEventStatus.DISMISSED, null, 10)).hasSize(1);
|
||||
}
|
||||
|
||||
@Test
|
||||
void anAcknowledgedRowIsStillOpenWorkSoItStays() {
|
||||
FileRunEvent event = store.record(failure(FailureKind.UNKNOWN, TEAM, "f1", "boom"));
|
||||
store.applyStatus(event.id(), TEAM, FileRunEventStatus.ACKNOWLEDGED, "reviewer");
|
||||
|
||||
assertThat(store.list(TEAM, null, null, 10)).hasSize(1);
|
||||
}
|
||||
|
||||
@Test
|
||||
void filtersByStatus() {
|
||||
FileRunEvent open = store.record(failure(FailureKind.UNKNOWN, TEAM, "open", "a"));
|
||||
|
||||
+41
-10
@@ -6,6 +6,7 @@ import java.util.Collection;
|
||||
import java.util.HashMap;
|
||||
import java.util.List;
|
||||
import java.util.Map;
|
||||
import java.util.Objects;
|
||||
import java.util.Optional;
|
||||
import java.util.function.Function;
|
||||
|
||||
@@ -53,16 +54,6 @@ class InMemoryFileRunEventRepository implements FileRunEventRepository {
|
||||
return kindId == null || kindId.equals(entity.getKindId());
|
||||
}
|
||||
|
||||
@Override
|
||||
public List<FileRunEventEntity> findByTeam(Long teamId, String kindId, Pageable pageable) {
|
||||
return page(
|
||||
newestFirst(
|
||||
rows.values().stream()
|
||||
.filter(e -> sameTeam(e, teamId) && sameKind(e, kindId))
|
||||
.toList()),
|
||||
pageable);
|
||||
}
|
||||
|
||||
@Override
|
||||
public List<FileRunEventEntity> findByTeamAndStatus(
|
||||
Long teamId, FileRunEventStatus status, String kindId, Pageable pageable) {
|
||||
@@ -124,6 +115,46 @@ class InMemoryFileRunEventRepository implements FileRunEventRepository {
|
||||
return 1;
|
||||
}
|
||||
|
||||
@Override
|
||||
public int markFilesRemoved(
|
||||
Long teamId,
|
||||
String actor,
|
||||
Collection<String> fileIds,
|
||||
Instant now,
|
||||
Collection<FileRunEventStatus> allowedFrom) {
|
||||
int closed = 0;
|
||||
for (FileRunEventEntity entity : rows.values()) {
|
||||
if (entity.getOrigin() != FailureOrigin.TOOL
|
||||
|| !sameTeam(entity, teamId)
|
||||
|| !Objects.equals(entity.getActor(), actor)
|
||||
|| entity.getFileId() == null
|
||||
|| !fileIds.contains(entity.getFileId())
|
||||
|| !allowedFrom.contains(entity.getStatus())) {
|
||||
continue;
|
||||
}
|
||||
entity.setStatus(FileRunEventStatus.FILE_REMOVED);
|
||||
entity.setStatusActor(actor);
|
||||
entity.setStatusAt(now);
|
||||
closed++;
|
||||
}
|
||||
return closed;
|
||||
}
|
||||
|
||||
@Override
|
||||
public List<FileRunEventEntity> findByTeamAndStatusIn(
|
||||
Long teamId, List<FileRunEventStatus> statuses, String kindId, Pageable pageable) {
|
||||
return page(
|
||||
newestFirst(
|
||||
rows.values().stream()
|
||||
.filter(
|
||||
e ->
|
||||
sameTeam(e, teamId)
|
||||
&& statuses.contains(e.getStatus())
|
||||
&& sameKind(e, kindId))
|
||||
.toList()),
|
||||
pageable);
|
||||
}
|
||||
|
||||
@Override
|
||||
public List<FileRunEventEntity> findByTeamAndDedupKey(
|
||||
Long teamId, String dedupKey, Pageable pageable) {
|
||||
|
||||
+86
-14
@@ -89,8 +89,9 @@ class PolicyFailureRecorderTest {
|
||||
recorder.recordRunFailure(
|
||||
"run-1",
|
||||
"policy-1",
|
||||
"dana@example.com",
|
||||
null,
|
||||
null,
|
||||
"dana@example.com",
|
||||
"Policy run failed: locked",
|
||||
passwordFailure());
|
||||
|
||||
@@ -114,8 +115,9 @@ class PolicyFailureRecorderTest {
|
||||
recorder.recordRunFailure(
|
||||
"run-1",
|
||||
"policy-1",
|
||||
"dana@example.com",
|
||||
null,
|
||||
null,
|
||||
"dana@example.com",
|
||||
"Policy run failed: something we do not recognise",
|
||||
new RuntimeException("boom"));
|
||||
|
||||
@@ -133,6 +135,7 @@ class PolicyFailureRecorderTest {
|
||||
"policy-1",
|
||||
null,
|
||||
null,
|
||||
null,
|
||||
"Policy run failed: java.lang.NullPointerException",
|
||||
new RuntimeException("npe"));
|
||||
|
||||
@@ -147,7 +150,7 @@ class PolicyFailureRecorderTest {
|
||||
when(policyStore.get("policy-1")).thenReturn(Optional.of(policy("policy-1", TEAM)));
|
||||
|
||||
recorder.recordRunFailureAs(
|
||||
FailureKind.UNKNOWN, "run-3", "policy-1", null, "could not be queued");
|
||||
FailureKind.UNKNOWN, "run-3", "policy-1", null, null, "could not be queued");
|
||||
|
||||
assertThat(store.list(TEAM, null, null, 10)).hasSize(1);
|
||||
}
|
||||
@@ -161,9 +164,21 @@ class PolicyFailureRecorderTest {
|
||||
when(policyStore.get("policy-1")).thenReturn(Optional.of(policy("policy-1", TEAM)));
|
||||
|
||||
recorder.recordRunFailure(
|
||||
"run-1", "policy-1", "dana@example.com", null, "locked", passwordFailure());
|
||||
"run-1",
|
||||
"policy-1",
|
||||
null,
|
||||
null,
|
||||
"dana@example.com",
|
||||
"locked",
|
||||
passwordFailure());
|
||||
recorder.recordRunFailure(
|
||||
"run-2", "policy-1", "dana@example.com", null, "locked", passwordFailure());
|
||||
"run-2",
|
||||
"policy-1",
|
||||
null,
|
||||
null,
|
||||
"dana@example.com",
|
||||
"locked",
|
||||
passwordFailure());
|
||||
|
||||
List<FileRunEvent> events = store.list(TEAM, null, null, 10);
|
||||
assertThat(events).hasSize(2);
|
||||
@@ -173,14 +188,62 @@ class PolicyFailureRecorderTest {
|
||||
.containsExactlyInAnyOrder("run-1", "run-2");
|
||||
}
|
||||
|
||||
@Test
|
||||
void namesTheSourceWhenNoUserWasInvolved() {
|
||||
// An unattended file has no actor, so the source is the only attribution a reviewer
|
||||
// gets: which bucket, folder or webhook fed the run.
|
||||
when(policyStore.get("policy-1")).thenReturn(Optional.of(policy("policy-1", TEAM)));
|
||||
|
||||
recorder.recordRunFailure(
|
||||
"run-1",
|
||||
"policy-1",
|
||||
"src-s3-invoices",
|
||||
null,
|
||||
null,
|
||||
"locked",
|
||||
passwordFailure());
|
||||
|
||||
FileRunEvent event = store.list(TEAM, null, null, 10).getFirst();
|
||||
assertThat(event.sourceId()).isEqualTo("src-s3-invoices");
|
||||
assertThat(event.actor()).isNull();
|
||||
}
|
||||
|
||||
@Test
|
||||
void keepsTwoSourcesApartEvenWhenTheyFailIdentically() {
|
||||
// Same kind, same policy, no user on either: without the source they would be one row.
|
||||
when(policyStore.get("policy-1")).thenReturn(Optional.of(policy("policy-1", TEAM)));
|
||||
|
||||
recorder.recordRunFailureAs(
|
||||
FailureKind.UNKNOWN, "run-1", "policy-1", "src-a", null, "unreachable");
|
||||
recorder.recordRunFailureAs(
|
||||
FailureKind.UNKNOWN, "run-2", "policy-1", "src-b", null, "unreachable");
|
||||
|
||||
assertThat(store.list(TEAM, null, null, 10))
|
||||
.hasSize(2)
|
||||
.extracting(FileRunEvent::sourceId)
|
||||
.containsExactlyInAnyOrder("src-a", "src-b");
|
||||
}
|
||||
|
||||
@Test
|
||||
void thatSameRunFailingTwiceStaysOneIncident() {
|
||||
when(policyStore.get("policy-1")).thenReturn(Optional.of(policy("policy-1", TEAM)));
|
||||
|
||||
recorder.recordRunFailure(
|
||||
"run-1", "policy-1", "dana@example.com", null, "locked", passwordFailure());
|
||||
"run-1",
|
||||
"policy-1",
|
||||
null,
|
||||
null,
|
||||
"dana@example.com",
|
||||
"locked",
|
||||
passwordFailure());
|
||||
recorder.recordRunFailure(
|
||||
"run-1", "policy-1", "dana@example.com", null, "locked", passwordFailure());
|
||||
"run-1",
|
||||
"policy-1",
|
||||
null,
|
||||
null,
|
||||
"dana@example.com",
|
||||
"locked",
|
||||
passwordFailure());
|
||||
|
||||
assertThat(store.list(TEAM, null, null, 10))
|
||||
.singleElement()
|
||||
@@ -198,7 +261,7 @@ class PolicyFailureRecorderTest {
|
||||
when(policyStore.get("policy-1")).thenReturn(Optional.of(policy("policy-1", TEAM)));
|
||||
|
||||
recorder.recordRunFailure(
|
||||
"run-1", "policy-1", null, null, "boom", new RuntimeException());
|
||||
"run-1", "policy-1", null, null, null, "boom", new RuntimeException());
|
||||
|
||||
assertThat(store.list(TEAM, null, null, 10)).hasSize(1);
|
||||
}
|
||||
@@ -207,7 +270,8 @@ class PolicyFailureRecorderTest {
|
||||
void leavesAnAdHocRunUnteamedRatherThanGuessing() {
|
||||
// No stored policy means no team to attribute it to. Recorded unteamed rather than
|
||||
// attributed to whichever team happened to be nearby.
|
||||
recorder.recordRunFailure("run-1", null, null, null, "boom", new RuntimeException());
|
||||
recorder.recordRunFailure(
|
||||
"run-1", null, null, null, null, "boom", new RuntimeException());
|
||||
|
||||
assertThat(store.list(null, null, null, 10)).hasSize(1);
|
||||
assertThat(store.list(TEAM, null, null, 10)).isEmpty();
|
||||
@@ -224,6 +288,7 @@ class PolicyFailureRecorderTest {
|
||||
"policy-1",
|
||||
null,
|
||||
null,
|
||||
null,
|
||||
"boom",
|
||||
new RuntimeException()))
|
||||
.doesNotThrowAnyException();
|
||||
@@ -258,6 +323,7 @@ class PolicyFailureRecorderTest {
|
||||
"policy-1",
|
||||
null,
|
||||
null,
|
||||
null,
|
||||
"Policy run failed: locked",
|
||||
passwordFailure()))
|
||||
.doesNotThrowAnyException();
|
||||
@@ -270,7 +336,13 @@ class PolicyFailureRecorderTest {
|
||||
assertThatCode(
|
||||
() ->
|
||||
recorder.recordRunFailure(
|
||||
"run-1", "policy-1", null, null, "no cause", null))
|
||||
"run-1",
|
||||
"policy-1",
|
||||
null,
|
||||
null,
|
||||
null,
|
||||
"no cause",
|
||||
null))
|
||||
.doesNotThrowAnyException();
|
||||
assertThat(store.list(TEAM, null, null, 10).getFirst().kind())
|
||||
.isEqualTo(FailureKind.UNKNOWN);
|
||||
@@ -286,9 +358,9 @@ class PolicyFailureRecorderTest {
|
||||
when(policyStore.get("policy-1")).thenReturn(Optional.of(policy("policy-1", TEAM)));
|
||||
|
||||
recorder.recordRunFailure(
|
||||
"run-1", "policy-1", null, null, "boom", new IOException("x"));
|
||||
"run-1", "policy-1", null, null, null, "boom", new IOException("x"));
|
||||
recorder.recordRunFailure(
|
||||
"run-1", "policy-1", null, null, "boom", new IOException("x"));
|
||||
"run-1", "policy-1", null, null, null, "boom", new IOException("x"));
|
||||
|
||||
List<FileRunEvent> events = store.list(TEAM, null, null, 10);
|
||||
assertThat(events).hasSize(1);
|
||||
@@ -301,9 +373,9 @@ class PolicyFailureRecorderTest {
|
||||
when(policyStore.get("policy-1")).thenReturn(Optional.of(policy("policy-1", TEAM)));
|
||||
|
||||
recorder.recordRunFailure(
|
||||
"run-1", "policy-1", null, null, "boom", new IOException("x"));
|
||||
"run-1", "policy-1", null, null, null, "boom", new IOException("x"));
|
||||
recorder.recordRunFailure(
|
||||
"run-2", "policy-1", null, null, "boom", new IOException("x"));
|
||||
"run-2", "policy-1", null, null, null, "boom", new IOException("x"));
|
||||
|
||||
assertThat(store.list(TEAM, null, null, 10)).hasSize(2);
|
||||
}
|
||||
|
||||
+8
-1
@@ -24,7 +24,14 @@ class RecordFailurePrivacyTest {
|
||||
|
||||
private static RecordFailure withDetail(String detail) {
|
||||
return RecordFailure.forRun(
|
||||
FailureKind.UNKNOWN, 1L, "dana@example.com", "policy-1", "run-1", null, detail);
|
||||
FailureKind.UNKNOWN,
|
||||
1L,
|
||||
"dana@example.com",
|
||||
"policy-1",
|
||||
"run-1",
|
||||
null,
|
||||
null,
|
||||
detail);
|
||||
}
|
||||
|
||||
@Test
|
||||
|
||||
+7
-5
@@ -216,7 +216,7 @@ class PolicyControllerTest {
|
||||
}
|
||||
|
||||
private static PolicyRunHandle handle(String runId) {
|
||||
PolicyRun run = new PolicyRun(runId, null, definitionWithStep(), null);
|
||||
PolicyRun run = new PolicyRun(runId, null, definitionWithStep(), null, null);
|
||||
return new PolicyRunHandle(runId, CompletableFuture.completedFuture(run));
|
||||
}
|
||||
|
||||
@@ -317,7 +317,7 @@ class PolicyControllerTest {
|
||||
@Test
|
||||
@DisplayName("returns the run view when present")
|
||||
void found() {
|
||||
PolicyRun run = new PolicyRun("run-3", null, definitionWithStep(), null);
|
||||
PolicyRun run = new PolicyRun("run-3", null, definitionWithStep(), null, null);
|
||||
when(runRegistry.get("run-3")).thenReturn(run);
|
||||
|
||||
ResponseEntity<PolicyRunView> response = controller.status("run-3");
|
||||
@@ -346,9 +346,11 @@ class PolicyControllerTest {
|
||||
@Test
|
||||
@DisplayName("excludes ad-hoc runs and runs owned by others")
|
||||
void filtersRuns() {
|
||||
PolicyRun adHoc = new PolicyRun("adhoc", null, definitionWithStep(), null);
|
||||
PolicyRun ownedStored = new PolicyRun("owned", "policy-A", definitionWithStep(), null);
|
||||
PolicyRun otherStored = new PolicyRun("other", "policy-B", definitionWithStep(), null);
|
||||
PolicyRun adHoc = new PolicyRun("adhoc", null, definitionWithStep(), null, null);
|
||||
PolicyRun ownedStored =
|
||||
new PolicyRun("owned", "policy-A", definitionWithStep(), null, null);
|
||||
PolicyRun otherStored =
|
||||
new PolicyRun("other", "policy-B", definitionWithStep(), null, null);
|
||||
when(runRegistry.all()).thenReturn(List.of(adHoc, ownedStored, otherStored));
|
||||
|
||||
// ownedByCurrentUser: strip then re-apply scope reproduces the key only for the owned
|
||||
|
||||
+36
-2
@@ -225,7 +225,41 @@ class PolicyEngineTest {
|
||||
// A failed run is recorded durably, so an admin can see it after the in-memory run expires.
|
||||
verify(failureRecorder)
|
||||
.recordRunFailure(
|
||||
eq(runId), any(), any(), any(), anyString(), any(Throwable.class));
|
||||
eq(runId), any(), any(), any(), any(), anyString(), any(Throwable.class));
|
||||
}
|
||||
|
||||
@Test
|
||||
void recordsWhichSourceFedAFailedRun() throws Exception {
|
||||
// The source is threaded onto the run so an unattended failure is attributable: there is no
|
||||
// user to name for a file that arrived from a bucket.
|
||||
when(toolMetadataService.isMultiInput(ROTATE)).thenReturn(false);
|
||||
when(internalApiClient.post(eq(ROTATE), any())).thenThrow(new RuntimeException("boom"));
|
||||
|
||||
PolicyRunHandle handle =
|
||||
engine.runPolicy(
|
||||
new Policy(
|
||||
"p1",
|
||||
"rotate",
|
||||
"owner",
|
||||
true,
|
||||
List.of(),
|
||||
List.of(new PipelineStep(ROTATE, Map.of())),
|
||||
OutputSpec.inline()),
|
||||
PolicyInputs.of(List.of(pdf("input", "input.pdf"))),
|
||||
PolicyProgressListener.NOOP,
|
||||
"src-s3-invoices",
|
||||
"file-hash-1");
|
||||
handle.completion().get(10, TimeUnit.SECONDS);
|
||||
|
||||
verify(failureRecorder)
|
||||
.recordRunFailure(
|
||||
anyString(),
|
||||
any(),
|
||||
eq("src-s3-invoices"),
|
||||
eq("file-hash-1"),
|
||||
any(),
|
||||
anyString(),
|
||||
any(Throwable.class));
|
||||
}
|
||||
|
||||
@Test
|
||||
@@ -238,7 +272,7 @@ class PolicyEngineTest {
|
||||
doThrow(new RuntimeException("event store unavailable"))
|
||||
.when(failureRecorder)
|
||||
.recordRunFailure(
|
||||
anyString(), any(), any(), any(), anyString(), any(Throwable.class));
|
||||
anyString(), any(), any(), any(), any(), anyString(), any(Throwable.class));
|
||||
|
||||
PolicyRunHandle handle =
|
||||
engine.submit(
|
||||
|
||||
+5
-1
@@ -97,7 +97,11 @@ class PolicyRunRegistryTest {
|
||||
private PolicyRun register(String runId) {
|
||||
PolicyRun run =
|
||||
new PolicyRun(
|
||||
runId, null, new PipelineDefinition(runId, List.of(), List.of()), null);
|
||||
runId,
|
||||
null,
|
||||
new PipelineDefinition(runId, List.of(), List.of()),
|
||||
null,
|
||||
null);
|
||||
registry.register(run);
|
||||
return run;
|
||||
}
|
||||
|
||||
+11
-10
@@ -78,13 +78,13 @@ class PolicyRunnerTest {
|
||||
@Test
|
||||
void runsOnceWithNoFilesWhenThePolicyHasNoSources() {
|
||||
Policy policy = policy(List.of());
|
||||
when(policyEngine.runPolicy(eq(policy), any(), any(), any()))
|
||||
when(policyEngine.runPolicy(eq(policy), any(), any(), any(), any()))
|
||||
.thenReturn(new PolicyRunHandle("r", new CompletableFuture<>()));
|
||||
|
||||
runner.run(policy);
|
||||
|
||||
ArgumentCaptor<PolicyInputs> inputs = ArgumentCaptor.forClass(PolicyInputs.class);
|
||||
verify(policyEngine).runPolicy(eq(policy), inputs.capture(), any(), any());
|
||||
verify(policyEngine).runPolicy(eq(policy), inputs.capture(), any(), any(), any());
|
||||
assertTrue(inputs.getValue().primary().isEmpty());
|
||||
// Ledger hygiene still runs: rows recorded for a generator policy's folder outputs
|
||||
// are pruned by its own sweeps rather than accumulating until the policy is deleted.
|
||||
@@ -137,12 +137,12 @@ class PolicyRunnerTest {
|
||||
List.of(
|
||||
ResolvedInput.of(PolicyInputs.of(List.of())),
|
||||
ResolvedInput.of(PolicyInputs.of(List.of()))));
|
||||
when(policyEngine.runPolicy(any(), any(), any(), any()))
|
||||
when(policyEngine.runPolicy(any(), any(), any(), any(), any()))
|
||||
.thenReturn(new PolicyRunHandle("r", new CompletableFuture<>()));
|
||||
|
||||
runner.run(policy);
|
||||
|
||||
verify(policyEngine, times(2)).runPolicy(eq(policy), any(), any(), any());
|
||||
verify(policyEngine, times(2)).runPolicy(eq(policy), any(), any(), any(), any());
|
||||
}
|
||||
|
||||
@Test
|
||||
@@ -154,7 +154,7 @@ class PolicyRunnerTest {
|
||||
when(folderSource.supports(spec)).thenReturn(true);
|
||||
when(folderSource.resolve(eq(spec), any())).thenReturn(List.of(unit));
|
||||
CompletableFuture<PolicyRun> completion = new CompletableFuture<>();
|
||||
when(policyEngine.runPolicy(any(), any(), any(), any()))
|
||||
when(policyEngine.runPolicy(any(), any(), any(), any(), any()))
|
||||
.thenReturn(new PolicyRunHandle("r", completion));
|
||||
|
||||
runner.run(policy);
|
||||
@@ -175,7 +175,7 @@ class PolicyRunnerTest {
|
||||
when(folderSource.supports(spec)).thenReturn(true);
|
||||
when(folderSource.resolve(eq(spec), any())).thenReturn(List.of(unit));
|
||||
CompletableFuture<PolicyRun> completion = new CompletableFuture<>();
|
||||
when(policyEngine.runPolicy(any(), any(), any(), any()))
|
||||
when(policyEngine.runPolicy(any(), any(), any(), any(), any()))
|
||||
.thenReturn(new PolicyRunHandle("r", completion));
|
||||
|
||||
runner.run(policy);
|
||||
@@ -224,12 +224,12 @@ class PolicyRunnerTest {
|
||||
when(folderSource.supports(spec)).thenReturn(true);
|
||||
when(folderSource.resolve(eq(spec), any()))
|
||||
.thenReturn(List.of(ResolvedInput.of(PolicyInputs.of(List.of()))));
|
||||
when(policyEngine.runPolicy(any(), any(), any(), any()))
|
||||
when(policyEngine.runPolicy(any(), any(), any(), any(), any()))
|
||||
.thenReturn(new PolicyRunHandle("r", new CompletableFuture<>()));
|
||||
|
||||
runner.run(policy, SweepKind.LIGHT);
|
||||
|
||||
verify(policyEngine).runPolicy(eq(policy), any(), any(), any());
|
||||
verify(policyEngine).runPolicy(eq(policy), any(), any(), any(), any());
|
||||
verify(processedLedger, never()).markSeen(any(), any());
|
||||
verify(processedLedger, never()).deleteUnseen(any(), anyLong());
|
||||
}
|
||||
@@ -244,12 +244,13 @@ class PolicyRunnerTest {
|
||||
when(folderSource.resolve(eq(broken), any())).thenThrow(new IOException("mount gone"));
|
||||
when(folderSource.resolve(eq(healthy), any()))
|
||||
.thenReturn(List.of(ResolvedInput.of(PolicyInputs.of(List.of()))));
|
||||
when(policyEngine.runPolicy(any(), any(), any(), any()))
|
||||
when(policyEngine.runPolicy(any(), any(), any(), any(), any()))
|
||||
.thenReturn(new PolicyRunHandle("r", new CompletableFuture<>()));
|
||||
|
||||
runner.run(policy);
|
||||
|
||||
verify(policyEngine).runPolicy(eq(policy), any(), any(), any()); // healthy source still ran
|
||||
verify(policyEngine)
|
||||
.runPolicy(eq(policy), any(), any(), any(), any()); // healthy source still ran
|
||||
verify(processedLedger, never()).deleteUnseen(any(), anyLong()); // history preserved
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user