Repository navigation
Extend ad-hoc RPM content search with package groups, errata and module streams - #1664
Conversation
…le streams
Add rpm/content-search/package-groups/, errata/ and module-streams/ endpoints,
the no-ContentView-required counterparts to content-views/{pk}/search/rpm/...
Repository versions are passed as repeated repository_version query params.
The query logic for these three searches is extracted into shared functions
(package_group_search, errata_search, module_stream_search) used by both the
ContentView-backed and ad-hoc endpoints, so their behavior stays identical.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reviewer's GuideThis PR extends ad-hoc RPM content search with package groups, errata, and module streams, while refactoring the corresponding Content View search logic into shared implementations to keep behavior consistent across both endpoint families. Sequence diagram for ad-hoc RPM content searchsequenceDiagram
actor User
participant API as RpmContentSearchViewSet
participant Resolver as resolve_repository_versions
participant Search as SharedSearchFunction
participant Serializer as ContentSearchSerializer
User->>API: GET rpm/content-search/{search_type}/?repository_version=...
API->>Resolver: resolve_repository_versions
Resolver-->>API: versions_by_domain
API->>Search: package_group_search / errata_search / module_stream_search
Search-->>API: filtered and merged content
API->>Serializer: serialize results
Serializer-->>API: response data
API-->>User: search results
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 7 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="pulp_service/pulp_service/app/content_view_viewsets.py" line_range="336" />
<code_context>
+ limit = _int_param(request, "limit", default=25, minimum=1, maximum=100)
+
+ def build_queryset(versions):
+ qs = content_for_versions(PackageGroup, versions)
+ if search:
+ qs = qs.filter(name__icontains=search)
</code_context>
<issue_to_address>
**Large searches exhaust worker memory**
When A request includes multiple repository versions in one domain whose combined content-ID sets are large, `content_for_versions` adds every `version.content_ids` value to a Python set before applying the search filters or result limit. Large repository versions therefore make these new searches consume memory proportional to all content in the versions, potentially exhausting the worker before results are returned.
Avoid materializing all content IDs in Python, or otherwise bound the versions/content processed before constructing the search queryset.
Also at `pulp_service/pulp_service/app/content_view_viewsets.py:482`, `pulp_service/pulp_service/app/content_view_viewsets.py:560`.
</issue_to_address>
### Comment 2
<location path="pulp_service/pulp_service/app/content_view_viewsets.py" line_range="570-571" />
<code_context>
+ rows = []
+ for domain, versions in versions_by_domain.items():
+ with with_domain(domain):
+ rows.extend(build_queryset(versions)[:limit])
+ rows.sort(key=lambda m: getattr(m, sort_field), reverse=descending)
+ return rows[:limit]
+
</code_context>
<issue_to_address>
**Descending searches omit highest-ranked streams**
When `sort_by` is descending and a domain has more matching streams than the requested limit, `module_stream_search` slices each domain’s ascending queryset to `limit` before sorting the collected rows descending. The highest-ranked streams are discarded before the sort, so callers receive the wrong results for a descending request.
Apply the requested sort direction to each domain’s queryset before slicing it to the per-domain limit.
</issue_to_address>
### Comment 3
<location path="pulp_service/pulp_service/app/content_search_viewsets.py" line_range="203-204" />
<code_context>
+ def list(self, request, *args, **kwargs):
+ versions_by_domain = self._versions_by_domain(request)
+ page = package_group_search(request, versions_by_domain)
+ serializer = self.get_serializer(page, many=True)
+ return Response({"results": serializer.data})
+
+
</code_context>
<issue_to_address>
**Cross-domain streams lose packages**
When a stream with RPM packages comes from a domain other than the request's domain, `module_stream_search` returns the stream after `with_domain` exits, so `ContentViewModuleStreamSerializer.get_packages` lazily queries packages in the request's domain and returns incomplete or incorrect results.
Load or serialize each stream's packages while its own domain context is active.
</issue_to_address>
### Comment 4
<location path="pulp_service/pulp_service/app/content_view_viewsets.py" line_range="496" />
<code_context>
+ page, total = scatter_gather(
+ versions_by_domain,
+ build_queryset,
+ order_by=(sort_field,),
+ limit=limit,
+ offset=offset,
</code_context>
<issue_to_address>
**Paginated errata repeat or disappear**
When a caller paginates results with tied values for the requested sort field, `errata_search` passes only `sort_field` to `scatter_gather` as `order_by`, leaving tied rows without a stable order across requests; clients receive duplicate or skipped errata.
Add a unique, stable tiebreaker, such as the row ID, to the errata ordering.
</issue_to_address>
### Comment 5
<location path="pulp_service/pulp_service/app/content_view_viewsets.py" line_range="365" />
<code_context>
+ order.append(key)
+ else:
+ merged[key].packages = _union_package_lists(merged[key].packages, group.packages)
+ order.sort(key=lambda key: key[0])
+ return [merged[key] for key in order[:limit]]
+
</code_context>
<issue_to_address>
**Same-name groups change order and selected results**
When more than the result limit of distinct groups share a name and the repository-version input order changes, `package_group_search` sorts merged groups only by name, so equal-name groups retain domain and input iteration order; the ordering and the subset returned after applying the limit change with the supplied repository versions.
Sort merged groups by both name and a stable tiebreaker such as ID.
</issue_to_address>
### Comment 6
<location path="pulp_service/pulp_service/tests/functional/test_rpm_content_search.py" line_range="157-158" />
<code_context>
+ auth=admin_auth,
+ timeout=60,
+ )
+ assert filtered.status_code == 200, filtered.text
+ assert all(erratum["type"] == known_type for erratum in filtered.json()["results"])
+
+ def test_missing_repository_version_param_is_400(self, setup_domain, bindings_cfg, admin_auth):
</code_context>
<issue_to_address>
**Empty results pass the type-filter test**
When the type-filter endpoint returns no results despite matching errata existing, `all()` in the type-filter test returns `True` for an empty results list, so the test passes without detecting that the filter returned no errata.
Assert that the filtered results are nonempty before checking that every erratum has the expected type.
</issue_to_address>
### Comment 7
<location path="pulp_service/pulp_service/tests/functional/test_rpm_content_search.py" line_range="116" />
<code_context>
+ timeout=60,
+ )
+ assert resp.status_code == 200, resp.text
+ assert isinstance(resp.json()["results"], list)
+
+ def test_missing_repository_version_param_is_400(self, setup_domain, bindings_cfg, admin_auth):
</code_context>
<issue_to_address>
**Package-group aggregation goes unchecked**
When the package-group search drops all results, when the package-group search returns an empty list for every request, `test_search_aggregates_across_domains` still passes: line 116 checks only that `results` is a list, so it never detects that aggregation is broken.
Assert that the response contains expected groups and verifies the cross-version aggregation behavior.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 7 findings to address first, and if repository-version resolution or domain scoping is wrong, an authenticated caller could retrieve content from a domain they cannot read. Reverting removes the endpoints, but cannot retract content already returned.
Blocking findings: pulp_service/pulp_service/app/content_view_viewsets.py:336, pulp_service/pulp_service/app/content_view_viewsets.py:571, pulp_service/pulp_service/app/content_search_viewsets.py:204, pulp_service/pulp_service/app/content_view_viewsets.py:496, pulp_service/pulp_service/app/content_view_viewsets.py:365, and 2 more
| limit = _int_param(request, "limit", default=25, minimum=1, maximum=100) | ||
|
|
||
| def build_queryset(versions): | ||
| qs = content_for_versions(PackageGroup, versions) |
There was a problem hiding this comment.
🟡 Medium · Large searches exhaust worker memory
When A request includes multiple repository versions in one domain whose combined content-ID sets are large, content_for_versions adds every version.content_ids value to a Python set before applying the search filters or result limit. Large repository versions therefore make these new searches consume memory proportional to all content in the versions, potentially exhausting the worker before results are returned.
Avoid materializing all content IDs in Python, or otherwise bound the versions/content processed before constructing the search queryset.
Also at pulp_service/pulp_service/app/content_view_viewsets.py:482, pulp_service/pulp_service/app/content_view_viewsets.py:560.
Prompt for AI agents
In `pulp_service/pulp_service/app/content_view_viewsets.py` at line 336:
**Large searches exhaust worker memory**
When A request includes multiple repository versions in one domain whose combined content-ID sets are large, `content_for_versions` adds every `version.content_ids` value to a Python set before applying the search filters or result limit. Large repository versions therefore make these new searches consume memory proportional to all content in the versions, potentially exhausting the worker before results are returned.
Avoid materializing all content IDs in Python, or otherwise bound the versions/content processed before constructing the search queryset.
Also at `pulp_service/pulp_service/app/content_view_viewsets.py:482`, `pulp_service/pulp_service/app/content_view_viewsets.py:560`.| rows.extend(build_queryset(versions)[:limit]) | ||
| rows.sort(key=lambda m: getattr(m, sort_field), reverse=descending) |
There was a problem hiding this comment.
🟠 High · Descending searches omit highest-ranked streams
When sort_by is descending and a domain has more matching streams than the requested limit, module_stream_search slices each domain’s ascending queryset to limit before sorting the collected rows descending. The highest-ranked streams are discarded before the sort, so callers receive the wrong results for a descending request.
Apply the requested sort direction to each domain’s queryset before slicing it to the per-domain limit.
Prompt for AI agents
In `pulp_service/pulp_service/app/content_view_viewsets.py` at lines 570-571:
**Descending searches omit highest-ranked streams**
When `sort_by` is descending and a domain has more matching streams than the requested limit, `module_stream_search` slices each domain’s ascending queryset to `limit` before sorting the collected rows descending. The highest-ranked streams are discarded before the sort, so callers receive the wrong results for a descending request.
Apply the requested sort direction to each domain’s queryset before slicing it to the per-domain limit.| serializer = self.get_serializer(page, many=True) | ||
| return Response({"results": serializer.data}) |
There was a problem hiding this comment.
🟡 Medium · Cross-domain streams lose packages
When a stream with RPM packages comes from a domain other than the request's domain, module_stream_search returns the stream after with_domain exits, so ContentViewModuleStreamSerializer.get_packages lazily queries packages in the request's domain and returns incomplete or incorrect results.
Load or serialize each stream's packages while its own domain context is active.
Prompt for AI agents
In `pulp_service/pulp_service/app/content_search_viewsets.py` at lines 203-204:
**Cross-domain streams lose packages**
When a stream with RPM packages comes from a domain other than the request's domain, `module_stream_search` returns the stream after `with_domain` exits, so `ContentViewModuleStreamSerializer.get_packages` lazily queries packages in the request's domain and returns incomplete or incorrect results.
Load or serialize each stream's packages while its own domain context is active.| page, total = scatter_gather( | ||
| versions_by_domain, | ||
| build_queryset, | ||
| order_by=(sort_field,), |
There was a problem hiding this comment.
🟡 Medium · Paginated errata repeat or disappear
When a caller paginates results with tied values for the requested sort field, errata_search passes only sort_field to scatter_gather as order_by, leaving tied rows without a stable order across requests; clients receive duplicate or skipped errata.
Add a unique, stable tiebreaker, such as the row ID, to the errata ordering.
Prompt for AI agents
In `pulp_service/pulp_service/app/content_view_viewsets.py` at line 496:
**Paginated errata repeat or disappear**
When a caller paginates results with tied values for the requested sort field, `errata_search` passes only `sort_field` to `scatter_gather` as `order_by`, leaving tied rows without a stable order across requests; clients receive duplicate or skipped errata.
Add a unique, stable tiebreaker, such as the row ID, to the errata ordering.| order.append(key) | ||
| else: | ||
| merged[key].packages = _union_package_lists(merged[key].packages, group.packages) | ||
| order.sort(key=lambda key: key[0]) |
There was a problem hiding this comment.
🟡 Medium · Same-name groups change order and selected results
When more than the result limit of distinct groups share a name and the repository-version input order changes, package_group_search sorts merged groups only by name, so equal-name groups retain domain and input iteration order; the ordering and the subset returned after applying the limit change with the supplied repository versions.
Sort merged groups by both name and a stable tiebreaker such as ID.
Prompt for AI agents
In `pulp_service/pulp_service/app/content_view_viewsets.py` at line 365:
**Same-name groups change order and selected results**
When more than the result limit of distinct groups share a name and the repository-version input order changes, `package_group_search` sorts merged groups only by name, so equal-name groups retain domain and input iteration order; the ordering and the subset returned after applying the limit change with the supplied repository versions.
Sort merged groups by both name and a stable tiebreaker such as ID.| assert filtered.status_code == 200, filtered.text | ||
| assert all(erratum["type"] == known_type for erratum in filtered.json()["results"]) |
There was a problem hiding this comment.
🟡 Medium · Empty results pass the type-filter test
When the type-filter endpoint returns no results despite matching errata existing, all() in the type-filter test returns True for an empty results list, so the test passes without detecting that the filter returned no errata.
Assert that the filtered results are nonempty before checking that every erratum has the expected type.
Prompt for AI agents
In `pulp_service/pulp_service/tests/functional/test_rpm_content_search.py` at lines 157-158:
**Empty results pass the type-filter test**
When the type-filter endpoint returns no results despite matching errata existing, `all()` in the type-filter test returns `True` for an empty results list, so the test passes without detecting that the filter returned no errata.
Assert that the filtered results are nonempty before checking that every erratum has the expected type.| timeout=60, | ||
| ) | ||
| assert resp.status_code == 200, resp.text | ||
| assert isinstance(resp.json()["results"], list) |
There was a problem hiding this comment.
🟡 Medium · Package-group aggregation goes unchecked
When the package-group search drops all results, when the package-group search returns an empty list for every request, test_search_aggregates_across_domains still passes: line 116 checks only that results is a list, so it never detects that aggregation is broken.
Assert that the response contains expected groups and verifies the cross-version aggregation behavior.
Prompt for AI agents
In `pulp_service/pulp_service/tests/functional/test_rpm_content_search.py` at line 116:
**Package-group aggregation goes unchecked**
When the package-group search drops all results, when the package-group search returns an empty list for every request, `test_search_aggregates_across_domains` still passes: line 116 checks only that `results` is a list, so it never detects that aggregation is broken.
Assert that the response contains expected groups and verifies the cross-version aggregation behavior.
|
/retest |
|
All PipelineRuns for this commit have already succeeded. Use |
Summary
Extends the ad-hoc
rpm/content-search/endpoints (added in #1559) with three new search types, each matching the query behavior of itscontent-views/{pk}/search/rpm/...counterpart. Repository versions are passed directly as repeatedrepository_versionquery params — no persisted ContentView required.rpm/content-search/package-groups/packageslists of same(name, id)groups across repo versions are unionedrpm/content-search/errata/type/severityfilters, sort, pagination, CVE references includedrpm/content-search/module-streams/rpm_namesfilter, sort, cappedChanges
content_search_viewsets.py, inheriting auth/RBAC fromRpmContentSearchViewSet(inaccessible domains are silently excluded).package_group_search,errata_search,module_stream_search) plus shared OpenAPI params, so both endpoint families behave identically. No behavior change to the existing ContentView endpoints.docs/rpm-content-search.md, changelog fragment.Testing
ruff checkpasses on changed files.setup_domainfixture repo has no modules.🤖 Generated with Claude Code
Summary by Sourcery
Extend ad-hoc RPM content search to cover package groups, errata, and module streams without requiring a persisted Content View.
New Features:
Enhancements:
Documentation:
Tests:
Chores: