feat:[PEA-139] Removed prefix from azure asset id field and falttened…#176
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR refactors ChangesAsset DocId Azure Handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
assets-management/services/svc-asset-delta-engine/src/main/java/com/paladincloud/common/assets/AssetDocumentHelper.java (1)
529-537: 💤 Low valueConsider adding empty map check for consistency.
While iterating over an empty map is safe, adding an
isEmpty()check before theforEachwould be consistent withaddLegacyTags(line 518) and avoid unnecessary iteration overhead.♻️ Proposed consistency improvement
private void addFlattenedTags(Map<String, Object> data, AssetDTO dto) { var tagData = data.get(AssetDocumentFields.TAGS); if (tagData instanceof Map) { `@SuppressWarnings`("unchecked") var tagMap = (Map<String, Object>) tagData; - tagMap.forEach((key, value) -> { - dto.addType(STR."\{AssetDocumentFields.asTag(key)}", value); - }); + if (!tagMap.isEmpty()) { + tagMap.forEach((key, value) -> { + dto.addType(STR."\{AssetDocumentFields.asTag(key)}", value); + }); + } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@assets-management/services/svc-asset-delta-engine/src/main/java/com/paladincloud/common/assets/AssetDocumentHelper.java` around lines 529 - 537, In addFlattenedTags, add the same empty-map guard used in addLegacyTags: after casting tagData to tagMap (Map<String,Object>), check if tagMap.isEmpty() and return early to avoid the forEach when there are no entries; this affects the method addFlattenedTags and the usage of AssetDocumentFields.TAGS and dto.addType (and the AssetDocumentFields.asTag(key) key formatting) to keep behavior consistent and avoid unnecessary iteration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@assets-management/services/svc-asset-delta-engine/src/main/java/com/paladincloud/common/assets/AssetDocumentHelper.java`:
- Around line 141-144: The change to docId generation causes Azure entries to
lose the "azure_{type}_" prefix because docId is set to concatenatedId when
dataSource equals "azure"; confirm whether this was intentional and either
revert to the prior Azure format or add explicit migration/compatibility
handling: update the docId assignment in AssetDocumentHelper (variables:
concatenatedId, docId, dataSource, type, and the StringHelper.concatenate call)
so Azure follows the expected "azure_{type}_{concatenatedId}" pattern OR
implement a dual-lookup/migration path that maps old (prefixed) IDs to new ones,
plus update documentation and tests that assert Azure docId semantics.
---
Nitpick comments:
In
`@assets-management/services/svc-asset-delta-engine/src/main/java/com/paladincloud/common/assets/AssetDocumentHelper.java`:
- Around line 529-537: In addFlattenedTags, add the same empty-map guard used in
addLegacyTags: after casting tagData to tagMap (Map<String,Object>), check if
tagMap.isEmpty() and return early to avoid the forEach when there are no
entries; this affects the method addFlattenedTags and the usage of
AssetDocumentFields.TAGS and dto.addType (and the AssetDocumentFields.asTag(key)
key formatting) to keep behavior consistent and avoid unnecessary iteration.
🪄 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: a28908c0-9409-46a4-b405-830152e3f8d8
📒 Files selected for processing (2)
assets-management/services/svc-asset-delta-engine/src/main/java/com/paladincloud/common/assets/AssetDocumentHelper.javaassets-management/services/svc-asset-delta-engine/src/test/java/com/paladincloud/commons/assets/AssetDocumentHelperTests.java
Comment on lines
+141
to
+144
| var concatenatedId = StringHelper.concatenate(data, docIdFields, "_"); | ||
| var docId = "azure".equalsIgnoreCase(dataSource) | ||
| ? concatenatedId | ||
| : STR."\{dataSource}_\{type}_\{concatenatedId}"; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Java version supports template strings (21+)
if [ -f "pom.xml" ]; then
echo "=== Java version in pom.xml ==="
rg -n '<java.version>|<maven.compiler.source>|<maven.compiler.target>' pom.xml
fi
if [ -f "build.gradle" ]; then
echo "=== Java version in build.gradle ==="
rg -n 'sourceCompatibility|targetCompatibility' build.gradle
fi
# Search for code that might construct or parse Azure docIds with prefix
echo -e "\n=== Searching for azure docId construction patterns ==="
rg -nP --type=java 'azure.*_.*_' -C2
# Search for migration logic or docId format changes
echo -e "\n=== Searching for docId migration or format handling ==="
rg -nP --type=java 'docId.*azure|azure.*docId' -C2 | head -50Repository: PaladinCloud/ce-extensions
Length of output: 1049
🏁 Script executed:
#!/bin/bash
# Find pom.xml files in the repository
echo "=== Locating pom.xml files ==="
fd -n "pom.xml" | head -20
# Look for build.gradle
echo -e "\n=== Locating build.gradle files ==="
fd -n "build.gradle" | head -20Repository: PaladinCloud/ce-extensions
Length of output: 618
🏁 Script executed:
#!/bin/bash
# Once we find pom.xml, check the Java version in the relevant pom.xml
if [ -f "assets-management/services/svc-asset-delta-engine/pom.xml" ]; then
echo "=== Java version in svc-asset-delta-engine pom.xml ==="
cat assets-management/services/svc-asset-delta-engine/pom.xml | grep -A2 -B2 "java.version\|maven.compiler"
fi
# Also check parent pom if exists
if [ -f "assets-management/pom.xml" ]; then
echo -e "\n=== Java version in assets-management pom.xml ==="
cat assets-management/pom.xml | grep -A2 -B2 "java.version\|maven.compiler"
fi
if [ -f "pom.xml" ]; then
echo -e "\n=== Java version in root pom.xml ==="
cat pom.xml | grep -A2 -B2 "java.version\|maven.compiler"
fiRepository: PaladinCloud/ce-extensions
Length of output: 693
🏁 Script executed:
#!/bin/bash
# Get the full context of the buildDocId method to understand the breaking change
echo "=== Full buildDocId method context ==="
cat -n assets-management/services/svc-asset-delta-engine/src/main/java/com/paladincloud/common/assets/AssetDocumentHelper.java | sed -n '120,160p'Repository: PaladinCloud/ce-extensions
Length of output: 2085
🏁 Script executed:
#!/bin/bash
# Search for tests that validate docId generation
echo "=== Searching for docId tests ==="
fd -n "Test\.java$|Tests\.java$" assets-management/services/svc-asset-delta-engine | xargs -I {} bash -c 'grep -l "docId\|buildDocId" "{}" 2>/dev/null && echo "Found in: {}"' | head -10Repository: PaladinCloud/ce-extensions
Length of output: 353
🏁 Script executed:
#!/bin/bash
# Search for any references to the docId format or Azure docId handling elsewhere in the codebase
echo "=== Searching for docId format documentation or comments ==="
rg -nP --type=java 'docId|buildDocId' assets-management/services/svc-asset-delta-engine/src | head -30Repository: PaladinCloud/ce-extensions
Length of output: 5665
Confirm the intentional Azure docId format change and verify data migration.
The Java 21 template string syntax is valid and correctly supported. However, the change introduces a breaking format for Azure docIds: from azure_type_resourceid to just resourceid. This affects existing documents stored in Elasticsearch and downstream lookups.
Verify that:
- This format change is intentional and documented
- Existing Azure documents are migrated or no longer queried by the old format
- Any systems that depend on the
azure_prefix for Azure docIds are updated accordingly
🧰 Tools
🪛 PMD (7.24.0)
[High] 144-144: Parse Error: LexException: Lexical error in file 'assets-management/services/svc-asset-delta-engine/src/main/java/com/paladincloud/common/assets/AssetDocumentHelper.java' at line 144, column 25: "{" (123), after : ""\" (in lexical state DEFAULT)
(Parse Error)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@assets-management/services/svc-asset-delta-engine/src/main/java/com/paladincloud/common/assets/AssetDocumentHelper.java`
around lines 141 - 144, The change to docId generation causes Azure entries to
lose the "azure_{type}_" prefix because docId is set to concatenatedId when
dataSource equals "azure"; confirm whether this was intentional and either
revert to the prior Azure format or add explicit migration/compatibility
handling: update the docId assignment in AssetDocumentHelper (variables:
concatenatedId, docId, dataSource, type, and the StringHelper.concatenate call)
so Azure follows the expected "azure_{type}_{concatenatedId}" pattern OR
implement a dual-lookup/migration path that maps old (prefixed) IDs to new ones,
plus update documentation and tests that assert Azure docId semantics.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
… tags for azure
Summary by CodeRabbit
Release Notes
Improvements
Tests