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

json/yaml/xml writer lists all packages in "documentDescribes" #242

Closed
meretp opened this issue Oct 18, 2022 · 9 comments
Closed

json/yaml/xml writer lists all packages in "documentDescribes" #242

meretp opened this issue Oct 18, 2022 · 9 comments
Labels
writer Issues concerning the writing layer

Comments

@meretp
Copy link
Collaborator

meretp commented Oct 18, 2022

In the current implementation all packages are written to the "documentDescribes"-Tag. According to the JSON-example, packages should be listed in a tag "packages" and "documentDescribes" should only contain the SPDXIDs from the objects that the document describes.

@nicoweidner nicoweidner added the writer Issues concerning the writing layer label Oct 20, 2022
@jasinner
Copy link

jasinner commented Oct 24, 2022

Isn't this a duplicate of #184 ? Although this is a better description I think because it correctly points out the need to list all SPDXIDs in the documentDescribes tag, which is only mentioned in the comments of 184.

@jasinner
Copy link

Actually I'm starting to wonder what is the correct use of documentDescribes. In the examples directory for the spec documentDescribes seems to only include some files and packages. Only those which are directly related to the SPDX-Document object in the relationships? There are other packages and files present in the respective packages and files lists which are not listed in documentDescribes. Those seem to be children of the 'top-level packages and files.

@goneall
Copy link
Member

goneall commented Oct 24, 2022

Actually I'm starting to wonder what is the correct use of documentDescribes.

I just went back and looked through the spec and I didn't see a very good definition or description. Probably something that needs to be addressed.

From some of the SPDX meetings I've been involved in, documentDescribes is intended to be the primary packages and/or files the document is intended to document (or describe). This gives the consumer of the SPDX document a starting point(s) for traversing the graph of relationships. In many cases documentDescribes will point to a single package as the root of a tree of relationships. Other files/packages/snippets within the documents will have some direct or indirect relationship to the described package.

@nicoweidner
Copy link
Collaborator

I definitely agree that there is no clear description anywhere (as is the case for all specialties of the json format).

My understanding, on a working level, was that: In json (and yaml and xml), all relationships of type SPDX-Document DESCRIBES SPDX-Element (where element can be a package, file or snippet) are omitted from the relationships list, and the corresponding spdx ids (of the elements) are instead listed in documentDescribes.

Do you agree with that statement, @goneall ? This is coming up in various PRs, so it would be super helpful to have at least a semi-official statement on how it should work 😅

@nicoweidner
Copy link
Collaborator

nicoweidner commented Oct 25, 2022

Just looked at some examples in tools-java: The 2.2 json example has duplicated relationships, i.e.: The elements which are listed in documentDescribes appear again as DESCRIBES relationships: https://github.com/spdx/tools-java/blob/92e1c5d29eacb0081139b0c05e5e6270b231788c/testResources/SPDXJSONExample-v2.2.spdx.json#L245-L256
In the 2.3 json example, this duplication does not appear.

@goneall Are you aware of any changes between the two versions, or is this unintentional?

edit: Same pattern in the examples in spdx-spec (they may be identical)

edit2: More soft evidence: When using tools-java to "convert" the 2.2 json example mentioned above into json, the DESCRIBES relationships vanish. For reference, this is the command I used:

$ java -jar tools-java-1.1.2-SNAPSHOT-jar-with-dependencies.jar Convert ../testResources/SPDXJSONExample-v2.2.spdx.json output.json

@goneall
Copy link
Member

goneall commented Oct 25, 2022

Are you aware of any changes between the two versions, or is this unintentional?

This was an intentional change - they were related to the discussion in this issue: spdx/spdx-spec#704

@nicoweidner
Copy link
Collaborator

We discussed the following strategy for dealing with documentDescribes:

Parsing:
Read both the relationships in the input document as well as the contents of documentDescribes. In the latter case, only add a relationship if no identical relationship exists already (i.e.: Handle the case where a document contains both the relationship as well as the documentDescribes entry).

Writing:
If a relationship of type SPDX-doc DESCRIBES SPDX-element is present and does not have a comment, replace it by the corresponding id entry in documentDescribes. If the relationship has a comment, however, add both the documentDescribes entry and the relationship.

In the weird case of a SPDX-doc DESCRIBES SPDX-element with a comment, there is no great solution, and this seemed a reasonable one. The relationship cannot be dropped without losing the comment, and not writing a documentDescribes entry would greatly diminish the value of that field.
@goneall @maxhbr Happy to hear comments if you disagree.

@goneall
Copy link
Member

goneall commented Oct 27, 2022

@nicoweidner Agree with the approach - it is unlikely we're run into relationship comments on a document describes, but if we do you are already handling the duplications on read.

An alternative approach would be to add the describes relationship comment to an Annotation at the document level to preserve the comment - but I think your proposal is a better approach than using the annotations.

@meretp
Copy link
Collaborator Author

meretp commented Nov 7, 2022

Fixed with #254

@meretp meretp closed this as completed Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
writer Issues concerning the writing layer
Projects
None yet
Development

No branches or pull requests

4 participants