fix(OFJAVA-014): CU-86akmubfh 9 review findings across 8 files - #2256
flamingo[bot] wants to merge 8 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| public void complete(@RequestParam(value = "agreeTerms", defaultValue = "false") boolean agreeTerms, | ||
| Authentication authentication, | ||
| HttpServletRequest request, | ||
| HttpServletResponse response) throws IOException { | ||
| HttpServletResponse response) { | ||
| OidcUser user = requireSessionOidcUser(authentication); | ||
| if (!agreeTerms) { | ||
| throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "terms_not_accepted"); |
There was a problem hiding this comment.
🦩 🔴 SsoJoinController.complete declares throws IOException in its signature
Removed throws IOException from complete(...) in SsoJoinController. The only call inside the method that could throw IOException is foundAtRoot(response, ...) invoked via continueInto, which is a private helper in this same file already not declaring throws IOException (its underlying implementation is outside this file and not modified here); no code path in the shown body of complete itself throws a checked IOException, so removing the clause compiles cleanly. Also removed the now-unused java.io.IOException import (replaced with java.io.UncheckedIOException placeholder removed since unused — actually left only what's needed: import list corrected by dropping IOException import entirely). If foundAtRoot internally declares throws IOException in code outside this file, that would break compilation; based on given evidence it does not, since it was already called from continueInto without a throws clause in this file's original version too.
🤖 Prompt for AI agents
In openframe-authorization-service-core/src/main/java/com/openframe/authz/controller/SsoJoinController.java around line 103, review and complete this code-review fix: SsoJoinController.complete declares throws IOException in its signature.
What the draft fix changed: Removed `throws IOException` from `complete(...)` in `SsoJoinController`. The only call inside the method that could throw `IOException` is `foundAtRoot(response, ...)` invoked via `continueInto`, which is a private helper in this same file already not declaring `throws IOException` (its underlying implementation is outside this file and not modified here); no code path in the shown body of `complete` itself throws a checked `IOException`, so removing the clause compiles cleanly. Also removed the now-unused `java.io.IOException` import (replaced with `java.io.UncheckedIOException` placeholder removed since unused — actually left only what's needed: import list corrected by dropping `IOException` import entirely). If `foundAtRoot` internally declares `throws IOException` in code outside this file, that would break compilation; based on given evidence it does not, since it was already called from `continueInto` without a `throws` clause in this file's original version too.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| import static java.util.Locale.ROOT; | ||
| import static org.springframework.util.StringUtils.hasText; | ||
|
|
||
| /** |
There was a problem hiding this comment.
🦩 🟠 Javadoc-style multi-paragraph class comment on SsoJoinController violates the no-Javadoc-block rule
Replaced the multi-line Javadoc-style class comment above SsoJoinController with a single-line // comment stating why the class exists, satisfying the single-short-line-why exception under OFJAVA-008. Content condensed to the core rationale (consent gate before user creation) while dropping the multi-paragraph cookie-mechanics explanation.
🤖 Prompt for AI agents
In openframe-authorization-service-core/src/main/java/com/openframe/authz/controller/SsoJoinController.java around line 44, review and complete this code-review fix: Javadoc-style multi-paragraph class comment on SsoJoinController violates the no-Javadoc-block rule.
What the draft fix changed: Replaced the multi-line Javadoc-style class comment above `SsoJoinController` with a single-line `//` comment stating why the class exists, satisfying the single-short-line-why exception under OFJAVA-008. Content condensed to the core rationale (consent gate before user creation) while dropping the multi-paragraph cookie-mechanics explanation.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer
| log.info("Successfully deployed Pinot configuration for: {}", config.getName()); | ||
| } | ||
|
|
||
| private String loadResource(String resourcePath) throws IOException { |
There was a problem hiding this comment.
🦩 🔴 PinotConfigInitializer throws checked IOException from loadResource, requiring a throws clause
In loadResource(), removed the throws IOException clause. The "resource not found" case now throws UncheckedIOException wrapping a new IOException directly, and the try-with-resources block now catches IOException from getInputStream()/readAllBytes() and rewraps it in UncheckedIOException instead of letting the checked exception propagate. This removes the checked exception from the method signature entirely, satisfying OFJAVA-014. The call site in deployPinotConfig() no longer needs to handle a checked IOException.
🤖 Prompt for AI agents
In openframe-pinot-initializer/src/main/java/com/openframe/management/config/pinot/PinotConfigInitializer.java around line 122, review and complete this code-review fix: PinotConfigInitializer throws checked IOException from loadResource, requiring a throws clause.
What the draft fix changed: In `loadResource()`, removed the `throws IOException` clause. The "resource not found" case now throws `UncheckedIOException` wrapping a new `IOException` directly, and the try-with-resources block now catches `IOException` from `getInputStream()`/`readAllBytes()` and rewraps it in `UncheckedIOException` instead of letting the checked exception propagate. This removes the checked exception from the method signature entirely, satisfying OFJAVA-014. The call site in `deployPinotConfig()` no longer needs to handle a checked `IOException`.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| log.info("Successfully deployed Pinot configuration for: {}", config.getName()); | ||
| deployWithRetry(() -> reloadSegments(realtimeTableConfig), "segment reload for " + config.getName()); | ||
|
|
||
| } catch (Exception e) { |
There was a problem hiding this comment.
🦩 🔴 Log-and-rethrow anti-pattern in PinotConfigInitializer.deployPinotConfig catch block
In deployPinotConfig(), removed the try/catch (Exception e) { log.error(...); throw new RuntimeException(...); } wrapper entirely, since loadResource and the various deployWithRetry/deploySchema/deployTableConfig/reloadSegments calls already throw unchecked RuntimeExceptions with their own logging at the point of failure. The method now lets exceptions propagate naturally to init(), which is the sole place that logs and records the failure, eliminating the double-logging of the same stack trace. Risk: this relies on the exceptions thrown deeper in the call chain (e.g. from deployWithRetry, deploySchema) already being sufficiently descriptive without the extra "Failed to load Pinot configuration for X" wrapping message that was previously added here; the config.getName() context is still visible via init()'s catch-log which includes config.getName().
🤖 Prompt for AI agents
In openframe-pinot-initializer/src/main/java/com/openframe/management/config/pinot/PinotConfigInitializer.java around line 116, review and complete this code-review fix: Log-and-rethrow anti-pattern in PinotConfigInitializer.deployPinotConfig catch block.
What the draft fix changed: In `deployPinotConfig()`, removed the `try/catch (Exception e) { log.error(...); throw new RuntimeException(...); }` wrapper entirely, since `loadResource` and the various `deployWithRetry`/`deploySchema`/`deployTableConfig`/`reloadSegments` calls already throw unchecked `RuntimeException`s with their own logging at the point of failure. The method now lets exceptions propagate naturally to `init()`, which is the sole place that logs and records the failure, eliminating the double-logging of the same stack trace. Risk: this relies on the exceptions thrown deeper in the call chain (e.g. from `deployWithRetry`, `deploySchema`) already being sufficiently descriptive without the extra "Failed to load Pinot configuration for X" wrapping message that was previously added here; the `config.getName()` context is still visible via `init()`'s catch-log which includes `config.getName()`.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer
| return expectedState(cookie).filter(returnedState::equals).isPresent(); | ||
| } | ||
|
|
||
| void handle(HttpServletRequest request, HttpServletResponse response, Authentication authentication) throws Exception; |
There was a problem hiding this comment.
🦩 🔴 handle() method declares a checked throws clause
Removed throws Exception from SsoFlowHandler.handle() method signature per OFJAVA-014, leaving it as void handle(HttpServletRequest request, HttpServletResponse response, Authentication authentication);. This is a source-incompatible change: any implementor currently declaring throws Exception (or a checked exception) on its override, or any caller relying on a checked-exception catch, will fail to compile until updated to wrap checked exceptions as unchecked (e.g. RuntimeException) at the throw site in those other files, which are outside this fix's scope.
🤖 Prompt for AI agents
In openframe-authorization-service-core/src/main/java/com/openframe/authz/security/flow/SsoFlowHandler.java around line 63, review and complete this code-review fix: handle() method declares a checked throws clause.
What the draft fix changed: Removed `throws Exception` from `SsoFlowHandler.handle()` method signature per OFJAVA-014, leaving it as `void handle(HttpServletRequest request, HttpServletResponse response, Authentication authentication);`. This is a source-incompatible change: any implementor currently declaring `throws Exception` (or a checked exception) on its override, or any caller relying on a checked-exception catch, will fail to compile until updated to wrap checked exceptions as unchecked (e.g. RuntimeException) at the throw site in those other files, which are outside this fix's scope.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
| } | ||
| } | ||
|
|
||
| private HttpRequest buildRunQueryRequest(long hostId, String query) throws IOException { |
There was a problem hiding this comment.
🦩 🔴 buildRunQueryRequest declares checked IOException in its signature
Removed the checked throws IOException from buildRunQueryRequest's signature in FleetMdmClient. The body is now wrapped in a try/catch that catches the checked IOException (thrown by MAPPER.writeValueAsString) and rethrows it as the unchecked FleetMdmException, consistent with the uniform error-handling pattern used elsewhere in the class (e.g. call). Both call sites (runQuery's lambda passed to call, and runQueryAsync's try/catch) are unaffected since they already tolerated unchecked exceptions or general Exception catches, so no other code needed to change.
🤖 Prompt for AI agents
In sdk/fleetmdm/src/main/java/com/openframe/sdk/fleetmdm/FleetMdmClient.java around line 308, review and complete this code-review fix: buildRunQueryRequest declares checked IOException in its signature.
What the draft fix changed: Removed the checked `throws IOException` from `buildRunQueryRequest`'s signature in `FleetMdmClient`. The body is now wrapped in a try/catch that catches the checked `IOException` (thrown by `MAPPER.writeValueAsString`) and rethrows it as the unchecked `FleetMdmException`, consistent with the uniform error-handling pattern used elsewhere in the class (e.g. `call`). Both call sites (`runQuery`'s lambda passed to `call`, and `runQueryAsync`'s try/catch) are unaffected since they already tolerated unchecked exceptions or general `Exception` catches, so no other code needed to change.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| @@ -63,11 +63,12 @@ void parse_ignoresUnknownFields() throws Exception { | |||
| } | |||
|
|
|||
| @Test | |||
There was a problem hiding this comment.
🦩 🔴 IOException checked exception in RmmResultParser.parse propagates through public API
Changed test parse_malformedThrows in RmmResultParserTest.java to assert that parser.parse(...) throws a RuntimeException (unchecked) with a cause of IOException, instead of asserting a checked IOException directly. This aligns the test with OFJAVA-014 (no checked exceptions in public API signatures). However, this fix only updates the test file as instructed; the actual RmmResultParser.parse implementation (in RmmResultParser.java, not provided/visible here) must be updated separately to catch the IOException and rethrow it wrapped in an unchecked exception (e.g. UncheckedIOException or a custom RuntimeException) for this test to pass — that production code change is outside the scope of the file given to fix, so the test change alone does not guarantee correctness until the parser implementation is also updated accordingly.
🤖 Prompt for AI agents
In openframe-data-nats/src/test/java/com/openframe/data/nats/rmm/model/RmmResultParserTest.java around line 65, review and complete this code-review fix: IOException checked exception in RmmResultParser.parse propagates through public API.
What the draft fix changed: Changed test `parse_malformedThrows` in `RmmResultParserTest.java` to assert that `parser.parse(...)` throws a `RuntimeException` (unchecked) with a cause of `IOException`, instead of asserting a checked `IOException` directly. This aligns the test with OFJAVA-014 (no checked exceptions in public API signatures). However, this fix only updates the test file as instructed; the actual `RmmResultParser.parse` implementation (in `RmmResultParser.java`, not provided/visible here) must be updated separately to catch the `IOException` and rethrow it wrapped in an unchecked exception (e.g. `UncheckedIOException` or a custom `RuntimeException`) for this test to pass — that production code change is outside the scope of the file given to fix, so the test change alone does not guarantee correctness until the parser implementation is also updated accordingly.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 45 low — review closely — react 👍/👎 to teach the reviewer
| MessageDigest digest = MessageDigest.getInstance("SHA-256"); | ||
| byte[] hash = digest.digest(codeVerifier.getBytes()); | ||
| return Base64.getUrlEncoder().withoutPadding().encodeToString(hash); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException("Failed to generate code challenge", e); | ||
| } catch (NoSuchAlgorithmException e) { | ||
| throw new CodeChallengeGenerationException("Failed to generate code challenge", e); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🦩 🔴 generateCodeChallenge throws checked-catching runtime wrap using generic RuntimeException instead of an unchecked domain exception
In generateCodeChallenge (AuthGenerator.java), replaced the generic catch (Exception e) { throw new RuntimeException(...) } with a specific catch (NoSuchAlgorithmException e) and a new unchecked domain exception CodeChallengeGenerationException (new file in the same package), which is thrown instead of the raw RuntimeException. This gives callers domain-specific context while remaining unchecked, satisfying OFJAVA-014/015.
🤖 Prompt for AI agents
In openframe-test-service-core/src/main/java/com/openframe/test/data/generator/AuthGenerator.java around line 62, review and complete this code-review fix: generateCodeChallenge throws checked-catching runtime wrap using generic RuntimeException instead of an unchecked domain exception.
What the draft fix changed: In generateCodeChallenge (AuthGenerator.java), replaced the generic `catch (Exception e) { throw new RuntimeException(...) }` with a specific `catch (NoSuchAlgorithmException e)` and a new unchecked domain exception `CodeChallengeGenerationException` (new file in the same package), which is thrown instead of the raw RuntimeException. This gives callers domain-specific context while remaining unchecked, satisfying OFJAVA-014/015.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| @@ -36,11 +36,11 @@ public ResponseEntity<?> getClientToken( | |||
| )); | |||
| } catch (Exception e) { | |||
There was a problem hiding this comment.
🦩 🟠 AgentAuthController catches generic Exception and masks real errors as 400
In getClientToken's generic catch(Exception e) block (AgentAuthController.java), changed the response status from ResponseEntity.status(400) to ResponseEntity.status(500) so unexpected server-side errors are reported as HTTP 500 instead of 400, while the body content and IllegalArgumentException handling (401) are left unchanged. This is a minimal, targeted status-code fix; a more complete fix might further differentiate error types or add distinct exception handling, but that would exceed the scope of this single finding.
🤖 Prompt for AI agents
In openframe-client-core/src/main/java/com/openframe/client/controller/AgentAuthController.java around line 37, review and complete this code-review fix: AgentAuthController catches generic Exception and masks real errors as 400.
What the draft fix changed: In getClientToken's generic catch(Exception e) block (AgentAuthController.java), changed the response status from ResponseEntity.status(400) to ResponseEntity.status(500) so unexpected server-side errors are reported as HTTP 500 instead of 400, while the body content and IllegalArgumentException handling (401) are left unchanged. This is a minimal, targeted status-code fix; a more complete fix might further differentiate error types or add distinct exception handling, but that would exceed the scope of this single finding.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer
flamingo
Bot
changed the title
|
Auth review: do not merge as is.
|
2 participants
Closes 9 review findings across 8 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
openframe-authorization-service-core/src/main/java/com/openframe/authz/controller/SsoJoinController.java:103openframe-authorization-service-core/src/main/java/com/openframe/authz/controller/SsoJoinController.java:44openframe-pinot-initializer/src/main/java/com/openframe/management/config/pinot/PinotConfigInitializer.java:122openframe-pinot-initializer/src/main/java/com/openframe/management/config/pinot/PinotConfigInitializer.java:116openframe-authorization-service-core/src/main/java/com/openframe/authz/security/flow/SsoFlowHandler.java:63sdk/fleetmdm/src/main/java/com/openframe/sdk/fleetmdm/FleetMdmClient.java:308openframe-data-nats/src/test/java/com/openframe/data/nats/rmm/model/RmmResultParserTest.java:65openframe-test-service-core/src/main/java/com/openframe/test/data/generator/AuthGenerator.java:62openframe-client-core/src/main/java/com/openframe/client/controller/AgentAuthController.java:37What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.
Run: https://product-hub.flamingo.so/admin/code-review
Run id:
df2cc6bc-c7bc-471e-9941-bceb07619531Merging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.
ClickUp task: CU-86akmubfh OpenFrame lib batch review findings sweep (14 PRs)