Include BomRef within Component hash calculation#678
Include BomRef within Component hash calculation#678wkoot wants to merge 2 commits intoCycloneDX:mainfrom
Conversation
|
this is considered a breaking change, as it changes existing behavior in a non-backwards-compatible way |
tests/test_real_world_examples.py
Outdated
| json = json_loads(input_json.read()) | ||
|
|
||
| bom = Bom.from_json(json) | ||
| self.assertEqual(4, len(bom.components)) |
There was a problem hiding this comment.
this is a disputable assertion.
the two of the components of the original BOM were duplicated in the first place, why keep them?
There was a problem hiding this comment.
What is the correct place for this to be disputed, is it part of the implementation or the spec discussion?
There was a problem hiding this comment.
it is something that could be discussed here: https://github.com/CycloneDX/specification/discussions
There was a problem hiding this comment.
Where in the spec is it explained what the uniqueness criteria for components are?
And is it a bug that the libraries in other programming languages have implemented this differently, or is that also something that's an ongoing discussion?
|
this PR claims to solve #677 none of the added tests showcase this broken/fixed behaviour. |
|
this PR claims to solve #540 the original problem was none of the added tests showcase this broken/fixed behaviour. |
wkoot
force-pushed
the
wkoot-patch-1
branch
3 times, most recently
from
1caaaf5 to
229585e
Compare
Fixes CycloneDX#540 Signed-off-by: wkoot <3715211+wkoot@users.noreply.github.com>
Addresses CycloneDX#677 Signed-off-by: wkoot <3715211+wkoot@users.noreply.github.com>
wkoot
force-pushed
the
wkoot-patch-1
branch
from
229585e to
efcca53
Compare
| self.description, self.scope, tuple(self.hashes), | ||
| tuple(self.licenses), self.copyright, self.cpe, | ||
| self.purl, | ||
| self.purl, self.bom_ref.value, |
There was a problem hiding this comment.
I see why you go with self.bom_ref.value instead of self.bom_ref.
BomRef.__hash__ exists and is implemented to account for the __id__ of the "empty" object entity, so that None values are unequal.
Your implementation does not do so, and may cause unexpected behaviour here.
There was a problem hiding this comment.
Indeed I had originally only added self.bom_ref, but that would then not deduplicate the Nones. Is there a reason for the BomRef hash to include the id? It seems superfluous when there is a unique identifier. Note also that the code claims a random UUIDv4 should have been assigned when there is no ref, but this is not actually the case
Would it be desirable to only include the |
breaking changes are embraced and not a stopper. it was just a remark. |
|
@wkoot , I just wanted to ask how things are going along. |
|
@jkowalleck sadly, I currently don't have any more time allotted for sbom related stuff :( |
|
superseded by #754 |
For some this is considered a bug-fix, for others this is a feature - it is a breaking change anyway since it modifies the order of things. ---- TODO: - [x] **every** symbol that has a property `bom-ref` MUST utilize it for dunder methods `hash`,`eq`,`gt`,`lt`,... - [x] add new test cases from #753 - [x] add new test cases from #540 - [x] add new test cases from #677 - [x] create new tests snapshots (if applicable) ---- > [!important] > depends on #755 supersedes #678 closes #678 fixes #753 fixes #540 fixes #677 --------- Signed-off-by: wkoot <3715211+wkoot@users.noreply.github.com> Signed-off-by: Jan Kowalleck <jan.kowalleck@gmail.com> Co-authored-by: wkoot <3715211+wkoot@users.noreply.github.com>
|
superseded by #754 |
2 participants
Closes #753
Fixes #540
Fixes #677