Bug fixes by tamari-oz · Pull Request #207 · spdx/tools-python

tamari-oz · 2022-01-16T10:01:58Z

4 fixes are addressed:

Relationship generation on non-TV output formats (was working only on TV).
Missing encoding when dumping to YAML.
Fixed Validation error not returning in relationship.
NoAssert does not contain default repr

The qualifiers where not correct Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com> Signed-off-by: OzTamari <oz.tamari@whitesourcesoftware.com>

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com> Signed-off-by: OzTamari <oz.tamari@whitesourcesoftware.com>

Reported-by: Rich Steenwyk <rich.steenwyk@gmail.com> Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com> Signed-off-by: OzTamari <oz.tamari@whitesourcesoftware.com>

1. yaml creation missing encoding. 2. NoAssert cannot be written to a file (not serializable) as it does not have default repr. 3. Problem creating relationships Signed-off-by: OzTamari <oz.tamari@whitesourcesoftware.com>

Signed-off-by: OzTamari <oz.tamari@whitesourcesoftware.com>

1. yaml creation missing encoding. 2. NoAssert cannot be written to a file (not serializable) as it does not have default repr. 3. Problem creating relationships Signed-off-by: OzTamari <oz.tamari@whitesourcesoftware.com>

Signed-off-by: OzTamari <oz.tamari@whitesourcesoftware.com>

…ccording to SPDX 2.2 spec 7.9.1 (only for JSON, XML, YAML as it does not happen on TV)

licquia

I don't think we're going to add a dependency on numpy for all users of the library. Also, could you add tests for the specific deficiencies you fixed?

licquia · 2022-03-15T00:41:31Z

 from spdx.writers.tagvalue import InvalidDocumentError
 from spdx.writers.jsonyamlxml import Writer
 from spdx.parsers.loggers import ErrorMessages
+import numpy as np


Can you remove the dependency on numpy? You should be able to test for regular int, float, etc.

Signed-off-by: rammatzkvosky <ram.matzkovsky@whitesourcesoftware.com>

…ue is not available at WhiteSource Updating setup.py version to '0.7.0a3.post6' Signed-off-by: rammatzkvosky <ram.matzkovsky@whitesourcesoftware.com>

nicoweidner

There are several valuable fixes in this PR, but several other things need more work.

@tamari-oz: Given the age of this PR, it would be greatly appreciated if you could indicate whether you are still active and will have time to finish the PR. Else I would try to take over so we can get the fixes merged.

nicoweidner · 2022-09-06T12:09:09Z

+    name='ws-spdx-tools',
+    version='0.7.0a3.post7',


I assume these changes are from the fork and shouldn't be committed?

nicoweidner · 2022-09-06T12:40:35Z

+                    messages.append(
+                        "Package checksum must be instance of spdx.checksum.Algorithm"
+                    )
+                # this check was added by the WhiteSouse PS Team ( for cases where SHA1 is not available at WhiteSource )


I haven't seen such attribution comments elsewhere in the project, and I don't think they are viable. If there are many contributors, it would lead to a lot of clutter. I would be grateful if you remove them here and in other locations where they were added

Hi @nicoweidner - will do

Hi @rammatzkvosky ! Since we didn't hear back for a while, @meretp took over this PR and put up a new version here. Hope that is alright!

nicoweidner · 2022-09-06T12:42:43Z

+        except:
+            pass


Just swallowing all exceptions is bad. Why was this added?

nicoweidner · 2022-09-06T13:09:50Z

+                if not self.check_sum.value:
+                    delattr(self, 'check_sum')


I don't understand what this accomplishes. If a checksum object without a value is present, it gets removed - does some downstream check depend on this?
Also, if the instance check above fails, we are still adding a validation error message. Is that intended?

Sorry if I am being slow here, I just started working on this project 😅

@nicoweidner -In our platform, there are cases where a package sha1 is missing ( empty value ).
For tv format, there is a downstream check for that which fails the report.

For that reason, we add this check.

nicoweidner · 2022-09-06T13:25:28Z

        self.relationship_comment = relationship_comment

+    def __eq__(self, other):
+        return True if self.relationship == other.relationship else False


This should include an instance check on the other object:

Suggested change

return True if self.relationship == other.relationship else False

return (

isInstance(other, Relationship)

and self.relationship == other.relationship

)

nicoweidner · 2022-09-07T13:18:29Z

+        unique_doc_packages = {}
+        for doc_package in self.document.packages:
+            if doc_package.spdx_id not in unique_doc_packages.keys():
+                unique_doc_packages[doc_package.spdx_id] = doc_package


Similar to the extracted licenses: Fine for now, but eventually I would move this to validations

nicoweidner · 2022-09-07T13:22:18Z

+        # commented by the WS PS Team
+        # package_objects = []
+        # for package in self.document.packages:
+        #     package_info_object = self.create_package_info(package)
+        #     package_info_object["files"] = self.create_file_info(package)
+        #     package_objects.append({"Package": package_info_object})
+        #
+        # self.document_object["documentDescribes"] = package_objects
+
+        # the previous code block has been adjusted by the WhiteSouse PS Team.
+        # adding documentDescribes and packages sections.


Please remove the comments. It will be in the git history anyway, and the comments will just clutter the code

nicoweidner · 2022-09-07T13:57:04Z

-            package_objects.append({"Package": package_info_object})
+            if package.files:
+                package_info_object["files"] = self.create_file_info(package)
+            # create a list of the packages' names for the 'documentDescribes' field


Suggested change

# create a list of the packages' names for the 'documentDescribes' field

# create a list of the packages' spdx ids for the 'documentDescribes' field

