fix(thoughtspot): check the Ossie document's shape before reading it

[ihadoop]

The bug

convert() promises ConversionError for input it cannot handle -- but a value of the wrong type where a container is read reached a .get(), a for or a len() first:

$ convert({'datasets': {'orders': {...}}, ...})   # datasets is a mapping, not a list
AttributeError: 'str' object has no attribute 'get'      # in ossie_to_thoughtspot.py:747

A traceback naming neither the document nor the key, for input the converter already refuses in every other shape. Six crash sites, all the same class:

input	site	raised
datasets: {...}	ossie_to_thoughtspot.py:747	AttributeError
metrics: {...}	stash.py:85	AttributeError
source: 7	ossie_to_thoughtspot.py:175	TypeError: object of type 'int' has no len()
expression: 'orders.amount'	ossie_to_thoughtspot.py:227	AttributeError
dialects: {dialect: ..., expression: ...}	ossie_to_thoughtspot.py:237	AttributeError
a dialect expression: 42	formula.py:308	AttributeError: 'int' object has no attribute 'strip'

This is the to-tml row of #469 that PR #553 does not touch (#553 is the same bug class on the TML side, to-ossie).

The check

_require_document_shape walks the nodes the readers assume -- datasets and their fields, a field's or metric's expression and its dialects[], relationships, and custom_extensions[] -- and names the path and the type found:

the Ossie document is not convertible:
  datasets[0].fields[0].expression.dialects[0].expression must be a string, not int

custom_extensions[].data is included because it is parsed as JSON, so a mapping there reached json.loads as the wrong type rather than being reported.

Two deliberate limits

Narrower than the schema. It checks the shapes this converter actually reads, not everything core-spec/ossie-schema.json forbids -- otherwise a property this converter ignores could start failing a document that converts correctly today. A control test asserts that: a dataset with no fields, an ai_context in its string form, a field with only an ANSI_SQL dialect and no THOUGHTSPOT entry, and dimension + object-form ai_context all still convert.

Dependency-free. Written by hand rather than validating with jsonschema against the published schema, because that is what keeps this converter's runtime dependency at PyYAML, as it is today.

The diff is a pure addition (+195/-0): no existing line changes.

Refs, not Closes

#469 has one more to-tml row that this does not fix: a wrongly-typed value inside a stashed custom_extensions. Measured on main after #553, scalar values are already handled (the witness checks isinstance(stashed, str)), but a container is not:

model stash unsurfaced_formulas = 'x'  ->  AttributeError at ossie_to_thoughtspot.py:2147

That is a third bug class in its own right -- the stash payload, validated by test_stash_key_classification.py and documented in docs/ -- so it belongs in its own change rather than being folded in here. Order #553 -> this -> the stash row closes #469 completely.

Test plan

• 14 new cases in TestDocumentShapeIsCheckedBeforeItIsRead: 13 parametrised paths (each asserts the exact message, not just an exception) + the control case above
• converters/thoughtspot: 1017 passed (was 1003), including both round-trip suites and the hypothesis property tests
• the six crash sites above now return ConversionError with a path, verified directly against the tpcds fixture
• dev/check-license (Apache RAT): passed
• CI: uv sync && uv run pytest in converters/thoughtspot on 3.11-3.14 (local run was 3.14)

Independent of #552/#553/#554 -- branched from main, and ossie_to_thoughtspot.py is a file none of the other two thoughtspot PRs edit (#553 edits tml_to_ossie.py, #554 edits the witness in ossie_to_thoughtspot.py around line 861, which is far from these additions at the end of the file; if #554 merges first a rebase may be trivial).

[jbonofre]

Relation to my inline comment, the new tests don't cover a null list element. Please add a parametrized case for each container (datasets, fields, metrics, relationships, dialects, custom_extensions). Each should assert that convert() raises ConversionError and not AttributeError.

[jbonofre]

_require_shape returns None when it's given None, and the callers in _require_document_shape then call .get() on the result without a guard.
For datasets: [null], dataset is None, the shape check passes, and dataset.get("name") raises a bare AttributeError. fields, metrics, relationships, dialects and custom_extensions have the same problem with a null element.

