Fix new schema areas in existing documents (Astro) Part II (#5440)

* Prevent data corruption when stubbing areas for Astro

* Fix false positive orphan area warnings

* Guard against corrupt area items

Author: myovchev

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<string, any>[] = 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<string, any>[] = (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 = {}) {
       })}
     </Wrapper>
   ) : (
-    // 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 && (
       <div
         data-apos-area-error
         role="alert"

packages/apostrophe/modules/@apostrophecms/area/index.js

@@ -140,7 +140,7 @@ module.exports = {
               // so this logic is reproduced partially
               self.apos.doc.walk(area, (o, k, v) => {
                 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);

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);

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');
           }
         }
       },