Story 2673: Correct future bug of boost version ordering

libraries/github.py

@@ -80,6 +80,7 @@ def get_commit_data_for_repo_versions(key, min_version=""):
     Get commits from one x.x.0 release to the next x.x.0 release. Commits
     to and from patches or beta versions are ignored.
 
+    min_version is a version name, e.g. boost-1.92.0
     """
     library = Library.objects.get(key=key)
     parser = re.compile(
@@ -96,6 +97,13 @@ def get_commit_data_for_repo_versions(key, min_version=""):
         r"(?:(?P<insertions>\d+) insertions)?.*?(?:(?P<deletions>\d+) deletions)?",
     )
 
+    min_version_re = re.compile(r"^boost-(\d+)\.(\d+)\.(\d+)$")
+    # Tuple in the form of (major, minor, patch)
+    if match := min_version_re.match(min_version):
+        parsed_mv = match.groups()
+    else:
+        parsed_mv = []
+
     retry_count = 0
     with tempfile.TemporaryDirectory() as temp_dir:
         git_dir = Path(temp_dir) / f"{library.key}.git"
@@ -127,15 +135,13 @@ def get_commit_data_for_repo_versions(key, min_version=""):
             + list(
                 Version.objects.minor_versions()
                 .filter(library_version__library__key=library.key)
+                .filter(version_array__gte=parsed_mv)
                 .order_by("version_array")
                 .values_list("name", flat=True)
             )
             + ["master"]
         )
         for a, b in zip(versions, versions[1:]):
-            if a < min_version and b < min_version:
-                # Don't bother comparing two versions we don't care about
-                continue
             shortstat = subprocess.run(
                 ["git", "--git-dir", str(git_dir), "diff", f"{a}..{b}", "--shortstat"],
                 capture_output=True,
@@ -500,12 +506,26 @@ def update_commits(self, library: Library, clean=False, min_version=""):
         """Import a record of all commits between LibraryVersions."""
         authors = {}
         commits = []
+        min_version_re = re.compile(r"^boost-(\d+)\.(\d+)\.(\d+)$")
+        # Tuple in the form of (major, minor, patch)
+        if match := min_version_re.match(min_version):
+            parsed_mv = match.groups()
+        else:
+            parsed_mv = []
         library_versions = {
             x.version.name: x
-            for x in LibraryVersion.objects.filter(
-                library=library, version__name__gte=min_version
-            ).select_related("version")
+            for x in LibraryVersion.objects.with_version_split()
+            .filter(library=library, version_array__gte=parsed_mv)
+            .select_related("version")
         }
+        library_versions.update(
+            {
+                x.version.name: x
+                for x in LibraryVersion.objects.filter(
+                    library=library, version__name__in=["master", "develop"]
+                ).select_related("version")
+            }
+        )
         library_version_updates = []
 
         def handle_commit(commit: ParsedCommit):
@@ -575,12 +595,19 @@ def handle_version_diff_stat(diff: VersionDiffStat):
                 # Unscoped, a run with a floor deletes the whole library and
                 # rebuilds only the top of it, and the commits below the floor
                 # are gone from the table until someone runs a full import.
-                doomed = Commit.objects.filter(
+                doomed = Commit.objects.with_version_split().filter(
+                    library_version__library=library,
+                    version_array__gte=parsed_mv,
+                )
+                doomed_non_standard = Commit.objects.filter(
                     library_version__library=library,
-                    library_version__version__name__gte=min_version,
+                    library_version__version__name__in=["master", "develop"],
+                )
+                doomed_ids = list(doomed.values_list("pk", flat=True)) + list(
+                    doomed_non_standard.values_list("pk", flat=True)
                 )
-                doomed_ids = list(doomed.values_list("pk", flat=True))
                 doomed.delete()
+                doomed_non_standard.delete()
             Commit.objects.bulk_create(
                 commits,
                 update_conflicts=True,

libraries/management/commands/import_library_version_docs_urls.py

@@ -47,10 +47,12 @@ def command(release: str, new: bool, min_version: str):
             processed.
     """
     click.secho("Saving links to version-specific library docs...", fg="green")
+    min_version_parts = [int(part) for part in min_version.split(".")]
     version_qs = (
         Version.objects.with_partials()
         .active()
-        .filter(name__gte=f"boost-{min_version}")
+        .with_version_split()
+        .filter(version_array__gte=min_version_parts)
     )
     if release:
         versions = version_qs.filter(name__icontains=release).order_by("-name")