convert() promises ConversionError, and #469 is meant to remove this failure mode, so I think this needs fixing before merge.

I suggest only an absent container key should be allowed to be None. A None element inside a list should be rejected with a ConversionError that names the path, for example datasets[0] must be a mapping, got null.

[ihadoop]

Correction to my description above

I wrote, under Refs, not Closes: "Order #553 -> this -> the stash row closes #469 completely." That was wrong, and I now have the data to say so.

While writing #556 (the stash-payload row) I fuzzed the shipped tpcds fixture on a tree containing #553 + #555 + #556 together -- 400 mutations per direction, replacing one leaf at a random path with each of str / int / None / bool / list / dict / float / bytes / empty list / empty dict -- and bare tracebacks are still plentiful beyond the three PRs:

• to-ossie: 25 distinct sites. Largest group: a physical column entry that is not a mapping, hitting (physical.get("db_column_properties") or {}).get("data_type") at tml_to_ossie.py:475. Also a tuple as a set element (:2862), a membership test on an int (:1846), a non-mapping document body (tml.py:121), and bytes reaching json.dumps (encoder.py:182).
• to-tml: 13 sites. Chiefly a non-string at ossie_to_thoughtspot.py:818's .strip()/.casefold(), an unhashable value as a datatypes.py:108 dict key, and a None formula expr at formula.py:308.

So #469's tables were examples of the class, not its inventory, and none of #553/#555/#556 should close it -- #556 says Refs for exactly this reason. My sentence above was an untested extrapolation from the issue's two bullet points; the fuzz is the measurement, and it contradicts me.

Full numbers and the site list are in #556. Nothing else in my description changes -- the shape check itself, and the reason it stays narrower than the schema, stand as written.

[ihadoop]

@jbonofre you were right, and it was a real hole rather than a missing test -- thank you for the precise diagnosis.

_require_shape returned None for None in both readings of it -- the key is absent, which every reader already handles, and the value is null, which the caller then calls .get() on. So datasets: [null] passed the check and raised anyway:

datasets: [null]      -> AttributeError: 'NoneType' object has no attribute 'get'
fields / metrics / relationships / expression.dialects / custom_extensions
                    -> the same class, one level deeper

Fixed in 52aa0ca: _require_shape takes present, every list element is checked with it, and the report names the path with not null instead of Python's NoneType -- the Ossie document is not convertible: datasets[0] must be a mapping, not null. All six containers you named are covered as parametrised cases, so they read as one table.

I kept one side of the distinction as a test rather than assuming it: metrics: null is still legal, because it means "no metrics" and the readers handle that. A check that cannot tell absent from null is the bug being fixed here, so trading it for a different false rejection would not have been an improvement.

Your comment also caught something I should have hit before pushing: the first version of my new cases hard-failed db_column_name / sql_output_column... actually that one was a test failure I found locally -- but writing the null cases surfaced test_no_internal_process_language rejecting a comment of mine, which is now reworded. Full suite on the branch: 1024 passed.

Anything else on this one before I move on to the remaining sweep sites?

[jbonofre]

This requires data and vendor_name to be strings on every custom_extensions entry. But read_stash skips entries whose vendor_name is not THOUGHTSPOT before it looks at data. So a document with e.g. {vendor_name: "SNOWFLAKE", data: {...}} converted fine before and now raises ConversionError. That conflicts with the docstring "a document that converts correctly today cannot start failing".

Could you only validate data when vendor_name == VENDOR_KEY? Or drop the data check here, since read_stash already reports it. A regression test with a non-Thoughtspot extension carrying a nested data mapping would pin this down.

[jbonofre]

Same concern as above, for dialects. Every expression.dialects[] entry must have string dialect and expression values, including entries for dialects we never read. If the readers only look at THOUGHTSPOT / ANSI_SQL, a sibling entry with an unexpected type under another dialect now fails the whole conversion.

Can we restrict the type check to the dialects the readers consume?

[jbonofre]

Nit: only the root ai_context is validated here. The readers also touch field.get("ai_context") in _restore_ai_context and dataset.get("ai_context"), and ai_context can appear on metrics and fields, so a wrongly typed value there can still reach them.