API changes to support SPDX 3.0 #924

pragnya17 · 2025-02-07T18:54:04Z

Update the validation/parsing and generation workflows to reflect the different SPDX 3.0 defined entities.
Add a cmd line parameter for different compliance standards. For example, parse SbomFileExample.txt -complianceStandard NTIA
Make sure there are no duplicate spdx elements
Unit testing command line API changes, workflows, utility methods

github-actions · 2025-02-07T18:58:43Z

This PR changes files in the API project. Does it change any of the API interfaces in any way? Please note that this includes the following types of changes:

Changing the signature of an existing interface method
Adding a new method to an existing interface
Adding a required data member to a class that an existing interface method consumes

Because any of these changes can potentially break a downstream consumer with customized interface implementations, these changes need to be treated as breaking changes. Please do one of the following:

Option 1 - Publish this as a breaking change

Update the documentation to show the new functionality
Bump the major version in the next release
Be sure to highlight the breaking changes in the release notes

Option 2 - Refactor the changes to be non-breaking

Review this commit, which adds a new interface in a backward-compatible way
Refactor the change to follow this pattern so that existing interfaces are left completely intact
Bump the minor version in the next release

github-actions · 2025-02-07T19:00:01Z

This PR changes files in the API project. Does it change any of the API interfaces in any way? Please note that this includes the following types of changes:

Changing the signature of an existing interface method
Adding a new method to an existing interface
Adding a required data member to a class that an existing interface method consumes

Because any of these changes can potentially break a downstream consumer with customized interface implementations, these changes need to be treated as breaking changes. Please do one of the following:

Option 1 - Publish this as a breaking change

Update the documentation to show the new functionality
Bump the major version in the next release
Be sure to highlight the breaking changes in the release notes

Option 2 - Refactor the changes to be non-breaking

Review this commit, which adds a new interface in a backward-compatible way
Refactor the change to follow this pattern so that existing interfaces are left completely intact
Bump the minor version in the next release

github-actions · 2025-02-07T19:15:50Z

This PR changes files in the API project. Does it change any of the API interfaces in any way? Please note that this includes the following types of changes:

Changing the signature of an existing interface method
Adding a new method to an existing interface
Adding a required data member to a class that an existing interface method consumes

Because any of these changes can potentially break a downstream consumer with customized interface implementations, these changes need to be treated as breaking changes. Please do one of the following:

Option 1 - Publish this as a breaking change

Update the documentation to show the new functionality
Bump the major version in the next release
Be sure to highlight the breaking changes in the release notes

Option 2 - Refactor the changes to be non-breaking

Review this commit, which adds a new interface in a backward-compatible way
Refactor the change to follow this pattern so that existing interfaces are left completely intact
Bump the minor version in the next release

…s other breaking changes

github-actions · 2025-02-07T20:01:20Z

This PR changes files in the API project. Does it change any of the API interfaces in any way? Please note that this includes the following types of changes:

Changing the signature of an existing interface method
Adding a new method to an existing interface
Adding a required data member to a class that an existing interface method consumes

Because any of these changes can potentially break a downstream consumer with customized interface implementations, these changes need to be treated as breaking changes. Please do one of the following:

Option 1 - Publish this as a breaking change

Update the documentation to show the new functionality
Bump the major version in the next release
Be sure to highlight the breaking changes in the release notes

Option 2 - Refactor the changes to be non-breaking

Review this commit, which adds a new interface in a backward-compatible way
Refactor the change to follow this pattern so that existing interfaces are left completely intact
Bump the minor version in the next release

DaveTryon · 2025-02-07T23:23:36Z

src/Microsoft.Sbom.Api/Executors/ComponentDetectionBaseWalker.cs

-            var directory = Path.GetDirectoryName(spdxSbomConfig.ManifestJsonFilePath);
-            directory = fileSystemUtils.GetFullPath(directory);
-            if (!string.IsNullOrEmpty(directory))
+            if (sbomConfigs.TryGet(supportedSpdxManifest, out var spdxSbomConfig))


Presumably we can only match 1 config--should we break out of the loop after the first match?

I wasn't sure if the customer was allowed to pass in multiple manifests. Seems like the conclusion is that there is only matching manifest based on your comment and what I've seen in the rest of the code, so I'll add a break statement after a successful match

src/Microsoft.Sbom.Api/Executors/GenerateResult.cs

DaveTryon · 2025-02-07T23:32:29Z

src/Microsoft.Sbom.Api/Executors/IJsonSerializationStrategy.cs

+
+namespace Microsoft.Sbom.Api.Workflows.Helpers;
+
+public interface IJsonSerializationStrategy


I'm not seeing this interface in any test classes. Is that a gap we should address?

This is tested in the workflow tests where there are UTs to switch to different serialization strategies based on the manifest version. So I don't think that there's necessarily a gap here.

src/Microsoft.Sbom.Api/Executors/IJsonSerializationStrategy.cs

src/Microsoft.Sbom.Api/Executors/JsonSerializationStrategyFactory.cs

