fix:[NEXT-1943] Merge assets, finding matches in lower case#171
Conversation
📝 WalkthroughWalkthroughThis PR enhances asset ID reconciliation by implementing case-insensitive lookups through a normalized Changes
Sequence Diagram(s)sequenceDiagram
participant Input as Input Assets
participant Merge as MergeAssets Logic
participant PrimaryMap as primaryIdMap
participant Existing as existingAssets
participant AssetDTO as AssetDTO Model
participant Output as Updated State
Input->>PrimaryMap: Register primary assets<br/>(normalized lower + original)
Input->>Existing: Load existing assets
Input->>Merge: Process opinion assets
Merge->>PrimaryMap: Lookup by normalized docId<br/>(case-insensitive)
PrimaryMap-->>Merge: Return mapped asset
alt Asset found in existing
Merge->>AssetDTO: Update docId (if mapped)
Merge->>AssetDTO: Sync resourceId when<br/>matching (case-insensitive)
AssetDTO-->>Merge: Updated asset
Merge->>Existing: Record in latestAssetsDataMap
Merge-->>Output: Track as updated
else Asset not found
Merge->>Existing: Check if missing/deleted
Merge-->>Output: Track as new or deleted
end
Merge->>Output: Filter nulls from properties
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
assets-management/services/svc-asset-delta-engine/src/main/java/com/paladincloud/common/assets/MergeAssets.java (1)
79-83:⚠️ Potential issue | 🔴 CriticalThe remapped
docIdis not applied to the new opinion asset.At line 71, when a primary asset exists,
docIdis remapped to the primary's canonical id. However,latestDocis never updated with this remapped value. When line 83 callsassetHelper.createFrom(latestDoc), the helper internally rebuildsdocIdfromlatestDocviabuildDocId(), regenerating the original (uncorrected) id. This causes a mismatch: the response map key uses the canonical id, but the DTO'sdocIdfield retains the original casing, causing the asset to be indexed incorrectly.Fix
if (isNew) { - response.newAssets.put(docId, assetHelper.createFrom(latestDoc)); + var newAsset = assetHelper.createFrom(latestDoc); + newAsset.setDocId(docId); + response.newAssets.put(docId, newAsset); response.updatedAssets.remove(docId); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets-management/services/svc-asset-delta-engine/src/main/java/com/paladincloud/common/assets/MergeAssets.java` around lines 79 - 83, The new opinion asset's DTO is built from latestDoc before latestDoc's docId is updated to the remapped canonical id, so assetHelper.createFrom(latestDoc) regenerates the original (uncanonical) id; update latestDoc with the remapped docId (the same value used as the response.newAssets map key) before calling assetHelper.createFrom(latestDoc) — i.e., after you detect/assign the canonical docId (the variable docId) when a primary asset exists, set that value into latestDoc (the field used by buildDocId()) so createFrom(latestDoc) produces a DTO whose docId matches the canonical id in latestAssetsDataMap/response.newAssets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@assets-management/services/svc-asset-delta-engine/src/main/java/com/paladincloud/common/assets/MergeAssets.java`:
- Around line 49-57: Normalize all primary-id keys and lookups using
Locale.ROOT: when building primaryIdMap in MergeAssets (primaryAssets.forEach
block), call primaryId.toLowerCase(Locale.ROOT) instead of toLowerCase(); when
checking or retrieving entries use
primaryIdMap.get(docId.toLowerCase(Locale.ROOT)) and
primaryIdMap.containsKey(docId.toLowerCase(Locale.ROOT)) (normalize docId before
lookup), and in the deletion-path use the normalized lookup against primaryIdMap
for existingDocId (e.g.,
primaryIdMap.get(existingDocId.toLowerCase(Locale.ROOT))) instead of querying
primaryAssets directly so deletedPrimaryAssets is populated correctly.
---
Outside diff comments:
In
`@assets-management/services/svc-asset-delta-engine/src/main/java/com/paladincloud/common/assets/MergeAssets.java`:
- Around line 79-83: The new opinion asset's DTO is built from latestDoc before
latestDoc's docId is updated to the remapped canonical id, so
assetHelper.createFrom(latestDoc) regenerates the original (uncanonical) id;
update latestDoc with the remapped docId (the same value used as the
response.newAssets map key) before calling assetHelper.createFrom(latestDoc) —
i.e., after you detect/assign the canonical docId (the variable docId) when a
primary asset exists, set that value into latestDoc (the field used by
buildDocId()) so createFrom(latestDoc) produces a DTO whose docId matches the
canonical id in latestAssetsDataMap/response.newAssets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a57c1f19-5c75-4417-975f-f900e79c42a0
📒 Files selected for processing (4)
.gitignoreassets-management/services/svc-asset-delta-engine/src/main/java/com/paladincloud/common/assets/AssetDTO.javaassets-management/services/svc-asset-delta-engine/src/main/java/com/paladincloud/common/assets/MergeAssets.javaassets-management/services/svc-asset-delta-engine/src/test/java/com/paladincloud/commons/assets/MergeOpinionsTests.java
| // To handle secondary sources with all lowercase docId's, provide a mechanism to resolve | ||
| // both the lowercase & mixed case id to the primary asset | ||
| var primaryIdMap = new HashMap<String, AssetDTO>(); | ||
| if (primaryAssets != null) { | ||
| primaryAssets.forEach((primaryId, primaryAsset) -> { | ||
| primaryIdMap.put(primaryId.toLowerCase(), primaryAsset); | ||
| primaryIdMap.put(primaryId, primaryAsset); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is Java String.toLowerCase()locale-sensitive, and isLocale.ROOT the recommended choice when normalizing case-insensitive identifiers?
💡 Result:
Yes.
String.toLowerCase()is locale-sensitive: it uses the default locale (equivalent totoLowerCase(Locale.getDefault())) and can produce unexpected results for locale-independent text such as identifiers (classic example: Turkish dotted/dotless “I”). [1]- For normalizing case-insensitive identifiers/keys, you should pass an explicit locale and avoid the default locale.
Locale.ROOTis the language-neutral (“base”) locale intended for locale-sensitive operations when you don’t want user-locale effects, and is widely recommended for this purpose (e.g., by static analysis guidance). [2] [3]
So, for identifier normalization, prefer:
s.toLowerCase(Locale.ROOT)(or similarly toUpperCase(Locale.ROOT)). [2] [3]
Sources: Oracle String javadoc on locale sensitivity and identifier examples [1]; Oracle Locale.ROOT definition as language-neutral [2]; Error Prone guidance recommending Locale.ROOT for locale-independent casing [3].
🏁 Script executed:
cd assets-management/services/svc-asset-delta-engine && find . -name "MergeAssets.java" -type fRepository: PaladinCloud/ce-extensions
Length of output: 133
🏁 Script executed:
cd assets-management/services/svc-asset-delta-engine && wc -l src/main/java/com/paladincloud/common/assets/MergeAssets.javaRepository: PaladinCloud/ce-extensions
Length of output: 135
🏁 Script executed:
cd assets-management/services/svc-asset-delta-engine && sed -n '40,120p' src/main/java/com/paladincloud/common/assets/MergeAssets.javaRepository: PaladinCloud/ce-extensions
Length of output: 4023
🏁 Script executed:
cd assets-management/services/svc-asset-delta-engine && sed -n '101,120p' src/main/java/com/paladincloud/common/assets/MergeAssets.javaRepository: PaladinCloud/ce-extensions
Length of output: 1198
🏁 Script executed:
cd assets-management/services/svc-asset-delta-engine && grep -n "Assets.java" src/main/java/com/paladincloud/common/assets/MergeAssets.java || echo "No direct reference found"Repository: PaladinCloud/ce-extensions
Length of output: 95
Normalize every primary-id lookup with Locale.ROOT; fix the deletion-path lookup bypass.
The current implementation has three critical gaps in case normalization:
-
Line 54: Uses
String.toLowerCase()without specifyingLocale.ROOT, which is locale-sensitive and can break identifier matching (example: Turkish dotted/dotless "I"). -
Lines 69 & 89: Look up in
primaryIdMapusing the rawdocIdwithout case normalization:- Line 69:
primaryIdMap.get(docId)will miss if the incomingdocIdcase differs from the storedprimaryIdcase - Line 89:
primaryIdMap.containsKey(docId)fails for the same reason
- Line 69:
-
Line 114: Bypasses
primaryIdMapentirely and looks up directly inprimaryAssetsusing the rawexistingDocId. A mixed-case primary asset will never be found here, leavingdeletedPrimaryAssetsincomplete and stale primary docs undeleted.
Suggested fixes
+import java.util.Locale;
+
var primaryIdMap = new HashMap<String, AssetDTO>();
if (primaryAssets != null) {
primaryAssets.forEach((primaryId, primaryAsset) -> {
- primaryIdMap.put(primaryId.toLowerCase(), primaryAsset);
- primaryIdMap.put(primaryId, primaryAsset);
+ primaryIdMap.put(primaryId.toLowerCase(Locale.ROOT), primaryAsset);
});
}
- var primaryAsset = primaryIdMap.get(docId);
+ var primaryAsset = primaryIdMap.get(docId.toLowerCase(Locale.ROOT));
if (primaryAsset != null) {
- if (primaryIdMap.containsKey(docId)) {
+ if (primaryAsset != null) {
- var primaryAsset = primaryAssets.get(existingDocId);
+ var primaryAsset = primaryIdMap.get(existingDocId.toLowerCase(Locale.ROOT));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@assets-management/services/svc-asset-delta-engine/src/main/java/com/paladincloud/common/assets/MergeAssets.java`
around lines 49 - 57, Normalize all primary-id keys and lookups using
Locale.ROOT: when building primaryIdMap in MergeAssets (primaryAssets.forEach
block), call primaryId.toLowerCase(Locale.ROOT) instead of toLowerCase(); when
checking or retrieving entries use
primaryIdMap.get(docId.toLowerCase(Locale.ROOT)) and
primaryIdMap.containsKey(docId.toLowerCase(Locale.ROOT)) (normalize docId before
lookup), and in the deletion-path use the normalized lookup against primaryIdMap
for existingDocId (e.g.,
primaryIdMap.get(existingDocId.toLowerCase(Locale.ROOT))) instead of querying
primaryAssets directly so deletedPrimaryAssets is populated correctly.
2 participants
This also fixes a null reference exception when dealing with some assets (it wasn't able to parse records with null values)
Summary by CodeRabbit
Bug Fixes
Tests