diff --git a/converters/orionbelt/src/ossie_orionbelt/obml_to_osi.py b/converters/orionbelt/src/ossie_orionbelt/obml_to_osi.py index db9ce97..2d0142b 100644 --- a/converters/orionbelt/src/ossie_orionbelt/obml_to_osi.py +++ b/converters/orionbelt/src/ossie_orionbelt/obml_to_osi.py @@ -405,11 +405,24 @@ def _convert_column( ext_data["obml_comment"] = col_obj["comment"] if col_obj.get("owner"): ext_data["obml_owner"] = col_obj["owner"] - # Preserve OBML-only dimension properties (timeGrain, format, resultType, etc.) + # Preserve OBML-only dimension properties (timeGrain, format, resultType, etc.). + # OBML allows N dimensions over one column (grain variants, role-playing + # via ``via``); OSI is field-centric (one dimension per field). The first + # match is the primary dimension carried on the field; any extras are + # preserved as descriptors so the reverse trip can rebuild them instead of + # dropping them silently. matched_dim: dict[str, Any] | None = None + extra_dims: list[dict[str, Any]] = [] for _dim_name, dim_obj in obml_dimensions.items(): - if dim_obj.get("dataObject") == do_name and dim_obj.get("column") == col_name: + if dim_obj.get("dataObject") != do_name or dim_obj.get("column") != col_name: + continue + if matched_dim is None: matched_dim = dim_obj + # Preserve the dimension's OBML name explicitly. The OSI field + # name is the physical code, so without this the round-trip + # would rename the dimension to its column code. Authoritative + # (do not rely on synonyms, which mix names and user aliases). + ext_data["obml_dimension_name"] = _dim_name if dim_obj.get("timeGrain"): ext_data["obml_time_grain"] = dim_obj["timeGrain"] if dim_obj.get("format"): @@ -422,7 +435,38 @@ def _convert_column( ext_data["obml_dimension_owner"] = dim_obj["owner"] if dim_obj.get("via"): ext_data["obml_dimension_via"] = dim_obj["via"] - break + # The dimension's own synonyms and vendor extensions have no + # native OSI slot (OSI has no dimension entity), so preserve them + # authoritatively here for the reverse trip. The customExtensions + # are also emitted as field foreign extensions below for OSI-tool + # visibility; that path lands them on the column on re-import, + # while this one restores them to the dimension. + if dim_obj.get("synonyms"): + ext_data["obml_dimension_synonyms"] = dim_obj["synonyms"] + if dim_obj.get("customExtensions"): + ext_data["obml_dimension_custom_extensions"] = dim_obj["customExtensions"] + else: + descriptor: dict[str, Any] = {"name": _dim_name} + for prop in ("resultType", "timeGrain", "format", "description", "owner", "via"): + if dim_obj.get(prop): + descriptor[prop] = dim_obj[prop] + # Carry the extra dimension's own synonyms and vendor extensions + # too, so it round-trips with the same fidelity as the primary. + if dim_obj.get("synonyms"): + descriptor["synonyms"] = dim_obj["synonyms"] + if dim_obj.get("customExtensions"): + descriptor["customExtensions"] = dim_obj["customExtensions"] + extra_dims.append(descriptor) + if extra_dims: + ext_data["obml_extra_dimensions"] = extra_dims + extra_names = ", ".join(d["name"] for d in extra_dims) + self.warnings.append( + f"Column '{do_name}.{col_name}' backs {len(extra_dims) + 1} OBML " + f"dimensions; OSI represents one dimension per field, so the " + f"{len(extra_dims)} additional dimension(s) ({extra_names}) are " + f"preserved via an OBSL extension for the reverse conversion but " + f"are not natively visible to other OSI tools." + ) field["custom_extensions"] = [ { "vendor_name": _VENDOR_OBML, diff --git a/converters/orionbelt/src/ossie_orionbelt/osi_to_obml.py b/converters/orionbelt/src/ossie_orionbelt/osi_to_obml.py index 6ca635d..b5d03cd 100644 --- a/converters/orionbelt/src/ossie_orionbelt/osi_to_obml.py +++ b/converters/orionbelt/src/ossie_orionbelt/osi_to_obml.py @@ -582,10 +582,20 @@ def _extract_dimensions(self, datasets: list) -> dict: if isinstance(ai_ctx, dict) and ai_ctx.get("synonyms"): dim_def["synonyms"] = list(ai_ctx["synonyms"]) # Restore OBML-only dimension properties from custom_extensions + restored_name: str | None = None + extra_descriptors: list[Any] = [] for ext in field.get("custom_extensions", []): if ext.get("vendor_name") in _OBML_VENDOR_READ: try: ext_data = json.loads(ext.get("data", "{}")) + # Extension data is opaque to ``validate_osi``, so a + # foreign payload may put any JSON here. Only accept a + # non-empty string as the dimension name (it becomes a + # dict key); otherwise ignore it and fall back to the + # field name. + _name = ext_data.get("obml_dimension_name") + if isinstance(_name, str) and _name: + restored_name = _name if ext_data.get("obml_time_grain"): dim_def["timeGrain"] = ext_data["obml_time_grain"] if ext_data.get("obml_dimension_format"): @@ -598,28 +608,98 @@ def _extract_dimensions(self, datasets: list) -> dict: dim_def["owner"] = ext_data["obml_dimension_owner"] if ext_data.get("obml_dimension_via"): dim_def["via"] = ext_data["obml_dimension_via"] + # The dimension's own synonyms / vendor extensions, + # restored authoritatively to the dimension. Opaque + # foreign data, so keep only well-shaped entries. + _syns = ext_data.get("obml_dimension_synonyms") + if isinstance(_syns, list): + clean_syns = [s for s in _syns if isinstance(s, str) and s] + if clean_syns: + dim_def["synonyms"] = clean_syns + _exts = ext_data.get("obml_dimension_custom_extensions") + if isinstance(_exts, list): + clean_exts = [e for e in _exts if isinstance(e, dict)] + if clean_exts: + dim_def["customExtensions"] = clean_exts + # Additional dimensions over the same column, preserved + # by the export because OSI has no slot for them. + _extras = ext_data.get("obml_extra_dimensions") + if isinstance(_extras, list): + extra_descriptors = _extras except (json.JSONDecodeError, TypeError): pass break - # Dimension names must be unique across the model. When the same - # field name occurs in more than one data object (e.g. Orders.date - # and Invoices.date), qualify the later one with its data object - # instead of silently overwriting the earlier dimension. - key = field_name - if key in dimensions and dimensions[key].get("dataObject") != ds_name: - key = f"{ds_name} {field_name}" - suffix = 2 - while key in dimensions: - key = f"{ds_name} {field_name} {suffix}" - suffix += 1 - self.warnings.append( - f"Dimension name '{field_name}' occurs in multiple data " - f"objects; emitted '{ds_name}.{field_name}' as dimension " - f"'{key}' to avoid a collision." - ) - dimensions[key] = dim_def + # Prefer the dimension's restored OBML name (export stashes it on + # the field). The OSI field name is the physical code, so this is + # what keeps an OBML-origin round-trip from renaming dimensions to + # their code. Drop it from synonyms to avoid a self-referential + # alias. + if restored_name and dim_def.get("synonyms"): + dim_def["synonyms"] = [s for s in dim_def["synonyms"] if s != restored_name] + if not dim_def["synonyms"]: + del dim_def["synonyms"] + base_name = restored_name or field_name + self._insert_dimension(dimensions, ds_name, base_name, dim_def) + # Rebuild any additional OBML dimensions the export preserved for + # this column (OSI is one-dimension-per-field). Each descriptor is + # opaque foreign-modifiable data, so guard its shape. + for desc in extra_descriptors: + if not isinstance(desc, dict): + continue + dname = desc.get("name") + if not (isinstance(dname, str) and dname): + continue + extra_def: dict[str, Any] = { + "dataObject": ds_name, + "column": field_name, + "resultType": desc.get("resultType") or abstract_type, + } + for prop in ("timeGrain", "format", "description", "owner", "via"): + value = desc.get(prop) + if isinstance(value, str) and value: + extra_def[prop] = value + # Restore the extra dimension's own synonyms / vendor + # extensions. Opaque foreign data, so keep only well-shaped + # entries (string synonyms; dict extensions). + syns = desc.get("synonyms") + if isinstance(syns, list): + clean_syns = [s for s in syns if isinstance(s, str) and s] + if clean_syns: + extra_def["synonyms"] = clean_syns + exts = desc.get("customExtensions") + if isinstance(exts, list): + clean_exts = [e for e in exts if isinstance(e, dict)] + if clean_exts: + extra_def["customExtensions"] = clean_exts + self._insert_dimension(dimensions, ds_name, dname, extra_def) return dimensions + def _insert_dimension( + self, dimensions: dict[str, Any], ds_name: str, base_name: str, dim_def: dict[str, Any] + ) -> None: + """Insert ``dim_def`` under a unique key. + + Dimension names must be unique across the model. When ``base_name`` + already names a dimension on a *different* data object (foreign OSI where + two datasets share a bare field name and no OBML-origin name was + restored), qualify the later one with its data object and warn instead of + silently overwriting the earlier dimension. A restored OBML name is unique + by construction, so the qualification is foreign-OSI only. + """ + key = base_name + if key in dimensions and dimensions[key].get("dataObject") != ds_name: + key = f"{ds_name} {base_name}" + suffix = 2 + while key in dimensions: + key = f"{ds_name} {base_name} {suffix}" + suffix += 1 + self.warnings.append( + f"Dimension name '{base_name}' occurs in multiple data " + f"objects; emitted '{ds_name}.{base_name}' as dimension " + f"'{key}' to avoid a collision." + ) + dimensions[key] = dim_def + def _convert_metrics(self, osi_metrics: list, ds_map: dict) -> tuple[dict, dict]: """ Convert OSI metrics to OBML measures and metrics. diff --git a/converters/orionbelt/tests/test_osi_converter_properties.py b/converters/orionbelt/tests/test_osi_converter_properties.py index 4d07303..5b9433e 100644 --- a/converters/orionbelt/tests/test_osi_converter_properties.py +++ b/converters/orionbelt/tests/test_osi_converter_properties.py @@ -270,21 +270,28 @@ def test_data_object_comment_roundtrip(self): class TestDimensionProperties: """Dimension properties roundtrip. - Note: OSI uses column *code* as the field name, so after roundtrip the - dimension key becomes the physical code (ORDER_DATE). + The OSI field name is the physical column code, but the export stashes the + dimension's OBML name in an ``obml_dimension_name`` extension, so the + round-trip restores the original key (``Order Date``), not the code (#220). """ + def test_dimension_name_roundtrip(self): + result = _roundtrip(_OBML_FULL) + # The OBML dimension name is restored, not renamed to the column code. + assert "Order Date" in result["dimensions"] + assert "ORDER_DATE" not in result["dimensions"] + def test_dimension_result_type_roundtrip(self): result = _roundtrip(_OBML_FULL) - assert result["dimensions"]["ORDER_DATE"].get("resultType") == "date" + assert result["dimensions"]["Order Date"].get("resultType") == "date" def test_dimension_description_roundtrip(self): result = _roundtrip(_OBML_FULL) - assert result["dimensions"]["ORDER_DATE"].get("description") == "Order date dimension" + assert result["dimensions"]["Order Date"].get("description") == "Order date dimension" def test_dimension_owner_roundtrip(self): result = _roundtrip(_OBML_FULL) - assert result["dimensions"]["ORDER_DATE"].get("owner") == "analytics" + assert result["dimensions"]["Order Date"].get("owner") == "analytics" class TestCumulativeMetricDataType: diff --git a/converters/orionbelt/tests/test_osi_converter_roundtrip_robustness.py b/converters/orionbelt/tests/test_osi_converter_roundtrip_robustness.py index 02c79f2..1e80efa 100644 --- a/converters/orionbelt/tests/test_osi_converter_roundtrip_robustness.py +++ b/converters/orionbelt/tests/test_osi_converter_roundtrip_robustness.py @@ -1,3 +1,20 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + """Regression tests for issue #201: OSI<->OBML converter round-trip fidelity and validation robustness. @@ -17,6 +34,8 @@ import json from typing import Any +import pytest + import ossie_orionbelt.converter as conv # --------------------------------------------------------------------------- @@ -253,6 +272,308 @@ def test_both_dimensions_survive(self) -> None: assert any("multiple data objects" in w for w in c.warnings) +# --------------------------------------------------------------------------- +# #220: OBML dimension name restored across the round-trip +# --------------------------------------------------------------------------- + + +class TestDimensionNameRoundTrip: + """An OBML-origin round-trip restores each dimension's name (the OSI field + name is the physical code, so without the ``obml_dimension_name`` extension + the dimension would be renamed to its code) and never trips the collision + fallback, since the restored names are unique by construction.""" + + _OBML = { + "version": 1.0, + "dataObjects": { + "Orders": { + "code": "FACT_ORDERS", + "database": "WH", + "schema": "PUBLIC", + "columns": { + "Order Date": {"code": "order_dt", "abstractType": "date"}, + "Region": {"code": "rgn", "abstractType": "string"}, + "Amount": {"code": "amt", "abstractType": "float"}, + }, + }, + "Invoices": { + "code": "FACT_INV", + "database": "WH", + "schema": "PUBLIC", + "columns": {"Invoice Date": {"code": "inv_dt", "abstractType": "date"}}, + }, + }, + # Names deliberately differ from their column codes; both date dims would + # collide on the code path if names were not restored. + "dimensions": { + "Order Placed On": { + "dataObject": "Orders", + "column": "Order Date", + "resultType": "date", + }, + "Sales Region": {"dataObject": "Orders", "column": "Region", "resultType": "string"}, + "Invoice Raised On": { + "dataObject": "Invoices", + "column": "Invoice Date", + "resultType": "date", + }, + }, + "measures": { + "Revenue": { + "columns": [{"dataObject": "Orders", "column": "Amount"}], + "aggregation": "sum", + "resultType": "float", + } + }, + } + + def _roundtrip(self) -> tuple[dict[str, Any], list[str]]: + osi = conv.OBMLtoOSI(self._OBML, model_name="sales").convert() + c = conv.OSItoOBML(osi) + return c.convert(), c.warnings + + def test_names_restored_not_renamed_to_code(self) -> None: + obml, _ = self._roundtrip() + assert set(obml["dimensions"]) == { + "Order Placed On", + "Sales Region", + "Invoice Raised On", + } + + def test_no_collision_fallback_for_obml_origin(self) -> None: + _, warnings = self._roundtrip() + assert not any("collision" in w.lower() for w in warnings), warnings + + def test_restored_name_not_left_as_a_synonym(self) -> None: + obml, _ = self._roundtrip() + # The dimension must not carry its own restored name as a synonym. + for name, dim in obml["dimensions"].items(): + assert name not in dim.get("synonyms", []) + + def test_primary_dimension_synonyms_and_extensions_preserved(self) -> None: + # The primary (single) dimension on a column keeps its own synonyms and + # vendor extensions across the round-trip, not just the scalar props. + obml = { + "version": 1.0, + "dataObjects": { + "Orders": { + "code": "o", + "database": "W", + "schema": "P", + "columns": {"Status": {"code": "status", "abstractType": "string"}}, + } + }, + "dimensions": { + "Status": { + "dataObject": "Orders", + "column": "Status", + "synonyms": ["state", "condition"], + "customExtensions": [{"vendor": "TABLEAU", "data": '{"role": "dim"}'}], + } + }, + } + back = conv.OSItoOBML(conv.OBMLtoOSI(obml).convert()).convert() + dim = back["dimensions"]["Status"] + assert dim.get("synonyms") == ["state", "condition"] + assert dim.get("customExtensions") == [{"vendor": "TABLEAU", "data": '{"role": "dim"}'}] + + @pytest.mark.parametrize("bad", [["x"], 5, "", {}, None]) + def test_non_string_restored_name_is_ignored(self, bad: Any) -> None: + # obml_dimension_name is opaque to validate_osi, so a foreign payload may + # put any JSON there. A non-string (would be an unhashable dict key) must + # be ignored and fall back to the field name, never crash the converter. + osi = { + "version": "0.2.0.dev0", + "semantic_model": [ + { + "name": "s", + "datasets": [ + { + "name": "Orders", + "source": "WH.PUBLIC.orders", + "fields": [ + { + "name": "dt", + "expression": { + "dialects": [{"dialect": "ANSI_SQL", "expression": "dt"}] + }, + "dimension": {}, + "custom_extensions": [ + { + "vendor_name": "ORIONBELT", + "data": json.dumps({"obml_dimension_name": bad}), + } + ], + } + ], + } + ], + } + ], + } + obml = conv.OSItoOBML(osi).convert() + assert list(obml["dimensions"]) == ["dt"] + + +# --------------------------------------------------------------------------- +# #222: N OBML dimensions over one column +# --------------------------------------------------------------------------- + + +class TestMultipleDimensionsPerColumn: + """OBML allows N dimensions over one column (grain variants, role-playing); + OSI is one-dimension-per-field. The export preserves the extras via an + extension and warns (not silent); the import rebuilds them all (#222).""" + + _OBML = { + "version": 1.0, + "dataObjects": { + "Orders": { + "code": "FACT", + "database": "WH", + "schema": "PUBLIC", + "columns": { + "date": {"code": "order_date", "abstractType": "date"}, + "Amount": {"code": "amt", "abstractType": "float"}, + }, + } + }, + "dimensions": { + "Order Day": { + "dataObject": "Orders", + "column": "date", + "resultType": "date", + "timeGrain": "day", + }, + "Order Month": { + "dataObject": "Orders", + "column": "date", + "resultType": "date", + "timeGrain": "month", + }, + "Order Quarter": { + "dataObject": "Orders", + "column": "date", + "resultType": "date", + "timeGrain": "quarter", + }, + }, + "measures": { + "Revenue": { + "columns": [{"dataObject": "Orders", "column": "Amount"}], + "aggregation": "sum", + "resultType": "float", + } + }, + } + + def _roundtrip(self) -> tuple[dict[str, Any], list[str]]: + exp = conv.OBMLtoOSI(self._OBML, model_name="s") + osi = exp.convert() + return conv.OSItoOBML(osi).convert(), exp.warnings + + def test_all_dimensions_survive_with_distinct_grains(self) -> None: + obml, _ = self._roundtrip() + dims = obml["dimensions"] + assert set(dims) == {"Order Day", "Order Month", "Order Quarter"} + assert {d["timeGrain"] for d in dims.values()} == {"day", "month", "quarter"} + # All three still point at the same physical column. + assert {d["column"] for d in dims.values()} == {"order_date"} + + def test_export_warns_not_silent(self) -> None: + _, warnings = self._roundtrip() + assert any("backs 3 OBML dimensions" in w for w in warnings), warnings + + def test_extra_dimension_synonyms_and_extensions_preserved(self) -> None: + # An extra dimension's own synonyms and vendor extensions must survive, + # matching the primary's fidelity (not just the scalar props). + obml = { + "version": 1.0, + "dataObjects": { + "Orders": { + "code": "F", + "database": "W", + "schema": "P", + "columns": {"date": {"code": "dt", "abstractType": "date"}}, + } + }, + "dimensions": { + "Order Day": { + "dataObject": "Orders", + "column": "date", + "resultType": "date", + "timeGrain": "day", + }, + "Order Month": { + "dataObject": "Orders", + "column": "date", + "resultType": "date", + "timeGrain": "month", + "synonyms": ["monthly date"], + "customExtensions": [{"vendor": "ACME", "data": '{"y": 2}'}], + }, + }, + } + back = conv.OSItoOBML(conv.OBMLtoOSI(obml, model_name="s").convert()).convert() + month = back["dimensions"]["Order Month"] + assert month.get("synonyms") == ["monthly date"] + assert month.get("customExtensions") == [{"vendor": "ACME", "data": '{"y": 2}'}] + + @pytest.mark.parametrize( + "bad", + [ + "not-a-list", + [5], + [{"no_name": 1}], + [{"name": ["x"]}], + [{"name": ""}], + [{"name": "OK"}, 7], + [{"name": "OK", "synonyms": "not-a-list"}], + [{"name": "OK", "synonyms": [1, {}]}], + [{"name": "OK", "customExtensions": "bad"}], + [{"name": "OK", "customExtensions": [7]}], + ], + ) + def test_malformed_extra_dimensions_never_crash(self, bad: Any) -> None: + # obml_extra_dimensions is opaque to validate_osi. A malformed payload + # (not a list, or items that are not dicts / lack a string name) must be + # skipped, never crash the converter on an unhashable key. + osi = { + "version": "0.2.0.dev0", + "semantic_model": [ + { + "name": "s", + "datasets": [ + { + "name": "Orders", + "source": "WH.PUBLIC.orders", + "fields": [ + { + "name": "dt", + "expression": { + "dialects": [{"dialect": "ANSI_SQL", "expression": "dt"}] + }, + "dimension": {}, + "custom_extensions": [ + { + "vendor_name": "ORIONBELT", + "data": json.dumps({"obml_extra_dimensions": bad}), + } + ], + } + ], + } + ], + } + ], + } + obml = conv.OSItoOBML(osi).convert() + # The primary field-name dimension is always present; any well-formed + # extra (the {"name": "OK"} case) is rebuilt, malformed items skipped. + assert "dt" in obml["dimensions"] + assert all(isinstance(k, str) for k in obml["dimensions"]) + + # --------------------------------------------------------------------------- # P2: validate_osi robustness on malformed input # ---------------------------------------------------------------------------