Skip to content

fix(mapping): read Manual Mapping save fields from panel state, send excluded/attribute mappings - #150

Open
jeremiaspf wants to merge 1 commit into
databrickslabs:developfrom
jeremiaspf:fix/mapping-manual-save-state
Open

fix(mapping): read Manual Mapping save fields from panel state, send excluded/attribute mappings#150
jeremiaspf wants to merge 1 commit into
databrickslabs:developfrom
jeremiaspf:fix/mapping-manual-save-state

Conversation

@jeremiaspf

Copy link
Copy Markdown

What

Two related bugs in saving from Manual Mapping (the SQL-first alternative to the Designer for creating mappings directly):

  • Entity save: id_column/label_column were read from the panel's visual summary text (which can be stale, or show "Not set" even when the panel's actual state is valid), instead of from the panel's own internal state (EntityPanelState). The save call now also sends attribute_mappings and, when present, excluded_attributes — neither was persisted from this screen before at all.
  • Relationship save: sourceColumn/targetColumn are likewise now read from RelPanelState instead of the visual summary. The save call now also sends source_class, target_class and direction (recovered from the existing mapping when there is one), plus attribute_mappings/excluded_attributes.

Why

Reading from the rendered summary text meant a save could silently capture the wrong (or empty) ID/label/source/target column whenever the summary hadn't caught up with the panel's real state. Not sending attribute_mappings/excluded_attributes meant those were lost entirely when saving from this screen. Not sending source_class/target_class/direction for relationships meant every relationship created via Manual Mapping needed a separate backfill step to reconstruct that information from the ontology after the fact.

How to test

  1. Open Manual Mapping, configure an entity's ID/label columns and some attribute mappings (including excluding one), save — reload and confirm all of it persisted, including the excluded attribute.
  2. Configure a relationship's source/target columns, save — confirm source_class/target_class/direction are present on the saved relationship without needing any separate consistency-check pass.

Note for maintainers

There's a companion fix we're holding back for now: src/back/objects/mapping/Mapping.py in our 0.7.1-based tree also (a) preserves an existing excluded_attributes list on update when the incoming payload omits it, and (b) backfills source_class/target_class from the property's domain/range when a relationship consistency-check finds them empty (the legacy-data counterpart to this PR's frontend fix). We saw Mapping.py has since grown a substantial schema-drift feature on develop, so we want to make sure our change still makes sense against that before proposing it — will follow up with a separate PR once we've checked.

…excluded/attribute mappings

When saving an entity mapping from Manual Mapping, id_column/label_column
were read from the panel's visual summary text (which could be stale or
show "Not set"), instead of from the panel's own internal state
(EntityPanelState). attribute_mappings and, when present,
excluded_attributes are now sent too — previously neither was persisted
from this screen at all.

When saving a relationship mapping, sourceColumn/targetColumn are now
likewise read from RelPanelState instead of the visual summary, and
source_class/target_class/direction are now sent (recovered from the
existing mapping when there is one) along with attribute_mappings/
excluded_attributes. Previously a relationship created via Manual Mapping
carried no source/target entity or direction, which is exactly what the
consistency-check backfill (domain/range resolution) has to reconstruct
after the fact — see the companion generation PR.
@jeremiaspf
jeremiaspf requested a review from a team as a code owner August 22, 2026 09:27
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@benoitcayladbx benoitcayladbx left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Relevant — please keep this. Manual Mapping on current develop still reads ID/label/source/target from the summary DOM (epSummaryId, rpSummarySource, …) and still posts class_name / property_name. Those keys are ignored by Mapping.build_entity_mapping / build_relationship_mapping, which expect ontology_class_label / property_label. So this PR fixes a real save-corruption + schema-drift bug, not a 0.7.1 leftover.

Please address the items below before we merge. Merge after or with #152 (backend preserve/backfill); landing 150 alone still drops excluded_attributes on any save that omits the key.

Must fix

  1. Remove //JERRY comments (.cursor/05 §English-only artifacts, src/.coding_rules.md §3). Author tags and commented-out old lines (//JERRY const idColumn = …) do not belong in develop. Delete the old lines; keep the EntityPanelState / RelPanelState reads.

  2. Always send excluded_attributes, including []. You only attach the key when length > 0. Combined with #152’s “omit ⇒ preserve previous list”, a user who clears every exclusion in Manual Mapping cannot persist that clear — the old list comes back. Preferred contract:

    • frontend always posts excluded_attributes: [...EntityPanelState.excludedAttributes] (possibly empty);
    • backend treats key present (even empty) as authoritative, and only carries forward when the key is absent. See the #152 comment.
  3. First-time relationship save still stores empty source_class / target_class. Copying from existingRel is a no-op when there is no previous mapping. Either:

    • resolve domain/range from the loaded ontology in this save path, or
    • let Mapping.build_relationship_mapping fill them when the payload omits them (better: one place, used by Manual + any other client).
      #152’s diagnostic fallback is display-only; it does not persist the fields.
  4. Changelogchangelogs/v0.8.0/<github-user>_YYYY-MM-DD.log (English, .cursorrules).

  5. CLA still pending.

Nice to have (same PR is fine)

  • Duplication: the filteredAttrMappings block is copied for entity and relationship. Extract a small helper (Fowler Extract Function, src/.coding_rules.md §9).
  • Tests: there are no JS unit tests for this file; at least extend tests/units/mapping/ so a payload with ontology_class_label + excluded_attributes round-trips through add_or_update_entity_mapping (covers the field-name fix even without a browser).

The state-vs-summary change itself is the right fix — just clean the debug comments and make the excluded/source-class contract explicit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants