diff --git a/src/launchpad/api/update_api_models.py b/src/launchpad/api/update_api_models.py index facc378f..205520c6 100644 --- a/src/launchpad/api/update_api_models.py +++ b/src/launchpad/api/update_api_models.py @@ -65,6 +65,9 @@ class UpdateData(BaseModel): app_id: str build_version: str build_number: Optional[int] + build_number_raw: Optional[str] = Field( + None, description="Unparsed build identifier (e.g. raw CFBundleVersion) for display purposes" + ) artifact_type: int apple_app_info: Optional[AppleAppInfo] = None android_app_info: Optional[AndroidAppInfo] = None diff --git a/src/launchpad/artifact_processor.py b/src/launchpad/artifact_processor.py index aa301441..c54b726d 100644 --- a/src/launchpad/artifact_processor.py +++ b/src/launchpad/artifact_processor.py @@ -570,7 +570,7 @@ def _prepare_update_data( dequeued_at: datetime, app_icon_id: str | None, ) -> dict[str, Any]: - build_number = int(app_info.build) if app_info.build.isdigit() else None + build_number = _parse_build_number(app_info.build) apple_app_info = None if isinstance(app_info, AppleAppInfo): @@ -602,6 +602,7 @@ def _prepare_update_data( app_id=app_info.app_id, build_version=app_info.version, build_number=build_number, + build_number_raw=app_info.build, artifact_type=_get_artifact_type(artifact).value, apple_app_info=apple_app_info, android_app_info=android_app_info, @@ -662,3 +663,40 @@ def _guess_message(code: ProcessingErrorCode, e: Exception) -> ProcessingErrorMe elif code == ProcessingErrorCode.ARTIFACT_PROCESSING_TIMEOUT: return ProcessingErrorMessage.PROCESSING_TIMEOUT return ProcessingErrorMessage.UNKNOWN_ERROR + + +# Zero-padding width per CFBundleVersion component when packing it into a single +# sortable int. 6 digits comfortably covers any realistic CI build counter while +# leaving the packed value well under BoundedBigIntegerField's max (bigint). +_BUILD_NUMBER_COMPONENT_WIDTH = 6 + + +def _parse_build_number(build: str) -> int | None: + """Parse a raw build identifier (e.g. CFBundleVersion) into a sortable int. + + Plain integer builds (the common case, e.g. Android versionCode) pass through + unchanged. Apple's CFBundleVersion also allows up to three dot-separated + non-negative integers (e.g. "1.2.3"); those are packed into a single int by + zero-padding each component, which preserves correct ordering as long as no + component reaches 10**_BUILD_NUMBER_COMPONENT_WIDTH. Anything else + (non-numeric, malformed) returns None, same as today. + + Known limitation: plain-integer values are left small while packed dotted + values are much larger, so if the same app_id/build_version ever has builds + in both conventions, this int alone is not a reliable ordering between them. + We can't fix that here — a single artifact update has no visibility into + sibling artifacts' formats, and unifying the magnitude would break backward + compatibility with every already-stored plain-integer build_number. Sentry's + tiebreak query should treat this as a fast, common-case sort key and fall + back to comparing the raw build string (build_number_raw) directly when it + needs to be authoritative across mixed formats. + """ + if build.isdigit(): + return int(build) + + parts = build.split(".") + if 2 <= len(parts) <= 3 and all(p.isdigit() and len(p) <= _BUILD_NUMBER_COMPONENT_WIDTH for p in parts): + parts += ["0"] * (3 - len(parts)) + return sum(int(part) * 10 ** (_BUILD_NUMBER_COMPONENT_WIDTH * (2 - i)) for i, part in enumerate(parts)) + + return None diff --git a/tests/unit/artifacts/test_artifact_processor.py b/tests/unit/artifacts/test_artifact_processor.py index 4276d371..8e7aa42d 100644 --- a/tests/unit/artifacts/test_artifact_processor.py +++ b/tests/unit/artifacts/test_artifact_processor.py @@ -1,8 +1,12 @@ +from datetime import datetime from unittest.mock import Mock, patch +import pytest + from objectstore_client import Client as ObjectstoreClient -from launchpad.artifact_processor import ArtifactProcessor, ServiceConfig +from launchpad.artifact_processor import ArtifactProcessor, ServiceConfig, _parse_build_number +from launchpad.artifacts.android.aab import AAB from launchpad.artifacts.apple.zipped_xcarchive import ZippedXCArchive from launchpad.artifacts.artifact import Artifact from launchpad.constants import ( @@ -11,6 +15,7 @@ ProcessingErrorMessage, ) from launchpad.sentry_client import SentryClient, SentryClientError +from launchpad.size.models.android import AndroidAppInfo from launchpad.utils.objectstore import ObjectstoreConfig from launchpad.utils.statsd import FakeStatsd @@ -398,3 +403,85 @@ def test_process_message_project_not_skipped(self, mock_process, mock_sentry_cli "increment", {"metric": "artifact.processing.completed", "value": 1, "tags": None}, ) in calls + + +class TestParseBuildNumber: + @pytest.mark.parametrize( + "build,expected", + [ + # Plain integers (e.g. Android versionCode) pass through unchanged + ("9999", 9999), + ("0", 0), + ("1", 1), + # Apple CFBundleVersion: up to three dot-separated non-negative integers + ("1.2.3", 1_000_002_000_003), + ("1.2", 1_000_002_000_000), + # Malformed or unsupported shapes fall back to None, same as before + ("1.2.a", None), + ("abc", None), + ("1.2.3.4", None), + ("", None), + # A component too wide for the padding width is refused rather than + # silently corrupting the ordering of adjacent components + ("1234567.2.3", None), + ], + ) + def test_parse_build_number(self, build, expected): + assert _parse_build_number(build) == expected + + def test_dotted_builds_sort_correctly_within_a_version(self): + # Naive "strip the dots and parse as one number" concatenation would rank + # 1.99 ("199") above 2.0 ("20"), even though 1.99 is the earlier build. + assert _parse_build_number("1.99") < _parse_build_number("2.0") + + def test_distinguishes_builds_that_naive_concatenation_would_collide(self): + # "1.2.3", "12.3", and "1.23" would all naively concatenate to "123". + assert _parse_build_number("1.2.3") != _parse_build_number("12.3") + assert _parse_build_number("1.2.3") != _parse_build_number("1.23") + + +class TestPrepareUpdateData: + def setup_method(self): + """Set up test fixtures.""" + mock_sentry_client = Mock(spec=SentryClient) + mock_statsd = Mock() + mock_objectstore_client = Mock(spec=ObjectstoreClient) + self.processor = ArtifactProcessor(mock_sentry_client, mock_statsd, mock_objectstore_client) + + def test_dotted_build_number_is_parsed_and_raw_value_preserved(self): + app_info = AndroidAppInfo( + name="My App", + version="1.2.300", + build="1.2.3", + app_id="com.example.app", + has_proguard_mapping=False, + ) + + update_data = self.processor._prepare_update_data( + app_info=app_info, + artifact=Mock(spec=AAB), + dequeued_at=datetime(2026, 1, 1), + app_icon_id=None, + ) + + assert update_data["build_number"] == 1_000_002_000_003 + assert update_data["build_number_raw"] == "1.2.3" + + def test_plain_build_number_still_parses_as_int(self): + app_info = AndroidAppInfo( + name="My App", + version="1.2.300", + build="9999", + app_id="com.example.app", + has_proguard_mapping=False, + ) + + update_data = self.processor._prepare_update_data( + app_info=app_info, + artifact=Mock(spec=AAB), + dequeued_at=datetime(2026, 1, 1), + app_icon_id=None, + ) + + assert update_data["build_number"] == 9999 + assert update_data["build_number_raw"] == "9999"