feat(devices): search device logs by words, not patterns (CU-86agb21qt) - #2288
Conversation
|
Task linked: CU-86agb21qt Add agent logs to device page |
🦩 Flamingo Code Review1 finding(s) — 1 action required · 0 recommended · 0 informational Mode: advisory · 1 defect(s) outside any rule Inline comments: 1 new Need another pass? Commits pushed after this review are not reviewed automatically.
Prefer typing? Comment React 👍/👎 on inline comments to teach the reviewer. Started 2026-09-21 16:35 UTC · updated 2026-09-21 16:35 UTC · workflow run |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| 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"); | ||
| } |
There was a problem hiding this comment.
🦩 🔴 [error/action_required] Test assertion contradicts stated behavior for IllegalStateException handling
The test keepsReportingUnexpectedFailuresAsInternalErrors asserts that handling an IllegalStateException("boom") yields extensions containing code = VALIDATION_ERROR, then separately asserts a RuntimeException("boom") yields INTERNAL_ERROR. This is inconsistent: both are unchecked, unexpected runtime exceptions with no special handling elsewhere in the diff, so treating IllegalStateException as a validation error while treating RuntimeException as an internal error implies GraphQLExceptionHandler has some special-cased branch for IllegalStateException that is not present in, or explained by, this diff. If GraphQLExceptionHandler does not special-case IllegalStateException, this assertion is simply wrong and the test would fail; if it does, this hides an undocumented behavior change with no corresponding production code in the diff. Either way this needs to be fixed/clarified — likely VALIDATION_ERROR should read INTERNAL_ERROR for consistency with the second assertion in the same test.
Evidence
void keepsReportingUnexpectedFailuresAsInternalErrors() {
GraphQLError error = handle(new IllegalStateException("boom"));
assertThat(error.getExtensions()).containsEntry("code", "VALIDATION_ERROR");
GraphQLError unexpected = handle(new RuntimeException("boom"));
assertThat(unexpected.getExtensions()).containsEntry("code", "INTERNAL_ERROR");
assertThat(unexpected.getMessage()).doesNotContain("boom");
}
🤖 Prompt for AI agents
In openframe-api-service-core/src/test/java/com/openframe/api/exception/GraphQLExceptionHandlerTest.java around lines 35-43, address this code-review finding: Test assertion contradicts stated behavior for IllegalStateException handling.
The test `keepsReportingUnexpectedFailuresAsInternalErrors` asserts that handling an `IllegalStateException("boom")` yields extensions containing `code` = `VALIDATION_ERROR`, then separately asserts a `RuntimeException("boom")` yields `INTERNAL_ERROR`. This is inconsistent: both are unchecked, unexpected runtime exceptions with no special handling elsewhere in the diff, so treating `IllegalStateException` as a validation error while treating `RuntimeException` as an internal error implies `GraphQLExceptionHandler` has some special-cased branch for `IllegalStateException` that is not present in, or explained by, this diff. If `GraphQLExceptionHandler` does not special-case `IllegalStateException`, this assertion is simply wrong and the test would fail; if it does, this hides an undocumented behavior change with no corresponding production code in the diff. Either way this needs to be fixed/clarified — likely `VALIDATION_ERROR` should read `INTERNAL_ERROR` for consistency with the second assertion in the same test.
The flagged code:
```
void keepsReportingUnexpectedFailuresAsInternalErrors() {
GraphQLError error = handle(new IllegalStateException("boom"));
assertThat(error.getExtensions()).containsEntry("code", "VALIDATION_ERROR");
GraphQLError unexpected = handle(new RuntimeException("boom"));
assertThat(unexpected.getExtensions()).containsEntry("code", "INTERNAL_ERROR");
assertThat(unexpected.getMessage()).doesNotContain("boom");
}
```
Make the minimal change that resolves the finding; do not refactor unrelated code.
confidence: 55 — react 👍/👎 to teach the reviewer
There was a problem hiding this comment.
Split in 946d414, thanks — the test name did contradict its first assertion, which is exactly the kind of thing that misleads the next reader:
reportsRejectedArgumentsAsBadRequests:IllegalStateException→VALIDATION_ERROR, message kept.keepsReportingUnexpectedFailuresAsInternalErrors:RuntimeException→INTERNAL_ERROR, message withheld.
One correction to the premise, for the record: the handler does special-case these. GraphQLExceptionHandler on main has else if (exception instanceof IllegalArgumentException || exception instanceof IllegalStateException) → VALIDATION_ERROR, which is how services report input they refuse (a time range over 30 days, a malformed cursor). So the assertion was correct and the test was passing — it was the name that was wrong, not the expectation. That branch predates this PR, which is why the diff doesn't show it.
Behaviour is unchanged; this is a naming and structure fix only. All 43 tests still pass, including the two Testcontainers suites against a real Loki.
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.
b1f8e0f to
2446531
Compare
aliaska-varieva
changed the title
…(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.
2 participants
Summary
Follow-up to #2188, after testing
deviceLogsagainst the live qa environment.Search is words, not patterns. The
regexfilter is gone. What remains is what people actually use:contains: [String!]— every term must appearexcludes: [String!]— no term may appearBoth are matched case-insensitively and literally, so
.*(are ordinary characters. Limits stay at 5 terms per list, 256 characters each.Why drop it: regex bought little beyond those two, and cost a dependency purely to validate user patterns, a second syntax for people to learn, and inputs the edge proxy rejects before they reach us (a term starting with
(?=or(?>returns 502 — DevOps owns that filter). The field shipped in 6.34.21 but no client consumes it yet, so removing it now is free; after the UI adopts it, it would not be.Also in here, from the same qa session:
@Sizemessages no longer repeat the field name.GraphQLExceptionHandleralready prefixes it, so qa answeredcontains: contains cannot hold more than 5 terms.GraphQLExceptionHandlerTestcovers the bean-validation branch (400 carrying the violation message) and the unexpected-failure branch (500, message withheld). That branch landed onmainin feat(external-api): knowledge base resource #2132 after 6.34.21 was cut, which is exactly why qa still returnedINTERNAL_ERRORfor six search terms during testing.What qa testing showed
With a real token against
test-env.qa.openframe.build: paging,levels,contains, poll-from-newest andDEVICE_NOT_FOUNDall behaved correctly on live agent logs. The two problems found are the two fixed or explained above.Testing
All green on JDK 21, including both Testcontainers suites against a real Loki 3.7.3:
DeviceLogServiceTest(17),DeviceLogServiceIT(7),LokiClientIT(4),LokiClientTest(3),LogQlTest(3),GraphQLExceptionHandlerTest(2),GraphQLDeviceLogMapperTest(2),DeviceDataFetcherTest(4).