Skip to content

fix:[NEXT-1943] Merge assets, finding matches in lower case #171

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
arunpaladin merged 1 commit into from
Apr 9, 2026
Merged

fix:[NEXT-1943] Merge assets, finding matches in lower case #171

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,4 +392,5 @@ pyrightconfig.json
# Lambda layer artifacts
**/bin/layers/bin/extension.zip
**/bin/layers/bin/extensions/*-layer
**/bin/*
**/bin/*
/assets-management/services/svc-asset-delta-engine/.aws-sam
5 changes: 4 additions & 1 deletion ...ervices/svc-asset-delta-engine/src/main/java/com/paladincloud/common/assets/AssetDTO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,10 @@ public class AssetDTO {
@JsonAnySetter
private void addAdditionalProperty(String key, Object value) {
if (!key.equals(AssetDocumentFields.OPINIONS)) {
additionalProperties.put(key, value);
// ConcurrentHashMap does not allow null values
if (value != null) {
additionalProperties.put(key, value);
}
}
}

Expand Down
42 changes: 34 additions & 8 deletions ...ices/svc-asset-delta-engine/src/main/java/com/paladincloud/common/assets/MergeAssets.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,16 @@ static public MergeAssets process(AssetDocumentHelper assetHelper,
Map<String, AssetDTO> primaryAssets) {
var response = new MergeAssets();

// 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);
});
}
Comment on lines +49 to +57

@coderabbitai coderabbitai Bot Apr 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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 to toLowerCase(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.ROOT is 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 f

Repository: 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.java

Repository: 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.java

Repository: 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.java

Repository: 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:

  1. Line 54: Uses String.toLowerCase() without specifying Locale.ROOT, which is locale-sensitive and can break identifier matching (example: Turkish dotted/dotless "I").

  2. Lines 69 & 89: Look up in primaryIdMap using the raw docId without case normalization:

    • Line 69: primaryIdMap.get(docId) will miss if the incoming docId case differs from the stored primaryId case
    • Line 89: primaryIdMap.containsKey(docId) fails for the same reason

  3. Line 114: Bypasses primaryIdMap entirely and looks up directly in primaryAssets using the raw existingDocId. A mixed-case primary asset will never be found here, leaving deletedPrimaryAssets incomplete 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.


var latestAssetsDataMap = new HashMap<String, Map<String, Object>>();
latestAssets.forEach(latestDoc -> {
var idField = latestDoc.getOrDefault(assetHelper.getIdField(), "").toString();
Expand All @@ -54,8 +64,19 @@ static public MergeAssets process(AssetDocumentHelper assetHelper,
STR."Asset missing the id field '\{assetHelper.getIdField()}'");
}
var docId = assetHelper.buildDocId(latestDoc);
latestAssetsDataMap.put(docId, latestDoc);
var asset = existingAssets.get(docId);

var primaryAsset = primaryIdMap.get(docId);
if (primaryAsset != null) {
docId = primaryAsset.getDocId();
if (asset != null) {
asset.setDocId(docId);
} else {
asset = existingAssets.get(docId);
}
}

latestAssetsDataMap.put(docId, latestDoc);
var isNew = asset == null || (assetHelper.isPrimarySource()
&& asset.getPrimaryProvider() == null);
if (isNew) {
Expand All @@ -65,32 +86,37 @@ static public MergeAssets process(AssetDocumentHelper assetHelper,
response.updatedAssets.put(docId, asset);
assetHelper.updateFrom(latestDoc, asset);

if (primaryAssets != null && primaryAssets.containsKey(docId)) {
var primaryAsset = primaryAssets.get(docId);
if (primaryIdMap.containsKey(docId)) {
assetHelper.updateFrom(latestDoc, primaryAsset);
response.updatedPrimaryAssets.put(docId, primaryAsset);
if (primaryAsset.getResourceId().equalsIgnoreCase(docId)) {
// For Tenable, the resource id is always lowercase. Update that if possible
primaryAsset.setResourceId(docId);
primaryAsset.setLegacyResourceId(docId);
}
}
}
});

existingAssets.forEach((key, value) -> {
if (!isAssetProcessed(key, response)) {
existingAssets.forEach((_, value) -> {
var existingDocId = value.getDocId();
if (!isAssetProcessed(existingDocId, response)) {
if (assetHelper.isPrimarySource()) {
var assetState = value.getAssetState();
if (assetState == null || !(assetState.equals(AssetState.SUSPICIOUS)
|| assetState.equals(AssetState.RECONCILING))) {
response.missingAssets.put(key, value);
response.missingAssets.put(existingDocId, value);
assetHelper.missing(value);
}
} else {
if (assetHelper.missing(value)) {
response.deletedOpinionAssets.add(value);
var primaryAsset = primaryAssets.get(key);
var primaryAsset = primaryAssets.get(existingDocId);
if (primaryAsset != null && primaryAsset.getPrimaryProvider() == null) {
response.deletedPrimaryAssets.add(primaryAsset);
}
} else {
response.updatedAssets.put(key, value);
response.updatedAssets.put(existingDocId, value);
}
}
}
Expand Down
25 changes: 25 additions & 0 deletions ...-asset-delta-engine/src/test/java/com/paladincloud/commons/assets/MergeOpinionsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,32 @@ void primaryAndOpinionUpdatedFromOpinion() throws JsonProcessingException {
assertEquals(Set.of("gcp_vminstance_q13"), merger.getUpdatedPrimaryAssets().keySet());
assertEquals(List.of(), merger.getDeletedOpinionAssets());
assertEquals(List.of(), merger.getDeletedPrimaryAssets());
assertEquals(0, merger.getNewAssets().size());
assertEquals(0, merger.getNewPrimaryAssets().size());
}

// With both a primary and opinion having different-cased doc ids
@Test
void primaryAndOpinionDifferentCaseDocIds() throws JsonProcessingException {
var mixedCaseId = "gcp_vminstance_Q13_Doggo";
var existingPrimary = createExistingPrimary(List.of(mixedCaseId), "gcp", null);
var existingOpinions = createExistingOpinion(List.of(mixedCaseId.toLowerCase()), "gcp",
defaultReportingSource,
null);
var latest = createLatest(List.of("q13_doggo"), "gcp", defaultReportingSource, defaultRawData);

// Override the doc id given by the helper
existingPrimary.get(mixedCaseId).setDocId(mixedCaseId);
existingOpinions.get(mixedCaseId.toLowerCase()).setDocId(mixedCaseId.toLowerCase());

var creator = getHelper(defaultReportingSource, defaultReportingService);
var merger = MergeAssets.process(creator, existingOpinions, latest, existingPrimary);

assertEquals(Set.of(), merger.getMissingAssets().keySet());
assertEquals(Set.of("gcp_vminstance_Q13_Doggo"), merger.getUpdatedAssets().keySet());
assertEquals(Set.of("gcp_vminstance_Q13_Doggo"), merger.getUpdatedPrimaryAssets().keySet());
assertEquals(List.of(), merger.getDeletedOpinionAssets());
assertEquals(List.of(), merger.getDeletedPrimaryAssets());
assertEquals(0, merger.getNewAssets().size());
assertEquals(0, merger.getNewPrimaryAssets().size());
}
Expand Down
Loading