zombor/pergamum
1
0
Fork 0
Code Issues Pull requests 5 Projects Releases Packages 2 Wiki Activity Actions

refactor(lint): burn down god-function exclusions for dedup/home/librarystats (bookshelf-bbsd.5) #917

Open
zombor wants to merge 1 commit from bd-bookshelf-bbsd.5 into main
pull from: bd-bookshelf-bbsd.5
merge into: zombor:main
Conversation 3 Commits 1 Files changed 6 +1250 -1143
zombor commented 2026-07-03 21:07:17 +00:00
Owner
Copy link

Summary

  • Extract helpers across internal/dedup (handler.go + service.go), internal/home/service.go, and internal/librarystats/service.go to bring 13 god-functions under the live funlen/gocyclo/nestif lint gates
  • Delete 12 .golangci.yml exclusion entries for those packages (no exclusions added — #911 rule satisfied)
  • Remove ubp.date_finished from recentlyFinishedSQL SELECT (unused in RailBook; ORDER BY works without it in SELECT on MySQL 8)

Key extractions per package

dedup/service.go: collectScanRows, buildScanNextCursor, mergeInTx

dedup/handler.go: resolveLibraryScope, parseScanParams, executeScanWithCount, executeMergeWithFiles, parseMergeAllParams, executeDeleteNonSelected, validateMergeRequestIDs, validateSourceID, parseScanCursor, parseScanLimit, wrapMoveFilesError, wrapMergeError, wrapDeleteEnqueueError, parseMergeAllCriteria, parseMergeAllStrategy, computeNonKeeperIDs, parseDeleteNonSelectedRequest, decodeMergeRequest, checkPreMoveFeasibility, renderScanResult

home/service.go: resolveHomeScope, filterLibsByAccess, resolveBookCount, fetchUserLibraryIDs, appendRailIf, recTitle, buildUserRails, buildGlobalRails, buildAllRails, scanSimpleRailRows, scanContinueReadingRows, enrichRailBooks, buildAuthorMap, applyAuthorMap, fetchDiscoverIDRange, computeDiscoverAnchor, fillDiscoverResult, fetchSeedBook, trimAndConvertSimilar

librarystats/service.go: collectionCounts, fetchCollectionCounts, collectionDists, fetchCollectionDists, fetchCollectionTopN, buildDecadeMap, peakDecadeFromMap, goldenEraStartFromMap

Test plan

  • make test passes (all packages)
  • make coverage passes (100% gate maintained)
  • golangci-lint run ./internal/dedup/... ./internal/home/... ./internal/librarystats/... — 0 issues
  • .golangci.yml diff: only deletions for the three target packages; internal/librarystats/cache.go funlen exclusion for get preserved

Closes bead bookshelf-bbsd.5 on merge.

## Summary - Extract helpers across `internal/dedup` (handler.go + service.go), `internal/home/service.go`, and `internal/librarystats/service.go` to bring 13 god-functions under the live funlen/gocyclo/nestif lint gates - Delete 12 `.golangci.yml` exclusion entries for those packages (no exclusions added — #911 rule satisfied) - Remove `ubp.date_finished` from `recentlyFinishedSQL` SELECT (unused in RailBook; ORDER BY works without it in SELECT on MySQL 8) ### Key extractions per package **dedup/service.go:** `collectScanRows`, `buildScanNextCursor`, `mergeInTx` **dedup/handler.go:** `resolveLibraryScope`, `parseScanParams`, `executeScanWithCount`, `executeMergeWithFiles`, `parseMergeAllParams`, `executeDeleteNonSelected`, `validateMergeRequestIDs`, `validateSourceID`, `parseScanCursor`, `parseScanLimit`, `wrapMoveFilesError`, `wrapMergeError`, `wrapDeleteEnqueueError`, `parseMergeAllCriteria`, `parseMergeAllStrategy`, `computeNonKeeperIDs`, `parseDeleteNonSelectedRequest`, `decodeMergeRequest`, `checkPreMoveFeasibility`, `renderScanResult` **home/service.go:** `resolveHomeScope`, `filterLibsByAccess`, `resolveBookCount`, `fetchUserLibraryIDs`, `appendRailIf`, `recTitle`, `buildUserRails`, `buildGlobalRails`, `buildAllRails`, `scanSimpleRailRows`, `scanContinueReadingRows`, `enrichRailBooks`, `buildAuthorMap`, `applyAuthorMap`, `fetchDiscoverIDRange`, `computeDiscoverAnchor`, `fillDiscoverResult`, `fetchSeedBook`, `trimAndConvertSimilar` **librarystats/service.go:** `collectionCounts`, `fetchCollectionCounts`, `collectionDists`, `fetchCollectionDists`, `fetchCollectionTopN`, `buildDecadeMap`, `peakDecadeFromMap`, `goldenEraStartFromMap` ## Test plan - [x] `make test` passes (all packages) - [x] `make coverage` passes (100% gate maintained) - [x] `golangci-lint run ./internal/dedup/... ./internal/home/... ./internal/librarystats/...` — 0 issues - [x] `.golangci.yml` diff: only deletions for the three target packages; `internal/librarystats/cache.go` funlen exclusion for `get` preserved Closes bead bookshelf-bbsd.5 on merge.
wf-detail-older-execution.png
56 KiB
recompute-match-score-kebab-open.png
109 KiB
wf-detail-older-execution.png recompute-match-score-kebab-open.png
refactor(lint): burn down god-function exclusions for dedup, home, librarystats (bookshelf-bbsd.5)
All checks were successful
/ E2E API (pull_request) Successful in 2m34s
Details
/ Integration (pull_request) Successful in 4m8s
Details
/ Lint (pull_request) Successful in 4m37s
Details
/ JS Unit Tests (pull_request) Successful in 1m42s
Details
/ Test (pull_request) Successful in 5m45s
Details
/ E2E Browser (pull_request) Successful in 3m56s
Details
4c83eed676
 
Extract helpers to bring ScanHandler, MergeHandler, MergeAllHandler,
DeleteNonSelectedHandler, Merge, Scan (dedup), ListContinueReading,
ListDiscover, ListOnDeck, ListRecommended, Get (home), and
GetCollectionStats, computePeakDecadeAndGoldenEra (librarystats)
under the live funlen/gocyclo/nestif lint gates.

Key extractions:
- dedup/service.go: collectScanRows, buildScanNextCursor, mergeInTx
- dedup/handler.go: resolveLibraryScope, parseScanParams, executeScanWithCount,
  executeMergeWithFiles, parseMergeAllParams, executeDeleteNonSelected, + wrappers
- home/service.go: resolveHomeScope, buildUserRails, buildGlobalRails, buildAllRails,
  scanSimpleRailRows, scanContinueReadingRows, enrichRailBooks, buildAuthorMap,
  applyAuthorMap, fetchDiscoverIDRange, computeDiscoverAnchor, fillDiscoverResult,
  fetchSeedBook, trimAndConvertSimilar
- librarystats/service.go: collectionCounts, fetchCollectionCounts,
  collectionDists, fetchCollectionDists, fetchCollectionTopN,
  buildDecadeMap, peakDecadeFromMap, goldenEraStartFromMap

Delete 12 .golangci.yml exclusion entries (funlen/gocyclo/nestif) for
the three packages. No exclusions added (#911 rule satisfied).

Also removes ubp.date_finished from recentlyFinishedSQL SELECT (not needed
by RailBook; ORDER BY still works without it in SELECT on MySQL 8).

Closes bead bookshelf-bbsd.5 on merge.
forgejo-actions commented 2026-07-03 21:12:48 +00:00
Copy link

Workflow Detail page screenshot (wf-detail-older-execution)

Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent.

wf-detail-older-execution

**Workflow Detail page screenshot** (wf-detail-older-execution) Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent. ![wf-detail-older-execution](/attachments/b14c0571-387f-4b8f-96f0-1fb3c260252e)
forgejo-actions commented 2026-07-03 21:13:10 +00:00
Copy link

Recompute Match Score — kebab open screenshot (recompute-match-score-kebab-open)

recompute-match-score-kebab-open

**Recompute Match Score — kebab open screenshot** (recompute-match-score-kebab-open) ![recompute-match-score-kebab-open](/attachments/0fd36410-3255-4b86-b4af-e933897e8a5c)
zombor commented 2026-07-03 21:20:30 +00:00
Author
Owner
Copy link

Security Review — bookshelf-bbsd.5 (refactor god-functions: dedup/home/librarystats)

Reviewed diff: origin/main...origin/bd-bookshelf-bbsd.5
Focus: DISK_TYPE guard, multi-user scoping, file-selection correctness, SQL injection, arch boundary.

[MINOR] internal/home/service.go:38 — filterLibsByAccess returns unfiltered libs for empty-but-non-nil userLibraryIDs

When userLibraryIDs is non-nil but empty (authenticated user with no mapped libraries), filterLibsByAccess returns the full, unfiltered libs slice instead of an empty one. The function's own doc comment acknowledges this: "caller is responsible for the fail-closed early exit before using the result."

The current caller (Get) correctly discards the returned libs via the early-exit check at line 239 (if userID != 0 && userLibraryIDs != nil && len(userLibraryIDs) == 0). No data is currently leaked.

However the contract is fragile for a library-access filter: the function silently violates its stated purpose for the empty-non-nil case and couples correctness to a caller-side invariant. A future caller that reuses filterLibsByAccess without implementing the same early exit would expose all library names to a zero-access user.

Suggested fix: return an empty (non-nil) slice when len(userLibraryIDs) == 0 && userLibraryIDs != nil, matching what buildRailQuery does for the same case (AND 1=0). The caller's early exit can then be retained as a belt-and-suspenders guard, but the function no longer relies on it for correctness.

All primary security concerns verified clean:

  • DISK_TYPE=NETWORK guard (dedup destructive ops): Preserved. checkPreMoveFeasibility in executeMergeWithFiles still probes moveFiles(ctx, MergeRequest{MoveFiles: true}) before any DB writes; wrapMoveFilesError correctly maps ErrNetworkDisk → middleware.ErrConflict. DeleteNonSelectedHandler guard is preserved via wrapDeleteEnqueueError mapping ErrNetworkDisk → ErrConflict.

  • File selection (no off-by-one deletes the kept file): computeNonKeeperIDs is a faithful extraction of the original AllIDs minus KeepIDs logic. mergeInTx operates on SourceBookIDs and writes to TargetBookID — source books are deleted, target is kept. Logic unchanged.

  • Ownership checks: resolveLibraryScope preserves the fail-closed library scope check (empty scopedIDs → ErrNotFound). Book-level ownership check inside Merge service is untouched. executeDeleteNonSelected correctly calls checkOwnership(ctx, nonKeeperIDs, userLibraryIDs) before enqueuing deletion.

  • Multi-user scoping (user_id in queries): resolveLibraryScope reads userID exclusively from extractUser(r) (session), never from request body or path params. All rail queries (listContinueReading, listRecentlyFinished, listOnDeck, listRecommended) continue to receive both userID (session-bound) and userLibraryIDs. buildRailQuery correctly injects AND 1=0 for authenticated-but-no-libraries, and AND b.library_id IN (?) for the normal case — no WHERE clause was dropped.

  • SQL injection: All fmt.Sprintf SQL construction in dedup/service.go uses strings.Join(placeholders, ", ") where placeholders are "?" strings only. No user-supplied value is interpolated into SQL text.

  • Architecture boundary: No go-workflows / wfengine import in internal/dedup, internal/home, or internal/librarystats.

  • date_finished removed from recentlyFinishedSQL SELECT: Correct. The column was selected in the original but scanned into a discarded sql.NullTime. The ORDER BY ubp.date_finished clause is retained (referencing the table column, not a SELECT alias), so ordering is unchanged. Tests updated correspondingly.

  • golangci.yml suppressions removed: Expected and positive outcome of the refactor — the extracted helpers are individually within funlen/gocyclo/nestif limits.

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

## Security Review — bookshelf-bbsd.5 (refactor god-functions: dedup/home/librarystats) Reviewed diff: `origin/main...origin/bd-bookshelf-bbsd.5` Focus: DISK_TYPE guard, multi-user scoping, file-selection correctness, SQL injection, arch boundary. --- [MINOR] internal/home/service.go:38 — filterLibsByAccess returns unfiltered libs for empty-but-non-nil userLibraryIDs When `userLibraryIDs` is non-nil but empty (authenticated user with no mapped libraries), `filterLibsByAccess` returns the full, unfiltered `libs` slice instead of an empty one. The function's own doc comment acknowledges this: "caller is responsible for the fail-closed early exit before using the result." The current caller (`Get`) correctly discards the returned `libs` via the early-exit check at line 239 (`if userID != 0 && userLibraryIDs != nil && len(userLibraryIDs) == 0`). No data is currently leaked. However the contract is fragile for a library-access filter: the function silently violates its stated purpose for the empty-non-nil case and couples correctness to a caller-side invariant. A future caller that reuses `filterLibsByAccess` without implementing the same early exit would expose all library names to a zero-access user. Suggested fix: return an empty (non-nil) slice when `len(userLibraryIDs) == 0 && userLibraryIDs != nil`, matching what `buildRailQuery` does for the same case (`AND 1=0`). The caller's early exit can then be retained as a belt-and-suspenders guard, but the function no longer relies on it for correctness. --- **All primary security concerns verified clean:** - **DISK_TYPE=NETWORK guard (dedup destructive ops):** Preserved. `checkPreMoveFeasibility` in `executeMergeWithFiles` still probes `moveFiles(ctx, MergeRequest{MoveFiles: true})` before any DB writes; `wrapMoveFilesError` correctly maps `ErrNetworkDisk` → `middleware.ErrConflict`. `DeleteNonSelectedHandler` guard is preserved via `wrapDeleteEnqueueError` mapping `ErrNetworkDisk` → `ErrConflict`. - **File selection (no off-by-one deletes the kept file):** `computeNonKeeperIDs` is a faithful extraction of the original `AllIDs minus KeepIDs` logic. `mergeInTx` operates on `SourceBookIDs` and writes to `TargetBookID` — source books are deleted, target is kept. Logic unchanged. - **Ownership checks:** `resolveLibraryScope` preserves the fail-closed library scope check (empty scopedIDs → ErrNotFound). Book-level ownership check inside `Merge` service is untouched. `executeDeleteNonSelected` correctly calls `checkOwnership(ctx, nonKeeperIDs, userLibraryIDs)` before enqueuing deletion. - **Multi-user scoping (user_id in queries):** `resolveLibraryScope` reads userID exclusively from `extractUser(r)` (session), never from request body or path params. All rail queries (`listContinueReading`, `listRecentlyFinished`, `listOnDeck`, `listRecommended`) continue to receive both `userID` (session-bound) and `userLibraryIDs`. `buildRailQuery` correctly injects `AND 1=0` for authenticated-but-no-libraries, and `AND b.library_id IN (?)` for the normal case — no WHERE clause was dropped. - **SQL injection:** All `fmt.Sprintf` SQL construction in `dedup/service.go` uses `strings.Join(placeholders, ", ")` where placeholders are `"?"` strings only. No user-supplied value is interpolated into SQL text. - **Architecture boundary:** No `go-workflows` / `wfengine` import in `internal/dedup`, `internal/home`, or `internal/librarystats`. - **`date_finished` removed from `recentlyFinishedSQL` SELECT:** Correct. The column was selected in the original but scanned into a discarded `sql.NullTime`. The `ORDER BY ubp.date_finished` clause is retained (referencing the table column, not a SELECT alias), so ordering is unchanged. Tests updated correspondingly. - **golangci.yml suppressions removed:** Expected and positive outcome of the refactor — the extracted helpers are individually within funlen/gocyclo/nestif limits. REVIEW VERDICT: 0 blocker, 0 major, 1 minor
All checks were successful
/ E2E API (pull_request) Successful in 2m34s
Required
Details
/ Integration (pull_request) Successful in 4m8s
Required
Details
/ Lint (pull_request) Successful in 4m37s
Required
Details
/ JS Unit Tests (pull_request) Successful in 1m42s
Details
/ Test (pull_request) Successful in 5m45s
Required
Details
/ E2E Browser (pull_request) Successful in 3m56s
Required
Details
This pull request is blocked because it's outdated.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin bd-bookshelf-bbsd.5:bd-bookshelf-bbsd.5
git switch bd-bookshelf-bbsd.5
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
zombor/pergamum!917
No description provided.