fix(data-products): reject non-data-asset entities as ports + drop bad tableColumn port rows#30186
Conversation
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
github-actions
Bot
added
backend
safe to test
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
🟡 Playwright Results — all passed (34 flaky)✅ 4537 passed · ❌ 0 failed · 🟡 34 flaky · ⏭️ 95 skipped
🟡 34 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
c32f0f4 to
5cabbfb
Compare
Code Review ✅ Approved 1 resolved / 1 findingsHardens Data Product ports by validating entity eligibility during writes and skipping unregistered types during reads, resolving the 500 error for invalid port relationships. ✅ 1 resolved✅ Edge Case: isPortEligibleType can throw for time-series entity types
OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
…d tableColumn port rows
A data product port must reference a data asset (a dataset such as a table,
topic, or dashboard). Adding a port did not validate this, and a historical
input/output port relationship pointing at a column (entity type "tableColumn",
a search-only pseudo type with no repository) makes
GET /dataProducts/{..}/portsView 500 with "Entity repository for tableColumn
not found".
- Add-ports now rejects any reference that is not a real entity supporting the
dataProducts field, reported as a per-row failure (consistent with the
existing opposite-port and ownership checks). Eligibility is derived from the
entity's own capabilities (repository + dataProducts field, excluding
time-series types), so new asset types are covered automatically with no list
to maintain.
- Adds a 2.0.0 data migration that deletes existing dataProduct -> tableColumn
input/output port relationships so already-affected instances stop 500-ing.
Adds an integration test asserting a non-data-asset entity is rejected as a
port.
5cabbfb to
e31393b
Compare
sonika-shah
changed the title
…s-non-asset-guard # Conflicts: # bootstrap/sql/migrations/native/2.0.0/mysql/postDataMigrationSQLScript.sql # bootstrap/sql/migrations/native/2.0.0/postgres/postDataMigrationSQLScript.sql
This comment has been minimized.
This comment has been minimized.
…y dropped populateEntityReferences works on a copy and executeBulkPortsOperation discards its return, so an unresolvable ref (tableColumn) is NOT stripped from the assets list — it reaches the in-loop eligibility check and is reported as a per-row failure. This test pins that behavior.
This comment has been minimized.
This comment has been minimized.
Code Review ✅ Approved 2 resolved / 2 findingsPrevents 500 errors in Data Product views by implementing write-time validation for port entities and migrating historical invalid 'tableColumn' relationships. No open issues remain following the implementation of the eligibility guard and verification of idempotent migration scripts. ✅ 2 resolved✅ Edge Case: isPortEligibleType can throw for time-series entity types
✅ Bug: Port eligibility check moved after populateEntityReferences drops bad refs
OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
Problem
Opening a Data Product detail page can fail with a 500 and the toast:
The failing request is
GET /api/v1/dataProducts/name/{fqn}/portsView.Root cause
A data product port is a relationship (
INPUT_PORT/OUTPUT_PORT) to some entity, andgetPaginatedPortsloads each port withEntity.getEntities, which resolves a repositoryvia
Entity.getEntityRepository. That throwsEntityNotFoundExceptionfor a type with noregistered repository — including
tableColumn, a search-index-only pseudo type(
Entity.TABLE_COLUMN) that is deliberately not a first-class entity. So a single portrelationship pointing at a column makes the whole
portsViewresponse 500.A port is meant to reference a data asset, and the add-ports operation did not enforce this.
Fix
Validate on add. Adding a port now rejects any reference that is not a real entity
supporting the
dataProductsfield, reported as a per-row failure (consistent with theexisting opposite-port and ownership checks in the same loop):
No hardcoded type list, so new asset types are covered automatically.
Migration to clean existing data. A 2.0.0
postDataMigrationSQLScriptdeletes anyexisting
dataProduct -> tableColumninput/output port relationships (relation 23/24) soalready-affected instances stop 500-ing. The read path (
getPaginatedPorts) is leftunchanged — no per-request filtering.
Port eligibility == data product asset eligibility (intentional)
The eligibility check above is exactly the rule that governs what can be added as a data
product asset (
validateAssetDataProductAssignmentrequires the entity to carry adataProductsfield, and output ports already require the target to be a data productasset). So a type is a valid port iff it is a valid data product asset — ports do not
introduce a separate, narrower "dataset-only" rule. Concretely this means dataset entities
(table, topic, dashboard, container, mlmodel, …) and every other data-product-taggable entity
(e.g.
glossaryTerm,tag,testCase) are accepted, while non-assets (user,team,domain,query, …) and the non-entitytableColumnare rejected — the same set the assetsAPI accepts. Keeping ports and assets on one shared rule avoids drift and needs no maintained
whitelist.
Tests
Adds
DataProductResourceIT#test_addPort_rejectsNonDataAssetEntity: a user (a real entitythat is not a data asset) is rejected when added as an input port, and the ports view still
loads with zero ports. Fails without change (1).
Greptile Summary
This PR fixes a 500 error on the
portsViewendpoint caused bytableColumnport relationships that reference a pseudo-type with no registered entity repository. The fix has two parts: a write-time validation that rejects non-data-asset entities when adding ports, and a 2.0.0 SQL migration to remove existing corrupt rows.isPortEligibleTypechecks that a port target has an entity repository, is not a time-series entity, and has adataProductsschema field — exactly the same rule used by the assets API — and issues a per-row failure in the bulk response instead of persisting the relationship.dataProduct → tableColumnINPUT_PORT (relation 23) and OUTPUT_PORT (relation 24) rows fromentity_relationshipin both MySQL and Postgres scripts; ordinals are correct per theentityRelationship.jsonenum definition.user(real entity, not a data asset) and atableColumn(pseudo-type) are each rejected as a port with a per-row failure, and that the ports view loads cleanly afterward.Confidence Score: 5/5
Safe to merge — the write-side guard, migration, and tests are all consistent and correct.
The validation logic is minimal and well-scoped: it rejects ineligible types before any DB write, uses the same eligibility rule as the existing assets API, and the read path is left completely unchanged. The SQL migration deletes only the known problematic relationship rows using verified ordinals. Two integration tests cover both the real-entity-non-asset and pseudo-type cases.
No files require special attention.
Important Files Changed
isPortEligibleTypehelper and a guard at the top of the add branch inexecuteBulkPortsOperation; read path (getPaginatedPorts) is left unchanged, relying on the migration to remove bad rows.dataProduct → tableColumnINPUT_PORT/OUTPUT_PORT rows; relation ordinals 23 and 24 match the schema enum definition.addInputPortsWithResulthelper; tests correctly assert per-row failure status and zero ports in the subsequent view response.Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Client participant DataProductRepository participant EntityUtil participant DB Client->>DataProductRepository: "PUT /inputPorts/add {assets}" DataProductRepository->>EntityUtil: populateEntityReferences(assets) Note over EntityUtil: Updates resolvable refs in-place. Returns filtered copy (discarded). Original assets list unchanged. EntityUtil-->>DataProductRepository: (return value discarded) loop for each ref in assets alt not isPortEligibleType DataProductRepository-->>DataProductRepository: per-row failure added else eligible type in opposite port DataProductRepository-->>DataProductRepository: per-row failure added else eligible DataProductRepository->>DB: addRelationship INPUT_PORT DataProductRepository-->>DataProductRepository: per-row success end end DataProductRepository-->>Client: BulkOperationResult%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant Client participant DataProductRepository participant EntityUtil participant DB Client->>DataProductRepository: "PUT /inputPorts/add {assets}" DataProductRepository->>EntityUtil: populateEntityReferences(assets) Note over EntityUtil: Updates resolvable refs in-place. Returns filtered copy (discarded). Original assets list unchanged. EntityUtil-->>DataProductRepository: (return value discarded) loop for each ref in assets alt not isPortEligibleType DataProductRepository-->>DataProductRepository: per-row failure added else eligible type in opposite port DataProductRepository-->>DataProductRepository: per-row failure added else eligible DataProductRepository->>DB: addRelationship INPUT_PORT DataProductRepository-->>DataProductRepository: per-row success end end DataProductRepository-->>Client: BulkOperationResultReviews (5): Last reviewed commit: "test(data-products): assert tableColumn ..." | Re-trigger Greptile