diff --git a/openframe-api-lib/src/main/java/com/openframe/api/dto/device/DeviceLogFilterCriteria.java b/openframe-api-lib/src/main/java/com/openframe/api/dto/device/DeviceLogFilterCriteria.java index 8e51536801..48a9cbda9b 100644 --- a/openframe-api-lib/src/main/java/com/openframe/api/dto/device/DeviceLogFilterCriteria.java +++ b/openframe-api-lib/src/main/java/com/openframe/api/dto/device/DeviceLogFilterCriteria.java @@ -29,11 +29,6 @@ public class DeviceLogFilterCriteria { */ private List excludes; - /** - * Opt-in RE2 pattern, applied case-insensitively. - */ - private String regex; - /** * Inclusive lower bound; defaults to {@code to} minus the default lookback. */ diff --git a/openframe-api-lib/src/main/java/com/openframe/api/service/device/DeviceLogService.java b/openframe-api-lib/src/main/java/com/openframe/api/service/device/DeviceLogService.java index 6ff675f024..082171f42e 100644 --- a/openframe-api-lib/src/main/java/com/openframe/api/service/device/DeviceLogService.java +++ b/openframe-api-lib/src/main/java/com/openframe/api/service/device/DeviceLogService.java @@ -28,8 +28,6 @@ import java.util.List; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; -import java.util.regex.Pattern; -import java.util.regex.PatternSyntaxException; import static java.util.stream.Collectors.joining; @@ -55,8 +53,6 @@ public class DeviceLogService { private static final String AGENT_LOGS_JOB = "agent-logs"; private static final String CASE_INSENSITIVE = "(?i)"; - /** Lookaround, atomic groups and backreferences: Java compiles them, Loki's RE2 engine rejects them. */ - private static final Pattern UNSUPPORTED_REGEX = Pattern.compile("\\(\\?[=!<>]|\\\\[1-9]"); private static final long NANOS_PER_SECOND = 1_000_000_000L; private final LokiClient lokiClient; @@ -124,9 +120,6 @@ static String buildQuery(String tenantDomain, String machineId, DeviceLogFilterC // Line filters before the metadata filter: the cheapest stage runs first appendTermFilters(query, " |~ ", criteria.getContains()); appendTermFilters(query, " !~ ", criteria.getExcludes()); - if (StringUtils.hasText(criteria.getRegex())) { - query.append(" |~ ").append(LogQl.quote(CASE_INSENSITIVE + criteria.getRegex())); - } return query.append(" | machine_id=").append(LogQl.quote(machineId)).toString(); } @@ -229,7 +222,6 @@ private static int pageSize(Integer requested) { private static void validateSearch(DeviceLogFilterCriteria criteria) { validateTerms(criteria.getContains(), "contains"); validateTerms(criteria.getExcludes(), "excludes"); - validateRegex(criteria.getRegex()); } /** @@ -255,26 +247,6 @@ private static void validateTerms(List terms, String field) { } } - /** - * Rejected here rather than at Loki, so a mistyped pattern reads as a bad request instead of a failed query. - */ - private static void validateRegex(String regex) { - if (!StringUtils.hasText(regex)) { - return; - } - if (regex.length() > MAX_SEARCH_LENGTH) { - throw new IllegalArgumentException("regex cannot exceed " + MAX_SEARCH_LENGTH + " characters"); - } - if (UNSUPPORTED_REGEX.matcher(regex).find()) { - throw new IllegalArgumentException("regex cannot use lookaround, atomic groups or backreferences"); - } - try { - Pattern.compile(regex); - } catch (PatternSyntaxException e) { - throw new IllegalArgumentException("regex is not valid: " + e.getDescription()); - } - } - private static void validateRange(Instant from, Instant to) { if (!from.isBefore(to)) { throw new IllegalArgumentException("'from' must be before 'to'"); diff --git a/openframe-api-lib/src/test/java/com/openframe/api/service/device/DeviceLogServiceIT.java b/openframe-api-lib/src/test/java/com/openframe/api/service/device/DeviceLogServiceIT.java index 3d948c92da..ec5434b824 100644 --- a/openframe-api-lib/src/test/java/com/openframe/api/service/device/DeviceLogServiceIT.java +++ b/openframe-api-lib/src/test/java/com/openframe/api/service/device/DeviceLogServiceIT.java @@ -179,19 +179,14 @@ void matchesSearchTextLiterallySoItCannotWidenTheQuery() { } @Test - void narrowsWithSeveralTermsExclusionsAndRegex() { + void narrowsWithSeveralTermsAndExclusions() { List allTerms = walk(MACHINE_ID, DeviceLogFilterCriteria.builder().from(FROM) .contains(List.of("connection", "failed")).build(), 500); List withoutADecade = walk(MACHINE_ID, DeviceLogFilterCriteria.builder().from(FROM) .contains(List.of("heartbeat")).excludes(List.of("line-01")).build(), 500); - List byRegex = walk(MACHINE_ID, DeviceLogFilterCriteria.builder().from(FROM) - .regex("line-00[0-9] ").build(), 500); - assertThat(allTerms).hasSize(ERROR_LINES); assertThat(withoutADecade).hasSize(DEVICE_LINES - ERROR_LINES - 10) .extracting(DeviceLogEntry::getMessage).noneMatch(message -> message.contains("line-01")); - assertThat(byRegex).hasSize(10) - .extracting(DeviceLogEntry::getMessage).first().isEqualTo("line-009 heartbeat ok"); } @Test diff --git a/openframe-api-lib/src/test/java/com/openframe/api/service/device/DeviceLogServiceTest.java b/openframe-api-lib/src/test/java/com/openframe/api/service/device/DeviceLogServiceTest.java index 99a7d2602b..c02dc21c59 100644 --- a/openframe-api-lib/src/test/java/com/openframe/api/service/device/DeviceLogServiceTest.java +++ b/openframe-api-lib/src/test/java/com/openframe/api/service/device/DeviceLogServiceTest.java @@ -248,7 +248,6 @@ void buildsOneLineFilterPerTermBeforeTheMetadataFilter() { DeviceLogFilterCriteria filter = DeviceLogFilterCriteria.builder() .contains(List.of("connection", "failed")) .excludes(List.of("heartbeat")) - .regex("fd=\\d+") .from(FROM) .to(TO) .build(); @@ -257,27 +256,21 @@ void buildsOneLineFilterPerTermBeforeTheMetadataFilter() { verify(lokiClient).queryRange( "{job=\"agent-logs\", tenant_domain=\"acme.openframe.ai\"}" - + " |~ \"(?i)connection\" |~ \"(?i)failed\" !~ \"(?i)heartbeat\" |~ \"(?i)fd=\\\\d+\"" + + " |~ \"(?i)connection\" |~ \"(?i)failed\" !~ \"(?i)heartbeat\"" + " | machine_id=\"machine-1\"", FROM_NANOS, TO_NANOS + 1, 101, LokiDirection.BACKWARD); } @Test - void rejectsTooManyTermsAndPatternsLokiCannotRun() { + void rejectsMoreThanFiveTerms() { assertThatThrownBy(() -> service.queryDeviceLogs(MACHINE_ID, DeviceLogFilterCriteria.builder().from(FROM).to(TO) .contains(List.of("a", "b", "c", "d", "e", "f")).build(), page(null, null))) .isInstanceOf(IllegalArgumentException.class); - assertThatThrownBy(() -> service.queryDeviceLogs(MACHINE_ID, DeviceLogFilterCriteria.builder().from(FROM).to(TO) - .regex("(?=lookahead)").build(), page(null, null))) - .isInstanceOf(IllegalArgumentException.class); - assertThatThrownBy(() -> service.queryDeviceLogs(MACHINE_ID, DeviceLogFilterCriteria.builder().from(FROM).to(TO) - .regex("unclosed(").build(), page(null, null))) - .isInstanceOf(IllegalArgumentException.class); verifyNoInteractions(lokiClient); } @Test - void ignoresBlankTermsAndRejectsAtomicGroups() { + void ignoresBlankTerms() { service.queryDeviceLogs(MACHINE_ID, DeviceLogFilterCriteria.builder().from(FROM).to(TO) .contains(List.of("a", "", " ", "b", "c", "d", "e")).build(), page(null, null)); @@ -287,11 +280,6 @@ void ignoresBlankTermsAndRejectsAtomicGroups() { + " |~ \"(?i)a\" |~ \"(?i)b\" |~ \"(?i)c\" |~ \"(?i)d\" |~ \"(?i)e\"" + " | machine_id=\"machine-1\"", FROM_NANOS, TO_NANOS + 1, 101, LokiDirection.BACKWARD); - - // Java compiles an atomic group, Loki's RE2 engine does not - assertThatThrownBy(() -> service.queryDeviceLogs(MACHINE_ID, DeviceLogFilterCriteria.builder().from(FROM).to(TO) - .regex("(?>atomic)").build(), page(null, null))) - .isInstanceOf(IllegalArgumentException.class); } private static DeviceLogFilterCriteria window() { diff --git a/openframe-api-service-core/src/main/java/com/openframe/api/dto/device/DeviceLogFilterInput.java b/openframe-api-service-core/src/main/java/com/openframe/api/dto/device/DeviceLogFilterInput.java index e1e28db136..2e3a4ddde1 100644 --- a/openframe-api-service-core/src/main/java/com/openframe/api/dto/device/DeviceLogFilterInput.java +++ b/openframe-api-service-core/src/main/java/com/openframe/api/dto/device/DeviceLogFilterInput.java @@ -17,14 +17,11 @@ public class DeviceLogFilterInput { private List levels; - @Size(max = 5, message = "contains cannot hold more than 5 terms") - private List<@Size(max = 256, message = "search terms cannot exceed 256 characters") String> contains; + @Size(max = 5, message = "cannot hold more than 5 terms") + private List<@Size(max = 256, message = "cannot exceed 256 characters") String> contains; - @Size(max = 5, message = "excludes cannot hold more than 5 terms") - private List<@Size(max = 256, message = "search terms cannot exceed 256 characters") String> excludes; - - @Size(max = 256, message = "regex cannot exceed 256 characters") - private String regex; + @Size(max = 5, message = "cannot hold more than 5 terms") + private List<@Size(max = 256, message = "cannot exceed 256 characters") String> excludes; private Instant from; diff --git a/openframe-api-service-core/src/main/java/com/openframe/api/mapper/GraphQLDeviceLogMapper.java b/openframe-api-service-core/src/main/java/com/openframe/api/mapper/GraphQLDeviceLogMapper.java index 5885a09fc2..4ec3ac4622 100644 --- a/openframe-api-service-core/src/main/java/com/openframe/api/mapper/GraphQLDeviceLogMapper.java +++ b/openframe-api-service-core/src/main/java/com/openframe/api/mapper/GraphQLDeviceLogMapper.java @@ -25,7 +25,6 @@ public DeviceLogFilterCriteria toFilterCriteria(DeviceLogFilterInput input) { .levels(input.getLevels()) .contains(input.getContains()) .excludes(input.getExcludes()) - .regex(input.getRegex()) .from(input.getFrom()) .to(input.getTo()) .build(); diff --git a/openframe-api-service-core/src/main/resources/schema/device-log.graphqls b/openframe-api-service-core/src/main/resources/schema/device-log.graphqls index 54f13bf541..3239d0e026 100644 --- a/openframe-api-service-core/src/main/resources/schema/device-log.graphqls +++ b/openframe-api-service-core/src/main/resources/schema/device-log.graphqls @@ -20,7 +20,6 @@ input DeviceLogFilterInput { levels: [DeviceLogLevel!] # Any of these levels; all levels when omitted contains: [String!] # All of these must appear in the message; case-insensitive, matched literally. Max 5 terms of 256 characters excludes: [String!] # None of these may appear in the message; same matching and limits - regex: String # Opt-in RE2 pattern, case-insensitive, max 256 characters. Lookaround and backreferences are rejected from: Instant # Inclusive; defaults to 7 days before `to` to: Instant # Inclusive; defaults to now. The range may not exceed 30 days } diff --git a/openframe-api-service-core/src/test/java/com/openframe/api/exception/GraphQLExceptionHandlerTest.java b/openframe-api-service-core/src/test/java/com/openframe/api/exception/GraphQLExceptionHandlerTest.java index 989774015a..4a4b402a41 100644 --- a/openframe-api-service-core/src/test/java/com/openframe/api/exception/GraphQLExceptionHandlerTest.java +++ b/openframe-api-service-core/src/test/java/com/openframe/api/exception/GraphQLExceptionHandlerTest.java @@ -1,66 +1,67 @@ package com.openframe.api.exception; -import com.openframe.core.exception.ErrorCode; -import com.openframe.core.exception.NotFoundException; +import com.openframe.api.dto.device.DeviceLogFilterInput; import graphql.GraphQLError; import graphql.execution.DataFetcherExceptionHandlerParameters; +import graphql.execution.DataFetcherExceptionHandlerResult; import jakarta.validation.ConstraintViolationException; import jakarta.validation.Validation; import jakarta.validation.Validator; -import jakarta.validation.constraints.NotBlank; import org.junit.jupiter.api.Test; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; class GraphQLExceptionHandlerTest { private final GraphQLExceptionHandler handler = new GraphQLExceptionHandler(); @Test - void constraintViolationIsAValidationErrorWithTheConstraintMessage() { - Validator validator = Validation.buildDefaultValidatorFactory().getValidator(); - ConstraintViolationException exception = new ConstraintViolationException(validator.validate(new Input(""))); + void reportsBeanValidationFailuresAsBadRequestsWithTheViolationMessage() { + DeviceLogFilterInput tooManyTerms = DeviceLogFilterInput.builder() + .contains(List.of("a", "b", "c", "d", "e", "f")) + .build(); + ConstraintViolationException exception = violationsOf(tooManyTerms); GraphQLError error = handle(exception); - assertEquals("name: name is required", error.getMessage()); - assertEquals(ErrorCode.VALIDATION_ERROR.getCode(), error.getExtensions().get("code")); - assertEquals(400, error.getExtensions().get("httpStatus")); - } - - @Test - void illegalArgumentKeepsItsMessageAsValidationError() { - GraphQLError error = handle(new IllegalArgumentException("Tag not found: t-1")); - - assertEquals("Tag not found: t-1", error.getMessage()); - assertEquals(ErrorCode.VALIDATION_ERROR.getCode(), error.getExtensions().get("code")); + assertThat(error.getExtensions()).containsEntry("code", "VALIDATION_ERROR").containsEntry("httpStatus", 400); + // The handler prefixes the offending field, so the message itself should not repeat it + assertThat(error.getMessage()).isEqualTo("contains: cannot hold more than 5 terms"); } @Test - void typedNotFoundKeepsItsCode() { - GraphQLError error = handle(new NotFoundException(ErrorCode.ORGANIZATION_NOT_FOUND, "Organization not found")); + void reportsRejectedArgumentsAsBadRequests() { + // The handler maps IllegalArgumentException and IllegalStateException to VALIDATION_ERROR: + // services throw them for input they refuse, such as a time range over 30 days + GraphQLError error = handle(new IllegalStateException("range too wide")); - assertEquals("Organization not found", error.getMessage()); - assertEquals("ORGANIZATION_NOT_FOUND", error.getExtensions().get("code")); - assertEquals(404, error.getExtensions().get("httpStatus")); + assertThat(error.getExtensions()).containsEntry("code", "VALIDATION_ERROR"); + assertThat(error.getMessage()).isEqualTo("range too wide"); } @Test - void unexpectedRuntimeExceptionDoesNotLeakItsMessage() { - GraphQLError error = handle(new RuntimeException("jdbc://secret-host is down")); + void keepsReportingUnexpectedFailuresAsInternalErrors() { + GraphQLError error = handle(new RuntimeException("boom")); - assertEquals("An unexpected error occurred. Please try again later.", error.getMessage()); - assertEquals(ErrorCode.INTERNAL_ERROR.getCode(), error.getExtensions().get("code")); + assertThat(error.getExtensions()).containsEntry("code", "INTERNAL_ERROR").containsEntry("httpStatus", 500); + assertThat(error.getMessage()).doesNotContain("boom"); } private GraphQLError handle(Throwable exception) { - DataFetcherExceptionHandlerParameters parameters = mock(DataFetcherExceptionHandlerParameters.class); - when(parameters.getException()).thenReturn(exception); - return handler.handleException(parameters).join().getErrors().getFirst(); + DataFetcherExceptionHandlerParameters parameters = DataFetcherExceptionHandlerParameters + .newExceptionParameters() + .exception(exception) + .build(); + DataFetcherExceptionHandlerResult result = handler.handleException(parameters).join(); + return result.getErrors().get(0); } - private record Input(@NotBlank(message = "name is required") String name) { + private static ConstraintViolationException violationsOf(DeviceLogFilterInput input) { + try (var factory = Validation.buildDefaultValidatorFactory()) { + Validator validator = factory.getValidator(); + return new ConstraintViolationException(validator.validate(input)); + } } }