Skip to content

Commit

Permalink
fix(debugfile): Recognize chunk-uploaded Proguard files
Browse files Browse the repository at this point in the history
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 getsentry/sentry-cli#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.
  • Loading branch information
szokeasaurusrex committed Nov 21, 2024
1 parent ebae966 commit 6fc3398
Show file tree
Hide file tree
Showing 2 changed files with 91 additions and 4 deletions.
9 changes: 6 additions & 3 deletions src/sentry/models/debugfile.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 [
Expand Down
86 changes: 85 additions & 1 deletion tests/sentry/models/test_debugfile.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -298,3 +304,81 @@ def test_simple_cache_clear(self):

# But it's gone now
assert not os.path.isfile(difs[PROGUARD_UUID])


@pytest.mark.parametrize(
("path", "name", "uuid"),
(
(
"/proguard/mapping-00000000-0000-0000-0000-000000000000.txt",
None,
"00000000-0000-0000-0000-000000000000",
),
(
"/proguard/00000000-0000-0000-0000-000000000000.txt",
None,
"00000000-0000-0000-0000-000000000000",
),
(
"/var/folders/x5/zw3gnf_x3ts0dwg56362ftrw0000gn/T/tmpbs2r93sr",
"/proguard/mapping-00000000-0000-0000-0000-000000000000.txt",
"00000000-0000-0000-0000-000000000000",
),
(
"/var/folders/x5/zw3gnf_x3ts0dwg56362ftrw0000gn/T/tmpbs2r93sr",
"/proguard/00000000-0000-0000-0000-000000000000.txt",
"00000000-0000-0000-0000-000000000000",
),
),
)
def test_proguard_files_detected(path, name, uuid):
# ProGuard files are detected by the path/name, not the file contents.
# So, the ProGuard check should not depend on the file existing.
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) == 1

(dif_meta,) = detected
assert dif_meta.file_format == "proguard"
assert dif_meta.debug_id == uuid
assert dif_meta.data == {"features": ["mapping"]}


@pytest.mark.parametrize(
("path", "name"),
("/var/folders/x5/zw3gnf_x3ts0dwg56362ftrw0000gn/T/tmpbs2r93sr", None),
("/var/folders/x5/zw3gnf_x3ts0dwg56362ftrw0000gn/T/tmpbs2r93sr", "not-a-proguard-file.txt"),
(
# Note: "/" missing from beginning of path
"proguard/mapping-00000000-0000-0000-0000-000000000000.txt",
None,
),
(
"/var/folders/x5/zw3gnf_x3ts0dwg56362ftrw0000gn/T/tmpbs2r93sr",
# Note: "/" missing from beginning of path
"proguard/mapping-00000000-0000-0000-0000-000000000000.txt",
),
)
def test_proguard_file_not_detected(path, name):
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.
# We handle this by setting detected to an empty list, which will
# cause the test to pass.
detected = []

# On the off chance that the file exists and it was detected as a DIF,
# it should never be detected as a ProGuard file, since ProGuard files
# are detected by the path/name, not the file contents.
for dif_meta in detected:
assert dif_meta.file_format != "proguard"

0 comments on commit 6fc3398

Please sign in to comment.