Skip to content

Read back the excluded files our tag-value writer emits - #906

Open
arpitjain099 wants to merge 1 commit into
spdx:mainfrom
arpitjain099:fix/verification-code-excludes
Open

arpitjain099 wants to merge 1 commit into
spdx:mainfrom
arpitjain099:fix/verification-code-excludes

Conversation

@arpitjain099

Copy link
Copy Markdown

The tag-value writer joins a package verification code's excluded files with a space:

excluded_files_str = " (excludes: " + " ".join(verification_code.excluded_files) + ")"

and the tag-value parser splits the same field on a comma:

excluded_files = match.group(verif_code_exc_files_grp).split(",")

so a document this library writes does not survive being read back by it:

written : PackageVerificationCode: d6a770ba38583ed4bb4525bd96e50461655d2758 (excludes: ./package.spdx ./excluded.txt ./third.bin)
wrote   : ['./package.spdx', './excluded.txt', './third.bin']
read    : ['./package.spdx ./excluded.txt ./third.bin']

Three excluded files become one, whose name contains spaces. A comma separated list has a smaller version of the same problem: (excludes: a.txt, b.txt) parses to ['a.txt', ' b.txt'], with the space kept on every entry after the first.

test_write_tag_value already writes a document and parses it back, and it passes, because package_verification_code_fixture has exactly one excluded file. With one file the space join and the comma split agree, so nothing in the suite covers the case where they do not.

Which side to change

I changed the parser, not the writer. The specification's tag-value form for this field is singular and defines no separator for several files, so neither side is wrong against it (clause 7.9.3, example 1: (excludes: FileName), with the example showing (excludes: ./package.spdx)). The array forms in JSON and RDF carry the list properly and are unaffected. Given that, changing the writer would alter what every other tool reading our output sees, for no gain, while making the parser lenient costs nothing and fixes the round trip.

So the parser now splits on commas and on whitespace, and drops empty fragments, which also takes care of the leading space above. Documents written by this library and documents written with commas both read back correctly.

Worth saying plainly: this does not decide what the canonical tag-value separator should be. If you would rather the writer emit commas, or would rather take it upstream to the spec, I am happy to follow that instead.

Verification

test_write_tag_value_with_several_excluded_files writes a document whose package has three excluded files, parses it back, and asserts both that the list survives and that the documents are equal. test_parse_package_verification_code_excluded_files covers the four spellings directly: space separated, comma separated, comma and space separated, and a single file. Three of those fail on main; the single file case passes either way and is there so a change that breaks what already worked is caught.

pytest tests/ is 1034 passed, 3 skipped, ignoring tests/spdx3/validation/json_ld, which fails to import on main too because pyshacl is not installed in my environment. black --check, isort --check-only and flake8 are clean on the three touched files.

The tag-value writer joins a verification code's excluded files with a space,
while the parser splits the same field on a comma. A document written by this
library and read back by it returns every excluded file glued into one string:

  wrote ['./package.spdx', './excluded.txt', './third.bin']
  read  ['./package.spdx ./excluded.txt ./third.bin']

With one excluded file, which is all the specification shows and all the
fixture carries, the two agree by accident, so test_write_tag_value passes.

The specification defines no separator for several excluded files in tag-value
(clause 7.9.3 example 1 is "(excludes: FileName)", singular), so neither side is
wrong against it and changing the writer would change what other tools read.
Split on commas and on whitespace instead, and drop empty fragments, which also
fixes a comma-and-space separated list returning a leading space on every entry
after the first.

Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
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

Successfully merging this pull request may close these issues.

1 participant