From 055d69e0ffa7182aabc7c3e27282b48b2e0abfb7 Mon Sep 17 00:00:00 2001 From: Sijibomi Ogunniransi Date: Sat, 3 Oct 2026 12:04:39 +0100 Subject: [PATCH 1/2] fix(thoughtspot): guard wrongly-typed TML values from bare tracebacks Wrongly-typed values in a hand-edited TML document raised bare TypeError/AttributeError during to-ossie conversion, breaking the converter's "never a bare traceback" contract. PR #482 covered the column and model name cases; this covers the remaining scalar reads it enumerated as untouched: - aggregation as a list or dict (membership test on an unhashable value) - a formula expr of the wrong type, on both the field and metric paths - a physical data_type of the wrong type (unhashable in datatypes.to_ossie) - a db_column_name of the wrong type (crashed identifiers.normalise) - a join 'on' of the wrong type (AttributeError on .strip()) Each now logs a TS-* WARNING and degrades (skip, or fall back), matching the surrounding degrade-and-continue pattern, rather than aborting the whole document. The container-type cases (columns, model_tables, properties, join destination) and the to-tml stash direction from #469 are left for a follow-up. Refs #469 --- .../src/ossie_thoughtspot/tml_to_ossie.py | 68 ++++++++- .../tests/test_tml_to_ossie_typed_values.py | 132 ++++++++++++++++++ 2 files changed, 198 insertions(+), 2 deletions(-) create mode 100644 converters/thoughtspot/tests/test_tml_to_ossie_typed_values.py diff --git a/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py b/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py index e3c5edc0..ed1bdffa 100644 --- a/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py +++ b/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py @@ -378,6 +378,21 @@ def _physical_datatype( data_type = (physical.get("db_column_properties") or {}).get("data_type") if data_type is None: return None + if not isinstance(data_type, str): + # A non-string data_type (e.g. a list in a hand-edited document) has no + # Ossie equivalent and would crash datatypes.to_ossie on an unhashable + # value; report it like an unmapped type and emit no datatype. + log.add( + code=f"{code_prefix}-DATATYPE-UNMAPPED", + severity=Severity.WARNING, + message=( + f"physical column {column_name!r} on table {table_name!r} has " + f"non-string data_type {data_type!r}, which has no Ossie " + f"equivalent; no datatype is emitted for this {kind}" + ), + object_ref=object_ref, + ) + return None ossie_type = datatypes.to_ossie(data_type) if ossie_type is None: log.add( @@ -432,7 +447,13 @@ def _physical_db_column_name( ) if physical is None: return None - return physical.get("db_column_name") + db_column_name = physical.get("db_column_name") + # A non-string db_column_name (wrong type in a hand-edited document) is + # unusable as an identifier basis and would crash identifiers.normalise in + # the caller; treat it as absent, the same as a missing db_column_name. + if not isinstance(db_column_name, str): + return None + return db_column_name def _resolve_name_collision( @@ -690,6 +711,18 @@ def convert_field( object_ref=object_ref, ) return None + if not isinstance(formula_entry["expr"], str): + log.add( + code="TS-FIELD-FORMULA-INVALID", + severity=Severity.WARNING, + message=( + f"column {display_name!r} has formula_id {formula_id!r}, whose " + f"formulas[] entry has a non-string expr " + f"{formula_entry['expr']!r}; no field can be built" + ), + object_ref=object_ref, + ) + return None expr = formula_entry["expr"] dataset = attribute_dataset(expr, resolve, log, object_ref=object_ref) if dataset is None: @@ -1015,7 +1048,11 @@ def convert_metric( object_ref = f"metric:{display_name}" aggregation_raw = properties.get("aggregation", "NONE") - if aggregation_raw not in _AGGREGATION: + # A non-string aggregation (a list or dict in a hand-edited document) would + # make the `not in _AGGREGATION` membership test raise on an unhashable + # value; treat it as unrecognised and fall back to NONE like any other + # unknown aggregation rather than letting a bare TypeError escape. + if not isinstance(aggregation_raw, str) or aggregation_raw not in _AGGREGATION: log.add( code="TS-METRIC-AGGREGATION-UNKNOWN", severity=Severity.WARNING, @@ -1079,6 +1116,18 @@ def convert_metric( object_ref=object_ref, ) return None + if not isinstance(formula_entry["expr"], str): + log.add( + code="TS-METRIC-FORMULA-INVALID", + severity=Severity.WARNING, + message=( + f"column {display_name!r} has formula_id {formula_id!r}, whose " + f"formulas[] entry has a non-string expr " + f"{formula_entry['expr']!r}; no metric can be built" + ), + object_ref=object_ref, + ) + return None expr = formula_entry["expr"] metric_name = _field_or_metric_identifier( display_name, None, allocator, log, kind="metric", object_ref=object_ref, @@ -1830,6 +1879,21 @@ def _relationship_from_join( apply to, so `from`/`to` there stay exactly TML's own, unswapped. """ object_ref = f"relationship:{name}" + if on_expression is not None and not isinstance(on_expression, str): + # A non-string `on` (a list or int in a hand-edited document) would raise + # a bare AttributeError on the .strip() below; report it as a malformed + # condition instead, the same degrade-and-skip the parse failure gets. + log.add( + code="TS-JOIN-MALFORMED", + severity=Severity.WARNING, + message=( + f"join {name!r} from {from_prefix!r} to {to_prefix!r} has a " + f"non-string condition {on_expression!r}; it cannot be represented " + f"as a relationship" + ), + object_ref=object_ref, + ) + return None, None, False if not on_expression or not on_expression.strip(): log.add( code="TS-JOIN-NO-CONDITION", diff --git a/converters/thoughtspot/tests/test_tml_to_ossie_typed_values.py b/converters/thoughtspot/tests/test_tml_to_ossie_typed_values.py new file mode 100644 index 00000000..638f1cc5 --- /dev/null +++ b/converters/thoughtspot/tests/test_tml_to_ossie_typed_values.py @@ -0,0 +1,132 @@ +# 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. + +"""Wrongly-*typed* TML values degrade cleanly instead of raising bare tracebacks. + +Issue #469: a hand-edited TML document can carry a value of the wrong Python +type (a list where a string is expected, an int for an expression, and so on). +PR #482 covered the column and model `name` cases; these cover the remaining +TML->Ossie scalar reads it enumerated as untouched — `aggregation`, a formula +`expr`, a physical `data_type`, a `db_column_name`, and a join `on` — each of +which used to escape as a bare TypeError/AttributeError, breaking the +converter's "never a bare traceback" contract. Each now logs a TS-* WARNING and +degrades, matching the surrounding degrade-and-continue pattern. +""" + +from ossie_thoughtspot.issues import IssueLog +from ossie_thoughtspot.tml_to_ossie import ( + _physical_db_column_name, + _relationship_from_join, + convert_field, + convert_metric, +) + + +def _resolve(table, column): + return None if table == "MISSING" else f"{table.lower()}.{column.lower()}" + + +def _lookup(data_type="DOUBLE", db_column_name="AMOUNT"): + """A table_lookup returning one physical column, ORDERS::AMOUNT.""" + column = {"name": "AMOUNT", "db_column_name": db_column_name, + "db_column_properties": {"data_type": data_type}} + table = {"ORDERS": {"name": "ORDERS", "columns": [column]}} + return table.get + + +def _codes(log): + return {issue["code"] for issue in log.as_dicts()} + + +def test_non_string_aggregation_falls_back_to_none_and_is_reported(): + # `aggregation` as a list would make the `not in _AGGREGATION` membership + # test raise on an unhashable value; it is treated as NONE and reported. + log = IssueLog() + metric = convert_metric( + {"name": "Total Amount", "column_id": "ORDERS::AMOUNT", + "properties": {"column_type": "MEASURE", "aggregation": ["SUM"]}}, + {}, _lookup(), _resolve, log, + ) + + assert metric is not None + assert "TS-METRIC-AGGREGATION-UNKNOWN" in _codes(log) + + +def test_non_string_metric_formula_expr_is_reported_and_skipped(): + log = IssueLog() + metric = convert_metric( + {"name": "Bad Metric", "formula_id": "f1", + "properties": {"column_type": "MEASURE", "aggregation": "SUM"}}, + {"f1": {"id": "f1", "expr": 42}}, _lookup(), _resolve, log, + ) + + assert metric is None + assert "TS-METRIC-FORMULA-INVALID" in _codes(log) + + +def test_non_string_field_formula_expr_is_reported_and_skipped(): + log = IssueLog() + field = convert_field( + {"name": "Bad Field", "formula_id": "f1", + "properties": {"column_type": "ATTRIBUTE"}}, + {"f1": {"id": "f1", "expr": 42}}, _lookup(), _resolve, log, + ) + + assert field is None + assert "TS-FIELD-FORMULA-INVALID" in _codes(log) + + +def test_non_string_data_type_emits_no_datatype_and_is_reported(): + # `data_type` as a list has no Ossie equivalent and would crash + # datatypes.to_ossie on an unhashable value; the field is still built. + log = IssueLog() + field = convert_field( + {"name": "Amount", "column_id": "ORDERS::AMOUNT", + "properties": {"column_type": "ATTRIBUTE"}}, + {}, _lookup(data_type=["DOUBLE"]), _resolve, log, + ) + + assert field is not None + assert "datatype" not in field + assert "TS-FIELD-DATATYPE-UNMAPPED" in _codes(log) + + +def test_non_string_db_column_name_is_treated_as_absent(): + # A non-string db_column_name is unusable as an identifier basis and would + # crash identifiers.normalise downstream; it reads as absent (None). + assert _physical_db_column_name("ORDERS", "AMOUNT", _lookup(db_column_name=42)) is None + + +def test_non_string_join_condition_is_reported_as_malformed(): + log = IssueLog() + relationship, unrepresentable, has_residual = _relationship_from_join( + name="orders_to_customers", + from_prefix="ORDERS", + to_prefix="CUSTOMERS", + on_expression=42, + join_type=None, + cardinality=None, + join_shape="inline", + referencing_join=None, + table_lookup=lambda name: None, + log=log, + ) + + assert relationship is None + assert unrepresentable is None + assert has_residual is False + assert "TS-JOIN-MALFORMED" in _codes(log) From 15eda517518a62edd4926f21a7b2ad917e329cb6 Mon Sep 17 00:00:00 2001 From: Sijibomi Ogunniransi Date: Sat, 3 Oct 2026 21:13:39 +0100 Subject: [PATCH 2/2] fix(thoughtspot): move #469 guards to the data boundary Address review on #508. The per-reader guards left the wrongly-typed values reaching the model-scope stash, so to-tml still raised bare tracebacks. Move each guard to where the data is built, so every reader is covered, and test end to end through convert() and to-tml. - Formula expr: sanitize once where the formulas dict is built, dropping a non-string expr (TS-FORMULA-EXPR-INVALID) so convert_field, convert_metric, and both stash paths take their existing "no expr" path. Removes the two per-reader guards. - db_column_name: sanitize in _physical_columns_with_valid_db_names when physical_columns_by_prefix is built (TS-COLUMN-DB-NAME-INVALID), so the resolver, the field stash, and _physical_db_column_name all read it as absent instead of emitting ORDERS.42 or crashing json.dumps. - join on: only a truthy non-string is invalid, so falsy values keep their prior TS-JOIN-NO-CONDITION meaning; a truthy one is dropped under a distinct TS-JOIN-CONDITION-INVALID (not TS-JOIN-MALFORMED, which preserves the join), and the docstring is updated. - Strengthen the aggregation test to assert the NONE fallback. Refs #469 --- .../src/ossie_thoughtspot/tml_to_ossie.py | 115 ++++++---- .../tests/test_tml_to_ossie_typed_values.py | 205 +++++++++++------- 2 files changed, 201 insertions(+), 119 deletions(-) diff --git a/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py b/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py index ed1bdffa..257e742d 100644 --- a/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py +++ b/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py @@ -447,13 +447,7 @@ def _physical_db_column_name( ) if physical is None: return None - db_column_name = physical.get("db_column_name") - # A non-string db_column_name (wrong type in a hand-edited document) is - # unusable as an identifier basis and would crash identifiers.normalise in - # the caller; treat it as absent, the same as a missing db_column_name. - if not isinstance(db_column_name, str): - return None - return db_column_name + return physical.get("db_column_name") def _resolve_name_collision( @@ -711,18 +705,6 @@ def convert_field( object_ref=object_ref, ) return None - if not isinstance(formula_entry["expr"], str): - log.add( - code="TS-FIELD-FORMULA-INVALID", - severity=Severity.WARNING, - message=( - f"column {display_name!r} has formula_id {formula_id!r}, whose " - f"formulas[] entry has a non-string expr " - f"{formula_entry['expr']!r}; no field can be built" - ), - object_ref=object_ref, - ) - return None expr = formula_entry["expr"] dataset = attribute_dataset(expr, resolve, log, object_ref=object_ref) if dataset is None: @@ -1116,18 +1098,6 @@ def convert_metric( object_ref=object_ref, ) return None - if not isinstance(formula_entry["expr"], str): - log.add( - code="TS-METRIC-FORMULA-INVALID", - severity=Severity.WARNING, - message=( - f"column {display_name!r} has formula_id {formula_id!r}, whose " - f"formulas[] entry has a non-string expr " - f"{formula_entry['expr']!r}; no metric can be built" - ), - object_ref=object_ref, - ) - return None expr = formula_entry["expr"] metric_name = _field_or_metric_identifier( display_name, None, allocator, log, kind="metric", object_ref=object_ref, @@ -1365,6 +1335,39 @@ def _normalized_physical_columns(body: dict, kind: str) -> list[dict]: return [_normalize_physical_column(entry, kind) for entry in _raw_physical_columns(body, kind)] +def _physical_columns_with_valid_db_names( + columns: list[dict], prefix: str, log: IssueLog +) -> list[dict]: + """`columns` with any non-string `db_column_name` dropped to `None`. + + Every consumer of a physical column -- the ANSI_SQL resolver, the field + stash, and `_physical_db_column_name` -- reads `db_column_name` from + `physical_columns_by_prefix`. A wrongly-typed value (an int, a list, or a + YAML-parsed date in a hand-edited document) left in place emits a nonsense + warehouse reference like `ORDERS.42`, or raises a bare error in to-tml's + json.dumps. Dropping it to `None` here, once, sends every consumer down its + existing "no warehouse name" path, and the loss is reported rather than + silently emitted. + """ + sanitized = [] + for column in columns: + db_column_name = column.get("db_column_name") + if db_column_name is not None and not isinstance(db_column_name, str): + log.add( + code="TS-COLUMN-DB-NAME-INVALID", + severity=Severity.WARNING, + message=( + f"physical column {column.get('name')!r} on dataset {prefix!r} " + f"has a non-string db_column_name {db_column_name!r}; it is " + f"ignored and no warehouse column name is used for it" + ), + object_ref=f"dataset:{prefix}", + ) + column = {**column, "db_column_name": None} + sanitized.append(column) + return sanitized + + #: The TML `db_column_properties.data_type` spelling `datatypes.to_tml` would #: emit by default for each Ossie datatype whose TML source has more than one #: valid spelling (the datatype map's Boolean and Float rows). Stashing the @@ -1863,7 +1866,8 @@ def _relationship_from_join( """One join -> `(relationship, unrepresentable_entry, has_residual_predicates)`. Exactly one of `relationship`/`unrepresentable_entry` is non-`None` (or - both `None` when there is no condition at all to report). Implements the + both `None` when there is no condition to report, or the condition is not a + representable string and the join is dropped). Implements the *Non-equality joins* table: at least one equality pair emits a `Relationship`, with any residual predicates riding along in its own `custom_extensions` rather than withholding the relationship; zero @@ -1879,17 +1883,22 @@ def _relationship_from_join( apply to, so `from`/`to` there stay exactly TML's own, unswapped. """ object_ref = f"relationship:{name}" - if on_expression is not None and not isinstance(on_expression, str): - # A non-string `on` (a list or int in a hand-edited document) would raise - # a bare AttributeError on the .strip() below; report it as a malformed - # condition instead, the same degrade-and-skip the parse failure gets. + if on_expression and not isinstance(on_expression, str): + # A truthy non-string `on` (a list or int in a hand-edited document) + # would raise a bare AttributeError on the .strip() below. It is not a + # representable condition, so the join is dropped -- not preserved in + # `unrepresentable_joins`, whose stash stores `on` verbatim and would + # then carry the non-string value on into to-tml. A distinct code, not + # TS-JOIN-MALFORMED, because that one preserves the join. A falsy + # non-string (`[]`, `{}`, `0`, `False`) keeps its prior "no condition" + # meaning and falls through to the check below. log.add( - code="TS-JOIN-MALFORMED", + code="TS-JOIN-CONDITION-INVALID", severity=Severity.WARNING, message=( f"join {name!r} from {from_prefix!r} to {to_prefix!r} has a " f"non-string condition {on_expression!r}; it cannot be represented " - f"as a relationship" + f"as a relationship and is dropped" ), object_ref=object_ref, ) @@ -2314,8 +2323,8 @@ def convert(document_set: DocumentSet) -> OssieConversion: dataset_bodies[prefix] = dataset_dict dataset_stashes[prefix] = ds_stash table_docs[prefix] = table_doc.body - physical_columns_by_prefix[prefix] = _normalized_physical_columns( - table_doc.body, table_doc.kind + physical_columns_by_prefix[prefix] = _physical_columns_with_valid_db_names( + _normalized_physical_columns(table_doc.body, table_doc.kind), prefix, log ) fields_by_dataset[prefix] = [] @@ -2357,9 +2366,29 @@ def resolve(table: str, column: str) -> str | None: return f"{table}.{warehouse_reference}" # -- Phase 3: fields and metrics ------------------------------------------ - formulas: dict[str, dict] = { - f["id"]: f for f in (model_body.get("formulas") or []) if f.get("id") - } + # A non-string formula expr (an int, a list, or a YAML-parsed date in a + # hand-edited document) reaches every reader below -- convert_field, + # convert_metric, and both the unattributed and unsurfaced stash paths -- + # and would raise a bare TypeError in to-ossie or in to-tml's json.dumps. + # Report it once here and drop the key, so each reader takes its existing + # "entry has no expr" path instead. + formulas: dict[str, dict] = {} + for f in model_body.get("formulas") or []: + formula_id = f.get("id") + if not formula_id: + continue + if "expr" in f and not isinstance(f["expr"], str): + log.add( + code="TS-FORMULA-EXPR-INVALID", + severity=Severity.WARNING, + message=( + f"formula {formula_id!r} has a non-string expr {f['expr']!r}; " + f"it is ignored and no expression is emitted or preserved for it" + ), + object_ref=f"formula:{formula_id}", + ) + f = {k: v for k, v in f.items() if k != "expr"} + formulas[formula_id] = f metrics: list[dict] = [] # Field identifiers are scoped per dataset in Ossie (Field.name is unique # "within the dataset"); metrics are scoped to the whole model (Metric.name 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 638f1cc5..7d171d17 100644 --- a/converters/thoughtspot/tests/test_tml_to_ossie_typed_values.py +++ b/converters/thoughtspot/tests/test_tml_to_ossie_typed_values.py @@ -20,113 +20,166 @@ Issue #469: a hand-edited TML document can carry a value of the wrong Python type (a list where a string is expected, an int for an expression, and so on). PR #482 covered the column and model `name` cases; these cover the remaining -TML->Ossie scalar reads it enumerated as untouched — `aggregation`, a formula -`expr`, a physical `data_type`, a `db_column_name`, and a join `on` — each of -which used to escape as a bare TypeError/AttributeError, breaking the -converter's "never a bare traceback" contract. Each now logs a TS-* WARNING and -degrades, matching the surrounding degrade-and-continue pattern. +TML->Ossie scalar reads it enumerated as untouched -- `aggregation`, a formula +`expr`, a physical `data_type`, a `db_column_name`, and a join `on`. + +The guards for the values that also flow into the model-scope stash (`expr` and +`db_column_name`) live at the point the data is built, not at one reader, so +the whole `to-ossie` -> `to-tml` round trip stays free of bare tracebacks +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. """ +from ossie_thoughtspot import ossie_to_thoughtspot from ossie_thoughtspot.issues import IssueLog -from ossie_thoughtspot.tml_to_ossie import ( - _physical_db_column_name, - _relationship_from_join, - convert_field, - convert_metric, -) +from ossie_thoughtspot.tml import DocumentSet, TmlDocument +from ossie_thoughtspot.tml_to_ossie import _relationship_from_join, convert -def _resolve(table, column): - return None if table == "MISSING" else f"{table.lower()}.{column.lower()}" +# -- builders (mirroring tests/test_tml_to_ossie.py) -------------------------- +def _table(name, columns): + return TmlDocument( + kind="table", + body={"name": name, "db": "SALES", "schema": "PUBLIC", "db_table": name, + "connection": {"name": "My Snowflake"}, "columns": columns}, + guid=None, + ) -def _lookup(data_type="DOUBLE", db_column_name="AMOUNT"): - """A table_lookup returning one physical column, ORDERS::AMOUNT.""" - column = {"name": "AMOUNT", "db_column_name": db_column_name, - "db_column_properties": {"data_type": data_type}} - table = {"ORDERS": {"name": "ORDERS", "columns": [column]}} - return table.get +def _column(name, db_column_name, data_type="VARCHAR"): + return {"name": name, "db_column_name": db_column_name, + "db_column_properties": {"data_type": data_type}} -def _codes(log): - return {issue["code"] for issue in log.as_dicts()} +def _model(columns, formulas=None): + body = {"name": "Sales Analytics", "model_tables": [{"name": "ORDERS"}], + "columns": columns} + if formulas is not None: + body["formulas"] = formulas + return TmlDocument(kind="model", body=body, guid=None) -def test_non_string_aggregation_falls_back_to_none_and_is_reported(): - # `aggregation` as a list would make the `not in _AGGREGATION` membership - # test raise on an unhashable value; it is treated as NONE and reported. - log = IssueLog() - metric = convert_metric( - {"name": "Total Amount", "column_id": "ORDERS::AMOUNT", + +def _document_set(model_doc, table_doc): + return DocumentSet(model=model_doc, tables=(table_doc,)) + + +def _codes(issues): + return {issue["code"] for issue in issues} + + +def _no_traceback_to_tml(ossie_document): + """to-tml must not raise on the converted document (the stash is the part a + user is least likely to hand-edit correctly).""" + result = ossie_to_thoughtspot.convert(ossie_document) + assert result is not None + return result + + +# -- aggregation -------------------------------------------------------------- + +def test_non_string_aggregation_falls_back_to_none(): + # A list aggregation would crash the `not in _AGGREGATION` membership test on + # an unhashable value; it is reported and the metric emits as NONE (a bare + # column reference, no aggregate wrapper), not dropped. + orders = _table("ORDERS", [_column("Amount", "O_TOTALPRICE", "DOUBLE")]) + model = _model(columns=[ + {"name": "Total", "column_id": "ORDERS::Amount", "properties": {"column_type": "MEASURE", "aggregation": ["SUM"]}}, - {}, _lookup(), _resolve, log, - ) + ]) - assert metric is not None - assert "TS-METRIC-AGGREGATION-UNKNOWN" in _codes(log) + result = convert(_document_set(model, orders)) + assert "TS-METRIC-AGGREGATION-UNKNOWN" in _codes(result.issues.as_dicts()) + metric = result.model["metrics"][0] + thoughtspot = next(d["expression"] for d in metric["expression"]["dialects"] + if d["dialect"] == "THOUGHTSPOT") + assert thoughtspot == "[ORDERS::Amount]" # NONE: no sum(...)/avg(...) wrapper -def test_non_string_metric_formula_expr_is_reported_and_skipped(): - log = IssueLog() - metric = convert_metric( - {"name": "Bad Metric", "formula_id": "f1", - "properties": {"column_type": "MEASURE", "aggregation": "SUM"}}, - {"f1": {"id": "f1", "expr": 42}}, _lookup(), _resolve, log, + +# -- formula expr (reaches convert_field/metric AND the stash) ---------------- + +def test_non_string_formula_expr_is_reported_and_not_stashed(): + orders = _table("ORDERS", [_column("Amount", "O_TOTALPRICE", "DOUBLE")]) + model = _model( + columns=[{"name": "Bad", "formula_id": "f1", + "properties": {"column_type": "MEASURE", "aggregation": "SUM"}}], + formulas=[{"id": "f1", "name": "Bad", "expr": 42}], ) - assert metric is None - assert "TS-METRIC-FORMULA-INVALID" in _codes(log) + result = convert(_document_set(model, orders)) + assert "TS-FORMULA-EXPR-INVALID" in _codes(result.issues.as_dicts()) + assert result.model.get("metrics", []) == [] # the bad formula built no metric + # The invalid expr must not have reached the model-scope stash, or to-tml + # would raise a bare TypeError on it. + _no_traceback_to_tml(result.model) -def test_non_string_field_formula_expr_is_reported_and_skipped(): - log = IssueLog() - field = convert_field( - {"name": "Bad Field", "formula_id": "f1", - "properties": {"column_type": "ATTRIBUTE"}}, - {"f1": {"id": "f1", "expr": 42}}, _lookup(), _resolve, log, + +def test_unsurfaced_non_string_formula_expr_is_reported_and_not_stashed(): + # A formula no column references is normally preserved verbatim in the + # unsurfaced-formulas stash; a non-string expr must be dropped there too. + orders = _table("ORDERS", [_column("Amount", "O_TOTALPRICE", "DOUBLE")]) + model = _model( + columns=[{"name": "Amount", "column_id": "ORDERS::Amount", + "properties": {"column_type": "ATTRIBUTE"}}], + formulas=[{"id": "orphan", "name": "Orphan", "expr": ["not", "a", "string"]}], ) - assert field is None - assert "TS-FIELD-FORMULA-INVALID" in _codes(log) + result = convert(_document_set(model, orders)) + assert "TS-FORMULA-EXPR-INVALID" in _codes(result.issues.as_dicts()) + _no_traceback_to_tml(result.model) -def test_non_string_data_type_emits_no_datatype_and_is_reported(): - # `data_type` as a list has no Ossie equivalent and would crash - # datatypes.to_ossie on an unhashable value; the field is still built. - log = IssueLog() - field = convert_field( - {"name": "Amount", "column_id": "ORDERS::AMOUNT", + +# -- db_column_name (reaches the resolver, the field stash, and the helper) --- + +def test_non_string_db_column_name_emits_no_warehouse_reference(): + orders = _table("ORDERS", [_column("Amount", 42, "DOUBLE")]) + model = _model(columns=[ + {"name": "Amount", "column_id": "ORDERS::Amount", "properties": {"column_type": "ATTRIBUTE"}}, - {}, _lookup(data_type=["DOUBLE"]), _resolve, log, - ) + ]) - assert field is not None - assert "datatype" not in field - assert "TS-FIELD-DATATYPE-UNMAPPED" in _codes(log) + result = convert(_document_set(model, orders)) + assert "TS-COLUMN-DB-NAME-INVALID" in _codes(result.issues.as_dicts()) + field = result.model["datasets"][0]["fields"][0] + # No `ORDERS.42` leaked into an ANSI_SQL sibling; the invalid name is absent. + for dialect in field.get("expression", {}).get("dialects", []): + assert "42" not in dialect["expression"] + _no_traceback_to_tml(result.model) -def test_non_string_db_column_name_is_treated_as_absent(): - # A non-string db_column_name is unusable as an identifier basis and would - # crash identifiers.normalise downstream; it reads as absent (None). - assert _physical_db_column_name("ORDERS", "AMOUNT", _lookup(db_column_name=42)) is None +# -- join `on` ---------------------------------------------------------------- -def test_non_string_join_condition_is_reported_as_malformed(): +def _run_join(on_expression): log = IssueLog() - relationship, unrepresentable, has_residual = _relationship_from_join( - name="orders_to_customers", - from_prefix="ORDERS", - to_prefix="CUSTOMERS", - on_expression=42, - join_type=None, - cardinality=None, - join_shape="inline", - referencing_join=None, - table_lookup=lambda name: None, - log=log, + result = _relationship_from_join( + name="orders_to_customers", from_prefix="ORDERS", to_prefix="CUSTOMERS", + on_expression=on_expression, join_type=None, cardinality=None, + join_shape="inline", referencing_join=None, + table_lookup=lambda name: None, log=log, ) + return result, log.as_dicts() + + +def test_truthy_non_string_join_condition_is_dropped_and_reported(): + (relationship, unrepresentable, has_residual), issues = _run_join(42) assert relationship is None - assert unrepresentable is None + assert unrepresentable is None # dropped, not preserved assert has_residual is False - assert "TS-JOIN-MALFORMED" in _codes(log) + assert "TS-JOIN-CONDITION-INVALID" in _codes(issues) + + +def test_falsy_non_string_join_condition_keeps_its_no_condition_meaning(): + # `on: []`, `on: 0`, etc. meant "no condition" before this change and must + # keep reporting TS-JOIN-NO-CONDITION, not the new invalid-condition code. + for falsy in ([], {}, 0, False): + (relationship, unrepresentable, _), issues = _run_join(falsy) + 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)