Read a string in a cell as a string, in the binary dictionary too

package.json

@@ -1,6 +1,6 @@
 {
   "name": "data-explorer-core",
-  "version": "1.22.0",
+  "version": "1.23.0",
   "description": "Parser, data model, and node schema for Simulink data-dictionary, model, MAT-file, and project files. Presentation-independent; embeddable in-process.",
   "license": "BSD-3-Clause",
   "type": "module",

src/datamodel/parser/BinarySlddParser.ts

@@ -582,6 +582,16 @@ function parseCellElement(el: XmlNode): unknown {
   // Nested object
   const childElements = el.Element;
   if (childElements && childElements.length > 0) {
+    // A nested MATLAB string, decoded to its text through the same helper the entry
+    // path (parseEntryValue) and the property path (parsePropContent) already use —
+    // this was the third nesting site and the only one without the branch, so a
+    // `string` in a cell decoded as a generic OBJECT of class `string` and displayed
+    // as `<1x1 string>` with the text stranded in the saveobj bag. The check has to
+    // come before parseElement below for the same reason it does in parsePropContent:
+    // the object tail is the fall-through, not a case this shape belongs to.
+    if (childElements[0]['@_Class'] === 'string') {
+      return parseStringValue(childElements[0], dimension);
+    }
     return parseElement(childElements[0]);
   }
   return text || '';

test/binarySlddValues.test.ts

@@ -231,6 +231,47 @@ describe('parseBinarySlddParts — cell elements', () => {
     });
   });
 
+  // REGRESSION. A `string` in a cell is the THIRD place this format nests one, and it
+  // was the one place the reader had no branch for it: parseEntryValue has had the
+  // Class="string" check since strings were supported, parsePropContent got its own for
+  // a struct field / object property, and parseCellElement never got one. So the element
+  // fell through to the generic nested-object tail and decoded as an OBJECT of class
+  // `string` — `{"a"}` displayed as `{<1x1 string>}`, with the text stranded inside the
+  // saveobj bag where no formatter looks. The JSON channel showed `{"a"}` the whole time,
+  // so the same dictionary read two ways depending on which format it was saved in.
+  //
+  // The XML below is MATLAB's own bytes, read off a compressed-binary dictionary MATLAB
+  // wrote for `{"a"}` and `{["a" "b"]}` (probe_cell_string.m): a cell element holding an
+  // object is a CLASSLESS <Element> wrapping the object's own <Element Class="...">, which
+  // is why the nested-object test above is right for a Simulink.Parameter and why only
+  // `string` needs lifting out of that tail.
+  it('REGRESSION: decodes a string element to its text, not an object summary', () => {
+    const stringEl = (saveobjAttrs: string, ...chars: string[]): string =>
+      '<Element><Element Class="string">' +
+      `<P Source="saveobj" PropertyType="any" Class="cell"${saveobjAttrs}>` +
+      chars.map((c) => `<Element Class="char">${c}</Element>`).join('') +
+      '</P></Element></Element>';
+
+    // A 1x1 string is the one-element LIST, which is exactly what the entry path and
+    // the JSON channel both produce — so the cell's child is a string-kind node and
+    // _serializeCellXml writes MATLAB's envelope back (see cellElementShape.test.ts).
+    // MATLAB leaves the Dimension off the saveobj cell at 1x1, so that spelling is the
+    // one that has to work.
+    expect(cellElements(stringEl('', 'a'))).toEqual([['a']]);
+    // A string ARRAY keeps its shape in the String envelope, as it does everywhere else.
+    expect(cellElements(stringEl(' Dimension="1*2"', 'a', 'b'))).toEqual([
+      {
+        _array_type: 'String',
+        _dimensions: [1, 2],
+        _elements: ['a', 'b'],
+        _mw_element_type: 'MATLABArray',
+      },
+    ]);
+    // Two string elements side by side: each wrapper is decoded on its own, so the
+    // cell keeps its length. MATLAB writes one wrapper per element (cStrTwo).
+    expect(cellElements(stringEl('', 'a'), stringEl('', 'b'))).toEqual([['a'], ['b']]);
+  });
+
   it('falls back to the element text for a class it does not decode', () => {
     // A cell can hold a type from a release we do not model. Showing the raw text
     // beats dropping the element, which would silently shorten the cell.

test/binaryWriteBackGate.test.ts

@@ -388,3 +388,149 @@ describe('a string Parameter value survives the binary write-back', () => {
     expect(findByName(reopened, 'aParam').displayValue).toBe('"abc"');
   });
 });
