SPDX-2.2 validation #490
SPDX-2.2 validation #490
Conversation
Signed-off-by: Armin Tänzer <[email protected]>
Signed-off-by: Armin Tänzer <[email protected]>
Signed-off-by: Armin Tänzer <[email protected]> [issue-463] fix file validation tests Signed-off-by: Armin Tänzer <[email protected]>
Signed-off-by: Armin Tänzer <[email protected]> [issue-463] fix snippet validation tests Signed-off-by: Armin Tänzer <[email protected]>
Signed-off-by: Armin Tänzer <[email protected]>
Signed-off-by: Armin Tänzer <[email protected]>
Signed-off-by: Armin Tänzer <[email protected]>
Signed-off-by: Armin Tänzer <[email protected]>
Signed-off-by: Armin Tänzer <[email protected]>
There was a problem hiding this comment.
Thanks for the additions! I have only four small remarks and two more general:
With the version-specific validation I think it would make sense to think about a class-based implementation again. With that you could instantiate a Validator class with a specific version instead of passing the version every time as an argument. With more different versions, one could then further think about a base Validator and version-specific Validators as child classes. However, since there is not much more to add here and the argument of the additional parameter passing is also not that strong for now, this change is not really necessary. Maybe in a future refactoring.
Another remark concerning the cli-tool: I don't like the fact that a document with version "SPDX-2.1" is marked as invalid. I would prefer to catch the not supported versions earlier in that case. It makes sense to also have this validation in validate_full_spdx_document for library users but for the cli we should add some additional logic.
| ValidationMessage( | ||
| f"{checksum.algorithm.name} is not supported in SPDX-2.2", context) | ||
| ) | ||
|
|
There was a problem hiding this comment.
I think we should return here as the algorithm is not valid.
| @@ -48,6 +48,6 @@ def validate_creation_info(creation_info: CreationInfo) -> List[ValidationMessag | |||
|
|
|||
| validation_messages.extend(validate_actors(creation_info.creators, creation_info.spdx_id)) | |||
|
|
|||
| validation_messages.extend(validate_external_document_refs(creation_info.external_document_refs, creation_info.spdx_id)) | |||
| validation_messages.extend(validate_external_document_refs(creation_info.external_document_refs, creation_info.spdx_id, spdx_version)) | |||
There was a problem hiding this comment.
Auto formatting inserts a line break here.
| if spdx_version == "SPDX-2.2" and reference_type in ["advisory", "fix", "url", "swid"]: | ||
| validation_messages.append( | ||
| ValidationMessage(f'externalPackageRef type "{reference_type}" is not supported in SPDX-2.2', context) | ||
| ) | ||
|
|
There was a problem hiding this comment.
I would move this validation to the beginning and return. I don't think that it makes sense to first validate references on a lower level and afterwards check if they are valid in general.
| (creation_info_fixture(spdx_version="SPDX2.3"), "SPDX2.3", | ||
| 'the document\'s spdx_version must be of the form "SPDX-[major].[minor]" but is: SPDX2.3'), | ||
| 'only SPDX versions "SPDX-2.2" and "SPDX-2.3" are supported, but the document\'s spdx_version is: SPDX2.3'), |
There was a problem hiding this comment.
You could add one more testcase for a document with version "SPDX-2.1"
Signed-off-by: Armin Tänzer <[email protected]>
Signed-off-by: Armin Tänzer <[email protected]>
Signed-off-by: Armin Tänzer <[email protected]>
Signed-off-by: Armin Tänzer <[email protected]>
fixes #463