Conversation
|
Claude finished @nollymar's task in 3m 28s —— View job ReviewClean fix for the primary use case (single ensure item). A few things worth flagging: 1. Unchecked cast to
((ContentTypeFactoryImpl) this.contentTypeFactory).extractOrderByForComparator(...)
((ContentTypeFactoryImpl) this.contentTypeFactory).createContentTypeComparator(...)
2. Offset adjustment uses
adjustedOffset = Math.max(0, offset - authorizedIncluded.size());
3. Re-sort on every page is unnecessary on pages 2+
4. Multi-ensure offset math has an edge case with non-contiguous natural positions With multiple ensure items whose natural positions don't form a contiguous block, the simple SQLUtil.sanitizeSortBy fix and ContentTypeFactoryImpl.find lowercase fix — both are correct and necessary companions to the main fix. |
…set-duplicate-in-content-type-filter-after-reload-and-become-unselectable
…set-duplicate-in-content-type-filter-after-reload-and-become-unselectable
…set-duplicate-in-content-type-filter-after-reload-and-become-unselectable
Content type id and inode are the same value, so tracking both in includedIds was redundant. Removed duplicate inode() calls in favor of id() only. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…set-duplicate-in-content-type-filter-after-reload-and-become-unselectable
…set-duplicate-in-content-type-filter-after-reload-and-become-unselectable
…SortBy SQLUtil.sanitizeSortBy() applied toLowerCase() after stripping ASC/DESC suffixes, so uppercase inputs like "upper(name) ASC" were never stripped, leaving "upper(name) asc" which is not in the whitelist — returning empty string and producing "ORDER BY " (SQL syntax error). Fix: apply toLowerCase() first so both "ASC" and "DESC" variants are correctly stripped before the whitelist check. Also fixes the return path for DESC which had the same case-sensitivity issue. Additionally guard ContentTypeFactoryImpl.find() against appending ORDER BY when sanitization returns empty, as a defensive layer. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…) SQL SELECT_BY_VAR_NAMES uses LOWER(velocity_var_name) on the column side but was binding the parameter as-is. PostgreSQL = is case-sensitive for varchar, so mixed-case callers never matched. Lowercasing the param aligns with the SQL's intent and fixes the ensure-item lookup introduced by this branch. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
What's the problem?
The
/api/v1/contenttypeendpoint supports anensureparameter — a way to guarantee that a specific content type appears in the first page of results, even if it wouldn't land there in normal alphabetical order. Content Drive uses this so that when you reload the page with a content type already selected in the URL (e.g.?filters=contentType:dotAsset), the multiselect dropdown can immediately show your pre-selected item.The problem: the pagination math didn't account for the slot that the
ensureitem "borrowed" from page 1. This caused two bugs at once:Bug 1 — The ensured item appears twice
Consider 7 content types sorted alphabetically and a request for 3 per page with
ensure=FileAsset(which naturally lives at position 5):[Blog, DotAsset, FileAsset][Blog, DotAsset, FileAsset]✅[Generic, News, Product][Generic, News, FileAsset]❌ FileAsset again![Widget][Widget]✅Bug 2 — A content type goes missing forever
Because
FileAssetwas "borrowed" to page 1, it displaced the item that should have been in the 3rd slot. That item —Genericin the example — gets pushed to page 2. But the offset for page 2 was never adjusted, so the database query skips right over it:So after scrolling through the entire list,
FileAssetshowed up twice andGenericnever appeared at all.Root Cause
Both bugs trace back to two lines in
ContentTypeAPIImpl.search():Problem 1 — The ensure item lookup was called with the client's offset:
Problem 2 — The normal paginated query used the raw client offset, unaware that page 1 had "borrowed" a slot:
The Fix
Three targeted changes in
ContentTypeAPIImpl.search():offset=0— their IDs are needed every time to build the exclusion list for the normal query.offset == 0) — subsequent pages don't re-include them.ensureCountfrom the offset on pages 2+ — this shifts the window back so the displaced item is no longer skipped.Result with the fix:
All 7 items returned exactly once.
Proposed Changes
ContentTypeAPIImpl.search()— fix ensure item lookup offset, gate page-1-only inclusion, apply adjusted offset to normal paginated queryAdditional Info
This fix covers the single base-type path (the path taken by Content Drive when no base-type filter is active — the scenario that triggered the original bug report). The multi-type UNION path (
ContentTypeFactoryImpl.searchMultipleTypes) has analogous code and the same latent bugs, but it is only reached when multiple base-type filters are selected simultaneously — a separate, lower-priority follow-up.Videos
Issue
video.mov
Fix
video.mov
This PR fixes: #35213
This PR fixes: #35213