+
+// Defect 52 — a `string` inside a CELL, in the binary channel only. The report was
+// cosmetic (`{"a"}` displayed as `{<1x1 string>}`) and the write path was not: rebuilt,
+// that element went out as an EMPTY `<Element Class="string">`, so the text was gone from
+// the file. Both halves have one cause, and it is this repo's recurring one. Three sites
+// in BinarySlddParser nest MATLAB's string object — parseEntryValue for an entry's own
+// value, parsePropContent for a struct field or an object property, parseCellElement for a
+// cell element — and only the first two had a `Class="string"` branch. Without it the
+// element fell through to the generic nested-object tail and decoded as an OBJECT of class
+// `string`, whose text sits in the saveobj bag where neither a formatter nor a writer
+// looks: hence the summary on screen and the empty envelope in the file.
+//
+// The fixtures are MATLAB's own, written by `test/parity/matlab/probe_cell_string.m`,
+// which also records the shape the missing branch keys on: a cell element holding an
+// OBJECT is a classless `<Element>` wrapping the object's own `<Element Class="...">`.
+// That shape is why the tail was right for every other class, and why the fix lifts
+// `string` out of it rather than replacing it.
+describe('defect 52: a string in a cell keeps its text, in the file as well as on screen', () => {
+  // MATLAB's own value for each, as `getValue()` reports it in probe_cell_string.m's
+  // read-back — `cell[1 1] of {string[1 1]}` and so on.
+  const CELLS: Array<[string, string]> = [
+    ['cStr', '{"a"}'], // the reported case
+    ['cStrArr', '{["a" "b"]}'], // a string ARRAY in a cell: the shape has to survive too
+    ['cStrTwo', '{"a", "b"}'], // two elements, each wrapped on its own
+    ['cMixed', '{1, "a", \'b\'}'], // beside a double and a char, neither of which it may become
+  ];
+
+  /**
+   * The `<Object>` block of one entry, from a chunk either MATLAB or we wrote, with its
+   * LastMod stamp dropped — a rebuild restamps a modified entry, by design, so the stamp is
+   * the one line that cannot match and the only one whose mismatch means nothing.
+   */
+  function entryObject(xml: string, name: string): string {
+    const at = xml.indexOf('>' + name + '</P>');
+    expect(at, name).toBeGreaterThan(-1);
+    const open = xml.lastIndexOf('<Object ', at);
+    const block = xml.slice(open, xml.indexOf('</Object>', at) + '</Object>'.length);
+    return block.replace(/\s*<P Name="LastMod"[^>]*>[^<]*<\/P>/, '');
+  }
+
+  /** MATLAB's own chunk for a fixture, to diff a rebuild against. */
+  function matlabChunk(fixture: string): string {
+    const p = fileURLToPath(new URL('./' + fixture, import.meta.url));
+    const zip = unzipSync(new Uint8Array(readFileSync(p)));
+    return new TextDecoder().decode(zip['data/chunk0.xml']);
+  }
+
+  it('reads MATLAB\'s own bytes as strings, not as <1x1 string>', () => {
+    const sldd = loadFile('../fixtures/cellstr_binary.sldd', 'cellstr_binary.sldd');
+    for (const [name, display] of CELLS) {
+      expect(String(findEntry(sldd, name).displayValue), name).toBe(display);
+    }
+    // The CLASS as well as the text: decoded as an object the child's whole value was the
+    // summary string, so a text-only assertion could pass on a node with no string in it.
+    expect(findEntry(sldd, 'cStr').children[0].dataType).toBe('string');
+    expect(findEntry(sldd, 'cStrArr').children[0].dataType).toBe('string');
+    // The control that keeps the fix narrow: a non-string object in a cell still goes
+    // through the nested-object tail the string branch was lifted out of.
+    expect(findEntry(sldd, 'cObj').children[0].className).toBe('Simulink.Parameter');
+    // And the sibling site that already had its branch, so a regression there is visible
+    // here too rather than only in the entry-level tests. Asserted on the FIELD, because a
+    // struct entry summarizes: sStr's own display is `<1x1 struct>` whatever its field holds.
+    const f = findEntry(sldd, 'sStr').children[0];
+    expect(f.name).toBe('f');
+    expect(String(f.displayValue)).toBe('"a"');
+    expect(f.dataType).toBe('string');
+  });
+
+  it('and reads them the same way the TEXT channel does', () => {
+    // The invariant the defect broke, stated BETWEEN the channels rather than as two
+    // per-channel expectations: one dictionary saved in either format is one value. Two
+    // separate expectations is what this repo had — the JSON channel was right the whole
+    // time and said nothing about the XML one.
+    const bin = loadFile('../fixtures/cellstr_binary.sldd', 'cellstr_binary.sldd');
+    // Every ELEMENT too, not just the entry row: the element is where the class lives, and
+    // `{<1x1 string>}` differs from `{"a"}` at the entry row only because the element's
+    // display is interpolated into it. A container whose row agreed and whose contents did
+    // not is exactly what this defect was.
+    const txt = loadFile('../fixtures/cellstr_text.sldd', 'cellstr_text.sldd');
+    const shown = (root: any, name: string): string[] => {
+      const n = findEntry(root, name);
+      return [String(n.displayValue), ...n.children.map((c: any) => String(c.displayValue))];
+    };
+    for (const [name] of [...CELLS, ['cObj']]) {
+      expect(shown(bin, name), name).toEqual(shown(txt, name));
+    }
+    // sStr is compared on its own row alone, and deliberately not on its field. The channels
+    // DISAGREE there, for a reason that has nothing to do with strings or with cells: the
+    // build of MATLAB that wrote these two fixtures (27.1.0.3393633) no longer emits
+    // `_fields` in a text dictionary, and StructNode.parse builds a scalar struct's field
+    // children from `_fields` alone — so the text channel shows this struct with no field row
+    // at all, while the binary channel derives the list from the element bag
+    // (BinarySlddParser's `_fields: Object.keys(parsed[0])`). That is its own defect, with its
+    // own blast radius — every struct in every current-MATLAB text .sldd — and it is not
+    // asserted either way here.
+    expect(String(findEntry(bin, 'sStr').displayValue)).toBe(
+      String(findEntry(txt, 'sStr').displayValue),
+    );
+  });
+
+  it('writes every saveobj payload back, rather than an empty envelope', () => {
+    // The silent half, on the rebuild probe_writeback_bin hands MATLAB and with nothing
+    // edited: before the fix, merely SAVING a dictionary that contained a string in a cell
+    // deleted the text.
+    const xml = rebuildXml('fixtures/cellstr_binary.sldd');
+    expect(xml).not.toMatch(/<Element Class="string">\s*<\/Element>/);
+    expect(xml).not.toContain('<Element Class="string"/>');
+    // Counted, because "no empty envelope" is also true of a chunk that dropped the
+    // elements entirely. Six strings in six places: cStr, cStrArr, cStrTwo x2, cMixed,
+    // sStr — every nesting site the format has, in one file.
+    const envelopes = xml.match(/<P Source="saveobj" PropertyType="any" Class="cell"/g) ?? [];
+    expect(envelopes).toHaveLength(6);
+    expect(xml.match(/<P Source="saveobj" PropertyType="any" Class="cell" Dimension="1\*2"/g))
+      .toHaveLength(1); // cStrArr's, the only one that is not 1x1
+  });
+
+  it('and the rebuilt chunk reopens as the values MATLAB wrote', () => {
+    // End to end, which is the assertion that would have failed loudest before the fix:
+    // read MATLAB's file, write it back untouched, read that. Text-level checks can all
+    // pass on a chunk whose envelope is intact but attached to the wrong element.
+    const xml = rebuildXml('fixtures/cellstr_binary.sldd');
+    const uri = 'mem://cellstr-reopen';
+    DataModel.removeDataSource(uri);
+    const reopened = DataModel.addDataSource(uri, parseBinarySlddParts(xml, {}), {
+      path: 'cellstr_binary.sldd',
+    });
+    for (const [name, display] of CELLS) {
+      expect(String(findByName(reopened, name).displayValue), name).toBe(display);
+    }
+  });
+
+  it('byte for byte MATLAB\'s own, for the cells MATLAB writes a Dimension on', () => {
+    // The strongest form available: the rebuilt entry IS MATLAB's bytes. Restricted to the
+    // two multi-element cells because of one difference that predates this defect and has
+    // nothing to do with strings — on a 1x1 cell we write `Dimension="1*1"` where MATLAB
+    // writes no attribute at all (cObj's Simulink.Parameter cell has it too, and it
+    // round-trips: parseEntryValue defaults an absent Dimension to 1x1). That is save
+    // churn, not data loss, and pinning these two keeps the string bytes themselves exact
+    // while leaving that one free.
+    const ours = rebuildXml('fixtures/cellstr_binary.sldd');
+    const theirs = matlabChunk('fixtures/cellstr_binary.sldd');
+    for (const name of ['cStrTwo', 'cMixed']) {
+      expect(entryObject(ours, name), name).toBe(entryObject(theirs, name));
+    }
+  });
+});

