Skip to content

fix: resolve correct aspect name per entity type in UpdateDescription#86

Merged
cjimti merged 3 commits intomainfrom
fix/85-update-description-entity-type-aware
Mar 6, 2026
Merged

fix: resolve correct aspect name per entity type in UpdateDescription#86
cjimti merged 3 commits intomainfrom
fix/85-update-description-entity-type-aware

Conversation

@cjimti
Copy link
Member

@cjimti cjimti commented Mar 6, 2026

Closes #85.

Problem

UpdateDescription() hardcoded editableDatasetProperties as the aspect name (line 59) and readEditableProperties() used the same hardcoded value (line 67). This caused DataHub to reject writes for any non-dataset entity — dashboards, glossary terms, domains, data products, containers, charts, data flows, and data jobs all failed.

This blocked apply_knowledge in mcp-data-platform from updating descriptions on non-dataset entities.

Solution

Introduced a mapping table (descriptionAspectMap) that resolves the correct aspect name and field name for each entity type extracted from the URN:

Entity Type Aspect Name Field
dataset editableDatasetProperties description
dashboard editableDashboardProperties description
chart editableChartProperties description
dataFlow editableDataFlowProperties description
dataJob editableDataJobProperties description
container editableContainerProperties description
dataProduct dataProductProperties description
domain domainProperties description
glossaryTerm glossaryTermInfo definition
glossaryNode glossaryNodeInfo definition

Special cases handled

  • glossaryTerm / glossaryNode: The description field is definition, not description — the mapping resolves this automatically.
  • dataProduct / domain: These use non-editable property aspects (no editable prefix).
  • Unsupported types (e.g., tag): Return a clear ErrUnsupportedEntityType sentinel error.

Key design decision

Changed editablePropertiesAspect from a fixed struct to a map[string]json.RawMessage wrapper. This preserves all existing fields during read-modify-write regardless of which aspect is being updated — critical because different aspects have different schemas (e.g., glossaryTermInfo has termSource, name, sourceRef, etc. alongside definition).

Changes

pkg/client/write.go

  • Added descriptionAspectInfo struct and descriptionAspectMap (10 entity types)
  • Added ErrUnsupportedEntityType sentinel error
  • Added lookupDescriptionAspect() resolver function
  • Replaced fixed editablePropertiesAspect struct with map-based implementation + MarshalJSON() and setDescription() methods
  • UpdateDescription() now resolves aspect/field via lookupDescriptionAspect()
  • readEditableProperties() now accepts aspectName parameter

pkg/client/write_test.go

  • Updated 4 existing tests to work with map-based aspect (no behavior change for datasets)
  • Added TestLookupDescriptionAspect — 12 cases covering all 10 supported types + 2 unsupported
  • Added TestUpdateDescription_EntityTypes — 5 entity types verify correct aspect name, entity type, and field name in the wire format
  • Added TestUpdateDescription_UnsupportedEntityType — verifies ErrUnsupportedEntityType sentinel
  • Added TestUpdateDescription_GlossaryTermPreservesExistingFields — verifies definition field is used and sibling fields (termSource, name) are preserved through read-modify-write

Verification

make verify   # All checks passed
  • Lint: 0 issues (43 linters)
  • Tests: all pass with -race -shuffle=on
  • Coverage: above 80% threshold
  • Security: gosec + govulncheck clean
  • Mutation: above 60% threshold
  • Deadcode: only expected library API warnings
  • Build: clean

…#85)

UpdateDescription hardcoded editableDatasetProperties, causing writes
to fail for non-dataset entities. Now resolves the correct aspect name
and field name from a mapping table based on the entity type in the URN.
@codecov
Copy link

codecov bot commented Mar 6, 2026

Codecov Report

❌ Patch coverage is 82.14286% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.30%. Comparing base (575023b) to head (4ec89a0).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #86      +/-   ##
==========================================
- Coverage   89.36%   89.30%   -0.06%     
==========================================
  Files          47       47              
  Lines        3290     3309      +19     
==========================================
+ Hits         2940     2955      +15     
- Misses        226      228       +2     
- Partials      124      126       +2     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

cjimti added 2 commits March 5, 2026 18:22
- Rename editablePropertiesAspect to descriptionAspect (handles non-editable aspects too)
- Use errors.New for ErrUnsupportedEntityType sentinel
- Add UnmarshalJSON for symmetric serialization
- Strengthen test assertions to verify preserved field values, not just existence
- Document intentionally excluded entity types
@cjimti cjimti merged commit 32ba9ec into main Mar 6, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: UpdateDescription hardcodes editableDatasetProperties — fails for non-dataset entities

1 participant