src/Microsoft.Sbom.Api/Executors/Spdx2SerializationStrategy.cs

src/Microsoft.Sbom.Api/Executors/Spdx3SerializationStrategy.cs

src/Microsoft.Sbom.Api/Output/MetadataBuilder.cs

DaveTryon · 2025-02-08T00:20:50Z

src/Microsoft.Sbom.Api/Workflows/Helpers/ExternalDocumentReferenceGenerator.cs

    private readonly IEnumerable<ISourcesProvider> sourcesProviders;

    private readonly IRecorder recorder;

+    public ISbomConfig SbomConfig { get; set; }


Can SbomConfig and SpdxManifestVersion be set more than once? Not for this PR, but a small follow-up PR could easily add a class that makes these write-once

No they should only be write-once, I will create a work item to make sure this change gets taken up in a separate PR.

src/Microsoft.Sbom.Api/Workflows/Helpers/ExternalDocumentReferenceGenerator.cs

src/Microsoft.Sbom.Parsers.Spdx30SbomParser/Parser/ContextsResult.cs

DaveTryon · 2025-02-08T01:32:02Z

src/Microsoft.Sbom.Parsers.Spdx30SbomParser/Parser/ContextsResult.cs

    }

-    public IEnumerable<string> Contexts => ((IEnumerable<string>)this.Result!).Select(r => r);
+    public IEnumerable<object> Contexts { get; set; }


object can literally be anything. Is there a way to improve the type safety?

DaveTryon · 2025-02-08T01:35:58Z

test/Microsoft.Sbom.Api.Tests/VersionSpecificPins/Version_3_0/InterfaceConcretionTests.cs

@@ -187,7 +187,11 @@ private class PinnedIFileTypeUtils : IFileTypeUtils

    private class PinnedIJsonArrayGenerator : IJsonArrayGenerator<PinnedIJsonArrayGenerator>


With these changes, we are intentionally doing away with the Version 3 interface, so this file should be in a different folder--calling it Version_4_0 would be consistent with the previous naming

pragnya17 · 2025-02-10T18:20:51Z

/azp run

src/Microsoft.Sbom.Parsers.Spdx30SbomParser/Generator.cs

src/Microsoft.Sbom.Parsers.Spdx30SbomParser/Parser/SPDX30Parser.cs

DaveTryon · 2025-02-10T19:39:52Z

src/Microsoft.Sbom.Parsers.Spdx30SbomParser/Parser/SPDX30Parser.cs

+    /// <exception cref="ParserException"></exception>
+    public void ValidateSbomDocCreationForNTIA(List<Element> elementsList)
+    {
+        var spdxDocumentElements = elementsList.Where(element => element is SpdxDocument);


Where returns an IEnumerable, which has some benefits, but the first thing that happens is to call Count(), which walks the entire list. Would the code to simpler if you made spdxDocumentElements a List by appending .ToList() on line 339?

src/Microsoft.Sbom.Parsers.Spdx30SbomParser/Parser/SPDX30Parser.cs

DaveTryon · 2025-02-10T19:49:15Z

src/Microsoft.Sbom.Parsers.Spdx30SbomParser/Parser/SPDX30Parser.cs

+
+        metadata.SpdxVersion = spdxCreationInfoElement?.SpecVersion;
+
+        return metadata;


This can set multiple properties of SpdxMetadata to null, when they're not declared as nullable in SpdxMetadata. .NET 8.0 assumes non-nullable types unless we explicitly tell it otherwise

If result has null values then SpdxMetadata should also have those values set to null, so this is intentional

src/Microsoft.Sbom.Parsers.Spdx30SbomParser/Utils/SPDXToSbomFormatConverterExtensions.cs

src/Microsoft.Sbom.Parsers.Spdx30SbomParser/Generator.cs

DaveTryon · 2025-02-10T20:04:32Z

test/Microsoft.Sbom.Api.Tests/SBOMGeneratorTest.cs

@@ -100,4 +102,25 @@ public async Task When_GenerateSbomAsync_WithNoRecordedErrors_Then_EmptyEntityEr
        mockRecorder.Verify();
        mockWorkflow.Verify();
    }
+


Code coverage for the generator hits most of the happy paths, but leaves a lot of error cases untouched. Should we try to cover more of the error paths?

test/Microsoft.Sbom.Api.Tests/Workflows/ManifestGenerationWorkflowTests.cs

test/Microsoft.Sbom.Api.Tests/Workflows/SbomParserBasedValidationWorkflowTests.cs

DaveTryon · 2025-02-10T20:20:58Z

test/Microsoft.Sbom.Api.Tests/Workflows/SbomParserBasedValidationWorkflowTests.cs

@@ -379,4 +464,143 @@ public async Task SbomParserBasedValidationWorkflowTests_ReturnsSuccessAndValida
        fileSystemMock.VerifyAll();
        osUtilsMock.VerifyAll();
    }
+
+    [TestMethod]
+    public async Task SbomParserBasedValidationWorkflowTests_SetsComplianceStandard_Succeeds()


