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.

Sign up for GitHub

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

Jump to bottom

[Issue 359] Add Json writer for new data model #379

Merged

armintaenzertng merged 55 commits into spdx:refactor-python-tools from nicoweidner:issue-359

Jan 3, 2023

Collaborator

nicoweidner commented Dec 21, 2022

Fixes #359

Signed-off-by: Nicolaus Weidner nicolaus.weidner@tngtech.com

nicoweidner force-pushed the issue-359 branch 3 times, most recently from e68d842 to 9045c5b Compare

December 21, 2022 22:28

meretp reviewed

View reviewed changes

Collaborator

meretp left a comment

Overall I think it's a nice structure for the writer- eventhough I have to admit that I needed some time to get the bigger picture and to understand the functionality of the convertor and the role of the properties. Here are some comments from a first look.

src/datetime_conversions.py Outdated Show resolved Hide resolved

tests/writer/json/test_json_writer.py Outdated Show resolved Hide resolved

tests/writer/__init__.py Outdated Show resolved Hide resolved

tests/test_datetime_conversions.py Outdated Show resolved Hide resolved

tests/test_datetime_conversions.py Outdated Show resolved Hide resolved

src/jsonschema/document_properties.py Outdated Show resolved Hide resolved

armintaenzertng reviewed

View reviewed changes

Collaborator

armintaenzertng left a comment

Very thorough implementation that takes a while to get used to but seems very OK :)

src/datetime_conversions.py Outdated Show resolved Hide resolved

src/jsonschema/document_converter.py Outdated Show resolved Hide resolved

src/jsonschema/converter.py Show resolved Hide resolved

src/jsonschema/relationship_filters.py Outdated Show resolved Hide resolved

src/jsonschema/optional_utils.py Outdated Show resolved Hide resolved

src/jsonschema/document_converter.py

+                          element_ids = [file.spdx_id for file in document.files]
+                          element_ids.extend([package.spdx_id for package in document.packages])
+                          element_ids.extend([snippet.spdx_id for snippet in document.snippets])
+                          document_annotations = filter(lambda annotation: annotation.spdx_id not in element_ids,

Collaborator

armintaenzertng Dec 29, 2022

This will add annotations with external spdx_ids to the document.

I don't know how to properly write them out, though, as json does not support annotations with SPDX id references. Should we just discard them?

Collaborator Author

nicoweidner Dec 30, 2022

Hm, good question. Dropping them feels bad. But I don't see any solution that avoids losing information.

@goneall : The situation is that a SPDX document contains an annotation, where the annotated element is an external element. Where should the annotation go in the serialized json? The json schema puts annotations directly on the annotated element. This works fine for packages, files, snippets and the document itself, but there is no place for annotations targeting external elements.

tests/jsonschema/test_external_document_ref_converter.py Outdated Show resolved Hide resolved

tests/jsonschema/test_relationship_converter.py Show resolved Hide resolved

tests/fixtures.py Show resolved Hide resolved

src/writer/json/json_writer.py Outdated Show resolved Hide resolved

meretp mentioned this pull request

Issue 381 add tag value writer #395

Closed

armintaenzertng reviewed

View reviewed changes

src/jsonschema/checksum_converter.py

+              def algorithm_to_json_string(algorithm: ChecksumAlgorithm) -> str:
+                  name_with_dash: str = algorithm.name.replace("_", "-")
+                  if "BLAKE2B" in name_with_dash:
+                      return name_with_dash.replace("BLAKE2B", "BLAKE2b")
+                  return name_with_dash

Collaborator

armintaenzertng Dec 29, 2022

This is needed in the tag-value writer, too

Collaborator Author

nicoweidner Dec 30, 2022

But that isn't merged yet, right? Then once this PR is merged, the other one can be rebased and can extract it

Collaborator

armintaenzertng commented Dec 30, 2022

I discussed with @maxhbr that we want users to only write valid documents per default. This means that a validation step should be included in the writers that has to be explicitly toggled off if the user does not want that.

armintaenzertng reviewed

View reviewed changes

src/jsonschema/package_converter.py Show resolved Hide resolved

armintaenzertng mentioned this pull request

[issue-381] Add tag-value writer #396

Merged

nicoweidner force-pushed the issue-359 branch 2 times, most recently from 8ac17fb to 26fe138 Compare

December 30, 2022 20:34

Collaborator Author

nicoweidner commented Dec 30, 2022

I discussed with @maxhbr that we want users to only write valid documents per default. This means that a validation step should be included in the writers that has to be explicitly toggled off if the user does not want that.

There is now a validate parameter which defaults to True

nicoweidner force-pushed the issue-359 branch from 0359001 to 26fe138 Compare

December 30, 2022 20:53

nicoweidner added 6 commits

January 2, 2023 15:34


          [bugfix] Fix snippet copyright text type

b750cdf

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359] Recover datetime utils from git history, add tests

