Skip to content

Fix #26863 certification tags leaking into tags field on LIST and missing appliedBy audit trail#26876

Merged
yan-3005 merged 7 commits intomainfrom
ram/#26863
Mar 31, 2026
Merged

Fix #26863 certification tags leaking into tags field on LIST and missing appliedBy audit trail#26876
yan-3005 merged 7 commits intomainfrom
ram/#26863

Conversation

@yan-3005
Copy link
Copy Markdown
Contributor

@yan-3005 yan-3005 commented Mar 31, 2026

  • batchFetchTags() now filters out certification tags (matching the existing single-entity getTags() behavior), preventing cert tags from appearing in the tags field when entities are fetched via the LIST endpoint
  • applyCertification() propagates tagLabel.getAppliedBy() instead of null, preserving the audit trail of who applied the certification
  • updateCertification() sets appliedBy from updatingUser before calling applyCertification(), so the author is recorded on PATCH/PUT
  • applyCertificationBatch() includes appliedBy from the incoming tagLabel when building the batch TagLabel
  • Unit tests: batchFetchTagsFiltersCertificationTags, applyCertificationPropagatesAppliedBy, applyCertificationBatchPropagatesAppliedBy
  • Integration test: test_certificationTagNotLeakingIntoTagsField verifies both GET (single) and LIST (batch) paths exclude cert tags from tags field

Fixes findings from Gitar bot review on #26826

Describe your changes:

Fixes #26863

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.

…pliedBy audit trail

- batchFetchTags() now filters out certification tags (matching the existing
  single-entity getTags() behavior), preventing cert tags from appearing in
  the tags field when entities are fetched via the LIST endpoint
- applyCertification() propagates tagLabel.getAppliedBy() instead of null,
  preserving the audit trail of who applied the certification
- updateCertification() sets appliedBy from updatingUser before calling
  applyCertification(), so the author is recorded on PATCH/PUT
- applyCertificationBatch() includes appliedBy from the incoming tagLabel
  when building the batch TagLabel
- Unit tests: batchFetchTagsFiltersCertificationTags,
  applyCertificationPropagatesAppliedBy, applyCertificationBatchPropagatesAppliedBy
- Integration test: test_certificationTagNotLeakingIntoTagsField verifies
  both GET (single) and LIST (batch) paths exclude cert tags from tags field

Fixes findings from Gitar bot review on #26826

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@yan-3005 yan-3005 self-assigned this Mar 31, 2026
Copilot AI review requested due to automatic review settings March 31, 2026 06:16
@yan-3005 yan-3005 added safe to test Add this label to run secure Github workflows on PRs backend labels Mar 31, 2026
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 two certification-related issues in the backend tagging pipeline: (1) preventing certification tags from appearing in the regular tags field for LIST (batch) fetches, and (2) preserving appliedBy when applying/updating certifications so audit trails correctly record who applied them.

Changes:

  • Filter out certification tags from batchFetchTags() results to match single-entity getTags() behavior.
  • Propagate/set appliedBy when applying certifications (single + batch) and during certification updates.
  • Add unit + integration tests covering both batch tag filtering and appliedBy propagation.

Reviewed changes

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

File Description
openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java Filters cert tags out of batch tag fetches and propagates appliedBy in certification application/update flows.
openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/EntityRepositoryCertificationTest.java Adds unit tests for cert-tag filtering in batch fetch and appliedBy propagation in certification writes.
openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TagResourceIT.java Adds an integration test ensuring certification tags don’t leak into the tags field on both GET and LIST paths.

…ry list copy

- applyCertification/applyCertificationBatch: use entity.getUpdatedBy()
  instead of tagLabel.getAppliedBy() so the audit trail reflects the
  authenticated server-side user, preventing client-provided appliedBy
  from being trusted/spoofed
- batchFetchTags: removeIf directly on the mutable list returned by
  populateTagLabel instead of defensively copying on every entity
- Update unit tests to set updatedBy on the entity (not on the tagLabel)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 31, 2026

🟡 Playwright Results — all passed (21 flaky)

✅ 3445 passed · ❌ 0 failed · 🟡 21 flaky · ⏭️ 223 skipped