libraries/managers.py

@@ -3,6 +3,8 @@
 from django.db import models
 from django.db.models import Q, Count
 
+from versions.managers import VersionArrayMixin
+
 from libraries.bots import is_bot_name
 
 
@@ -26,6 +28,22 @@ def get_queryset(self):
         return super().get_queryset().exclude_bots()
 
 
+class LibraryVersionQueryset(VersionArrayMixin):
+    _version_field_name = "version__name"
+    _version_field_beta = "version__beta"
+
+
+LibraryVersionManager = models.Manager.from_queryset(LibraryVersionQueryset)
+
+
+class CommitQueryset(VersionArrayMixin):
+    _version_field_name = "library_version__version__name"
+    _version_field_beta = "library_version__version__beta"
+
+
+CommitManager = models.Manager.from_queryset(CommitQueryset)
+
+
 class IssueQuerySet(models.QuerySet):
     def closed_during_release(self, version, prior_version):
         """Get the issues that were closed during a specific version.

libraries/models.py

@@ -28,6 +28,8 @@
     CommitAuthorManager,
     HumanCommitAuthorManager,
     IssueManager,
+    LibraryVersionManager,
+    CommitManager,
 )
 from mailing_list.models import EmailData
 from mailing_list.tasks import calculate_mailing_list_activity
@@ -446,6 +448,8 @@ def __str__(self):
 
 
 class Commit(models.Model):
+    objects = CommitManager()
+
     author = models.ForeignKey(CommitAuthor, on_delete=models.CASCADE)
     library_version = models.ForeignKey("LibraryVersion", on_delete=models.CASCADE)
     sha = models.CharField(max_length=40)
@@ -764,6 +768,8 @@ class LibraryVersion(models.Model):
         "23": "C++23",
     }
 
+    objects = LibraryVersionManager()
+
     version = models.ForeignKey(
         "versions.Version",
         related_name="library_version",

libraries/tests/test_utils.py

@@ -190,12 +190,46 @@ def test_generate_library_docs_url_string_view():
         ("boost-1.82.0", "boost-1.83.0", "boost-1.85.0", False),
         # Case: Version is above max version
         ("boost-1.86.0", "boost-1.83.0", "boost-1.85.0", False),
+        # Case: Minor version is 10, min is 100
+        ("boost-1.11.0", "boost-1.100.0", "boost-1.120.0", False),
+        # Case: Minor version is 110
+        ("boost-1.110.0", "boost-1.99.0", "boost-1.120.0", True),
+        # Case: Names are slugs
+        ("boost_1_110_0", "boost_1_99_0", "boost_1_120_0", True),
+        # Special cases for master and develop
+        # Case: Develop is min, version is not master
+        ("boost_1_100_0", "develop", None, False),
+        # Case: Develop is min, version is master
+        ("master", "develop", None, True),
+        # Case: Master is min
+        ("boost_1_100_0", "master", None, False),
+        # Case: Develop is min
+        ("boost_1_100_0", "develop", None, False),
+        # Case: Develop is min, version is master
+        ("master", "develop", None, True),
+        # Case: Master is max, version is not master
+        ("boost_1_100_0", None, "master", True),
+        # Case: Master is max, version is not master, but version is less than min
+        ("boost_1_90_0", "boost_1_100_0", "master", False),
+        # Case: Master is max, version is master
+        ("master", None, "master", False),
+        # Case: Develop is max, version is not master or develop
+        ("boost_1_100_0", None, "develop", True),
+        # Case: Develop is max, version is master
+        ("master", None, "develop", False),
+        # Case: master is max, develop is version, and we set a minimum
+        ("develop", "boost_1_90_0", "master", True),
     ],
 )
 def test_version_within_range(version, min_version, max_version, expected):
     assert version_within_range(version, min_version, max_version) == expected
 
 
+def test_mismatched_slug_name_raises_error():
+    with pytest.raises(ValueError):
+        version_within_range("boost-1.110.0", "boost_1_99_0", "boost_1_120_0"),
+
+
 def test_get_first_last_day_last_month():
     first_day, last_day = get_first_last_day_last_month()
 

libraries/utils.py

@@ -4,6 +4,7 @@
 from itertools import islice
 from types import SimpleNamespace
 from typing import TYPE_CHECKING
+from typing import Iterable
 
 import boto3
 import structlog
@@ -181,14 +182,123 @@ def format_duration(seconds: int) -> str:
 def version_within_range(
     version: str, min_version: str = None, max_version: str = None
 ):
-    """Direct string comparison, assuming 'version', 'min_version', and 'max_version'
+    """Parses parts of versions and compares them, assuming 'version', 'min_version', and 'max_version'
     follow the same format.
 
-    Expects format `boost-1.84.0`
+    Expects format `boost-1.84.0` or 'boost_1_84_0' (name or slug)
+
+    Special Cases:
+
+    'develop' and 'master' are also acceptable version strings.
+
+    'master' is newer than 'develop' which is newer than anything else.
     """
