Skip to content

Removing dataProduct asset should remove output port asset as well#25885

Merged
harshach merged 2 commits intomainfrom
fix_data_products_assets
Feb 15, 2026
Merged

Removing dataProduct asset should remove output port asset as well#25885
harshach merged 2 commits intomainfrom
fix_data_products_assets

Conversation

@harshach
Copy link
Copy Markdown
Collaborator

@harshach harshach commented Feb 14, 2026

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.

Summary by Gitar

  • Fixed output port cleanup: Modified DataProductRepository.bulkRemoveAssets() to delete OUTPUT_PORT relationships when removing data product assets, preventing orphaned relationships
  • Improved operation tracking: Changed from checking overall bulk operation status to iterating through getSuccessRequest() for individual operations, improving error resilience
  • Comprehensive test coverage: Added integration test test_removingAssetFromDataProductRemovesItFromOutputPorts to validate API behavior and Playwright E2E test for UI asset removal workflow

Comment on lines +251 to +252
deleteRelationship(
dataProduct.getId(), DATA_PRODUCT, ref.getId(), ref.getType(), Relationship.OUTPUT_PORT);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Bug: INPUT_PORT relationship not cleaned up when removing assets

When an asset is removed from a data product, only the OUTPUT_PORT relationship is deleted (line 252), but the INPUT_PORT relationship is not. An asset can be designated as either an input port or an output port (not both, as enforced by validation at line 331). If the removed asset was an input port, the INPUT_PORT relationship will be left orphaned.\n\nThe fix should also delete the INPUT_PORT relationship for the removed asset. Since deleteRelationship is a no-op when no matching row exists, it's safe to call both deletes unconditionally.

Suggested fix:

    for (BulkResponse response : listOrEmpty(result.getSuccessRequest())) {
      EntityReference ref = (EntityReference) response.getRequest();
      LineageUtil.removeDataProductsLineage(
          ref.getId(), ref.getType(), List.of(dataProduct.getEntityReference()));
      deleteRelationship(
          dataProduct.getId(), DATA_PRODUCT, ref.getId(), ref.getType(), Relationship.OUTPUT_PORT);
      deleteRelationship(
          dataProduct.getId(), DATA_PRODUCT, ref.getId(), ref.getType(), Relationship.INPUT_PORT);
    }

Was this helpful? React with 👍 / 👎

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Feb 14, 2026

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 65%
65.69% (56268/85654) 45.11% (29416/65205) 47.94% (8891/18548)

@gitar-bot
Copy link
Copy Markdown

gitar-bot Bot commented Feb 14, 2026

🔍 CI failure analysis for cd88924: CI failures persist: MCP tests (Docker version incompatibility), integration tests (resource cleanup timing), Playwright E2E (28 flaky tests across shards), Python 3.10 CockroachDB profiler (2 failed). Python 3.11 all tests passed. All unrelated to PR changes.

Issue

Multiple CI jobs continue to fail after merging main into PR branch (commit cd88924):

1. MCP Tests: Docker client version incompatibility
2. Integration Tests (Java): Resource cleanup timing issues in both database combinations
3. Playwright E2E Tests: Flaky test failures in multiple shards
4. Python 3.10 Tests: CockroachDB profiler integration test failures (confirmed)
5. Python 3.11 Tests: ✅ All tests passed

Root Cause

MCP Tests - Docker Compatibility Issue

Error: client version 1.32 is too old. Minimum supported API version is 1.44

Details: 2 tests failed (McpIntegrationTest, McpToolsValidationTest), 36 passed.

Integration Tests - Resource Lifecycle

Jobs: integration-tests-postgres-opensearch, integration-tests-mysql-elasticsearch

Identical failure pattern: Async background operations fail:

java.lang.RuntimeException: Connection pool shut down
java.lang.IllegalStateException: NDManager has been closed already

Pattern: Race condition between test completion and resource cleanup in virtual threads.

Playwright E2E Tests - Flakiness

Shard 3/6: 1 failed (Settings navigation drag-and-drop), 16 flaky
Shard 4/6: 1 failed (Lineage settings config), 12 flaky

Total: 28 flaky tests with element timing issues.

Python Tests - Version-Specific Failures

Python 3.10 (job 63639151162) - CONFIRMED FAILURES:

  • 2 failed: test_table_metric_computer_cockroach.py
    • TestCockroachTableMetricComputer::test_compute_returns_row_count
    • TestCockroachTableMetricComputer::test_compute_returns_column_metadata
  • 549 passed, 21 skipped, 1 xfailed
  • Runtime: 1:21:07

