From 0c815d19f70dbc9ec16af23bb21c4d30bd128e64 Mon Sep 17 00:00:00 2001 From: Dalai Felinto Date: Tue, 23 Jul 2024 12:56:58 +0200 Subject: [PATCH 1/4] Use a stricter semantic version check for some version fields This applies to: * blender_version_min * blender_version_max * schema_version The only field which still takes a full semantic version is the `version` field itself. See related discussion: https://projects.blender.org/blender/blender/issues/124885 --- extensions/tests/test_manifest.py | 57 ++++++++++++++++++++++++++----- files/validators.py | 45 ++++++++++++++++++++++-- 2 files changed, 90 insertions(+), 12 deletions(-) diff --git a/extensions/tests/test_manifest.py b/extensions/tests/test_manifest.py index e3bd23e4..33715835 100644 --- a/extensions/tests/test_manifest.py +++ b/extensions/tests/test_manifest.py @@ -400,11 +400,8 @@ class ValidateManifestFields(TestCase): data['blender_version_min'] = '2.9' with self.assertRaises(ValidationError) as e: ManifestValidator(data) - self.assertEqual(1, len(e.exception.messages)) - self.assertIn('blender_version_min', e.exception.messages[0]) - self.assertIn('4.2.0', e.exception.messages[0]) data['blender_version_min'] = '4.1.0' with self.assertRaises(ValidationError) as e: @@ -419,12 +416,8 @@ class ValidateManifestFields(TestCase): data['blender_version_min'] = '4.2.0-beta' with self.assertRaises(ValidationError) as e: ManifestValidator(data) - self.assertEqual( - e.exception.messages, - [ - 'Manifest value error: blender_version_min should be at least "4.2.0"', - ], - ) + self.assertIn('blender_version_min', e.exception.messages[0]) + self.assertIn('should follow', e.exception.messages[0]) def test_blender_version_max(self): data = { @@ -443,6 +436,52 @@ class ValidateManifestFields(TestCase): self.assertEqual(1, len(e.exception.messages)) self.assertIn('blender_version_max', e.exception.messages[0]) + def test_non_semver_versions(self): + data = { + **self.mandatory_fields, + **self.optional_fields, + } + + for field, value in ( + ('blender_version_min', '4.2'), + ('blender_version_min', '4.2.1+alpha'), + ('blender_version_min', '5'), + ('blender_version_max', '4.3'), + ('blender_version_max', '4.3.0+alpha'), + ('blender_version_max', '5'), + ('schema_version', '1.0'), + ('schema_version', '1'), + ('schema_version', '1.0.0.0'), + ): + value_original = data[field] + data[field] = value + + with self.assertRaises(ValidationError) as e: + ManifestValidator(data) + self.assertEqual(1, len(e.exception.messages)) + self.assertIn(field, e.exception.messages[0]) + + data[field] = value_original + + def test_version(self): + data = { + **self.mandatory_fields, + **self.optional_fields, + } + + data['version'] = '1.2.0' + ManifestValidator(data) + + data['version'] = '1.2.0+alpha' + ManifestValidator(data) + + for bad_version in ['1.2', '1.2.0.0', '1']: + data['version'] = bad_version + with self.assertRaises(ValidationError) as e: + ManifestValidator(data) + self.assertEqual(1, len(e.exception.messages)) + self.assertIn('version', e.exception.messages[0]) + def test_licenses(self): data = { **self.mandatory_fields, diff --git a/files/validators.py b/files/validators.py index 73056ce3..8c0a4f6d 100644 --- a/files/validators.py +++ b/files/validators.py @@ -536,7 +536,46 @@ class VersionValidator: ) -class SchemaVersionValidator(VersionValidator): +class NonSemVerVersionValidator(VersionValidator): + """These version are a sub-set of the semantic version + + They only support major.minor.patch. + """ + + @staticmethod + def is_version_string(version): + import re + + # Regex from: + # https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string + + # Define the regex pattern for major.minor.patch (do not include any additional label). + pattern = r'^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)' + + # Make sure we match the entire string with the pattern. + return bool(re.fullmatch(pattern, version)) + + @classmethod + def validate(cls, *, name: str, value: str, manifest: dict) -> str: + """Return error message if cannot validate, otherwise returns nothing""" + err_message = mark_safe( + f'Manifest value error: {name} should follow a ' + f'semantic version ' + f'with only major.minor.patch ' + f'e.g., "{cls.example}"' + ) + + # Play safe and first guarantee that the string can be validated as a Version type + # which is expected throghout the rest of the code. + if super().validate(name=name, value=value, manifest=manifest): + return err_message + + # Make sure ew only have major.minor.patch. + if not cls.is_version_string(version=value): + return err_message + + +class SchemaVersionValidator(NonSemVerVersionValidator): example = '1.0.0' @classmethod @@ -558,7 +597,7 @@ class SchemaVersionValidator(VersionValidator): ) -class VersionMinValidator(VersionValidator): +class VersionMinValidator(NonSemVerVersionValidator): example = '4.2.0' @classmethod @@ -574,7 +613,7 @@ class VersionMinValidator(VersionValidator): ) -class VersionMaxValidator(VersionValidator): +class VersionMaxValidator(NonSemVerVersionValidator): example = '4.3.0' @classmethod -- 2.30.2 From 60ebb72795d9aa91d5868b724a70cafae27b1ba6 Mon Sep 17 00:00:00 2001 From: Dalai Felinto Date: Tue, 23 Jul 2024 14:10:15 +0200 Subject: [PATCH 2/4] From review: minor changes --- files/validators.py | 32 ++++++++++++-------------------- 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/files/validators.py b/files/validators.py index 8c0a4f6d..057786ec 100644 --- a/files/validators.py +++ b/files/validators.py @@ -1,5 +1,6 @@ from semantic_version import Version import logging +import re from django.core.exceptions import ValidationError from django.core.validators import URLValidator, validate_unicode_slug @@ -536,7 +537,7 @@ class VersionValidator: ) -class NonSemVerVersionValidator(VersionValidator): +class SemVerNoLabelValidator(VersionValidator): """These version are a sub-set of the semantic version They only support major.minor.patch. @@ -544,8 +545,6 @@ class NonSemVerVersionValidator(VersionValidator): @staticmethod def is_version_string(version): - import re - # Regex from: # https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string @@ -558,24 +557,17 @@ class NonSemVerVersionValidator(VersionValidator): @classmethod def validate(cls, *, name: str, value: str, manifest: dict) -> str: """Return error message if cannot validate, otherwise returns nothing""" - err_message = mark_safe( - f'Manifest value error: {name} should follow a ' - f'semantic version ' - f'with only major.minor.patch ' - f'e.g., "{cls.example}"' - ) - - # Play safe and first guarantee that the string can be validated as a Version type - # which is expected throghout the rest of the code. - if super().validate(name=name, value=value, manifest=manifest): - return err_message - - # Make sure ew only have major.minor.patch. + # Make sure we only have major.minor.patch. if not cls.is_version_string(version=value): - return err_message + return mark_safe( + f'Manifest value error: {name} should follow a ' + f'semantic version ' + f'with only major.minor.patch ' + f'e.g., "{cls.example}"' + ) -class SchemaVersionValidator(NonSemVerVersionValidator): +class SchemaVersionValidator(SemVerNoLabelValidator): example = '1.0.0' @classmethod @@ -597,7 +589,7 @@ class SchemaVersionValidator(NonSemVerVersionValidator): ) -class VersionMinValidator(NonSemVerVersionValidator): +class VersionMinValidator(SemVerNoLabelValidator): example = '4.2.0' @classmethod @@ -613,7 +605,7 @@ class VersionMinValidator(NonSemVerVersionValidator): ) -class VersionMaxValidator(NonSemVerVersionValidator): +class VersionMaxValidator(SemVerNoLabelValidator): example = '4.3.0' @classmethod -- 2.30.2 From dcfdfdb0bfa4ae1bfb6748773f2e11e69ff02a6a Mon Sep 17 00:00:00 2001 From: Dalai Felinto Date: Tue, 23 Jul 2024 14:23:31 +0200 Subject: [PATCH 3/4] From review: pre-compile regex and regex tweaks --- files/validators.py | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/files/validators.py b/files/validators.py index 057786ec..64a8b6a8 100644 --- a/files/validators.py +++ b/files/validators.py @@ -21,6 +21,10 @@ import files.models logger = logging.getLogger(__name__) +# Define the regex pattern for major.minor.patch (do not include any additional label). +# https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string +version_regex_pattern = re.compile(r'^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)$') + @deconstructible class FileMIMETypeValidator: @@ -537,7 +541,7 @@ class VersionValidator: ) -class SemVerNoLabelValidator(VersionValidator): +class SemVerNoLabelValidator(StringValidator): """These version are a sub-set of the semantic version They only support major.minor.patch. @@ -545,26 +549,24 @@ class SemVerNoLabelValidator(VersionValidator): @staticmethod def is_version_string(version): - # Regex from: - # https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string - - # Define the regex pattern for major.minor.patch (do not include any additional label). - pattern = r'^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)' - - # Make sure we match the entire string with the pattern. - return bool(re.fullmatch(pattern, version)) + return version_regex_pattern.match(version) @classmethod def validate(cls, *, name: str, value: str, manifest: dict) -> str: """Return error message if cannot validate, otherwise returns nothing""" + err_message = mark_safe( + f'Manifest value error: {name} should follow a ' + f'semantic version ' + f'with only major.minor.patch ' + f'e.g., "{cls.example}"' + ) + + if super().validate(name=name, value=value, manifest=manifest): + return err_message + # Make sure we only have major.minor.patch. if not cls.is_version_string(version=value): - return mark_safe( - f'Manifest value error: {name} should follow a ' - f'semantic version ' - f'with only major.minor.patch ' - f'e.g., "{cls.example}"' - ) + return err_message class SchemaVersionValidator(SemVerNoLabelValidator): -- 2.30.2 From 41907b0b2d8a9bdcbc608ef408b1f5978cb045c1 Mon Sep 17 00:00:00 2001 From: Dalai Felinto Date: Tue, 23 Jul 2024 14:32:14 +0200 Subject: [PATCH 4/4] Simplify version_max logic --- files/validators.py | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/files/validators.py b/files/validators.py index 64a8b6a8..d1431aff 100644 --- a/files/validators.py +++ b/files/validators.py @@ -624,15 +624,7 @@ class VersionMaxValidator(SemVerNoLabelValidator): # assuming that VersionMinValidator has caught this and reported properly return - try: - max = Version(value) - except (ValueError, TypeError): - return mark_safe( - f'Manifest value error: {name} should follow a ' - f'semantic version. ' - f'e.g., "{cls.example}"' - ) - + max = Version(value) if max <= min: return mark_safe( 'Manifest value error: blender_version_max should be greater than ' -- 2.30.2