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

Don't crash on duplicated filetypes #630

Closed
bmihaila-bd opened this issue May 4, 2023 · 7 comments
Closed

Don't crash on duplicated filetypes #630

bmihaila-bd opened this issue May 4, 2023 · 7 comments

Comments

@bmihaila-bd
Copy link

Some SBOM creation tools create fileTypes entries which might contain duplicates, e.g.

"fileTypes": [
                "SOURCE",
                "TEXT",
                "SOURCE",
            ],

The MR #310 added code for supporting multiple fileTypes but the code is too strict in that it does not allow duplicates. It throws a raise CardinalityError("File::FileType") in case of dupes. The spec does not forbid this https://spdx.github.io/spdx-spec/v2.2.2/file-information/#83-file-type-field so the parser should not be that strict and crash on such files. I see a similar behavior for licenseInfoInFiles which do not raise an exception on duplicates.

@meretp
Copy link
Collaborator

meretp commented May 4, 2023

Thanks for opening this issue. In the meanwhile we merged a refactored version of the codebase to main but I assume that you are refering to the current release v0.7.1?

@bmihaila-bd
Copy link
Author

yes, using the latest pypi version of 0.7.1

@meretp
Copy link
Collaborator

meretp commented May 4, 2023

Alright and yes, your argumentation sounds conclusive, the parser should probably not be that strict.

As mentioned above we did a major refactoring of the codebase. The refactored version allows duplicated fileTypes, it is currently available as an alpha release on PyPi. Although you would need to do a bit of work on the migration (you can find a guide on that here) I would recommend to use that version.
The version from v0.7.1 will only receive bug fixes and no new features but I think we could add an easy fix for this one and delete the check for cardinality. Contributions welcome! :)

@bmihaila-bd
Copy link
Author

Thanks. Will try the alpha, was not aware of that. Regarding the fix. My oneliner fix was to just not do that check and raise the Cardinality error, i.e. just remove that code. Depending on how much you want to "fix" somewhat broken SBOMs you could just use a set instead of a list and hence remove duplicates but as you mention, that old code branch is bugifx only so I guess just skipping the raise is the simpler fix.

@bmihaila-bd
Copy link
Author

btw, the link in the README towards the migration is broken https://github.com/spdx/tools-python/wiki/How-to-migrate-from-0.7-to-1.0

@meretp
Copy link
Collaborator

meretp commented May 4, 2023

Thanks. Will try the alpha, was not aware of that. Regarding the fix. My oneliner fix was to just not do that check and raise the Cardinality error, i.e. just remove that code. Depending on how much you want to "fix" somewhat broken SBOMs you could just use a set instead of a list and hence remove duplicates but as you mention, that old code branch is bugifx only so I guess just skipping the raise is the simpler fix.

Yes, I agree on that, simply skip should be sufficient.

btw, the link in the README towards the migration is broken https://github.com/spdx/tools-python/wiki/How-to-migrate-from-0.7-to-1.0

Yeah I also noticed that and opened a PR #631 for that.

@armintaenzertng
Copy link
Collaborator

fixed by #671

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

No branches or pull requests

3 participants