feat(preprod): Create new multiproject list builds endpoint #105395
Conversation
github-actions
bot
added
the
Scope: Backend
This stack of pull requests is managed by Graphite. Learn more about stacking. |
github-actions
bot
added
the
Scope: Frontend
|
🚨 Warning: This pull request contains Frontend and Backend changes! It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently. Have questions? Please ask in the |
|
|
||
| List preprod builds for an organization with optional filtering by projects and pagination. | ||
|
|
||
| :pparam string organization_id_or_slug: the id or slug of the organization the |
There was a problem hiding this comment.
Mind putting required/optional in these for each param? I noticed Sentry as a whole doesn't do it generally but I personally find it quite useful when looking at an endpoint
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| except InvalidParams: | ||
| raise ParseError(detail="Invalid date range") | ||
|
|
||
| queryset = PreprodArtifact.objects.filter(project_id__in=project_ids) |
There was a problem hiding this comment.
Missing select_related causes N+1 queries for project access
The queryset at line 92 filters PreprodArtifact objects without select_related('project'), but the transform_preprod_artifact_to_build_details function accesses artifact.project.id and artifact.project.slug. This causes an additional database query for each artifact in the results. With the default pagination of 25 items (max 100), this means 25-100 extra queries per request. Other endpoints in this codebase that access artifact.project.* use select_related("project") to avoid this issue.
Additional Locations (1)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #105395 +/- ##
========================================
Coverage 80.58% 80.58%
========================================
Files 9441 9442 +1
Lines 404928 405022 +94
Branches 25682 25682
========================================
+ Hits 326296 326395 +99
+ Misses 78163 78158 -5
Partials 469 469 |
|
|
||
| # Get projects with permission checking | ||
| req_proj_ids = self.get_requested_project_ids_unchecked(request) | ||
| projects = self.get_projects(request, organization, project_ids=req_proj_ids) |
There was a problem hiding this comment.
Looking at the definition of get_projects, I think it will auto parse them from the request and use that to filter (organization.py:406) if not provided. That means I think you can skip the req_proj_ids parsing
|
|
||
| project_ids = [p.id for p in projects] | ||
|
|
||
| # Record analytics once for the first project |
There was a problem hiding this comment.
Would it be better to record for all and modify PreprodArtifactApiListBuildsEvent to take an array for project_id and we can just set the array for existing invocations to be a single item array?
| ) | ||
| ) | ||
|
|
||
| if not features.has( |
There was a problem hiding this comment.
Might be better to check this earlier so we don't do work unless the user's opted in
| PreprodArtifact.ArtifactType.AAB, | ||
| PreprodArtifact.ArtifactType.APK, | ||
| ] | ||
| ) |
There was a problem hiding this comment.
Might be good to throw an error here if some unsupported platform comes along (or just error logging)
| queryset = queryset.order_by("-date_added") | ||
|
|
||
| if start and end: | ||
| queryset = queryset.filter(date_added__gte=start, date_added__lte=end) |
There was a problem hiding this comment.
Feels weird to have this at the bottom, maybe put up top when we parse start/end? Unless it has a good reason to be down here, then that might warrant a comment IMO
| ] | ||
| ) | ||
|
|
||
| queryset = queryset.order_by("-date_added") |
There was a problem hiding this comment.
Any reason this is down here? Might be best to just add this to the object creation above.
|
|
||
| release_version_parsed = False | ||
| release_version = params.get("release_version") | ||
| if release_version: |
There was a problem hiding this comment.
Feels a bit cleaner to just remove the release_version_parsed and add an else block
release_version = params.get("release_version")
if release_version:
parsed_version = parse_release_version(release_version)
if parsed_version:
queryset = queryset.filter(
app_id__icontains=parsed_version.app_id,
build_version__icontains=parsed_version.build_version,
)
release_version_parsed = True
else:
app_id = params.get("app_id")
if app_id:
queryset = queryset.filter(app_id__exact=app_id)
build_version = params.get("build_version")
if build_version:
queryset = queryset.filter(build_version__icontains=build_version)
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.