Fix asset deduplication logic#465
Merged
Merged
Conversation
Signed-off-by: somiljain2006 <[email protected]>
Signed-off-by: somiljain2006 <[email protected]>
somiljain2006
force-pushed
the
Duplicate-reporting-issue
branch
from
June 8, 2026 20:54
369c5e2 to
9d0f79b
Compare
Contributor
Author
|
@san-zrl Can you review this pr? |
san-zrl
requested changes
Jun 9, 2026
san-zrl
left a comment
san-zrl
left a comment
Contributor
There was a problem hiding this comment.
Potential Edge Cases NOT Covered:
- Multiple overlapping roots: If 3+ algorithms overlap (e.g., AES, AES-128, AES-256 all detected), only one pair is processed per iteration. The reorganizer may need mu
- Case sensitivity edge case: equalsIgnoreCase("Unknown") is used, but algorithm name comparison in isRelatedCipherFamily() uses case-sensitive equals(). This could cause issues if algorithm names have inconsistent casing.
- Ambiguous prefix matching: "RC2" and "RC256" would be considered related (RC2 is prefix of RC256 followed by digit '5'), which may not be intended.
- Child key conflicts: When merging children with toKeep.put(child), if both nodes have children of the same Kind, only one survives (the check !toKeep.getChildren().containsKey(child.getKind()) prevents overwriting, but the removed node's child is lost).
- Empty name strings: While null is handled, empty strings ("") are not explicitly checked and could cause unexpected behavior in prefix matching.
- Non-Algorithm BlockCipher nodes: The rule checks instanceof Algorithm but is registered for BlockCipher.class. If BlockCipher nodes exist that aren't Algorithms, they won't be deduplicated.
Recommendation
The implementation works correctly for the intended use case (BouncyCastle AES + CTR mode detection). However, consider:
- Adding tests for multiple overlapping roots
- Handling child merge conflicts more explicitly
- Adding validation for edge cases in algorithm name matching
- Documenting the reorganizer's behavior when multiple passes are needed
Signed-off-by: somiljain2006 <[email protected]>
somiljain2006
force-pushed
the
Duplicate-reporting-issue
branch
from
June 9, 2026 09:35
d5fb3f5 to
40b857b
Compare
Contributor
Author
|
@san-zrl Thanks for the review. I've addressed the reported edge cases:
For the remaining points:
|
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.
Fixes #295
When processing certain Bouncy Castle block cipher constructions, multiple block-cipher-related nodes can be produced for the same source location. The existing deduplication logic relied on strict checks against the BlockCipher interface, which prevented some algorithm nodes from participating in reorganization even when they represented block-cipher assets.
This change updates the deduplication rule to operate on Algorithm nodes while preserving the existing BlockCipher node-kind filtering. It also adds null-safe handling for algorithm names and improves merging behavior when generic wrapper nodes are encountered.
Tests were added to cover: