From 7f18af55d8ee357313bf49dd4a8208e4da6c5ebf Mon Sep 17 00:00:00 2001 From: Daniel Szoke Date: Thu, 21 Nov 2024 11:52:37 -0500 Subject: [PATCH] fix(debugfile): Recognize chunk-uploaded Proguard files The `detect_dif_from_path` function does not correctly identify Proguard files which are chunk uploaded because these files have a randomly-assigned temporary path, and the logic tries to guess whether the file is a Proguard file based on its path. However, the `detect_dif_from_path` function also takes an optional `name` option, which for chunk-uploaded files, is the file name that is specified by the client making the assemble call. This has not been a problem yet, since Sentry CLI does not support chunk uploading Proguard files, but we are adding support in https://github.com/getsentry/sentry-cli/issues/2196, and so we need to fix the backend to allow chunk uploads of Proguard files. Here, we update the logic to check both the `path` and the `name` for potentially matching a Proguard file. --- src/sentry/models/debugfile.py | 9 ++- tests/sentry/models/test_debugfile.py | 83 ++++++++++++++++++++++++++- 2 files changed, 88 insertions(+), 4 deletions(-) diff --git a/src/sentry/models/debugfile.py b/src/sentry/models/debugfile.py index ea0d64793ac8b..c7572ddccfeb4 100644 --- a/src/sentry/models/debugfile.py +++ b/src/sentry/models/debugfile.py @@ -338,7 +338,10 @@ def create_dif_from_id( return dif, True -def _analyze_progard_filename(filename: str) -> str | None: +def _analyze_progard_filename(filename: str | None) -> str | None: + if filename is None: + return None + match = _proguard_file_re.search(filename) if match is None: return None @@ -474,9 +477,9 @@ def detect_dif_from_path( :raises BadDif: If the file is not a valid DIF. """ - # proguard files (proguard/UUID.txt) or + # Proguard files have a path or a name like (proguard/UUID.txt) or # (proguard/mapping-UUID.txt). - proguard_id = _analyze_progard_filename(path) + proguard_id = _analyze_progard_filename(path) or _analyze_progard_filename(name) if proguard_id is not None: data = {"features": ["mapping"]} return [ diff --git a/tests/sentry/models/test_debugfile.py b/tests/sentry/models/test_debugfile.py index 48d5eca08166d..569351ea06dad 100644 --- a/tests/sentry/models/test_debugfile.py +++ b/tests/sentry/models/test_debugfile.py @@ -6,10 +6,16 @@ from io import BytesIO from typing import Any +import pytest from django.core.files.uploadedfile import SimpleUploadedFile from django.urls import reverse -from sentry.models.debugfile import DifMeta, ProjectDebugFile, create_dif_from_id +from sentry.models.debugfile import ( + DifMeta, + ProjectDebugFile, + create_dif_from_id, + detect_dif_from_path, +) from sentry.models.files.file import File from sentry.testutils.cases import APITestCase, TestCase @@ -298,3 +304,78 @@ def test_simple_cache_clear(self): # But it's gone now assert not os.path.isfile(difs[PROGUARD_UUID]) + + +@pytest.mark.parametrize( + ("path", "name", "should_detect", "uuid"), + ( + ( + "/proguard/mapping-00000000-0000-0000-0000-000000000000.txt", + None, + True, + "00000000-0000-0000-0000-000000000000", + ), + ( + "/proguard/00000000-0000-0000-0000-000000000000.txt", + None, + True, + "00000000-0000-0000-0000-000000000000", + ), + ( + "/var/folders/x5/zw3gnf_x3ts0dwg56362ftrw0000gn/T/tmpbs2r93sr", + "/proguard/mapping-00000000-0000-0000-0000-000000000000.txt", + True, + "00000000-0000-0000-0000-000000000000", + ), + ( + "/var/folders/x5/zw3gnf_x3ts0dwg56362ftrw0000gn/T/tmpbs2r93sr", + "/proguard/00000000-0000-0000-0000-000000000000.txt", + True, + "00000000-0000-0000-0000-000000000000", + ), + ( + "/var/folders/x5/zw3gnf_x3ts0dwg56362ftrw0000gn/T/tmpbs2r93sr", + None, + False, + None, + ), + ( + "/var/folders/x5/zw3gnf_x3ts0dwg56362ftrw0000gn/T/tmpbs2r93sr", + "not-a-proguard-file.txt", + False, + None, + ), + ( + # Note: "/" missing from beginning of path + "proguard/mapping-00000000-0000-0000-0000-000000000000.txt", + None, + False, + None, + ), + ( + "/var/folders/x5/zw3gnf_x3ts0dwg56362ftrw0000gn/T/tmpbs2r93sr", + # Note: "/" missing from beginning of path + "proguard/mapping-00000000-0000-0000-0000-000000000000.txt", + False, + None, + ), + ), +) +def test_proguard_files_detected(path, name, should_detect, uuid): + try: + detected = detect_dif_from_path(path, name) + except FileNotFoundError: + # If the file is not detected as a ProGuard file, detect_dif_from_path + # attempts to open the file, which likely doesn't exist. + # ProGuard files are detected by the path/name, not the file contents. + # So, the ProGuard check should not depend on the file existing. + assert not should_detect + return + + assert len(detected) == bool(should_detect) + + if should_detect: + (dif_meta,) = detected + assert dif_meta.file_format == "proguard" + assert dif_meta.debug_id == uuid + assert dif_meta.data == {"features": ["mapping"]}