-    if min_version and version < min_version:
+
+    SPECIAL_CASES = ("master", "develop")
+
+    def _test_special_case(case_name: str):
+        """
+        Tests the "special" cases of master and develop. Returns three possible outcomes:
+
+        True - the value is definitely in the range, no more evaluation needed
+        False - the value is definitely outside the range, no more evaluation needed
+        None - no conclusion can be drawn from the case, continue evaluation
+        """
+        # nothing is newer than a special case, other than another special case
+        if min_version == case_name:
+            return False
+        # special is newer than everything except itself
+        if max_version == case_name:
+            if version == max_version:
+                return False
+            # covers the case that max = master and version = develop
+            elif version in SPECIAL_CASES and min_version not in SPECIAL_CASES:
+                return True
+            elif not min_version:
+                return True
+        # A version of special case is newer than any min, but outside of any max
+        if version == case_name:
+            if min_version and not max_version and not version == min_version:
+                return True
+            else:
+                return False
+        return None
+
+    if (
+        version in SPECIAL_CASES
+        or max_version in SPECIAL_CASES
+        or min_version in SPECIAL_CASES
+    ):
+        # the logic for both special cases are the same, if we test for master first
+        for case in SPECIAL_CASES:
+            value = _test_special_case(case)
+            if value is not None:
+                return value
+
+        # if a conclusion wasn't drawn, then we have a special case max and a normal min, so set
+        # max to none and perform normal evaluation
+        max_version = None
+
+    # Strip trailing -number from patches
+    version = re.sub("-\d+$", "", version, 1)
+
+    _name_re = re.compile(r"^boost-(\d+)\.(\d+)\.(\d+).?[\d\w]*$")
+    _slug_re = re.compile(r"^boost_(\d+)_(\d+)_(\d+)_?[\d\w]*$")
+
+    def _parse_name(s: str):
+        if parsed_name := _name_re.match(s):
+            if len(parsed_name.groups()) == 3:
+                return parsed_name.groups()
+        return None
+
+    def _parse_slug(s: str):
+        if parsed_slug := _slug_re.match(s):
+            if len(parsed_slug.groups()) == 3:
+                return parsed_slug.groups()
+        return None
+
+    def _parse_values(con_func: callable, version, min_version, max_version):
+        v_parts = max_parts = min_parts = None
+        v_parts = con_func(version)
+        if v_parts:
+            if min_version:
+                min_parts = con_func(min_version)
+                if not min_parts:
+                    """Incorrectly formatted version."""
+                    raise ValueError("Version incorrectly formatted.")
+            if max_version:
+                max_parts = con_func(max_version)
+                if not max_parts:
+                    """Incorrectly formatted version."""
+                    raise ValueError("Version incorrectly formatted.")
+        return v_parts, min_parts, max_parts
+
+    def _compare_parts(less: Iterable, more: Iterable):
+        if len(less) != 3 or len(more) != 3:
+            raise ValueError("Values not made of 3 parts")
+        for a, b in zip(less, more):
+            if int(a) < int(b):
+                return True
+            elif int(a) > int(b):
+                return False
+
+        return False
+
+    v_parts, min_parts, max_parts = _parse_values(
+        _parse_name, version, min_version, max_version
+    )
+    if not v_parts:
+        v_parts, min_parts, max_parts = _parse_values(
+            _parse_slug, version, min_version, max_version
+        )
+
+    if not v_parts:
+        """Incorrectly formatted version."""
+        raise ValueError("Version incorrectly formatted.")
+
+    if min_parts and _compare_parts(v_parts, min_parts):
         return False
-    if max_version and version > max_version:
+    if max_parts and _compare_parts(max_parts, v_parts):
         return False
     return True