Python 3.11 (job 63639151158) - ✅ ALL PASSED:

  • 4414 total tests passed (3863 unit + 551 integration)
  • No failures
  • Runtime: ~1.5 hours

Details

Complete Test Summary:

  • MCP: 2 failed (Docker incompatibility), 36 passed
  • Java Integration: Resource cleanup errors (both PostgreSQL+OpenSearch and MySQL+Elasticsearch)
  • Playwright: 2 failed, 28 flaky across shards 3/6 and 4/6
  • Python 3.10: 2 failed (CockroachDB profiler), 549 passed
  • Python 3.11: ✅ 4414 passed, 0 failed

Unrelated to PR Changes

This PR modifies:

  1. DataProductRepository.bulkRemoveAssets() - OUTPUT_PORT relationship cleanup
  2. DataProductResourceIT.java - integration test for data products
  3. InputOutputPorts.spec.ts - E2E test for input/output ports

Evidence of no relation:

  1. MCP failures: Different module, Docker infrastructure issue
  2. Integration failures: Vector embedding/search services not touched
  3. Playwright failures: Settings, lineage, custom properties, glossary, permissions - none related to data product asset removal
  4. Python failures:
    • CockroachDB profiler tests (database-specific metrics/profiling)
    • Python 3.11 passes completely - same codebase, proving failures are version/environment-specific
    • No profiler or CockroachDB code in PR

PR's own tests pass:

  • Integration test: test_removingAssetFromDataProductRemovesItFromOutputPorts
  • E2E test: InputOutputPorts.spec.ts ✅ (NOT in failed/flaky lists)
  • Python 3.11: All tests including data product tests ✅

Conclusion: All failures are pre-existing infrastructure issues (Docker version incompatibility, resource cleanup timing), test flakiness (Playwright UI timing with 28 flaky tests), and version-specific issues (Python 3.10 CockroachDB profiler fails; Python 3.11 same code passes completely). None are related to the data product asset cleanup changes in this PR.

Code Review ⚠️ Changes requested 0 resolved / 2 findings

The OUTPUT_PORT cleanup fix in bulkRemoveAssets is correct and well-tested, but two previous findings remain unresolved: bulkAddAssets still uses the old ApiStatus.SUCCESS gate (inconsistent with the new per-item approach), and INPUT_PORT relationships are still not cleaned up when removing assets.

⚠️ Bug: bulkAddAssets lineage skipped on partial success

📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/DataProductRepository.java:234-239

The bulkAddAssets method (line 234) still uses the old pattern: it checks result.getStatus().equals(ApiStatus.SUCCESS) and iterates over all request.getAssets(). This means on a partial success (some assets added, some failed), lineage is not added for any asset — even the ones that succeeded.

The bulkRemoveAssets method was just fixed to iterate over result.getSuccessRequest() for exactly this reason. The same fix pattern should be applied to bulkAddAssets for consistency and correctness.

Suggested fix
    for (BulkResponse response : listOrEmpty(result.getSuccessRequest())) {
      EntityReference ref = (EntityReference) response.getRequest();
      LineageUtil.addDataProductsLineage(
          ref.getId(), ref.getType(), List.of(dataProduct.getEntityReference()));
    }
⚠️ Bug: INPUT_PORT relationship not cleaned up when removing assets

📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/DataProductRepository.java:251-252

When an asset is removed from a data product, only the OUTPUT_PORT relationship is deleted (line 252), but the INPUT_PORT relationship is not. An asset can be designated as either an input port or an output port (not both, as enforced by validation at line 331). If the removed asset was an input port, the INPUT_PORT relationship will be left orphaned.\n\nThe fix should also delete the INPUT_PORT relationship for the removed asset. Since deleteRelationship is a no-op when no matching row exists, it's safe to call both deletes unconditionally.

Suggested fix
    for (BulkResponse response : listOrEmpty(result.getSuccessRequest())) {
      EntityReference ref = (EntityReference) response.getRequest();
      LineageUtil.removeDataProductsLineage(
          ref.getId(), ref.getType(), List.of(dataProduct.getEntityReference()));
      deleteRelationship(
          dataProduct.getId(), DATA_PRODUCT, ref.getId(), ref.getType(), Relationship.OUTPUT_PORT);
      deleteRelationship(
          dataProduct.getId(), DATA_PRODUCT, ref.getId(), ref.getType(), Relationship.INPUT_PORT);
    }

Tip

Comment Gitar fix CI or enable auto-apply: gitar auto-apply:on

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

@sonarqubecloud
Copy link
Copy Markdown

@harshach harshach merged commit 9f8529e into main Feb 15, 2026
24 of 33 checks passed
@harshach harshach deleted the fix_data_products_assets branch February 15, 2026 00:30
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.

1 participant