nicoweidner · 2022-09-07T15:02:34Z

        Return a list of relationship nodes
        """
-        return map(self.create_relationship_node, self.document.relationships)
+        return list(map(self.create_relationship_node, self.document.relationships))


I don't think it makes a difference here whether we return an iterable or a list. The doc says it returns a list, but then again all other similar functions return an iterable as well, so I'm inclined to leave it that way (or fix all the docs)

nicoweidner · 2022-09-07T15:06:17Z

+        # Add relationship
+        for relate_node in self.relationships():
+            relate_triple = (doc_node, self.spdx_namespace.relationship, relate_node)
+            self.graph.add(relate_triple)


I tried this out using

$ convertor -f tag data/SPDXTagExample -o output.rdf

and the output contained this, which doesn't seem to make sense (relationships should be separate entities):

<ns1:relationship> <ns1:Relationship rdf:about="SPDXRef-DOCUMENT"> <ns1:relationshipType>CONTAINS</ns1:relationshipType> <ns1:relationshipType>DESCRIBES</ns1:relationshipType> <ns1:relationshipType>COPY_OF</ns1:relationshipType> <ns1:relatedSpdxElement>SPDXRef-File</ns1:relatedSpdxElement> <ns1:relatedSpdxElement>DocumentRef-spdx-tool-1.2:SPDXRef-ToolsElement</ns1:relatedSpdxElement> <ns1:relatedSpdxElement>SPDXRef-Package</ns1:relatedSpdxElement> </ns1:Relationship> </ns1:relationship>

I think this either needs more work, or should be handled as a separate issue - whichever way you prefer!

armintaenzertng · 2022-10-21T12:41:37Z

@tamari-oz, do you plan to get back to this?

nicoweidner · 2022-11-02T16:34:33Z

I will continue this in #263, which I already rebased on main

tamari-oz mentioned this pull request Jan 16, 2022

Fix relationships on non TV output formats #206

Closed

pombredanne and others added 8 commits January 16, 2022 12:07

Bump version and qualifiers

277a517

The qualifiers where not correct Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com> Signed-off-by: OzTamari <oz.tamari@whitesourcesoftware.com>

Do not build for Python 2 and bump version

bccf672

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com> Signed-off-by: OzTamari <oz.tamari@whitesourcesoftware.com>

Fix minor typo spdx#190

597978e

Reported-by: Rich Steenwyk <rich.steenwyk@gmail.com> Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com> Signed-off-by: OzTamari <oz.tamari@whitesourcesoftware.com>

Fixing 3 issues that hinder the tool:

5986c91

1. yaml creation missing encoding. 2. NoAssert cannot be written to a file (not serializable) as it does not have default repr. 3. Problem creating relationships Signed-off-by: OzTamari <oz.tamari@whitesourcesoftware.com>

Adjust fix in NoAsset to pass test

0f615ee

Signed-off-by: OzTamari <oz.tamari@whitesourcesoftware.com>

Re-enabled relationships on non-TV output formats.

cbae4c0

Signed-off-by: OzTamari <oz.tamari@whitesourcesoftware.com>

Fixing 3 issues that hinder the tool:

e6122e2

1. yaml creation missing encoding. 2. NoAssert cannot be written to a file (not serializable) as it does not have default repr. 3. Problem creating relationships Signed-off-by: OzTamari <oz.tamari@whitesourcesoftware.com>

Adjust fix in NoAsset to pass test

716905b

Signed-off-by: OzTamari <oz.tamari@whitesourcesoftware.com>

tamari-oz force-pushed the ws-patches branch from 902fb18 to 716905b Compare January 16, 2022 10:07

tamari-oz and others added 5 commits February 7, 2022 11:50

Adding 'PackageVerificationCode' only if 'FilesAnalyzed' is True as a…

3395930

…ccording to SPDX 2.2 spec 7.9.1 (only for JSON, XML, YAML as it does not happen on TV)

make SBOM aligned with SPDX Tool Validator

fe40893

adding filter licenses and json converter

5fc3694

update version

a6cee6a

remove duplicate sha1 and empty files field

bd45d41

licquia requested changes Mar 15, 2022

View reviewed changes

rammatzkvosky and others added 5 commits March 27, 2022 14:42

updating setup.py version to '0.7.0a3.post5'

3fdd1a4

Signed-off-by: rammatzkvosky <ram.matzkovsky@whitesourcesoftware.com>

Adding condition to validate_checksum() for cases where check_sum.val…

25abc73

…ue is not available at WhiteSource Updating setup.py version to '0.7.0a3.post6' Signed-off-by: rammatzkvosky <ram.matzkovsky@whitesourcesoftware.com>

Added exception for delete check_sum

6a48660

Added exception for delete check_sum

b316c0c

Added exception for delete check_sum

e7d4f51

nicoweidner requested changes Sep 7, 2022

View reviewed changes

nicoweidner added the stale label Oct 20, 2022

nicoweidner mentioned this pull request Nov 2, 2022

Assorted bug fixes #263

Closed

nicoweidner closed this Nov 2, 2022

meretp mentioned this pull request Nov 9, 2022

Assorted bug fixes #270

Merged

-        return True if self.relationship == other.relationship else False
+        return (
+            isInstance(other, Relationship)
+            and self.relationship == other.relationship
+        )

	# create a list of the packages' names for the 'documentDescribes' field
	# create a list of the packages' spdx ids for the 'documentDescribes' field

Conversation

tamari-oz commented Jan 16, 2022

Uh oh!

licquia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicoweidner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

armintaenzertng commented Oct 21, 2022

Uh oh!

nicoweidner commented Nov 2, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants