fix: stop excluding AEM structure entry from migration (CMG-1112) - #1152
Conversation
✅ 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
🟠 High Severity - SLA Breached Issues (with fixes)Showing 7 issue(s) that have exceeded the 30-day SLA threshold:
ℹ️ 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:
❌ BUILD FAILED - Security checks failed Please review and fix the security vulnerabilities before merging. |
🔒 Security Scan Results
⏱️ SLA Breach Summary
🟠 High Severity - SLA Breached Issues (with fixes)Showing 7 issue(s) that have exceeded the 30-day SLA threshold:
ℹ️ 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:
❌ BUILD FAILED - Security checks failed Please review and fix the security vulnerabilities before merging. |
🔒 Security Scan Results
⏱️ SLA Breach Summary
🟠 High Severity - SLA Breached Issues (with fixes)Showing 7 issue(s) that have exceeded the 30-day SLA threshold:
ℹ️ 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:
❌ BUILD FAILED - Security checks failed Please review and fix the security vulnerabilities before merging. |
🔒 Security Scan Results
⏱️ SLA Breach Summary
🟠 High Severity - SLA Breached Issues (with fixes)Showing 7 issue(s) that have exceeded the 30-day SLA threshold:
ℹ️ 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:
❌ BUILD FAILED - Security checks failed Please review and fix the security vulnerabilities before merging. |
🔒 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.
Review — CMG-1112 follow-up (head e002672)
Reviewed the AEM change against the code at the PR head. The direction is right — reverting the outright exclusion and deduplicating instead is the correct reading of the requirement. But I don't think the PR's central claim holds:
The existing
modelId::localecollision key already guarantees exactly one entry gets created once the outright exclusion is removed.
It guarantees at most one, not exactly one. There is a reachable path where the collision key is consumed by a file that produces no entry, which then suppresses the sibling file that would have produced one — the same "structure entry is missing" symptom QA reopened the ticket for, just now reproducible instead of order-dependent. Details in the inline comment.
1 blocker, 1 question, 1 nit.
question: the sort makes the winner deterministic, but not semantically chosen
api/src/services/aem.service.ts:1370 → the tie-break is the lexicographically-first relative path. Nothing in that ordering correlates with "is real content" vs "is template metadata". When the winner does produce an entry, its title (:1422), templateUid/contentType (:1424-1428) and field data (:1430) all come from the winning file — so on a genuine collision the surviving entry's content is decided by filename sort order.
The comment being deleted in this PR asserted the structure file can "compete with a real page for the same derived id". The PR description instead describes both colliding files as template/model definitions (notitle.model.json + page-content-full-width.template.json). Those two readings imply very different blast radii, and I can't tell which is accurate from the code alone.
Could you confirm from the actual export: are the two files that collide on modelId::locale both template/structure definitions, or is one of them a real content page? If the latter, sorting locks in deterministically dropping a real page rather than fixing it, and the tie-break needs to prefer the real page explicitly (e.g. keep the repo:path detection from #1146, but use it to rank which file wins instead of to exclude).
nit: Affected Areas doesn't match the diff
Only api is ticked, but the diff also touches:
ui/package.json+ui/package-lock.json—react-router/react-router-dom^7.15.0→^7.18.2, plus apostcssoverride. This is a real behavioural dependency change, and per the security-scan comments it's what moved the gate fromBUILD FAILED(7 SLA breaches with fixes) toBUILD PASSED WITH WARNINGS(0). Worth tickinguiso it isn't reviewed as api-only.package.json—postcssoverride>=8.5.10→>=8.5.23.upload-api/src/config/index.{ts,json}— trailing-comma and trailing-newline only, no functional change. Incidental churn; fine to drop from the PR.
Not asking you to split it if the dep bumps are what unblocks CI — just declare them.
Reviewed e002672. Note the trigger fired on e3d9df5; this review covers the current head, which is 1 commit further along.
Generated by Claude Code
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.
|
Addressing the review-level question and nit from the latest round: Question (semantic tie-break): I can only speak to the sample data included in this repo ( Nit (Affected Areas / incidental churn): Updating the PR description to tick |
🔒 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. |
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).
🔒 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. |
4 participants
🔗 Jira Ticket
CMG-1112
📋 PR Type
📝 Description
What changed?
Why?
The prior fix (merged in #1146) correctly stopped the "structure" entry from being duplicated, but did so by excluding it from migration entirely on the assumption it was non-content template/schema metadata. QA reopened CMG-1112 after observing the "structure" entry now isn't migrated at all. The actual requirement is: migrate it, just once — not duplicated, not missing. The existing modelId::locale collision key (from the previous fix) already guarantees exactly one entry gets created once the outright exclusion is removed; the sort addresses the original reviewer concern (arbitrary/non-reproducible collision winner) without needing to drop the entry.
🧩 Affected Areas
api— Node.js backendui— React frontend🧪 How to Test
Expected result:
exactly one Contentstack entry for the "structure" content exists after iteration 1, it is present (not excluded), and no additional duplicate appears after iteration 2 or later.
📸 Screenshots / Recordings
N/A — backend-only change, no UI impact.
✅ Author Checklist
feature/,bugfix/, orhotfix/+ 5–30 lowercase chars👀 Reviewer Notes