fix(structured-properties): allow updating properties that already have badge enabled #15845
+67
−5
Conversation
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
…ve badge enabled When updating a structured property that already has showAsAssetBadge=true, the validation was incorrectly failing with "Cannot have more than one property set with show as badge" even though the property being updated was the same one with the badge. The fix filters out the current entity URN from the badge conflict check, so validation only fails when a *different* property already has the badge. Fixes CUS-6349
|
✅ Meticulous spotted 0 visual differences across 952 screens tested: view results. Meticulous evaluated ~8 hours of user flows against your PR. Expected differences? Click here. Last updated for commit e74c1cc. This comment will update as new commits are pushed. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
datahub-cyborg
bot
added
the
needs-review
Add blank lines before code blocks and lists per Prettier requirements. Co-Authored-By: Claude Opus 4.5 <[email protected]>
| .build(); | ||
| // Only need to get first set, if there are more then will have to resolve bad state | ||
| ScrollResult scrollResult = scrollIterator.next(); | ||
| if (CollectionUtils.isNotEmpty(scrollResult.getEntities())) { |
Collaborator
There was a problem hiding this comment.
By shifting this you are removing a null check, are you sure this can never be null? Presumably it existed for a reason
| if (scrollResult.getEntities().size() > 1) { | ||
| // Filter out the current entity being updated - it's allowed to keep its own badge | ||
| List<SearchEntity> otherBadgeEntities = | ||
| scrollResult.getEntities().stream() |
Collaborator
There was a problem hiding this comment.
Additional filtering should occur after verifying that the initial list is not empty or null, no?
datahub-cyborg
bot
added
pending-submitter-response
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Fixes a bug where updating any setting on a structured property that already has
showAsAssetBadge=truewould fail with:The error occurs even though the property being updated IS the one with the badge - no new badge is being added.
Root Cause
ShowPropertyAsBadgeValidatorsearches for all properties withshowAsAssetBadge=true, but never excludes the current entity being updated from the conflict check. So when updating property X which already has the badge, it finds X in the search results and incorrectly treats it as a conflict.Fix
Filter out the current entity URN from the search results before checking for badge conflicts. Validation now only fails when a different property already has the badge enabled.
Additional Changes
Added PR guidelines section to
CLAUDE.mdto ensure Claude Code follows the project's PR template when creating pull requests.Related Issues
Test Plan
testValidUpsertWhenUpdatingSamePropertyWithBadge()that verifies updating a property that already has the badge succeedsshowAsAssetBadge=trueenabledChecklist