diff --git a/docs/data-formats.md b/docs/data-formats.md index 32e7198..dfe7247 100644 --- a/docs/data-formats.md +++ b/docs/data-formats.md @@ -385,7 +385,7 @@ document's own subject, and not a 3rd-party package. It appears as a component m "name": "ui-lib", "version": "2.1.0", "purl": "pkg:generic/ui-lib@2.1.0", - "group": "/OrgName/GroupA", + "group": "GroupA", "properties": [ { "name": "softagram:internal", "value": "true" }, { "name": "softagram:packageName", "value": "ui-lib" }, @@ -522,7 +522,7 @@ and every internal component — publishes its position in the model: ```json { "bom-ref": "repoa", "name": "repoA", - "group": "/OrgName/GroupA", + "group": "GroupA", "type": "application", "version": "", "purl": "", @@ -537,7 +537,7 @@ and every internal component — publishes its position in the model: | Field | Meaning | |-------|---------| -| `group` | The **full path of the parent element**, not just its name. Omitted for a top-level element, which has no parent path. | +| `group` | The **name of the parent element**. Omitted for a top-level element, whose parent is the model root and has no name. | | `properties[softagram:elementPath]` | The element's own full path. A property rather than a field because the CycloneDX component schema sets `additionalProperties: false`. | | `properties[softagram:elementType]` | What the **model** calls this element — `repository`, `dir`, `file`, and so on. **Present only when the model carries a type**; see below. | | `externalReferences[type=vcs]` | The `repo_url` of the element, or of the **nearest ancestor** carrying a non-blank one. Absent when no ancestor has one — never a placeholder. | @@ -581,37 +581,36 @@ Components describing **3rd-party packages** carry none of these. Their identity single-SBOM document. This is long-standing behaviour rather than a recent change, and altering it would change the identity every existing single-SBOM consumer files the document under, so it is documented here rather than quietly repaired. -- `group + '/' + name == elementPath` below the top level. -- Two repositories that share a name under different groups are distinguished by `group` and - `elementPath`. They are *not* reliably distinguished by `bom-ref`, whose collision suffix - (`repoa`, `repoa-2`) depends on traversal order and can change between model generations. +- `elementPath` ends with `'/' + group + '/' + name` below the top level. `group` is a single + segment of the path, not the whole of it — read `elementPath` when you need the location. +- Two repositories that share a name under **differently named** groups are distinguished by + `group` and `elementPath`; under **identically named** groups in different parts of the tree, + by `elementPath` alone. They are *not* reliably distinguished by `bom-ref`, whose collision + suffix (`repoa`, `repoa-2`) depends on traversal order and can change between model + generations. ### Caveats - **The first path segment is the estate root and is not stable.** It changes when the estate is renamed or restructured. Read it from the path rather than hardcoding it. -- **`group` holds a path, not a package namespace.** The CycloneDX specification suggests - avoiding special characters in `group` and shows package coordinates such as - `org.apache.commons`. A model group is a tree location, so this converter puts the parent's - full path there. Tools that render `group` as a package coordinate will show the path. - - **What the path buys, stated accurately.** It distinguishes two *groups* that share a name under - different parents — `/Estate/TeamA/tools` and `/Estate/TeamB/tools` — *in a model that has such - a pair*. Many models have none. Measured across three real single-root estates at both level 2 - and level 3, every full-path `group` value mapped one-to-one onto its bare name (73 → 73, 78 → - 78, 11 → 11), so the prefix disambiguated nothing in any of them. - - So the path is a guarantee that holds for *every* model, not a fix for a collision every model - has, and a reader should not infer that their own `group` values would collide without it. Note - the collision it guards against is between **groups**; two repositories sharing a name under - *differently* named groups are already distinguished by the group name alone. - - **If you want the bare name, take it from the path**; the last segment is the parent's name. - And if you want an unambiguous identifier for the element itself, use - `properties[softagram:elementPath]`, which is unique across all documents from one model and is - the string the serial number is derived from — `group` is not the field to reach for. Note the - first path segment is the estate root and changes when the estate is renamed, so anything - derived from `group` inherits that instability. +- **`group` does not identify an element on its own.** It is the parent's name, so two groups + that share a name in different parts of the tree — `/Estate/TeamA/tools` and + `/Estate/TeamB/tools` — produce the same `group`. Use `properties[softagram:elementPath]` when + you need an unambiguous identifier: it is unique across all documents from one model and is the + string the serial number is derived from. + + **This is a deliberate trade, changed in 1.17.0.** `group` previously carried the parent's full + path, which kept such a pair apart. Two things made that the worse deal. CycloneDX defines + `group` as "a shortened, single name of the company or project that produced the component" and + asks that special characters be avoided, so a path was not what a consumer reading the field + expects. More seriously, the first path segment is the estate root — the least stable part of + the path — and `group` is an identity field, so renaming an estate re-identified every component + beneath it at once. The collision the path guarded against was not observed in any of ten real + estates measured, where every full-path `group` mapped one-to-one onto its bare name; the rename + cost was observed, across 689 documents in a live estate. + + **Consumers who parsed the path out of `group` should read `softagram:elementPath` instead**, + which is unchanged and still carries the full path. - **`purl` and `version` are empty on the metadata component**, and on an internal component whose element publishes no unambiguous package. A repository has no package identity and no version of its own; a path is not a valid purl and is deliberately not placed there. An diff --git a/docs/superpowers/specs/2026-08-11-sbom-element-path-design.md b/docs/superpowers/specs/2026-08-11-sbom-element-path-design.md index 59c5e90..9f7d87e 100644 --- a/docs/superpowers/specs/2026-08-11-sbom-element-path-design.md +++ b/docs/superpowers/specs/2026-08-11-sbom-element-path-design.md @@ -4,6 +4,13 @@ Design for a change to `src/sgraph/converters/sbom_cyclonedx_generator.py`. Baseline: sgraph **1.9.0** (`upstream/main` @ `fb65e0d`), 237 tests green. +> **Partly superseded in 1.17.0.** This document specifies `group` as the parent element's +> *full path*. That was changed to the parent's *bare name*: the path put the unstable estate +> root inside an identity field, and CycloneDX defines `group` as a single name rather than a +> location. The rest of the design — `softagram:elementPath`, the derived serial, the vcs +> ancestor walk — is unchanged. See "Caveats" in `docs/data-formats.md`. This file is kept as +> the record of the original decision. + --- ## 1. The problem diff --git a/src/sgraph/converters/sbom_cyclonedx_generator.py b/src/sgraph/converters/sbom_cyclonedx_generator.py index e05a2e6..046f2bf 100644 --- a/src/sgraph/converters/sbom_cyclonedx_generator.py +++ b/src/sgraph/converters/sbom_cyclonedx_generator.py @@ -930,9 +930,22 @@ def analyze_3rdparty(external_root, sbom): def _add_element_location(component, elem): """Publish where elem sits in the model, and what the model calls it, on a component for elem. - 'group' carries the parent's full path rather than its bare name so that two identically - named groups under different roots stay distinguishable, and is omitted for a top-level - element, whose parent is the model root and has no path of its own. + 'group' carries the parent's bare name, and is omitted for a top-level element, whose + parent is the model root and has no name of its own. + + It used to carry the parent's full path, to keep two identically named groups under + different roots apart. That traded a collision no measured estate had for two costs every + estate paid. CycloneDX defines group as "a shortened, single name of the company or + project that produced the component" and asks that special characters be avoided, so a + value like '/Estate/AccessManagement' is not what a consumer reading the field expects. + Worse, the first path segment is the estate root -- the least stable part of the path -- + and group is an identity field, so renaming an estate re-identified every component + beneath it at once. That happened to a live estate across 689 documents. + + The collision the path guarded against is now accepted knowingly: two same-named groups + under different roots share a group value. Identity does not rest on it. The full path is + still published verbatim as softagram:elementPath, the document serial is derived from + that path, and both still tell such a pair apart. The element's own path goes into a property because CycloneDX sets additionalProperties: false on component, leaving properties[] as the only schema-valid place for it. It is also @@ -959,9 +972,9 @@ def _add_element_location(component, elem): elementPath yields a document a consumer cannot resolve and cannot detect as broken. Raising at the call site is the better failure. """ - parent_path = elem.parent.getPath() - if parent_path: - component['group'] = parent_path + parent_name = elem.parent.name + if parent_name: + component['group'] = parent_name component.setdefault('properties', []).append({ 'name': ELEMENT_PATH_PROPERTY, 'value': elem.getPath() diff --git a/tests/converters/sbom_cyclonedx_generator_test.py b/tests/converters/sbom_cyclonedx_generator_test.py index 8f1e5e8..96d0634 100644 --- a/tests/converters/sbom_cyclonedx_generator_test.py +++ b/tests/converters/sbom_cyclonedx_generator_test.py @@ -1639,13 +1639,13 @@ def test_metadata_component_carries_element_path(): assert find_property(component, 'softagram:elementPath') == '/OrgName/GroupA/repoA' -def test_metadata_component_carries_the_parent_path_as_group(): - """group holds the parent's full path, so two same-named groups stay distinguishable.""" +def test_metadata_component_carries_the_parent_name_as_group(): + """group holds the parent's bare name, which is what CycloneDX asks the field to hold.""" model, _ = get_model_and_model_api(MULTI_MODEL) result = generate_multi_from_sgraph(model, level=3) - assert sbom_of(result, 'repoA')['metadata']['component']['group'] == '/OrgName/GroupA' - assert sbom_of(result, 'repoB')['metadata']['component']['group'] == '/OrgName/GroupA' + assert sbom_of(result, 'repoA')['metadata']['component']['group'] == 'GroupA' + assert sbom_of(result, 'repoB')['metadata']['component']['group'] == 'GroupA' def test_element_path_matches_the_serial_number_for_every_sbom(): @@ -1669,7 +1669,7 @@ def test_element_location_is_level_agnostic(): result = generate_multi_from_sgraph(model, level=2) component = sbom_of(result, 'GroupA')['metadata']['component'] - assert component['group'] == '/OrgName' + assert component['group'] == 'OrgName' assert find_property(component, 'softagram:elementPath') == '/OrgName/GroupA' @@ -1692,7 +1692,7 @@ def test_selected_element_sbom_also_carries_its_location(): sbom = generate_for_element_from_sgraph(model, '/OrgName/GroupA/repoA/src') component = sbom['metadata']['component'] - assert component['group'] == '/OrgName/GroupA/repoA' + assert component['group'] == 'repoA' assert find_property(component, 'softagram:elementPath') == '/OrgName/GroupA/repoA/src' @@ -1713,11 +1713,11 @@ def test_mirrored_repositories_are_distinguished_by_their_location(): assert [c['name'] for c in components] == ['shared', 'shared'] by_group = {c['group']: c for c in components} - assert sorted(by_group) == ['/OrgName/GroupA', '/OrgName/GroupB'] + assert sorted(by_group) == ['GroupA', 'GroupB'] - assert find_property(by_group['/OrgName/GroupA'], 'softagram:elementPath') \ + assert find_property(by_group['GroupA'], 'softagram:elementPath') \ == '/OrgName/GroupA/shared' - assert find_property(by_group['/OrgName/GroupB'], 'softagram:elementPath') \ + assert find_property(by_group['GroupB'], 'softagram:elementPath') \ == '/OrgName/GroupB/shared' # The collision suffix keeps bom-refs unique within the set, but does not identify either one @@ -1764,9 +1764,9 @@ def test_mirrored_repositories_carry_their_own_distinct_repository_urls(): by_group = {sbom['metadata']['component']['group']: sbom['metadata']['component'] for sbom in result} - assert [r['url'] for r in by_group['/OrgName/GroupA']['externalReferences'] + assert [r['url'] for r in by_group['GroupA']['externalReferences'] if r['type'] == 'vcs'] == ['https://example.org/org/groupa-shared.git'] - assert [r['url'] for r in by_group['/OrgName/GroupB']['externalReferences'] + assert [r['url'] for r in by_group['GroupB']['externalReferences'] if r['type'] == 'vcs'] == ['https://example.org/org/groupb-shared.git'] @@ -1780,7 +1780,7 @@ def test_nearest_repo_url_wins_over_a_more_distant_ancestor(): result = generate_multi_from_sgraph(model, level=3) component = next(s['metadata']['component'] for s in result - if s['metadata']['component']['group'] == '/OrgName/GroupA') + if s['metadata']['component']['group'] == 'GroupA') vcs = [r['url'] for r in component['externalReferences'] if r['type'] == 'vcs'] assert vcs == ['https://example.org/org/groupa-shared.git'] @@ -1807,7 +1807,7 @@ def test_transitive_internal_components_carry_their_location(): result = generate_multi_from_sgraph(model, level=3, transitive=True) repo_b = next(c for c in sbom_of(result, 'repoA')['components'] if c['name'] == 'repoB') - assert repo_b['group'] == '/OrgName/GroupA' + assert repo_b['group'] == 'GroupA' assert find_property(repo_b, 'softagram:elementPath') == '/OrgName/GroupA/repoB' # The pre-existing internal marker survives alongside the new property assert find_property(repo_b, 'softagram:internal') == 'true' @@ -1825,12 +1825,12 @@ def test_inlined_mirror_is_told_apart_from_its_host_by_its_published_location(): model, _ = get_model_and_model_api(MIRRORED_MODEL) result = generate_multi_from_sgraph(model, level=3, transitive=True) - host = next(s for s in result if s['metadata']['component']['group'] == '/OrgName/GroupA') + host = next(s for s in result if s['metadata']['component']['group'] == 'GroupA') inlined = next(c for c in host['components'] if find_property(c, 'softagram:internal') == 'true') assert inlined['name'] == host['metadata']['component']['name'] == 'shared' - assert inlined['group'] == '/OrgName/GroupB' + assert inlined['group'] == 'GroupB' assert find_property(inlined, 'softagram:elementPath') == '/OrgName/GroupB/shared' # List comprehensions rather than a {type: url} dict: these pin cardinality too, so a @@ -1838,7 +1838,7 @@ def test_inlined_mirror_is_told_apart_from_its_host_by_its_published_location(): vcs = [r['url'] for r in inlined['externalReferences'] if r['type'] == 'vcs'] assert vcs == ['https://example.org/org/groupb-shared.git'] - mirror = next(s for s in result if s['metadata']['component']['group'] == '/OrgName/GroupB') + mirror = next(s for s in result if s['metadata']['component']['group'] == 'GroupB') mirror_serial = mirror['serialNumber'].replace('urn:uuid:', '') bom_links = [r['url'] for r in inlined['externalReferences'] if r['type'] == 'bom'] assert bom_links == [f'urn:cdx:{mirror_serial}/1'] @@ -4402,3 +4402,110 @@ def test_cli_rejects_an_unsupported_spec_version(tmp_path): assert '--spec-version' in proc.stderr for supported in SUPPORTED_SPEC_VERSIONS: assert supported in proc.stderr, proc.stderr + + +# --- group is a name, not a path --- +# +# 'group' used to carry the parent's full path. Three things were wrong with that, and the +# first two were already conceded in docs/data-formats.md as caveats rather than repaired: +# +# 1. CycloneDX defines group as "a shortened, single name of the company or project that +# produced the component" and says "whitespace and special characters should be +# avoided". A value like '/Estate/AccessManagement' is neither. +# 2. The first path segment is the estate root, the least stable part of the path. Because +# group is an identity field, renaming an estate rewrote the identity of every component +# in every document at once. That is not hypothetical: a reporting estate renamed its +# root and saw exactly this across 689 documents. +# 3. The disambiguation it was said to buy was not measured in any real estate. Across ten +# of them every full-path group mapped one-to-one onto its bare name. +# +# The full path remains published, in softagram:elementPath, which is the schema-lawful place +# for it and is what a consumer needing a tree location should read. + + +def test_group_is_a_name_not_a_path(): + """A path in group contradicts what CycloneDX says the field is for.""" + model, _ = get_model_and_model_api(MIRRORED_MODEL) + + documents = generate_multi_from_sgraph(model, level=3, transitive=True) + + groups = [d['metadata']['component']['group'] for d in documents + if d['metadata']['component'].get('group')] + assert groups, 'fixture produced no group values to check' + for group in groups: + assert '/' not in group, ( + f'group {group!r} is a path; CycloneDX asks for a single name with special ' + f'characters avoided, and the full path is published as softagram:elementPath') + + +def test_component_group_is_a_name_not_a_path(): + """The same holds for components inside a document, not only its metadata.""" + model, _ = get_model_and_model_api(MIRRORED_MODEL) + + documents = generate_multi_from_sgraph(model, level=3, transitive=True) + + for document in documents: + for component in document.get('components', []): + if component.get('group'): + assert '/' not in component['group'], component['group'] + + +def _estate(root_name): + """Two groups under one estate root, each holding a repository.""" + model = SGraph(SElement(None, '')) + for group_name in ('TeamA', 'TeamB'): + element = model.createOrGetElementFromPath(f'/{root_name}/{group_name}/service') + element.attrs['type'] = 'repository' + return model + + +def test_group_is_unchanged_by_an_estate_root_rename(): + """Renaming the estate must not rewrite the identity of every component in it. + + group is an identity field for consumers such as Dependency-Track. Carrying the estate + root inside it meant an organisational rename -- the least stable segment of the path -- + silently re-identified everything below it. + """ + before = generate_multi_from_sgraph(_estate('TalenomSoftware'), level=3, transitive=True) + after = generate_multi_from_sgraph(_estate('SaaS'), level=3, transitive=True) + + groups_before = sorted(d['metadata']['component'].get('group', '') for d in before) + groups_after = sorted(d['metadata']['component'].get('group', '') for d in after) + + assert groups_before == groups_after, ( + f'an estate rename changed every group value: {groups_before} -> {groups_after}') + + +def test_the_estate_root_is_still_recoverable_after_the_change(): + """Dropping the path from group must not lose it -- elementPath still carries it.""" + documents = generate_multi_from_sgraph(_estate('SaaS'), level=3, transitive=True) + + paths = [find_property(d['metadata']['component'], 'softagram:elementPath') + for d in documents] + assert all(p and p.startswith('/SaaS/') for p in paths), paths + + +def test_groups_sharing_a_name_under_different_roots_stay_distinguishable(): + """The cost of the change, made explicit and shown to be covered. + + A bare name is all that separates two same-named groups under different roots, so their + group values now collide where the full path kept them apart. That collision is accepted + deliberately: identity does not rest on group. elementPath and the document serial both + still separate them, and neither was affected by this change. + """ + model = SGraph(SElement(None, '')) + for root in ('RootX', 'RootY'): + element = model.createOrGetElementFromPath(f'/{root}/Shared/service') + element.attrs['type'] = 'repository' + + documents = generate_multi_from_sgraph(model, level=3, transitive=True) + components = [d['metadata']['component'] for d in documents] + + groups = {c.get('group') for c in components} + assert groups == {'Shared'}, f'expected the collision this change accepts, got {groups}' + + paths = {find_property(c, 'softagram:elementPath') for c in components} + assert paths == {'/RootX/Shared/service', '/RootY/Shared/service'}, paths + + serials = {d['serialNumber'] for d in documents} + assert len(serials) == len(documents), 'document serials must still be distinct'