fix(thoughtspot): SQL View merge read the wrong column key; output paths could collide

Both findings from @jbonofre's second review, and both are defects in code from
this PR's previous two commits rather than pre-existing ones. 859 tests pass.

SQL VIEW COLUMNS WERE DROPPED BY THE MERGE I ADDED. `_deduplicate_table_documents`
merged only `columns`, but a SQL View keeps its output columns under
`sql_view_columns`. Two aliased datasets over one SQL View therefore lost the
second's fields entirely, `build_model` then raised TS-MODEL-COLUMN-ID-MISSING
for the fields that were dropped, and the body-divergence comparison fired
spuriously because it excluded only `columns` too. So the fix for duplicate
Table documents was worse than the defect it replaced.

Reading the wrong column key is this converter's most repeated mistake -- three
separate silent failures now -- so the kind-to-key rule is named once, in
`_column_key_for`, and both call sites use it. The divergence comparison ignores
both keys, and a kind mismatch is itself a divergence.

A RUN COULD DESTROY ITS OWN OUTPUT. `--issues` and `-o` resolving to one path
passed the does-it-already-exist check on a fresh run; `_write_issues` then
replaced the converted document with the JSON issue log and the command exited
0. The same collision was reachable between `--issues` and a generated `to-tml`
filename. Both commands now reject a repeated target before writing anything,
compared after `resolve()` so two spellings of one file are caught, and
regardless of `--force` -- which licenses overwriting files that were already
there, not destroying an output this run just produced.

Both fixes are mutation-checked: reverting either fails the new tests.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: djwaldo

converters/thoughtspot/src/ossie_thoughtspot/cli.py

@@ -135,6 +135,32 @@ def _safe_target_path(directory: Path, filename: str) -> Path:
     return target
 
 
+def _colliding(paths: list[Path]) -> list[Path]:
+    """Any path this run would write more than once.
+
+    Compared after `resolve()`, because two spellings of one file (a relative
+    path and an absolute one, a symlinked directory) are the same file on disk.
+    Checked BEFORE any write: `--issues` pointing at `-o` passed the
+    does-it-already-exist check on a fresh run, and then `_write_issues`
+    replaced the converted document with the JSON issue log and the command
+    exited 0. `--force` does not license this -- it permits overwriting files
+    that were already there, not destroying one of this run's own outputs.
+    """
+    seen: dict[Path, int] = {}
+    for path in paths:
+        resolved = path.resolve()
+        seen[resolved] = seen.get(resolved, 0) + 1
+    return sorted(path for path, count in seen.items() if count > 1)
+
+
+def _refuse_collision(paths: list[Path]) -> str:
+    names = ", ".join(str(p) for p in paths)
+    return (
+        f"refusing to write the same path twice in one run: {names}. "
+        f"--issues must name a different file from the converted output."
+    )
+
+
 def _existing(paths: list[Path]) -> list[Path]:
     return [p for p in paths if p.exists()]
 
@@ -171,6 +197,10 @@ def _cmd_to_ossie(args: argparse.Namespace) -> int:
         return 1
 
     targets = [output_path] + ([issues_path] if issues_path else [])
+    collisions = _colliding(targets)
+    if collisions:
+        print(f"Error: {_refuse_collision(collisions)}", file=sys.stderr)
+        return 1
     if not args.force:
         existing = _existing(targets)
         if existing:
@@ -212,6 +242,10 @@ def _cmd_to_tml(args: argparse.Namespace) -> int:
         return 1
 
     all_targets = [path for path, _ in targets] + ([issues_path] if issues_path else [])
+    collisions = _colliding(all_targets)
+    if collisions:
+        print(f"Error: {_refuse_collision(collisions)}", file=sys.stderr)
+        return 1
     if not args.force:
         existing = _existing(all_targets)
         if existing:

converters/thoughtspot/src/ossie_thoughtspot/ossie_to_thoughtspot.py

@@ -1179,11 +1179,26 @@ def _field_physical_display_name(field: dict) -> str | None:
     return None
 
 
