Fix DBT converter round-trip losses entity issue

[MonkeyCanCode]

Summary

Currently a primary, unique, and foreign entity whose expression differs from its name is silently lost on MSI → OSI → MSI conversion. This is due to the following:

From converters/dbt/src/ossie_dbt/msi_to_osi.py, we have following:

    @staticmethod
    def _extract_keys(entities: Sequence[Entity]) -> Tuple[Optional[List[str]], List[List[str]]]:
        primary_key: Optional[List[str]] = None
        unique_keys: List[List[str]] = []

        for entity in entities:
            col = entity.expr if entity.expr is not None else entity.name
            if entity.type is EntityType.PRIMARY:
                primary_key = [col]
            elif entity.type is EntityType.UNIQUE:
                unique_keys.append([col])

        return primary_key, unique_keys

However, from converters/dbt/src/ossie_dbt/osi_to_msi.py following:

        if field.name in primary_key_cols:
            entities.append(
                PydanticEntity(
                    name=field.name,
                    type=EntityType.PRIMARY,
                    expr=expr_or_none,
                    description=field.description,
                    label=field.label,
                    role=None,
                    config=None,
                )
            )
            return
        if field.name in unique_key_cols:
            entities.append(
                PydanticEntity(
                    name=field.name,
                    type=EntityType.UNIQUE,
                    expr=expr_or_none,
                    description=field.description,
                    label=field.label,
                    role=None,
                    config=None,
                )
            )
            return
        if field.name in foreign_key_cols:
            entities.append(
                PydanticEntity(
                    name=field.name,
                    type=EntityType.FOREIGN,
                    expr=expr_or_none,
                    description=field.description,
                    label=field.label,
                    role=None,
                    config=None,
                )
            )
            return

Here is the local test result with the fixed code and newly added test:

➜  dbt git:(fix_dbt_roundtrip) ✗ uv run pytest
========================================================================== test session starts ==========================================================================
platform darwin -- Python 3.11.13, pytest-9.1.1, pluggy-1.6.0
rootdir: /Users/yong/Desktop/GitHome/ossie/converters/dbt
configfile: pyproject.toml
testpaths: tests
plugins: syrupy-5.5.3
collected 100 items

tests/test_msi_to_osi.py ..................................................................                                                                       [ 66%]
tests/test_osi_to_msi.py ..................................                                                                                                       [100%]

------------------------------------------------------------------------ snapshot report summary ------------------------------------------------------------------------
5 snapshots passed.
========================================================================== 100 passed in 0.34s ==========================================================================

Related Issues

Checklist

Specification

• Spec changes are included in core-spec/ and follow the existing structure
• Spec changes have been discussed on the mailing list or in a linked issue
• Breaking changes to the spec are clearly called out in the summary

Ontology

• Ontology changes in ontology/ are consistent with spec changes
• New or modified terms are defined and documented

Converters

• Converter logic in converters/ is updated to reflect spec or ontology changes
• New converters include tests under the converter's test directory

Validation

• Validation rules in validation/ are updated if the spec changed
• New validation cases are covered by tests

Documentation

• docs/ is updated to reflect any user-facing changes
• New features or behaviors are documented with examples where appropriate
• CONTRIBUTING.md is updated if the contribution process changed

Examples

• examples/ are added or updated for any new spec constructs or converter support

Tests

• All existing tests pass (pytest / CI green)
• New functionality is covered by tests

Compliance

• ASF license headers are present on all new source files
• No third-party dependencies are added without PMC/IPMC approval

[MonkeyCanCode]

@QMalcolm mind take a look?

[Copilot]

🟡 Changes recommended

The code change also affects UNIQUE and FOREIGN entity classification, but the added regression test only covers PRIMARY, leaving key paths untested.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes a round-trip conversion bug in the DBT converter where PRIMARY/UNIQUE/FOREIGN entities could be silently dropped during MSI → Ossie → MSI when the entity expr differs from the entity name.

Changes:

• Update OssieToMSIConverter field classification to recognize keys by either field.name or the field expression (expr).
• Add a regression test to ensure a PRIMARY entity with expr != name survives MSI → Ossie → MSI.

File summaries

File	Description
converters/dbt/src/ossie_dbt/ossie_to_msi.py	Classifies PRIMARY/UNIQUE/FOREIGN entities by matching either field.name or expr against key sets.
converters/dbt/tests/test_ossie_to_msi.py	Adds a regression test covering the PRIMARY entity expr != name round-trip case.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[jbonofre]

I think that matching by expr membership (in addition to name) opens a false-positive collision risk: if an unrelated field's expr happens to equal another field's key expression, it gets misclassified.

For instance, if we have a dataset orders with primary_key=["id"] and two fields sharing expr="id" (something like customer_id and foo_id) both get classified as PRIMARY entities, and the dimension is lost.

On the other hand, a genuine FK field whose expr collides with another field's PK expr gets classified PRIMARY instead of FOREIGN, losing the relationship's cardinality on round-trip.

Since expr isn't guaranteed unique across fields the way name is, this trades one false-negative bug for a false-positive one 😄