fix(VCST-5417): page content served blank — select by status, make writes atomic - #151
Merged
Conversation
…t preservation Adds a regression test that drives the real PageBuilderPageController UpdateGroup + PublishGroup across two rename -> Save -> Publish cycles and asserts content blocks survive. Proven valid (RED when the CopyPageContentAsync step is removed). Test-only change: adds a ProjectReference to ...Web so the real controller is reachable from the test project. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…Date
GET grouped/{groupId}/content picked the page with the newest ModifiedDate out
of a set mixing Draft, Published and Archived, then streamed whatever it held.
"Newest" is not "current":
* UpdateGroup creates the draft row and seeds its content in a separate step,
so in between the newest page is a draft with no content at all;
* NormalizePublishedPages demotes superseded pages to Archived in the same
save that promotes the new one, so those share the Published page timestamp
and the tie-break was undefined.
Missing content and empty content were also indistinguishable - both produced
an empty 200 - so a wrong pick was served as a legitimately blank page.
Select by status instead (Draft when requested, then Published, then Archived
as a last resort for fully archived groups) and fall through any candidate that
holds nothing; report 404 when the group has no content anywhere. The NULL vs
empty-string distinction the fall-through rests on already exists in the schema:
SaveBinaryAsync writes '' before appending chunks, so NULL means never seeded.
TryLoadBinaryAsync surfaces it; LoadBinaryAsync is obsolete but kept working via
a default interface implementation so external implementers are unaffected.
CopyPageContentAsync no longer writes an empty result, which would have flipped
the target from NULL to '' and destroyed that distinction.
Cascade cache invalidation from a page to its group. GroupedPageBuilderPage
caches the whole Pages collection, but the base ClearCache only expires the
page region, and the changed-event handlers re-read the group through the very
cache left stale - so after PublishGroup deleted the superseded pages the group
kept serving deleted page ids until the process restarted.
Also fix a connection leak on the content path: the DI scope backing the
repository DbContext was never disposed and OpenConnectionAsync had no matching
close, so every read or write held a pooled connection until GC.
SaveBinaryAsync blanks PageContent and then appends the payload one statement at a time. Nothing opened a transaction, so every save was briefly visible to concurrent readers as empty and then as truncated content - and since an empty string is a legitimate value, the read path serves it rather than falling through to the live page. Own a transaction unless the caller already started one, in which case the existing enlistment logic uses theirs. Replace the load-then-save copy with a single server-side UPDATE. The payload no longer round-trips through the application for the write, the copy cannot interleave with a change to the source, and a NULL source now yields a NULL target - copy semantics, and the state readers need to fall through to the live page. MySQL rejects reading the table under update in a plain subquery (1093), so it wraps the read in a derived table. The asset reference index is keyed by page id and still has to be rebuilt from the copied content afterwards. CopyContentAsync is a default interface method, like TryLoadBinaryAsync, so implementations of the published contract that predate it keep working. Tests cover the default interface implementations against a fake that provides only the two original members. The per-provider copy SQL needs a real database and is not covered here.
basilkot
force-pushed
the
claude/qa-autofix/VCST-5417
branch
from
July 22, 2026 12:34
aeedbcf to
880b3fb
Compare
basilkot
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 880b3fb. Configure here.
A save whose body is empty or unparsable used to be written to the page's content
column verbatim. The reader-side fall-through added for VCST-5417 cannot undo that:
it rescues a page whose column is NULL — a draft created but never seeded — while a
written empty value is indistinguishable from content the author cleared on purpose.
So the page reads as deliberately empty, shadows the live version, and the next
publish promotes it and deletes the page that still held the content.
Read and validate the body before touching the group. Order matters: the draft is
created before anything is written, so validating after it would strand an unseeded
row — the group would read as "Published, has draft with changes" for a save that
never happened, and unpublishing would then be refused.
Only well-formedness is checked, not the document's shape: content stored by older
designer versions can be an array rather than the current { settings, content }
object, and both still load. What this rules out is a payload that never parses —
a truncated upload, which every reader downstream chokes on.
Clearing every block stays a legitimate save; the guard is about payloads that carry
no document, not documents that carry no blocks.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
basilkot
added a commit
that referenced
this pull request
Jul 27, 2026
Backport of the content-integrity fixes from dev (#148, #151 and the VP-9220 guard) onto the 3.830 support line, scoped to what the data loss needs. The dev-only companions those PRs sit next to — the asset-reference index, the page-changed re-index event, SyncGroupSettingsToContent, the CopyPageContent endpoint, UpdateGroup draft-seeding — are deliberately not brought along. Reported by a customer on Azure SQL: pages opened blank in the designer at random and days of work were lost. Four defects combined into that. Writes were not atomic. SaveBinaryAsync blanked the content column in one autocommitted statement and then appended the payload 8 KB at a time in more autocommitted statements. Anything that interrupted the sequence — a dropped connection, a request the client aborted, one of the transient faults Azure SQL documents as expected — left the page empty for good. The write now runs in a transaction when the caller has not started one, so an interruption rolls back and the previous content survives. The body is also drained before the transaction opens, so a stalled upload fails before the column is touched. Reads could not tell "never written" from "written and empty". A draft row whose content column is NULL now reports as having no content, so the reader falls through to the published page instead of serving a blank 200 over it. Selection picked the newest row rather than the current one. SavePageContent creates the draft row and writes its content in a separate step, so in between — and permanently, after a write that failed — the draft was the newest row with no content, and it shadowed the published page that still held the work. Selection is now by status: draft when requested, then published. Saves that carried no document were written verbatim. An empty or unparsable body became "deliberately empty" content, which the fall-through cannot rescue because it is indistinguishable from content the author cleared on purpose; the next publish then promoted it and deleted the page that still had the content. Such a body is now refused before the group is touched, so no draft row is stranded either. Clearing every block stays a legitimate save. Every content read and write also leaked a DI scope, a DbContext and an open pooled connection. On Azure SQL that walks into the per-database session limit, whose errors are exactly the transient faults that used to blank a page — the leak fed the corruption. The repository now owns that scope and disposes it. #148 comes along because the base repository hardcoded T-SQL for every provider: PostgreSQL failed outright and MySQL accepted the blanking statement and then failed the append, wiping content on every save. Verified: build clean (TreatWarningsAsErrors), 20 tests green. Both invariants proven red first — removing the guard fails 7 tests, restoring the newest-row selection fails 4. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DO NOT MERGE until human review.
Summary
Test-only PR. Adds the missing regression coverage for VCST-5417 (published-page rename losing content on the next publish). No production code changes — the described data loss is already fixed on
dev: content is cloned into each new version viaCopyPageContentAsyncinUpdateGroupbeforePublishGrouppromotes it. This PR only adds the regression test that would have caught it.Fixes coverage gap for JIRA VCST-5417 — https://virtocommerce.atlassian.net/browse/VCST-5417
What the test does
PublishedRenameContentPreservationTestsdrives the realPageBuilderPageController.UpdateGroup+PublishGroupacross tworename → Save → Publishcycles and asserts the page's content blocks survive each cycle.CopyPageContentAsync) is removed, and GREEN on currentdevwhere the copy is present.ProjectReferencefrom the test.csprojto...Web.Files (tests-only)
tests/VirtoCommerce.PageBuilderModule.Tests/PublishedRenameContentPreservationTests.cs— new regression test.tests/VirtoCommerce.PageBuilderModule.Tests/VirtoCommerce.PageBuilderModule.Tests.csproj— addsProjectReferenceto...Web.No files under
src/are modified.Verification
dotnet build -c Debug(test project + transitive...Web) — succeeded, 0 warnings / 0 errors.dotnet test(xUnit v3 / Microsoft.Testing.Platform runner) — Total: 8, Failed: 0, Errors: 0, Skipped: 0.Reviewer notes
This adds coverage only; it does not alter runtime behavior. Since the underlying fix is already on
dev, the test is green here and serves as a guard against regression ofCopyPageContentAsyncinUpdateGroup.Note
High Risk
Touches core page persistence, HTTP content APIs, and caching; behavior changes (404 vs blank 200, rejected saves, copy semantics) can affect storefront rendering and publish/rename flows.
Overview
Fixes VCST-5417 / VP-9220 blank or lost Page Builder content by changing how pages are chosen for read, how blobs are stored, and what saves are allowed.
GET grouped content no longer picks the newest
ModifiedDateacross Draft/Published/Archived. It walks Draft → Published → Archived (whendraft=true) and streams the first page whose storage reports real content.LoadContentToStreamAsyncnow returnsfalsefor missing rows or NULLPageContent(unseeded draft), writes nothing to the response, and only flushes when content exists—so callers can fall through or return 404 instead of a blank 200.The data layer adds
TryLoadBinaryAsync(NULL vs empty string),CopyContentAsync(server-side SQL copy, MySQL 1093 workaround), transactions around chunked saves, connection close infinally, andIAsyncDisposablerepositories that dispose the DI scope so pooled connections are released.CopyPageContentAsyncuses DB copy and preserves NULL “never seeded” semantics.POST save content reads and validates non-empty, well-formed JSON before creating a draft or writing.
PageBuilderPageServiceexpires group cache when individual pages change.Regression tests cover rename/publish preservation, content selection, save guards, and
IContentStreamRepositorydefault interface compatibility.Reviewed by Cursor Bugbot for commit 9ef69b5. Bugbot is set up for automated code reviews on this repo. Configure here.
Artifact URL:
https://vc3prerelease.blob.core.windows.net/packages/VirtoCommerce.PageBuilderModule_3.1018.0-pr-151-9ef6.zip