Skip to content

Conversation

@chromy
Copy link
Contributor

@chromy chromy commented Jan 9, 2026

Add support the following properties:

  • download_count
  • build_version
  • build_number
  • download_size
  • install_size

Also add support for:

  • numeric fields (download_count, download_size, install_size)
  • 'size' type fields (download_size, install_size)
  • and join results with the 'main artifact' size.

Finally prevent a 500 from build tags endpoint when passed an invalid field.

@chromy chromy requested a review from a team as a code owner January 9, 2026 12:44
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jan 9, 2026
Comment on lines +40 to +50
# is defined later in this file
from sentry.preprod.models import PreprodArtifactSizeMetrics

main_metrics = PreprodArtifactSizeMetrics.objects.filter(
preprod_artifact=OuterRef("pk"),
metrics_artifact_type=PreprodArtifactSizeMetrics.MetricsArtifactType.MAIN_ARTIFACT,
)
return self.annotate(
install_size=Subquery(main_metrics.values("max_install_size")[:1]),
download_size=Subquery(main_metrics.values("max_download_size")[:1]),
)

This comment was marked as outdated.

{"detail": ERR_FEATURE_REQUIRED.format("organizations:preprod-frontend-routes")},
status=400,
)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tag values endpoint allows annotation-based keys that fail

Medium Severity

The BuildTagKeyValuesEndpoint validates keys against search_config.allowed_keys, which now includes download_count, download_size, and install_size. These are annotations created by annotate_download_count() and annotate_main_size_metrics(), but this endpoint doesn't add those annotations before querying. When users request tag values for these keys, the query will fail because queryset.values(db_key) references fields that don't exist on the model without the annotations.

Fix in Cursor Fix in Web

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Copy link
Contributor

@runningcode runningcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

# Key mappings for user-friendly names
key_mappings={
"app_id": ["app_id", "package_name", "bundle_id"],
"app_id": ["package_name", "bundle_id"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we not need to map app_id since it already app_id ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep!

assert len(response.json()) == 1
assert response.json()[0]["app_info"]["app_id"] == "foo"

@with_feature("organizations:preprod-frontend-routes")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are only tests for download count, not the other routes, but i assume they all work the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added one for install size which tests the size bit. It's true it might be good to test all of the fields at least once, I'll fix in a follow up.

queryset = queryset.values(db_key)
queryset = queryset.exclude(**{f"{db_key}__isnull": True})
queryset = queryset.annotate_download_count()
queryset = queryset.annotate_main_size_metrics()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The annotate_main_size_metrics() call occurs after the queryset is grouped with .values(db_key), causing the Subquery with OuterRef("pk") to fail as the primary key is unavailable.
Severity: HIGH

🔍 Detailed Analysis

In BuildTagKeyValuesEndpoint.get(), the queryset is grouped by a specific key using .values(db_key) before annotate_main_size_metrics() is called. This annotation method uses a Subquery with OuterRef("pk") to fetch size metrics. However, after the grouping operation, the primary key of individual artifacts is no longer present in the queryset's context. As a result, when the API is queried for install_size or download_size tag values, the subquery will fail to resolve, leading to NULL or incorrect data being returned for those fields.

💡 Suggested Fix

Move the call to annotate_main_size_metrics() to before the queryset is grouped by .values(db_key). Alternatively, refactor the annotation to use aggregate functions that are compatible with grouped querysets, similar to how annotate_download_count() is implemented.

🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/sentry/preprod/api/endpoints/builds.py#L253

Potential issue: In `BuildTagKeyValuesEndpoint.get()`, the queryset is grouped by a
specific key using `.values(db_key)` before `annotate_main_size_metrics()` is called.
This annotation method uses a `Subquery` with `OuterRef("pk")` to fetch size metrics.
However, after the grouping operation, the primary key of individual artifacts is no
longer present in the queryset's context. As a result, when the API is queried for
`install_size` or `download_size` tag values, the subquery will fail to resolve, leading
to `NULL` or incorrect data being returned for those fields.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 8404362

@chromy chromy merged commit a93d06c into master Jan 9, 2026
66 checks passed
@chromy chromy deleted the chromy/2026-01-09-numeric branch January 9, 2026 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants