feat:[PEA-646] Response structure modification in data shipper for ta…#2398
Merged
Conversation
GitHub recommends upgrading from v2 to v3.
Contributor
WalkthroughThe PR refactors ChangesTaggingSummary Deserialization Refactor
CodeQL Workflow Upgrade
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/util/AssetGroupUtil.java (2)
334-337: ⚡ Quick winReuse
ObjectMapperas a static field instead of instantiating per call.
ObjectMapperis thread-safe after configuration and expensive to create. Instantiating it on every method invocation adds unnecessary overhead, especially if this method is called frequently.♻️ Suggested refactor
Add a static field at class level:
private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();Then update the method:
- ObjectMapper mapper = new ObjectMapper(); - Map<String, Object> summaryInfo = mapper.convertValue( + Map<String, Object> summaryInfo = OBJECT_MAPPER.convertValue( summaryResponse.data, new TypeReference<Map<String, Object>>() { });🤖 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 `@jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/util/AssetGroupUtil.java` around lines 334 - 337, The ObjectMapper is being instantiated locally within the method in AssetGroupUtil, which creates unnecessary overhead on every invocation since ObjectMapper is expensive to create. Add a private static final field named OBJECT_MAPPER initialized with new ObjectMapper() at the class level in AssetGroupUtil. Then, replace the local instantiation of ObjectMapper mapper with a reference to the static OBJECT_MAPPER field in the method where convertValue is called.
326-337: 💤 Low valueConsider using a single serialization library instead of mixing Gson and Jackson.
The code deserializes JSON to POJO using Gson, then converts POJO to Map using Jackson. This mixing introduces potential inconsistencies (different null handling, type coercion, naming strategies) and increases maintenance complexity.
Since the codebase already uses both libraries, pick one for this flow. Jackson can do both steps:
♻️ Alternative using Jackson only
ObjectMapper mapper = new ObjectMapper(); // or reuse static instance JsonNode root = mapper.readTree(response); JsonNode dataNode = root.get("data"); if (dataNode == null || dataNode.isNull()) { LOGGER.warn("No data found in tagging summary response for ag: {}", ag); return resultList; } Map<String, Object> summaryInfo = mapper.convertValue(dataNode, new TypeReference<Map<String, Object>>() {}); resultList.add(summaryInfo);🤖 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 `@jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/util/AssetGroupUtil.java` around lines 326 - 337, Replace the mixed Gson and Jackson usage with Jackson only to avoid serialization library inconsistencies. Remove the Gson deserialization that creates the TaggingSummaryResponse object, and instead use the ObjectMapper to directly read the JSON response as a tree structure, then extract the "data" node and check if it is null or empty before converting it to a Map. This consolidates the serialization logic to a single library and improves maintainability by eliminating potential null handling and type coercion differences between Gson and Jackson.
🤖 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 @.github/workflows/codeQl.yaml:
- Line 50: Replace the mutable version tags (`@v3`) in the three GitHub CodeQL
action references with immutable commit SHAs. For each occurrence of
github/codeql-action/init@v3, github/codeql-action/analyze@v3, and the other
CodeQL action on line 84, replace the `@v3` tag with the full commit SHA (e.g.,
`@abc123def456`...) to ensure these actions are pinned to specific immutable
commits rather than floating version tags that could change unexpectedly.
In
`@jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/util/AssetGroupUtil.java`:
- Around line 339-341: The catch block for Exception handling in this method
logs the error but then silently continues without signaling the failure to the
caller, which masks Lambda invocation failures and prevents downstream systems
from distinguishing between missing data and actual failures. Modify the
exception handling to either rethrow the exception after logging (to preserve
the original behavior and allow callers to handle the failure), return an
Optional or result wrapper that explicitly indicates success or failure, or add
monitoring metrics to alert when Lambda failures occur. Ensure the method's
contract clearly communicates whether the operation succeeded or failed to
callers.
---
Nitpick comments:
In
`@jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/util/AssetGroupUtil.java`:
- Around line 334-337: The ObjectMapper is being instantiated locally within the
method in AssetGroupUtil, which creates unnecessary overhead on every invocation
since ObjectMapper is expensive to create. Add a private static final field
named OBJECT_MAPPER initialized with new ObjectMapper() at the class level in
AssetGroupUtil. Then, replace the local instantiation of ObjectMapper mapper
with a reference to the static OBJECT_MAPPER field in the method where
convertValue is called.
- Around line 326-337: Replace the mixed Gson and Jackson usage with Jackson
only to avoid serialization library inconsistencies. Remove the Gson
deserialization that creates the TaggingSummaryResponse object, and instead use
the ObjectMapper to directly read the JSON response as a tree structure, then
extract the "data" node and check if it is null or empty before converting it to
a Map. This consolidates the serialization logic to a single library and
improves maintainability by eliminating potential null handling and type
coercion differences between Gson and Jackson.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e6e7edc6-3aff-402d-9cf6-f906de642a18
📒 Files selected for processing (3)
.github/workflows/codeQl.yamljobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/util/AssetGroupUtil.javajobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/util/TaggingSummaryResponse.java
| # Initializes the CodeQL tools for scanning. | ||
| - name: Initialize CodeQL | ||
| uses: github/codeql-action/init@v2 | ||
| uses: github/codeql-action/init@v3 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all GitHub Actions uses are SHA-pinned (40-hex) rather than mutable tags.
rg -nP '^\s*uses:\s*[^@]+@([a-f0-9]{40})\s*$|^\s*uses:\s*' .github/workflows/*.y*mlRepository: PaladinCloud/CE
Length of output: 14775
Pin CodeQL actions to immutable commit SHAs.
Lines 50, 65, and 84 use mutable version tags (@v3) instead of immutable commit SHAs. This creates supply-chain risk and should be pinned to specific commits.
🔒 Suggested fix
- uses: github/codeql-action/init@v3
+ uses: github/codeql-action/init@<full_commit_sha>
- uses: github/codeql-action/autobuild@v3
+ uses: github/codeql-action/autobuild@<full_commit_sha>
- uses: github/codeql-action/analyze@v3
+ uses: github/codeql-action/analyze@<full_commit_sha>🧰 Tools
🪛 zizmor (1.25.2)
[error] 50-50: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/codeQl.yaml at line 50, Replace the mutable version tags
(`@v3`) in the three GitHub CodeQL action references with immutable commit SHAs.
For each occurrence of github/codeql-action/init@v3,
github/codeql-action/analyze@v3, and the other CodeQL action on line 84, replace
the `@v3` tag with the full commit SHA (e.g., `@abc123def456`...) to ensure these
actions are pinned to specific immutable commits rather than floating version
tags that could change unexpectedly.
Source: Linters/SAST tools
Comment on lines
339
to
341
| } catch (Exception e) { | ||
| LOGGER.error("Error retrieving tagging summary data", e); | ||
| throw e; | ||
| LOGGER.error("Error retrieving tagging summary data for ag: {}", ag, e); | ||
| } |
Contributor
There was a problem hiding this comment.
Silent error swallowing changes the method's failure contract.
The previous implementation logged and re-threw exceptions, allowing callers to handle failures. The new implementation catches all exceptions and returns an empty list, making it impossible for callers to distinguish between "no tagging data exists" and "the Lambda invocation failed."
This could mask production issues—if the Lambda consistently fails, downstream systems will process empty data without any indication of failure.
Consider either:
- Rethrowing after logging (preserves original behavior)
- Returning an
Optionalor a result wrapper that indicates success/failure - At minimum, adding a metric or alert for failures
🛠️ Suggested fix to preserve failure signaling
} catch (Exception e) {
LOGGER.error("Error retrieving tagging summary data for ag: {}", ag, e);
+ throw e;
}
- return resultList;
}🤖 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
`@jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/util/AssetGroupUtil.java`
around lines 339 - 341, The catch block for Exception handling in this method
logs the error but then silently continues without signaling the failure to the
caller, which masks Lambda invocation failures and prevents downstream systems
from distinguishing between missing data and actual failures. Modify the
exception handling to either rethrow the exception after logging (to preserve
the original behavior and allow callers to handle the failure), return an
Optional or result wrapper that explicitly indicates success or failure, or add
monitoring metrics to alert when Lambda failures occur. Ensure the method's
contract clearly communicates whether the operation succeeded or failed to
callers.
arunpaladin
approved these changes
Jun 18, 2026
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
2 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.
…gging summary
Description
Please include a summary of the changes and the related issues. Please also include relevant motivation and context. List
any dependencies that are required for this change.
Problem
Solution
Fixes # (issue if any)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also
list any relevant details for your test configuration
Checklist:
Other Information:
List any documentation updates that are needed for the Wiki
Summary by CodeRabbit
Chores
v3.Bug Fixes