+#: Every body key that can hold columns, for callers that must ignore all of them.
+_COLUMN_KEYS = frozenset({"columns", "sql_view_columns"})
+
+
+def _column_key_for(kind: str) -> str:
+    """Which body key holds a document's columns.
+
+    A SQL View stores them under `sql_view_columns`, a Table under `columns`.
+    Named once because reading the wrong one is this converter's most repeated
+    mistake: it has now caused three separate silent failures, most recently in
+    `_deduplicate_table_documents`, which merged only `columns` and so dropped
+    every field of a second dataset sharing one SQL View.
+    """
+    return "sql_view_columns" if kind == "sql_view" else "columns"
+
+
 def _physical_columns_of(table_doc: TmlDocument | None) -> list[dict]:
     if table_doc is None:
         return []
-    key = "sql_view_columns" if table_doc.kind == "sql_view" else "columns"
-    return table_doc.body.get(key) or []
+    return table_doc.body.get(_column_key_for(table_doc.kind)) or []
 
 
 def _restore_ai_context(properties: dict, ai_context: object, log: IssueLog, *, object_ref: str) -> None:
@@ -2133,17 +2148,21 @@ def _deduplicate_table_documents(
             order.append(name)
             continue
 
-        seen_columns = {c.get("name") for c in first.body.get("columns") or []}
-        for column in table.body.get("columns") or []:
+        column_key = _column_key_for(table.kind)
+        seen_columns = {c.get("name") for c in first.body.get(column_key) or []}
+        for column in table.body.get(column_key) or []:
             if column.get("name") not in seen_columns:
-                first.body.setdefault("columns", []).append(column)
+                first.body.setdefault(column_key, []).append(column)
                 seen_columns.add(column.get("name"))
 
+        # Both column keys are excluded from the divergence comparison, not just
+        # the one this kind uses: leaving `sql_view_columns` in made every merged
+        # SQL View report a spurious body difference on top of losing the data.
         ignoring_columns = (
-            {k: v for k, v in first.body.items() if k != "columns"},
-            {k: v for k, v in table.body.items() if k != "columns"},
+            {k: v for k, v in first.body.items() if k not in _COLUMN_KEYS},
+            {k: v for k, v in table.body.items() if k not in _COLUMN_KEYS},
         )
-        if ignoring_columns[0] != ignoring_columns[1]:
+        if first.kind != table.kind or ignoring_columns[0] != ignoring_columns[1]:
             log.add(
                 code="TS-TABLE-ALIAS-BODY-DIVERGENT",
                 severity=Severity.WARNING,

converters/thoughtspot/tests/test_cli.py

@@ -339,3 +339,56 @@ def test_ossie_to_thoughtspot_convert_agrees_with_the_cli_on_filenames(tmp_path)
     out_dir = tmp_path / "out"
     cli.main(["to-tml", str(ossie_path), "-o", str(out_dir)])
     assert {p.name for p in out_dir.iterdir()} == expected
+
+
+class TestOutputPathsMustNotCollide:
+    """No run may write the same path twice.
+
+    `--issues` pointing at `-o` passed the does-it-already-exist check on a
+    fresh run, then `_write_issues` replaced the converted document with the
+    JSON issue log and the command exited 0 -- the output silently destroyed by
+    the same invocation that produced it.
+    """
+
+    def test_to_ossie_refuses_issues_equal_to_output(self, tmp_path):
+        target = tmp_path / "out.yaml"
+        code = cli.main([
+            "to-ossie", *[str(p) for p in _tml_paths("minimal")],
+            "-o", str(target), "--issues", str(target),
+        ])
+        assert code == 1
+        assert not target.exists(), "nothing should have been written"
+
+    def test_force_does_not_license_a_self_collision(self, tmp_path):
+        # --force permits overwriting files that were already there; it does not
+        # permit destroying one of this run's own outputs.
+        target = tmp_path / "out.yaml"
+        assert cli.main([
+            "to-ossie", *[str(p) for p in _tml_paths("minimal")],
+            "-o", str(target), "--issues", str(target), "--force",
+        ]) == 1
+
+    def test_a_relative_and_absolute_spelling_of_one_path_still_collides(self, tmp_path, monkeypatch):
+        # Compared after resolve(), because two spellings are one file on disk.
+        monkeypatch.chdir(tmp_path)
+        assert cli.main([
+            "to-ossie", *[str(p) for p in _tml_paths("minimal")],
+            "-o", str(tmp_path / "out.yaml"), "--issues", "out.yaml",
+        ]) == 1
+
+    def test_to_tml_refuses_issues_aimed_at_a_generated_document(self, tmp_path):
+        ossie_file = tmp_path / "in.yaml"
+        _write_ossie_yaml_from_fixture("minimal", ossie_file)
+        out_dir = tmp_path / "tml"
+        assert cli.main([
+            "to-tml", str(ossie_file), "-o", str(out_dir),
+            "--issues", str(out_dir / "orders.table.tml"),
+        ]) == 1
+
+    def test_distinct_paths_are_still_accepted(self, tmp_path):
+        target, issues = tmp_path / "out.yaml", tmp_path / "issues.json"
+        assert cli.main([
+            "to-ossie", *[str(p) for p in _tml_paths("minimal")],
+            "-o", str(target), "--issues", str(issues),
+        ] ) == 0
+        assert target.exists() and issues.exists()

converters/thoughtspot/tests/test_ossie_to_thoughtspot.py

@@ -896,3 +896,59 @@ def test_two_entries_with_no_distinguishing_alias_are_an_error(self):
         codes = [i["code"] for i in result.issues.as_dicts()]
         assert "TS-MODEL-TABLE-ENTRY-AMBIGUOUS" in codes
         assert result.issues.has_errors()
+
+
+class TestMergingTwoDatasetsThatShareOneSqlView:
+    """A SQL View keeps its columns under `sql_view_columns`, not `columns`.
+
+    `_deduplicate_table_documents` merged only `columns`, so two aliased
+    datasets over one SQL View lost the second's fields entirely -- and then
+    `build_model`, checking both aliases against the single merged document,
+    raised TS-MODEL-COLUMN-ID-MISSING for the fields that were dropped. The
+    spurious body-divergence warning fired too, because `sql_view_columns` was
+    not excluded from that comparison either.
+
+    Reading the wrong column key is this converter's most repeated mistake --
+    three separate silent failures now -- which is why the kind-to-key rule is
+    named once in `_column_key_for`.
+    """
+
+    @staticmethod
+    def _sql_view_dataset(alias, column):
+        payload = {
+            "_v": 1, "connection_name": "C", "tml_name": "RETURNS_SV",
+            "alias": alias, "sql_output_columns": {column: column},
+        }
+        return {
+            "name": alias, "source": "SELECT 1",
+            "fields": [{
+                "name": column,
+                "expression": {"dialects": [
+                    {"dialect": "THOUGHTSPOT", "expression": f"[{alias}::{column}]"}
+                ]},
+            }],
+            "custom_extensions": [
+                {"vendor_name": "THOUGHTSPOT", "data": json.dumps(payload)}
+            ],
+        }
+
+    def _convert(self):
+        return convert({"version": "0.2.0.dev0", "name": "M", "datasets": [
+            self._sql_view_dataset("A", "c_a"),
+            self._sql_view_dataset("B", "c_b"),
+        ]})
+
+    def test_both_aliases_columns_survive_the_merge(self):
+        [document] = self._convert().documents.tables
+        assert document.kind == "sql_view"
+        assert [c["name"] for c in document.body["sql_view_columns"]] == ["c_a", "c_b"]
+
+    def test_no_column_is_reported_missing(self):
+        result = self._convert()
+        codes = [i["code"] for i in result.issues.as_dicts()]
+        assert "TS-MODEL-COLUMN-ID-MISSING" not in codes
+        assert not result.issues.has_errors()
+
+    def test_no_spurious_body_divergence_is_reported(self):
+        codes = [i["code"] for i in self._convert().issues.as_dicts()]
+        assert "TS-TABLE-ALIAS-BODY-DIVERGENT" not in codes