fix(home,users): add unique tiebreakers to non-total ORDER BY clauses (bookshelf-cehm) #898

Merged

zombor merged 1 commit from bd-bookshelf-cehm into main 2026-07-03 19:25:40 +00:00

Conversation 8 Commits 1 Files changed 4 +77 -4

zombor commented 2026-07-03 12:22:40 +00:00

Owner

Copy link

Summary

Three home-dashboard rails and one users query ordered by a non-unique timestamp column then applied LIMIT — ties produce arbitrary ordering, causing both UI inconsistency and potential test flakes (the review-standard non-total-order flake pattern).

• continueReadingSQL (home/service.go:358): ORDER BY ubp.last_read_time DESC → + ubp.book_id DESC
• recentlyFinishedSQL (home/service.go:459): ORDER BY ubp.date_finished DESC → + ubp.book_id DESC
• onDeckSQL (home/service.go:893): ORDER BY us.last_series_read DESC → + b.id DESC
• GetAdminUserContentRestrictions (users/content_restriction.go:70): ORDER BY restriction_type, mode, value → + id ASC

Mirrors the existing pattern in lastReadBookSQL which already uses , ubp.book_id DESC as a tiebreaker.

Test plan

• Black-box SQL-shape tests added to each function's Describe block — capture the emitted query string and assert the tiebreaker substring is present
• make test passes
• make coverage passes (100% gate green)
• golangci-lint run ./internal/home/... ./internal/users/... — 0 issues

Closes bead bookshelf-cehm on merge.

## Summary Three home-dashboard rails and one users query ordered by a non-unique timestamp column then applied LIMIT — ties produce arbitrary ordering, causing both UI inconsistency and potential test flakes (the review-standard non-total-order flake pattern). - `continueReadingSQL` (home/service.go:358): `ORDER BY ubp.last_read_time DESC` → `+ ubp.book_id DESC` - `recentlyFinishedSQL` (home/service.go:459): `ORDER BY ubp.date_finished DESC` → `+ ubp.book_id DESC` - `onDeckSQL` (home/service.go:893): `ORDER BY us.last_series_read DESC` → `+ b.id DESC` - `GetAdminUserContentRestrictions` (users/content_restriction.go:70): `ORDER BY restriction_type, mode, value` → `+ id ASC` Mirrors the existing pattern in `lastReadBookSQL` which already uses `, ubp.book_id DESC` as a tiebreaker. ## Test plan - [x] Black-box SQL-shape tests added to each function's Describe block — capture the emitted query string and assert the tiebreaker substring is present - [x] `make test` passes - [x] `make coverage` passes (100% gate green) - [x] `golangci-lint run ./internal/home/... ./internal/users/...` — 0 issues Closes bead bookshelf-cehm on merge.

wf-detail-older-execution.png

55 KiB

recompute-match-score-kebab-open.png

109 KiB

recompute-match-score-kebab-open.png

109 KiB

wf-detail-older-execution.png

55 KiB

wf-detail-older-execution.png

55 KiB

recompute-match-score-kebab-open.png

109 KiB

zombor added 1 commit 2026-07-03 12:22:40 +00:00

fix(home,users): add unique tiebreakers to non-total ORDER BY clauses (bookshelf-cehm) ... fbe9b12dd5

Three home-dashboard rail queries and one users query sorted by a
non-unique timestamp column then LIMIT, making ties produce arbitrary
ordering (the review-standard flake pattern):

- continueReadingSQL:  ORDER BY ubp.last_read_time DESC → + ubp.book_id DESC
- recentlyFinishedSQL: ORDER BY ubp.date_finished DESC  → + ubp.book_id DESC
- onDeckSQL:           ORDER BY us.last_series_read DESC → + b.id DESC
- GetAdminUserContentRestrictions: ORDER BY restriction_type, mode, value → + id ASC

Mirrors the pattern already used in lastReadBookSQL (ubp.book_id DESC).
Black-box SQL-shape tests capture the emitted query string and assert
each tiebreaker is present.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

forgejo-actions commented 2026-07-03 12:25:28 +00:00

Copy link

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

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

**Workflow Detail page screenshot** (wf-detail-older-execution) Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent. 

forgejo-actions commented 2026-07-03 12:25:44 +00:00

Copy link

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

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

zombor force-pushed bd-bookshelf-cehm from fbe9b12dd5 to 30564da540 2026-07-03 19:03:36 +00:00 Compare

forgejo-actions commented 2026-07-03 19:06:36 +00:00

Copy link

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

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

