From 24465313db5e3e7369ef638d3a41fa12ff2b4688 Mon Sep 17 00:00:00 2001 From: Aliaska Varieva Date: Mon, 21 Sep 2026 18:41:40 +0200 Subject: [PATCH 1/2] feat(devices): search device logs by words, not patterns (CU-86agb21qt) Follow-up to #2188, after testing deviceLogs against qa. Drop the regex filter. Search is now words in, words out: `contains` requires every term, `excludes` rejects lines holding one, both matched literally and case-insensitively. Regex bought little that those two do not, and cost a dependency to validate, a second syntax for users, and patterns the edge proxy rejects before they reach the service. It shipped in 6.34.21 but no client uses it yet, so removing it now is free. Drop the field name from the @Size messages on DeviceLogFilterInput: the GraphQL error handler already prefixes the offending field, so qa answered "contains: contains cannot hold more than 5 terms". Add GraphQLExceptionHandlerTest over the bean-validation branch (400 with the violation message) and the unexpected-failure branch (500, message withheld). That branch reached main in #2132 after 6.34.21 was cut, which is why qa still returned INTERNAL_ERROR for six search terms when I tested. --- .../dto/device/DeviceLogFilterCriteria.java | 5 -- .../api/service/device/DeviceLogService.java | 28 -------- .../service/device/DeviceLogServiceIT.java | 7 +- .../service/device/DeviceLogServiceTest.java | 18 +---- .../api/dto/device/DeviceLogFilterInput.java | 11 ++-- .../api/mapper/GraphQLDeviceLogMapper.java | 1 - .../main/resources/schema/device-log.graphqls | 1 - .../GraphQLExceptionHandlerTest.java | 66 +++++++++---------- 8 files changed, 38 insertions(+), 99 deletions(-) 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..ca52d9c265 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,60 @@ 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")); - - assertEquals("Organization not found", error.getMessage()); - assertEquals("ORGANIZATION_NOT_FOUND", error.getExtensions().get("code")); - assertEquals(404, error.getExtensions().get("httpStatus")); - } + void keepsReportingUnexpectedFailuresAsInternalErrors() { + GraphQLError error = handle(new IllegalStateException("boom")); - @Test - void unexpectedRuntimeExceptionDoesNotLeakItsMessage() { - GraphQLError error = handle(new RuntimeException("jdbc://secret-host is down")); + assertThat(error.getExtensions()).containsEntry("code", "VALIDATION_ERROR"); - assertEquals("An unexpected error occurred. Please try again later.", error.getMessage()); - assertEquals(ErrorCode.INTERNAL_ERROR.getCode(), error.getExtensions().get("code")); + GraphQLError unexpected = handle(new RuntimeException("boom")); + assertThat(unexpected.getExtensions()).containsEntry("code", "INTERNAL_ERROR"); + assertThat(unexpected.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)); + } } } From 946d4147740c810c03d9fe03ce84049e6eba6d41 Mon Sep 17 00:00:00 2001 From: Aliaska Varieva Date: Mon, 21 Sep 2026 19:20:24 +0200 Subject: [PATCH 2/2] test(api): split the exception handler cases by the code they assert (CU-86agb21qt) One test asserted VALIDATION_ERROR for IllegalStateException under a name about internal errors, which reads as a contradiction. Split it: rejected arguments (IllegalArgumentException / IllegalStateException -> 400, message kept) and unexpected failures (RuntimeException -> 500, message withheld). Behaviour is unchanged; the handler already mapped both. --- .../exception/GraphQLExceptionHandlerTest.java | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) 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 ca52d9c265..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 @@ -32,14 +32,21 @@ void reportsBeanValidationFailuresAsBadRequestsWithTheViolationMessage() { } @Test - void keepsReportingUnexpectedFailuresAsInternalErrors() { - GraphQLError error = handle(new IllegalStateException("boom")); + 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")); assertThat(error.getExtensions()).containsEntry("code", "VALIDATION_ERROR"); + assertThat(error.getMessage()).isEqualTo("range too wide"); + } + + @Test + void keepsReportingUnexpectedFailuresAsInternalErrors() { + GraphQLError error = handle(new RuntimeException("boom")); - GraphQLError unexpected = handle(new RuntimeException("boom")); - assertThat(unexpected.getExtensions()).containsEntry("code", "INTERNAL_ERROR"); - assertThat(unexpected.getMessage()).doesNotContain("boom"); + assertThat(error.getExtensions()).containsEntry("code", "INTERNAL_ERROR").containsEntry("httpStatus", 500); + assertThat(error.getMessage()).doesNotContain("boom"); } private GraphQLError handle(Throwable exception) {