Skip to content

fix: quote FQN before entity link comparison in self-approval prevention#26766

Merged
yan-3005 merged 1 commit intomainfrom
ram/fix-dotted-fqn-self-approval
Mar 25, 2026
Merged

fix: quote FQN before entity link comparison in self-approval prevention#26766
yan-3005 merged 1 commit intomainfrom
ram/fix-dotted-fqn-self-approval

Conversation

@yan-3005
Copy link
Copy Markdown
Contributor

Describe your changes:

Fixes

I worked on ... because ...

Type of change:

  • Bug fix
  • Improvement
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.

@yan-3005 yan-3005 self-assigned this Mar 25, 2026
Copilot AI review requested due to automatic review settings March 25, 2026 11:32
@yan-3005 yan-3005 added safe to test Add this label to run secure Github workflows on PRs To release Will cherry-pick this PR into the release branch backend labels Mar 25, 2026
@gitar-bot
Copy link
Copy Markdown

gitar-bot bot commented Mar 25, 2026

Code Review ✅ Approved

Fixes self-approval prevention by properly quoting fully qualified names before entity link comparison, handling edge cases with dotted FQNs. No issues found.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes self-approval prevention in governance approval workflows when the updater’s username contains dots by ensuring the username is quoted consistently before building the entity link used for assignee removal.

Changes:

  • Quote updatedBy via FullyQualifiedName.quoteName() before constructing the <#E::user::...> entity link used to remove self from assignees.
  • Add a focused unit regression test for dotted usernames and additional self-approval prevention cases.
  • Update the existing integration test to use a dotted username and escape quoted FQNs in the JSON patch payload.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/userTask/impl/SetApprovalAssigneesImpl.java Quotes updatedBy before entity-link comparison so self-removal works for dotted usernames.
openmetadata-service/src/test/java/org/openmetadata/service/governance/workflows/elements/nodes/userTask/impl/SetApprovalAssigneesImplTest.java Adds unit regression coverage for dotted/simple/null updatedBy self-approval prevention behavior.
openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/WorkflowDefinitionResourceIT.java Adjusts self-approval IT to use a dotted username and properly escape quoted FQNs in the reviewers patch JSON.

@github-actions
Copy link
Copy Markdown
Contributor

OpenMetadata Service New-Code Coverage

PASS. Required changed-line coverage: 90.00% overall and per touched production file.

  • Overall executable changed lines: 2/2 covered (100.00%)
  • Missed executable changed lines: 0
  • Non-executable changed lines ignored by JaCoCo: 1
  • Changed production files: 1
File Covered Missed Executable Non-exec Coverage Uncovered lines
openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/userTask/impl/SetApprovalAssigneesImpl.java 2 0 2 1 100.00% -

Only changed executable lines under openmetadata-service/src/main/java are counted. Test files, comments, imports, and non-executable lines are excluded.

@sonarqubecloud
Copy link
Copy Markdown

@yan-3005 yan-3005 enabled auto-merge (squash) March 25, 2026 13:08
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 25, 2026

🟡 Playwright Results — all passed (20 flaky)

✅ 3411 passed · ❌ 0 failed · 🟡 20 flaky · ⏭️ 209 skipped

Shard Passed Failed Flaky Skipped
🟡 Shard 1 450 0 5 2
🟡 Shard 2 602 0 3 32
🟡 Shard 3 613 0 2 28
🟡 Shard 4 597 0 6 47
✅ Shard 5 587 0 0 67
🟡 Shard 6 562 0 4 33
🟡 20 flaky test(s) (passed on retry)
  • Features/CustomizeDetailPage.spec.ts › API Endpoint - customization should work (shard 1, 1 retry)
  • Flow/Metric.spec.ts › Verify Related Metrics Update (shard 1, 1 retry)
  • Flow/Tour.spec.ts › Tour should work from help section (shard 1, 1 retry)
  • Flow/Tour.spec.ts › Tour should work from URL directly (shard 1, 1 retry)
  • Pages/UserCreationWithPersona.spec.ts › Create user with persona and verify on profile (shard 1, 1 retry)
  • Features/BulkEditEntity.spec.ts › Glossary (shard 2, 2 retries)
  • Features/BulkImport.spec.ts › Keyboard Delete selection (shard 2, 1 retry)
  • Features/Glossary/GlossaryHierarchy.spec.ts › should cancel move operation (shard 2, 1 retry)
  • Features/Permissions/GlossaryPermissions.spec.ts › Team-based permissions work correctly (shard 3, 1 retry)
  • Flow/ExploreDiscovery.spec.ts › Should display deleted assets when showDeleted is checked and deleted is not present in queryFilter (shard 3, 1 retry)
  • Pages/Customproperties-part2.spec.ts › entityReferenceList shows item count, scrollable list, no expand toggle (shard 4, 1 retry)
  • Pages/DataContracts.spec.ts › Create Data Contract and validate for Table (shard 4, 1 retry)
  • Pages/DomainAdvanced.spec.ts › Remove multiple assets from domain at once (shard 4, 1 retry)
  • Pages/DomainDataProductsRightPanel.spec.ts › Should display overview tab content for data product in domain context (shard 4, 1 retry)
  • Pages/Domains.spec.ts › Data Product announcement create, edit & delete (shard 4, 1 retry)
  • Pages/Domains.spec.ts › Rename domain with owners and experts preserves assignments (shard 4, 1 retry)
  • Pages/ODCSImportExport.spec.ts › Multi-object ODCS contract - object selector shows all schema objects (shard 6, 1 retry)
  • Pages/Users.spec.ts › Permissions for table details page for Data Consumer (shard 6, 1 retry)
  • Pages/Users.spec.ts › Check permissions for Data Steward (shard 6, 1 retry)
  • VersionPages/EntityVersionPages.spec.ts › Directory (shard 6, 1 retry)

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

@yan-3005 yan-3005 merged commit b0d80e3 into main Mar 25, 2026
62 of 91 checks passed
@yan-3005 yan-3005 deleted the ram/fix-dotted-fqn-self-approval branch March 25, 2026 14:36
@github-actions
Copy link
Copy Markdown
Contributor

Changes have been cherry-picked to the 1.12.4 branch.

github-actions bot pushed a commit that referenced this pull request Mar 25, 2026
yan-3005 added a commit that referenced this pull request Mar 26, 2026
yan-3005 added a commit that referenced this pull request Mar 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend safe to test Add this label to run secure Github workflows on PRs To release Will cherry-pick this PR into the release branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants