From 8c2148d1bce94a13bb06016d1b6689ddbcc7a8b9 Mon Sep 17 00:00:00 2001 From: "13574798@qq.com" <13574798@qq.com> Date: Sat, 10 Oct 2026 20:03:17 +0800 Subject: [PATCH] fix(thoughtspot): report a wrongly-typed container, not a bare traceback `columns: oops` in a hand-edited Model document reached `for column in model_body.get("columns") or []`, which iterated the string's characters, and the first `.get()` on one raised AttributeError: 'str' object has no attribute 'get' a traceback naming neither the document nor the key. The converter contracts otherwise: `tml.load_document` already raises `ConversionError` for a root that is not a mapping, and `stash.py` calls that its own never-a-bare-traceback rule. #469 lists these container reads as the rows PR #482 left untouched -- `columns`, `model_tables`, `properties` and a join `destination`. Two helpers guard them at the boundary where the value is read: `_tml_list` (a collection, checking that each entry is a mapping too) and `_tml_mapping` (a sub-object). A Model with no column list yields no field, no metric and no dataset, so the four containers raise `ConversionError` naming the key and the type found, on the same line `load_document` draws. A join whose `destination` cannot be read costs one relationship and nothing else, so it skips and reports TS-JOIN-DESTINATION-INVALID, following the path a `referencing_join` that names no `joins_with` entry already takes rather than failing the whole model. Refs #469 -- that issue's to-tml rows (a schema-invalid Ossie document, and wrongly-typed values inside a stashed custom_extensions) still raise bare tracebacks and are untouched here. --- .../src/ossie_thoughtspot/tml_to_ossie.py | 73 +++++++++-- .../tests/test_tml_to_ossie_typed_values.py | 116 +++++++++++++++++- 2 files changed, 181 insertions(+), 8 deletions(-) diff --git a/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py b/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py index 666ae2fab..8614d5116 100644 --- a/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py +++ b/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py @@ -749,7 +749,7 @@ def convert_field( exactly as correct for a call that only ever converts one column at a time. """ - properties = column.get("properties") or {} + properties = _tml_mapping(column.get("properties"), "column `properties`") if properties.get("column_type") != "ATTRIBUTE": return None if allocator is None: @@ -1115,7 +1115,7 @@ def convert_metric( than it does across fields; a caller that omits it gets a fresh, private one, correct for a call that only ever converts one column. """ - properties = column.get("properties") or {} + properties = _tml_mapping(column.get("properties"), "column `properties`") if properties.get("column_type") != "MEASURE": return None if allocator is None: @@ -1436,7 +1436,7 @@ def _index_attribute_columns( """ index: set[tuple[str, str]] = set() for column in columns: - properties = column.get("properties") or {} + properties = _tml_mapping(column.get("properties"), "column `properties`") if properties.get("column_type") != "ATTRIBUTE": continue column_id = column.get("column_id") @@ -1460,6 +1460,38 @@ def _index_attribute_columns( return index +def _tml_list(value, what: str) -> list: + """A TML collection read from a hand-edited document. + + Issue #469: the converter contracts never to surface a bare traceback. A + value of the wrong Python type does -- iterating a string `columns:` yields + characters, and the first `.get()` on one raises `AttributeError` naming + neither the document nor the key. A wrongly-typed *container* leaves nothing + to salvage: with no column list there is no field, no metric and no dataset, + so it fails like the other structural problems in `tml.load_document`, as a + `ConversionError` that says which key held what type. + """ + if value is None: + return [] + if not isinstance(value, list): + raise ConversionError(f"{what} must be a list, not {type(value).__name__}") + for entry in value: + if not isinstance(entry, dict): + raise ConversionError( + f"{what} entries must be mappings, not {type(entry).__name__}" + ) + return value + + +def _tml_mapping(value, what: str) -> dict: + """A TML sub-object, or a `ConversionError` naming the key and the type.""" + if value is None: + return {} + if not isinstance(value, dict): + raise ConversionError(f"{what} must be a mapping, not {type(value).__name__}") + return value + + def _raw_physical_columns(body: dict, kind: str) -> list[dict]: """The verbatim physical-column list for a Table or SQL View document -- `columns[]` for a `table:`, `sql_view_columns[]` for a `sql_view:`. @@ -1474,7 +1506,7 @@ def _raw_physical_columns(body: dict, kind: str) -> list[dict]: `_normalized_physical_columns` below, never `body.get("columns")` directly. """ key = "sql_view_columns" if kind == "sql_view" else "columns" - return body.get(key) or [] + return _tml_list(body.get(key), f"{kind} {key}") def _normalize_physical_column(entry: dict, kind: str) -> dict: @@ -2278,7 +2310,23 @@ def _convert_join( object_ref=f"relationship:{referencing_join}", ) return None, None, None - to_prefix = (matched.get("destination") or {}).get("name") + destination = matched.get("destination") + if destination is not None and not isinstance(destination, dict): + # A `destination` of the wrong type would raise AttributeError on the + # `.get("name")` below (issue #469). One unlocatable target dataset + # costs one relationship, so this skips like a missing + # `referencing_join` does rather than failing the whole model. + log.add( + code="TS-JOIN-DESTINATION-INVALID", + severity=Severity.WARNING, + message=( + f"joins_with entry {referencing_join!r} has destination " + f"{destination!r}, which is not a mapping; the join is skipped" + ), + object_ref=f"relationship:{referencing_join}", + ) + return None, None, None + to_prefix = (destination or {}).get("name") on_expression = matched.get("on") join_type = join.get("type", matched.get("type")) cardinality = join.get("cardinality", matched.get("cardinality")) @@ -2384,6 +2432,17 @@ def convert(document_set: DocumentSet) -> OssieConversion: log = IssueLog() model_body = document_set.model.body + # Fail on a wrongly-typed container before anything reads through it: these + # three keys are what every dataset, field, metric and the model stash are + # built from, so a string or int here would otherwise reach a `.get()` or a + # `for` loop and raise AttributeError/TypeError from inside the conversion + # (issue #469). + _tml_mapping(model_body.get("properties"), "model `properties`") + _tml_list(model_body.get("model_tables"), "model `model_tables`") + model_columns = _tml_list(model_body.get("columns"), "model `columns`") + for column in model_columns: + _tml_mapping(column.get("properties"), "model column `properties`") + model_name_raw = model_body.get("name") if "name" in model_body and not isinstance(model_name_raw, str): # Same malformed-type hazard as a column `name` (see @@ -2507,7 +2566,7 @@ def table_lookup(name: str) -> dict | None: return {"columns": columns} # -- Phase 2: the cross-model resolver ----------------------------------- - model_columns = model_body.get("columns") or [] + # `model_columns` is the validated list from the top of convert(). attribute_index = _index_attribute_columns(model_columns, log) def resolve(table: str, column: str) -> tuple[str, str] | None: @@ -2594,7 +2653,7 @@ def resolve(table: str, column: str) -> tuple[str, str] | None: for column in model_columns: display_name = column.get("name", "") - properties = column.get("properties") or {} + properties = _tml_mapping(column.get("properties"), "column `properties`") try: field = convert_field( column, formulas, table_lookup, resolve, log, field_name_allocator diff --git a/converters/thoughtspot/tests/test_tml_to_ossie_typed_values.py b/converters/thoughtspot/tests/test_tml_to_ossie_typed_values.py index 7d171d171..7059ce34e 100644 --- a/converters/thoughtspot/tests/test_tml_to_ossie_typed_values.py +++ b/converters/thoughtspot/tests/test_tml_to_ossie_typed_values.py @@ -30,12 +30,25 @@ documents through `convert()` (and `ossie_to_thoughtspot.convert()` for the stashed values) and assert on the issue log and the emitted document, not only on the single-helper return value. + +The last rows of #469 -- a value that is not a container at all where a +container is read (`columns`, `model_tables`, `properties`, a join +`destination`) -- are covered by the "wrongly-typed containers" section at the +end. Those fail at the read boundary, before any per-column work can be salvaged +from the document. """ +import pytest + from ossie_thoughtspot import ossie_to_thoughtspot +from ossie_thoughtspot.errors import ConversionError from ossie_thoughtspot.issues import IssueLog from ossie_thoughtspot.tml import DocumentSet, TmlDocument -from ossie_thoughtspot.tml_to_ossie import _relationship_from_join, convert +from ossie_thoughtspot.tml_to_ossie import ( + _convert_join, + _relationship_from_join, + convert, +) # -- builders (mirroring tests/test_tml_to_ossie.py) -------------------------- @@ -183,3 +196,104 @@ def test_falsy_non_string_join_condition_keeps_its_no_condition_meaning(): assert relationship is None and unrepresentable is None assert "TS-JOIN-NO-CONDITION" in _codes(issues) assert "TS-JOIN-CONDITION-INVALID" not in _codes(issues) + + +# -- wrongly-typed containers ------------------------------------------------- + +def _expect_conversion_error(document_set, expected: str): + with pytest.raises(ConversionError) as caught: + convert(document_set) + assert str(caught.value) == expected + + +@pytest.mark.parametrize("columns", ["oops", {"order_id": "ORDERS::order_id"}, 42]) +def test_model_columns_of_the_wrong_container_type_names_the_key_and_type( + columns, +): + # Before, `for column in model_body.get("columns") or []` iterated a string's + # characters and the first `.get()` raised `AttributeError: 'str' object has + # no attribute 'get'` -- a traceback naming neither the document nor the key. + # A model with no column list can produce no field and no metric, so this is + # the hard-failure boundary `tml.load_document` already draws. + orders = _table("ORDERS", [_column("Amount", "O_TOTALPRICE", "DOUBLE")]) + model = _model(columns=columns) + + _expect_conversion_error( + _document_set(model, orders), "model `columns` must be a list, not " + + type(columns).__name__ + ) + + +def test_model_columns_entry_of_the_wrong_container_type(): + orders = _table("ORDERS", [_column("Amount", "O_TOTALPRICE", "DOUBLE")]) + model = _model(columns=["order_id"]) + + _expect_conversion_error( + _document_set(model, orders), + "model `columns` entries must be mappings, not str", + ) + + +@pytest.mark.parametrize("model_tables", ["ORDERS", 42]) +def test_model_tables_of_the_wrong_container_type(model_tables): + orders = _table("ORDERS", [_column("Amount", "O_TOTALPRICE", "DOUBLE")]) + model = TmlDocument( + kind="model", + body={"name": "Sales Analytics", "model_tables": model_tables, + "columns": []}, + guid=None, + ) + + _expect_conversion_error( + _document_set(model, orders), + f"model `model_tables` must be a list, not {type(model_tables).__name__}", + ) + + +def test_column_properties_of_the_wrong_container_type(): + orders = _table("ORDERS", [_column("Amount", "O_TOTALPRICE", "DOUBLE")]) + model = _model(columns=[ + {"name": "Amount", "column_id": "ORDERS::Amount", "properties": "ATTRIBUTE"}, + ]) + + _expect_conversion_error( + _document_set(model, orders), + "model column `properties` must be a mapping, not str", + ) + + +def test_physical_columns_of_the_wrong_container_type(): + # The Table document's own list, reached through the table_lookup the + # resolver and the datatype reader share. + orders = TmlDocument( + kind="table", + body={"name": "ORDERS", "db": "SALES", "schema": "PUBLIC", + "db_table": "ORDERS", "connection": {"name": "My Snowflake"}, + "columns": "O_TOTALPRICE"}, + guid=None, + ) + model = _model(columns=[ + {"name": "Amount", "column_id": "ORDERS::Amount", + "properties": {"column_type": "ATTRIBUTE"}}, + ]) + + _expect_conversion_error(_document_set(model, orders), + "table columns must be a list, not str") + + +def test_join_destination_of_the_wrong_container_type_is_skipped_and_reported(): + # One unlocatable target dataset costs one relationship, not the model, so + # this follows the existing `referencing_join`-not-found path rather than + # raising: `(destination or {}).get("name")` used to raise AttributeError. + log = IssueLog() + relationship, unrepresentable, candidate = _convert_join( + "ORDERS", + {"referencing_join": "orders_to_customers", "with": "CUSTOMERS"}, + {"joins_with": [{"name": "orders_to_customers", "destination": "CUSTOMERS"}]}, + frozenset({"ORDERS", "CUSTOMERS"}), + lambda name: None, + log, + ) + + assert (relationship, unrepresentable, candidate) == (None, None, None) + assert "TS-JOIN-DESTINATION-INVALID" in _codes(log.as_dicts())