fix(OFJAVA-017): CU-86akeeczr 2 review findings across 2 files - #2086
flamingo[bot] wants to merge 2 commits into
Conversation
| return client; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🦩 🟠 PackageSearchService.clientFor throws IllegalStateException for a configuration-level defect masked as runtime data issue
In PackageSearchService's constructor, added a startup-time validation loop that iterates over all PackageManagerType enum values and throws IllegalStateException immediately if any type lacks a registered PackageManagerClient, so missing client wiring now fails fast at application boot rather than silently causing a per-request 500 via clientFor. The clientFor method itself is left unchanged (its check now becomes effectively unreachable defensive code, which is acceptable since it still documents/enforces the invariant).
🤖 Prompt for AI agents
In openframe-api-service-core/src/main/java/com/openframe/api/service/packagesearch/PackageSearchService.java around line 57, review and complete this code-review fix: PackageSearchService.clientFor throws IllegalStateException for a configuration-level defect masked as runtime data issue.
What the draft fix changed: In `PackageSearchService`'s constructor, added a startup-time validation loop that iterates over all `PackageManagerType` enum values and throws `IllegalStateException` immediately if any type lacks a registered `PackageManagerClient`, so missing client wiring now fails fast at application boot rather than silently causing a per-request 500 via `clientFor`. The `clientFor` method itself is left unchanged (its check now becomes effectively unreachable defensive code, which is acceptable since it still documents/enforces the invariant).
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer
| public void revokeTokensOfDeletedUsers() { | ||
| int revoked = 0; | ||
| int failed = 0; | ||
| for (AppleUserToken token : tokenRepository.findAll()) { |
There was a problem hiding this comment.
🦩 🟠 AppleTokenRevocationScheduler scans entire token repository unbounded and tenant-unscoped
In revokeTokensOfDeletedUsers, replaced the unbounded tokenRepository.findAll() with a paged sweep using tokenRepository.findAll(Pageable) (Spring Data's PagingAndSortingRepository method) iterating pages of size 200 until exhausted, avoiding loading the entire token collection into memory at once. This addresses the unbounded memory-load concern but does NOT address tenant scoping — AppleUserTokenRepository was not shown/available to modify with a tenant-scoped query method, so a complete fix per OPENFRAM conventions (iterating per-tenant or filtering by tenant) still requires adding a tenant-aware repository method, which is outside what could be safely inferred/added without seeing that repository interface and the TenantScoped contract in this file's scope.
🤖 Prompt for AI agents
In openframe-authorization-service-core/src/main/java/com/openframe/authz/service/sso/apple/AppleTokenRevocationScheduler.java around line 38, review and complete this code-review fix: AppleTokenRevocationScheduler scans entire token repository unbounded and tenant-unscoped.
What the draft fix changed: In `revokeTokensOfDeletedUsers`, replaced the unbounded `tokenRepository.findAll()` with a paged sweep using `tokenRepository.findAll(Pageable)` (Spring Data's `PagingAndSortingRepository` method) iterating pages of size 200 until exhausted, avoiding loading the entire token collection into memory at once. This addresses the unbounded memory-load concern but does NOT address tenant scoping — `AppleUserTokenRepository` was not shown/available to modify with a tenant-scoped query method, so a complete fix per OPENFRAM conventions (iterating per-tenant or filtering by tenant) still requires adding a tenant-aware repository method, which is outside what could be safely inferred/added without seeing that repository interface and the TenantScoped contract in this file's scope.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer
Closes 2 review findings across 2 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
openframe-api-service-core/src/main/java/com/openframe/api/service/packagesearch/PackageSearchService.java:57openframe-authorization-service-core/src/main/java/com/openframe/authz/service/sso/apple/AppleTokenRevocationScheduler.java:38What 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:
4f0d6344-55d4-49de-bc86-cac2510f432fMerging 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-86akeeczr OpenFrame lib batch review findings sweep 2 (13 PRs)