Shard Passed Failed Flaky Skipped
🟡 Shard 1 452 0 3 2
🟡 Shard 2 616 0 2 32
🟡 Shard 3 617 0 3 27
🟡 Shard 4 615 0 9 47
✅ Shard 5 587 0 0 67
🟡 Shard 6 558 0 4 48
🟡 21 flaky test(s) (passed on retry)
  • Features/CustomizeDetailPage.spec.ts › Dashboard - customization should work (shard 1, 2 retries)
  • Pages/AuditLogs.spec.ts › should apply both User and EntityType filters simultaneously (shard 1, 2 retries)
  • Pages/UserCreationWithPersona.spec.ts › Create user with persona and verify on profile (shard 1, 1 retry)
  • Features/BulkEditEntity.spec.ts › Glossary (shard 2, 1 retry)
  • Features/DataQuality/BundleSuiteBulkOperations.spec.ts › Bulk selection operations (shard 2, 1 retry)
  • Features/Permissions/GlossaryPermissions.spec.ts › Team-based permissions work correctly (shard 3, 1 retry)
  • Flow/CustomizeWidgets.spec.ts › Total Data Assets Widget (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 Api Collection (shard 4, 1 retry)
  • Pages/DataContractsSemanticRules.spec.ts › Validate Description Rule Is_Set (shard 4, 1 retry)
  • Pages/DomainAdvanced.spec.ts › Remove multiple assets from domain at once (shard 4, 1 retry)
  • Pages/Domains.spec.ts › Rename domain with tags and glossary terms preserves associations (shard 4, 1 retry)
  • Pages/DomainUIInteractions.spec.ts › Select domain from global dropdown filters explore (shard 4, 1 retry)
  • Pages/Entity.spec.ts › Glossary Term Add, Update and Remove for child entities (shard 4, 1 retry)
  • Pages/Entity.spec.ts › Glossary Term Add, Update and Remove for child entities (shard 4, 1 retry)
  • Pages/Entity.spec.ts › Inactive Announcement create & delete (shard 4, 1 retry)
  • Pages/Glossary.spec.ts › Column dropdown drag-and-drop functionality for Glossary Terms table (shard 6, 1 retry)
  • Pages/HyperlinkCustomProperty.spec.ts › should show No Data placeholder when hyperlink has no value (shard 6, 1 retry)
  • Pages/UserDetails.spec.ts › Create team with domain and verify visibility of inherited domain in user profile after team removal (shard 6, 1 retry)
  • Pages/Users.spec.ts › Permissions for table details page for Data Consumer (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

Copilot AI review requested due to automatic review settings March 31, 2026 08:29
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

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

- Rename applyCertificationPropagatesAppliedBy to
  applyCertificationUsesEntityUpdatedByAsAppliedBy
- Rename applyCertificationBatchPropagatesAppliedBy to
  applyCertificationBatchUsesEntityUpdatedByAsAppliedBy
  Names now accurately reflect that entity.getUpdatedBy() (server-side)
  is the source of appliedBy, not the incoming tagLabel field
- Run yarn ui-checkstyle and mvn spotless:apply to fix formatting

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 64%
64.87% (58823/90674) 44.47% (30904/69491) 47.69% (9336/19576)

…perator

The `?` operator in `json::jsonb ? 'certification'` was interpreted by JDBI3
as a positional parameter, causing UnableToCreateStatementException during
the v1125 certification migration on PostgreSQL. Escape with `??` so JDBI3
passes a literal `?` to the database driver.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 31, 2026 12:15
@gitar-bot
Copy link
Copy Markdown

gitar-bot bot commented Mar 31, 2026

Code Review ✅ Approved

Fixes certification tags leaking into the tags field on LIST operations and restores the missing appliedBy audit trail. No issues found.

Options

Display: compact → Showing less information.

Comment with these commands to change:

Compact
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

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

@yan-3005 yan-3005 changed the title Fix certification tags leaking into tags field on LIST and missing appliedBy audit trail Fix #26863 certification tags leaking into tags field on LIST and missing appliedBy audit trail Mar 31, 2026
@sonarqubecloud
Copy link
Copy Markdown

@sonarqubecloud
Copy link
Copy Markdown

@yan-3005 yan-3005 enabled auto-merge (squash) March 31, 2026 14:09
@yan-3005 yan-3005 merged commit 2ef1420 into main Mar 31, 2026
66 of 67 checks passed
@yan-3005 yan-3005 deleted the ram/#26863 branch March 31, 2026 16:23
yan-3005 added a commit that referenced this pull request Apr 1, 2026
…sing appliedBy audit trail (#26876)

* Fix certification tags leaking into tags field on LIST and missing appliedBy audit trail

- batchFetchTags() now filters out certification tags (matching the existing
  single-entity getTags() behavior), preventing cert tags from appearing in
  the tags field when entities are fetched via the LIST endpoint
- applyCertification() propagates tagLabel.getAppliedBy() instead of null,
  preserving the audit trail of who applied the certification
- updateCertification() sets appliedBy from updatingUser before calling
  applyCertification(), so the author is recorded on PATCH/PUT
- applyCertificationBatch() includes appliedBy from the incoming tagLabel
  when building the batch TagLabel
- Unit tests: batchFetchTagsFiltersCertificationTags,
  applyCertificationPropagatesAppliedBy, applyCertificationBatchPropagatesAppliedBy
- Integration test: test_certificationTagNotLeakingIntoTagsField verifies
  both GET (single) and LIST (batch) paths exclude cert tags from tags field

Fixes findings from Gitar bot review on #26826

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Address Copilot review: use server-side updatedBy and avoid unnecessary list copy

- applyCertification/applyCertificationBatch: use entity.getUpdatedBy()
  instead of tagLabel.getAppliedBy() so the audit trail reflects the
  authenticated server-side user, preventing client-provided appliedBy
  from being trusted/spoofed
- batchFetchTags: removeIf directly on the mutable list returned by
  populateTagLabel instead of defensively copying on every entity
- Update unit tests to set updatedBy on the entity (not on the tagLabel)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Fix certification column missing on all list/service views

Now that batchFetchTags() correctly filters certification tags from the
tags field (matching single-entity getTags() behavior), list views can
no longer rely on cert tags leaking into tags as a fallback. Request
the dedicated certification field explicitly instead.

- Add TabSpecificField.CERTIFICATION to commonTableFields — fixes
  ServiceDetailsPage (all 12 fetch functions), SchemaTablesTab,
  DataModelsTable, and ServiceVersionPage.fetchDirectories in one shot
- Fix ServiceVersionPage: 8 fetch functions (databases, topics,
  dashboards, pipelines, mlModels, containers, searchIndexes,
  collections) that manually specify OWNERS,TAGS — each updated to
  include CERTIFICATION
- DatabaseSchemaTable: remove now-redundant explicit CERTIFICATION
  addition since commonTableFields already includes it

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Address Copilot review: rename misleading test names and fix lint

- Rename applyCertificationPropagatesAppliedBy to
  applyCertificationUsesEntityUpdatedByAsAppliedBy
- Rename applyCertificationBatchPropagatesAppliedBy to
  applyCertificationBatchUsesEntityUpdatedByAsAppliedBy
  Names now accurately reflect that entity.getUpdatedBy() (server-side)
  is the source of appliedBy, not the incoming tagLabel field
- Run yarn ui-checkstyle and mvn spotless:apply to fix formatting

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Fix JDBI3 positional param error for PostgreSQL JSONB key-existence operator

The `?` operator in `json::jsonb ? 'certification'` was interpreted by JDBI3
as a positional parameter, causing UnableToCreateStatementException during
the v1125 certification migration on PostgreSQL. Escape with `??` so JDBI3
passes a literal `?` to the database driver.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
(cherry picked from commit 2ef1420)
ulixius9 pushed a commit that referenced this pull request Apr 6, 2026
…sing appliedBy audit trail (#26876)

* Fix certification tags leaking into tags field on LIST and missing appliedBy audit trail

- batchFetchTags() now filters out certification tags (matching the existing
  single-entity getTags() behavior), preventing cert tags from appearing in
  the tags field when entities are fetched via the LIST endpoint
- applyCertification() propagates tagLabel.getAppliedBy() instead of null,
  preserving the audit trail of who applied the certification
- updateCertification() sets appliedBy from updatingUser before calling
  applyCertification(), so the author is recorded on PATCH/PUT
- applyCertificationBatch() includes appliedBy from the incoming tagLabel
  when building the batch TagLabel
- Unit tests: batchFetchTagsFiltersCertificationTags,
  applyCertificationPropagatesAppliedBy, applyCertificationBatchPropagatesAppliedBy
- Integration test: test_certificationTagNotLeakingIntoTagsField verifies
  both GET (single) and LIST (batch) paths exclude cert tags from tags field

Fixes findings from Gitar bot review on #26826

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Address Copilot review: use server-side updatedBy and avoid unnecessary list copy

- applyCertification/applyCertificationBatch: use entity.getUpdatedBy()
  instead of tagLabel.getAppliedBy() so the audit trail reflects the
  authenticated server-side user, preventing client-provided appliedBy
  from being trusted/spoofed
- batchFetchTags: removeIf directly on the mutable list returned by
  populateTagLabel instead of defensively copying on every entity
- Update unit tests to set updatedBy on the entity (not on the tagLabel)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Fix certification column missing on all list/service views

Now that batchFetchTags() correctly filters certification tags from the
tags field (matching single-entity getTags() behavior), list views can
no longer rely on cert tags leaking into tags as a fallback. Request
the dedicated certification field explicitly instead.

- Add TabSpecificField.CERTIFICATION to commonTableFields — fixes
  ServiceDetailsPage (all 12 fetch functions), SchemaTablesTab,
  DataModelsTable, and ServiceVersionPage.fetchDirectories in one shot
- Fix ServiceVersionPage: 8 fetch functions (databases, topics,
  dashboards, pipelines, mlModels, containers, searchIndexes,
  collections) that manually specify OWNERS,TAGS — each updated to
  include CERTIFICATION
- DatabaseSchemaTable: remove now-redundant explicit CERTIFICATION
  addition since commonTableFields already includes it

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Address Copilot review: rename misleading test names and fix lint

- Rename applyCertificationPropagatesAppliedBy to
  applyCertificationUsesEntityUpdatedByAsAppliedBy
- Rename applyCertificationBatchPropagatesAppliedBy to
  applyCertificationBatchUsesEntityUpdatedByAsAppliedBy
  Names now accurately reflect that entity.getUpdatedBy() (server-side)
  is the source of appliedBy, not the incoming tagLabel field
- Run yarn ui-checkstyle and mvn spotless:apply to fix formatting

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Fix JDBI3 positional param error for PostgreSQL JSONB key-existence operator

The `?` operator in `json::jsonb ? 'certification'` was interpreted by JDBI3
as a positional parameter, causing UnableToCreateStatementException during
the v1125 certification migration on PostgreSQL. Escape with `??` so JDBI3
passes a literal `?` to the database driver.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Analysis: Analyse the gitar bot comments on Certification

3 participants