feat: add dashboard rollup helper for portfolio category summaries#5
feat: add dashboard rollup helper for portfolio category summaries#5ak684 wants to merge 2 commits into
Conversation
|
@OpenHands /autoreview |
|
I'm on it! ak684 can track my progress at all-hands.dev |
ak684
left a comment
ak684
left a comment
There was a problem hiding this comment.
First-pass review below.
Found 1 security issue, 3 bugs, 1 cleanup, and 1 suggestion in the single new file.
| Category | Count | Severity |
|---|---|---|
| SQL injection via string concatenation | 1 | 🔒 security |
Off-by-one (limit + 1) |
1 | 🐛 bug |
NullPointerException risk (.equals on nullable value) |
1 (two call-sites) | 🐛 bug |
| Empty-list produces invalid SQL | 1 | 🐛 bug |
| Javadoc inaccuracy | 1 | 🧹 cleanup |
Untyped Map<String,Object> data carrier |
1 | 💡 suggestion |
The SQL-injection and off-by-one issues should be fixed before merge. The NPE guards and empty-list check are also important for correctness.
| * given category. Returns 0 if no matches. | ||
| */ | ||
| public static long totalInvestmentByCategory( | ||
| final List<Map<String, Object>> companies, final String category) { |
There was a problem hiding this comment.
💡 suggestion — untyped data carrier
List<Map<String, Object>> relies on magic-string keys ("category", "amount") with unchecked casts to Number. This is fragile and hard to refactor safely.
Consider introducing a small record (e.g., record CompanyInvestment(String category, long amount, …)) so the compiler can catch key typos and type mismatches.
There was a problem hiding this comment.
Acknowledged — this is a good suggestion. Deferring to the follow-up PR that wires these methods into controller endpoints, since that's when we'll know the actual shape of the data coming from the repository layer. Introducing a record now would likely need to change once the callers are concrete.
SummaryThe
|
|
@OpenHands /autofix |
|
I'm on it! ak684 can track my progress at all-hands.dev |
- Fix NPE risk: flip .equals() comparisons so category (non-null) is the receiver instead of the map value (lines 31, 50) - Fix off-by-one: change .limit(limit + 1) to .limit(limit) (line 54) - Fix SQL injection: use parameterized ? placeholders via new CategoryFilter record instead of string interpolation - Fix empty-list invalid SQL: return '1=0' for empty categories - Fix inaccurate Javadoc: describe the actual IN clause output Co-authored-by: openhands <openhands@all-hands.dev>
|
Fixes pushed, all addressable threads resolved.
The 💡 suggestion (typed data carrier record) was acknowledged but deferred — it makes more sense to introduce once the controller layer callers are concrete in the follow-up PR. |
Summary of Work CompletedThe Autofix Workflow Compliance
Changes Are ConciseAll changes were confined to the single file under review (
No additional files were created or modified. The commit message and all thread replies reference the exact commit SHA. Build verification was skipped only because |
2 participants
Summary
Adds a small utility for category-level investment totals, top-N rankings, and a SQL category-filter builder used by upcoming dashboard rollup endpoints.
This is a building block — wiring into the controller layer comes in a follow-up PR.
Test plan