Skip to content

fix(reporting): resolve #283 review blockers (Print Schedules — connection deadlock + bookmarks/comments/perf) - #285

Merged
gpascucci merged 2 commits into
mainfrom
fix/print-schedules-review-followups
Aug 14, 2026
Merged

fix(reporting): resolve #283 review blockers (Print Schedules — connection deadlock + bookmarks/comments/perf)#285
gpascucci merged 2 commits into
mainfrom
fix/print-schedules-review-followups

Conversation

@gpascucci

@gpascucci gpascucci commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Why

#283 (Story 20.2, combined Print Schedules PDF) was merged while the review was still CHANGES_REQUESTED, so these five review fixes — including a blocker — did not make it into main. This is the follow-up.

Fixes

  1. 🔴 Connection-pool deadlock (blocker). PrintService.render was @Transactional(readOnly=true) — the tx manager held one pooled connection for the whole method while ReportService.fillSchedule9 checked out a second raw connection. So every /print including Schedule 9 pinned 2 connections on a pool of 5; ~5 concurrent prints (year-end close) deadlock until the 30s timeout. render() is six independent read-only reads — dropped the annotation (each service manages its own tx; the Sch 9 fill borrows/releases its own connection).
  2. Single-schedule bookmark. The sectionCount > 1 gate meant a one-schedule /print had no outline (violates BR-08). JR 7.0.8 batch bookmarks only emit for >1 doc, so switched to a per-template outline anchor (bookmarkLevel=1) threaded via a bookmarkTitle fill param; standalone GET /schedule9 stays bookmark-less. New IT: single Schedule 6 → exactly one bookmark.
  3. Comments-only keeps the row identifier. With printScheduleInformation=false, printComments=true, p_do_print_body had been hiding the identifier columns. Un-gated areaLabel (Sch 6), type (Sch 7B), location (Sch 11) — matching legacy (which left them ungated) and Sch 5. New IT asserts the identifier survives.
  4. Deferred-selection logging now fires on the allSchedules path too.
  5. Perf: Schedule 9 empty pre-check uses a COUNT query instead of materializing the full record list then re-querying in the template.

Verification

421 ITs green (PrintScheduleIT incl. the two new bookmark/identity cases), checkstyle clean — verified in WSL against Testcontainers Oracle. Cherry-picked cleanly onto current main.

Resolves the review comments from @SScholefield and @paulushcgcj on #283.

🤖 Generated with Claude Code


Thanks for the PR!

Deployments, as required, will be available below:

Please create PRs in draft mode. Mark as ready to enable:

After merge, new images are deployed in:

…landed)

PR #283 (Story 20.2 combined Print Schedules PDF) was merged while Sofia's
CHANGES_REQUESTED review was still open, so these five review fixes did not make
it into main. Applying them here as a follow-up.

1. [BLOCKER] Drop @transactional(readOnly=true) from PrintService.render. The
   DataSourceTransactionManager held one pooled connection for the whole method
   while ReportService.fillSchedule9 checked out a SECOND raw connection — so
   every /print including Schedule 9 pinned 2 connections on a pool of 5; ~5
   concurrent print requests deadlocked until the 30s timeout. render() is six
   independent read-only reads (each *Service manages its own tx; the Sch 9 fill
   borrows/releases its own connection), so no method-wide tx is needed.
2. [BOOKMARKS] One bookmark per rendered schedule, including single-schedule
   prints. JR 7.0.8 batch-mode bookmarks only emit for >1 doc, so switched to a
   per-template outline anchor (bookmarkLevel=1) threaded via a bookmarkTitle
   fill parameter; renderSchedule9Pdf passes null (standalone stays bookmark-less).
   New IT: single Schedule 6 -> exactly one bookmark.
3. [COMMENTS-ONLY] Keep the row identifier visible when only comments print —
   un-gated areaLabel (Sch 6), type (Sch 7B), location (Sch 11) from
   p_do_print_body, matching legacy (which left those columns ungated) and Sch 5.
   New IT: printScheduleInformation=false + printComments=true keeps the identifier.
4. [LOG] Log deferred selections (1/2/3/4/8/10 + Mill Info) even on the
   allSchedules path (removed the suppression).
5. [PERF] Schedule 9 empty pre-check uses a COUNT query instead of materializing
   the full record list then re-querying in the template.

Verified: 421 ITs green (PrintScheduleIT incl. the 2 new bookmark/identity cases),
checkstyle clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… too

CR follow-up: logUnimplementedSelections only checked the individual deferred
flags, so a {allSchedules:true} request (individual flags unset) still didn't log
the skipped 1/2/3/4/8/10 — the exact case the review asked to cover. Include
allSchedules in the condition (the comment already described this behaviour).
@gpascucci

Copy link
Copy Markdown
Contributor Author

Self-CR (adversarial pass over the five fixes)

Reviewed each fix against its intent; all sound, and I caught + fixed one gap in my own change.

  • 🔴 Connection deadlock@Transactional removed from PrintService.render; correct — it's six independent read-only reads (each *Service runs its own tx, the Sch 9 fill borrows/releases its own connection), so no method-wide tx that would pin a second connection. Rationale documented inline.
  • Bookmarks — the batch-mode approach genuinely can't bookmark a single-doc PDF in JR 7.0.8 (JRPdfExporter gates addBookmark on items.size() > 1); the per-template outline anchor (bookmarkLevel=1, anchorNameExpression=$P{bookmarkTitle}, gated on non-null) is the right fix. Standalone GET /schedule9 passes null → no anchor. Single-schedule + combined bookmark cases both have ITs.
  • Comments-only identity — un-gated location (Sch 11), areaLabel (Sch 6), type (Sch 7B) from p_do_print_body, verified against legacy (which left those ungated) and matching Sch 5. IT added.
  • Sch 9 perfcountRecords mirrors findRecords' WHERE (category '9', mill/year) with no joins/projection; empty→404 preserved.
  • Deferred-selection log (fixed a gap) — the first pass only checked the individual deferred flags, so a pure {allSchedules:true} request still wouldn't log the skipped 1/2/3/4/8/10 — the exact case @paulushcgcj raised. Fixed in 91866dc (include allSchedules in the condition).

Verified: 421 ITs green + checkstyle clean; the log-only follow-up compiles clean (no test impact). Ready to merge — this brings main up to the reviewed 20.2 state.

@gpascucci
gpascucci merged commit c8b3dbc into main Aug 14, 2026
26 checks passed
@gpascucci
gpascucci deleted the fix/print-schedules-review-followups branch August 14, 2026 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant