docs(content-drive): spec for listing long-text projection trim (#37185) - #37189
Conversation
|
Claude finished @ihoffmann-dot's task in 1m 41s —— View job Review — spec PR (long-text projection trim, #37185)
This PR carries Blocking pre-merge item — ✅ resolved
New Issues
Resolved (prior review items now addressed in the file)
Note on the PR description (outside the diff — cannot fix in this file)The PR body still says "No open clarifications remaining" while OQ-3, OQ-5, OQ-7 remain open in the spec ( Everything blocking has been cleared and the reviewer has approved; the two items above are documentation-consistency fixes, non-blocking. · |
fabrizzio-dotCMS
left a comment
fabrizzio-dotCMS
left a comment
There was a problem hiding this comment.
Reviewed the spec against main (I read at 88af0bad55; the spec pins 87745ff315, nothing I checked moved in between).
Containing this to the transform layer is the right call — I agree with the scope, and with rejecting option (b). Duplicating content-type field introspection into a 2,700-line class that owns no other field responsibility would buy no containment, since both approaches modify the same dotContentMap and therefore hit Site Browser identically.
Most of what follows is inside that scope. The spec is unusually well-researched — I re-verified every file:line claim in it and they all reproduce, including both corrections it makes to the issue. My concerns are about the decision that was made last (OQ-1) not having been re-validated against the code afterwards.
One caveat on the framing
The spec is scoped to the transform layer on the premise that this is where the cost sits. That is true for the payload, but not for the load.
By the time DefaultTransformStrategy runs, findContentlets(inodes) (BrowserAPIImpl.java:1033,1072) has already materialized every contentlet in full from cache or DB, and copyContentlet.getMap() (DotContentletTransformerImpl.java:101) has already copied every field, bodies included, into the map. So the fix recovers Jackson serialization, wire bytes and browser retention, and leaves the DB/cache read and the map build untouched.
That is a defensible place to stop — a partially loaded contentlet would break the inode-keyed contentlet cache and the ADR-0018 read-your-writes contract, so a real column-level projection is a redesign rather than a fix. Note that OQ-3 (client-declared projection) does not change this either: it still filters after the load, it only moves the decision to the caller.
But the spec should state it, because it bounds what this item can deliver. "Reducing the other 27% of the payload" is listed as a non-goal; "the contentlet is still loaded and mapped in full" is not, and it is the first thing a reviewer of PR 2 will ask.
Blocking: the recommended implementation is silently undone for Story Block and JSON fields
StrategyResolverImpl#resolveStrategies (StrategyResolverImpl.java:126-143) adds DefaultTransformStrategy first, then the option-triggered strategies. StoryBlockViewStrategy#transform (StoryBlockViewStrategy.java:48-71) then re-reads the value from the source Contentlet — not from the map — and does map.put(field.variable(), jsonAsMap). JSONViewStrategy.java:44-58 does the same for JSON fields.
Both STORY_BLOCK_VIEW and JSON_VIEW are in defaultOptions, which is exactly the option set dotContentMap uses. So a truncation applied inside DefaultTransformStrategy is overwritten for every Story Block field.
FILTER_BINARIES is not a valid precedent for this, and for a specific reason: it works because BINARIES_VIEW is not in defaultOptions, so nothing runs after it to undo the map.remove. The spec's "same strategy, same trigger point, the mechanism is otherwise identical" does not hold for the two field types that are in defaultOptions.
Fixable — a strategy ordered last, or the cleanup(map) hook DotContentletTransformerImpl.java:104 already calls on the last strategy — but it is a different change from the one described, and the "roughly four files, ~15 lines" estimate no longer holds.
Related: there is no string to truncate for Story Block. After STORY_BLOCK_VIEW the map value is a LinkedHashMap, not a String. "WysiwygField.class, TextAreaField.class and StoryBlockField.class fields get their string value cut to 150 characters" is only true for the first two. Story Block needs its own decision (drop the key? keep the first N nodes? serialize-then-cut, which produces invalid JSON?) and it is not made anywhere in the document.
OQ-1's rationale does not hold in the grid it is protecting
Truncation was chosen over removal because removal would blank a "Show In List" column. But extra columns fall through #columnTypeForField (dot-content-drive-shell.component.ts:433-458) to TEXT, and TEXT renders as plain interpolation — @default { {{ value }} } (dot-folder-list-view.component.html:271). So today:
- a listed WYSIWYG cell shows escaped raw HTML. The first 150 characters of a real body are frequently markup (
<p><span style="...">…) with no visible prose, so the truncation would preserve a column showing nothing readable; - a listed Story Block cell renders
[object Object].
The column the decision protects is already broken for two of the three field types in scope. That does not necessarily make truncation the wrong choice, but the argument that justifies it needs redoing with this in hand — option (i), rejected for "blanking a configured column silently", is blanking a column that currently shows markup soup or [object Object]. If the truncation is meant to yield a readable preview it has to extract text before cutting, which is a third distinct change.
The remove→truncate decision did not reach the spec body
It reached issue #37185 and this PR description, but not spec.md:
| Location | Still says |
|---|---|
| Verification method → Integration | "long-text keys absent (AC-001)" — AC-001 says truncated |
| Verification method → Unit | "the option's removal logic … asserting exactly which keys survive" |
| AC-008 | "removing a long-text key could plausibly blank a rendered column" |
| Backward compatibility / rollback | "Here N removes a field, so rolling back to N-1 restores it" |
The rollback paragraph is the one that changes conclusion. Truncation is worse than removal for forward compatibility: a VTL template, an OSGi plugin, or a Site Browser consumer reading body gets a silently wrong, plausible-looking value instead of a missing one — a failure no UtilMethods.isSet() guard catches. That deserves saying explicitly, and OQ-6 should be re-answered on truncation semantics rather than inheriting the reasoning from the rejected option.
The PR description is ahead of the spec here — its checklist correctly says the test pins "the key set (and truncation length)", and hedges "removing/shrinking". Worth syncing the document to it.
Nothing gates latency
AC-001 only requires payload to drop by at least half, against a baseline where the body is 73% of it. Truncating to 150 characters should land near the full 73%, so as written an implementation that misses Story Block entirely (per the blocker above) still passes. p50/p95 appears only under "Manual / measurement" as something to record, not to meet. Given the ~65 ms the issue attributes to this item is partly the map build — which this fix does not touch — a latency AC, or an explicit statement that latency is not gated, would help.
Smaller items
- Test coverage is understated. "There is no
ContentDriveResourceTestand noContentDriveHelperTest" is true by name, but six Drive test classes exist:ContentDriveFieldFilterTest,ContentDriveHelperContentletAPIComparisonTest,ContentDriveKeywordSearchTest,ContentDriveWorkflowFilterTest,ContentDriveWorkflowArchiveStepTest,ContentDriveFieldFilterResolverTest. I grepped all six and the substantive claim survives — none asserts on a row's key set or onbody— but naming them would turn "will break no existing test" from an assertion into a verified one. - ADR-0018 is dismissed on the wrong grounds. "Cited for item 2, not this item" — but
ContentDriveFieldFilterTest.java:66asserts the ADR-0018 routing contract in the Drive path, andBrowserAPIImpl.java:1995,2192,2531implement it there. The dismissal is probably still right, since truncation is post-hydration and cannot change where data is read from; it should be stated on that basis. - The new
TransformOptionsconstant has no proposed name — every AC refers to "the new option". - 150 is a bare magic number. No derivation (grid column width? p50 title length?). Given the blast radius reaches Site Browser, VTL and OSGi, either tie it to OQ-7's
Configproperty or justify it. - No truncation marker. Nothing tells a consumer the value was cut. Trailing
…, a sibling<field>Truncated: true, or nothing — but decided in the spec, not in PR 2. - Character vs code point.
substring(0, 150)on a boundary that splits a surrogate pair emits a lone surrogate into the JSON response. Needs an AC or an explicit non-concern.
PR description
Proposed Changes describes a different template than the one used. spec.md follows spec-issue-template.md and has 0 user stories, 0 FR- requirements, 0 SC- success criteria, and no "dotCMS Legacy Considerations" section — the 8 acceptance criteria and the regression-risk analysis are the accurate parts. More importantly it says "No open clarifications remaining" while the spec carries 7 open questions, 6 unresolved, under a heading that reads "must be answered before /speckit-plan finalizes". Someone approving on the description alone would be approving past the gate.
Suggested sequencing
Answer OQ-2 (does Site Browser change too) and OQ-4 (which field types count as long text) first — they bound the change — then re-run OQ-1 with the rendering evidence above and add a Story Block sub-decision. If the WYSIWYG column really is unreadable today, option (iv) / OQ-3 becomes more attractive than the spec currently rates it, since the frontend already knows its showInListFields and can ask for exactly what it will render.
Also worth confirming before building: #37148 is closed, and its guidance was to land item 1 first and then re-evaluate whether items 2–4 are still needed. Item 1's spec (#37230) is still open, so that re-evaluation has not happened yet.
Credit where it is due — the Postman correction is right (allItemsMatch appears exactly once in the collection, line 995, its own declaration; the item.body read is dead), the "webAssetOptions() removes nothing, it merely decorates less" correction is right, and "no item.body read anywhere in core-web on a drive result" is right (the only hit is dot-template-builder.component.html:23, an unrelated template body). Every other code reference checks out too.
|
@fabrizzio-dotCMS confirmed in code: |
fabrizzio-dotCMS
left a comment
fabrizzio-dotCMS
left a comment
There was a problem hiding this comment.
Round 2 — re-review of c1652773
Re-reviewed against main @ 340c703feb. Thanks for the rewrite — this is a real improvement, and the blocking defect is genuinely fixed.
Short version: B1/B2/B4 from round 1 are closed and the corrected mechanism checks out against the code. What's left is one factual error that is the same error as my round-1 B3 (just moved from Story Block to WYSIWYG), one piece of work the spec now depends on that has nothing to build on, and a remove→truncate language pass that has survived a full round — including the one open question a human has to answer.
✅ Resolved
Strategy ordering (B1). Verified end to end:
StrategyResolverImpl#resolveStrategiesadds the default strategy first (:126-128), then base-type (:130-136), then iteratesoptionsdirectly (:138-143). ✅- Ordinals:
STORY_BLOCK_VIEW= 18,JSON_VIEW= 21. ✅ - The load-bearing fact the spec doesn't cite: the whole thing rests on
optionsalways being anEnumSet, and it is —DotTransformerBuilder#buildends withnew DotContentletTransformerImpl(contentlets, resolver, EnumSet.copyOf(optionsHolder), user)(DotTransformerBuilder.java:306), and the only other constructor is@VisibleForTesting(DotContentletTransformerImpl.java:60-64). So no production path hands over aHashSet. Worth stating in the spec — "EnumSet iteration order" guarantees nothing unless every caller passes one. - Bonus, so nobody re-raises it in PR 2: running last also means receiving the
cleanup(map)call (DotContentletTransformerImpl.java:107). Harmless —cleanupis declared once inAbstractTransformStrategy.java:105and overridden by none of the 13 strategies. One line in the spec closes that question preemptively.
Framing caveat (B4) — in at :35-40, and it says exactly the right thing: the field is still loaded in full, the ≈65 ms is serialization/transfer, not DB.
OQ-2 and OQ-4 resolved with the trade-offs written down. Rollback classification correctly reworked with the Story Block type-change exception.
🔴 B1′ — "WYSIWYG … renders correctly" is false in exactly the way Story Block was
The corrected rationale (:150-157) splits the two: Story Block gets preview extraction because its column is broken; WYSIWYG/TextArea get a raw substring because they "are plain strings the grid already renders correctly".
Re-verified on main — that premise doesn't hold for WYSIWYG:
#columnTypeForField(dot-content-drive-shell.component.ts:451-477) has no WYSIWYG branch. WYSIWYG'sdataTypeisLONG_TEXT, so it falls throughswitch (field.dataType)'sdefault:toTEXT— same as Story Block.- The
TEXTcell is@default { {{ value }} }(dot-folder-list-view.component.html:277). Angular interpolation escapes, so a WYSIWYG cell renders its HTML source, visibly, as text — not prose.
So body.substring(0, 150) on a real WYSIWYG body commonly yields only markup (<p><span style="font-family: …) — a "preview" with zero readable words — and can cut mid-tag or mid-entity. The column is broken today for the same reason Story Block's is, and the spec's own argument for extracting text from Story Block applies unchanged.
Pick one, in the spec:
- Strip markup before truncating for WYSIWYG too — one shared preview helper for all three field types, which is simpler than the current asymmetry; or
- State plainly that the WYSIWYG column renders escaped markup today and will keep doing so, drop "renders correctly", and let AC-003 promise a bounded value rather than a usable preview.
Either way AC-003 changes — it currently promises the cell shows "the 150-character truncation, not the full body and not blank" as if that were a preview.
🔴 B2′ — the Story Block preview has nothing to build on, and its input isn't always a Map
Two problems with "extracts a plain-text preview from the parsed block content" (:161-166):
No existing extractor. I searched: StoryBlockAPI exposes refreshReferences, refreshStoryBlockValueReferences, getDependencies — nothing text-related. StoryBlockUtil has only isEmptyStoryBlock / isEmptyBlock / isTextContentEmpty. So the preview means a new recursive traversal of the block tree collecting text nodes, with its own edge cases: nested lists and tables, marks, and embedded contentlets (StoryBlockAPI.allowedTypes = dotContent, dotImage, dotVideo) that carry no text at all. That's the substance of the change, and the "roughly five files" estimate (:388-395) hides it inside "a new strategy class (e.g. LongTextTruncationStrategy)".
The input is not always a LinkedHashMap. Reading StoryBlockViewStrategy.java:52-69, three shapes reach the map:
| Condition | What lands in the map |
|---|---|
value is not valid JSON (:54) |
the raw String, untouched |
readValue throws (:57-67) |
null — jsonAsMap is still null when map.put runs at :68 |
| happy path | LinkedHashMap |
The new strategy has to handle all three. The raw-String branch in particular is a long-text value that would sail straight past a Map-only truncation — the exact bug B1 was about, one level down.
🟠 S1′ — the remove→truncate migration is still incomplete, one round later
Regression Risk was rewritten; four other places weren't. The first two change conclusions:
| Where | Still says | Why it matters |
|---|---|---|
:341 Integration bullet |
"long-text keys absent (AC-001)" | Contradicts AC-001 (:288). This bullet is the TDD Red-gate contract (Principle V) — a test written from it encodes the rejected behaviour. |
OQ-6 :449-455 |
"removing a field is a forward-compatibility risk … (N-1 restores the field)" | OQ-6 is one of the questions a human must answer, and it's still phrased about a decision that was reversed. It should now ask about the only shape change the fix actually makes: Story Block object → string. |
:132-134 Correction callout |
"A fix must remove keys … The precedent for removal already exists — FILTER_BINARIES" |
Contradicts the decision and the corrected Recommended-direction text at :376-395, which now says FILTER_BINARIES is only a partial analog. |
:357 Jest bullet |
"nothing reads the removed keys" | Cosmetic, but it's the fourth one. |
🟠 S2′ — test-coverage claim is still an assertion
Still only "There is no ContentDriveResourceTest and no ContentDriveHelperTest" (:267) — both true, I re-checked. But six Drive test classes exist and are still unnamed:
dotcms-integration/src/test/java/com/dotcms/rest/api/v1/drive/:ContentDriveFieldFilterTest,ContentDriveHelperContentletAPIComparisonTest,ContentDriveKeywordSearchTest,ContentDriveWorkflowFilterTest,ContentDriveWorkflowArchiveStepTestdotCMS/src/test/java/com/dotcms/rest/api/v1/drive/ContentDriveFieldFilterResolverTest
My round-1 read of all six stands — none asserts a row's key set — so the conclusion is right. Name them and say so; "will break no existing test" currently reads as untested.
🟠 S3′ — ADR-0018 still dismissed on the wrong grounds
:90-93 still says ADR-0018 "is cited in issue #37148 for item 2, not for this item". But ContentDriveFieldFilterTest.java:66 asserts that routing contract in the Drive path. The dismissal is still correct — truncation happens post-hydration, so it can't change where data is read from — but that's the reason to give.
🟠 S4′ — 150, the marker, and code points: all three round-1 items untouched
Keyword-searched the current spec: no "surrogate", no "code point", no truncation "marker", and no Config reference near the truncation discussion.
- 150 is still bare — no derivation, and no tie to OQ-7's
Configproperty even though OQ-7 is right there. - No truncation marker decided — trailing
…, a sibling<field>Truncated: true, or nothing. - No code-point rule.
substring(0, 150)splitting a surrogate pair emits a lone surrogate into the JSON response. Needs an AC or an explicit non-concern. B1′ makes this worse for WYSIWYG: a raw substring can also land inside a tag or an entity.
🟠 S5′ — still no latency AC
AC-001 gates payload only ("drops by at least half"). p50/p95 appears exactly once, at :359, under Manual / measurement, as something to record. Now that the spec itself says DB load is untouched, state plainly whether latency is gated or explicitly not gated — otherwise a reader of PR 2 will assume the ≈65 ms is the target.
🟡 S6′ — the PR description is stale relative to the spec it carries
- "The recommended implementation mirrors an existing precedent (
DefaultTransformStrategy#addBinaries, same trigger point, same per-field-type iteration)" — the spec now says that's precisely what does not work, and moved the logic to its own strategy class. - "No open clarifications remaining" — four remain (OQ-3, OQ-5, OQ-6, OQ-7) under a heading whose own rule is "must be answered before
/speckit-planfinalizes".
Plan-level notes (not gate items)
defaultOptions()clears.DotTransformerBuilder#defaultOptions(:243-247) starts withoptionsHolder.clear(), as does nearly every view method in that builder. The new opt-in method must not clear, and must be chained afterdefaultOptions()atBrowserAPIImpl#dotContentMap. A copy-paste of the neighbouring methods silently wipes the default set.- "After
STORY_BLOCK_VIEWandJSON_VIEW" is necessary but not sufficient in general.DATETIME_FIELDS_TO_TIMESTAMP(22),SKIP_WIDGET_CODE_RENDERING(23),HISTORY_VIEW(24) andCLEAR_EXISTING_DATA(25) also sit afterJSON_VIEW. It is sufficient fordotContentMap, which usesdefaultOptionsalone. Append the constant at the end of the enum —TransformOptions.java:115-117already has a comment block marking that as the extension point — and the ordinal test the spec asks for at:349-351becomes meaningful.
Freshness — the pin is 364 commits stale, but nothing that matters moved
| What | Status on 340c703feb |
|---|---|
transform package (StrategyResolverImpl, TransformOptions, StoryBlockViewStrategy, JSONViewStrategy, DefaultTransformStrategy, DotContentletTransformerImpl) |
byte-unchanged since the pin — every mechanism claim holds exactly as written ✅ |
BrowserAPIImpl |
+373 lines. createContentMap's four-way branch now :1942-1955 (spec: :1891-1892); dotContentMap now :3044-3045 (spec: :2717). Structure identical, numbers stale. |
dot-folder-list-view.component.html |
extra-column <td class="truncate"> :211 → :220; @default { {{ value }} } :267 → :277. The spec cites :215 and :271-273, which match neither the pinned commit nor main — the claims reproduce, the numbers never did. |
dot-content-drive-field-filter-menu.component.ts:133 |
fields.filter((field) => field.listed), still no field-type restriction ✅ |
| The six Drive test classes | all still present ✅ |
Re-pin to a current commit, or state that references are as-of 87745ff315 and accept the drift.
Recommended next step
B1′ first. If the answer is "one shared preview helper for all three field types", then B2′ collapses into it, AC-003 becomes honest, S4′'s code-point question is answered by construction (you build a string from text nodes rather than cutting bytes out of markup), and the file estimate gets restated once instead of twice.
Leaving this as a comment rather than a change request — the three round-1 blockers are genuinely closed, and whether the remainder gates approval is the team's call.
…IWYG/TextArea/Story Block
|
Approved. One thing to fix before merge: The remaining round-2 items are non-blocking and I'm not gating on them, but they don't disappear on their own either:
None of these change the approach, which is why they're not blocking. Nice rewrite. |
…18 dismissal reasoning, gate latency explicitly, clarify OQ heading
# Conflicts: # .gitignore
fabrizzio-dotCMS
left a comment
fabrizzio-dotCMS
left a comment
There was a problem hiding this comment.
Re-approved — the spec phase is cleared. All three non-blocking items are closed, plus the feature.json fix-before-merge.
S3′ — the ADR-0018 dismissal now rests on the load-bearing fact. It explicitly drops the old "cited for item 2, not this item" framing, concedes the ADR does govern the Drive path generally, and rests the conclusion on truncation happening post-hydration, downstream of the read path. Your supporting citation checks out — that integration test does assert the routing contract for Drive, in its class javadoc.
S5′ — latency is now declared and explicitly not gated, with the reason attached: this fix touches serialization and wire payload only, and the DB/cache and map-build cost inside the ≈65 ms is untouched, so an improvement isn't guaranteed. That's exactly the ambiguity a PR 2 reader would otherwise have resolved by assuming ≈65 ms was a target.
S6′ — the Open Questions heading is honest now: which four are resolved, that the plan ran under an explicit team exception, and that OQ-3/OQ-5/OQ-7 are deferred non-blocking product decisions with no task depending on them. Saying what actually happened beats retro-fitting the process around it.
Also picked up without being asked: the Jest bullet went from "nothing reads the removed keys" to "nothing reads the truncated-away portion of these keys" — the last surviving remove-era phrasing from the first round. That thread is finally closed.
spec.md alone now: plan artifacts and .specify/feature.json both out, merge with main resolved.
3 participants
Spec-Kit PR 1 of 2. Carries the spec alone. Needs a developer approval (not a merge) before /speckit-plan runs.
Resolves the spec phase of #37185.
Proposed Changes
spec.md— 2 prioritized user stories, 5 functional requirements, 3 success criteria, 8 acceptance criteria, regression-risk/blast-radius analysis, and the dotCMS Legacy Considerations section. No open clarifications remaining.Summary
Every row
POST /api/v1/drive/searchreturns for a generic Content item carries the full value of that contentlet's long-text fields (WYSIWYG, TextArea, Story Block) even though the listing is a grid. On the benchmarked dataset a 40-row page measured 159 KB, ~73% of it a single long-text field, paid on serialization, on the wire, and in browser memory.The issue's premise — "the grid doesn't render these fields" — is not quite right. The grid renders any field flagged "Show In List" on the content type, including long-text ones, with no field-type restriction. An unconditional removal would silently blank that column for any content type configured that way.
Decision: truncate to 150 characters rather than remove, applied uniformly regardless of
listedstatus. This bounds payload in every case and preserves the Show In List column as a preview instead of blanking it. The frontend already visually clips these cells with CSS ellipsis — that's display-only clipping of the full string already in the response, so server-side truncation is what actually removes the bytes; no frontend change required.The recommended implementation mirrors an existing precedent (
DefaultTransformStrategy#addBinaries, same trigger point, same per-field-type iteration) doing a bounded-lengthmap.putinstead ofmap.remove.Checklist
item.bodyread in the Postman collection's "Text Search - Alpha Filter" test needs removing or replacing with a real assertionAdditional Info
Parent epic #36814. Originally investigated as item 3 of #37148 (umbrella investigation issue).
dotContentMapis shared with Site Browser (POST /api/v1/browser) and reachable from the legacy DWR Site Browser and from OSGi viaAPILocator.getBrowserAPI()— broader blast radius than Content Drive alone, covered in the spec's Regression Risk section./speckit-adr-contextconsulted — no ADR identified as binding for this item; it changes which fields survive into the response, not where data is read from. No new ADR proposed.🤖 Generated with Claude Code
This PR fixes: #37185
This PR fixes: #37185
This PR fixes: #37185