test/cellElementShape.test.ts

@@ -228,33 +228,27 @@ for (const format of ['json', 'binary'] as SlddFormat[]) {
       expect(fresh.children[0].dataType).toBe('char');
     });
 
+    // Both of these used to re-read as `{<1x1 string>}` on the BINARY channel only, and
+    // that was pinned here as a limitation attributed to the undecoded .mat MCOS payload.
+    // It was not that at all: an .sldd carries no MCOS blob, and the write side was
+    // already byte-correct. The reader was missing one branch —
+    // BinarySlddParser.parseCellElement had no Class="string" case, so MATLAB's own
+    // spelling for a string in a cell (a classless <Element> wrapping
+    // <Element Class="string">, confirmed against R2027a by probe_cell_string.m) fell through
+    // to the generic nested-object tail and became an object of class `string`. Both
+    // channels now agree, which is the whole point of running this block over both.
     it('a string array stays a string array', () => {
       const { display, fresh } = editAndReread('{["a"; "b"]}');
       // The edit itself is right in both channels — this is the 2x1 that used to
       // come back as `{["a" "b"]}`.
       expect(display).toBe('{["a"; "b"]}');
-      if (format === 'binary') {
-        // Then the XML channel loses it, and NOT because of anything above: a string
-        // nested in a cell is written as an MCOS payload this repo does not decode
-        // for an unnamed value, so it re-reads as an opaque summary with no text.
-        // That is the known limitation DESIGN.md records for a string in a struct
-        // field or cell element (and it degrades the same way with the shape fix
-        // reverted). Pinned rather than skipped, so the day the payload is decoded
-        // this line fails and the real assertion below takes over.
-        expect(String(fresh.displayValue)).toBe('{<1x1 string>}');
-        return;
-      }
       expect(String(fresh.displayValue)).toBe('{["a"; "b"]}');
       expect(fresh.children[0].dataType).toBe('string');
     });
 
     it('a 1x1 string stays a string', () => {
       const { display, fresh } = editAndReread('{"a"}');
       expect(display).toBe('{"a"}');
-      if (format === 'binary') {
-        expect(String(fresh.displayValue)).toBe('{<1x1 string>}');
-        return;
-      }
       expect(fresh.children[0].dataType).toBe('string');
       expect(String(fresh.displayValue)).toBe('{"a"}');
     });
@@ -279,14 +273,10 @@ for (const format of ['json', 'binary'] as SlddFormat[]) {
         const first = editAndReread(typed);
         const second = editAndReread(first.display);
         expect(second.display, typed).toBe(first.display);
-        // On the binary channel a string element does not survive the write at all
-        // (see 'a string array stays a string array'), so what re-reads is the
-        // opaque summary rather than the value. The DISPLAY invariant above is the
-        // one this test is about and it holds in both channels.
-        if (format === 'binary' && typed.indexOf('"') >= 0) {
-          expect(String(second.fresh.displayValue), typed).toBe('{<1x1 string>}');
-          continue;
-        }
+        // No per-format exception any more: the string cases used to need one on the
+        // binary channel because a string element did not survive the re-read (see the
+        // note above the two string tests). Every shape in this list now re-reads as
+        // the value it displays, in both channels.
         expect(String(second.fresh.displayValue), typed).toBe(first.display);
       }
     });