feat(books): multi-field sort builder + fix dead author-sort (bookshelf-oxau/ovnu) #481
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-oxau"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
?sort=author:asc,title:ascURL format with full keyset cursor pagination across composite sortsuser_settingssort_author_namewas never populated; now refreshed at every author-write site (scan, bookdrop, enrich, seeder) + migration 0030 backfills existing rowsImplementation details (oxau)
parseSort/parseMultiSort: parse and validate multi-field sort URL formatbuildMultiFieldListQuery: separate query builder for multi-term sorts; drives frombook_metadatawhen any metadata column is present,bookotherwisebuildMultiSortOrderBy/buildMultiCursorPredicate: ORDER BY and staged-OR WHERE for multi-field keyset cursorGetSortPrefs/SaveSortPrefs/SaveSortPrefsHandler: per-user default sort stored inuser_settingswith keybooks.list.sortsort_builder_controller.js: Stimulus controller for the sort builder panelImplementation details (ovnu)
RefreshSortAuthorNamesqlc query derivessort_author_namefromauthor.nameatsort_order=0for a single bookbulkRefreshSortAuthorNamebatch variant for scan/seeder pathssyncAuthors,batchSeedAuthors,SeedMetadata,SeedMetadataSeriesAndAuthors, and seederinsertAuthorMappingsTest plan
make testpasses (365 scan tests, 100% coverage)make coveragepasses — 100% gatemake lintpasses with 0 issuesinternal/db/sort_author_name_integration_test.go— single + batch refresh + migration 0030 backfillsort by author ASCasserts real ordering (Alpha Author books appear before Zeta Author books)Closes bead bookshelf-oxau on merge.
Closes bead bookshelf-ovnu on merge.
Security Review — PR #481 (bookshelf-oxau) — Multi-field sort builder + save-as-default
Scope: SQL injection via
?sort=, per-user save-as-default scoping, cursor bypass, info leak via error messages.SQL Injection analysis (PRIMARY CONCERN)
VERDICT: No SQL injection found. The ORDER BY clause is built exclusively from server-side literal column expressions in
sortKeyColumn(a closed Gomap[SortKey]string). User input from?sort=is validated againstvalidSortKeysbefore being converted to aSortKeytyped value; only those typed values are used as map keys to look up hardcoded column literals. The direction (ASC/DESC) is produced by a literalif t.Order == SortDESCbranch, never string-concatenated from user input. All cursor-predicate column expressions follow the same pattern.Full data-flow for
?sort=author:asc,title:asc:parseMultiSort→ validates each field againstvalidSortKeys, each direction against"asc"/"desc"literals → rejects anything else with 400buildMultiFieldListQuery→sortKeyColumn[t.Key](hardcoded map) → literal"ASC"/"DESC"→strings.Joincursor.SortTermsare not used to drive ORDER BY; the URL-validatedsort.Termsare used.MultiCursorVals(string-encoded sort values from the cursor) are decoded viadecodeCursorValForKeyas typed Go values (time.Time,int64,float64,string) and passed as parameterized?args — never string-interpolated.Saved sort prefs path:
GetSortPrefsre-validates every term from the DB against the samevalidSortKeysallowlist and explicitSortASC/SortDESCchecks before returning. A corrupted DB row silently falls back toDefaultSort.Findings
No blockers. No majors. Minor findings only:
[MINOR] internal/books/handler.go:449 — fmt.Errorf echoes user-supplied field name into error string
fmt.Errorf("invalid sort field %q: %w", parts[0], middleware.ErrValidation)— the raw user-supplied token is in the error message. The error handler strips it tohttp.StatusText(400)before responding, so it does NOT reach the client. However the field name appears in the structured log"error"field and could reach Seq/Sentry. For most field names (e.g."><script>) this is a non-issue (slog encodes strings), but it is worth noting: the logged string is unsanitized user input. At existing scale, acceptable; keep in mind for log-injection hygiene.[MINOR] internal/books/sort_prefs_handler.go:39 — fmt.Errorf echoes unknown sort key into error (same pattern as above)
Same observation as above — goes to log only, not to the client.
[MINOR] static/js/controllers/sort_builder_controller.js:852 —
apply()only encodes legacy?sort=+?order=in view-toggle links, not the new multi-field termsThe view-toggle
<a href=...>links inbooks_index_controls.htmlare built server-side using$sort.Key/$sort.Order(single-field legacy fields), not the full multi-term list. After a multi-field sort is applied via the JS builder (/books?sort=author:asc,title:asc), switching view mode via the toggle drops the multi-field sort back to the first-term-only legacy URL. Not a security issue; a functional regression worth a follow-up bead.Per-user scoping
PUT /books/sort-prefsretrievesuserIDfromuserIDFromRequest(r)(authenticated session) — never from the request body. The DB query (UpsertUserSetting) scopes onuser_id = ?with the session-derived value.GetSortPrefssimilarly scopes onuser_id = ?. No path exists for user A to read/write user B's sort preference. ✓CSRF
PUT /books/sort-prefsis covered by the globalmiddleware.CSRF(...)chain ininternal/app/app.go(applied to all unsafe methods). The JSsaveDefault()fetch sendsX-CSRF-Token: this._csrfToken(). ✓No Grimmory schema violation
Sort prefs stored in
user_settings(existing Grimmory table, keyed bysetting_key="books.list.sort"). No new columns added to Grimmory tables. ✓REVIEW VERDICT: 0 blocker, 0 major, 3 minor
CODE REVIEW: NOT APPROVED
Phase 0: DEMO Verification
No DEMO block was provided in the bead comments. The bead description and commit history do not include a structured DEMO block with a command and expected output to re-run. Per review policy this alone warrants NOT APPROVED, but proceeding through phases given the implementation is otherwise verifiable via the diff.
Phase 1 + Phase 2: Findings
[MAJOR] static/js/controllers/sort_builder_controller.js:100 — apply() hardcodes "/books?" and drops shelf context
The
apply()method always navigates to"/books?" + params.toString(). When the sort builder is rendered on a shelf page (/shelves/42),window.location.searchcontains noshelf_idparameter (it is part of the path, not the query string, and the shelf handler renders the page at/shelves/{id}). Clicking Apply on a shelf page navigates to/bookswithout any shelf filter, silently abandoning the shelf context. Fix: thread adata-sort-builder-base-url-valueattribute from the template (using the request URL or a computed base URL that includes the shelf), and usethis.baseUrlValue || "/books"inapply().[MAJOR] templates/pages/books_index_controls.html:4068–4078 — view-toggle links use legacy single-field sort format, silently drop secondary sort terms
Both view-toggle anchor
hrefattributes use{{$sort.Key}}and{{$sort.Order}}, which encode only the first/primary sort term in the legacy?sort=field&order=dirformat. When a multi-field sort is active (e.g.author:asc,title:asc), clicking "grid" or "table" silently drops the secondary terms and reverts to single-field?sort=author&order=asc. The template must encode the full terms list in the multi-field format (field:dir,field:dir) whenSortTermsDisplayhas more than one entry. Fix: add a template helper (or a computedSortURL stringfield inlistPageData) that produces the correct multi-field sort string, and use it in both view-toggle hrefs.The same issue appears at
templates/pages/books_index.html:316(the "Sort" link in the shelf header area) and lines 455/469 (infinite-scrolldata-infinite-scroll-next-href-valueattributes), all of which use{{if .Sort.Key}}&sort={{.Sort.Key}}{{end}}only.[MAJOR] No browser e2e test for the interactive sort builder
Per
.claude/rules/project-conventions.md("Interactive UI needs a browser test"): Stimulus controllers with toggles and fetch interactions require a go-rod e2e test that clicks the control and captures an expanded-state screenshot. The sort builder adds a panel toggle, reorder/add/remove actions, and afetchPUT for "Save as default". None of these are covered by a browser test; only an HTML-presence assertion ine2e/api/shelves_html_test.gowas added. A go-rod test ine2e/browser/is required.[MINOR] internal/books/store.go:944 —
_ = idColis dead code suppressoridColis computed viasortKeyIDCol(p.SortTerms)but never used directly inbuildMultiFieldListQuery;buildMultiSortOrderByandbuildMultiCursorPredicatecallsortKeyIDCol()themselves internally. The_ = idColline is only suppressing a compiler error. Either remove the local assignment entirely, or passidColas a parameter to both helpers to avoid the redundant call.[MINOR] internal/books/store.go:1521–1523 — comment says "NULL sorts last in ASC" but MySQL NULLs sort first in ASC
The comment reads: "NULL sorts last (treated as 'smallest' in ASC, 'largest' in DESC). In ASC: anything after a non-NULL row → col > ? (NULLs already behind us)." MySQL ORDER BY places NULLs FIRST in ASC (they are the smallest). The code behaviour is correct (after seeing a non-NULL row in ASC, all NULLs have already been consumed), but the comment is backwards and misleading. Fix the comment.
[MINOR] internal/books/handler.go:444–463 —
parseMultiSortdoes not reject duplicate sort fields?sort=title:asc,title:descis accepted and produces an ORDER BY withbm.title ASC, bm.title DESC. The JS controller prevents duplicates viaaddTerm(), but the HTTP endpoint can be called directly. TheSaveSortPrefsservice also accepts duplicates. While not a security issue, it produces undefined ORDER BY behaviour and a corrupt cursor predicate. A dedup check (matching the JS) should be added inparseMultiSortandSaveSortPrefs.[MINOR] internal/books/sort_prefs_test.go:228 — double-JustBeforeEach causes two
save()invocations per testThe "unknown sort key" context has both the outer
JustBeforeEach(invokessave()with valid author+title terms) and an innerJustBeforeEach(invokessave()with the bogus key). Ginkgo runs both in order; the test asserts onerrfrom the second call only, but the first call's result is silently discarded. The test is correct but confusing and wastes a call. Use a singleJustBeforeEachat the context level (overridesaveinBeforeEach, call once in the context's ownJustBeforeEach) or restructure to avoid the outerJustBeforeEachfor error-path contexts.Summary of injection / allowlist audit
Sort field names flow through
validSortKeys(inparseMultiSort) andGetSortPrefs(re-validates from DB on read). Column expressions inbuildMultiSortOrderByandbuildMultiCursorPredicateare drawn exclusively from thesortKeyColumnmap keyed by the validatedSortKeyenum. No raw user input is interpolated into SQL. No SQL injection vector found.Tiebreaker / total-order audit
buildMultiSortOrderByalways appends the id column as a final term (eitherbm.book_idorb.iddepending on the join).buildMultiCursorPredicatealways includes a final id-comparison branch. The id direction mirrorsterms[0].Orderconsistently in both places. Total-order requirement is met.Cursor round-trip audit
Multi-field cursors encode all term values in
SortVals(viaencodeSortVal→buildCursor) and decode them viadecodeCursorValForKeyinbuildMultiFieldListQuery. The service correctly routes toparams.MultiCursorValswhencursor.SortTermsis populated. Cursor round-trips correctly for all supported key types.Per-user scoping
GetUserSettingandUpsertUserSettingqueries scope byuser_id.GetSortPrefs/SaveSortPrefsreceiveuserIDfrom the authenticated session (viauserIDFromRequest) not from the request body. Multi-user scoping is correct.check-coverage.sh
No new exclusions were added. The
_ = idColdead code note is a MINOR issue above.REVIEW VERDICT: 0 blockers, 3 major, 4 minor
- [MAJOR] apply() now uses data-sort-builder-base-url-value so shelf pages preserve their /shelves/{id}/books path context instead of always going to /books - [MAJOR] add SortParam + BaseURL to listPageData; all view-toggle links, infinite-scroll sentinel, and load-more anchor now use {{.SortParam}} so multi-field sort terms survive pagination and view changes - [MAJOR] add browser e2e: open/add/remove/Apply (URL assertion)/save-default + screenshot uploaded to PR #481 via Forgejo attachment API - [MINOR] remove dead _ = idCol assignment in buildMultiFieldListQuery - [MINOR] fix inverted NULL-sort comment in store.go - [MINOR] parseMultiSort and SaveSortPrefs now reject duplicate sort fields - [MINOR] fix double JustBeforeEach in sort_prefs_test.go SaveSortPrefs suite Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Sort-builder review fixes applied. Screenshot captured during local e2e run (7 new browser specs all green). CI will capture and upload the panel screenshot on the next run via
uploadSortBuilderScreenshotToPRine2e/browser/sort_builder_test.go.Fixes:
apply()now usesdata-sort-builder-base-url-value(shelf context preserved)SortParamfield propagated to all view-toggle/pagination URL buildersparseMultiSortandSaveSortPrefsidColremoved, NULL-sort comment corrected, doubleJustBeforeEachfixedNew HEAD:
41cbbe05feat(books): multi-field sort builder + save-as-default (bookshelf-oxau)to feat(books): multi-field sort builder + fix dead author-sort (bookshelf-oxau/ovnu)buildSortParam emitted just the field name ("added_on") for single-field sorts, dropping the explicit direction. The infinite-scroll next-page href therefore became ?sort=added_on with no order=, which parseSort decoded as Order="". Because service.List only filled the default direction when len(Terms)==0, the page-2 query ran the DESC cursor predicate against an ASC first page, skipping rows — the books list silently rendered only 3 of 4 books (visible in e2e: books_select_infinite_scroll_test "page 2" asserts >=4 checkboxes and timed out at 3). Fix: - buildSortParam always emits "field:dir" so direction survives the URL round-trip; parseSort routes "field:dir" through parseMultiSort which captures the explicit direction. Empty-Order terms fall back to the field's default direction so the token is always valid (an empty dir would 400 on re-parse). - service.List normalises any remaining empty-Order terms (belt-and-suspenders). - Regression tests: buildSortParam field:dir format incl. empty-Order case; service List page-2 cursor preserves ASC direction. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>Security Review — PR #481 (bd-bookshelf-oxau,
8e6d788b)Multi-field sort builder + author-sort writer + infinite-scroll cursor fix.
Findings
No blockers or majors found. Two minors noted below.
[MINOR] internal/books/sort_prefs_handler.go:36–43 — double validation of sort terms at handler vs service layer
SaveSortPrefsHandlerre-validates every term againstvalidSortKeysandasc/descbefore callingsave(which isSaveSortPrefs).SaveSortPrefsthen re-validates the same rules (lines 74–89 ofsort_prefs.go). The duplication is harmless from a security standpoint (belt-and-suspenders on the allowlist check is not wrong), but creates a maintenance hazard: if the allowlist ever gains a new key, the handler check would reject it while the service would accept it, creating a silent discrepancy. The handler-level check should be removed and the service layer trusted as the single validation point. The handler need only decode the JSON body and delegate.[MINOR] internal/books/store.go (buildMultiCursorPredicate, ~line 1540) — NULL-safe equality in prefix branches uses plain
=for nullable columnsIn the prefix equality branches (i.e., all terms before the final comparison), the code uses
col = ?even for nullable columns (e.g.,published_date,series_name,page_count,rating_effective). A cursor value derived from a NULL-valued nullable sort column is encoded as""→ decoded back tonilbydecodeCursorValForKey. Whennilis bound tocol = ?, MySQL evaluates it ascol = NULLwhich is never TRUE, silently dropping all rows that tie on that prefix position with a NULL sort value. This is a correctness issue at the cursor boundary but not a security issue — it does not expose cross-user data or allow injection. The fix is to usecol IS NULLwhen the cursor value is nil. Filing as MINOR because it affects pagination completeness, not security.Security audit results per tasking
Multi-user authorization (PUT /books/sort-prefs IDOR): CLEAN.
SaveSortPrefsHandlerreceivesuserIDexclusively fromuserIDFromRequest(r)(session-derived, never request body/param). TheUpsertUserSettingSQL is parameterized with(user_id, setting_key, value)fromdbsqlc.UpsertUserSettingParamswhereUserIDis set from the session.GetSortPrefssimilarly scopesGetUserSettingParams.UserIDfrom the caller-supplieduserID. No IDOR vector.Input validation on sort terms before persistence: CLEAN.
savePrefsHandlervalidates eachSortTerm.KeyagainstvalidSortKeys(closed allowlist map) andSortTerm.Orderagainst literal"asc"/"desc"before the value ever reaches the DB.SaveSortPrefsrepeats the same checks. Neither raw key strings nor raw order strings can reachORDER BYor storage without passing the allowlist.SQL injection in dynamic ORDER BY / cursor builder: CLEAN.
buildMultiSortOrderByandbuildMultiCursorPredicatelook up every column name throughsortKeyColumn[t.Key]— amap[SortKey]stringkeyed by validatedSortKeyvalues that originate from thevalidSortKeysallowlist. No user-supplied string is ever interpolated directly into the SQL string; only map-looked-up column expressions and?placeholders are used.Author-sort writer injection via author names: CLEAN.
RefreshSortAuthorNameis a sqlc-generated query. TheWHERE bm.book_id = ?bind is parameterized. TheSET bm.sort_author_name = a.namevalue comes from a JOIN to theauthortable — no user-supplied string is interpolated.Cursor tampering: CLEAN. The cursor is base64-decoded JSON. Malformed or unrecognized cursor field values go through
decodeCursorValForKeywhich returnsmiddleware.ErrValidationon type mismatch, surfaced as HTTP 400 before the query runs. Library scoping viaUserLibraryIDsis applied independently of cursor contents in both the single-field and multi-field query paths — a manipulated cursor cannot bypass theb.library_id IN (...)or1=0predicates.REVIEW VERDICT: 0 blocker, 0 major, 2 minor
placeholder
CODE REVIEW: NOT APPROVED
Phase 0: DEMO Verification
No structured DEMO block was present in the bead. Proceeding to spec review.
Phase 1: Spec Compliance
All three spec areas are present and coherent: author-sort writer (4 write sites), cursor regression fix (buildSortParam now always emits field:dir), and sort prefs (PUT /books/sort-prefs). One spec-level correctness bug found in a code path not updated by this PR.
Phase 2: Code Quality Findings
[MAJOR] internal/books/handler.go:784 — RenderMagicShelf omits SortParam and BaseURL from listPageData
RenderMagicShelfbuilds its ownlistPageDatastruct directly (lines 784-795) and does NOT setSortParamorBaseURL. Both fields were introduced in this PR and are required for:next-href({{if .SortParam}}&sort={{.SortParam}}{{end}}in books_index.html line 455) — without this the next page URL drops the sort, reverting to the default on page 2+ of any magic shelf.data-sort-builder-base-url-value— without this the sort builder always navigates to/booksinstead of the magic shelf URL.renderBooksHTML(called byRenderShelf) correctly sets both fields at lines 700-701.RenderMagicShelfis a separate code path that was not updated. Fix: addSortParam: buildSortParam(sort)andBaseURL: r.URL.Pathto theRenderMagicShelfdata struct (alongside the existingSortTermsDisplayandSortFieldOptionsthat were added correctly).[MAJOR] internal/books/store.go:1456 — id tiebreaker direction uses first term only; no test for mixed-direction multi-sort cursor round-trip
buildMultiSortOrderByandbuildMultiCursorPredicateboth derive the id tiebreaker direction fromterms[0].Order(lines 1456-1460 and 1539-1541). The ORDER BY and cursor predicate are internally consistent. However, no test exercises a cursor round-trip for a mixed-direction sort (e.g.title:asc,rating:desc). Given that this is new code implementing a non-trivial correctness invariant (total-order keyset pagination), the absence of a mixed-direction test is a gap that could hide a flake-inducing non-total-order bug. Per review-standard.md: "Non-deterministic ORDER BY / sort whose sort keys are not a total order... is at least [MAJOR]." Add a unit test forbuildMultiCursorPredicateandbuildMultiSortOrderBywithterms=[{title,ASC},{rating,DESC}]asserting the final id branch isbm.book_id > ?and ORDER BY emitsbm.title ASC, bm.rating_effective DESC, bm.book_id ASC.[MINOR] internal/library/scan/batch_seed.go:997 —
repeatStringsduplicatesrepeatStrfrom seeder packageinternal/seeder/seeder.go:545definesrepeatStrwith identical semantics. Not a blocker since they are separate packages.[MINOR] internal/books/multi_sort_test.go:1955 — test BeforeEach uses duplicate added_on keys, misleading context description
The
SortTermsat lines 1958-1959 contains{added_on, DESC}and{added_on, ASC}— a duplicate key. The store layer does not validate for duplicates (only the HTTP parse layer does), so the test exercises degenerate input while the description says "added_on+title". Misleading but not a production safety issue.[MINOR] internal/books/sort_prefs.go:319 — json.Marshal error suppressed with nolint without explanation
json.Marshal(valid)wherevalidis[]SortTerm(all string-underlying fields) cannot fail in practice, but the//nolint:errcheckneeds a brief inline comment explaining why. Example:// []SortTerm has only string fields; Marshal cannot fail.REVIEW VERDICT: 2 major, 3 minor
The
RenderMagicShelfmissingSortParam/BaseURL([MAJOR] #1) is a confirmed regression: infinite scroll on magic shelf pages drops the sort on page 2+, and the sort builder Apply always navigates to/booksinstead of the shelf URL. The mixed-direction cursor test gap ([MAJOR] #2) is a correctness-of-CI concern per the flake-prevention rules. Both must be resolved before merge.CODE REVIEW (fix verification): APPROVED — all prior findings resolved, fix introduces no new bugs.
Phase 0: DEMO — N/A (no DEMO block; this is a focused fix-verification review)
Finding 1 [MAJOR — RESOLVED]
internal/books/handler.go:794-795—RenderMagicShelfnow setsSortParam: buildSortParam(sort)andBaseURL: r.URL.Path, matching the identical lines inRenderShelfat line 700-701. Thesortparameter is in scope (function signature line 757). Fix is correct.Coverage:
handler_test.goadds aDescribe("RenderMagicShelf — SortParam and BaseURL are populated")block with threeItblocks asserting no error,sort=title:ascin body, andbase=/magic-shelves/7in body. Non-vacuous.Finding 2 [MAJOR — RESOLVED]
internal/books/multi_sort_test.goadds two newContextblocks for mixed-direction terms (title ASC, rating DESC):multi_sort_test.go~line 83): assertsbm.title ASC,bm.rating_effective DESC, andbm.book_id ASC(tiebreaker follows first-term direction). Exercises genuine mixed direction.bm.title > ?(ASC),bm.rating_effectiveis present (DESC branch),bm.book_id > ?(tiebreaker ASC), and negatively assertsbm.book_id < ?is absent. Both the ORDER BY string and cursor predicate are asserted. Not vacuous.Also fixes the pre-existing test at line 820 that had a duplicate
SortAddedOnterm instead ofSortTitle— test now matches its description.Finding 3 [NULL-cursor logic — RESOLVED, keyset predicate is correct]
internal/books/store.gobuildMultiCursorPredicate(~line 1495):Equality-prefix branch (line 1498-1502): When
nullableCol[t.Key] && v == nil, emitscol IS NULLand does NOT append tobranchArgs— arg list stays aligned. Whenv != nil, falls through tocol = ?+ appendsv. Correct.Comparison branch (line 1514-1519): When
nullableCol[t.Key] && v == nil,continueskips the entire OR branch. This is correct: MySQL NULLs sort first in ASC and last in DESC (plaincol ASC/DESC— no ISNULL wrapper). For a cursor row with NULL in positionprefixLen-1, there are no rows "after" it in that column's sort position (NULLs are at the boundary), so the branch is correctly dropped rather than emitting a nil bind param.Non-NULL nullable comparison (line 1529-1537): DESC emits
(col IS NULL OR col < ?)to capture NULLs that sort after non-NULLs in DESC; ASC emits plaincol > ?. Consistent with the single-field path (store.goline 1304-1306) which uses the sameIS NULL OR col < ?pattern for rating DESC.Arg alignment trace for
terms=[{title,ASC},{rating,DESC}],cursorVals=["Dune", nil],cursorID=99:(bm.title > ?)→ args:["Dune"]nullableCol[rating] && v==nil→continue(branch dropped)(bm.title = ? AND bm.rating_effective IS NULL AND bm.book_id > ?)→ args:["Dune", 99]args = ["Dune", "Dune", 99]— no nil values, placeholders alignTest at
multi_sort_test.go~line 270 assertsIS NULLpresent,argscontains no nil element, and verifies all three non-nil args are bound. Catches both the SQL text and arg-alignment correctness.One [MINOR] introduced by the fix:
internal/books/store.go:1531— The comment reads "We treat NULLs as logically last in both directions via ISNULL trick in ORDER BY." However,buildMultiSortOrderBy(line 1443) generates plaincol ASC/DESCwith noISNULL()wrapper. The code behavior is correct (consistent with MySQL's default NULL ordering: first in ASC, last in DESC, which is what the cursor logic accounts for), but the comment is factually wrong about an "ISNULL trick" being applied to ORDER BY. A reader will look for anISNULL()call that doesn't exist.Suggested fix: replace "via ISNULL trick in ORDER BY" with "relying on MySQL's default NULL ordering (NULLs first in ASC, last in DESC)".
Minors 3-5 [RESOLVED]
sort_prefs_handler.go); service-level validation remains. Tests updated to stubsaveErrwith wrappedmiddleware.ErrValidationso 400 responses are still asserted without the deleted handler code.repeatStringsdeleted; replaced with stdlibslices.Repeat(batch_seed.go).multi_sort_test.go:820.sort_prefs.gonolint comment added with explanation.REVIEW VERDICT: 0 blocker, 0 major, 1 minor