You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ossie.apache.org by jb...@apache.org on 2026/10/10 15:48:50 UTC
(ossie) branch main updated: fix(thoughtspot): report a wrongly-typed container, not a bare traceback (#553)
This is an automated email from the ASF dual-hosted git repository.
jbonofre pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/ossie.git
The following commit(s) were added to refs/heads/main by this push:
new 6a836550 fix(thoughtspot): report a wrongly-typed container, not a bare traceback (#553)
6a836550 is described below
commit 6a8365509b368789a1a037edc91fe7e892fb4125
Author: sunshine <13...@qq.com>
AuthorDate: Sat Oct 10 23:48:44 2026 +0800
fix(thoughtspot): report a wrongly-typed container, not a bare traceback (#553)
`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 666ae2fa..8614d511 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 convert(document_set: DocumentSet) -> OssieConversion:
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 convert(document_set: DocumentSet) -> OssieConversion:
for column in model_columns:
display_name = column.get("name", "<unnamed>")
- 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 7d171d17..7059ce34 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 @@ rather than just the field/metric readers. These tests therefore drive full
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())