[pull] develop from mermaid-js:develop

.changeset/c4-named-attr-text-slot.md

@@ -0,0 +1,10 @@
+---
+'mermaid': patch
+---
+
+fix(c4): stop wrapping non-text named attributes that land in a text slot
+
+A named attribute such as `$tags` or `$sprite` can arrive in the positional
+slot of a text field when the argument before it is omitted. It was then
+stored as `{ text: value }` rather than a string, so `$tags` given without a
+description crashed rendering.

docs/syntax/c4.md

@@ -104,6 +104,8 @@ Mermaid's C4 diagram syntax is compatible with plantUML. See example below:
 
 ```
 
+The colours in this example are a demonstration of custom styling, not the defaults. `UpdateElementStyle` turns "Banking Customer A" red, and the `UpdateRelStyle` statements beside it recolour four relationships - three blue, including the one leaving "Banking Customer A", and the one arriving from the e-mail system red. Remove those statements and every element and relationship renders in the standard C4 palette.
+
 For an example, see the source code demos/index.html
 
 5 types of C4 charts are supported.
@@ -318,6 +320,8 @@ config:
 
 ```
 
+As above, "Banking Customer A" is red because the example restyles it with `UpdateElementStyle`, not because that is the default appearance of a `Person`.
+
 ## C4 Container diagram (C4Container)
 
 ```mermaid-example

packages/mermaid/src/diagrams/c4/c4Db.ts

@@ -15,6 +15,13 @@ import type { C4Boundary, C4Rel, C4Shape } from './c4Types.js';
  */
 type ParserAttribute = string | Record<string, string>;
 
+/**
+ * The fields a C4 element, boundary or relationship stores as `{ text }`. Everything else
+ * a named attribute can set - `$tags`, `$sprite`, `$link`, colours, `$shape` - is a plain
+ * string.
+ */
+const TEXT_FIELDS = new Set(['label', 'descr', 'techn', 'type']);
+
 /**
  * Apply optional C4 attributes to `bag` from the parser.
  *
@@ -31,7 +38,12 @@ const assignAttributes = <Bag extends C4Shape | C4Boundary | C4Rel>(
     }
     if (typeof value === 'object') {
       const [key, val] = Object.entries(value)[0];
-      bag[key] = val;
+      // Same rule as the positional text slots: a text field is stored as `{ text }`
+      // whichever slot it arrived through. `System_Boundary` and friends splice their
+      // kind in as a positional argument, so an explicit `$type` shifts along into this
+      // one and would otherwise land here as a bare string and overwrite the wrapped
+      // value the type slot just set.
+      bag[key] = TEXT_FIELDS.has(key) ? { text: val } : val;
     } else {
       bag[field] = value;
     }
@@ -110,22 +122,22 @@ export const addRel = function (
   rel.label = { text: label };
 
   if (techn === undefined || techn === null) {
-    rel.techn = { text: '' };
+    rel.techn ??= { text: '' };
   } else {
     if (typeof techn === 'object') {
       const [key, value] = Object.entries(techn)[0];
-      rel[key] = { text: value };
+      rel[key] = TEXT_FIELDS.has(key) ? { text: value } : value;
     } else {
       rel.techn = { text: techn };
     }
   }
 
   if (descr === undefined || descr === null) {
-    rel.descr = { text: '' };
+    rel.descr ??= { text: '' };
   } else {
     if (typeof descr === 'object') {
       const [key, value] = Object.entries(descr)[0];
-      rel[key] = { text: value };
+      rel[key] = TEXT_FIELDS.has(key) ? { text: value } : value;
     } else {
       rel.descr = { text: descr };
     }
@@ -167,11 +179,11 @@ export const addPersonOrSystem = function (
   }
 
   if (descr === undefined || descr === null) {
-    personOrSystem.descr = { text: '' };
+    personOrSystem.descr ??= { text: '' };
   } else {
     if (typeof descr === 'object') {
       const [key, value] = Object.entries(descr)[0];
-      personOrSystem[key] = { text: value };
+      personOrSystem[key] = TEXT_FIELDS.has(key) ? { text: value } : value;
     } else {
       personOrSystem.descr = { text: descr };
     }
@@ -216,22 +228,22 @@ export const addContainer = function (
   }
 
   if (techn === undefined || techn === null) {
-    container.techn = { text: '' };
+    container.techn ??= { text: '' };
   } else {
     if (typeof techn === 'object') {
       const [key, value] = Object.entries(techn)[0];
-      container[key] = { text: value };
+      container[key] = TEXT_FIELDS.has(key) ? { text: value } : value;
     } else {
       container.techn = { text: techn };
     }
   }
 
   if (descr === undefined || descr === null) {
-    container.descr = { text: '' };
+    container.descr ??= { text: '' };
   } else {
     if (typeof descr === 'object') {
       const [key, value] = Object.entries(descr)[0];
-      container[key] = { text: value };
+      container[key] = TEXT_FIELDS.has(key) ? { text: value } : value;
     } else {
       container.descr = { text: descr };
     }
@@ -276,22 +288,22 @@ export const addComponent = function (
   }
 
   if (techn === undefined || techn === null) {
-    component.techn = { text: '' };
+    component.techn ??= { text: '' };
   } else {
     if (typeof techn === 'object') {
       const [key, value] = Object.entries(techn)[0];
-      component[key] = { text: value };
+      component[key] = TEXT_FIELDS.has(key) ? { text: value } : value;
     } else {
       component.techn = { text: techn };
     }
   }
 
   if (descr === undefined || descr === null) {
-    component.descr = { text: '' };
+    component.descr ??= { text: '' };
   } else {
     if (typeof descr === 'object') {
       const [key, value] = Object.entries(descr)[0];
-      component[key] = { text: value };
+      component[key] = TEXT_FIELDS.has(key) ? { text: value } : value;
     } else {
       component.descr = { text: descr };
     }
@@ -339,7 +351,7 @@ export const addPersonOrSystemBoundary = function (
   } else {
     if (typeof type === 'object') {
       const [key, value] = Object.entries(type)[0];
-      boundary[key] = { text: value };
+      boundary[key] = TEXT_FIELDS.has(key) ? { text: value } : value;
     } else {
       boundary.type = { text: type };
     }
@@ -390,7 +402,7 @@ export const addContainerBoundary = function (
   } else {
     if (typeof type === 'object') {
       const [key, value] = Object.entries(type)[0];
-      boundary[key] = { text: value };
+      boundary[key] = TEXT_FIELDS.has(key) ? { text: value } : value;
     } else {
       boundary.type = { text: type };
     }
@@ -444,18 +456,18 @@ export const addDeploymentNode = function (
   } else {
     if (typeof type === 'object') {
       const [key, value] = Object.entries(type)[0];
-      boundary[key] = { text: value };
+      boundary[key] = TEXT_FIELDS.has(key) ? { text: value } : value;
     } else {
       boundary.type = { text: type };
     }
   }
 
   if (descr === undefined || descr === null) {
-    boundary.descr = { text: '' };
+    boundary.descr ??= { text: '' };
   } else {
     if (typeof descr === 'object') {
       const [key, value] = Object.entries(descr)[0];
-      boundary[key] = { text: value };
+      boundary[key] = TEXT_FIELDS.has(key) ? { text: value } : value;
     } else {
       boundary.descr = { text: descr };
     }

packages/mermaid/src/diagrams/c4/parser/c4NamedAttributes.spec.ts

@@ -0,0 +1,146 @@
+import c4Db from '../c4Db.js';
+// @ts-ignore: JISON doesn't support types
+import c4 from './c4Diagram.jison';
+import { setConfig } from '../../../config.js';
+
+setConfig({ securityLevel: 'strict' });
+
+/**
+ * A named attribute can land in a positional slot when the argument before it is omitted.
+ * Only the slot's own field is text, so the value must not be wrapped in `{ text }` just
+ * because it arrived through a text slot - consumers read `$tags` and `$sprite` as strings.
+ */
+describe('named attributes arriving in a positional text slot', function () {
+  beforeEach(function () {
+    c4.parser.yy = c4Db;
+    c4.parser.yy.clear();
+  });
+
+  it('keeps $tags a string when the description is omitted', function () {
+    c4.parser.parse(`C4Context\nSystem(s, "S", $tags="cylinder")`);
+
+    expect(c4.parser.yy.getC4ShapeArray()[0].tags).toBe('cylinder');
+  });
+
+  it('keeps $tags a string when the description is given', function () {
+    c4.parser.parse(`C4Context\nSystem(s, "S", "desc", $tags="cylinder")`);
+
+    const [shape] = c4.parser.yy.getC4ShapeArray();
+    expect(shape.tags).toBe('cylinder');
+    expect(shape.descr.text).toBe('desc');
+  });
+
+  it('still stores the description as text when it is the one named', function () {
+    c4.parser.parse(`C4Context\nSystem(s, "S", $descr="a description")`);
+
+    expect(c4.parser.yy.getC4ShapeArray()[0].descr.text).toBe('a description');
+  });
+
+  it('keeps $sprite and $link strings on a container', function () {
+    c4.parser.parse(`C4Container\nContainer(c, "C", $sprite="browser", $link="https://x.test")`);
+
+    const [shape] = c4.parser.yy.getC4ShapeArray();
+    expect(shape.sprite).toBe('browser');
+    expect(shape.link).toBe('https://x.test');
+  });
+
+  it('keeps a relationship $tags a string when techn and descr are omitted', function () {
+    c4.parser.parse(`C4Context
+Person(a, "A")
+Person(b, "B")
+Rel(a, b, "uses", $tags="async")`);
+
+    expect(c4.parser.yy.getRels()[0].tags).toBe('async');
+  });
+
+  // `Container` takes `techn` before `descr`, so a named `$descr` here lands in the
+  // `techn` slot. The techn handler assigned it correctly, and then the descr handler's
+  // "argument absent" branch overwrote it with an empty default - the description was
+  // silently lost rather than misshaped.
+  it('keeps a named $descr that lands in an earlier slot', function () {
+    c4.parser.parse(`C4Container\nContainer(c, "C", $descr="a description")`);
+
+    expect(c4.parser.yy.getC4ShapeArray()[0].descr.text).toBe('a description');
+  });
+
+  it('still defaults descr to empty when it is genuinely absent', function () {
+    c4.parser.parse(`C4Container\nContainer(c, "C", $techn="Java")`);
+
+    const [shape] = c4.parser.yy.getC4ShapeArray();
+    expect(shape.techn.text).toBe('Java');
+    expect(shape.descr.text).toBe('');
+  });
+
+  it('keeps a named $descr on a relationship, where techn also comes first', function () {
+    c4.parser.parse(`C4Context
+Person(a, "A")
+Person(b, "B")
+Rel(a, b, "uses", $descr="how it is used")`);
+
+    expect(c4.parser.yy.getRels()[0].descr.text).toBe('how it is used');
+  });
+
+  /**
+   * Every kind of declaration, given exactly one named attribute so that it lands in the
+   * first optional slot - which is rarely its own. Each bug found in this area so far
+   * only showed up for one such combination, so they are enumerated rather than sampled.
+   *
+   * `descr`, `techn` and `type` are stored as `{ text }`; `tags`, `sprite` and `link` are
+   * plain strings. Which slot the value travelled through must not change that.
+   */
+  describe('one named attribute, whichever slot it lands in', function () {
+    const TEXT_FIELDS = new Set(['descr', 'techn', 'type']);
+
+    const expectField = (subject: Record<string, unknown>, field: string) => {
+      if (TEXT_FIELDS.has(field)) {
+        expect(subject[field]).toEqual({ text: 'V' });
+      } else {
+        expect(subject[field]).toBe('V');
+      }
+    };
+
+    const shape = () => c4.parser.yy.getC4ShapeArray()[0];
+    const boundary = () =>
+      c4.parser.yy.getBoundaries().find((b: { alias: string }) => b.alias !== 'global');
+
+    const elements: [string, string, string[]][] = [
+      ['C4Context', 'System(x, "L", $F="V")', ['descr', 'tags', 'sprite', 'link']],
+      ['C4Context', 'Person(x, "L", $F="V")', ['descr', 'tags', 'sprite', 'link']],
+      ['C4Container', 'Container(x, "L", $F="V")', ['descr', 'techn', 'tags', 'sprite', 'link']],
+      ['C4Component', 'Component(x, "L", $F="V")', ['descr', 'techn', 'tags', 'sprite', 'link']],
+    ];
+    for (const [header, template, fields] of elements) {
+      it.each(fields)(`${template.replace('$F', '$%s')}`, function (field) {
+        c4.parser.parse(`${header}\n${template.replace('$F', `$${field}`)}`);
+        expectField(shape(), field);
+      });
+    }
+
+    // `System_Boundary` and `Container_Boundary` splice their kind in as a positional
+    // argument, so an explicit `$type` shifts one slot along into `tags`.
+    const boundaries: [string, string, string[]][] = [
+      ['C4Context', 'System_Boundary(x, "L", $F="V")', ['type', 'tags', 'link']],
+      ['C4Container', 'Container_Boundary(x, "L", $F="V")', ['type', 'tags', 'link']],
+      ['C4Deployment', 'Node(x, "L", $F="V")', ['type', 'descr', 'tags', 'sprite', 'link']],
+    ];
+    for (const [header, template, fields] of boundaries) {
+      it.each(fields)(`${template.replace('$F', '$%s')}`, function (field) {
+        c4.parser.parse(
+          `${header}\n${template.replace('$F', `$${field}`)} {\nContainer(i, "I")\n}`
+        );
+        expectField(boundary(), field);
+      });
+    }
+
+    it.each(['techn', 'descr', 'tags', 'sprite', 'link'])(
+      'Rel(a, b, "uses", $%s="V")',
+      function (field) {
+        c4.parser.parse(`C4Context
+Person(a, "A")
+Person(b, "B")
+Rel(a, b, "uses", $${field}="V")`);
+        expectField(c4.parser.yy.getRels()[0], field);
+      }
+    );
+  });
+});

packages/mermaid/src/docs/syntax/c4.md

@@ -51,6 +51,8 @@ Mermaid's C4 diagram syntax is compatible with plantUML. See example below:
 
 ```
 
+The colours in this example are a demonstration of custom styling, not the defaults. `UpdateElementStyle` turns "Banking Customer A" red, and the `UpdateRelStyle` statements beside it recolour four relationships - three blue, including the one leaving "Banking Customer A", and the one arriving from the e-mail system red. Remove those statements and every element and relationship renders in the standard C4 palette.
+
 For an example, see the source code demos/index.html
 
 5 types of C4 charts are supported.
@@ -216,6 +218,8 @@ config:
 
 ```
 
+As above, "Banking Customer A" is red because the example restyles it with `UpdateElementStyle`, not because that is the default appearance of a `Person`.
+
 ## C4 Container diagram (C4Container)
 
 ```mermaid-example