Code smell: If we need to have > 100 lines of code to run a 2 line test, we probably have some fundamental problems with our interface story

Since I am setting the compliance standard in the workflow (when the parser gets created), all of these mocks are created in order to run the workflow which will set the compliance standard that I am asserting for.

Maybe I can create a work item to take this issue up in a separate PR? I think it would involve some extensive rewriting of interfaces

github-actions · 2025-02-10T23:10:57Z

This PR changes files in the API project. Does it change any of the API interfaces in any way? Please note that this includes the following types of changes:

Changing the signature of an existing interface method
Adding a new method to an existing interface
Adding a required data member to a class that an existing interface method consumes

Because any of these changes can potentially break a downstream consumer with customized interface implementations, these changes need to be treated as breaking changes. Please do one of the following:

Option 1 - Publish this as a breaking change

Update the documentation to show the new functionality
Bump the major version in the next release
Be sure to highlight the breaking changes in the release notes

Option 2 - Refactor the changes to be non-breaking

Review this commit, which adds a new interface in a backward-compatible way
Refactor the change to follow this pattern so that existing interfaces are left completely intact
Bump the minor version in the next release

pragnya17 · 2025-02-10T23:16:32Z

/azp run

github-actions · 2025-02-12T00:04:16Z

This PR changes files in the API project. Does it change any of the API interfaces in any way? Please note that this includes the following types of changes:

Changing the signature of an existing interface method
Adding a new method to an existing interface
Adding a required data member to a class that an existing interface method consumes

Because any of these changes can potentially break a downstream consumer with customized interface implementations, these changes need to be treated as breaking changes. Please do one of the following:

Option 1 - Publish this as a breaking change

Update the documentation to show the new functionality
Bump the major version in the next release
Be sure to highlight the breaking changes in the release notes

Option 2 - Refactor the changes to be non-breaking

Review this commit, which adds a new interface in a backward-compatible way
Refactor the change to follow this pattern so that existing interfaces are left completely intact
Bump the minor version in the next release

pragnya17 · 2025-02-12T16:49:57Z

/azp run

ppandrate added 7 commits January 22, 2025 16:32

update UT to use spdx 3.0 generator

e0d9061

generator api changes + restructuring classes

414a3fb

merge conflict

3e4fd88

wokring generator api changes with UT

bbdd751

working UT for compliance standard

9010c31

parsing deduplication

0c6c7a3

generator deduplication + add context

b5a943e

pragnya17 requested a review from a team as a code owner February 7, 2025 18:54

merge conflict

c462e85

remove from public docs until release

886040a

clean up changes + update interface pins

da1a30f

update spdxmetadata - including this breaking change since this PR ha…

06ef615

…s other breaking changes

DaveTryon reviewed Feb 7, 2025

View reviewed changes

src/Microsoft.Sbom.Api/Executors/GenerateResult.cs Outdated Show resolved Hide resolved

DaveTryon reviewed Feb 7, 2025

View reviewed changes

src/Microsoft.Sbom.Api/Executors/GenerateResult.cs Outdated Show resolved Hide resolved

DaveTryon reviewed Feb 7, 2025

View reviewed changes

src/Microsoft.Sbom.Api/Executors/IJsonSerializationStrategy.cs Show resolved Hide resolved

DaveTryon reviewed Feb 7, 2025

View reviewed changes

src/Microsoft.Sbom.Api/Executors/JsonSerializationStrategyFactory.cs Outdated Show resolved Hide resolved

DaveTryon reviewed Feb 7, 2025

View reviewed changes

src/Microsoft.Sbom.Api/Executors/JsonSerializationStrategyFactory.cs Show resolved Hide resolved

DaveTryon reviewed Feb 7, 2025

View reviewed changes

src/Microsoft.Sbom.Api/Executors/Spdx2SerializationStrategy.cs Show resolved Hide resolved

DaveTryon reviewed Feb 8, 2025

View reviewed changes

src/Microsoft.Sbom.Api/Executors/Spdx3SerializationStrategy.cs Outdated Show resolved Hide resolved

DaveTryon reviewed Feb 8, 2025

View reviewed changes

src/Microsoft.Sbom.Api/Executors/Spdx3SerializationStrategy.cs Outdated Show resolved Hide resolved

DaveTryon reviewed Feb 8, 2025

View reviewed changes

src/Microsoft.Sbom.Api/Executors/Spdx3SerializationStrategy.cs Show resolved Hide resolved

DaveTryon reviewed Feb 8, 2025

View reviewed changes

src/Microsoft.Sbom.Api/Output/MetadataBuilder.cs Show resolved Hide resolved

DaveTryon reviewed Feb 8, 2025

View reviewed changes

src/Microsoft.Sbom.Api/Workflows/Helpers/ExternalDocumentReferenceGenerator.cs Show resolved Hide resolved

DaveTryon reviewed Feb 8, 2025

View reviewed changes

src/Microsoft.Sbom.Parsers.Spdx30SbomParser/Parser/ContextsResult.cs Show resolved Hide resolved

DaveTryon reviewed Feb 8, 2025

View reviewed changes