Dev - #1153
Conversation
usedEntryUids was reserving modelId::locale unconditionally right after computing uid, before the later contentType/mappedLocale check decided whether an entry would actually be produced. A file that fell into the "no content type matched" branch (e.g. a template structure export with no mapped content type) still consumed the key — permanently blocking a sibling file sharing that key from producing the real entry, leaving zero entries instead of one. That's the same "structure entry missing" symptom QA reopened CMG-1112 for, now made deterministic by the added sort instead of order-dependent. Move the reservation into the success branch so a non-producing file never blocks its sibling, while still guaranteeing at most one entry per modelId::locale.
Reverts upload-api/src/config/index.ts and index.json to match dev exactly. These were unrelated trailing-comma/trailing-newline diffs that had ridden along in this branch's history, not part of the CMG-1112 fix (flagged in PR review).
fix: stop excluding AEM structure entry from migration (CMG-1112)
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
🔒 Security Scan Results
⏱️ SLA Breach Summary
ℹ️ Vulnerabilities Without Available Fixes (Informational Only)The following vulnerabilities were detected but do not have fixes available (no upgrade or patch). These are excluded from failure thresholds:
Consider reviewing these vulnerabilities when fixes become available. |
umesh-more-cstk
left a comment
umesh-more-cstk
left a comment
There was a problem hiding this comment.
Automated review of dev → pre-stage at c2d98f2 (4 files, +35/−42).
1 blocker · 2 questions · 1 nit
The AEM change is two moves: createEntry now walks entry files in sorted order and defers the usedEntryUids reservation into the success branch, and the template-structure exclusion block added earlier for CMG-1112 is removed.
The reservation move (aem.service.ts:1437) is right, and it brings this layer into line with extractEntries, which has always done its contentType check before seenEntryUids.add (upload-api/migration-aem/libs/entries/index.ts:77-100). A file that can't produce an entry no longer burns the key its sibling needs — that's the reopened symptom fixed. My questions are about what the sort does and doesn't guarantee alongside it, and about the sibling layer not being sorted with it.
Findings
- blocker —
package.json:32: the postcss bump lands only inui. There is noworkspaceskey at root and each sub-project installs from its own lockfile, so this root override propagates nowhere (root's tree has no postcss at all).api/package-lock.jsonstill resolves postcss 8.5.15 andupload-api/package-lock.json8.5.20, both under the>=8.5.23floor this PR sets — the SLA-breached CVE stays open in two of three workspaces. Fix: add the override toapi/package.jsonandupload-api/package.jsonand regenerate both lockfiles. - question —
aem.service.ts:1370: sorting makes the collision winner deterministic but not correct; the "at most one entry permodelId::locale" invariant is untouched, so if the structure export resolves a content type it sorts first (conf/<content/), takes the key, and the real page is dropped as a duplicate. If it doesn't resolve one, removing the exclusion block is a no-op for output and the deferred reservation is doing all the work. Which case does the CMG-1112 export hit? - question —
aem.service.ts:1370:extractEntries(index.ts:58) still walks raw fs order, so the two layers can pick different winners for the same colliding id and the entry-mapper row ends up describing a different file than the entry that was migrated. - nit — PR hygiene / scope: the description is the unfilled template (title
Dev, Jira left asMIGRATION-XXXX, no PR Type or Affected Areas ticked, empty What/Why). The diff spansapi(AEM entry-collision behaviour) andui+ root (CVE dependency bumps) — two unrelated changes riding together. Worth tickingapianduiand naming both, so the promotion intopre-stageis reviewable from the description alone.
Posted as a comment only — approval is the human reviewer's call.
Generated by Claude Code
| // files legitimately collide on the same modelId+locale (see CMG-1112), which one "wins" | ||
| // and gets migrated is deterministic and reproducible across machines/runs, rather than | ||
| // depending on filesystem directory order. | ||
| for await (const fileName of [...read(entriesDir)].sort()) { |
There was a problem hiding this comment.
question: Sorting makes the collision winner deterministic, but it doesn't make it correct — and the at most one entry per modelId::locale invariant is unchanged by this PR. So when the template structure export and a real page derive the same modelId (the exact CMG-1112 case the deleted block described), exactly one of them can still be migrated, and which one is now decided purely by lexicographic path order.
Failure scenario, if the structure export does resolve a content type (i.e. it carries a templateName/templateType that matches an otherCmsUid, or an isXF title match): conf/... sorts before content/... ("conf" < "cont"), so the structure file is processed first, matches at line 1432, reserves modelId::locale at 1437, and the real page then hits the duplicate check at ~1408 and is dropped with a Skipped duplicate entry warning. That swaps the reported symptom (structure entry missing) for a worse one (the actual page missing), and it's silent apart from a warn log.
Conversely, if the structure export never matches a content type — which is what 9708bdb's message assumes — then it can never emit an entry at all, and deleting the exclusion block is a no-op for output: the deferred reservation alone fixes the reopened bug. Either way the exclusion removal doesn't get the structure entry migrated.
Could you confirm which case the CMG-1112 export actually hits? If it's the first, the fix wants to be at the uid derivation rather than the walk order — fold the disambiguating path (dataLayer[id]['repo:path'] or :path) into modelId when two files share parseData.id, so both can be emitted instead of one shadowing the other; or, if only one should ever win, encode that precedence explicitly (real content beats template structure) rather than leaving it to sort().
Generated by Claude Code
| // files legitimately collide on the same modelId+locale (see CMG-1112), which one "wins" | ||
| // and gets migrated is deterministic and reproducible across machines/runs, rather than | ||
| // depending on filesystem directory order. | ||
| for await (const fileName of [...read(entriesDir)].sort()) { |
There was a problem hiding this comment.
question: The sibling layer wasn't sorted with it. upload-api/migration-aem/libs/entries/index.ts:58 still walks read(templatesDir) in raw fs order, and both files carry comments asserting the two collision policies must stay in lockstep ("the way extractEntries's own collision policy already works" here; "must match createEntry in the api's aem.service" there).
Failure scenario: for a modelId that two files collide on, extractEntries keeps the fs-order-first file and createEntry now keeps the sort-order-first file. When those differ, the entry-mapper row (entryName, language, otherCmsCTName — index.ts:102-110) describes the losing file while the entry actually written to Contentstack came from the winner. The uid matches, so nothing breaks loudly; the Entry Mapper UI just shows the wrong source name/locale for that row.
Suggested fix: apply the same [...read(templatesDir)].sort() at index.ts:58 so both layers pick the same winner.
Unrelated to this PR, but noting it while we're here: extractEntries keys on modelId alone (index.ts:97) whereas createEntry keys on modelId::mappedLocale. For a page with several locale variants, createEntry correctly emits one entry per locale, but only the first locale gets a mapper row — worth a follow-up ticket if that's not already known.
Generated by Claude Code
| "axios": ">=1.16.0", | ||
| "nth-check": ">=2.0.1", | ||
| "postcss": ">=8.5.10", | ||
| "postcss": ">=8.5.23", |
There was a problem hiding this comment.
blocker: This override doesn't reach anything, and two of the three workspaces stay on the vulnerable postcss.
Verified against the lockfiles at this head:
| lockfile | postcss | nanoid |
|---|---|---|
package-lock.json (root) |
not present | not present |
api/package-lock.json |
8.5.15 | 3.3.12 |
upload-api/package-lock.json |
8.5.20 | 3.3.16 |
ui/package-lock.json |
8.5.26 ✅ | 3.3.18 ✅ |
Root package.json has no workspaces key, and api/, upload-api/, ui/ each have their own package.json + lockfile and are installed independently — so a root overrides entry propagates to none of them. Root's own tree doesn't contain postcss either, so this line is inert on both counts; only the matching entry added to ui/package.json did any work.
Failure scenario: the CVE this PR is titled for (e002672, "close SLA-breached CVEs") stays open in api and upload-api at 8.5.15/8.5.20 — both below the >=8.5.23 floor this PR itself declares — while the scanner shows the root and ui manifests as remediated. The SLA ticket gets closed on a partial fix.
Suggested fix: add "postcss": ">=8.5.23" to the overrides block of api/package.json and upload-api/package.json, then regenerate api/package-lock.json and upload-api/package-lock.json (npm install --package-lock-only in each). That pulls nanoid up as a postcss dependency at the same time. Keep or drop this root line as you prefer — it has no effect either way.
Generated by Claude Code
3 participants
🔗 Jira Ticket
MIGRATION-XXXX
📋 PR Type
📝 Description
What changed?
Why?
🧩 Affected Areas
api— Node.js backendui— React frontendupload-api— Upload API serverdocker/docker-compose🧪 How to Test
Expected result:
📸 Screenshots / Recordings
🔗 Related PRs / Dependencies
✅ Author Checklist
feature/,bugfix/, orhotfix/+ 5–30 lowercase chars.env/example.envupdated if new environment variables were addednpm test)README.md/ docs updated if behaviour changed👀 Reviewer Notes