Delta Migration | 1.0.0 | In Drupal Assets are not getting attached to entries - #1149
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
ℹ️ 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. |
There was a problem hiding this comment.
Pull request overview
This PR fixes Drupal migration field-type processing so that asset (file) and entry (reference) fields don’t get incorrectly re-processed when earlier pipeline steps have already resolved them into Contentstack-style reference objects (identified by a .uid). This prevents valid assets/references from being dropped from migrated entries during both initial runs and delta re-runs.
Changes:
- Added pass-through handling in
processFieldByTypefor already-resolved asset objects (filecase) for both single and multiple values. - Added pass-through handling in
processFieldByTypefor already-resolved reference objects (referencecase) for both single and multiple values. - Preserved existing raw-ID lookup behavior for unresolved values (e.g.,
assets_${id}andcontent_type_entries_title_${id}flows).
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
🔒 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 — Drupal processFieldByType already-resolved asset/reference pass-through
Reviewed at head 5676adb (all 3 commits; no prior review of mine on this PR). Comment-only — leaving the merge gate to a human.
Scope: 1 file, api/src/services/drupal/entries.service.ts, +28/-4. Matches the single api box in Affected Areas. No lockfile or generated churn, not a draft. Scope is clean.
The fix's core premise checks out
Your reviewer note asks whether any resolved-object shape lacks .uid. I traced both maps and the answer is no:
- Assets —
assetIdis loaded fromassets/index.json(ASSETS_SCHEMA_FILE), which is theassetDatamap written bysaveAsset. Every entry is created withuid:+ "assets_${safeFid}" +wheresafeFidis explicitly validated non-empty (assets.service.ts,if (!safeFid) throw`). - References —
reference/reference.jsonentries are{ uid:+ "content_type_entries_title_${nid}" +, _content_type_uid }(references.service.ts:112-116), guarded byif (!entry?.nid) return.
So .uid is always a non-empty string on both, and the truthiness guard is safe from the empty-string / 0 optional-chaining trap. I also confirmed the pre-existing single-reference shape really was the [[{uid,...}]] double-wrap, so the Array.isArray early return is a genuine fix, and that the raw-ID paths (numeric IDs, GROUP_CONCAT arrays, existing reference resolution) are untouched as claimed.
Findings
1 blocker, 2 questions.
- blocker —
case 'file', line 452: for asset fieldsprocessFieldDataonly ever produces a single object, never an array, so the newmultipleguard is unreachable; and anadvanced.multiplefile mapping falls through both inner branches toreturn value, emitting a bare object where Contentstack needs an array. Multi-asset fields therefore still don't attach. Suggested normalization inline. - question —
case 'reference'single branch, line 507: the array early return doesn't resolve raw IDs, asymmetric with themultiplebranch 8 lines above. Probably unreachable viaprocessFieldData, but_tidfields build raw-ID arrays in thectValueloop. Suggested one-line mirror inline. - question — line 474: the
Asset ... not found or invalid, removing fieldlog is now unreachable for*_target_idasset fields, which makes test-plan step 7 unable to fail. Missing assets are dropped silently upstream inprocessFieldDatawith no log at all.
Non-blocking
- Jira link is malformed and inconsistent. The description has
[[CMG-774](https://contentstack.atlassian.net/browse/CMG-1114)— unbalanced[[, and the label (CMG-774, matching branchfeature/cmg-774) disagrees with the URL (CMG-1114). Worth fixing so the ticket trail is correct. - On the missing unit test:
processFieldByTypeis a pure function of(value, fieldMapping, assetId, referenceId)— no DB, no filesystem. A Vitest test with literalassetId/referenceIdmaps covers all fourfile/referencebranches in a few lines, and themultiple+ single-object case above is exactly what it would have caught. I'd push for adding it rather than deferring, givenapi/src/services/drupalhas no field-processing coverage today. - Author checklist still has Self-reviewed the diff, Existing tests pass locally, and Branch follows naming convention unchecked.
Generated by Claude Code
…e-value fields correctly
🔒 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. |
🔗 Jira Ticket
[CMG-774
📋 PR Type
📝 Description
What changed?
Fixed
processFieldByType'sfileandreferencecases inapi/src/services/drupal/entries.service.ts.Added handling for values that are already resolved into asset/reference objects identified by a
.uid.Already-resolved objects are now passed through as-is instead of attempting to derive:
assets_${value}content_type_entries_title_${value}Applied the fix to both single-value and
multiplearray branches.Why?
Drupal image/file fields such as
field_image_target_idwere being resolved twice:processFieldDatacorrectly resolves the rawtarget_idinto a Contentstack asset reference object.processFieldByType'sfilecase, which assumes the value is still a raw numeric ID.assets_[object Object].As a result,
field_imageand other asset/reference fields resolved earlier in the migration pipeline were missing from the final migrated entry.Fix
The fix short-circuits processing when the value is already a resolved object containing a
.uid.This is additive and does not change the existing raw-ID flow, including:
GROUP_CONCATarraysThe fix also works correctly for delta re-runs because Drupal entries are fully re-fetched and re-processed on every migration run rather than incrementally patched.
🧩 Affected Areas
api— Node.js backendui— React frontendupload-api— Upload API serverdocker/docker-compose🧪 How to Test
Run a Drupal migration against a content type containing:
field_imageInspect the generated entry JSON under:
data/<stack_id>/entries/<content_type>/<locale>/<locale>.jsonConfirm that the image/file and reference fields are present in the generated entry.
Verify that the fields contain valid Contentstack
uidreferences.Re-run the same migration as a second iteration/delta run.
Confirm that the fields remain present and correctly resolved after the second run.
Verify that no
"Asset ... not found or invalid, removing field"errors are logged for valid assets.Expected Result
field_imageand other asset/reference fields should:"Asset ... not found or invalid, removing field"errors for valid assets.📸 Screenshots / Recordings
🔗 Related PRs / Dependencies
✅ Author Checklist
feature/,bugfix/, orhotfix/+ 5–30 lowercase characters.env/example.envupdated if new environment variables were addednpm test)README.md/ documentation updated if behavior changed👀 Reviewer Notes
The guard relies on the presence of
.uidto identify an already-resolved asset/reference object.The
.uidfield is always set when asset references are constructed inapi/src/services/drupal/assets.service.ts.Please flag if there is any known resolved-object shape that does not contain
.uid.No automated test was added directly to the existing test suite because
api/src/services/drupalcurrently does not have field-processing unit tests to extend.The fix was manually verified using a standalone script covering both:
A Vitest unit test for
processFieldByTypecan be added if required.