forgejo-actions commented 2026-07-03 19:07:10 +00:00

Copy link

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

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

**Workflow Detail page screenshot** (wf-detail-older-execution) Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent. 

zombor referenced this pull request 2026-07-03 19:11:03 +00:00

fix(e2e/browser): gate 2 ungated screenshot helpers; add screenshot-policy-check guard (bookshelf-6krh) #910

zombor commented 2026-07-03 19:13:09 +00:00

Author

Owner

Copy link

Security Review — bookshelf-cehm (PR #898)

Scope: ORDER BY tiebreaker additions in internal/home/service.go and internal/users/content_restriction.go.

---

SQL Injection

All four tiebreakers (ubp.book_id DESC, b.id DESC, id ASC) are static string literals baked directly into Go var or const query strings at compile time. No user-supplied value participates in the ORDER BY construction. buildRailQuery injects only parameterized ? placeholders for library IDs, not column names. No injection surface exists.

Multi-user Scoping

WHERE clauses are untouched by the diff:

• continueReadingSQL: WHERE ubp.user_id = ? — unchanged, scoped by session userID.
• recentlyFinishedSQL: WHERE ubp.user_id = ? — unchanged, scoped by session userID.
• onDeckSQL: CTE scoped by ubp.user_id = ?; both anti-join subqueries use ubp2.user_id = ? and ubp3.user_id = ? — unchanged, all three bind to the session userID.
• GetAdminUserContentRestrictions: WHERE user_id = ? — unchanged; the function is wired in internal/users/wire.go behind d.AdminRequired with no change in this diff.

Adding a tiebreaker column to ORDER BY cannot widen a result set or cross user boundaries.

Information Leak

Ordering affects row sequence only, never row membership. The tiebreaker columns (ubp.book_id, b.id, id) are already part of the query's join graph and do not surface new data. No information leak is possible.

Architecture Boundary

internal/home/service.go imports only stdlib + internal/books + internal/library. internal/users/content_restriction.go imports only stdlib. No workflow-engine package referenced.

Test Hygiene

Both test files declare package home_test / package users_test (black-box). The query-capture tests work via the curried-function pattern: the inner BeforeEach overrides listCR / queryFn with a capturing stub; the outer JustBeforeEach calls the function, populating capturedQuery; the It block asserts on the captured string. No unexported symbols are accessed.

---

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review — bookshelf-cehm (PR #898) **Scope:** ORDER BY tiebreaker additions in `internal/home/service.go` and `internal/users/content_restriction.go`. --- ### SQL Injection All four tiebreakers (`ubp.book_id DESC`, `b.id DESC`, `id ASC`) are static string literals baked directly into Go `var` or `const` query strings at compile time. No user-supplied value participates in the ORDER BY construction. `buildRailQuery` injects only parameterized `?` placeholders for library IDs, not column names. No injection surface exists. ### Multi-user Scoping WHERE clauses are untouched by the diff: - `continueReadingSQL`: `WHERE ubp.user_id = ?` — unchanged, scoped by session userID. - `recentlyFinishedSQL`: `WHERE ubp.user_id = ?` — unchanged, scoped by session userID. - `onDeckSQL`: CTE scoped by `ubp.user_id = ?`; both anti-join subqueries use `ubp2.user_id = ?` and `ubp3.user_id = ?` — unchanged, all three bind to the session userID. - `GetAdminUserContentRestrictions`: `WHERE user_id = ?` — unchanged; the function is wired in `internal/users/wire.go` behind `d.AdminRequired` with no change in this diff. Adding a tiebreaker column to ORDER BY cannot widen a result set or cross user boundaries. ### Information Leak Ordering affects row sequence only, never row membership. The tiebreaker columns (`ubp.book_id`, `b.id`, `id`) are already part of the query's join graph and do not surface new data. No information leak is possible. ### Architecture Boundary `internal/home/service.go` imports only stdlib + `internal/books` + `internal/library`. `internal/users/content_restriction.go` imports only stdlib. No workflow-engine package referenced. ### Test Hygiene Both test files declare `package home_test` / `package users_test` (black-box). The query-capture tests work via the curried-function pattern: the inner `BeforeEach` overrides `listCR` / `queryFn` with a capturing stub; the outer `JustBeforeEach` calls the function, populating `capturedQuery`; the `It` block asserts on the captured string. No unexported symbols are accessed. --- REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-07-03 19:13:48 +00:00

Author

Owner

Copy link

CODE REVIEW: APPROVED

Phase 0 — DEMO Verification

No DEMO block in the bead. This is a pure SQL-string tiebreaker fix with no interactive output to demonstrate. The captured-query unit tests added in this PR serve as the practical verification mechanism. Proceeding per "PARTIAL with valid reason" treatment.

---

Phase 1 — Spec Compliance

Bead description lists 4 targets: continueReadingSQL, recentlyFinishedSQL, onDeckSQL in internal/home/service.go, and GetAdminUserContentRestrictions in internal/users/content_restriction.go. All 4 are present and addressed. No extra unneeded changes found.

---

Phase 2 — Code Quality

Total-order correctness

continueReadingSQL — internal/home/service.go:358
Primary: ubp.last_read_time DESC. Tiebreaker: ubp.book_id DESC.
Query filters WHERE ubp.user_id = ?; within a single user, (user_id, book_id) is unique in user_book_progress. Tiebreaker is unique within the result set. Total order achieved. ✓

recentlyFinishedSQL — internal/home/service.go:459
Primary: ubp.date_finished DESC. Tiebreaker: ubp.book_id DESC.
Same reasoning: ubp.book_id is unique per user in user_book_progress. Total order achieved. ✓

onDeckSQL — internal/home/service.go:893
Primary: us.last_series_read DESC. Tiebreaker: b.id DESC.
b.id is the book table PK — unconditionally unique across the result set. Total order achieved. ✓

GetAdminUserContentRestrictions — internal/users/content_restriction.go:70
Primary keys: restriction_type ASC, mode ASC, value ASC. Tiebreaker: id ASC.
id is the PK of user_content_restriction, unconditionally unique. Total order achieved. ✓

Other ORDER BY + LIMIT patterns in touched files verified:

• service.go:50 (lastReadBookSQL): already has ubp.book_id DESC tiebreaker — bead description noted this and left it alone. ✓
• service.go:676 (ORDER BY b.id LIMIT ?): b.id is PK, already a total order. ✓
• service.go:562 (authors batch query): ORDER BY bm.book_id, bm.sort_order with no LIMIT clause, scoped by WHERE bm.book_id IN (...). Not a flake risk for this pattern. ✓

Behavior preservation

All four tiebreakers are appended as the final key in their respective ORDER BY clauses. Primary sort semantics (recency of last read, finish date, last series read, restriction type/mode/value) are completely unchanged. ✓

Multi-user scoping

Content restriction query retains WHERE user_id = ?. Home rail queries retain WHERE ubp.user_id = ?. No scoping changes. Fail-closed. ✓

Test quality

[MINOR] internal/home/service_test.go (tiebreaker contexts for ListContinueReading, ListRecentlyFinished, ListOnDeck), internal/users/content_restriction_test.go (tiebreaker context) — Tests assert SQL string presence, not deterministic ordering behavior.

The new It blocks capture the query string and assert ContainSubstring("ubp.book_id DESC") / ContainSubstring(", id ASC"). This pins that the fix is textually in place, but does not demonstrate that two books with identical last_read_time return in book_id order. Given the curried-function stub architecture (no real DB in unit tests) this is the practical limit at the unit tier. The deterministic ordering behavior would require an integration test with deliberately tied timestamps. This does not block.

Ginkgo structure confirmed correct: outer BeforeEach sets the list function, inner BeforeEach overrides it with a capturing version, outer JustBeforeEach invokes it — capturedQuery is set before the It assertion runs. ✓

Other checks

• Black-box tests: package home_test, package users_test confirmed. ✓
• One-Expect-per-It: each new It block has exactly one Expect. ✓
• Var-at-top: var capturedQuery string declared at top of each Context. ✓
• No curried-dep convention violations. ✓

---

Verdict

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

Approved. The 1 minor (unit tests check SQL text rather than ordering behavior) is an inherent limitation of the stub-function test architecture, not a defect in the fix itself. All four tiebreakers are correct unique columns and all primary sort semantics are preserved.

## CODE REVIEW: APPROVED ### Phase 0 — DEMO Verification No DEMO block in the bead. This is a pure SQL-string tiebreaker fix with no interactive output to demonstrate. The captured-query unit tests added in this PR serve as the practical verification mechanism. Proceeding per "PARTIAL with valid reason" treatment. --- ### Phase 1 — Spec Compliance Bead description lists 4 targets: `continueReadingSQL`, `recentlyFinishedSQL`, `onDeckSQL` in `internal/home/service.go`, and `GetAdminUserContentRestrictions` in `internal/users/content_restriction.go`. All 4 are present and addressed. No extra unneeded changes found. --- ### Phase 2 — Code Quality #### Total-order correctness **`continueReadingSQL` — `internal/home/service.go:358`** Primary: `ubp.last_read_time DESC`. Tiebreaker: `ubp.book_id DESC`. Query filters `WHERE ubp.user_id = ?`; within a single user, `(user_id, book_id)` is unique in `user_book_progress`. Tiebreaker is unique within the result set. Total order achieved. ✓ **`recentlyFinishedSQL` — `internal/home/service.go:459`** Primary: `ubp.date_finished DESC`. Tiebreaker: `ubp.book_id DESC`. Same reasoning: `ubp.book_id` is unique per user in `user_book_progress`. Total order achieved. ✓ **`onDeckSQL` — `internal/home/service.go:893`** Primary: `us.last_series_read DESC`. Tiebreaker: `b.id DESC`. `b.id` is the `book` table PK — unconditionally unique across the result set. Total order achieved. ✓ **`GetAdminUserContentRestrictions` — `internal/users/content_restriction.go:70`** Primary keys: `restriction_type ASC, mode ASC, value ASC`. Tiebreaker: `id ASC`. `id` is the PK of `user_content_restriction`, unconditionally unique. Total order achieved. ✓ **Other ORDER BY + LIMIT patterns in touched files verified:** - `service.go:50` (lastReadBookSQL): already has `ubp.book_id DESC` tiebreaker — bead description noted this and left it alone. ✓ - `service.go:676` (`ORDER BY b.id LIMIT ?`): `b.id` is PK, already a total order. ✓ - `service.go:562` (authors batch query): `ORDER BY bm.book_id, bm.sort_order` with no LIMIT clause, scoped by `WHERE bm.book_id IN (...)`. Not a flake risk for this pattern. ✓ #### Behavior preservation All four tiebreakers are appended as the final key in their respective ORDER BY clauses. Primary sort semantics (recency of last read, finish date, last series read, restriction type/mode/value) are completely unchanged. ✓ #### Multi-user scoping Content restriction query retains `WHERE user_id = ?`. Home rail queries retain `WHERE ubp.user_id = ?`. No scoping changes. Fail-closed. ✓ #### Test quality [MINOR] `internal/home/service_test.go` (tiebreaker contexts for ListContinueReading, ListRecentlyFinished, ListOnDeck), `internal/users/content_restriction_test.go` (tiebreaker context) — Tests assert SQL string presence, not deterministic ordering behavior. The new `It` blocks capture the query string and assert `ContainSubstring("ubp.book_id DESC")` / `ContainSubstring(", id ASC")`. This pins that the fix is textually in place, but does not demonstrate that two books with identical `last_read_time` return in `book_id` order. Given the curried-function stub architecture (no real DB in unit tests) this is the practical limit at the unit tier. The deterministic ordering behavior would require an integration test with deliberately tied timestamps. This does not block. Ginkgo structure confirmed correct: outer `BeforeEach` sets the list function, inner `BeforeEach` overrides it with a capturing version, outer `JustBeforeEach` invokes it — `capturedQuery` is set before the `It` assertion runs. ✓ #### Other checks - **Black-box tests**: `package home_test`, `package users_test` confirmed. ✓ - **One-Expect-per-It**: each new `It` block has exactly one `Expect`. ✓ - **Var-at-top**: `var capturedQuery string` declared at top of each `Context`. ✓ - **No curried-dep convention violations.** ✓ --- ### Verdict ``` REVIEW VERDICT: 0 blocker, 0 major, 1 minor ``` Approved. The 1 minor (unit tests check SQL text rather than ordering behavior) is an inherent limitation of the stub-function test architecture, not a defect in the fix itself. All four tiebreakers are correct unique columns and all primary sort semantics are preserved.

zombor force-pushed bd-bookshelf-cehm from 30564da540 to 7e11614a3b 2026-07-03 19:18:17 +00:00 Compare

forgejo-actions commented 2026-07-03 19:21:18 +00:00

Copy link

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

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

**Workflow Detail page screenshot** (wf-detail-older-execution) Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent. 

forgejo-actions commented 2026-07-03 19:21:30 +00:00

Copy link

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

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

zombor merged commit ff2a48c5d0 into main 2026-07-03 19:25:40 +00:00

zombor referenced this pull request from a commit 2026-07-03 19:25:44 +00:00

Merge pull request 'fix(home,users): add unique tiebreakers to non-total ORDER BY clauses (bookshelf-cehm)' (#898) from bd-bookshelf-cehm into main

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

Subscribe

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!898

No description provided.