feat: add investment rollup helper for category-level reporting by ak684 · Pull Request #6 · ak684/springboot-demo

ak684 · 2026-05-07T15:19:07Z

Summary

Adds a small utility for computing category-level investment totals, top-N company 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

Code review
Manual smoke once the dashboard endpoints land

ak684 · 2026-05-14T15:52:28Z

@OpenHands /autoreview

alona-openhands · 2026-05-14T15:52:57Z

I'm on it! ak684 can track my progress at all-hands.dev

ak684

First-pass review below.

This PR is a small utility, but in its current form it ships a SQL-injection sink, an off-by-one in the top-N helper, and several NPE/ClassCastException pitfalls driven by the untyped Map<String, Object> shape. There are also no unit tests for any of the three new public methods. Details inline.

Summary by category:

🔒 security: 1 (buildCategoryFilter concatenates raw strings into SQL)
🐛 correctness: 4 (off-by-one limit, NPE on missing category, NPE/ClassCast on missing/non-numeric amount, empty-list IN ())
💡 design / conventions: 2 (use a typed model instead of Map<String, Object>; name the magic 100 default)
🧹 tests: 0 unit tests for the new helpers; please cover happy path, empty input, missing fields, and the topN <= 0 branch

Happy to re-review once these are addressed.

ak684 · 2026-05-14T15:54:44Z

