Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add spec version to bom #767

Merged
merged 3 commits into from
Nov 6, 2024
Merged

Conversation

m-brophy
Copy link
Contributor

@m-brophy m-brophy commented Nov 6, 2024

When trying to load CycloneDX files in rust using this library, calling parse_from_json (because we might get files of any spec version input) correctly detects the spec version and parses the json file.

However to validate the parsed bom returned, the validate method always validates against the default, v1.3. That means I have to call validate_version and pass in the spec version of the file but there's no method to retrieve that from the parsed bom.

This PR adds a spec_version field to the bom model so that the spec_version can be retrieved from the parsed bom and passed into validate_version

as discussed on slack with @justahero

Signed-off-by: m-brophy <mbrophy@redhat.com>
Signed-off-by: m-brophy <mbrophy@redhat.com>
@m-brophy m-brophy requested a review from a team as a code owner November 6, 2024 15:20
@Shnatsel
Copy link
Contributor

Shnatsel commented Nov 6, 2024

Why not make validate method also pick the correct version automatically?

@Shnatsel
Copy link
Contributor

Shnatsel commented Nov 6, 2024

Note: this is a semver-breaking change since it adds a new public field to a struct without #[non_exhaustive]. We'll need to bump the minor version of this crate.

@m-brophy
Copy link
Contributor Author

m-brophy commented Nov 6, 2024

Why not make validate method also pick the correct version automatically?

To do this I think I'd have to remove the implementation of the method from the Validate trait and implement validate() in Validate for Bom to get access to the bom fields. The drawback is I'd have to code it in the other 100 'Validateimplementations also. Is there a way to get access toself.spec_version` in the trait?

Maybe I can just override the code in the trait by providing a new method implementation in the Validate for Bom impl?

@Shnatsel
Copy link
Contributor

Shnatsel commented Nov 6, 2024

I'm not very familiar with this part of the codebase, and I didn't have to use it directly much. So I'm happy to defer to @justahero 's judgment on the code review, and accept whatever exposes the most easy-to-use API for people actually using this crate.

…ec_version field

Signed-off-by: m-brophy <mbrophy@redhat.com>
@justahero
Copy link
Contributor

Hello @m-brophy ,

thanks for opening the PR. The addition of the spec_version field makes the version available in the Bom model, but as @Shnatsel mentioned it's not used.

My suggestion is to override the validate method in the impl Validate for Bom block.

impl Validate for Bom {
    fn validate(&self) -> ValidationResult {
        self.validate_version(self.spec_version)
    }

    fn validate_version(&self, version: SpecVersion) -> ValidationResult { ... }
}

This way, when calling bom.validate() the SpecVersion of the Bom is passed instead of the default one. This applies to the Bom only, but I think that's fair, because it's the top level element and its version is propagated to all its children fields.

@m-brophy
Copy link
Contributor Author

m-brophy commented Nov 6, 2024

Thanks, I've done that now

@justahero
Copy link
Contributor

@m-brophy , nice you were faster than me. 👍

Yes, that makes use of the spec version in the validate method then.

@Shnatsel
Copy link
Contributor

Shnatsel commented Nov 6, 2024

We could probably make a serialization function that respects the BOM version field as well, but that can be added later in a semver-compatible way. So I'm OK with publishing a new version as-is. @justahero any objections to tagging v0.8.0 and publishing to crates.io?

@Shnatsel Shnatsel merged commit c64ed88 into CycloneDX:main Nov 6, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants