-
Notifications
You must be signed in to change notification settings - Fork 59
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
fix: properly output and read the filesAnalyzed field in JSON/YAML #210
Conversation
Signed-off-by: Keith Zantow <kzantow@gmail.com>
@@ -71,6 +71,11 @@ func TestLoad(t *testing.T) { | |||
func Test_Write(t *testing.T) { | |||
want := example.Copy() | |||
|
|||
// we always output FilesAnalyzed, even though we handle reading files where it is omitted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the weird part of filesAnalyzed is that if it is omitted technically the correct value is "true" but really no one does it this way... which is kind of cringe, because the intention is usually filesAnalyzed=false
... if we actually did a validation check when reading, likely >90% of SPDX documents will fail since they assume omission is default false based on the programming language defaults, and thus they don't include VerificationCode
which technically is required if filesAnalyzed=true
.
I see trade offs both ways, but since we do require user to signify their intention and the use of the library is not asking users to provide a mandatory field, a golang user will think that it defaults to false. My thought is to perhaps maybe flip the IsFilesAnalyzedTagPresent
to IsFilesAnalyzedTagOmitted
, or make the default value to true... i don't quite want to break the interface. This may create a experience a bit more aligned with golang
This however, doesn't fix the issue of all the input SBOMs having their filesAnalyzed intent misinterpreted, which technically isn't an issue for the library... but i think it will break enough of the ecosystem that we have to consider it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i thought about it a bit more... maybe what we can do is that during reading, is that if we see no VerificationCode
we can set filesAnalyzed=false
.. wdyt @kzantow ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the more i look into this, the worse it gets... since filesAnalyzed=false
is also an indication that a package doesn't contain any files, this messes things up..
I think my new thoughts around this is to just output it IsFilesAnalyzedTagPresent=true
, and otherwise omit it and not assert anything about its intended value but to parse it as is.. We think we will need to support this bad behavior as accounting for a deficiency of the spec.
Just FYI - I check the Java tools to see if it handles If you call the Note that this will not cause any parsing error - it only reports the issue in the Note - there was an issue in the Java library which may have masked the verification failures for some deeply nested dependencies - this has been fixed in the latest release. |
After discussion with @lumjjb we've decided to merge this PR and look at removing |
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [github.com/BurntSushi/toml](https://github.com/BurntSushi/toml) | require | minor | `v1.2.1` -> `v1.3.0` | | [github.com/go-git/go-git/v5](https://github.com/go-git/go-git) | require | minor | `v5.6.1` -> `v5.7.0` | | [github.com/spdx/tools-golang](https://github.com/spdx/tools-golang) | require | patch | `v0.5.0` -> `v0.5.1` | | [github.com/urfave/cli/v2](https://github.com/urfave/cli) | require | patch | `v2.25.3` -> `v2.25.5` | | golang.org/x/exp | require | digest | `dd950f8` -> `2e198f4` | | golang.org/x/tools | require | patch | `v0.9.1` -> `v0.9.3` | --- ### Release Notes <details> <summary>BurntSushi/toml</summary> ### [`v1.3.0`](https://github.com/BurntSushi/toml/releases/tag/v1.3.0) [Compare Source](https://github.com/BurntSushi/toml/compare/v1.2.1...v1.3.0) New features: - Support upcoming TOML 1.1 While it looks like TOML 1.1 is mostly stable and I don't expect any further major changes, there are *NO* compatibility guarantees as it is *NOT* yet released and *anything can still change*. To use it, set the `BURNTSUSHI_TOML_110` environment variable to any value, which can be done either with `os.SetEnv()` or by the user running a program. A full list is changes is available in the [TOML ChangeLog]; the two most notable ones are that newlines and trailing commas are now allowed in inline tables, and Unicode in bare keys can now be used – this is now a valid document: lëttërs = { ä = "a with diaeresis", è = "e with accent grave", } [TOML ChangeLog]: https://github.com/toml-lang/toml/blob/main/CHANGELOG.md - Allow MarshalTOML and MarshalText to be used on the document type itself, instead of only fields ([#​383](https://github.com/BurntSushi/toml/issues/383)). Bufixes: - `\` escapes at the end of line weren't processed correctly in multiline strings ([#​372](https://github.com/BurntSushi/toml/issues/372)). - Read over UTF-8 BOM ([#​381](https://github.com/BurntSushi/toml/issues/381)). - `omitempty` struct tag did not work for pointer values ([#​371](https://github.com/BurntSushi/toml/issues/371)). - Fix encoding anonymous structs on 32bit systems ([#​374](https://github.com/BurntSushi/toml/issues/374)). </details> <details> <summary>go-git/go-git</summary> ### [`v5.7.0`](https://github.com/go-git/go-git/releases/tag/v5.7.0) [Compare Source](https://github.com/go-git/go-git/compare/v5.6.1...v5.7.0) #### What's Changed - \*: Add support for initializing SHA256 repositories by [@​pjbgf](https://github.com/pjbgf) in [https://github.com/go-git/go-git/pull/707](https://github.com/go-git/go-git/pull/707) - git: add mirror clone option by [@​aymanbagabas](https://github.com/aymanbagabas) in [https://github.com/go-git/go-git/pull/735](https://github.com/go-git/go-git/pull/735) - git: Add support to ls-remote with peeled references. Fixes [#​749](https://github.com/go-git/go-git/issues/749) by [@​pjbgf](https://github.com/pjbgf) in [https://github.com/go-git/go-git/pull/750](https://github.com/go-git/go-git/pull/750) - git: fix cloning with branch name by [@​AriehSchneier](https://github.com/AriehSchneier) in [https://github.com/go-git/go-git/pull/755](https://github.com/go-git/go-git/pull/755) - git: Worktree, add check to see if file already checked in. Fixes [#​718](https://github.com/go-git/go-git/issues/718) by [@​cbbm142](https://github.com/cbbm142) in [https://github.com/go-git/go-git/pull/719](https://github.com/go-git/go-git/pull/719) - git: Worktree, git grep bare repositories by [@​aymanbagabas](https://github.com/aymanbagabas) in [https://github.com/go-git/go-git/pull/728](https://github.com/go-git/go-git/pull/728) - git: Add Depth to SubmoduleUpdateOptions by [@​matejrisek](https://github.com/matejrisek) in [https://github.com/go-git/go-git/pull/754](https://github.com/go-git/go-git/pull/754) - git: Testing, Fix tests not cleaning temp folders by [@​AriehSchneier](https://github.com/AriehSchneier) in [https://github.com/go-git/go-git/pull/769](https://github.com/go-git/go-git/pull/769) - git: remote, add support for a configurable timeout. by [@​andrewpollock](https://github.com/andrewpollock) in [https://github.com/go-git/go-git/pull/753](https://github.com/go-git/go-git/pull/753) - git: Allow Initial Branch to be configurable by [@​techknowlogick](https://github.com/techknowlogick) in [https://github.com/go-git/go-git/pull/764](https://github.com/go-git/go-git/pull/764) - storage: filesystem/dotgit, Improve load packed-refs by [@​fcharlie](https://github.com/fcharlie) in [https://github.com/go-git/go-git/pull/743](https://github.com/go-git/go-git/pull/743) - storage: filesystem, Populate index before use. Fixes [#​148](https://github.com/go-git/go-git/issues/148) by [@​AriehSchneier](https://github.com/AriehSchneier) in [https://github.com/go-git/go-git/pull/722](https://github.com/go-git/go-git/pull/722) - plumbing: resolve non-external delta references by [@​ZauberNerd](https://github.com/ZauberNerd) in [https://github.com/go-git/go-git/pull/485](https://github.com/go-git/go-git/pull/485) - plumbing/transport: fix regression in scp-like match by [@​jotadrilo](https://github.com/jotadrilo) in [https://github.com/go-git/go-git/pull/715](https://github.com/go-git/go-git/pull/715) - plumbing/transport: Add support for custom proxy settings by [@​aryan9600](https://github.com/aryan9600) in [https://github.com/go-git/go-git/pull/744](https://github.com/go-git/go-git/pull/744) - \*: small fixes across the codebase by [@​pjbgf](https://github.com/pjbgf) in [https://github.com/go-git/go-git/pull/770](https://github.com/go-git/go-git/pull/770) - \*: bump github.com/cloudflare/circl from 1.1.0 to 1.3.3 by [@​dependabot](https://github.com/dependabot) in [https://github.com/go-git/go-git/pull/776](https://github.com/go-git/go-git/pull/776) - \*: bump dependencies by [@​pjbgf](https://github.com/pjbgf) in [https://github.com/go-git/go-git/pull/748](https://github.com/go-git/go-git/pull/748) - \*: bump Go version to 1.18 on go.mod by [@​pjbgf](https://github.com/pjbgf) in [https://github.com/go-git/go-git/pull/774](https://github.com/go-git/go-git/pull/774) - \*: add Codeql workflow and bump dependencies by [@​pjbgf](https://github.com/pjbgf) in [https://github.com/go-git/go-git/pull/775](https://github.com/go-git/go-git/pull/775) - ci: fix upstream git build for master branch by [@​pjbgf](https://github.com/pjbgf) in [https://github.com/go-git/go-git/pull/739](https://github.com/go-git/go-git/pull/739) #### New Contributors - [@​ZauberNerd](https://github.com/ZauberNerd) made their first contribution in [https://github.com/go-git/go-git/pull/485](https://github.com/go-git/go-git/pull/485) - [@​jotadrilo](https://github.com/jotadrilo) made their first contribution in [https://github.com/go-git/go-git/pull/715](https://github.com/go-git/go-git/pull/715) - [@​fcharlie](https://github.com/fcharlie) made their first contribution in [https://github.com/go-git/go-git/pull/743](https://github.com/go-git/go-git/pull/743) - [@​AriehSchneier](https://github.com/AriehSchneier) made their first contribution in [https://github.com/go-git/go-git/pull/755](https://github.com/go-git/go-git/pull/755) - [@​cbbm142](https://github.com/cbbm142) made their first contribution in [https://github.com/go-git/go-git/pull/719](https://github.com/go-git/go-git/pull/719) - [@​aryan9600](https://github.com/aryan9600) made their first contribution in [https://github.com/go-git/go-git/pull/744](https://github.com/go-git/go-git/pull/744) - [@​matejrisek](https://github.com/matejrisek) made their first contribution in [https://github.com/go-git/go-git/pull/754](https://github.com/go-git/go-git/pull/754) - [@​andrewpollock](https://github.com/andrewpollock) made their first contribution in [https://github.com/go-git/go-git/pull/753](https://github.com/go-git/go-git/pull/753) - [@​techknowlogick](https://github.com/techknowlogick) made their first contribution in [https://github.com/go-git/go-git/pull/764](https://github.com/go-git/go-git/pull/764) **Full Changelog**: go-git/go-git@v5.6.1...v5.7.0 </details> <details> <summary>spdx/tools-golang</summary> ### [`v0.5.1`](https://github.com/spdx/tools-golang/releases/tag/v0.5.1) [Compare Source](https://github.com/spdx/tools-golang/compare/v0.5.0...v0.5.1) #### What's Changed - Add ability to specify JSON output options by [@​DmitriyLewen](https://github.com/DmitriyLewen) in [https://github.com/spdx/tools-golang/pull/213](https://github.com/spdx/tools-golang/pull/213) - Fix some optional params: `copyrightText`, `licenseListVersion`, `packageVerificationCode` by [@​lumjjb](https://github.com/lumjjb) in [https://github.com/spdx/tools-golang/pull/215](https://github.com/spdx/tools-golang/pull/215) - Properly output and read the `filesAnalyzed` field in JSON/YAML by [@​kzantow](https://github.com/kzantow) in [https://github.com/spdx/tools-golang/pull/210](https://github.com/spdx/tools-golang/pull/210) - Ensure no duplicates in relationships when shortcut fields are used. by [@​lumjjb](https://github.com/lumjjb) in [https://github.com/spdx/tools-golang/pull/218](https://github.com/spdx/tools-golang/pull/218) #### New Contributors - [@​testwill](https://github.com/testwill) made their first contribution in [https://github.com/spdx/tools-golang/pull/212](https://github.com/spdx/tools-golang/pull/212) - [@​DmitriyLewen](https://github.com/DmitriyLewen) made their first contribution in [https://github.com/spdx/tools-golang/pull/213](https://github.com/spdx/tools-golang/pull/213) **Full Changelog**: spdx/tools-golang@v0.5.0...v0.5.1 </details> <details> <summary>urfave/cli</summary> ### [`v2.25.5`](https://github.com/urfave/cli/releases/tag/v2.25.5) [Compare Source](https://github.com/urfave/cli/compare/v2.25.4...v2.25.5) #### What's Changed - Fix:(issue\_1737) Set bool count by taking care of num of aliases by [@​dearchap](https://github.com/dearchap) in [https://github.com/urfave/cli/pull/1740](https://github.com/urfave/cli/pull/1740) **Full Changelog**: urfave/cli@v2.25.4...v2.25.5 ### [`v2.25.4`](https://github.com/urfave/cli/releases/tag/v2.25.4) [Compare Source](https://github.com/urfave/cli/compare/v2.25.3...v2.25.4) #### What's Changed - Bug/fix issue 1703 by [@​jojje](https://github.com/jojje) in [https://github.com/urfave/cli/pull/1728](https://github.com/urfave/cli/pull/1728) - Fix:(issue\_1734) Show categories for subcommands by [@​dearchap](https://github.com/dearchap) in [https://github.com/urfave/cli/pull/1735](https://github.com/urfave/cli/pull/1735) - Fix:(issue\_1610). Keep RunAsSubcommand behaviour as before by [@​dearchap](https://github.com/dearchap) in [https://github.com/urfave/cli/pull/1736](https://github.com/urfave/cli/pull/1736) - Fix:(issue\_1731) Add fix for checking if aliases are set by [@​dearchap](https://github.com/dearchap) in [https://github.com/urfave/cli/pull/1732](https://github.com/urfave/cli/pull/1732) - Fix func name referenced in doc comment by [@​meatballhat](https://github.com/meatballhat) in [https://github.com/urfave/cli/pull/1738](https://github.com/urfave/cli/pull/1738) #### New Contributors - [@​jojje](https://github.com/jojje) made their first contribution in [https://github.com/urfave/cli/pull/1728](https://github.com/urfave/cli/pull/1728) **Full Changelog**: urfave/cli@v2.25.3...v2.25.4 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "before 6am on monday" in timezone Australia/Sydney, Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/google/osv-scanner). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS40OC4yIiwidXBkYXRlZEluVmVyIjoiMzUuMTAyLjEwIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->
The Package
FilesAnalyzed
field has somewhat backwards behavior from standard Go JSON -- if it's omitted, the value istrue
. It had been set asomitempty
, which means it was omitted if false (the default for a bool value). This resulted in an impossibility of setting it tofalse
, since it would be omitted from the JSON, which then should be interpreted astrue
. This PR adjust the behavior so the field is always output, and handles interpreting missing values astrue
.Fixes #209