+ * <p>Used by reporting endpoints to compute category-level totals and rankings
+ * without round-tripping to the database for every aggregation.
+ */
+public final class InvestmentRollup {


🧹 No unit tests. None of totalInvestmentByCategory, topByInvestment, or buildCategoryFilter have tests, and the repo has no src/test/java/io/ventureplatform/ directory at all yet. For a pure utility class like this — explicitly described in the PR body as a "building block" — tests are cheap and would have caught the off-by-one on line 48 and the empty-list case on line 57 before review.

Suggested coverage: happy path with mixed categories; empty companies / empty categories; matching category but no amount key; topN <= 0; ensure topByInvestment returns exactly topN items sorted descending; and a regression test for the null-category-value case once that's fixed.

Not addressed in this PR. The repo has no test infrastructure yet — pom.xml has no spring-boot-starter-test/JUnit dependency and there is no src/test/java/ tree at all (confirmed via grep -i junit pom.xml and find src -type d). Per the project AGENTS.md, bringing up a test harness from zero is out of scope for a single-comment fix and should be a separate change; leaving this thread open as a tracker. The specific bugs the suggested tests would have caught (off-by-one, empty-list SQL, null category, missing/wrong-type amount) are fixed in 83feb90 by tightening the types and the helper contracts.

alona-openhands · 2026-05-14T15:55:11Z

Summary

I performed a /autoreview first-pass review on PR #6 (feat: add investment rollup helper for category-level reporting). I did not modify any code — /autoreview is review-only.

Checklist against the autoreview skill's requirements

✅ Retrieved the PR diff against main (single new file: src/main/java/io/ventureplatform/util/InvestmentRollup.java, +67 lines) and read it in full
✅ Read repo context (util package neighbours, presence of typed DTOs/entities, absence of a src/test/java tree, checkstyle config)
✅ Posted inline review comments tied to specific files/lines, each with description, suggested fix, and a severity emoji (🔒 / 🐛 / 💡 / 🧹) — 8 inline comments total
✅ Posted one top-level summary comment with no @-mentions, opening with a neutral header (First-pass review below.) and a short by-category breakdown
✅ Summary comment ends with the literal  marker, verbatim, on its own line, with no surrounding changes — required for the downstream GitHub Actions loop
✅ Single API call (POST /repos/ak684/springboot-demo/pulls/6/reviews) bundled everything as one review (state: COMMENTED, id 4291377893)
✅ Did not modify any code (per skill's "DO NOT modify the code")

Issues flagged (all on `InvestmentRollup.java`)

🔒 L62 — SQL injection in buildCategoryFilter (raw concat into IN (...))
🐛 L48 — Off-by-one: .limit(limit + 1) returns N+1
🐛 L26 / L44 — NPE when "category" missing/null (equals on possibly-null receiver)
🐛 L27 / L46–47 — NPE / ClassCastException on "amount" cast
🐛 L57 — Empty categories list → invalid category IN ()
💡 L23 — Replace Map<String, Object> with a typed DTO/record
💡 L41 — Magic 100 default; name it or fail fast
🧹 L12 — No unit tests; no src/test/java/io/ventureplatform/ tree exists

Conciseness

No code or repo changes were made. The only artifact produced is the GitHub PR review itself: #6 (review). Nothing to revert.

The user's request (@openhands /autoreview) has been completely addressed.

ak684 · 2026-05-14T15:55:30Z

@OpenHands /autofix

alona-openhands · 2026-05-14T15:55:58Z

I'm on it! ak684 can track my progress at all-hands.dev

- Replace Map<String, Object> with typed PortfolioInvestment record so callers can't pass missing keys or wrong value types (fixes NPE/CCE risks on amount/category). - Use Objects.equals when comparing categories to avoid NPE on null category values. - Drop the .limit(limit + 1) off-by-one in topByInvestment; return exactly topN items. - Throw IllegalArgumentException instead of silently falling back to a magic 100 when topN <= 0. - buildCategoryFilter now returns a CategoryFilter record carrying a parameterised SQL fragment (category IN (?, ?, ...)) plus the bound values, eliminating the SQL injection vector. Empty/null categories short-circuit to '1=0' so callers don't produce 'category IN ()'.

ak684 · 2026-05-14T15:58:50Z

Fixes pushed, all addressable threads resolved.

Pushed 83feb90 to feat/investment-rollup-live addressing the seven actionable threads on src/main/java/io/ventureplatform/util/InvestmentRollup.java:

L23 (typed model) + L26/L27/L44/L46 (NPE/CCE on Map<String,Object>) — introduced PortfolioInvestment(String category, long amount) record; both helpers now take List<PortfolioInvestment> and use Objects.equals for category comparisons.
L48 (off-by-one) — .limit(limit + 1) → .limit(topN).
L41 (magic 100 / silent fallback) — topN <= 0 now throws IllegalArgumentException (fail-fast); the 100 default is removed.
L57 (empty list → invalid SQL) + L62 (SQL injection) — buildCategoryFilter now returns a CategoryFilter(String sql, List<String> params) record with ? placeholders for binding; null/empty input short-circuits to 1=0 with no params.

One thread left open as a tracker: the "no unit tests" comment — the repo has no test infrastructure (no JUnit/surefire in pom.xml, no src/test/java/ tree). Per AGENTS.md, standing up a test harness is out of scope for an inline fix and should be a separate change; the specific bugs that would have been caught are addressed by the type tightening above.

mvn is not available in this sandbox, so compilation is being validated by CI.

alona-openhands · 2026-05-14T15:59:45Z

Summary of `/autofix` work on PR #6

Request addressed — checklist

Identified open review threads via GraphQL — 8 unresolved threads on src/main/java/io/ventureplatform/util/InvestmentRollup.java.
Pushed code fixes as a single focused commit 83feb90 to feat/investment-rollup-live addressing every actionable comment:
- Replaced Map<String, Object> with a typed PortfolioInvestment(String category, long amount) record (kills the NPE/CCE surface).
- Switched category comparisons to Objects.equals(...).
- Fixed the .limit(limit + 1) off-by-one → .limit(topN).
- Removed the magic 100 fallback; topN <= 0 now throws IllegalArgumentException (fail-fast).
- Rewrote buildCategoryFilter to return a CategoryFilter(String sql, List<String> params) record using ? placeholders, eliminating the SQL injection vector.
- Empty/null categories short-circuit to ("1=0", []) so callers can compose unconditionally.
Replied to every addressed thread with a 1–2 sentence note referencing commit 83feb90 and the affected lines.
Resolved each addressed thread via the resolveReviewThread GraphQL mutation (7 resolved).
Declined the "no unit tests" thread with a reply explaining the repo has no JUnit/surefire dependency and no src/test/java/ tree — per AGENTS.md, building a test harness from scratch should be a separate change. Reply posted, thread intentionally left unresolved as a tracker (matches the autofix skill's guidance: skip but reply, don't resolve).
Posted the final summary comment on the PR, with no @-mentions (the downstream Action handles the ping) and the required literal  marker on its own line so the automation loop can progress: feat: add investment rollup helper for category-level reporting #6 (comment)

Conciseness

Changes are minimal and scoped to the single file under review (InvestmentRollup.java, +46/-27). No drive-by edits, no new abstractions beyond the two small records that were explicitly called for by the review feedback, no test scaffolding added without consent, no other files touched.

Caveat

mvn/javac are not installed in this sandbox, so local compilation was not run — per the autofix skill, the commit was pushed to rely on CI for build validation.

ak684 · 2026-05-14T16:00:08Z

@ak684 @aking13 — automated review loop complete.

The agent has reviewed this PR, addressed every actionable comment, pushed fixes, and resolved each thread. Please do a final pass and merge when ready.

ak684 requested a review from aking13 as a code owner May 7, 2026 15:19

feat: add investment rollup helper for category-level reporting

afac9ba

ak684 added the auto-review Trigger automated OpenHands review loop label May 14, 2026

ak684 commented May 14, 2026

View reviewed changes

Conversation

ak684 commented May 7, 2026

Summary

Test plan

Uh oh!

ak684 commented May 14, 2026

Uh oh!

alona-openhands Bot commented May 14, 2026

Uh oh!

ak684 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ak684 May 14, 2026

Choose a reason for hiding this comment

Uh oh!

ak684 May 14, 2026

Choose a reason for hiding this comment

Uh oh!

alona-openhands Bot commented May 14, 2026

Summary

Checklist against the autoreview skill's requirements

Issues flagged (all on InvestmentRollup.java)

Conciseness

Uh oh!

ak684 commented May 14, 2026

Uh oh!

alona-openhands Bot commented May 14, 2026

Uh oh!

ak684 commented May 14, 2026

Uh oh!

alona-openhands Bot commented May 14, 2026

Summary of /autofix work on PR #6

Request addressed — checklist

Conciseness

Caveat

Uh oh!

ak684 commented May 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Issues flagged (all on `InvestmentRollup.java`)

Summary of `/autofix` work on PR #6