diff --git a/.changeset/c4-named-attr-text-slot.md b/.changeset/c4-named-attr-text-slot.md new file mode 100644 index 00000000000..3f6268ada64 --- /dev/null +++ b/.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. diff --git a/docs/syntax/c4.md b/docs/syntax/c4.md index 4d246e7399f..e17baa6aea1 100644 --- a/docs/syntax/c4.md +++ b/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 diff --git a/packages/mermaid/src/diagrams/c4/c4Db.ts b/packages/mermaid/src/diagrams/c4/c4Db.ts index f08f3522c43..5a7b057cab4 100644 --- a/packages/mermaid/src/diagrams/c4/c4Db.ts +++ b/packages/mermaid/src/diagrams/c4/c4Db.ts @@ -15,6 +15,13 @@ import type { C4Boundary, C4Rel, C4Shape } from './c4Types.js'; */ type ParserAttribute = string | Record; +/** + * 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 = ( } 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 }; } diff --git a/packages/mermaid/src/diagrams/c4/parser/c4NamedAttributes.spec.ts b/packages/mermaid/src/diagrams/c4/parser/c4NamedAttributes.spec.ts new file mode 100644 index 00000000000..3a927b785ae --- /dev/null +++ b/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, 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); + } + ); + }); +}); diff --git a/packages/mermaid/src/docs/syntax/c4.md b/packages/mermaid/src/docs/syntax/c4.md index e75b205a377..f7c1ac6eba4 100644 --- a/packages/mermaid/src/docs/syntax/c4.md +++ b/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