72145b0

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359][squash] add license header

6b892fd

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359] Add Json property conversion setup for CreationInfo prope…

e2b06dd

…rties

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359] Remove logic from json property enums (will be included i…

f84ef3f

…n separate converters), refactor document_properties.py to include all document properties instead of just top-level ones

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359] Add abstract base converter class and checksum converter

cb395c5

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>

nicoweidner force-pushed the issue-359 branch from 048bbaa to f31cbcb Compare

January 2, 2023 14:40

armintaenzertng reviewed

View reviewed changes

tests/writer/json/test_json_writer.py Show resolved Hide resolved

tests/jsonschema/__init__.py Outdated Show resolved Hide resolved

src/jsonschema/converter.py Show resolved Hide resolved

armintaenzertng reviewed

View reviewed changes

src/jsonschema/converter.py Outdated Show resolved Hide resolved

nicoweidner added 7 commits

January 2, 2023 23:39


          [issue-359][squash] Remove license header from empty __init__.py

92c183e

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359] Add CreationInfo converter

aca25d1

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359] Add external document ref converter

dad3227

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359][squash] Rename variable

5f625b9

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359] Enable passing the full document to converters to account…

1804c21

… for (upcoming) more complex conversion cases

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359] Add annotation properties and converter

70a99bc

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359] Add external package ref properties and converter

a33766b

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>

nicoweidner added 26 commits

January 2, 2023 23:39


          [issue-359] Extract external package ref fixture for reuse in tests

d640c58

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359] Extract snippet fixture for reuse in tests

1aabe99

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359] Add SpdxNoAssertion and SpdxNone tests

0adbb75

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359] Don't write empty lists into the converted dictionary

c430a0d

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359] Add json writer

976287d

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359][squash] extract variables in test_json_writer.py

23894d7

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359] Make conversion test assertions more precise by excluding…

35d235e

… any additional properties

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359] Add more test fixtures

000cd80

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359] Add utility assertion function for mocks

fba6e21

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359] Add tests for annotation writing logic

410dd34

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359] Add relationship fixture to test fixtures

fbf6e57

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359] Add document_describes and has_files tests

f9997da

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359] Add tests for relationship conversion logic in DocumentCo…

fe7b0bd

…nverter

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359] Fix mock ordering in converter tests

c2b9ac0

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359] Make mock utilities compatible with Python 3.7

7aa1b74

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359] Remove license headers from empty init files

4aac32c

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359] Move relationship_filters to model package

030b1fc

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359] Add NOASSERTION case to extracted license name

b371f4c

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359] Add default implementation of json_property_name to conve…

f511d02

…rter

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359] Allow NOASSERTION and NONE for related element id in conv…

57b70af

…erter

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359] Remove JsonWriter class, convert to plain function

4727af7

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359] Add validation to json writer, with the possibility of di…

6c2d577

…sabling it

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359] Add docstrings to some core files

03fca1d

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359][squash] Fix docstring

3f0f111

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359] Extract common method to get all spdx element ids inside …

ebae51a

…a document

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>


          [issue-359] Use extracted relationship filtering methods in document_…

a0a804b

…validator.py

Signed-off-by: Nicolaus Weidner <nicolaus.weidner@tngtech.com>

nicoweidner force-pushed the issue-359 branch from 1adffef to a0a804b Compare

January 2, 2023 22:55

armintaenzertng approved these changes

View reviewed changes

Collaborator

armintaenzertng left a comment

Thank you very much, looks very good now :)

armintaenzertng merged commit 48c4e72 into spdx:refactor-python-tools

armintaenzertng mentioned this pull request

Refactor JSON writer for new data model #359

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet