diff --git a/packages/apostrophe-astro/components/AposArea.astro b/packages/apostrophe-astro/components/AposArea.astro index 94b6c27f8e..a790b34bbb 100644 --- a/packages/apostrophe-astro/components/AposArea.astro +++ b/packages/apostrophe-astro/components/AposArea.astro @@ -13,13 +13,22 @@ const { let attributes = {}; -// The backend sets `field` only on areas still in the schema; a missing or -// orphaned area lacks it and renders nothing. -const isArea = Boolean(area?.field); +// Enough structure to render as an area at all (hardens against malformed +// data — never crash the render). +const renderable = area?.metaType === "area" && Array.isArray(area?.items); -const widgets: Record[] = area?.items || []; +const isOrphan = Boolean(area?._isOrphan); +const isArea = renderable && !isOrphan; -const isEdit = isArea && area?._edit && Astro.url.searchParams.get("aposEdit"); +const hasField = Boolean(area?.field); + +// Defensive: a corrupt item (null/typeless) must never crash the render. +const widgets: Record[] = (area?.items || []).filter( + (item: any) => item && item.type, +); + +const isEdit = + hasField && area?._edit && Astro.url.searchParams.get("aposEdit"); const forceWrapper = aposAttributes || aposStyle || aposClassName; const WidgetComponent = widgetComponent ?? AposWidget; @@ -84,10 +93,12 @@ function getWidgetOptions(options: any = {}) { })} ) : ( - // Orphaned area. Dev-only diagnostic: `import.meta.env.DEV` is replaced - // with `false` in production, so this branch is dead-code eliminated. + // Genuine orphan only. Dev-only diagnostic: `import.meta.env.DEV` is + // replaced with `false` in production, so this branch is dead-code + // eliminated. Un-annotated (e.g. REST-delivered) areas never reach here — + // they render above. (import.meta as any).env.DEV && - area?.metaType === "area" && ( + isOrphan && (
{ if (v && v.metaType === 'area') { - const manager = self.apos.util.getManagerOf(o); + const manager = self.apos.util.getManagerOf(o, { log: false }); if (!manager) { self.apos.util.warnDevOnce( 'noManagerForDocInExternalFront', @@ -285,37 +285,95 @@ module.exports = { self.missingWidgetTypes[name] = true; } }, - // Build an empty area and attach it to `parent[name]`. When `parent` - // is doc-backed (has `_docId`, or is itself a doc) and `areaDotPath` - // is provided, also persist the area at that dot-path. The write is - // idempotent via `$eq: null`, so concurrent renders won't clobber each - // other. Returns the area. + // Build an empty area and attach it to `parent[name]`. When the area's + // location can be resolved inside the *persisted* document, also stub it + // into the database so the backend recognizes it for later edits. + // Returns the area. // - // Used by the `{% area %}` tag and the external front annotator as a + // Options: + // - `throwIfNotFound` (default `false`): when `parent` is doc-backed but + // the document or the container cannot be located in the database, + // throw a `notfound` error instead of returning an in-memory-only + // stub. The `{% area %}` tag opts in to preserve its historical + // behavior; the external front annotator leaves it off so a render is + // never brought down by such a case. + // + // Used by the `{% area %}` tag and the external front annotator as the // single source of truth for stubbing schema areas that have no value // yet. - async addMissingArea(parent, name, areaDotPath) { + async addMissingArea(parent, name, { throwIfNotFound = false } = {}) { const area = { metaType: 'area', _id: self.apos.util.generateId(), items: [] }; parent[name] = area; + const docId = parent._docId ?? (parent.metaType === 'doc' ? parent._id : null); - if (docId && areaDotPath) { - await self.apos.doc.db.updateOne( - { - _id: docId, - [areaDotPath]: { $eq: null } - }, - { - $set: { [areaDotPath]: self.apos.util.clonePermanent(area) } - } - ); + const areaDotPath = await self.resolvePersistedAreaDotPath(parent, name); + if (!areaDotPath) { + if (throwIfNotFound && docId) { + throw self.apos.error('notfound'); + } + return area; + } + + const result = await self.apos.doc.db.updateOne( + { + _id: docId, + // Idempotent and race-safe: only write when still absent. + [areaDotPath]: { $eq: null } + }, + { + $set: { [areaDotPath]: self.apos.util.clonePermanent(area) } + } + ); + if (result.modifiedCount === 0) { + // Another request stubbed it first (or it already existed): adopt + // the persisted `_id` so we render the same area. + const refreshed = await self.apos.doc.db.findOne({ _id: docId }); + const persisted = refreshed && self.apos.util.get(refreshed, areaDotPath); + if (persisted?._id) { + area._id = persisted._id; + } } return area; }, + + // Resolve the MongoDB dot-path at which `parent[name]` should be stored, + // computed from the *persisted* document so it always reflects real + // storage (not the in-memory graph with its loaded relationships). The + // `parent` object is located inside the freshly read document by its + // `_id`. Returns the dot-path string, or `null` when the area cannot be + // safely persisted (no doc id, doc not in the database, or `parent` is + // not part of the persisted document, e.g. loaded relationship data). + async resolvePersistedAreaDotPath(parent, name) { + const docId = parent._docId ?? + (parent.metaType === 'doc' ? parent._id : null); + if (!docId) { + return null; + } + const mainDoc = await self.apos.doc.db.findOne({ _id: docId }); + if (!mainDoc) { + return null; + } + if (parent._id === docId) { + return name; + } + if (!parent._id) { + return null; + } + const found = self.apos.util.findNestedObjectAndDotPathById( + mainDoc, + parent._id, + { ignoreDynamicProperties: true } + ); + if (!found) { + return null; + } + return `${found.dotPath}.${name}`; + }, prepForRender(area, context, fieldName) { const manager = self.apos.util.getManagerOf(context); const field = manager.schema.find(field => field.name === fieldName); diff --git a/packages/apostrophe/modules/@apostrophecms/area/lib/custom-tags/area.js b/packages/apostrophe/modules/@apostrophecms/area/lib/custom-tags/area.js index c9e5615ad1..865bd68a7b 100644 --- a/packages/apostrophe/modules/@apostrophecms/area/lib/custom-tags/area.js +++ b/packages/apostrophe/modules/@apostrophecms/area/lib/custom-tags/area.js @@ -59,34 +59,7 @@ module.exports = function(self) { } area = doc[name]; if (!area) { - // Problem: area is in schema but that doesn't guarantee it - // has a value, for instance the field could be new in the schema. - // But we need an area _id. Stub it into the db on the fly - // without race conditions - const docId = doc._docId || ((doc.metaType === 'doc') ? doc._id : null); - let areaDotPath; - if (docId) { - const mainDoc = await self.apos.doc.db.findOne({ _id: docId }); - if (!mainDoc) { - throw self.apos.error('notfound'); - } - let docDotPath; - try { - docDotPath = (doc._id === docId) ? '' : self.apos.util.findNestedObjectAndDotPathById(mainDoc, doc._id).dotPath; - } catch (e) { - // Race condition: someone removed the area's parent object. - // Unlikely thanks to advisory locking - throw self.apos.error('notfound'); - } - areaDotPath = docDotPath ? `${docDotPath}.${name}` : name; - } - area = await self.apos.area.addMissingArea(doc, name, areaDotPath); - if (docId) { - // Race-safety: re-read the persisted _id in case another request - // wrote first (our $eq: null write was a no-op in that case). - const refreshed = await self.apos.doc.db.findOne({ _id: docId }); - area._id = self.apos.util.get(refreshed, areaDotPath)._id; - } + area = await self.apos.area.addMissingArea(doc, name, { throwIfNotFound: true }); } const manager = self.apos.util.getManagerOf(doc); const field = manager.schema.find(field => field.name === name); diff --git a/packages/apostrophe/modules/@apostrophecms/template/index.js b/packages/apostrophe/modules/@apostrophecms/template/index.js index 5cd51cadd5..925a25932c 100644 --- a/packages/apostrophe/modules/@apostrophecms/template/index.js +++ b/packages/apostrophe/modules/@apostrophecms/template/index.js @@ -1352,19 +1352,18 @@ module.exports = { async annotateDocForExternalFront(doc, { scene } = {}) { const handled = new WeakSet(); const missingAreas = []; - self.apos.doc.walk(doc, (o, k, v, __dotPath) => { + self.apos.doc.walk(doc, (o, k, v) => { if (o._edit === true && !handled.has(o)) { handled.add(o); - // `__dotPath` is the path to `v` (= o[k]); the container `o` lives - // one segment up — '' for the top-level doc. - const dot = __dotPath.lastIndexOf('.'); - const containerDotPath = dot === -1 ? '' : __dotPath.substring(0, dot); for (const field of self.missingSchemaAreas(o)) { - missingAreas.push([ o, field, containerDotPath ]); + missingAreas.push([ o, field ]); } } if (v && v.metaType === 'area') { - const manager = self.apos.util.getManagerOf(o); + // A missing manager here is expected (e.g. an area reached on a + // container without a manager) and handled below, so suppress the + // low-level per-call log and rely on the once-per-process warning. + const manager = self.apos.util.getManagerOf(o, { log: false }); if (!manager) { self.apos.util.warnDevOnce( 'noManagerForDocInExternalFront', @@ -1374,6 +1373,7 @@ module.exports = { } const field = manager.schema.find(f => f.name === k); if (!field) { + v._isOrphan = true; self.apos.util.warnDevOnce( 'noSchemaFieldForAreaInExternalFront', `Area ${k} has no matching schema field in ${o.metaType} ${o.type || ''}` @@ -1385,15 +1385,8 @@ module.exports = { }); // Materialize every missing area, after the walk so we never add keys // to an object while it is being traversed. - for (const [ o, field, containerDotPath ] of missingAreas) { - const areaDotPath = containerDotPath - ? `${containerDotPath}.${field.name}` - : field.name; - const area = await self.apos.area.addMissingArea( - o, - field.name, - areaDotPath - ); + for (const [ o, field ] of missingAreas) { + const area = await self.apos.area.addMissingArea(o, field.name); area._edit = true; area._docId = o._docId ?? (o.metaType === 'doc' ? o._id : null); self.annotateAreaForExternalFront(field, area, { scene }); @@ -1406,6 +1399,7 @@ module.exports = { // at least as an empty array. annotateAreaForExternalFront(field, area, { scene } = {}) { + area._aposAnnotated = true; area.field = field; area.options = field.options; // Really widget configurations, but the method name is already set in @@ -1420,23 +1414,32 @@ module.exports = { }; }).filter(choice => !!choice); - area.items ||= []; + // Drop corrupt items (null, or not a widget). + area.items = (area.items || []).filter((item) => { + const valid = item && item.metaType === 'widget' && item.type; + if (!valid) { + self.apos.util.warnDevOnce( + 'corruptAreaItemInExternalFront', + `Dropping malformed item in area ${area._id || ''}` + ); + } + return valid; + }); + for (const item of area.items) { // Add _docId if area has one if (area._docId) { item._docId = area._docId; } - // Annotate each individual widget with its options - // Each widget must elect into this by creating an - // `annotateWidgetForExternalFront() method. + // Annotate each individual widget with its options. Each widget must + // elect into this by creating an `annotateWidgetForExternalFront()` + // method. const manager = self.apos.area.getWidgetManager(item.type); if (manager) { - const widgetOptions = manager.annotateWidgetForExternalFront(item, { scene }); - item._options = widgetOptions; + item._options = manager.annotateWidgetForExternalFront(item, { scene }); } else { self.apos.area.warnMissingWidgetType(item.type); - throw self.apos.error('invalid', 'Missing widget type'); } } }, diff --git a/packages/apostrophe/test/external-front.js b/packages/apostrophe/test/external-front.js index b49424cfe7..e12e5157c1 100644 --- a/packages/apostrophe/test/external-front.js +++ b/packages/apostrophe/test/external-front.js @@ -88,6 +88,9 @@ describe('External Front', function() { // Existing area still annotated, unchanged behavior assert(doc.main.field && doc.main.field.name === 'main'); assert(doc.main.options); + // Carries the provenance signal, and is never flagged as orphan + assert.strictEqual(doc.main._aposAnnotated, true); + assert.strictEqual(doc.main._isOrphan, undefined); // Missing area added as an empty, editable, annotated area assert(doc.extra, 'extra area was materialized'); @@ -99,6 +102,59 @@ describe('External Front', function() { assert(doc.extra.field && doc.extra.field.name === 'extra'); assert(doc.extra.options, 'annotated with options'); assert(Array.isArray(doc.extra.choices)); + assert.strictEqual(doc.extra._aposAnnotated, true); + assert.strictEqual(doc.extra._isOrphan, undefined); + }); + + it('flags a genuine orphan area, leaves valid areas alone, and never persists the flag', async function() { + // Insert a doc with an area whose field is no longer in the schema. + const docId = 'orphan-test:en:draft'; + await apos.doc.db.deleteOne({ _id: docId }); + await apos.doc.db.insertOne({ + _id: docId, + type: 'product', + metaType: 'doc', + aposMode: 'draft', + aposDocId: 'orphan-test', + aposLocale: 'en:draft', + title: 'Orphan Test', + slug: 'orphan-test', + main: { + metaType: 'area', + _id: 'orphan-main', + items: [] + }, + // `ghost` is not a field in the product schema (simulates a removed field) + ghost: { + metaType: 'area', + _id: 'orphan-ghost', + items: [] + } + }); + + const doc = await apos.doc.db.findOne({ _id: docId }); + doc._edit = true; + doc.main._edit = true; + doc.ghost._edit = true; + + await apos.template.annotateDocForExternalFront(doc); + + // The annotator owns the doc, so the orphan is flagged — and has no field. + // It is NOT marked `_aposAnnotated` (that signals a fully annotated area). + assert.strictEqual(doc.ghost._isOrphan, true, 'orphan flagged'); + assert.strictEqual(doc.ghost.field, undefined, 'orphan has no schema field'); + assert.strictEqual(doc.ghost._aposAnnotated, undefined, 'orphan is not _aposAnnotated'); + // The valid area is annotated normally and never flagged orphan. + assert(doc.main.field && doc.main._isOrphan === undefined); + assert.strictEqual(doc.main._aposAnnotated, true); + + // Neither flag is written to the database. + const persisted = await apos.doc.db.findOne({ _id: docId }); + assert.strictEqual(persisted.ghost._isOrphan, undefined, 'flag not persisted'); + assert.strictEqual(persisted.main._isOrphan, undefined, 'flag not persisted'); + assert.strictEqual(persisted.main._aposAnnotated, undefined, 'signal not persisted'); + + await apos.doc.db.deleteOne({ _id: docId }); }); it('annotateDocForExternalFront leaves missing areas alone on a non-editable doc', async function() { @@ -169,6 +225,222 @@ describe('External Front', function() { await apos.doc.db.deleteOne({ _id: docId }); }); + it('does not write missing areas at an in-memory path when a relationship target needs one (regression)', async function() { + // The editor's in-memory graph contains loaded relationship data. A widget + // in the host doc relates to a SEPARATE editable doc that is missing a + // schema area (added after it was created). The area must be stubbed at the + // related doc's OWN path — never at a path derived from the host's + // in-memory traversal, which would write into the wrong document and pad + // arrays with nulls. This reproduces the production corruption that left + // null items and stray fragments in published docs. + const hostId = 'reg-host:en:draft'; + const relatedId = 'reg-related:en:draft'; + await apos.doc.db.deleteMany({ _id: { $in: [ hostId, relatedId ] } }); + + // Host has both areas filled (nothing missing of its own) and a single + // rich-text widget in `main`. + await apos.doc.db.insertOne({ + _id: hostId, + type: 'product', + metaType: 'doc', + aposMode: 'draft', + aposDocId: 'reg-host', + aposLocale: 'en:draft', + title: 'Host', + slug: 'reg-host', + main: { + metaType: 'area', + _id: 'reg-host-main', + items: [ + { + _id: 'reg-host-widget', + metaType: 'widget', + type: '@apostrophecms/rich-text', + content: '

hi

' + } + ] + }, + extra: { + metaType: 'area', + _id: 'reg-host-extra', + items: [] + } + }); + // Related has `main` but no `extra` (field added to the schema later). + await apos.doc.db.insertOne({ + _id: relatedId, + type: 'product', + metaType: 'doc', + aposMode: 'draft', + aposDocId: 'reg-related', + aposLocale: 'en:draft', + title: 'Related', + slug: 'reg-related', + main: { + metaType: 'area', + _id: 'reg-related-main', + items: [] + } + }); + + const hostStored = await apos.doc.db.findOne({ _id: hostId }); + + // Build the in-memory graph: the host widget carries a loaded relationship + // to the related doc, which is editable and missing `extra`. + const related = await apos.doc.db.findOne({ _id: relatedId }); + related._edit = true; + related._docId = relatedId; + related.main._edit = true; + delete related.extra; + + const host = await apos.doc.db.findOne({ _id: hostId }); + host._edit = true; + host._docId = hostId; + host.main._edit = true; + host.main.items[0]._docId = hostId; + host.main.items[0]._related = [ related ]; + + await apos.template.annotateDocForExternalFront(host); + + // Related doc: `extra` stubbed at its OWN top-level path, clean, and its + // existing `main` is untouched (no host-relative path leaked in). + const relatedAfter = await apos.doc.db.findOne({ _id: relatedId }); + assert(relatedAfter.extra, 'extra stubbed on the related doc'); + assert.strictEqual(relatedAfter.extra.metaType, 'area'); + assert.deepStrictEqual(relatedAfter.extra.items, []); + assert.strictEqual(relatedAfter.extra._id, related.extra._id, 'same _id as in memory'); + assert.strictEqual(relatedAfter.main.items.length, 0, 'related main not padded'); + assert.ok(!Object.prototype.hasOwnProperty.call(relatedAfter, '0'), 'no numeric-key fragment'); + + // Host doc: completely untouched in the database. + const hostAfter = await apos.doc.db.findOne({ _id: hostId }); + assert.strictEqual(hostAfter.main.items.length, 1, 'host main.items not extended'); + assert( + hostAfter.main.items.every(i => i && i.metaType === 'widget'), + 'no null/typeless items in host' + ); + assert.deepStrictEqual(hostAfter, hostStored, 'host doc unchanged in the DB'); + + await apos.doc.db.deleteMany({ _id: { $in: [ hostId, relatedId ] } }); + }); + + it('annotateAreaForExternalFront drops corrupt items so they never reach the front end', function() { + const field = { + name: 'main', + options: { + widgets: { '@apostrophecms/rich-text': {} } + } + }; + const area = { + metaType: 'area', + _id: 'guard-area', + _docId: 'guard-doc:en:published', + items: [ + { + _id: 'w1', + metaType: 'widget', + type: '@apostrophecms/rich-text', + content: '

ok

' + }, + // The two shapes the production corruption produced. A `null` left in + // place would crash the Astro area renderer (`...item._options`). + null, + { + _id: 'frag', + foo: 'bar' + } + ] + }; + + assert.doesNotThrow(() => { + apos.template.annotateAreaForExternalFront(field, area, { scene: 'apos' }); + }); + + // Corrupt items are removed; only the valid, annotated widget remains. + assert.strictEqual(area.items.length, 1, 'corrupt items dropped'); + assert.strictEqual(area.items[0]._id, 'w1'); + assert(area.items[0]._options, 'valid widget annotated'); + assert.strictEqual(area.items[0]._docId, area._docId, 'valid widget got _docId'); + }); + + it('annotateAreaForExternalFront keeps an unknown widget type un-annotated instead of throwing', function() { + const field = { + name: 'main', + options: { + widgets: { '@apostrophecms/rich-text': {} } + } + }; + const area = { + metaType: 'area', + _id: 'unknown-area', + _docId: 'unknown-doc:en:published', + items: [ + { + _id: 'w1', + metaType: 'widget', + type: '@apostrophecms/rich-text', + content: '

ok

' + }, + // A real widget whose module is not registered (e.g. `custom-layout`). + { + _id: 'w2', + metaType: 'widget', + type: 'definitely-not-a-registered-widget' + } + ] + }; + + // No throw — a missing widget module must not 500 the whole render. + assert.doesNotThrow(() => { + apos.template.annotateAreaForExternalFront(field, area, { scene: 'apos' }); + }); + + // The unknown widget is preserved (so its content survives a save) but left + // un-annotated; the front end skips it. + assert.strictEqual(area.items.length, 2, 'unknown-type item preserved'); + assert(area.items[0]._options, 'valid widget annotated'); + assert.strictEqual(area.items[1].type, 'definitely-not-a-registered-widget'); + assert.strictEqual(area.items[1]._options, undefined, 'unknown widget not annotated'); + }); + + it('addMissingArea honors throwIfNotFound (tag behavior) and defaults to graceful (annotator)', async function() { + // Doc-backed parent whose document is not in the database (the missing-doc + // race the `{% area %}` tag historically treated as notfound). + const ghost = { + _id: 'ghost:en:published', + metaType: 'doc', + type: 'product' + }; + + // Opt-in (tag): throws notfound rather than persisting nothing silently. + await assert.rejects( + () => apos.area.addMissingArea(ghost, 'extra', { throwIfNotFound: true }), + err => err && err.name === 'notfound' + ); + // The in-memory stub is still attached for the caller. + assert(ghost.extra && ghost.extra.metaType === 'area'); + + // Default (annotator): degrades to an in-memory stub, never throws. + const ghost2 = { + _id: 'ghost2:en:published', + metaType: 'doc', + type: 'product' + }; + const area = await apos.area.addMissingArea(ghost2, 'extra'); + assert.strictEqual(area.metaType, 'area'); + assert(area._id, 'in-memory stub has an _id'); + assert.deepStrictEqual(area.items, []); + assert.strictEqual(ghost2.extra, area, 'stub attached to the parent'); + + // A parent with no docId is never an error, even with throwIfNotFound. + const unsaved = { + metaType: 'doc', + type: 'product' + }; + const unsavedArea = await apos.area.addMissingArea(unsaved, 'extra', { throwIfNotFound: true }); + assert.strictEqual(unsavedArea.metaType, 'area'); + }); + it('fetch home with external front', async function() { const data = await await apos.http.get('/', { headers: {